Re: [PATCH net v4 4/7] net: stmmac: Do not advertise S-VLAN stripping when it is disabled

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 0 · Low: 2

- [Low] Commit-message premise vs. actual code: with rx-vlan-offload off
  (priv->hw->hw_vlan_en == false) the driver did not stop stripping…
- [Low] The patch carries 'Fixes: 750011e239a5' yet its hunk depends on
  priv->hw->hw_svlan_en, a struct member added by the immediately…

Pre-existing issues:
- [High] Hardware-stripped S-VLAN tags are handed to the stack with the
  wrong TPID.

--

net: stmmac: Do not advertise S-VLAN stripping when it is disabled

The patch makes NETIF_F_HW_VLAN_STAG_RX follow NETIF_F_HW_VLAN_CTAG_RX in
stmmac_fix_features(), so that rx-vlan-stag-hw-parse is no longer reported
as enabled after rx-vlan-offload is turned off.  Both are driven by the same
EVLS field, so the two features are tied together in hardware.

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index cd9671493b41..c81c5bb5b075 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6189,6 +6189,13 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
>  	if (priv->plat->bugged_jumbo && (dev->mtu > ETH_DATA_LEN))
>  		features &= ~NETIF_F_CSUM_MASK;
>  
> +	if (priv->hw->hw_svlan_en) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_RX)
> +			features |= NETIF_F_HW_VLAN_STAG_RX;
> +		else
> +			features &= ~NETIF_F_HW_VLAN_STAG_RX;
> +	}
> +

[Severity: Low]
The commit message describes the problem purely as a stale advertisement
("rx-vlan-stag-hw-parse keeps being advertised as enabled"), but clearing
NETIF_F_HW_VLAN_STAG_RX here also turns off the software S-tag stripper in
the same file:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
	if ((vlan_proto == htons(ETH_P_8021Q) &&
	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
	    (vlan_proto == htons(ETH_P_8021AD) &&
	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
	...
}

With rx-vlan-offload off, hw_vlan_en is false and stmmac_rx() takes the
stmmac_rx_vlan() path, so before this change 802.1ad tags were still popped
into skb metadata by the driver; afterwards they are left in the linear
frame.

This looks harmless in practice, since __netif_receive_skb_core() calls
skb_vlan_untag() for any eth_type_vlan() protocol and re-establishes the tag
with the correct ETH_P_8021AD TPID.  Would it be worth mentioning the
datapath side effect in the changelog, so it is clear that only the place
where the S-tag is moved into skb metadata changes?

[Severity: Low]
The commit carries Fixes: 750011e239a5 but the hunk tests
priv->hw->hw_svlan_en, which is a struct mac_device_info member added by the
preceding, non-Fixes patch of this series ("net: stmmac: Rework S-VLAN
handling").  A backport of this patch alone onto a kernel that only has
750011e239a5 would not build.

Could a note about that dependency be added for stable backporters?

[Severity: High]
This isn't a bug introduced by this patch, but keeping
NETIF_F_HW_VLAN_STAG_RX advertised whenever NETIF_F_HW_VLAN_CTAG_RX is on
also keeps advertising an S-VLAN parse offload whose result is labelled with
the wrong TPID.

In the hardware path, the stripped outer tag is always handed up as 802.1Q:

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

while the descriptor helper deliberately accepts S-tagged and double-tagged
frames:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_wrback_get_rx_vlan_valid() {
	et_lt = FIELD_GET(XGMAC_RDES3_ET_LT, le32_to_cpu(p->des3));

	return et_lt >= XGMAC_ET_LT_VLAN_STAG &&
	       et_lt <= XGMAC_ET_LT_DVLAN_STAG_CTAG;
}

So on a dwxgmac2 core with rx-vlan-offload on, an 802.1ad frame is delivered
as 802.1Q: an 8021ad upper device never sees it, a same-VID 8021Q upper
device does, and a forwarded frame is re-tagged with the wrong TPID.

This appears to date back to 750011e239a5 rather than to this patch, and
vlan_rx_hw() is untouched by the rest of the series: the later "net: stmmac:
Disable S-Tag processing on dwmac4" only drops the STAG advertisement for
non-XGMAC cores, and dwxgmac210_vlan_ops still points .rx_hw_vlan at
vlan_rx_hw().  Since the ET/LT field already distinguishes S-tag, C-tag and
double tag, could vlan_rx_hw() use it to pick between ETH_P_8021Q and
ETH_P_8021AD?

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