Le Fri, 15 Apr 2022 13:55:03 +0300,
Vladimir Oltean <olteanv@xxxxxxxxx> 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@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.
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
