[PATCH 4/4] net: mdio_bus: associate OF nodes with their devices

["Alvin Šipraga" <alvin@pqrs.dk>]

From: Alvin Šipraga <alsi@bang-olufsen.dk>

barebox deep-probe will walk the device tree to ensure dependent devices
have been probed. In so doing, it uses the device_node::dev pointer to
check whether a given node has a device; if not, a device is created on
demand. The behaviour is recursive, so parent nodes without an
associated device will also have devices created on their behalf. The
recursion stops when a parent with a device is found.

Weird behaviour can ensure if, when devices with a device_node are
registered, the device_node::dev field is not populated. This patch
addresses a niche, benign, but also noisy error observed as a result of
this behaviour.

One mostly harmless consequence is that spurious devices can get created
when a suitable one already exists. In my case, I have an MDIO-connected
Ethernet switch with an internal MDIO bus and some internal PHYs. With
deep-probe, but without these changes, the devinfo output looks as
follows:

  `-- 30be0000.ethernet@30be0000.of
     `-- eth0
     `-- miibus0
        `-- mdio0-dev1d
           `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio0-phy1d
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@6.of
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@0.of
              `-- eth1
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@1.of
              `-- eth2
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@2.of
              `-- eth3
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@3.of
              `-- eth4
           `-- miibus1
              `-- mdio1-phy00
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy00
              `-- mdio1-phy01
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy01
              `-- mdio1-phy02
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy02
              `-- mdio1-phy03
                 `-- 0x00000000-0x0000003f (  64 Bytes): /dev/mdio1-phy03
     `-- 30be0000.ethernet@30be0000:mdio.of
        `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of
           `-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:mdio.of

Notice the last three devices with generic names; they are spurious
creations of the deep-probe algorithm. In fact, the devices have already
been created:

  real dev        spurious dev
  --------        ------------
  miibus0         30be0000.ethernet@30be0000:mdio.of
  mdio0-devid1d   30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of
  miibus1         30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:mdio.of

The only reason there aren't even more spurious devices created is that
deep-probe stopped at 30be0000.ethernet@30be0000.of, a platform device
whose device_node had its dev field populated correctly. But these
so-called real devices are all created by mdio_bus, which only links one
way, i.e. sets device::of_node, but not device_node::dev.

This issue probably goes unnoticed on most boards because, while the
call to of_device_ensure_probed() returns -ENODEV on the bottom listed
node, the code in phy.c doesn't check the return code, and the real
devices have already been probed, so no harm is done.

I observed much more surprising behaviour on my board because the switch
I am using, an RTL8365MB, has multiple possible management interfaces,
for which barebox has two drivers: realtek-smi and realtek-mdio (for
SMI- and MDIO-connected switches, respectively). The compatible strings
are the same, "realtek,rtl8365mb", but the bus types are different: the
former is a platform driver, the latter a phy (read: mdio) driver. In my
bootloader I have both drivers built in, because some of my boards use
SMI while others use MDIO.

When I would run the command 'boot bnet', bringing up my network
interface, DSA would call phy_device_connect() to connect my edev with
its corresponding phy, and a deep-probe would kick off because the phy's
parent's device_node::dev field was not populated. And during the
creation of the spurious device for my (already probed) Ethernet switch,
the platform code would find a compatible string match with realtek-smi
and try to probe the device with that driver:

  Booting entry 'bnet'
  ERROR: gpiolib: _gpio_request: gpio-233 (30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of-reset) status -16
  ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of: error Device or resource busy: failed to get RESET GPIO
  ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of: probe failed: Device or resource busy

As stated above, my switch is connected over MDIO, not SMI, so I really
should not be seeing any "realtek-smi" in my log. Nevertheless, because
phy.c doesn't check for errors, the errors are benign and my network
boot runs just fine.

Dumping the stack around the above error print in the realtek-smi driver
reveals exactly why things are going wrong:

  ERROR: gpiolib: _gpio_request: gpio-233 (30be0000.ethernet@30be0000:mdio:ethernet-switch@1d.of-reset) status -16
  Call trace:
  [<7dd8f400>] (unwind_backtrace+0x0/0x90)
  [<7dd8f4a0>] (dump_stack+0x10/0x18)
  [<7dd2d4d4>] (realtek_smi_probe+0x16c/0x2f4)
  [<7dd1da28>] (platform_probe+0x48/0x4c)
  [<7dd1cf24>] (device_probe+0x80/0x13c)
  [<7dd1d028>] (match+0x48/0x58)
  [<7dd1d3c8>] (register_device+0x178/0x184)
  [<7dd1da80>] (platform_device_register+0x18/0x20)
  [<7dd5501c>] (of_platform_device_create+0x2b8/0x2e0)
  [<7dd55170>] (of_device_create_on_demand+0x84/0xc0)
  [<7dd5513c>] (of_device_create_on_demand+0x50/0xc0)
  [<7dd552e0>] (of_device_ensure_probed+0x40/0x6c)
  [<7dd21398>] (phy_device_connect+0x174/0x230)
  [<7dd1fd74>] (dsa_port_start+0x60/0x1f0)
  [<7dd7a0e4>] (eth_open+0x24/0x44)
  [<7dd7d7e4>] (ifup_edev+0x9c/0x2ec)
  [<7dd7dbe4>] (ifup_all+0x110/0x204)
  [<7dd7dd2c>] (do_ifup+0x54/0xd4)
  [<7dd09e78>] (execute_command+0x44/0x8c)
  ...

The whole mess is rectified if the proper linking is done by mdio_bus,
since it is responsible for creating all of the devices to begin with.
So, make sure that the device_node::dev field is populated in all cases
there.

Note that while I also make this change in of_mdiobus_register_phy(), I
did not actually observe any unwanted behaviour by not doing so - the
problem above is specifically because it's not done in
of_mdiobus_register() or of_mdiobus_register_device(). But this is only
because of the way phys are looked up to begin with, so it seemed
reasonable to do it for them as well.

Signed-off-by: Alvin Šipraga <alsi@bang-olufsen.dk>
---
 drivers/net/phy/mdio_bus.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 332db6c05b11..94123ef6141c 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -119,6 +119,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio, struct device_node *chi
 	 * Associate the OF node with the device structure so it
 	 * can be looked up later
 	 */
+	child->dev = &phy->dev;
 	phy->dev.of_node = child;
 
 	/*
@@ -145,6 +146,7 @@ static int of_mdiobus_register_device(struct mii_bus *mdio,
 	if (IS_ERR(mdiodev))
 		return PTR_ERR(mdiodev);
 
+	child->dev = &mdiodev->dev;
 	mdiodev->dev.of_node = child;
 
 	ret = mdio_register_device(mdiodev);
@@ -315,6 +317,8 @@ int mdiobus_register(struct mii_bus *bus)
 	pr_info("%s: probed\n", dev_name(&bus->dev));
 
 	if (bus->dev.of_node) {
+		bus->dev.of_node->dev = &bus->dev;
+
 		/* Register PHY's as child node to mdio node */
 		of_mdiobus_register(bus, bus->dev.of_node);
 	}

-- 
2.43.0