From: Alvin Šipraga <alsi@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxx
`-- eth0
`-- miibus0
`-- mdio0-dev1d
`-- 0x00000000-0x0000003f ( 64 Bytes): /dev/mdio0-phy1d
`-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@xxxx
`-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@xxxx
`-- eth1
`-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@xxxx
`-- eth2
`-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@xxxx
`-- eth3
`-- 30be0000.ethernet@30be0000:mdio:ethernet-switch@1d:ports:port@xxxx
`-- 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@xxxxx
`-- 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@xxxxx
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@xxxxxxxxxxx, 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@xxxxx-reset) status -16
ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@xxxxx: error Device or resource busy: failed to get RESET GPIO
ERROR: realtek-smi 30be0000.ethernet@30be0000:mdio:ethernet-switch@xxxxx: 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@xxxxx-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@xxxxxxxxxxxxxxx>
---
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
