Linux Ethernet Bridging - Oct 2022 - [Bridge] [PATCH v8 net-next 05/12] net: dsa: propagate the locked flag down through the DSA layer

On Thu, Oct 20, 2022 at 04:35:06PM +0300, Vladimir Oltean
wrote:
> On Thu, Oct 20, 2022 at 04:24:16PM +0300, Ido Schimmel wrote:
> > On Thu, Oct 20, 2022 at 04:02:24PM +0300, Vladimir Oltean wrote:
> > > On Tue, Oct 18, 2022 at 06:56:12PM +0200, Hans J. Schultz wrote:
> > > > @@ -3315,6 +3316,7 @@ static int dsa_slave_fdb_event(struct
net_device *dev,
> > > >  	struct dsa_port *dp = dsa_slave_to_port(dev);
> > > >  	bool host_addr = fdb_info->is_local;
> > > >  	struct dsa_switch *ds = dp->ds;
> > > > +	u16 fdb_flags = 0;
> > > >  
> > > >  	if (ctx && ctx != dp)
> > > >  		return 0;
> > > > @@ -3361,6 +3363,9 @@ static int dsa_slave_fdb_event(struct
net_device *dev,
> > > >  		   orig_dev->name, fdb_info->addr,
fdb_info->vid,
> > > >  		   host_addr ? " as host address" :
"");
> > > >  
> > > > +	if (fdb_info->locked)
> > > > +		fdb_flags |= DSA_FDB_FLAG_LOCKED;
> > > 
> > > This is the bridge->driver direction. In which of the changes
up until
> > > now/through which mechanism will the bridge emit a
> > > SWITCHDEV_FDB_ADD_TO_DEVICE with fdb_info->locked = true?
> > 
> > I believe it can happen in the following call chain:
> > 
> > br_handle_frame_finish
> >    br_fdb_update // p->flags & BR_PORT_MAB
> >        fdb_notify
> >            br_switchdev_fdb_notify
> > 
> > This can happen with Spectrum when a packet ingresses via a locked
port
> > and incurs an FDB miss in hardware. The packet will be trapped and
> > injected to the Rx path where it should invoke the above call chain.
> 
> Ah, so this is the case which in mv88e6xxx would generate an ATU
> violation interrupt; in the Spectrum case it generates a special packet.
Not sure what you mean by "special" :) It's simply the packet that
incurred the FDB miss on the SMAC.
> Right now this packet isn't generated, right?
Right. We don't support BR_PORT_LOCKED so these checks are not currently
enabled in hardware. To be clear, only packets received via locked ports
are able to trigger the check.
> 
> I think we have the same thing in ocelot, a port can be configured to
> send "learn frames" to the CPU.
> 
> Should these packets be injected into the bridge RX path in the first
> place? They reach the CPU because of an FDB miss, not because the CPU
> was the intended destination.
The reason to inject them to the Rx path is so that they will trigger
the creation of the "locked" entry in the bridge driver (when MAB is
on), thereby notifying user space about the presence of a new MAC behind
the locked port. We can try to parse them in the driver and notify the
bridge driver via SWITCHDEV_FDB_ADD_TO_BRIDGE, but it's quite ugly...

On Thu, Oct 20, 2022 at 04:57:35PM +0300, Ido Schimmel
wrote:
> On Thu, Oct 20, 2022 at 04:35:06PM +0300, Vladimir Oltean wrote:
> > On Thu, Oct 20, 2022 at 04:24:16PM +0300, Ido Schimmel wrote:
> > > On Thu, Oct 20, 2022 at 04:02:24PM +0300, Vladimir Oltean wrote:
> > > > On Tue, Oct 18, 2022 at 06:56:12PM +0200, Hans J. Schultz
wrote:
> > > > > @@ -3315,6 +3316,7 @@ static int
dsa_slave_fdb_event(struct net_device *dev,
> > > > >  	struct dsa_port *dp = dsa_slave_to_port(dev);
> > > > >  	bool host_addr = fdb_info->is_local;
> > > > >  	struct dsa_switch *ds = dp->ds;
> > > > > +	u16 fdb_flags = 0;
> > > > >  
> > > > >  	if (ctx && ctx != dp)
> > > > >  		return 0;
> > > > > @@ -3361,6 +3363,9 @@ static int
dsa_slave_fdb_event(struct net_device *dev,
> > > > >  		   orig_dev->name, fdb_info->addr,
fdb_info->vid,
> > > > >  		   host_addr ? " as host address" :
"");
> > > > >  
> > > > > +	if (fdb_info->locked)
> > > > > +		fdb_flags |= DSA_FDB_FLAG_LOCKED;
> > > > 
> > > > This is the bridge->driver direction. In which of the
changes up until
> > > > now/through which mechanism will the bridge emit a
> > > > SWITCHDEV_FDB_ADD_TO_DEVICE with fdb_info->locked = true?
> > > 
> > > I believe it can happen in the following call chain:
> > > 
> > > br_handle_frame_finish
> > >    br_fdb_update // p->flags & BR_PORT_MAB
> > >        fdb_notify
> > >            br_switchdev_fdb_notify
> > > 
> > > This can happen with Spectrum when a packet ingresses via a
locked port
> > > and incurs an FDB miss in hardware. The packet will be trapped
and
> > > injected to the Rx path where it should invoke the above call
chain.
> > 
> > Ah, so this is the case which in mv88e6xxx would generate an ATU
> > violation interrupt; in the Spectrum case it generates a special
packet.
> 
> Not sure what you mean by "special" :) It's simply the packet
that
> incurred the FDB miss on the SMAC.
> 
> > Right now this packet isn't generated, right?
> 
> Right. We don't support BR_PORT_LOCKED so these checks are not
currently
> enabled in hardware. To be clear, only packets received via locked ports
> are able to trigger the check.
> 
> > 
> > I think we have the same thing in ocelot, a port can be configured to
> > send "learn frames" to the CPU.
> > 
> > Should these packets be injected into the bridge RX path in the first
> > place? They reach the CPU because of an FDB miss, not because the CPU
> > was the intended destination.
> 
> The reason to inject them to the Rx path is so that they will trigger
> the creation of the "locked" entry in the bridge driver (when MAB
is
> on), thereby notifying user space about the presence of a new MAC behind
> the locked port. We can try to parse them in the driver and notify the
> bridge driver via SWITCHDEV_FDB_ADD_TO_BRIDGE, but it's quite ugly...
"ugly" => your words, not mine... But abstracting things a bit,
doing
what you just said (SWITCHDEV_FDB_ADD_TO_BRIDGE) for learn frames would
be exactly the same thing as what mv88e6xxx is doing (so your "ugly"
comment equally applies to Marvell). The learn frames are "special" in
the sense that they don't belong to the data path of the software
bridge*, they are just hardware specific information which the driver
must deal with, using a channel that happens to be Ethernet and not an
IRQ/MDIO.

*in other words, a bridge with proper RX filtering should not even
receive these frames, or would need special casing for BR_PORT_MAB to
not drop them in the first place.

I would incline towards an unified approach for CPU assisted learning,
regardless of this (minor, IMO) difference between Marvell and other
vendors.

On Thu, Oct 20, 2022 at 04:57:35PM +0300, Ido Schimmel
wrote:
> > Right now this packet isn't generated, right?
> 
> Right. We don't support BR_PORT_LOCKED so these checks are not
currently
> enabled in hardware. To be clear, only packets received via locked ports
> are able to trigger the check.
You mean BR_PORT_MAB, not BR_PORT_LOCKED, right? AFAIU, "locked" means
drop unknown MAC SA, "mab" means "install BR_FDB_LOCKED entry on
port"
(and also maybe still drop, if "locked" is also set on port).

Sad there isn't any good documentation about these flags in the patches
that Hans is proposing.