netdev - Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_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, 30 Dec 2023 10:18:57 +0300
From: Arınç ÜNAL <arinc.unal@...nc9.com>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>, netdev@...r.kernel.org
Cc: linus.walleij@...aro.org, alsi@...g-olufsen.dk, andrew@...n.ch,
 f.fainelli@...il.com, olteanv@...il.com, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Subject: Re: [PATCH net-next v3 8/8] Revert "net: dsa: OF-ware slave_mii_bus"

On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote:
> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1.
> 
> The use of user_mii_bus is inappropriate when the hardware is described
> with a device-tree [1].
> 
> Since all drivers currently implementing ds_switch_ops.phy_{read,write}
> were not updated to utilize the MDIO information from OF with the
> generic "dsa user mii", they might not be affected by this change.
> 
> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u

This doesn't make sense to me: "The use of user_mii_bus is inappropriate
when the hardware is described with a device-tree." I think this is the
correct statement: "The use of user_mii_bus is inappropriate when the MDIO
bus of the switch is described on the device-tree."

The patch log is also not clear on why this leads to the removal of
OF-based registration from the DSA core driver.

I would change the patch log here to something like this:

net: dsa: do not populate user_mii_bus when switch MDIO bus is described

The use of ds->user_mii_bus is inappropriate when the MDIO bus of the
switch is described on the device-tree [1].

To keep things simple, make [all subdrivers that control a switch [with
MDIO bus] probed on OF] register the bus on the subdriver.

The subdrivers that control switches [with MDIO bus] probed on OF must
follow this logic to support all cases properly:

No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO
bus, set the interrupts for PHYs if "interrupt-controller" is defined at
the switch node.

Switch MDIO bus defined: Don't populate ds->user_mii_bus, 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].

There can be a case where ds->user_mii_bus is not populated, but
ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a
switch probed on OF, and it lets the DSA core driver to populate
ds->user_mii_bus and register the bus. We don't want this to happen.
Therefore, ds_switch_ops.phy_{read,write} should be only used on the
subdrivers that control switches probed on platform_data.

With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under
dsa_switch_setup() remains in use only for switches probed on
platform_data. Therefore, remove OF-based registration as it's useless now.

---

I think we should do all this in a single patch. I've done it on the MT7530
DSA subdriver which I maintain.

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");
+
  	bus = devm_mdiobus_alloc(dev);
  	if (!bus)
  		return -ENOMEM;

-	ds->user_mii_bus = bus;
+	if (mnp == NULL)
+		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);
  	if (ret) {
  		dev_err(dev, "failed to register MDIO bus: %d\n", ret);
  		if (priv->irq)

Arınç