Message-ID: <20230218205204.ie6lxey65pv3mgyh@skbuf>
Date:   Sat, 18 Feb 2023 22:52:04 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Arınç ÜNAL <arinc.unal@...nc9.com>
Cc:     Frank Wunderlich <frank-w@...lic-files.de>,
        netdev <netdev@...r.kernel.org>, erkin.bozoglu@...ont.com,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: Choose a default DSA CPU port

Hi Arınç,

On Sat, Feb 18, 2023 at 08:07:53PM +0300, Arınç ÜNAL wrote:
> Hey there folks,
> 
> The problem is this. Frank and I have got a Bananapi BPI-R2 with MT7623 SoC.
> The port5 of MT7530 switch is wired to gmac1 of the SoC. Port6 is wired to
> gmac0. Since DSA sets the first CPU port it finds on the devicetree, port5
> becomes the CPU port for all DSA slaves.
> 
> But we'd prefer port6 since it uses trgmii while port5 uses rgmii. There are
> also some performance issues with the port5 - gmac1 link.
> 
> Now we could change it manually on userspace if the DSA subdriver supported
> changing the DSA master.
> 
> I'd like to find a solution which would work for the cases of; the driver
> not supporting changing the DSA master, or saving the effort of manually
> changing it on userspace.
> 
> The solution that came to my mind:
> 
> Introduce a DT property to designate a CPU port as the default CPU port.
> If this property exists on a CPU port, that port becomes the CPU port for
> all DSA slaves.
> If it doesn't exist, fallback to the first-found-cpu-port method.
> 
> Frank doesn't like this idea:
> 
> > maybe define the default cpu in driver which gets picked up by core
> (define port6 as default if available).
> > Imho additional dts-propperty is wrong approch...it should be handled by
> driver. But cpu-port-selection is currently done in dsa-core which makes it
> a bit difficult.
> 
> What are your thoughts?
> 
> Arınç

Before making any change, I believe that the first step is to be in agreement
as to what the problem is.

The current DSA device tree binding has always chosen the numerically first
CPU port as the default CPU port. This was also the case at the time when the
mt7530 driver was accepted upstream. Clearly there are cases where this choice
is not the optimal choice (from the perspective of an outside observer).
For example when another CPU port has a higher link speed. But it's not
exactly obvious that higher link speed is always better. If you have a CPU
port at a higher link speed than the user ports, but it doesn't have flow
control, then the choice is practically worse than the CPU port which operates
at the same link speed as user ports.

So the choice between RGMII and TRGMII is not immediately obvious to some code
which should take this decision automatically. And this complicates things
a lot. If there is no downside to having the kernel take a decision automatically,
generally I don't have a problem taking it. But here, I would like to hear
some strong arguments why port 6 is preferable over port 5.

If there are strong reasons to consider port 6 a primary CPU port and
port 5 a secondary one, then there is also a very valid concern of forward
compatibility between the mt7530 driver and device trees from the future
(with multiple CPU ports defined). The authors of the mt7530 driver must have
been aware of the DSA binding implementation's choice of selecting the
first CPU port as the default, but they decided to hide their head in
the sand and pretend like this issue will never crop up. The driver has
not been coded up to accept port 5 as a valid CPU port until very recently.
What should have been done (*assuming strong arguments*) is that
dsa_tree_setup_default_cpu() should have been modified from day one of
mt7530 driver introduction, such that the driver has a way of specifying
a preferred default CPU port.

In other words, the fact that the CPU port becomes port 5 when booting
on a new device tree is equally a problem for current kernels as it is
for past LTS kernels. I would like this to be treated seriously, and
current + stable kernels should behave in a reasonable manner with
device trees from the future, before support for multiple CPU ports is
added to mt7530. Forcing users to change device trees in lockstep with
the kernel is not something that I want to maintain and support, if user
questions and issues do crop up.

Since this wasn't done, the only thing we're left with is to retroactively
add this functionality to pick a preferred default CPU port, as patches
to "net" which get backported to stable kernels. Given enough forethought
in the mt7530 driver development, this should not have been necessary,
but here we are.

Now that I expressed this point of view, let me comment on why your
proposal, Arınç, solves exactly nothing.

If you add a device tree property for the preferred CPU port, you
haven't solved any of the compatibility problems:

- old kernels + new device trees will not have the logic to interpret
  that device tree property
- old device trees + new kernels will not have that device tree property

so... yeah.

Powered by blists - more mailing lists