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

[Maxime Chevallier]

Hi,

On 7/29/26 17:02, Ovidiu Panait wrote:
> Hi Maxime,
> 
>>
>> Hi Ovidiu,
>>
>> On 7/29/26 11:51, Ovidiu Panait wrote:
>>> 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
>>> 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