| Message ID | E1qiEFs-007g7b-Lq@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6a23c555f7eb436d6799533675ffa179db3d5834 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: phy: fix regression with AX88772A PHY driver | expand |
On Mon, Sep 18, 2023 at 02:25:36PM +0100, Russell King (Oracle) wrote: > Marek reports that a deadlock occurs with the AX88772A PHY used on the > ASIX USB network driver: > > asix 1-1.4:1.0 (unnamed net_device) (uninitialized): PHY [usb-001:003:10] driver [Asix Electronics AX88772A] (irq=POLL) > Asix Electronics AX88772A usb-001:003:10: attached PHY driver(mii_bus:phy_addr=usb-001:003:10, irq=POLL) > asix 1-1.4:1.0 eth0: register 'asix' at usb-12110000.usb-1.4, ASIX AX88772 USB 2.0 Ethernet, a2:99:b6:cd:11:eb > asix 1-1.4:1.0 eth0: configuring for phy/internal link mode > > ============================================ > WARNING: possible recursive locking detected > 6.6.0-rc1-00239-g8da77df649c4-dirty #13949 Not tainted > -------------------------------------------- > kworker/3:3/71 is trying to acquire lock: > c6c704cc (&dev->lock){+.+.}-{3:3}, at: phy_start_aneg+0x1c/0x38 > > but task is already holding lock: > c6c704cc (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x100/0x2b8 > > This is because we now consistently call phy_process_state_change() > while holding phydev->lock, but the AX88772A PHY driver then goes on > to call phy_start_aneg() which tries to grab the same lock - causing > deadlock. > > Fix this by exporting the unlocked version, and use this in the PHY > driver instead. > > Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > Fixes: ef113a60d0a9 ("net: phy: call phy_error_precise() while holding the lock") > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Hi Russell Yes, this fixes the problem for stable. Reviewed-by: Andrew Lunn <andrew@lunn.ch> But maybe it would be better to move the hardware workaround into the PHY driver? Its the PHY which is broken, so why is the MAC working around it? Andrew
> Err? Sorry, but your comment makes little sense
Sorry, -EMORECOFFEE.
Andrew
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Mon, 18 Sep 2023 14:25:36 +0100 you wrote: > Marek reports that a deadlock occurs with the AX88772A PHY used on the > ASIX USB network driver: > > asix 1-1.4:1.0 (unnamed net_device) (uninitialized): PHY [usb-001:003:10] driver [Asix Electronics AX88772A] (irq=POLL) > Asix Electronics AX88772A usb-001:003:10: attached PHY driver(mii_bus:phy_addr=usb-001:003:10, irq=POLL) > asix 1-1.4:1.0 eth0: register 'asix' at usb-12110000.usb-1.4, ASIX AX88772 USB 2.0 Ethernet, a2:99:b6:cd:11:eb > asix 1-1.4:1.0 eth0: configuring for phy/internal link mode > > [...] Here is the summary with links: - [net-next] net: phy: fix regression with AX88772A PHY driver https://git.kernel.org/netdev/net-next/c/6a23c555f7eb You are awesome, thank you!
diff --git a/drivers/net/phy/ax88796b.c b/drivers/net/phy/ax88796b.c index 0f1e617a26c9..eb74a8cf8df1 100644 --- a/drivers/net/phy/ax88796b.c +++ b/drivers/net/phy/ax88796b.c @@ -90,7 +90,7 @@ static void asix_ax88772a_link_change_notify(struct phy_device *phydev) */ if (phydev->state == PHY_NOLINK) { phy_init_hw(phydev); - phy_start_aneg(phydev); + _phy_start_aneg(phydev); } } diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index 93a8676dd8d8..a5fa077650e8 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -981,7 +981,7 @@ static int phy_check_link_status(struct phy_device *phydev) * If the PHYCONTROL Layer is operating, we change the state to * reflect the beginning of Auto-negotiation or forcing. */ -static int _phy_start_aneg(struct phy_device *phydev) +int _phy_start_aneg(struct phy_device *phydev) { int err; @@ -1002,6 +1002,7 @@ static int _phy_start_aneg(struct phy_device *phydev) return err; } +EXPORT_SYMBOL(_phy_start_aneg); /** * phy_start_aneg - start auto-negotiation for this PHY device diff --git a/include/linux/phy.h b/include/linux/phy.h index 1351b802ffcf..3cc52826f18e 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -1736,6 +1736,7 @@ void phy_detach(struct phy_device *phydev); void phy_start(struct phy_device *phydev); void phy_stop(struct phy_device *phydev); int phy_config_aneg(struct phy_device *phydev); +int _phy_start_aneg(struct phy_device *phydev); int phy_start_aneg(struct phy_device *phydev); int phy_aneg_done(struct phy_device *phydev); int phy_speed_down(struct phy_device *phydev, bool sync);
Marek reports that a deadlock occurs with the AX88772A PHY used on the ASIX USB network driver: asix 1-1.4:1.0 (unnamed net_device) (uninitialized): PHY [usb-001:003:10] driver [Asix Electronics AX88772A] (irq=POLL) Asix Electronics AX88772A usb-001:003:10: attached PHY driver(mii_bus:phy_addr=usb-001:003:10, irq=POLL) asix 1-1.4:1.0 eth0: register 'asix' at usb-12110000.usb-1.4, ASIX AX88772 USB 2.0 Ethernet, a2:99:b6:cd:11:eb asix 1-1.4:1.0 eth0: configuring for phy/internal link mode ============================================ WARNING: possible recursive locking detected 6.6.0-rc1-00239-g8da77df649c4-dirty #13949 Not tainted -------------------------------------------- kworker/3:3/71 is trying to acquire lock: c6c704cc (&dev->lock){+.+.}-{3:3}, at: phy_start_aneg+0x1c/0x38 but task is already holding lock: c6c704cc (&dev->lock){+.+.}-{3:3}, at: phy_state_machine+0x100/0x2b8 This is because we now consistently call phy_process_state_change() while holding phydev->lock, but the AX88772A PHY driver then goes on to call phy_start_aneg() which tries to grab the same lock - causing deadlock. Fix this by exporting the unlocked version, and use this in the PHY driver instead. Reported-by: Marek Szyprowski <m.szyprowski@samsung.com> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Fixes: ef113a60d0a9 ("net: phy: call phy_error_precise() while holding the lock") Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- Reviewing the other PHY drivers, no others appear impacted, just this one. drivers/net/phy/ax88796b.c | 2 +- drivers/net/phy/phy.c | 3 ++- include/linux/phy.h | 1 + 3 files changed, 4 insertions(+), 2 deletions(-)