Message-ID: <20240508111328.ya4ydnmd6w764q5k@kandell>
Date: Wed, 8 May 2024 13:13:52 +0200
From: Marek Behún <kabel@...nel.org>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Marek Behún <kabel@...nel.org>, netdev@...r.kernel.org,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>
Subject: Re: [PATCH net-next v2 2/2] net: dsa: update the unicast MAC address
 when changing conduit

On Tue, May 07, 2024 at 11:18:27PM +0300, Vladimir Oltean wrote:
> Hi Marek,
> 
> On Thu, May 02, 2024 at 02:29:22PM +0200, Marek Behún wrote:
> > When changing DSA user interface conduit while the user interface is up,
> > DSA exhibits different behavior in comparison to when the interface is
> > down. This different behavior concers the primary unicast MAC address
> 
> nitpick: concerns
> 
> > stored in the port standalone FDB and in the conduit device UC database.
> > 
> > If we put a switch port down while changing the conduit with
> >   ip link set sw0p0 down
> >   ip link set sw0p0 type dsa conduit conduit1
> >   ip link set sw0p0 up
> > we delete the address in dsa_user_close() and install the (possibly
> > different) address in dsa_user_open().
> > 
> > But when changing the conduit on the fly, the old address is not
> > deleted and the new one is not installed.
> > 
> > Since we explicitly want to support live-changing the conduit, uninstall
> > the old address before calling dsa_port_assign_conduit() and install the
> > (possibly different) new address after the call.
> > 
> > Because conduit change might also trigger address change (the user
> > interface is supposed to inherit the conudit interface MAC address if no
> 
> nitpick: conduit
> 
> > address is defined in hardware (dp->mac is a zero address)), move the
> > eth_hw_addr_inherit() call from dsa_user_change_conduit() to
> > dsa_port_change_conduit(), just before installing the new address.
> > 
> > Fixes: 95f510d0b792 ("net: dsa: allow the DSA master to be seen and changed through rtnetlink")
> > Signed-off-by: Marek Behún <kabel@...nel.org>
> > ---
> 
> Sorry for the delay. I've tested this change and basically, while there
> is clearly a bug, that bug produces no adverse effects / cannot be
> reproduced with felix (the only mainline driver with the feature to
> change conduits). So it could be sent to 'net-next' rather that 'net' on
> that very ground, if there is no other separate reason for this to go to
> stable kernels anyway, I guess.

I did send this to net-next. The question is whether I should keep the
Fixes tag.

Marek

> There are 2 reasons why with felix the bug does not manifest itself.
> 
> First is because both the 'ocelot' and the alternate 'ocelot-8021q'
> tagging protocols have the 'promisc_on_conduit = true' flag. So the
> unicast address doesn't have to be in the conduit's RX filter - neither
> the old or the new conduit.
> 
> Second, dsa_user_host_uc_install() theoretically leaves behind host FDB
> entries installed towards the wrong (old) CPU port. But in felix_fdb_add(),
> we treat any FDB entry requested towards any CPU port as if it was a
> multicast FDB entry programmed towards _all_ CPU ports. For that reason,
> it is installed towards the port mask of the PGID_CPU port group ID:
> 
> 	if (dsa_port_is_cpu(dp))
> 		port = PGID_CPU;
> 
> It would be great if this clarification would be made in the commit
> message, to give the right impression to backporters seeking a correct
> bug impact assessment.
> 
> BTW, I'm curious how this is going to be handled with Marvell. Basically
> if all switch Ethernet interfaces have the same MAC address X which
> _isn't_ inherited from their respective conduit (so it is preserved when
> changing conduit), and you have a split conduit configuration like this:
> - half the user ports are under eth0
> - half the user ports are under eth1
> 
> then you have a situation where MAC address X needs to be programmed as
> a host FDB entry both towards the CPU port next to eth0, and towards
> that next to eth1.
> 
> There isn't any specific "core awareness" in DSA about the way in which
> host FDB entries towards multiple CPU ports are handled in the Felix case.
> So the core ends up having a not very good idea of what's happening
> behind the scenes, and basically requests a migration from the old CPU
> port to the new one, when in reality none takes place. I'm wondering how
> things are handled in your new code; maybe we need to adapt the core
> logic if there is a second implementation that's similar to felix in
> this regard. Basically I'm saying that dsa_user_host_uc_install() may
> not need to call dsa_port_standalone_host_fdb_add() when changing
> conduit, if we had dedicated DSA API for .host_fdb_add() rather than
> .port_fdb_add(port == CPU port).
> 
> Anyway, I was able to coerce the code (with extra patches) into validating
> that your patch works on a driver that hypothetically does things a bit
> differently than felix. So, with the commit message reorganized:
> 
> Reviewed-by: Vladimir Oltean <olteanv@...il.com>
> Tested-by: Vladimir Oltean <olteanv@...il.com>

Powered by blists - more mailing lists