netdev - Re: [PATCH net v2 2/7] net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530

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]
Message-ID: <20230613204655.7dk5f5pbcyrquva5@skbuf>
Date: Tue, 13 Jun 2023 23:46:55 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Arınç ÜNAL <arinc.unal@...nc9.com>,
	Daniel Golle <daniel@...rotopia.org>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Frank Wunderlich <frank-w@...lic-files.de>,
	Bartel Eerdekens <bartel.eerdekens@...stell8.be>,
	mithat.guner@...ont.com, erkin.bozoglu@...ont.com,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net v2 2/7] net: dsa: mt7530: fix trapping frames with
 multiple CPU ports on MT7530

On Tue, Jun 13, 2023 at 07:29:18PM +0100, Russell King (Oracle) wrote:
> Maybe it's just me being dumb, but I keep finding things difficult to
> understand, such as the above paragraph.
> 
> It sounds like you're saying that _before_ this patch, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* NOT BITS are set such that
> port 6 is the active CPU port. Therefore, things are broken, and this
> patch fixes it.
> 
> Or are you saying that after this patch is applied, port 5 is the
> active CPU port, but the CPU_PORT *FIELD* is set to port 6. If that's
> true, then I've no idea what the hell is going on here because it
> seems to be senseless.

There are 2 distinct patches at play. I'll be showing some tables below.
Their legend is that patch (1) affects only the second column and patch
(2) affects only the third column.

Patch (1): net: dsa: mt7530: fix trapping frames with multiple CPU ports on MT7530
----------------------------------------------------------------------------------

What you need to know looking at the current code in net-next is that
mt753x_cpu_port_enable() always overwrites the CPU_MASK field of MT7530_MFC,
because that contains a single port. So when operating on a device tree
with multiple CPU ports defined, only the last CPU port will be recorded
in that register, and this will affect the destination port for
trapped-to-CPU frames.

However, DSA, when operating on a DT with multiple CPU ports, makes the
dp->cpu_dp pointer of all user ports equal to the first CPU port. That
affects which DSA master is automatically brought up when user ports are
brought up. Minor issue, TBH, because it is sufficient for the user to
manually bring up the DSA master corresponding to the second CPU port,
and then trapped frames would be processed just fine. Prior to ~2021/v5.12,
that facility wasn't even a thing - the user always had to bring that
interface up manually.

That means that without patch (1) we have:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        5

The semi-problem is that the DSA master of the trapping port (6) is not
automatically brought up by the dsa_slave_open() logic, because no slave
has port 6 (the trapping port) as a master.

All that this patch is doing is that it moves around the trapping CPU
port towards one of the DSA masters that is up, so that the user doesn't
really need to care. The table becomes:

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
--------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             5                        5


Patch (2) net: dsa: introduce preferred_default_local_cpu_port and use on MT7530
--------------------------------------------------------------------------------

This patch influences the choice that DSA makes when it comes to the
dp->cpu_dp assignments of user ports, when fed a device tree with
multiple CPU ports.

The preference of the mt7530 driver is: if port 6 is defined in the DT
as a CPU port, choose that. Otherwise don't care (which implicitly means:
let DSA pick the first CPU port that is defined there, be it 5 or 6).

The effect of this patch taken in isolation is (showing only "after",
because "before" is the same as the "before" of patch 1):

CPU ports in the    Trapping CPU port        Default CPU port
device tree         (MT7530_MFC:CPU_MASK)    (all dp->cpu_dp point to it)
-------------------------------------------------------------------------
5                   5                        5
6                   6                        6
5 and 6             6                        6

As you can see, patch (2) resolves the same semi-problem even in
isolation because it makes the trapping port coincide with the default
CPU port in a different way.

> 
> I think at this point I just give up trying to understand what the
> hell these patches are trying to do - in my opinion, the commit
> messages are worded attrociously and incomprehensively.

+1, could have been better..