Re: [PATCH net v4 3/7] net: stmmac: selftests: Check the dev->features for S-TAG offload testing

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] stmmac_test_svlanoff() is re-gated on NETIF_F_HW_VLAN_STAG_TX,
  but the test body it enables validates a *double-tagged* (QinQ) frame,…

--

net: stmmac: selftests: Check the dev->features for S-TAG offload testing

The S-TAG insertion selftest was gated on the dvlan (double VLAN) DMA
capability, which is not the same thing as S-TAG insertion support. This
commit switches the gate to NETIF_F_HW_VLAN_STAG_TX, and keeps the test
in tree so it starts working if the driver ever advertises that feature.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> index 2f9f7746c40ae..de02c0da56dc3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
> @@ -1415,7 +1415,7 @@ static int stmmac_test_vlanoff(struct stmmac_priv *priv)
>  
>  static int stmmac_test_svlanoff(struct stmmac_priv *priv)
>  {
> -	if (!priv->dma_cap.dvlan)
> +	if (!(priv->dev->features & NETIF_F_HW_VLAN_STAG_TX))
>  		return -EOPNOTSUPP;
>  	return stmmac_test_vlanoff_common(priv, true);
>  }

[Severity: Medium]
Does the new gate match what the test body actually validates?  The gate
now keys on single S-TAG transmit offload, but the code it enables appears
to expect a double-tagged (QinQ) frame.

In stmmac_test_vlanoff_common() the frame is built untagged (attr is
`struct stmmac_packet_attrs attr = { }` and attr.vlan is never set, so
stmmac_test_get_udp_skb() skips its `if (attr->vlan)` block), and exactly
one acceleration tag is attached:

	__vlan_hwaccel_put_tag(skb, htons(proto), tpriv->vlan_id);
	skb->protocol = htons(proto);

while the ptype registered for the svlan case listens for 802.1Q:

	tpriv->pt.type = svlan ? htons(ETH_P_8021Q) : htons(ETH_P_IP);

And stmmac_test_vlan_validate() skips a 4-byte inner tag before the IP
header when double_vlan is set:

	ihdr = ip_hdr(skb);
	if (tpriv->double_vlan)
		ihdr = (struct iphdr *)(skb_network_header(skb) + 4);
	if (ihdr->protocol != IPPROTO_UDP)
		goto out;

With a conforming NETIF_F_HW_VLAN_STAG_TX implementation the looped-back
frame is DA|SA|0x88A8|TCI|0x0800|IP.  After skb_vlan_untag() the ethertype
is ETH_P_IP, so the ETH_P_8021Q ptype is never matched, and even if it
were, skb_network_header(skb) + 4 lands 4 bytes into the IP header and the
IPPROTO_UDP check fails.  Wouldn't that make the test report -ETIMEDOUT
the moment the feature bit is advertised?

Along the same lines, can the current transmit path ever produce the frame
the validator wants?  stmmac_vlan_insert() in stmmac_main.c ignores
skb->vlan_proto and hardcodes the inner tag and type:

	if (stmmac_set_desc_vlan_tag(priv, p, tag, 0x0, 0x0))
		return false;

and vlan_enable() in stmmac_vlan.c selects C-VLAN insertion only:

	value &= ~VLAN_CSVL; /* Only use CVLAN */

So the only way the retained test could pass seems to be an implementation
that wrongly inserts an extra inner C-TAG.

Would it make sense to also adjust the frame and the validation here (for
example setting attr.vlan for an inner C-TAG with pt.type = ETH_P_IP, or
dropping the double_vlan expectation for the S-TAG case) rather than only
swapping the capability bit, so the test matches the semantics of the
feature it is now gated on?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917215339.2022523-1-maxime.chevallier%40bootlin.com