Re: [PATCH net v6 2/5] net: stmmac: Stop toggling the EDVLP bit

[netdev-bot+sashiko]

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

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

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).

- [Medium] The COE benefit that the commit message gives as its reason
  for this change never shows up in this tree.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is still
  there at the end of the series for XGMAC 2.10.

--

net: stmmac: Stop toggling the EDVLP bit

The EDVLP bit is no longer set and cleared in the VLAN hash update paths
when an 802.1ad VLAN is registered. It is now always set from
vlan_set_hw_mode(), and a new dwxlgmac2_set_hw_vlan_mode() callback sets it
on dwxlgmac2 as well.

> 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

[ ... ]

> @@ -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)

[Severity: Medium]
The commit message says EDVLP is kept always enabled "so that COE can work
for packets with an inner VLAN header". Does the driver actually use the
checksum offload result for those packets?

stmmac_has_ip_ethertype() in stmmac_main.c rejects any frame that still has
a VLAN header in the payload:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_has_ip_ethertype() {
    ...
	proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb),
				    &depth);

	return (depth <= ETH_HLEN) &&
		(proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));
}

On RX, EVLS=STRIP_ALL strips only the outer tag and EIVLS is never
programmed, so the inner 802.1Q header stays in-band. Both stmmac_rx() and
stmmac_dispatch_skb_zc() then drop the hardware result:

	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb) ||
	    (status & csum_none))
		skb_checksum_none_assert(skb);

On TX, stmmac_features_check() clears the checksum features for the same
skbs. STAG_TX is not advertised, so a QinQ frame always has at least one
in-band tag:

	if (priv->plat->tx_queues_cfg[queue].coe_unsupported ||
	    !stmmac_has_ip_ethertype(skb))
		features &= ~(NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM);

This holds whether or not EDVLP is set. No later patch in the series changes
stmmac_has_ip_ethertype(). The later patch "net: stmmac: Rename double VLAN
references to svlan" also says "the networking core does not support
offloads for the inner tag anyway".

This patch changes VLAN_TAG programming on all DWMAC4/5, XGMAC and XLGMAC
cores. Could the COE rationale be dropped or corrected in the commit
message? If COE for inner-VLAN frames is the goal, does
stmmac_has_ip_ethertype() also need updating, along with a Fixes: tag?

> @@ -254,11 +252,9 @@ static void dwxgmac2_update_vlan_hash(struct mac_device_info *hw, u32 hash,
>  
>  		value |= VLAN_VTHM | VLAN_ETV;
>  		if (is_double) {
> -			value |= VLAN_EDVLP;
>  			value |= VLAN_ESVL;
>  			value |= VLAN_DOVLTC;
>  		} else {
> -			value &= ~VLAN_EDVLP;
>  			value &= ~VLAN_ESVL;
>  			value &= ~VLAN_DOVLTC;
>  		}

[Severity: High]
This problem was not introduced by this patch. It is still present on
XGMAC 2.10 cores at the end of the series, though.

Registering an 802.1ad VID follows this call chain, with is_double set:

stmmac_vlan_rx_add_vid()->stmmac_vlan_update()->dwxgmac2_update_vlan_hash()

That sets ESVL and DOVLTC. With hw_vlan_en, which is the default on XGMAC,
vlan_set_hw_mode() also programs EVLS=STRIP_ALL and EVLRXS. The MAC then
strips 0x88A8 outer tags and writes the TCI into RDES0.

dwxgmac2_wrback_get_rx_vlan_valid() accepts the S-tag packet types:

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

However, vlan_rx_hw() always reports the stripped tag as a C-tag:

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

Won't S-tagged frames then go to an 802.1Q upper device with the same VID,
or be dropped, and never reach the 802.1ad upper device? The software path
in stmmac_rx_vlan() does tell ETH_P_8021AD apart, so the two paths disagree.

The last patch in the series, "net: stmmac: Disable S-Tag processing on
dwmac4", stops advertising STAG_RX/STAG_FILTER only on dwmac4. Its commit
message says the XGMAC tag type can be derived from ET_LT. Yet vlan_rx_hw()
is unchanged, and XGMAC still advertises the S-tag features in
__stmmac_dvr_probe():

	if (priv->plat->core_type == DWMAC_CORE_XGMAC)
		ndev->features |= NETIF_F_HW_VLAN_STAG_RX;

On XGMAC, should vlan_rx_hw() use ET_LT to report ETH_P_8021AD, or should
XGMAC also stop advertising the S-tag features?

S-tag recognition appears to be controlled by ESVL/DOVLTC, not EDVLP.
Setting EDVLP unconditionally here does not seem to make this worse.

[ ... ]

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