netdev - Re: [RFC PATCH net-next 00/12] DSA changes for multiple CPU ports (part 3)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 May 2022 14:38:53 +0200
From:   Ansuel Smith <ansuelsmth@...il.com>
To:     Vladimir Oltean <vladimir.oltean@....com>
Cc:     Vladimir Oltean <olteanv@...il.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 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) 

-- 
	Ansuel