linux-kernel - Re: [PATCH net-next] net: dsa: mt7530: register OF node for internal MDIO bus

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: Wed, 3 Jan 2024 21:02:46 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Arınç ÜNAL <arinc.unal@...nc9.com>
Cc: Daniel Golle <daniel@...rotopia.org>,
	Landen Chao <Landen.Chao@...iatek.com>,
	DENG Qingfang <dqfext@...il.com>,
	Sean Wang <sean.wang@...iatek.com>, Andrew Lunn <andrew@...n.ch>,
	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>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	David Bauer <mail@...id-bauer.net>, mithat.guner@...ont.com,
	erkin.bozoglu@...ont.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH net-next] net: dsa: mt7530: register OF node for internal
 MDIO bus

On Thu, Dec 28, 2023 at 07:58:13PM +0300, Arınç ÜNAL wrote:
> As Daniel stated on a previous submission of this patch, being able to
> reference the PHYs on the switch MDIO bus is mandatory on MT7988 as
> calibration data from NVMEM for each PHY is required, so defining the MDIO
> bus is required to support MT7988. Therefore, we should support interrupts
> on device trees with the switch MDIO bus defined.

Understood and no objection there. I was just making sure that there is
no existing case in upstream where the internal PHYs are described in OF,
that we'd have to preserve IRQ functionality for.

> The implementation below follows this logic:
> 
> No switch MDIO bus defined: Register the MDIO bus, set the interrupts for
> PHYs if "interrupt-controller" is defined at the switch node.
> 
> Switch MDIO bus defined: Register the MDIO bus, set the interrupts for PHYs
> if ["interrupt-controller" is defined at the switch node and "interrupts"
> is defined at the PHY nodes under the switch MDIO bus node].
> 
> I think this approach fits your description so I'd like to agree that this
> should be the way for all DSA subdrivers. Please let me know what you
> think.
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 391c4dbdff42..bbd230a73ead 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  {
>  	struct dsa_switch *ds = priv->ds;
>  	struct device *dev = priv->dev;
> +	struct device_node *np, *mnp;
>  	struct mii_bus *bus;
>  	static int idx;
>  	int ret;
> +	np = priv->dev->of_node;
> +	mnp = of_get_child_by_name(np, "mdio");
> +

Empty line between variable declarations and code. Or you can initialize
them as part of their declaration, but you need to stick to the "longest
line first" rule.

Also, it would be good to also check of_device_is_available(mnp).

>  	bus = devm_mdiobus_alloc(dev);
>  	if (!bus)
>  		return -ENOMEM;
> -	ds->user_mii_bus = bus;
> +	if (mnp == NULL)

!mnp

> +		ds->user_mii_bus = bus;
> +
>  	bus->priv = priv;
>  	bus->name = KBUILD_MODNAME "-mii";
>  	snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++);
> @@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv)
>  	bus->parent = dev;
>  	bus->phy_mask = ~ds->phys_mii_mask;
> -	if (priv->irq)
> +	if (priv->irq && mnp == NULL)
>  		mt7530_setup_mdio_irq(priv);
> -	ret = devm_mdiobus_register(dev, bus);
> +	ret = devm_of_mdiobus_register(dev, bus, mnp);
> +	of_node_put(mnp);

This is going to be interesting. There isn't really a correct way to
manage the reference to "mnp", as far as I can tell. Normally, it should
have been possible to release the reference as you did. But you need
something along the lines of what Luiz/Russell have been discussing
here:

https://lore.kernel.org/netdev/20231220045228.27079-2-luizluca@gmail.com/

In any case, the devres variant of of_mdiobus_register() seems incompatible
with the mt7530 driver owning the "mnp" node for any longer than this,
because it has no hook to call of_node_put() once the MDIO bus is unregistered.

>  	if (ret) {
>  		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
>  		if (priv->irq)
> 
> With this device tree:
> 
> switch {
> 	interrupt-controller;
> }
> 
> [    1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> [    1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> [    1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> [    1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> [    1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)
> 
> With this device tree:
> 
> switch {
> 	interrupt-controller;
> 
> 	mdio {
> 		phy {
> 			reg = <0>;
> 		}
> 	}
> }
> 
> [    1.413101] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    1.429954] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    1.443704] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    1.455876] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> [    1.468079] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> 
> With this device tree:
> 
> switch {
> 	interrupt-controller;
> 
> 	mdio {
> 		phy {
> 			reg = <0>;
> 			interrupts = <0>;
> 		}
> 	}
> }
> 
> [    1.420534] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=17)
> [    1.433224] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=18)
> [    1.445338] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=19)
> [    1.457472] mt7530-mdio mdio-bus:1f lan4 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=20)
> [    1.469587] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=21)

Looks sane.

FWIW, I found Documentation/devicetree/bindings/net/dsa/microchip,lan937x.yaml
where internal PHYs don't have an 'interrupts' property, yet they are
probably still expected to use interrupts - according to ksz_irq_phy_setup().

Anyway, what's done is done, but I still don't see the point of making
the binding much more flexible than it needs to be.