netdev - Re: [PATCH net-next v2 5/7] net: dsa: realtek: Migrate user_mii

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]

Date: Sat, 23 Dec 2023 00:09:55 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch,
	f.fainelli@...il.com, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	arinc.unal@...nc9.com
Subject: Re: [PATCH net-next v2 5/7] net: dsa: realtek: Migrate user_mii_bus
 setup to realtek-dsa

On Fri, Dec 22, 2023 at 05:03:38PM -0300, Luiz Angelo Daros de Luca wrote:
> > Having the MDIO bus described in OF, but no phy-handle to its children
> > is a semantically broken device tree, we should make no effort whatsoever
> > to support it.
> 
> OK. I was trying to keep exactly that setup working.

Which setup exactly?

> Should I keep the check and bail out with an error like:
> 
> +       dsa_switch_for_each_user_port(dp, ds) {
> +               phy_node = of_parse_phandle(dp->dn, "phy-handle", 0);
> +               of_node_put(phy_node);
> +               if (phy_node)
> +                       continue;
> +               dev_err(priv->dev,
> +                       "'%s' is missing phy-handle",
> +                       dp->name);
> +               return -EINVAL;
> +       }
> 
> or should I simply let it break silently? The device-tree writers
> might like some feedback if they are doing it wrong. I guess neither
> DSA nor MDIO bus will say a thing about the missing phy-handle.

FWIW, it will not break silently, but like this (very easy to test, no need to guess):

[    7.196687] mscc_felix 0000:00:00.5 swp3 (uninitialized): failed to connect to PHY: -ENODEV
[    7.205168] mscc_felix 0000:00:00.5 swp3 (uninitialized): error -19 setting up PHY for tree 0, switch 0, port 3

If you have no other decision to make in the driver based on the
presence/absence of "phy-handle", it doesn't make much sense to bloat
the driver with dubious logic just to get an arguably prettier error.
I'm saying "dubious" because my understanding is that rtl8365mb "extint"
ports can also serve as user ports, and can additionally be fixed-links.
But arbitrary logic like this breaks that.

The cost/benefit ratio does not seem too favorable for this addition.