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

Hi,

On 7/29/26 17:02, Ovidiu Panait wrote:

Hi Maxime,

quoted

Hi Ovidiu,

On 7/29/26 11:51, Ovidiu Panait wrote:

quoted

stmmac_vlan_update() falls back to "perfect matching" when the VLAN hash
filter is unavailable (!priv->dma_cap.vlhash). This fallback has been
unreachable in normal operation since its introduction in
commit c7ab0b8088d7 ("net: stmmac: Fallback to VLAN Perfect filtering if
HASH is not available") because the NETIF_F_HW_VLAN_{CTAG,STAG}_FILTER
features are advertised only when priv->dma_cap.vlhash is true.

The fallback is also duplicating the code in vlan_add_hw_rx_fltr(),

which

quoted

is always available since stmmac_get_num_vlan() returns at least 1.

It's not exactly dead code, there's an stmmac selftest for that, see :

https://elixir.bootlin.com/linux/v7.2-
rc4/source/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c#L973

It does exactly what you say, it fakes the fact that we don't support
vlhash,
then exercise the perfect matching path.

But is it correct to drop that feature altogether ? Or should we just
relax the conditions for NETIF_F_HW_VLAN_{CTAG,STAG}_FILTER to be
advertised ?

We are not dropping the perfect matching feature by removing this code.
Basically, there are two perfect matching code paths that duplicate the
same logic and which diverged over time:

- vlan_update_hash() has the "if (perfect_match)" path that has never run on real
  hardware (unless running the selftest) because NETIF_F_HW_VLAN_{CTAG,STAG}_FILTER
  is only advertised if the core supports VLAN hash filtering.

- vlan_add_hw_rx_fltr() also has a perfect matching codepath, which calls into
  vlan_write_single(). This runs unconditionally and overwrites what
  vlan_update_hash() did anyway, as it runs after stmmac_vlan_update():

stmmac_vlan_rx_add_vid():
...
	ret = stmmac_vlan_update(priv, num_double_vlans);
	if (ret) {
		clear_bit(vid, priv->active_vlans);
		goto err_pm_put;
	}

	if (priv->hw->num_vlan) {
		ret = stmmac_add_hw_vlan_rx_fltr(priv, ndev, priv->hw, proto, vid);
		if (ret) {
			clear_bit(vid, priv->active_vlans);
			stmmac_vlan_update(priv, priv->num_double_vlans);
			goto err_pm_put;
		}
	}

This path is also more correct because it takes into account cores that support
extended VLAN filters.

Therefore, I think removing the perfect matching code in vlan_update_hash() is
the right choice here.

OK makes sense. Then the sensible followup (not necessarily for -net) would be to
update the selftests, as there's no point messing with the vlhash cap then, if I'm
getting this right.

Thanks :)

Maxime