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

[Ovidiu Panait]

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

[...]

> 
> [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?
> 

I tried to address this in the initial version, but since I do not have
access to a board using XGMAC, the fix was only compile tested:
https://lore.kernel.org/all/20260729095110.164157-6-ovidiu.panait.rb@xxxxxxxxxxx/

However, Nazim tested it and noticed that it doesn't fix the issue on
on dwxgmac2, so I dropped the patch from the series.

I think someone with access to a dwxgmac2 platform should be continuing the
investigation.

Note that the other "High" finding from Sashiko is referring to the same issue. 

Thanks,
Ovidiu

> 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