Re: [PATCH net v4 2/7] net: stmmac: Stop toggling the EDVLP bit
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-10 23:46:00
Also in:
linux-kselftest, lkml, netdev
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 - [Medium] The in-kernel selftests "VLAN Filtering (perf)" (stmmac_test_vlanfilt_perfect,… Pre-existing issues: - [Medium] XLGMAC Rx VLAN offload state is never programmed into the MAC: dwxlgmac2_set_hw_vlan_mode()… -- net: stmmac: Stop toggling the EDVLP bit The EDVLP bit is no longer toggled per 802.1ad VLAN registration. It is now set once from vlan_set_hw_mode() and kept enabled, and a new set_hw_vlan_mode callback is added for dwxlgmac2, which previously had none.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c index 983a90cb9767..200b34588c7f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -222,6 +219,7 @@ static void vlan_set_hw_mode(struct mac_device_info *hw) void __iomem *ioaddr = hw->pcsr; u32 value = readl(ioaddr + VLAN_TAG); + value |= VLAN_EDVLP; value &= ~VLAN_TAG_CTRL_EVLS_MASK; if (hw->hw_vlan_en)
[ ... ]
quoted hunk ↗ jump to hunk
@@ -283,6 +278,14 @@ static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash, } } +static void dwxlgmac2_set_hw_vlan_mode(struct mac_device_info *hw) +{ + void __iomem *ioaddr = hw->pcsr; + u32 value = readl(ioaddr + VLAN_TAG); + + writel(value | VLAN_EDVLP, ioaddr + VLAN_TAG); +} +
[Severity: Medium]
This isn't a bug introduced by this patch, but now that dwxlgmac2 gains a
set_hw_vlan_mode callback, should it also honour hw->hw_vlan_en?
The other implementation of the same ops slot, vlan_set_hw_mode(), programs
the Rx strip mode and the descriptor report bit:
value &= ~VLAN_TAG_CTRL_EVLS_MASK;
if (hw->hw_vlan_en)
value |= VLAN_TAG_STRIP_ALL;
else
value |= VLAN_TAG_STRIP_NONE;
value |= VLAN_TAG_CTRL_EVLRXS;
dwxlgmac2_set_hw_vlan_mode() only ORs VLAN_EDVLP, so EVLS and EVLRXS keep
whatever value they had.
At the same time stmmac_dvr_probe() forces the flag on and advertises the
offload for every core matched by dwmac_is_xmac(), which includes the
XLGMAC hwif entry (.core_type = DWMAC_CORE_XGMAC, .vlan =
&dwxlgmac2_vlan_ops):
if (dwmac_is_xmac(priv->plat->core_type)) {
ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
priv->hw->hw_vlan_en = true;
}
So on XLGMAC, ethtool -K <if> rxvlan on|off goes stmmac_set_features() ->
stmmac_set_hw_vlan_mode() -> dwxlgmac2_set_hw_vlan_mode(), flips
priv->hw->hw_vlan_en and returns success, while no strip or report bit is
written to MAC_VLAN_Tag.
The Rx path then takes the MAC level branch in stmmac_rx():
if (priv->hw->hw_vlan_en)
/* MAC level stripping. */
stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
else
/* Driver level stripping. */
stmmac_rx_vlan(priv->dev, skb);
and dwxlgmac2_vlan_ops has no .rx_hw_vlan handler, so that call resolves to
nothing.
To be clear, this is not packet or tag loss: with EVLS unprogrammed the tag
stays in-band and skb_vlan_untag() in the core Rx path still de-tags the
frame. The point is that driver bookkeeping and the actual MAC VLAN
configuration diverge, and the advertised NETIF_F_HW_VLAN_CTAG_RX does
nothing on this core. This is still the case at the end of the series
(bb70e68e6ebc): dwxlgmac2_vlan_ops has no .rx_hw_vlan and
dwxlgmac2_set_hw_vlan_mode() never programs EVLS/EVLRXS.
quoted hunk ↗ jump to hunk
const struct stmmac_vlan_ops dwmac_vlan_ops = { .update_vlan_hash = vlan_update_hash, .enable_vlan = vlan_enable,@@ -296,6 +299,7 @@ const struct stmmac_vlan_ops dwmac_vlan_ops = { 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, };
A separate question, about the preceding patch in the series rather than this one: [Severity: Medium] After "net: stmmac: Remove VLAN perfect matching dead code", do the two "(perf)" selftests still exercise a different path? stmmac_test_vlanfilt_perfect() and stmmac_test_dvlanfilt_perfect() in stmmac_selftests.c select the perfect matching path only by clearing the capability: priv->dma_cap.vlhash = 0; ret = __stmmac_test_vlanfilt(priv); priv->dma_cap.vlhash = prev_cap; but stmmac_vlan_update() no longer consults priv->dma_cap.vlhash: if (!netif_running(priv->dev)) return 0; return stmmac_update_vlan_hash(priv, priv->hw, hash); and vlan_update_hash()/dwxgmac2_update_vlan_hash() no longer have the perfect_match branch. That makes the override inert, so hash filtering (VTHM/VTFE) stays enabled during the "(perf)" runs and the entries named "VLAN Filtering (perf)" and "Double VLAN Filter (perf)" in stmmac_selftests[] run the same path as their non-perf counterparts. A broken perfect match filter, that is vlan_add_hw_rx_fltr() and the VLAN_TAG_DATA table, would be masked by the still active hash filter while ethtool -t reports these tests as passing. Both tests and their names are unchanged at the end of the series (bb70e68e6ebc). Should these two tests be removed, or reworked to actually disable hash filtering? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908164309.59282-1-ovidiu.panait.rb%40renesas.com