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

[Vladimir Oltean]

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@xxxxxxxxx> a écrit :
> > > later (vlan, etc).
> > > 
> > > Suggested-by: Laurent Gonzales <laurent.gonzales@xxxxxxxxxx>
> > > Suggested-by: Jean-Pierre Geslin <jean-pierre.geslin@xxxxxxxxxx>
> > > Suggested-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>  
> > 
> > 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@xxxxxxxxxxx>
> > > + */
> > > +
> > > +#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. Some things aren't (->port_fdb_add, ->port_fdb_del).
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.

> > 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?