Re: [PATCH net v6 1/5] net: stmmac: Remove VLAN perfect matching dead code

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch removes the only code that ever read
  priv->dma_cap.vlhash inside stmmac_vlan_update().
- [Low] The commit message's second reason for removal is wrong for
  XLGMAC.

--

net: stmmac: Remove VLAN perfect matching dead code

This commit removes the perfect-matching fallback in stmmac_vlan_update().
That fallback was used when the VLAN hash filter is unavailable
(!priv->dma_cap.vlhash). The commit also drops the perfect_match argument
from the update_vlan_hash() callbacks, together with the matching branches
in vlan_update_hash() and dwxgmac2_update_vlan_hash().

> The fallback is also duplicating the code in vlan_add_hw_rx_fltr(), which
> is always available since stmmac_get_num_vlan() returns at least 1.

[Severity: Low]
Does this hold for XLGMAC? hwif.c maps XLGMAC cores to dwxlgmac2_vlan_ops,
which has no add, del or restore filter callbacks:

drivers/net/ethernet/stmicro/stmmac/stmmac_vlan.c:
const struct stmmac_vlan_ops dwxlgmac2_vlan_ops = {
	.update_vlan_hash = dwxgmac2_update_vlan_hash,
	.enable_vlan = vlan_enable,
};

dwxlgmac2_setup() also never calls stmmac_get_num_vlan(). Only
dwmac4_setup() and dwxgmac2_setup() do. mac_device_info is zero-allocated,
so hw->num_vlan stays 0 on XLGMAC, and stmmac_vlan_rx_add_vid() skips the
num_vlan block.

On XLGMAC, the perfect_match branch removed from
dwxgmac2_update_vlan_hash() seems to be the only non-hash VLAN filter code.
It is not a duplicate of vlan_add_hw_rx_fltr().

The removal itself still looks safe. The first reason in the commit message
holds, since the filter features are only advertised when
dma_cap.vlhash is set. Also, the removed "pmatch = vid" ran after the
for_each_set_bit() loop, so it only ever programmed VID 4096.

Could this paragraph be corrected? Otherwise reviewers and backporters may
assume XLGMAC keeps a perfect-filter path.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec62fa7418f4..bdb856291340 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6853,29 +6853,18 @@ static u32 stmmac_vid_crc32_le(__le16 vid_le)
>  static int stmmac_vlan_update(struct stmmac_priv *priv, bool is_double)
>  {
>  	u32 crc, hash = 0;
> -	u16 pmatch = 0;
> -	int count = 0;
>  	u16 vid = 0;
>  
>  	for_each_set_bit(vid, priv->active_vlans, VLAN_N_VID) {
>  		__le16 vid_le = cpu_to_le16(vid);
>  		crc = bitrev32(~stmmac_vid_crc32_le(vid_le)) >> 28;
>  		hash |= (1 << crc);
> -		count++;
> -	}
> -
> -	if (!priv->dma_cap.vlhash) {
> -		if (count > 2) /* VID = 0 always passes filter */
> -			return -EOPNOTSUPP;
> -
> -		pmatch = vid;
> -		hash = 0;
>  	}
>  
>  	if (!netif_running(priv->dev))
>  		return 0;
>  
> -	return stmmac_update_vlan_hash(priv, priv->hw, hash, pmatch, is_double);
> +	return stmmac_update_vlan_hash(priv, priv->hw, hash, is_double);
>  }

[Severity: Low]
This removes the only check of priv->dma_cap.vlhash in
stmmac_vlan_update(). Do the "(perf)" ethtool selftests still test anything
different from the hash tests now?

stmmac_test_vlanfilt_perfect() and stmmac_test_dvlanfilt_perfect() in
stmmac_selftests.c still toggle that capability to force the perfect-match
path:

drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:stmmac_test_vlanfilt_perfect() {
    ...
	priv->dma_cap.vlhash = 0;
	ret = __stmmac_test_vlanfilt(priv);
	priv->dma_cap.vlhash = prev_cap;
    ...
}

With this patch, adding VID 0x123 makes stmmac_vlan_update() compute a
non-zero hash and always pass it to update_vlan_hash(). As a result,
vlan_update_hash() and dwxgmac2_update_vlan_hash() set
VLAN_VTHM | VLAN_ETV (and XGMAC_FILTER_VTFE on XGMAC), whatever the vlhash
toggle says.

Before this patch, the fallback forced hash = 0 and cleared VTHM. That left
only the perfect filters programmed by vlan_add_hw_rx_fltr() active.

The "VLAN Filtering (perf)" and "Double VLAN Filter (perf)" entries are
still registered in stmmac_selftests[]. They now pass on the hash filter
alone, because the test VIDs 0x124..0x126 fall into different hash bins. On
XLGMAC (num_vlan == 0, no add_hw_vlan_rx_fltr), they don't exercise perfect
filtering at all.

These selftests also seem to stay as they are later in the series. The
later rename only changes the dvlanfilt names to svlanfilt.

Should these two selftests be removed or reworked as part of this change?
Otherwise ethtool -t reports PASS for perfect filtering that is no longer
tested on its own.

[ ... ]

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