Re: [PATCH net-next 06/12] net: dsa: rzn1-a5psw: add Renesas RZ/N1 advanced 5 port switch driver

["Clément Léger" <clement.leger@bootlin.com>]

Le Fri, 15 Apr 2022 13:55:03 +0300,
Vladimir Oltean <olteanv@gmail.com> a écrit :

> On Fri, Apr 15, 2022 at 11:34:53AM +0200, Clément Léger wrote:
> > Le Thu, 14 Apr 2022 17:47:09 +0300,
> > Vladimir Oltean <olteanv@gmail.com> a écrit :  
> > > > later (vlan, etc).
> > > > 
> > > > Suggested-by: Laurent Gonzales <laurent.gonzales@non.se.com>
> > > > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@non.se.com>
> > > > Suggested-by: Phil Edworthy <phil.edworthy@renesas.com>    
> > > 
> > > Suggested? What did they suggest? "You should write a driver"?
> > > We have a Co-developed-by: tag, maybe it's more appropriate here?  
> > 
> > This driver was written from scratch but some ideas (port isolation
> > using pattern matcher) was inspired from a previous driver. I thought it
> > would be nice to give them credit for that.
> > 
> > [...]  
> 
> Ok, in that case I don't really know how to mark sources of inspiration
> in the commit message, maybe your approach is fine.
> 
> > > >  obj-y				+= hirschmann/
> > > >  obj-y				+= microchip/
> > > > diff --git a/drivers/net/dsa/rzn1_a5psw.c b/drivers/net/dsa/rzn1_a5psw.c
> > > > new file mode 100644
> > > > index 000000000000..5bee999f7050
> > > > --- /dev/null
> > > > +++ b/drivers/net/dsa/rzn1_a5psw.c
> > > > @@ -0,0 +1,676 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * Copyright (C) 2022 Schneider-Electric
> > > > + *
> > > > + * Clément Léger <clement.leger@bootlin.com>
> > > > + */
> > > > +
> > > > +#include <linux/clk.h>
> > > > +#include <linux/etherdevice.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_mdio.h>
> > > > +#include <net/dsa.h>
> > > > +#include <uapi/linux/if_bridge.h>    
> > > 
> > > Why do you need to include this header?  
> > 
> > It defines BR_STATE_* but I guess linux/if_bridge.h does include it.  
> 
> Yes.
> 
> > > > +static void a5psw_port_pattern_set(struct a5psw *a5psw, int port, int pattern,
> > > > +				   bool enable)
> > > > +{
> > > > +	u32 rx_match = 0;
> > > > +
> > > > +	if (enable)
> > > > +		rx_match |= A5PSW_RXMATCH_CONFIG_PATTERN(pattern);
> > > > +
> > > > +	a5psw_reg_rmw(a5psw, A5PSW_RXMATCH_CONFIG(port),
> > > > +		      A5PSW_RXMATCH_CONFIG_PATTERN(pattern), rx_match);
> > > > +}
> > > > +
> > > > +static void a5psw_port_mgmtfwd_set(struct a5psw *a5psw, int port, bool enable)    
> > > 
> > > Some explanation on what "management forward" means/does?  
> > 
> > I'll probably rename that cpu_port_forward to match the dsa naming.
> > It'll actually isolate the port from other ports by only forwarding the
> > packets to the CPU port.  
> 
> You could probably do without a rename by just adding a comment that
> says that it enables forwarding only towards the management port.
> 
> > > Please implement .shutdown too, it's non-optional.  
> > 
> > Hum, platform_shutdown does seems to check for the .shutdown callback:
> > 
> > static void platform_shutdown(struct device *_dev)
> > {
> > 	struct platform_device *dev = to_platform_device(_dev);
> > 	struct platform_driver *drv;
> > 
> > 	if (!_dev->driver)
> > 		return;
> > 
> > 	drv = to_platform_driver(_dev->driver);
> > 	if (drv->shutdown)
> > 		drv->shutdown(dev);
> > }
> > 
> > Is there some documentation specifying that this is mandatory ?
> > If so, should I just set it to point to an empty shutdown function then
> > ?  
> 
> I meant that for a DSA switch driver is mandatory to call dsa_switch_shutdown()
> from your ->shutdown method, otherwise subtle things break, sorry for being unclear.
> 
> Please blindly copy-paste the odd pattern that all other DSA drivers use
> in ->shutdown and ->remove (with the platform_set_drvdata(dev, NULL) calls),
> like a normal person :)
> 
> > > > + * @reg_lock: Lock for register read-modify-write operation    
> > > 
> > > Interesting concept. Generally we see higher-level locking schemes
> > > (i.e. a rmw lock won't really ensure much in terms of consistency of
> > > settings if that's the only thing that serializes concurrent thread
> > > accesses to some register).  
> > 
> > Agreed, this does not guarantee consistency of settings but guarantees
> > that rmw modifications are atomic between devices. I wasn't sure about
> > the locking guarantee that I could have. After looking at other
> > drivers, I guess I will switch to something more common such as using
> > a global mutex for register accesses.  
> 
> LOL, that isn't better...
> 
> Ideally locking would be done per functionality that the hardware can
> perform independently (like lookup table access, VLAN table access,
> forwarding domain control, PTP block, link state control, etc).
> You don't want to artificially serialize unrelated stuff.
> A "read-modify-write" lock would similarly artificially serialize
> unrelated stuff for you, even if you intend it to only serialize
> something entirely different.
> 
> Most things as seen by a DSA switch driver are implicitly serialized by
> the rtnl_mutex anyway. 

Is there a list of the functions that are protected by the RTNL lock
without having to deep dive in the whole stacks ? That would be really
useful to remove useless locking from my driver. But I guess I'll have
to look at other drivers to see that.

> Some things aren't (->port_fdb_add, ->port_fdb_del).

Ok, looks like for them a mutex is often used which also seems more
appropriate in my case since the operation on the learn table can take
some times.

> There is a point to be made about adding locks for stuff that is
> implicitly serialized by the rtnl_mutex, since you can't really test
> their effectiveness. This makes it more difficult for the driver writer
> to make the right decision about locking, since in some cases, the
> serialization given by the rtnl_mutex isn't something fundamental and
> may be removed, to reduce contention on that lock. In that case, it is
> always a nice surprise to find a backup locking scheme in converted
> drivers. With the mention that said backup locking scheme was never
> really tested, so it may be that it needs further work anyway.

Ok understood.

> 
> > > The selftests don't cover nearly enough, but just to make sure that they
> > > pass for your switch, when you use 2 switch ports as h1 and h2 (hosts),
> > > and 2 ports as swp1 and swp2? There's surprisingly little that you do on
> > > .port_bridge_join, I need to study the code more.  
> > 
> > Port isolation is handled by using a pattern matcher which is enabled
> > for each port at setup. If set, the port packet will only be forwarded
> > to the CPU port. When bridging is needed, the pattern matching is
> > disabled and thus, the packets are forwarded between all the ports that
> > are enabled in the bridge.  
> 
> Is there some public documentation for this pattern matcher?

Yes, the manual is public [1]. See the Advanced 5 Ports Switch section.

[1]
https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-r-engine-and-ethernet-peripherals

-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com