Message-ID: <20220706152923.mhc7vw7xkr7xkot4@skbuf>
Date:   Wed, 6 Jul 2022 18:29:23 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc:     Alvin Šipraga <ALSI@...g-olufsen.dk>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
        Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next RFC 0/3] net: dsa: realtek: drop custom slave MII

On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote:
> > Hi Luiz,
> >
> > On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote:
> > > This RFC patch series cleans realtek-smi custom slave mii bus. Since
> > > fe7324b932, dsa generic code provides everything needed for
> > > realtek-smi driver. For extra caution, this series should be applied
> > > in two steps: the first 2 patches introduce the new code path that
> > > uses dsa generic code. It will show a warning message if the tree
> > > contains deprecated references. It will still fall back to the old
> > > code path if an "mdio"
> > > is not found.
> >
> > In principle I like your changes, but I'm not sure if what you are doing
> > is allowed, since DT is ABI. The fact that you have to split this into
> > two steps, with the first step warning about old "incompatible" DTs
> > (your point 3 below) before the second step breaks that compatibility,
> > suggests that you are aware that you could be breaking old DTs.
> 
> Thanks Alvin for your review. Yes, that is a good question for the ML.
> I don't know at what level we can break compatibility (DT and driver).
> That's why it is a RFC.

DT bindings are only extended in backwards-compatible ways. Only in the
case where you can prove that there is no DT user of a certain binding,
and that none should appear either, is when you can consider breaking
the backward compatibility. The idea here is that old DT blobs may live
forever and be provided by fixed firmware such as U-Boot, you can't
really force anyone to update them.

> > I'm not going to argue with you if you say "but the node with compatible
> > realtek,smi-mdio was also called mdio in the bindings, so it shouldn't
> > break old DTs", which is a valid point. But if that is your rationale,
> > then there's no need to split the series at all, right?
> 
> The DT requires "realtek,smi-mdio" but also mentions the "mdio" name,
> not a generic name as "mdioX". If we agree that the name "mdio" is
> already required by the DT bindings, it is the driver implementation
> that is not compliant. Even if we are not violating the DT bindings,
> we are changing the driver behavior. That's why I suggested the
> transition process. I do believe that it would be very, very rare to
> name that mdio as anything other than "mdio" and even the driver
> itself is too fresh to be widespread. In a non-RFC series, I would
> also drop the "realtek,smi-mdio" compatible string from the bindings
> (as it is back compatible).

Technically the MDIO node should be named "mdio", at least that's what
Documentation/devicetree/bindings/net/mdio.yaml says, that it should be
"mdio" or "mdio@...ething". And since the #address-cells of the DSA
switch OF node itself (parent of MDIO node) is 0, it can't really be
"mdio@...ething", so that leaves us with "mdio".

However I don't think there is any validation for the node name.

> > If you want to avoid that debate, what you could do instead is add a
> > const char *slave_mii_compatible; member to struct dsa_switch, and try
> > searching in dsa_switch_setup() for a child node with that compatible if
> > the lookup of a node named "mdio" fails. I don't know if this would help
> > you do the same thing with other drivers.
> 
> The DSA change to accept "mdio" was an improvement to avoid adding a
> custom slave mdio when you already have a single mdio and just need to
> point to a DT node. Adding compatible strings for that situation does
> not make much sense as a compatible string is not necessary when you
> are already restricting your case to a single mdio. For more complex
> setups, you still need to create your own slave mdio implementation.
> Some drivers already depend on the "mdio" name and this series is also
> a suggestion for them to try their drivers dropping their custom slave
> mdio implementations.

I think we are going down a slippery slope here. DSA providing generic
code to register an MDIO bus is not something I'd consider core
functionality, and I don't consider any duplication of concerns or logic
if drivers register their own MDIO buses.

Here you are effectively proposing to gratuitously break the DT binding,
the obvious question is, what is there to even gain from this?

And why do you even need to remove the compatible string from the MDIO
node, can't you just ignore it, does it bother you in any way?

Powered by blists - more mailing lists