Re: [PATCH v2] net: stmmac: skip VLAN restore when VLAN hash ops are missing

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Tue, Mar 24, 2026 at 01:14:29PM +0100, Paolo Abeni wrote:
> On 3/21/26 6:38 AM, Michal Piekos wrote:
> > stmmac_vlan_restore() unconditionally calls stmmac_vlan_update() when
> > NETIF_F_VLAN_FEATURES is set. On platforms where priv->hw->vlan (or
> > ->update_vlan_hash) is not provided, stmmac_update_vlan_hash() returns
> > -EINVAL via stmmac_do_void_callback(), resulting in a spurious
> > "Failed to restore VLANs" error even when no VLAN filtering is in use.
> > 
> > Check presence of VLAN HW FILTER flags before stmmac_vlan_update().
> > 
> > Tested on Orange Pi Zero 3.
> > 
> > Fixes: bd7ad51253a7 ("net: stmmac: Fix VLAN HW state restore")
> > Signed-off-by: Michal Piekos <michal.piekos@mmpsystems.pl>
> > ---
> > This patch fixes a noisy "Failed to restore VLANs" message on platforms
> > where stmmac VLAN hash ops are not implemented.
> > stmmac_vlan_restore() calls stmmac_vlan_update() without checking for
> > VLAN hash ops presence which results in -EINVAL. 
> > ---
> > Changes in v2:
> > - Replace check for hash ops with check for HW FILTER flags
> > - Link to v1: https://lore.kernel.org/r/20260314-vlan-restore-error-v1-1-4fc6c3e2115f@mmpsystems.pl
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 6827c99bde8c..cfc0ce9cec9c 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -6863,7 +6863,8 @@ static int stmmac_vlan_restore(struct stmmac_priv *priv)
> >  {
> >  	int ret;
> >  
> > -	if (!(priv->dev->features & NETIF_F_VLAN_FEATURES))
> > +	if (!(priv->dev->features &
> > +	      (NETIF_F_HW_VLAN_CTAG_FILTER | NETIF_F_HW_VLAN_STAG_FILTER)))
> >  		return 0;
> >  
> >  	if (priv->hw->num_vlan)
> Adding Russell.
> 
> It's not obvious to me that with this change the
> restore_hw_vlan_rx_fltr() and vlan_update() callback are still invoked
> in all the relevant driver/features permutation.

I think there's a few questions here.

When CONFIG_VLAN_8021Q is enabled, then we set the filter features
if the vlhash dma capability is set:

        if (priv->dma_cap.vlhash) {
                ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
                ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
        }

Only dwmac4 and dwxgmac2 have the ability to set this feature (and
thus be synthesised with the feature enabled.) From what I understand,
when present, these features are permanently enabled (as they're not
set in hw_Features).

However, in stmmac_vlan_update() there is:

        if (!priv->dma_cap.vlhash) {
                if (count > 2) /* VID = 0 always passes filter */
                        return -EOPNOTSUPP;

                pmatch = vid;
                hash = 0;
        }

So, it is clear that the intention is that stmmac_vlan_update() will
be called even when the NETIF_F_HW_VLAN_*TAG_FILTER features are not
present - consider a core that implements the VLAN ops, but has
priv->dma_cap.vlhash false.

It looks like vlan support was added in dwmac v4.0 and later cores
(from the hwif table.) As the vlan ops are NULL before that, the
horrid stmmac_do_void_callback() crud will return -EINVAL (yuck). The
method it calls is a void function, so the only possible return values
are zero if implemented, or -EINVAL if not.

stmmac_vlan_update() itself only ever returns zero if the netif
isn't running, or the above return value. So, an error returned from
stmmac_vlan_update() just means that the driver has no support for
programming the vlan configuration.

Looking at the implementations for the case where vlhash is false,
(where stmmac_update_vlan_hash() will be called with hash=0 and
perfect_match with a vid, vlan_update_hash() still write
to the VLAN_TAG register to configure vlan handling. I haven't
looked up exactly what it does.

The other possibility is dwxgmac2_update_vlan_hash(). This looks
similar.

So, I don't think that the correct solution is to only call this
function when one or more of NETIF_F_HW_VLAN_*TAG_FILTER feature flags
are set - clearly the code is written to handle the case where the
update_vlan_hash method is populated but vlhash is false and these
feature flags are clear.

Now I'm wondering whether the code in the STMMAC_VLAN_TAG_USED block
is correct:

        /* Both mac100 and gmac support receive VLAN tag detection */
        ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
        if (dwmac_is_xmac(priv->plat->core_type)) {
                ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
                priv->hw->hw_vlan_en = true;
        }
        if (priv->dma_cap.vlhash) {
                ndev->features |= NETIF_F_HW_VLAN_CTAG_FILTER;
                ndev->features |= NETIF_F_HW_VLAN_STAG_FILTER;
        }
        if (priv->dma_cap.vlins)
                ndev->features |= NETIF_F_HW_VLAN_CTAG_TX;

The test for dwmac_is_xmac() matches for dwmac4 and dwxgmac cores. If
this is true, then we will have the vlan ops populated, and it means
that NETIF_F_HW_VLAN_CTAG_RX can be configured by the user rather than
being always-enabled. Is this correct? I'm not sure - I haven't dug
into enough of the documentation for this yet. However, I suggest
that we need to always call stmmac_update_vlan_hash() for these cores.

So, I'm coming to the conclusion that we either need a test for
dwmac_is_xmac() and not the feature flags, or maybe we just get rid
of the "Failed to restore VLANs" error print and make
stmmac_vlan_restore() return void (nothing uses the return value.)

In summary, I'm not sure what the correct approach is - please reach
out to Ovidiu Panait <ovidiu.panait.rb@renesas.com> who recently added
this code, but I'm fairly sure that this solution is incorrect.

Really, Ovidiu Panait should be reviewing this patch as his recent
change introduced the problematical error print.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!