Upon system sleep, mdio_bus_phy_suspend() stops the phy_state_machine(), but subsequent interrupts may retrigger it: They may have been left enabled to facilitate wakeup and are not quiesced until the ->suspend_noirq() phase. Unwanted interrupts may hence occur between mdio_bus_phy_suspend() and dpm_suspend_noirq(), as well as between dpm_resume_noirq() and mdio_bus_phy_resume(). Retriggering the phy_state_machine() through an interrupt is not only undesirable for the reason given in mdio_bus_phy_suspend() (freezing it midway with phydev->lock held), but also because the PHY may be inaccessible after it's suspended: Accesses to USB-attached PHYs are blocked once usb_suspend_both() clears the can_submit flag and PHYs on PCI network cards may become inaccessible upon suspend as well. Amend phy_interrupt() to avoid triggering the state machine if the PHY is suspended. Signal wakeup instead if the attached net_device or its parent has been configured as a wakeup source. (Those conditions are identical to mdio_bus_phy_may_suspend().) Postpone handling of the interrupt until the PHY has resumed. Before stopping the phy_state_machine() in mdio_bus_phy_suspend(), wait for a concurrent phy_interrupt() to run to completion. That is necessary because phy_interrupt() may have checked the PHY's suspend status before the system sleep transition commenced and it may thus retrigger the state machine after it was stopped. Likewise, after re-enabling interrupt handling in mdio_bus_phy_resume(), wait for a concurrent phy_interrupt() to complete to ensure that interrupts which it postponed are properly rerun. The issue was exposed by commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling"), but has existed since forever.
Unified · Split
| @@ -31,6 +31,7 @@ · 30 unmodified lines | |||
| 31 | #include <linux/io.h> | 31 | #include <linux/io.h> |
| 32 | #include <linux/uaccess.h> | 32 | #include <linux/uaccess.h> |
| 33 | #include <linux/atomic.h> | 33 | #include <linux/atomic.h> |
| 34 | #include <linux/suspend.h> | ||
| 34 | #include <net/netlink.h> | 35 | #include <net/netlink.h> |
| 35 | #include <net/genetlink.h> | 36 | #include <net/genetlink.h> |
| 36 | #include <net/sock.h> | 37 | #include <net/sock.h> |
| @@ -976,6 +977,28 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat) · 939 unmodified lines | |||
| 976 | struct phy_driver *drv = phydev->drv; | 977 | struct phy_driver *drv = phydev->drv; |
| 977 | irqreturn_t ret; | 978 | irqreturn_t ret; |
| 978 | 979 | ||
| 980 | /* Wakeup interrupts may occur during a system sleep transition. | ||
| 981 | * Postpone handling until the PHY has resumed. | ||
| 982 | */ | ||
| 983 | if (IS_ENABLED(CONFIG_PM_SLEEP) && phydev->irq_suspended) { | ||
| 984 | struct net_device *netdev = phydev->attached_dev; | ||
| 985 | |||
| 986 | if (netdev) { | ||
| 987 | struct device *parent = netdev->dev.parent; | ||
| 988 | |||
| 989 | if (netdev->wol_enabled) | ||
| 990 | pm_system_wakeup(); | ||
| 991 | else if (device_may_wakeup(&netdev->dev)) | ||
| 992 | pm_wakeup_dev_event(&netdev->dev, 0, true); | ||
| 993 | else if (parent && device_may_wakeup(parent)) | ||
| 994 | pm_wakeup_dev_event(parent, 0, true); | ||
| 995 | } | ||
| 996 | |||
| 997 | phydev->irq_rerun = 1; | ||
| 998 | disable_irq_nosync(irq); | ||
| 999 | return IRQ_HANDLED; | ||
| 1000 | } | ||
| 1001 | |||
| 979 | mutex_lock(&phydev->lock); | 1002 | mutex_lock(&phydev->lock); |
| 980 | ret = drv->handle_interrupt(phydev); | 1003 | ret = drv->handle_interrupt(phydev); |
| 981 | mutex_unlock(&phydev->lock); | 1004 | mutex_unlock(&phydev->lock); |
| @@ -278,6 +278,15 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev) · 277 unmodified lines | |||
| 278 | if (phydev->mac_managed_pm) | 278 | if (phydev->mac_managed_pm) |
| 279 | return 0; | 279 | return 0; |
| 280 | 280 | ||
| 281 | /* Wakeup interrupts may occur during the system sleep transition when | ||
| 282 | * the PHY is inaccessible. Set flag to postpone handling until the PHY | ||
| 283 | * has resumed. Wait for concurrent interrupt handler to complete. | ||
| 284 | */ | ||
| 285 | if (phy_interrupt_is_valid(phydev)) { | ||
| 286 | phydev->irq_suspended = 1; | ||
| 287 | synchronize_irq(phydev->irq); | ||
| 288 | } | ||
| 289 | |||
| 281 | /* We must stop the state machine manually, otherwise it stops out of | 290 | /* We must stop the state machine manually, otherwise it stops out of |
| 282 | * control, possibly with the phydev->lock held. Upon resume, netdev | 291 | * control, possibly with the phydev->lock held. Upon resume, netdev |
| 283 | * may call phy routines that try to grab the same lock, and that may | 292 | * may call phy routines that try to grab the same lock, and that may |
| @@ -315,6 +324,20 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev) · 31 unmodified lines | |||
| 315 | if (ret < 0) | 324 | if (ret < 0) |
| 316 | return ret; | 325 | return ret; |
| 317 | no_resume: | 326 | no_resume: |
| 327 | if (phy_interrupt_is_valid(phydev)) { | ||
| 328 | phydev->irq_suspended = 0; | ||
| 329 | synchronize_irq(phydev->irq); | ||
| 330 | |||
| 331 | /* Rerun interrupts which were postponed by phy_interrupt() | ||
| 332 | * because they occurred during the system sleep transition. | ||
| 333 | */ | ||
| 334 | if (phydev->irq_rerun) { | ||
| 335 | phydev->irq_rerun = 0; | ||
| 336 | enable_irq(phydev->irq); | ||
| 337 | irq_wake_thread(phydev->irq, phydev); | ||
| 338 | } | ||
| 339 | } | ||
| 340 | |||
| 318 | if (phydev->attached_dev && phydev->adjust_link) | 341 | if (phydev->attached_dev && phydev->adjust_link) |
| 319 | phy_start_machine(phydev); | 342 | phy_start_machine(phydev); |
| 320 | 343 | ||
| @@ -572,6 +572,10 @@ struct macsec_ops; · 571 unmodified lines | |||
| 572 | * @mdix_ctrl: User setting of crossover | 572 | * @mdix_ctrl: User setting of crossover |
| 573 | * @pma_extable: Cached value of PMA/PMD Extended Abilities Register | 573 | * @pma_extable: Cached value of PMA/PMD Extended Abilities Register |
| 574 | * @interrupts: Flag interrupts have been enabled | 574 | * @interrupts: Flag interrupts have been enabled |
| 575 | * @irq_suspended: Flag indicating PHY is suspended and therefore interrupt | ||
| 576 | * handling shall be postponed until PHY has resumed | ||
| 577 | * @irq_rerun: Flag indicating interrupts occurred while PHY was suspended, | ||
| 578 | * requiring a rerun of the interrupt handler after resume | ||
| 575 | * @interface: enum phy_interface_t value | 579 | * @interface: enum phy_interface_t value |
| 576 | * @skb: Netlink message for cable diagnostics | 580 | * @skb: Netlink message for cable diagnostics |
| 577 | * @nest: Netlink nest used for cable diagnostics | 581 | * @nest: Netlink nest used for cable diagnostics |
| @@ -626,6 +630,8 @@ struct phy_device { · 48 unmodified lines | |||
| 626 | 630 | ||
| 627 | /* Interrupts are enabled */ | 631 | /* Interrupts are enabled */ |
| 628 | unsigned interrupts:1; | 632 | unsigned interrupts:1; |
| 633 | unsigned irq_suspended:1; | ||
| 634 | unsigned irq_rerun:1; | ||
| 629 | 635 | ||
| 630 | enum phy_state state; | 636 | enum phy_state state; |
| 631 | 637 | ||