@@ -278,6 +279,7 @@ int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np)/* Loop over the child nodes and register a phy_device for each phy */for_each_available_child_of_node(np,child){+pr_info("[%s:%d] child:%pOF\n",__func__,__LINE__,child);addr=of_mdio_parse_addr(&mdio->dev,child);if(addr<0){scanphys=true;
I've just received a report of kernel lockup after switching OpenWrt
platform from kernel 5.4 to kernel 5.10:
https://bugs.openwrt.org/index.php?do=details&task_id=4055
The problem is phy_probe() and its:
mutex_lock(&phydev->lock);
It seems to me that "lock" mutex doesn't get initalized. It seems
phy_device_create() doesn't get called for an MDIO device.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 10:17:20
On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
This isn't necessarily a PHY / MDIO regression. It could be some core
change that exposed a PHY / MDIO bug.
I think what's going on is that the switch device is somehow being
probed by phylib. It looks to me like we don't check that the mdio
device being matched in phy_bus_match() is actually a PHY (by
checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
before proceeding with any matching.)
We do, however, check the driver side. This looks to me like a problem
especially when the mdio bus can contain a mixture of PHY devices and
non-PHY devices. However, I would expect this to also be blowing up in
the mainline kernel as well - but it doesn't seem to.
Maybe Andrew can provide a reason why this doesn't happen - maybe we've
just been lucky with out-of-bounds read accesses (to the non-existent
phy_device wrapped around the mdio_device?)
If my theory is correct, this patch should solve your issue:
On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
quoted
This isn't necessarily a PHY / MDIO regression. It could be some core
change that exposed a PHY / MDIO bug.
I think what's going on is that the switch device is somehow being
probed by phylib. It looks to me like we don't check that the mdio
device being matched in phy_bus_match() is actually a PHY (by
checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
before proceeding with any matching.)
We do, however, check the driver side. This looks to me like a problem
especially when the mdio bus can contain a mixture of PHY devices and
non-PHY devices. However, I would expect this to also be blowing up in
the mainline kernel as well - but it doesn't seem to.
Maybe Andrew can provide a reason why this doesn't happen - maybe we've
just been lucky with out-of-bounds read accesses (to the non-existent
phy_device wrapped around the mdio_device?)
I'll see if I can use buildroot to test unmodified kernel.
quoted hunk
If my theory is correct, this patch should solve your issue:
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 10:40:48
On Thu, Sep 30, 2021 at 12:30:52PM +0200, Rafał Miłecki wrote:
On 30.09.2021 12:17, Russell King (Oracle) wrote:
quoted
On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
quoted
This isn't necessarily a PHY / MDIO regression. It could be some core
change that exposed a PHY / MDIO bug.
I think what's going on is that the switch device is somehow being
probed by phylib. It looks to me like we don't check that the mdio
device being matched in phy_bus_match() is actually a PHY (by
checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
before proceeding with any matching.)
We do, however, check the driver side. This looks to me like a problem
especially when the mdio bus can contain a mixture of PHY devices and
non-PHY devices. However, I would expect this to also be blowing up in
the mainline kernel as well - but it doesn't seem to.
Maybe Andrew can provide a reason why this doesn't happen - maybe we've
just been lucky with out-of-bounds read accesses (to the non-existent
phy_device wrapped around the mdio_device?)
I'll see if I can use buildroot to test unmodified kernel.
quoted
If my theory is correct, this patch should solve your issue:
Hmm.
In phy_probe, can you add:
WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));
just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.
MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.
Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.
Could it be that openwrt is carrying a patch that is causing this
issue?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Sep 30, 2021 at 11:58:21AM +0200, Rafał Miłecki wrote:
quoted
This isn't necessarily a PHY / MDIO regression. It could be some core
change that exposed a PHY / MDIO bug.
I think what's going on is that the switch device is somehow being
probed by phylib. It looks to me like we don't check that the mdio
device being matched in phy_bus_match() is actually a PHY (by
checking whether mdiodev->flags & MDIO_DEVICE_FLAG_PHY is true
before proceeding with any matching.)
We do, however, check the driver side. This looks to me like a problem
especially when the mdio bus can contain a mixture of PHY devices and
non-PHY devices. However, I would expect this to also be blowing up in
the mainline kernel as well - but it doesn't seem to.
Maybe Andrew can provide a reason why this doesn't happen - maybe we've
just been lucky with out-of-bounds read accesses (to the non-existent
phy_device wrapped around the mdio_device?)
I'll see if I can use buildroot to test unmodified kernel.
I've used buildroot to use unmodified 5.10.57 kernel.
Let me start with explaining that there are 2 b53 drivers.
1. OpenWrt downstream swconfig-based b53 driver
Its b53_mdio.c registers as PHY driver by calling phy_driver_register()
2. Upstream DSA-based b53 driver
Its b53_mdio.c registers as MDIO driver by using mdio_module_driver()
With buildroot + kernel 5.10.57 + upstream DSA-based b53 driver I can't
see phy_probe() called for the /mdio-mux@18003000/mdio@200/switch@0 .
I'm not sure why as I have CONFIG_B53_MDIO_DRIVER=y . Maybe it's some
PHY device vs. MDIO device thing?
I'll proceed with Russell's request for checking MDIO_DEVICE_FLAG_PHY
now.
In phy_probe, can you add:
WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));
just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.
MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.
Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.
Could it be that openwrt is carrying a patch that is causing this
issue?
I don't think there is any OpenWrt patch affecting that.
MDIO_DEVICE_FLAG_PHY seems to be missing.
[ 5.593833] libphy: Fixed MDIO Bus: probed
[ 5.598383] libphy: iProc MDIO bus: probed
[ 5.602510] iproc-mdio 18003000.mdio: Broadcom iProc MDIO bus registered
[ 5.609918] libphy: mdio_mux: probed
[ 5.613533] mdio_bus 0.0:10: flags: 0x00000000
[ 5.618816] libphy: mdio_mux: probed
[ 5.622440] mdio_bus 0.200:00: flags: 0x00000000
[ 5.627479] Broadcom B53 (2) 0.200:00: flags: 0x00000000
[ 5.632811] ------------[ cut here ]------------
[ 5.637475] WARNING: CPU: 0 PID: 1 at drivers/net/phy/phy_device.c:2828 phy_probe+0x40/0x1d4
[ 5.645946] Modules linked in:
[ 5.649011] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.10.64 #0
[ 5.655030] Hardware name: BCM5301X
[ 5.658543] [<c0108410>] (unwind_backtrace) from [<c0104bc4>] (show_stack+0x10/0x14)
[ 5.666316] [<c0104bc4>] (show_stack) from [<c03dc6a8>] (dump_stack+0x94/0xa8)
[ 5.673565] [<c03dc6a8>] (dump_stack) from [<c01183e8>] (__warn+0xb8/0x114)
[ 5.680546] [<c01183e8>] (__warn) from [<c01184ac>] (warn_slowpath_fmt+0x68/0x78)
[ 5.688050] [<c01184ac>] (warn_slowpath_fmt) from [<c04b8d78>] (phy_probe+0x40/0x1d4)
[ 5.695909] [<c04b8d78>] (phy_probe) from [<c0457120>] (really_probe+0xfc/0x4e0)
[ 5.703325] [<c0457120>] (really_probe) from [<c0455378>] (bus_for_each_drv+0x74/0x98)
[ 5.711262] [<c0455378>] (bus_for_each_drv) from [<c04576b8>] (__device_attach+0xcc/0x120)
[ 5.719548] [<c04576b8>] (__device_attach) from [<c0456300>] (bus_probe_device+0x84/0x8c)
[ 5.727747] [<c0456300>] (bus_probe_device) from [<c04529ac>] (device_add+0x300/0x77c)
[ 5.735687] [<c04529ac>] (device_add) from [<c04ba3dc>] (mdio_device_register+0x38/0x5c)
[ 5.743801] [<c04ba3dc>] (mdio_device_register) from [<c04c1b08>] (of_mdiobus_register+0x198/0x2fc)
[ 5.752868] [<c04c1b08>] (of_mdiobus_register) from [<c04c238c>] (mdio_mux_init+0x178/0x2c0)
[ 5.761328] [<c04c238c>] (mdio_mux_init) from [<c04c2668>] (mdio_mux_mmioreg_probe+0x138/0x1fc)
[ 5.770054] [<c04c2668>] (mdio_mux_mmioreg_probe) from [<c0458ee4>] (platform_drv_probe+0x34/0x70)
[ 5.779032] [<c0458ee4>] (platform_drv_probe) from [<c0457120>] (really_probe+0xfc/0x4e0)
[ 5.787230] [<c0457120>] (really_probe) from [<c0457b04>] (device_driver_attach+0xe4/0xf4)
[ 5.795516] [<c0457b04>] (device_driver_attach) from [<c0457b90>] (__driver_attach+0x7c/0x110)
[ 5.804149] [<c0457b90>] (__driver_attach) from [<c04552d8>] (bus_for_each_dev+0x64/0x90)
[ 5.812340] [<c04552d8>] (bus_for_each_dev) from [<c04564f8>] (bus_add_driver+0xf8/0x1e0)
[ 5.820540] [<c04564f8>] (bus_add_driver) from [<c045819c>] (driver_register+0x88/0x118)
[ 5.828652] [<c045819c>] (driver_register) from [<c01017e4>] (do_one_initcall+0x54/0x1e8)
[ 5.836863] [<c01017e4>] (do_one_initcall) from [<c0801118>] (kernel_init_freeable+0x23c/0x290)
[ 5.845583] [<c0801118>] (kernel_init_freeable) from [<c065ad40>] (kernel_init+0x8/0x118)
[ 5.853781] [<c065ad40>] (kernel_init) from [<c0100128>] (ret_from_fork+0x14/0x2c)
[ 5.861367] Exception stack(0xc1035fb0 to 0xc1035ff8)
[ 5.866425] 5fa0: 00000000 00000000 00000000 00000000
[ 5.874618] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 5.882816] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 5.889473] ---[ end trace f28513567c7daf95 ]---
[ 5.894105] ------------[ cut here ]------------
[ 5.898743] WARNING: CPU: 0 PID: 1 at kernel/locking/mutex.c:951 __mutex_lock.constprop.0+0x744/0x848
[ 5.907983] DEBUG_LOCKS_WARN_ON(lock->magic != lock)
@@ -2824,6 +2824,9 @@ static int phy_probe(struct device *dev)structphy_driver*phydrv=to_phy_driver(drv);interr=0;+dev_info(dev,"flags: 0x%08x\n",phydev->mdio.flags);+WARN_ON(!(phydev->mdio.flags&MDIO_DEVICE_FLAG_PHY));+phydev->drv=phydrv;/* Disable the interrupt if the PHY doesn't support it
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 11:44:23
On Thu, Sep 30, 2021 at 01:29:33PM +0200, Rafał Miłecki wrote:
On 30.09.2021 12:40, Russell King (Oracle) wrote:
quoted
In phy_probe, can you add:
WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));
just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.
MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.
Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.
Could it be that openwrt is carrying a patch that is causing this
issue?
I don't think there is any OpenWrt patch affecting that.
MDIO_DEVICE_FLAG_PHY seems to be missing.
Right, so the mdio device being registered is a non-PHY MDIO device.
It doesn't have a struct phy_device around it - and so any access
outside of the mdio_device is an out-of-bounds access.
Consequently, phylib should not be matching this device. The only
remaining way I can see that this could happen is if a PHY driver has
an OF compatible, which phylib drivers should never have.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Sep 30, 2021 at 01:29:33PM +0200, Rafał Miłecki wrote:
quoted
On 30.09.2021 12:40, Russell King (Oracle) wrote:
quoted
In phy_probe, can you add:
WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));
just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.
MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.
Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.
Could it be that openwrt is carrying a patch that is causing this
issue?
I don't think there is any OpenWrt patch affecting that.
MDIO_DEVICE_FLAG_PHY seems to be missing.
Right, so the mdio device being registered is a non-PHY MDIO device.
It doesn't have a struct phy_device around it - and so any access
outside of the mdio_device is an out-of-bounds access.
I can confirm that.
of_mdiobus_register() iterates over node children. It calls
of_mdiobus_child_is_phy() for the /mdio-mux@18003000/mdio@200/switch@0
and that returns 0. It results in calling of_mdiobus_register_device().
So we have MDIO device as expected. It's not a PHY device.
Consequently, phylib should not be matching this device. The only
remaining way I can see that this could happen is if a PHY driver has
an OF compatible, which phylib drivers should never have.
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 12:30:24
On Thu, Sep 30, 2021 at 02:14:54PM +0200, Rafał Miłecki wrote:
On 30.09.2021 13:44, Russell King (Oracle) wrote:
quoted
On Thu, Sep 30, 2021 at 01:29:33PM +0200, Rafał Miłecki wrote:
quoted
On 30.09.2021 12:40, Russell King (Oracle) wrote:
quoted
In phy_probe, can you add:
WARN_ON(!(phydev->mdio.flags & MDIO_DEVICE_FLAG_PHY));
just to make sure we have a real PHY device there please? Maybe also
print the value of the flags argument.
MDIO_DEVICE_FLAG_PHY is set by phy_create_device() before the mutex is
initialised, so if it is set, the lock should be initialised.
Maybe also print mdiodev->flags in mdio_device_register() as well, so
we can see what is being registered and the flags being used for that
device.
Could it be that openwrt is carrying a patch that is causing this
issue?
I don't think there is any OpenWrt patch affecting that.
MDIO_DEVICE_FLAG_PHY seems to be missing.
Right, so the mdio device being registered is a non-PHY MDIO device.
It doesn't have a struct phy_device around it - and so any access
outside of the mdio_device is an out-of-bounds access.
I can confirm that.
of_mdiobus_register() iterates over node children. It calls
of_mdiobus_child_is_phy() for the /mdio-mux@18003000/mdio@200/switch@0
and that returns 0. It results in calling of_mdiobus_register_device().
So we have MDIO device as expected. It's not a PHY device.
Right - because it's a switch device - identified as having a
compatible but without having a PHY-like compatible.
compatible = "brcm,bcm53125";
This will be created by mdio_device_create(), which will not have
its bus_match method populated. So the only way a driver registered
on the MDIO bus_type can match is via the DT-based matching I've
previously mentioned.
quoted
Consequently, phylib should not be matching this device. The only
remaining way I can see that this could happen is if a PHY driver has
an OF compatible, which phylib drivers should never have.
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 13:08:19
On Thu, Sep 30, 2021 at 02:51:40PM +0200, Rafał Miłecki wrote:
On 30.09.2021 14:30, Russell King (Oracle) wrote:
quoted
quoted
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
You can see that is *uses* of_match_table.
What about refusing bugged drivers like above b53 with something like:
That will break all the MDIO based DSA and other non-PHY drivers,
sorry.
I suppose we could detect if the driver has the MDIO_DEVICE_IS_PHY flag
set, and reject any device that does not have MDIO_DEVICE_IS_PHY set:
@@ -939,6 +939,12 @@ EXPORT_SYMBOL_GPL(mdiobus_modify);staticintmdio_bus_match(structdevice*dev,structdevice_driver*drv){structmdio_device*mdio=to_mdio_device(dev);+structmdio_driver*mdiodrv=to_mdio_driver(drv);++/* Both the driver and device must type-match */+if(!(mdiodrv->mdiodrv.flags&MDIO_DEVICE_IS_PHY)==+!(mdio->flags&MDIO_DEVICE_FLAG_PHY))+return0;if(of_driver_match_device(dev,drv))return1;
In other words, the driver's state of the MDIO_DEVICE_IS_PHY flag
must match the device's MDIO_DEVICE_FLAG_PHY flag before we attempt
any matches.
If that's not possible, then we need to prevent phylib drivers from
using .of_match_table:
@@ -2501,6 +2501,16 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)return-EINVAL;}+/* PHYLIB device drivers must not match using a DT compatible table+*asthisbypassesourchecksthatthemdiodevthatisbeingmatched+*isbackedbyastructphy_device.Ifsuchacasehappens,wewill+*makeout-of-boundsaccessesandlockupinphydev->lock.+*/+if(WARN(new_driver->mdiodrv.driver.of_match_table,+"%s: driver must not provide a DT match table\n",+new_driver->name))+return-EINVAL;+new_driver->mdiodrv.flags|=MDIO_DEVICE_IS_PHY;new_driver->mdiodrv.driver.name=new_driver->name;new_driver->mdiodrv.driver.bus=&mdio_bus_type;
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 13:22:04
On Thu, Sep 30, 2021 at 02:07:44PM +0100, Russell King (Oracle) wrote:
quoted hunk
On Thu, Sep 30, 2021 at 02:51:40PM +0200, Rafał Miłecki wrote:
quoted
On 30.09.2021 14:30, Russell King (Oracle) wrote:
quoted
quoted
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
You can see that is *uses* of_match_table.
What about refusing bugged drivers like above b53 with something like:
That will break all the MDIO based DSA and other non-PHY drivers,
sorry.
I suppose we could detect if the driver has the MDIO_DEVICE_IS_PHY flag
set, and reject any device that does not have MDIO_DEVICE_IS_PHY set:
@@ -939,6 +939,12 @@ EXPORT_SYMBOL_GPL(mdiobus_modify);staticintmdio_bus_match(structdevice*dev,structdevice_driver*drv){structmdio_device*mdio=to_mdio_device(dev);+structmdio_driver*mdiodrv=to_mdio_driver(drv);++/* Both the driver and device must type-match */+if(!(mdiodrv->mdiodrv.flags&MDIO_DEVICE_IS_PHY)==+!(mdio->flags&MDIO_DEVICE_FLAG_PHY))+return0;if(of_driver_match_device(dev,drv))return1;
In other words, the driver's state of the MDIO_DEVICE_IS_PHY flag
must match the device's MDIO_DEVICE_FLAG_PHY flag before we attempt
any matches.
If that's not possible, then we need to prevent phylib drivers from
using .of_match_table:
@@ -2501,6 +2501,16 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)return-EINVAL;}+/* PHYLIB device drivers must not match using a DT compatible table+*asthisbypassesourchecksthatthemdiodevthatisbeingmatched+*isbackedbyastructphy_device.Ifsuchacasehappens,wewill+*makeout-of-boundsaccessesandlockupinphydev->lock.+*/+if(WARN(new_driver->mdiodrv.driver.of_match_table,+"%s: driver must not provide a DT match table\n",+new_driver->name))+return-EINVAL;+new_driver->mdiodrv.flags|=MDIO_DEVICE_IS_PHY;new_driver->mdiodrv.driver.name=new_driver->name;new_driver->mdiodrv.driver.bus=&mdio_bus_type;
I should also point out that as this b53 driver that is causing the
problem only exists in OpenWRT, this is really a matter for OpenWRT
developers rather than mainline which does not suffer this problem.
I suspect that OpenWRT developers will not be happy with either of
the two patches I've posted above - I suspect they are trying to
support both DSA and swconfig approaches with a single DT. That can
be made to work, but not with a PHYLIB driver being a wrapper around
the swconfig stuff (precisely because there's no phy_device in this
scenario.)
The only reason to patch mainline kernels would be to make them more
robust, and maybe to also make an explicit statement about what isn't
supported (having a phy_driver with its of_match_table member set.)
I probably should've made that clearer in my email with the patches.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-09-30 13:32:16
I should also point out that as this b53 driver that is causing the
problem only exists in OpenWRT, this is really a matter for OpenWRT
developers rather than mainline which does not suffer this problem.
I suspect that OpenWRT developers will not be happy with either of
the two patches I've posted above - I suspect they are trying to
support both DSA and swconfig approaches with a single DT. That can
be made to work, but not with a PHYLIB driver being a wrapper around
the swconfig stuff (precisely because there's no phy_device in this
scenario.)
The only reason to patch mainline kernels would be to make them more
robust, and maybe to also make an explicit statement about what isn't
supported (having a phy_driver with its of_match_table member set.)
I agree with you here. This is an OpenWRT problem. We would hopefully
catch such a driver at review time and reject it. We could make it
more robust in mainline, but as you said, OpenWRT developers might not
actually like it more robust.
Andrew
On Thu, Sep 30, 2021 at 02:51:40PM +0200, Rafał Miłecki wrote:
quoted
On 30.09.2021 14:30, Russell King (Oracle) wrote:
quoted
quoted
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
You can see that is *uses* of_match_table.
What about refusing bugged drivers like above b53 with something like:
That will break all the MDIO based DSA and other non-PHY drivers,
sorry.
I suppose we could detect if the driver has the MDIO_DEVICE_IS_PHY flag
set, and reject any device that does not have MDIO_DEVICE_IS_PHY set:
@@ -939,6 +939,12 @@ EXPORT_SYMBOL_GPL(mdiobus_modify);staticintmdio_bus_match(structdevice*dev,structdevice_driver*drv){structmdio_device*mdio=to_mdio_device(dev);+structmdio_driver*mdiodrv=to_mdio_driver(drv);++/* Both the driver and device must type-match */+if(!(mdiodrv->mdiodrv.flags&MDIO_DEVICE_IS_PHY)==+!(mdio->flags&MDIO_DEVICE_FLAG_PHY))+return0;if(of_driver_match_device(dev,drv))return1;
In OpenWrt & bugged b53 case we have:
1. Device without MDIO_DEVICE_FLAG_PHY
2. Driver with MDIO_DEVICE_IS_PHY
I think the logic should be to return 0 on mismatch (reverted).
Above code doesn't prevent probing bugged b53 driver.
quoted hunk
In other words, the driver's state of the MDIO_DEVICE_IS_PHY flag
must match the device's MDIO_DEVICE_FLAG_PHY flag before we attempt
any matches.
If that's not possible, then we need to prevent phylib drivers from
using .of_match_table:
@@ -2501,6 +2501,16 @@ int phy_driver_register(struct phy_driver *new_driver, struct module *owner)return-EINVAL;}+/* PHYLIB device drivers must not match using a DT compatible table+*asthisbypassesourchecksthatthemdiodevthatisbeingmatched+*isbackedbyastructphy_device.Ifsuchacasehappens,wewill+*makeout-of-boundsaccessesandlockupinphydev->lock.+*/+if(WARN(new_driver->mdiodrv.driver.of_match_table,+"%s: driver must not provide a DT match table\n",+new_driver->name))+return-EINVAL;+new_driver->mdiodrv.flags|=MDIO_DEVICE_IS_PHY;new_driver->mdiodrv.driver.name=new_driver->name;new_driver->mdiodrv.driver.bus=&mdio_bus_type;
FWIW it prevents probing b53:
[ 6.226037] ------------[ cut here ]------------
[ 6.230687] WARNING: CPU: 1 PID: 1 at drivers/net/phy/phy_device.c:2964 phy_driver_register+0xe4/0x108
[ 6.240073] Broadcom B53 (1): driver must not provide a DT match table
[ 6.246627] Modules linked in:
[ 6.249696] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.10.64 #0
[ 6.255714] Hardware name: BCM5301X
[ 6.259229] [<c0108410>] (unwind_backtrace) from [<c0104bc4>] (show_stack+0x10/0x14)
[ 6.266999] [<c0104bc4>] (show_stack) from [<c03dc6a8>] (dump_stack+0x94/0xa8)
[ 6.274249] [<c03dc6a8>] (dump_stack) from [<c01183e8>] (__warn+0xb8/0x114)
[ 6.281230] [<c01183e8>] (__warn) from [<c01184ac>] (warn_slowpath_fmt+0x68/0x78)
[ 6.288736] [<c01184ac>] (warn_slowpath_fmt) from [<c04b7278>] (phy_driver_register+0xe4/0x108)
[ 6.297464] [<c04b7278>] (phy_driver_register) from [<c081b72c>] (b53_phy_driver_register+0x14/0x6c)
[ 6.306622] [<c081b72c>] (b53_phy_driver_register) from [<c01017e4>] (do_one_initcall+0x54/0x1e8)
[ 6.315526] [<c01017e4>] (do_one_initcall) from [<c0801118>] (kernel_init_freeable+0x23c/0x290)
[ 6.324246] [<c0801118>] (kernel_init_freeable) from [<c065acd8>] (kernel_init+0x8/0x118)
[ 6.332445] [<c065acd8>] (kernel_init) from [<c0100128>] (ret_from_fork+0x14/0x2c)
[ 6.340031] Exception stack(0xc1035fb0 to 0xc1035ff8)
[ 6.345089] 5fa0: 00000000 00000000 00000000 00000000
[ 6.353280] 5fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 6.361470] 5fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[ 6.368119] ---[ end trace efac8022c3486581 ]---
I should also point out that as this b53 driver that is causing the
problem only exists in OpenWRT, this is really a matter for OpenWRT
developers rather than mainline which does not suffer this problem.
I suspect that OpenWRT developers will not be happy with either of
the two patches I've posted above - I suspect they are trying to
support both DSA and swconfig approaches with a single DT. That can
be made to work, but not with a PHYLIB driver being a wrapper around
the swconfig stuff (precisely because there's no phy_device in this
scenario.)
The only reason to patch mainline kernels would be to make them more
robust, and maybe to also make an explicit statement about what isn't
supported (having a phy_driver with its of_match_table member set.)
I agree with you here. This is an OpenWRT problem. We would hopefully
catch such a driver at review time and reject it. We could make it
more robust in mainline, but as you said, OpenWRT developers might not
actually like it more robust.
I was thinking about patching mdio_bus_match() / phy_driver_register()
to prevent other developers from doing the same mistake as OpenWrt &
b53. Also saving your time from reports similar to mine.
I understand it's an issue that OpenWrt has to handle downstream.
Thank you a lot for helping me investigate this problem.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-09-30 13:54:20
On Thu, Sep 30, 2021 at 03:42:38PM +0200, Rafał Miłecki wrote:
On 30.09.2021 15:07, Russell King (Oracle) wrote:
quoted
On Thu, Sep 30, 2021 at 02:51:40PM +0200, Rafał Miłecki wrote:
quoted
On 30.09.2021 14:30, Russell King (Oracle) wrote:
quoted
quoted
It's actually OpenWrt's downstream swconfig-based b53 driver that
matches this device.
I'm confused as downstream b53_mdio.c calls phy_driver_register(). Why
does it match MDIO device then? I thought MDIO devices should be
matches only with drivers using mdio_driver_register().
Note that I've no idea what he swconfig-based b53 driver looks like,
I don't have the source for that to hand.
If it calls phy_driver_register(), then it is registering a driver for
a MDIO device wrapped in a struct phy_device. If this driver has a
.of_match_table member set, then this is wrong - the basic rule is
PHY drivers must never match using DT compatibles.
because this is exactly what will occur - it bypasses the check that
the mdio_device being matched is in fact wrapped by a struct phy_device,
and we will access members of the non-existent phy_device, including
the "uninitialised" mutex.
If the swconfig-based b53 driver does want to bind to a phy_device based
DT node, then it needs to match using either a custom .match_phy_device
method in the PHY driver, or it needs to match using the PHY IDs.
You can see that is *uses* of_match_table.
What about refusing bugged drivers like above b53 with something like:
That will break all the MDIO based DSA and other non-PHY drivers,
sorry.
I suppose we could detect if the driver has the MDIO_DEVICE_IS_PHY flag
set, and reject any device that does not have MDIO_DEVICE_IS_PHY set:
@@ -939,6 +939,12 @@ EXPORT_SYMBOL_GPL(mdiobus_modify);staticintmdio_bus_match(structdevice*dev,structdevice_driver*drv){structmdio_device*mdio=to_mdio_device(dev);+structmdio_driver*mdiodrv=to_mdio_driver(drv);++/* Both the driver and device must type-match */+if(!(mdiodrv->mdiodrv.flags&MDIO_DEVICE_IS_PHY)==+!(mdio->flags&MDIO_DEVICE_FLAG_PHY))+return0;if(of_driver_match_device(dev,drv))return1;
In OpenWrt & bugged b53 case we have:
1. Device without MDIO_DEVICE_FLAG_PHY
2. Driver with MDIO_DEVICE_IS_PHY
I think the logic should be to return 0 on mismatch (reverted).
I assume you mean the test should've been != not == - then yes, you
are absolutely correct. Sorry, I'm still not over a cold.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!