Re: [PATCH net v6 5/5] net: stmmac: Disable S-Tag processing on dwmac4

[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: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: XGMAC still passes hardware-stripped
  802.1ad tags up the stack as 802.1Q.
- [Low] After this patch, the two S-VLAN selftests check for different
  things.

--

net: stmmac: Disable S-Tag processing on dwmac4

This commit limits NETIF_F_HW_VLAN_STAG_RX and NETIF_F_HW_VLAN_STAG_FILTER
to DWMAC_CORE_XGMAC. The reason given is that dwmac4 cannot report which
tag type was stripped, and vlan_rx_hw() hardcodes ETH_P_8021Q. On dwmac4,
802.1ad tags are then left in the packet for 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 2ce6e2724bcb..b837e8e27a35 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -8005,14 +8005,18 @@ 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;
> +	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
> +	if (priv->plat->core_type == DWMAC_CORE_XGMAC)
> +		ndev->features |= NETIF_F_HW_VLAN_STAG_RX;
> +
>  	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]
Won't XGMAC still pass hardware-stripped 802.1ad tags up the stack as
802.1Q after this change?

The commit message says that on dwxgmac2 "the outer tag type can be
determined based on that info". However, nothing in this patch or the rest
of the series actually does that.

dwxgmac210_vlan_ops still uses vlan_rx_hw() as .rx_hw_vlan, and that
function is unchanged:

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

dwxgmac2_wrback_get_rx_vlan_valid() accepts the ET_LT values with an S-tag
outer tag, but it returns only a bool, so the tag type is lost:

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

On XGMAC, hw_vlan_en is still set and STAG_RX/STAG_FILTER are still
advertised. stmmac_fix_features() also still ties STAG_RX to CTAG_RX.
Creating an 802.1ad VLAN on an XGMAC with vlhash then leads to:

stmmac_vlan_rx_add_vid(is_svlan = true)
  dwxgmac2_update_vlan_hash()    /* sets VLAN_ESVL */

and on receive:

stmmac_rx()
  stmmac_rx_hw_vlan()
    vlan_rx_hw()                 /* tags the skb as ETH_P_8021Q */

vlan_do_receive() then looks up (ETH_P_8021Q, vid), so the 802.1ad VLAN
device never gets the frame. Isn't this the same QinQ ping failure the
commit message describes?

Also, if an 802.1Q VLAN with the same VID exists, would S-tagged frames
from the wire be delivered to that interface instead?

Should vlan_rx_hw() take the protocol from ET_LT on XGMAC? Or should S-tag
offload be turned off on XGMAC as well?

>  	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]
On dwmac4, vlhash no longer implies NETIF_F_HW_VLAN_STAG_FILTER. Should
stmmac_test_svlanfilt() in stmmac_selftests.c be updated to match? It
still checks only:

	if (!priv->dma_cap.vlhash)
		return -EOPNOTSUPP;

Its sibling, stmmac_test_svlanfilt_perfect(), checks the feature flag:

	if (!(priv->dev->features & NETIF_F_HW_VLAN_STAG_FILTER))
		return -EOPNOTSUPP;

On a dwmac4 with vlhash, __stmmac_test_svlanfilt() calls
vlan_vid_add(priv->dev, htons(ETH_P_8021AD), tpriv->vlan_id). Without
STAG_FILTER, vlan_hw_filter_capable() returns false for 802.1ad. As a
result, stmmac_vlan_rx_add_vid() is never called and no S-VLAN filter is
programmed.

The test still runs, and for i > 0 the loop treats a missing tpriv->ok as
success:

		ret = tpriv->ok ? 0 : -ETIMEDOUT;
		if (ret && !i) {
			goto vlan_del;
		} else if (!ret && i) {
			ret = -EINVAL;
			goto vlan_del;
		} else {
			ret = 0;
		}

Can "SVLAN Filtering" then report a pass on hardware that no longer uses
S-VLAN filtering, while the perfect variant is skipped?

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