Message-ID: <20220524132435.vj35shnzbrtw3ikz@skbuf>
Date:   Tue, 24 May 2022 16:24:35 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Ansuel Smith <ansuelsmth@...il.com>
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Tobias Waldekranz <tobias@...dekranz.com>,
        Marek Behún <kabel@...nel.org>,
        DENG Qingfang <dqfext@...il.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "UNGLinuxDriver@...rochip.com" <UNGLinuxDriver@...rochip.com>,
        Colin Foster <colin.foster@...advantage.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Luiz Angelo Daros de Luca <luizluca@...il.com>,
        Roopa Prabhu <roopa@...dia.com>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Frank Wunderlich <frank-w@...lic-files.de>
Subject: Re: [RFC PATCH net-next 00/12] DSA changes for multiple CPU ports
 (part 3)

On Tue, May 24, 2022 at 02:38:53PM +0200, Ansuel Smith wrote:
> On Tue, May 24, 2022 at 12:29:06PM +0000, Vladimir Oltean wrote:
> > On Tue, May 24, 2022 at 02:02:19PM +0200, Ansuel Smith wrote:
> > > Probably offtopic but I wonder if the use of a LAG as master can
> > > cause some problem with configuration where the switch use a mgmt port
> > > to send settings. Wonder if with this change we will have to introduce
> > > an additional value to declare a management port that will be used since
> > > master can now be set to various values. Or just the driver will have to
> > > handle this with its priv struct (think this is the correct solution)
> > > 
> > > I still have to find time to test this with qca8k.
> > 
> > Not offtopic, this is a good point. dsa_tree_master_admin_state_change()
> > and dsa_tree_master_oper_state_change() set various flags in cpu_dp =
> > master->dsa_ptr. It's unclear if the cpu_dp we assign to a LAG should
> > track the admin/oper state of the LAG itself or of the physical port.
> > Especially since the lag->dsa_ptr is the same as one of the master->dsa_ptr.
> > It's clear that the same structure can't track both states. I'm thinking
> > we should suppress the NETDEV_CHANGE and NETDEV_UP monitoring from slave.c
> > on LAG DSA masters, and track only the physical ones. In any case,
> > management traffic does not really benefit from being sent/received over
> > a LAG, and I'm thinking we should just use the physical port.
> > Your qca8k_master_change() function explicitly only checks for CPU port
> > 0, which in retrospect was a very wise decision in terms of forward
> > compatibility with device trees with multiple CPU ports.
> 
> Switch can also have some hw limitation where mgmt packet are accepted
> only by one specific port and I assume using a LAG with load balance can
> cause some problem (packet not ack).
> 
> Yes I think the oper_state_change would be problematic with a LAG
> configuration since the driver should use the pysical port anyway (to
> prevent any hw limitation/issue) and track only that.
> 
> But I think we can put that on hold and think of a correct solution when
> we have a solid base with all of this implemented. Considering qca8k
> is the only user of that feature and things will have to change anyway
> when qca8k will get support for multiple cpu port, we can address that
> later. (in theory everything should work correctly if qca8k doesn't
> declare multiple cpu port or a LAG is not confugred) 

Consider this - the way in which DSA tracks the state of DSA masters
already "supports multiple [ physical ] CPU ports". It's just a matter
of driver writers acknowledging this and doing the right thing in the
ds->ops->master_state_change() callback. DSA tells us when any physical
master goes up or down, and it does this regardless of whether that
master's dsa_ptr is the dp->cpu_dp of any user port. Otherwise said,
given this device tree snippet:

eth0: ethernet@0 {
	...
};

eth1: ethernet@1 {
	...
};

ethernet-switch@0 {
	ethernet-ports {
		ethernet-port@0 {
			label = "swp0";
		};

		ethernet-port@1 {
			label = "swp1";
		};

		ethernet-port@2 {
			ethernet = <&eth0>;
		};

		ethernet-port@3 {
			ethernet = <&eth1>;
		};
	};
};

Current mainline DSA will create swp0@...0 and swp1@...0, but it will
call dsa_master_setup(eth0) and dsa_master_setup(eth1). Then it will
monitor the state of both eth0 and eth1, and pass updates to both
masters' states down to the driver.

It is therefore the responsibility of the driver to ensure forward
compatibility with multiple CPU ports (otherwise said, if one master
goes down, don't hurry to say "I don't have any management interface to
use for register access" - maybe the other one is still ok).
Consequently, you can use a DSA master for register access even if no
dp->cpu_dp points to it. This patch set is just about changing the
dp->cpu_dp mapping that is used for netdevice traffic, which is quite
orthogonal to the concern you describe.

Powered by blists - more mailing lists