Message-ID: <20240913185053.rr23ym5otprgiphb@skbuf>
Date: Fri, 13 Sep 2024 21:50:53 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Andrew Lunn <andrew@...n.ch>
Cc: Conor Dooley <conor@...nel.org>, netdev@...r.kernel.org,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Florian Fainelli <f.fainelli@...il.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next 2/4] dt-bindings: net: dsa: the adjacent DSA
 port must appear first in "link" property

Hi Conor, Andrew,

Thanks for taking a look at the patch.

On Fri, Sep 13, 2024 at 07:26:49PM +0200, Andrew Lunn wrote:
> On Fri, Sep 13, 2024 at 06:04:17PM +0100, Conor Dooley wrote:
> > On Fri, Sep 13, 2024 at 04:15:05PM +0300, Vladimir Oltean wrote:
> > > If we don't add something along these lines, it is absolutely impossible
> > > to know, for trees with 3 or more switches, which links represent direct
> > > connections and which don't.
> > > 
> > > I've studied existing mainline device trees, and it seems that the rule
> > > has been respected thus far. I've actually tested such a 3-switch setup
> > > with the Turris MOX.
> > 
> > What about out of tree (so in u-boot or the likes)? Are there other
> > users that we need to care about?

In U-Boot there is only armada-3720-turris-mox.dts which is in sync with
Linux as far as I'm aware. Also, we don't really support cascade ports
in U-Boot DM_DSA yet. So all device trees in U-Boot should be coming
from Linux. I'm not aware of other device tree users, sadly.

> > This doesn't really seem like an ABI change, if this is the established
> > convention, but feels like a fixes tag and backports to stable etc are
> > in order to me.
> 
> Looking at the next patch, it does not appear to change the behaviour
> of existing drivers, it just adds additional information a driver may
> choose to use.
> 
> As Vladimir says, all existing instances already appear to be in this
> order, but that is just happen stance, because it does not matter. So
> i would be reluctant to say there is an established convention.

Yes, indeed, it's not a convention yet. However, it is a convention I
would really like to establish, based on the practice I have seen.

> The mv88e6xxx driver does not care about the order of the list, and
> this patch series does not appear to change that. So i'm O.K. with the
> code changes.
> 
> > > -      Should be a list of phandles to other switch's DSA port. This
> > > -      port is used as the outgoing port towards the phandle ports. The
> > > -      full routing information must be given, not just the one hop
> > > -      routes to neighbouring switches
> > > +      Should be a list of phandles to other switch's DSA port. This port is
> > > +      used as the outgoing port towards the phandle ports. In case of trees
> > > +      with more than 2 switches, the full routing information must be given.
> > > +      The first element of the list must be the directly connected DSA port
> > > +      of the adjacent switch.
> 
> I would prefer to weaken this, just to avoid future backwards
> compatibility issues. `must` is too strong, because mv88e6xxx does not
> care, and there could be DT blobs out there which do not conform to
> this. Maybe 'For the SJA1105, the first element ...".
> 
> What i don't want is some future developer reading this, assume it is
> actually true when it maybe is not, and making use of it in the
> mv88e6xxx or the core, breaking backwards compatibility.

Backward compatibility issues aside, the way dp->link_dp is populated
can _only_ be done based on the assumption that this is true.

I'm afraid that any verb less strong than "must" would be insufficient
for my patch 3/4.

I have unpublished downstream patches which even make all the rest of
the "link = <...>" elements optional. Bottom line, only the direct
connection between ports (first element) represents hardware description.
The other reachable ports (the routing table, practically speaking) can be*
computed based on breadth-first search at DSA probe time. They are
listed in the device tree for "convenience".

*and IMO, also should be. For a 3-switch daisy chain, there are 8 links
to describe, and for a 4-switch daisy chain, there are 22 links, if my
math is correct. I think it's unreasonable to expect that board DT
writers won't make mistakes in listing this amount of elements, rather
than just concentrating on the physically relevant info - the direct
connection.

Baking the assumption proposed here into the binding now makes the BFS
algorithm perfectly implementable, and the binding much more scalable.

Do you have reasonable concerns that there exist device trees which are
written differently than "first 'link' element is the direct connection"?

Powered by blists - more mailing lists