Re: [PATCH net] net: phy: Warn about incorrect mdio_bus_phy_resume() state
From: Geert Uytterhoeven
Date: Tue Aug 16 2022 - 09:20:46 EST
Hi Florian,
On Fri, Aug 12, 2022 at 6:39 PM Florian Fainelli <f.fainelli@xxxxxxxxx> wrote:
> On 8/12/22 04:19, Marek Szyprowski wrote:
> > On 02.08.2022 01:34, Florian Fainelli wrote:
> >> Calling mdio_bus_phy_resume() with neither the PHY state machine set to
> >> PHY_HALTED nor phydev->mac_managed_pm set to true is a good indication
> >> that we can produce a race condition looking like this:
> >>
> >> CPU0 CPU1
> >> bcmgenet_resume
> >> -> phy_resume
> >> -> phy_init_hw
> >> -> phy_start
> >> -> phy_resume
> >> phy_start_aneg()
> >> mdio_bus_phy_resume
> >> -> phy_resume
> >> -> phy_write(..., BMCR_RESET)
> >> -> usleep() -> phy_read()
> >>
> >> with the phy_resume() function triggering a PHY behavior that might have
> >> to be worked around with (see bf8bfc4336f7 ("net: phy: broadcom: Fix
> >> brcm_fet_config_init()") for instance) that ultimately leads to an error
> >> reading from the PHY.
> >>
> >> Fixes: fba863b81604 ("net: phy: make PHY PM ops a no-op if MAC driver manages PHY PM")
> >> Signed-off-by: Florian Fainelli <f.fainelli@xxxxxxxxx>
> >
> > This patch, as probably intended, triggers a warning during system
> > suspend/resume cycle in the SMSC911x driver. I've observed it on ARM
> > Juno R1 board on the kernel compiled from next-202208010:
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 1 PID: 398 at drivers/net/phy/phy_device.c:323
> > mdio_bus_phy_resume+0x34/0xc8
I am seeing the same on the ape6evm and kzm9g development
boards with smsc911x Ethernet, and on various boards with Renesas
Ethernet (sh_eth or ravb) if Wake-on-LAN is disabled.
> Yes this is catching an actual issue in the driver in that the PHY state
> machine is still running while the system is trying to suspend. We could
> go about fixing it in a different number of ways, though I believe this
> one is probably correct enough to work and fix the warning:
> --- a/drivers/net/ethernet/smsc/smsc911x.c
> +++ b/drivers/net/ethernet/smsc/smsc911x.c
> @@ -1037,6 +1037,8 @@ static int smsc911x_mii_probe(struct net_device *dev)
> return ret;
> }
>
> + /* Indicate that the MAC is responsible for managing PHY PM */
> + phydev->mac_managed_pm = true;
> phy_attached_info(phydev);
>
> phy_set_max_speed(phydev, SPEED_100);
> @@ -2587,6 +2589,8 @@ static int smsc911x_suspend(struct device *dev)
> if (netif_running(ndev)) {
> netif_stop_queue(ndev);
> netif_device_detach(ndev);
> + if (!device_may_wakeup(dev))
> + phy_suspend(dev->phydev);
> }
>
> /* enable wake on LAN, energy detection and the external PME
> @@ -2628,6 +2632,8 @@ static int smsc911x_resume(struct device *dev)
> if (netif_running(ndev)) {
> netif_device_attach(ndev);
> netif_start_queue(ndev);
> + if (!device_may_wakeup(dev))
> + phy_resume(dev->phydev);
> }
>
> return 0;
Thanks for your patch, but unfortunately this does not work on ape6evm
and kzm9g, where the smsc911x device is connected to a power-managed
bus. It looks like the PHY registers are accessed while the device
is already suspended, causing a crash during system suspend:
8<--- cut here ---
Unhandled fault: imprecise external abort (0x1406) at 0x00000000
[00000000] *pgd=00000000
Internal error: : 1406 [#1] SMP ARM
CPU: 2 PID: 75 Comm: kworker/2:2 Not tainted
6.0.0-rc1-ape6evm-00977-gdc70725fbca5-dirty #375
Hardware name: Generic R8A73A4 (Flattened Device Tree)
Workqueue: events_power_efficient phy_state_machine
PC is at smsc911x_reg_read+0x30/0x48
LR is at smsc911x_reg_read+0x30/0x48
pc : [<c03807cc>] lr : [<c03807cc>] psr: 20030093
sp : f0891e30 ip : 00000000 fp : eff98605
r10: c092af80 r9 : c202e6e8 r8 : 20030013
r7 : c202e70c r6 : 000000a4 r5 : 20030093 r4 : c202e6c0
r3 : f0a31000 r2 : 00000002 r1 : f0a310a4 r0 : 00000000
Flags: nzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 4552006a DAC: 00000051
Register r0 information: NULL pointer
Register r1 information: 0-page vmalloc region starting at 0xf0a31000
allocated at smsc911x_drv_probe+0x108/0x934
Register r2 information: non-paged memory
Register r3 information: 0-page vmalloc region starting at 0xf0a31000
allocated at smsc911x_drv_probe+0x108/0x934
Register r4 information: slab kmalloc-4k start c202e000 pointer offset
1728 size 4096
Register r5 information: non-paged memory
Register r6 information: non-paged memory
Register r7 information: slab kmalloc-4k start c202e000 pointer offset
1804 size 4096
Register r8 information: non-paged memory
Register r9 information: slab kmalloc-4k start c202e000 pointer offset
1768 size 4096
Register r10 information: non-slab/vmalloc memory
Register r11 information: non-slab/vmalloc memory
Register r12 information: NULL pointer
Process kworker/2:2 (pid: 75, stack limit = 0x5239c21f)
Stack: (0xf0891e30 to 0xf0892000)
1e20: c202e6c0 00000006 c19da000 c202e6c0
1e40: 20030013 c03815bc 00000000 00000001 c19da000 c0381820 00000001 c19da000
1e60: c19da000 00000000 c39eacc0 00000000 c092af80 c037e694 c19da000 00000001
1e80: 00000000 c19da758 c19da000 00000001 00000000 c037e888 c29a0000 c29a03f4
1ea0: 00000078 c29a0448 c39eacc0 c037c878 c29a0000 c037cab4 c29a0000 c29a03f4
1ec0: c29a0000 c037fbc8 c29a0000 c29a03f4 c29a0000 c29a0448 00000004 c0377540
1ee0: c3ac8f00 c29a03f4 c29a0000 c0378588 009a03f4 c07cce88 c3ac8f00 c29a03f4
1f00: eff96780 00000000 eff98600 00000080 c092af80 c00426f0 00000001 00000000
1f20: c00425c4 c07cce88 c07bb574 00000000 c13fa5b8 c0da3f6c 00000000 c071c1da
1f40: 00000000 c07cce88 00000000 c3ac8f00 c3ac8f18 eff96780 c092a665 c07c9d00
1f60: eff967bc c0934e20 00000000 c0042b30 c2b8a500 c2ba65c0 c3ac8880 f0901ebc
1f80: c00428f0 c3ac8f00 00000000 c00494c4 c2ba65c0 c00493f4 00000000 00000000
1fa0: 00000000 00000000 00000000 c0009108 00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
smsc911x_reg_read from smsc911x_mac_read+0x4c/0xa0
smsc911x_mac_read from smsc911x_mii_read+0x38/0xb4
smsc911x_mii_read from __mdiobus_read+0x70/0xc4
__mdiobus_read from mdiobus_read+0x34/0x48
mdiobus_read from genphy_update_link+0x10/0xc8
genphy_update_link from genphy_read_status+0x10/0xc4
genphy_read_status from lan87xx_read_status+0x10/0x11c
lan87xx_read_status from phy_check_link_status+0x5c/0xbc
phy_check_link_status from phy_state_machine+0x78/0x218
phy_state_machine from process_one_work+0x2f0/0x4c4
process_one_work from worker_thread+0x240/0x2d0
worker_thread from kthread+0xd0/0xe0
kthread from ret_from_fork+0x14/0x2c
Exception stack(0xf0891fb0 to 0xf0891ff8)
1fa0: 00000000 00000000 00000000 00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
Code: e5933000 e1a05000 e1a00004 e12fff33 (e1a01005)
---[ end trace 0000000000000000 ]---
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds