linux-kernel - Re: [PATCH net-next v4 6/7] dt-bindings: marvell: Rewrite MV88E6xxx in schema

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: <4da86f94-c272-4e45-bd1d-bd52d0a01a81@arinc9.com>
Date:   Fri, 20 Oct 2023 12:13:10 +0300
From:   Arınç ÜNAL <arinc.unal@...nc9.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Marek Behún <kabel@...nel.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Gregory Clement <gregory.clement@...tlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        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>,
        Christian Marangi <ansuelsmth@...il.com>,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v4 6/7] dt-bindings: marvell: Rewrite MV88E6xxx
 in schema

On 19.10.2023 18:35, Vladimir Oltean wrote:
> Yikes, both these examples are actually broken, for a reason that was
> extensively discussed with Arınç in the past, and that he tried to
> automatically detect through dt-schema but was ultimately told it's too
> complicated.
> https://patchwork.kernel.org/project/netdevbpf/cover/20230916110902.234273-1-arinc.unal@arinc9.com/

True, and then I've figured that that approach was wrong from the start.
Read below.

> 
> Long story short: the "mdio" node is also the ds->slave_mii_bus (soon to
> be ds->user_mii_bus after Florian's inclusivity changes). Having a
> slave_mii_bus makes DSA know what to do with port nodes like this, which
> don't have an explicit phy-handle:
> 
> 	port@3 {
> 	    reg = <3>;
> 	    label = "lan4";
> 	};
> 
> but actually, call phy_connect() on the ds->slave_mii_bus at address 3
> (the port "reg").
> 
> The issue is that phy_connect() won't work if ds->slave_mii_bus has an
> OF presence, and ethernet-phy@3 isn't defined under it (which it isn't,
> you only put ethernet-phy@9). The super detailed reason is that the
> OF-based __of_mdiobus_register() does this:
> 
> 	/* Mask out all PHYs from auto probing.  Instead the PHYs listed in
> 	 * the device tree are populated after the bus has been registered */
> 	mdio->phy_mask = ~0;
> 
> So either:
> 
> - you delete the "mdio" node and the ethernet-phys under it, or
> - you add all ethernet-phys under the mdio node, and put phy-handles
>    from ports to each of them, and phy-modes of "internal"
> 
> What you have now is exactly what won't work, i.e. an OF-based
> slave_mii_bus with a non-OF-based phy_connect().
> 
> I don't want to see DT examples that are structurally broken, sorry,
> because then we wonder why users are confused.
> 
> Personally, I would opt for adding the more modern explicit phy-handle
> and phy-mode everywhere. Those also work with the U-Boot DM_DSA driver.

As can be seen on the conversation on the patch series you've referred to
above, this is ultimately what I've proposed to enforce on all ethernet
controllers (I hadn't mentioned phy-mode there yet). As long as Andrew
holds his stance, this won't happen. The whole conversation on that patch
series is a great read as to how a devicetree should be defined and how
drivers should interact with it.

But sure, they can at least be put on the examples for this specific
schema.

Arınç