Message-ID: <20220309155147.mdg34azyst4wwvfj@skbuf>
Date:   Wed, 9 Mar 2022 17:51:47 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     "Russell King (Oracle)" <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH net] net: dsa: silence fdb errors when unsupported

On Wed, Mar 09, 2022 at 02:18:48PM +0000, Russell King (Oracle) wrote:
> On Wed, Mar 09, 2022 at 12:41:43PM +0200, Vladimir Oltean wrote:
> > Hello Russell,
> > 
> > On Wed, Mar 09, 2022 at 10:35:32AM +0000, Russell King (Oracle) wrote:
> > > When booting with a Marvell 88e6xxx switch, the kernel spits out a
> > > load of:
> > > 
> > > [    7.820996] mv88e6085 f1072004.mdio-mii:04: port 3 failed to add aa:bb:cc:dd:ee:ff vid XYZ1 to fdb: -95
> > > [    7.835717] mv88e6085 f1072004.mdio-mii:04: port 2 failed to add aa:bb:cc:dd:ee:ff vid XYZ1 to fdb: -95
> > > [    7.851090] mv88e6085 f1072004.mdio-mii:04: port 1 failed to add aa:bb:cc:dd:ee:ff vid XYZ1 to fdb: -95
> > > [    7.968594] mv88e6085 f1072004.mdio-mii:04: port 0 failed to add aa:bb:cc:dd:ee:ff vid XYZ1 to fdb: -95
> > > [    8.035408] mv88e6085 f1072004.mdio-mii:04: port 3 failed to add aa:bb:cc:dd:ee:ff vid XYZ3 to fdb: -95
> > > 
> > > while the switch is being setup. Comments in the Marvell DSA driver
> > > indicate that "switchdev expects -EOPNOTSUPP to honor software VLANs"
> > > in mv88e6xxx_port_db_load_purge() so this error code should not be
> > > treated as an error.
> > > 
> > > Fixes: 3dc80afc5098 ("net: dsa: introduce a separate cross-chip notifier type for host FDBs")
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@...linux.org.uk>
> > > ---
> > > Hi,
> > > 
> > > I noticed these errors booting 5.16 on my Clearfog platforms with a
> > > Marvell DSA switch. It appears that the switch continues to work
> > > even though these errors are logged in the kernel log, so this patch
> > > merely silences the errors, but I'm unsure this is the right thing
> > > to do.
> > 
> > Can you please confirm that these errors have disappeared on net-next?
> 
> net-next: no warnings
> v5.17-rc7: warnings
> v5.16: warnings

Thanks. This means it was solved by this patch set, which had the exact
same symptoms:
https://patchwork.kernel.org/project/netdevbpf/cover/20220215170218.2032432-1-vladimir.oltean@nxp.com/
The cover letter and commit messages provide a full description of what
was wrong, has been wrong/a limitation for years, and give a sense of
why that work cannot be backported to v5.16, since it implies a
non-trivial amount of changes to the bridge driver, to switchdev and to
DSA.

> So, it looks like we need a patch for 5.17-rc7 and 5.16-stable to fix
> this. Do you have a better suggestion than my patch?

So to answer the question first, then to explain.

My better suggestion, taking into consideration more factors, is to
drop this patch and leave the code as it is.

The comment in the Marvell DSA driver, which you cited:

mv88e6xxx_port_db_load_purge:
	/* switchdev expects -EOPNOTSUPP to honor software VLANs */
	if (!vlan.valid)
		return -EOPNOTSUPP;

is a logical fallacy.

Fact: the bridge does _not_ check errors from br_switchdev_fdb_notify().
Not only that, but there isn't even any mechanism in place today such
that this would be possible. Otherwise, myself, Nikolay and Ido wouldn't
have been unsuccessfully scrambling for several months to address that
limitation (actually mostly me, but they were the direct victims of
trying to review my attempts):
https://patchwork.kernel.org/project/netdevbpf/cover/20211025222415.983883-1-vladimir.oltean@nxp.com/

So if switchdev is not aware of mv88e6xxx returning -EOPNOTSUPP from a
work item that is scheduled from the SWITCHDEV_FDB_ADD_TO_DEVICE atomic
handler, then logically, switchdev cannot anything from -EOPNOTSUPP.
This error code has no special meaning.

Note that there was a second call path to drivers' ds->ops->port_fdb_add(),
from the DSA slave's ndo_fdb_add, and that did propagate errors. But
that is "bridge bypass", not switchdev, and for doing more harm than
good, support for it was removed in commit b117e1e8a86d ("net: dsa:
delete dsa_legacy_fdb_add and dsa_legacy_fdb_del"). Not only is this
call path no longer present today, but -EOPNOTSUPP isn't a special error
code there, either.

Having said this, the initiator of the call path which fails with
-EOPNOTSUPP in mv88e6xxx_port_db_load_purge() is the bridge driver,
via switchdev.

The mv88e6xxx driver, via that check, essentially expects that the VID
from the FDB entry has been previously mapped in the VTU.

The bridge driver already ensures that the VLAN structure has been
created, before any FDB entries can be added in that VLAN. This is
proven by the attempt below to add an FDB entry in VLAN 5, in bridge
port swp0:

root@...ian:~# bridge fdb add dev swp0 static master 00:01:02:03:04:05 vlan 5
[109407.613593] bridge: RTM_NEWNEIGH with unconfigured vlan 5 on swp0
RTNETLINK answers: Invalid argument
root@...ian:~# bridge vlan
port    vlan ids
swp0     1 PVID Egress Untagged

br0      1 PVID Egress Untagged

root@...ian:~# bridge link
9: swp0@...2: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 master br0 state disabled priority 32 cost 4

So the sanity check captures what is essentially an invalid condition.
If the bridge notifies an FDB entry in a VLAN that isn't present in the
VTU, this is a bug.

Now on to my point. There is a trade-off to be made between things
running apparently smoothly and running correctly.

Since the DSA switchdev work item is the final entity that sees error
codes propagated from the driver, I find it valuable that these error
codes are not hidden in any way. I do not have access to all hardware,
I cannot foresee all code paths, but yet, users who notice errors in the
kernel log report these problems, we look into them and we fix them.
In fact, this is exactly the way in which Rafael pointed out to me the
mv88e6xxx issue, and this led to the aforementioned bridge/switchdev/DSA
rework. It is a process that works, and I would like it to not be
disrupted.

This isn't to say that there is no argument to be made for hiding these
error messages. There is a possibility that I might agree with silencing
some errors in some cases, but I have not heard a compelling argument
for that. Also, with mv88e6xxx and systemd-networkd, it isn't even the
first warning/error of its kind that gets printed by this driver out of
the blue. The driver also warns when systemd-networkd reinitializes
bridge VLANs on ports, stating that "port X already a member of VLAN y".
So I believe that mv88e6xxx users already have some tolerance for
non-fatal warnings. These latter warnings, by the way, have also
disappeared as a result of the patch set I linked in the beginning.

Therefore, I think it is in the best interest of the code base for users
to report the problems they see, instead of masking them. This means
walking through potentially known and already fixed problems/limitations,
debugging, explaining, testing, writing long emails etc, but I prefer
this to the alternative of cutting myself from a potentially valid
source of information.

Powered by blists - more mailing lists