Messages in this thread | | | Date | Tue, 16 Aug 2022 13:18:24 +0200 | Subject | Re: [PATCH net] net: phy: Warn about incorrect mdio_bus_phy_resume() state | From | Marek Szyprowski <> |
| |
On 12.08.2022 18:32, Florian Fainelli wrote: > On 8/12/22 04:19, Marek Szyprowski wrote: >> Hi All, >> >> 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@gmail.com> >> >> 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 >> Modules linked in: smsc911x cpufreq_powersave cpufreq_conservative >> crct10dif_ce ip_tables x_tables ipv6 [last unloaded: smsc911x] >> CPU: 1 PID: 398 Comm: rtcwake Not tainted 5.19.0+ #940 >> Hardware name: ARM Juno development board (r1) (DT) >> pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--) >> pc : mdio_bus_phy_resume+0x34/0xc8 >> lr : dpm_run_callback+0x74/0x350 >> ... >> Call trace: >> mdio_bus_phy_resume+0x34/0xc8 >> dpm_run_callback+0x74/0x350 >> device_resume+0xb8/0x258 >> dpm_resume+0x120/0x4a8 >> dpm_resume_end+0x14/0x28 >> suspend_devices_and_enter+0x164/0xa60 >> pm_suspend+0x25c/0x3a8 >> state_store+0x84/0x108 >> kobj_attr_store+0x14/0x28 >> sysfs_kf_write+0x60/0x70 >> kernfs_fop_write_iter+0x124/0x1a8 >> new_sync_write+0xd0/0x190 >> vfs_write+0x208/0x478 >> ksys_write+0x64/0xf0 >> __arm64_sys_write+0x14/0x20 >> invoke_syscall+0x40/0xf8 >> el0_svc_common.constprop.3+0x8c/0x120 >> do_el0_svc+0x28/0xc8 >> el0_svc+0x48/0xd0 >> el0t_64_sync_handler+0x94/0xb8 >> el0t_64_sync+0x15c/0x160 >> irq event stamp: 24406 >> hardirqs last enabled at (24405): [<ffff8000090c4734>] >> _raw_spin_unlock_irqrestore+0x8c/0x90 >> hardirqs last disabled at (24406): [<ffff8000090b3164>] >> el1_dbg+0x24/0x88 >> softirqs last enabled at (24144): [<ffff800008010488>] >> _stext+0x488/0x5cc >> softirqs last disabled at (24139): [<ffff80000809bf98>] >> irq_exit_rcu+0x168/0x1a8 >> ---[ end trace 0000000000000000 ]--- >> >> I hope the above information will help fixing the driver. > > 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:
Right, this fixes the warning (after fixing the minor compile issue, see below).
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > diff --git a/drivers/net/ethernet/smsc/smsc911x.c > b/drivers/net/ethernet/smsc/smsc911x.c > index 3bf20211cceb..e9c0668a4dc0 100644 > --- 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); phy_suspend(ndev->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); phy_resume(ndev->phydev); > } > > return 0; > Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
| |