Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 0 · Low: 1
- [High] Incomplete fix: the bug class the commit message names (a
hardware-stripped 802.1ad S-Tag being handed to the stack as…
- [Low] The in-kernel selftest stmmac_test_dvlanfilt()
(drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:1069-1076)…
--
net: stmmac: Disable S-Tag processing on dwmac4
This restricts NETIF_F_HW_VLAN_STAG_RX, NETIF_F_HW_VLAN_STAG_FILTER and
priv->hw->hw_svlan_en to DWMAC_CORE_XGMAC, on the grounds that dwmac4
cannot report which tag type the hardware stripped. The stated goal is
that 802.1ad tags are then left in place and handled by the software VLAN
path.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index c81c5bb5b075..eab2903a66e4 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7952,15 +7952,20 @@ static int __stmmac_dvr_probe(struct device *device,
> ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
> #ifdef STMMAC_VLAN_TAG_USED
> /* Both mac100 and gmac support receive VLAN tag detection */
> - ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
> - priv->hw->hw_svlan_en = true;
> + ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
> + if (priv->plat->core_type == DWMAC_CORE_XGMAC) {
> + ndev->features |= NETIF_F_HW_VLAN_STAG_RX;
> + priv->hw->hw_svlan_en = true;
> + }
[Severity: High]
Does this leave the misreported tag protocol reachable on dwxgmac2?
The commit message describes the bug as vlan_rx_hw() hardcoding
ETH_P_8021Q, but vlan_rx_hw() is unchanged and is still the .rx_hw_vlan
callback in dwxgmac210_vlan_ops:
drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c:vlan_rx_hw() {
if (hw->desc->get_rx_vlan_valid(rx_desc)) {
u16 vid = hw->desc->get_rx_vlan_tci(rx_desc);
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), vid);
}
}
On DWMAC_CORE_XGMAC hw_svlan_en stays true here, and dwmac_is_xmac()
below sets hw_vlan_en, so vlan_set_hw_mode() still programs the MAC to
recognise and strip S-Tags:
drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c:vlan_set_hw_mode() {
if (hw->hw_svlan_en)
value |= VLAN_ESVL | VLAN_DOVLTC;
...
if (hw->hw_vlan_en)
/* Always strip VLAN on Receive */
value |= VLAN_TAG_STRIP_ALL;
}
And dwxgmac2_wrback_get_rx_vlan_valid() collapses the ET_LT information
the commit message points at into a boolean, so vlan_rx_hw() has no way
to tell an S-Tag from a C-Tag:
drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_wrback_get_rx_vlan_valid()
{
return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG;
}
With that, doesn't a received 802.1ad frame on a dwxgmac2 interface with
default features still reach the stack with skb->vlan_proto ==
ETH_P_8021Q, so the 802.1ad VLAN device never sees it and an 802.1Q
sub-interface or bridge VLAN with the same VID does? That is the same
QinQ symptom the commit says it fixes.
Would it make sense to either derive the protocol in vlan_rx_hw() from
ET_LT, or state in the commit message that the dwxgmac2 side is handled
by a follow-up?
A second question on the gate itself: the DWXLGMAC entry in
drivers/net/ethernet/stmicro/stmmac/hwif.c also uses
core_type = DWMAC_CORE_XGMAC, but dwxlgmac2_vlan_ops has no .rx_hw_vlan
callback at all:
drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c {
const struct stmmac_vlan_ops dwxlgmac2_vlan_ops = {
.update_vlan_hash = dwxgmac2_update_vlan_hash,
.enable_vlan = vlan_enable,
.set_hw_vlan_mode = dwxlgmac2_set_hw_vlan_mode,
};
}
Should the check be narrower than core_type == DWMAC_CORE_XGMAC so
NETIF_F_HW_VLAN_STAG_RX is not advertised on DWXLGMAC?
> if (dwmac_is_xmac(priv->plat->core_type)) {
> ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
> priv->hw->hw_vlan_en = true;
> }
> if (priv->dma_cap.vlhash) {
> ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
> - ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
> + if (priv->plat->core_type == DWMAC_CORE_XGMAC)
> + ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
> }
[Severity: Low]
Should the selftest gate be updated to match this?
stmmac_test_dvlanfilt() gates on priv->dma_cap.vlhash, while its peer
stmmac_test_dvlanfilt_perfect() gates on the feature bit:
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c {
static int stmmac_test_dvlanfilt(struct stmmac_priv *priv)
{
if (!priv->dma_cap.vlhash)
return -EOPNOTSUPP;
...
}
static int stmmac_test_dvlanfilt_perfect(struct stmmac_priv *priv)
{
...
if (!(priv->dev->features & NETIF_F_HW_VLAN_STAG_FILTER))
return -EOPNOTSUPP;
...
}
}
Before this change both gates were equivalent, since STAG_FILTER was set
whenever vlhash was set. On a dwmac4 core with vlhash, does
stmmac_test_dvlanfilt() now still run while exercising no hardware? The
vlan_vid_add(priv->dev, htons(ETH_P_8021AD), ...) in
__stmmac_test_dvlanfilt() returns success without reaching
ndo_vlan_rx_add_vid():
net/8021q/vlan_core.c:vlan_add_rx_filter_info() {
if (!vlan_hw_filter_capable(dev, proto))
return 0;
}
so no VLAN hash or perfect filter entry is programmed and the frames are
validated through the software VLAN path instead. Would ethtool -t then
report the double-VLAN hash filter test as passing on a core where the
feature is no longer advertised, while the perfect-filter test is
correctly skipped?
> if (priv->dma_cap.vlins)
> ndev->features |= NETIF_F_HW_VLAN_CTAG_TX;
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908164309.59282-1-ovidiu.panait.rb%40renesas.com
