Re: [PATCH net v4 3/7] net: stmmac: Rework S-VLAN handling

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 1 · Low: 0

- [High] Unconditional S-VLAN enablement: `__stmmac_dvr_probe()` sets
  `priv->hw->hw_svlan_en = true` for every core…
- [Medium] The in-kernel stmmac selftests named "VLAN Filtering (perf)"
  and "Double VLAN Filter (perf)" no longer exercise perfect (VID-…

--

net: stmmac: Rework S-VLAN handling

This patch removes the is_double / num_double_vlans tracking from the
stmmac VLAN paths and replaces it with a new mac_device_info flag,
hw_svlan_en, which decides whether the ESVL and DOVLTC bits are set. The
bit programming moves out of the vlan hash update callbacks and into
vlan_set_hw_mode() and dwxlgmac2_set_hw_vlan_mode().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 24d64cce1d87..cd9671493b41 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6813,7 +6813,7 @@ static u32 stmmac_vid_crc32_le(__le16 vid_le)
>  	return crc;
>  }
>  
> -static int stmmac_vlan_update(struct stmmac_priv *priv, bool is_double)
> +static int stmmac_vlan_update(struct stmmac_priv *priv)
>  {
>  	u32 crc, hash = 0;
>  	u16 vid = 0;
> @@ -6827,7 +6827,7 @@ static int stmmac_vlan_update(struct stmmac_priv *priv, bool is_double)
>  	if (!netif_running(priv->dev))
>  		return 0;
>  
> -	return stmmac_update_vlan_hash(priv, priv->hw, hash, is_double);
> +	return stmmac_update_vlan_hash(priv, priv->hw, hash);
>  }

[Severity: Medium]

With the pmatch argument and the !priv->dma_cap.vlhash branch now gone from
stmmac_vlan_update() (removed by the earlier series commit "net: stmmac:
Remove VLAN perfect matching dead code"), do the selftests reported as
"VLAN Filtering (perf)" and "Double VLAN Filter (perf)" still exercise
perfect matching?

stmmac_test_vlanfilt_perfect() and stmmac_test_dvlanfilt_perfect() select
the perfect-match implementation by forcing the capability to zero:

drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:stmmac_test_vlanfilt_perfect() {
	priv->dma_cap.vlhash = 0;
	ret = __stmmac_test_vlanfilt(priv);
	priv->dma_cap.vlhash = prev_cap;
}

stmmac_vlan_update() no longer reads dma_cap.vlhash, so it always computes a
non-zero hash, and vlan_update_hash() / dwxgmac2_update_vlan_hash() then
program VLAN_HASH_TABLE and set VLAN_VTHM | VLAN_ETV regardless. Do both
"perf" tests now run the same hardware configuration as their hash
counterparts, so they can pass while the perfect MAC_VLAN_Tag_Filter
admission path is broken?

stmmac_selftests.c is unchanged at the end of the series (bb70e68e6ebc).
Should the tests be updated, or the reported names adjusted?

[ ... ]

> @@ -7962,6 +7946,7 @@ static int __stmmac_dvr_probe(struct device *device,
>  #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;
>  	if (dwmac_is_xmac(priv->plat->core_type)) {
>  		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
>  		priv->hw->hw_vlan_en = true;

[Severity: High]

hw_svlan_en is set here for every core, unlike hw_vlan_en just below which
is limited to xmac cores. Together with the move of the ESVL and DOVLTC
programming into vlan_set_hw_mode(), does this change when those two bits
are asserted?

Before this patch they were only set while an 802.1ad VID was registered
(is_double / num_double_vlans in vlan_update_hash() and
dwxgmac2_update_vlan_hash()) and were explicitly cleared otherwise. Now
they are programmed on every open and resume through
stmmac_hw_setup()->stmmac_set_hw_vlan_mode()->vlan_set_hw_mode(), and on
every feature change through stmmac_set_features().

With ESVL set the MAC treats 0x88A8 as a valid VLAN tag, and the same
register write selects VLAN_TAG_STRIP_ALL when hw_vlan_en is set, so
S-tags get stripped in hardware for all traffic. The receive path then
reinserts the TCI with a hardcoded protocol:

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);
	}
}

Does an 802.1ad frame therefore reach the stack labelled 802.1Q and get
demuxed onto the C-VLAN device with the same VID, while 802.1ad VLAN
interfaces stop receiving? The descriptor already carries the tag type -
dwxgmac2_wrback_get_rx_vlan_valid() decodes XGMAC_RDES3_ET_LT and
distinguishes S-tag from 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;
}

Separately, DOVLTC now permanently disables the hardware VLAN type
comparison. Does that mean a filter entry installed for a C-VLAN VID also
admits S-tagged frames with the same VID, and that S-tagged frames whose
VID is not registered are now dropped in hardware, where previously with
ESVL clear they were not treated as VLAN at all? Probe still advertises
NETIF_F_HW_VLAN_CTAG_FILTER and NETIF_F_HW_VLAN_STAG_FILTER as separate
offloads, and stmmac_vlan_rx_add_vid() / stmmac_vlan_rx_kill_vid() no
longer pass proto down to the hash configuration at all.

The later series commit "net: stmmac: Disable S-Tag processing on dwmac4"
describes this failure mode for GMAC4 and narrows hw_svlan_en to
core_type == DWMAC_CORE_XGMAC. At the end of the series (bb70e68e6ebc),
ESVL and DOVLTC still end up unconditionally set for dwxgmac2 and
dwxlgmac2, and vlan_rx_hw() still hardcodes ETH_P_8021Q, so does the same
mislabeling remain reachable on those cores?

The commit message presents this as dropping the is_double logic and adding
a hw_svlan_en flag. Could it also describe the receive stripping and
receive filtering behaviour changes?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
> index 200b34588c7f..fbb99b70ac27 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c

[ ... ]

> @@ -173,18 +172,9 @@ static void vlan_update_hash(struct mac_device_info *hw, u32 hash,
>  
>  	if (hash) {
>  		value |= VLAN_VTHM | VLAN_ETV;
> -		if (is_double) {
> -			value |= VLAN_ESVL;
> -			value |= VLAN_DOVLTC;
> -		} else {
> -			value &= ~VLAN_ESVL;
> -			value &= ~VLAN_DOVLTC;
> -		}
> -
>  		writel(value, ioaddr + VLAN_TAG);
>  	} else {
> -		value &= ~(VLAN_VTHM | VLAN_ETV | VLAN_ESVL);
> -		value &= ~VLAN_DOVLTC;
> +		value &= ~(VLAN_VTHM | VLAN_ETV);
>  		value &= ~VLAN_VID;
>  
>  		writel(value, ioaddr + VLAN_TAG);
> @@ -220,6 +210,12 @@ static void vlan_set_hw_mode(struct mac_device_info *hw)
>  	u32 value = readl(ioaddr + VLAN_TAG);
>  
>  	value |= VLAN_EDVLP;
> +
> +	if (hw->hw_svlan_en)
> +		value |= VLAN_ESVL | VLAN_DOVLTC;
> +	else
> +		value &= ~(VLAN_ESVL | VLAN_DOVLTC);
> +
>  	value &= ~VLAN_TAG_CTRL_EVLS_MASK;
>  
>  	if (hw->hw_vlan_en)

Since hw_svlan_en is true for every core and hw_vlan_en is true for every
xmac core, both branches below end up in the same register write, enabling
S-tag recognition and strip-all at once. Is that the intended combination
given vlan_rx_hw() cannot report the tag type?

[ ... ]

> @@ -283,7 +270,14 @@ 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);
> +	value |= VLAN_EDVLP;
> +
> +	if (hw->hw_svlan_en)
> +		value |= VLAN_ESVL | VLAN_DOVLTC;
> +	else
> +		value &= ~(VLAN_ESVL | VLAN_DOVLTC);
> +
> +	writel(value, ioaddr + VLAN_TAG);
>  }

This path previously only set EDVLP and never touched ESVL or DOVLTC. Does
XLGMAC now also get S-VLAN classification and the disabled tag type
comparison enabled on every open, resume and feature change? XLGMAC shares
core_type DWMAC_CORE_XGMAC, so it keeps hw_svlan_en set even after the
later dwmac4 fix in the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908164309.59282-1-ovidiu.panait.rb%40renesas.com