netdev - Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation if its OF node has status = "disabled"

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] [day] [month] [year] [list]

Date: Wed, 10 Jan 2024 18:35:51 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
Cc: 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>,
	Andrew Lunn <andrew@...n.ch>,
	Florian Fainelli <f.fainelli@...il.com>,
	Alvin Šipraga <alsi@...g-olufsen.dk>,
	Linus Walleij <linus.walleij@...aro.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Hauke Mehrtens <hauke@...ke-m.de>,
	Christian Marangi <ansuelsmth@...il.com>,
	Arınç ÜNAL <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 05/10] net: dsa: qca8k: skip MDIO bus creation
 if its OF node has status = "disabled"

On Thu, Jan 04, 2024 at 11:19:20PM -0300, Luiz Angelo Daros de Luca wrote:
> Don't you still need to put the node that is not available? Just put
> it unconditionally whenever you exit this function after you get it.
> of_node_put() can handle even NULL.

You're right. I've prepared a patch to handle this case correctly.
I don't think it's worth sending to 'net' now that 'net-next' has
closed, because as you say below, it's quite possible that the
!of_device_is_available() code path is never exercised by existing
device trees. So the bug is like the tree that falls in the forest but
nobody hears it. I will submit the correction when net-next reopens,
together with Alvin's suggested "err" -> "ret" renaming.

> I'm not sure if this and other simple switches can be useful without a
> valid MDIO.

Will that always be the case? As implausible as this may sound, I've
received DSA questions from people using the sja1105 as a two-port
adapter between MII on the CPU side and RMII on the PHY side. It was the
cheapest way of adapting their SoC to RMII, using a switch as not even a
port multiplier. I see that AR8237 has 1x RGMII and 1x SerDes, so maybe
somebody would want to use it that way, and sidestep the internal PHYs?
I don't know.

> Anyway, wouldn't it be equivalent to having an empty mdio
> node? It looks like it would work as well but without a specific code
> path.

I guess you could also express this that way too. Although, in case it
matters, an 'empty node' has to pass schema validation (has to have all
required properties), and a disabled one doesn't.

The idea with this patch was to deliberately change the status = "disabled"
handling that the driver already had, to make things more consistent
across the board. Each driver binding has its own unique interpretation
of an absent MDIO OF node already. Some consider the OF node optional
and thus register it anyway, some say that absent means it's not needed.
But I think status = "disabled" should be an unambiguous way to specify
through DT that the hardware component is disabled. This is not how
qca8k was interpreting it prior to this change.