Polling be gone on LAN95xx

Upon receiving data from the Interrupt Endpoint, the SMSC LAN95xx driver attempts to clear the signaled interrupts by writing "all ones" to the Interrupt Status Register.

However the driver only ever enables a single type of interrupt, namely the PHY Interrupt. And according to page 119 of the LAN950x datasheet, its bit in the Interrupt Status Register is read-only. There's no other way to clear it than in a separate PHY register:

https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf

Consequently, writing "all ones" to the Interrupt Status Register is pointless and can be dropped.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>

smsc95xx_reset() resets the PHY behind the PHY driver's back, which seems like a bad idea generally. Remove that portion of the function.

We're about to use PHY interrupts instead of polling to detect link changes on SMSC LAN95xx chips. Because smsc95xx_reset() is called from usbnet_open(), PHY interrupt settings are lost whenever the net_device is brought up.

There are two other callers of smsc95xx_reset(), namely smsc95xx_bind() and smsc95xx_reset_resume(), and both may indeed benefit from a PHY reset. However they already perform one through their calls to phy_connect_direct() and phy_init_hw().

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>

Cc Martyn Welch <[email protected]>

Cc Gabriel Hojda <[email protected]>

When a PHY interrupt is signaled, the SMSC LAN95xx driver updates the MAC full duplex mode and PHY flow control registers based on cached data in struct phy_device:

smsc95xx_status() # raises EVENT_LINK_RESET
usbnet_deferred_kevent()
smsc95xx_link_reset() # uses cached data in phydev

Simultaneously, phylib polls link status once per second and updates that cached data:

phy_state_machine()
phy_check_link_status()
phy_read_status()
lan87xx_read_status()
genphy_read_status() # updates cached data in phydev

If smsc95xx_link_reset() wins the race against genphy_read_status(), the registers may be updated based on stale data.

E.g. if the link was previously down, phydev->duplex is set to DUPLEX_UNKNOWN and that's what smsc95xx_link_reset() will use, even though genphy_read_status() may update it to DUPLEX_FULL afterwards.

PHY interrupts are currently only enabled on suspend to trigger wakeup, so the impact of the race is limited, but we're about to enable them perpetually.

Avoid the race by delaying execution of smsc95xx_link_reset() until phy_state_machine() has done its job and calls back via smsc95xx_handle_link_change().

Signaling EVENT_LINK_RESET on wakeup is not necessary because phylib picks up link status changes through polling. So drop the declaration of a ->link_reset() callback.

Note that the semicolon on a line by itself added in smsc95xx_status() is a placeholder for a function call which will be added in a subsequent commit. That function call will actually handle the INT_ENP_PHY_INT_ interrupt.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>

Link status of SMSC LAN95xx chips is polled once per second, even though they're capable of signaling PHY interrupts through the MAC layer.

Forward those interrupts to the PHY driver to avoid polling. Benefits are reduced bus traffic, reduced CPU overhead and quicker interface bringup.

Polling was introduced in 2016 by commit d69d16949346 ("usbnet:
smsc95xx: fix link detection for disabled autonegotiation"). Back then, the LAN95xx driver neglected to enable the ENERGYON interrupt, hence couldn't detect link-up events when auto-negotiation was disabled. The proper solution would have been to enable the ENERGYON interrupt instead of polling.

Since then, PHY handling was moved from the LAN95xx driver to the SMSC PHY driver with commit 05b35e7eb9a1 ("smsc95xx: add phylib support"). That PHY driver is capable of link detection with auto-negotiation disabled because it enables the ENERGYON interrupt.

Note that signaling interrupts through the MAC layer not only works with the integrated PHY, but also with an external PHY, provided its interrupt pin is attached to LAN95xx's nPHY_INT pin.

In the unlikely event that the interrupt pin of an external PHY is attached to a GPIO of the SoC (or not connected at all), the driver can be amended to retrieve the irq from the PHY's of_node.

To forward PHY interrupts to phylib, it is not sufficient to call phy_mac_interrupt(). Instead, the PHY's interrupt handler needs to run so that PHY interrupts are cleared. That's because according to page 119 of the LAN950x datasheet, "The source of this interrupt is a level. The interrupt persists until it is cleared in the PHY."

https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf

Therefore, create an IRQ domain with a single IRQ for the PHY. In the future, the IRQ domain may be extended to support the 11 GPIOs on the LAN95xx.

Normally the PHY interrupt should be masked until the PHY driver has cleared it. However masking requires a (sleeping) USB transaction and interrupts are received in (non-sleepable) softirq context. I decided not to mask the interrupt at all (by using the dummy_irq_chip's noop ->irq_mask() callback): The USB interrupt endpoint is polled in 1 msec intervals and normally that's sufficient to wake the PHY driver's IRQ thread and have it clear the interrupt. If it does take longer, worst thing that can happen is the IRQ thread is woken again. No big deal.

Because PHY interrupts are now perpetually enabled, there's no need to selectively enable them on suspend. So remove all invocations of smsc95xx_enable_phy_wakeup_interrupts().

In smsc95xx_resume(), move the call of phy_init_hw() before usbnet_resume() (which restarts the status URB) to ensure that the PHY is fully initialized when an interrupt is handled.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>

Reviewed-by Andrew Lunn <[email protected]> # from a PHY perspective

Cc Andre Edich <[email protected]>

---
Only change since v2:
* Drop call to __irq_enter_raw() which worked around a warning in generic_handle_domain_irq(). That warning is gone since 792ea6a074ae (queued on tip.git/irq/urgent).
(Marc Zyngier, Thomas Gleixner)

  drivers/net/usb/smsc95xx.c | 113 ++++++++++++++++++++-----------------
  1 file changed, 61 insertions(+), 52 deletions(-)

...

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Quoted 21 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change shortly before system sleep and by the time the phy_state_machine() worker gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

usb_suspend_both()
usb_suspend_interface()
smsc95xx_suspend()
usbnet_suspend()
__usbnet_status_stop_force() # stops interrupt polling,
# link change is signaled before this

   udev->can_submit = 0               # refuse further URBs

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

Taking a step back though, I'm wondering if there's a bigger problem here:
This is a USB device, so we stop receiving interrupts once the Interrupt Endpoint is no longer polled. But what if a PHY's interrupt is attached to a GPIO of the SoC and that interrupt is raised while the system is suspending? The interrupt handler may likewise try to reach an inaccessible (suspended) device.

The right thing to do would probably be to signal wakeup. But the PHY drivers' irq handlers instead schedule the phy_state_machine(). Perhaps we need something like the following at the top of phy_state_machine():

	if (phydev->suspended) {
		pm_wakeup_dev_event(&phydev->mdio.dev, 0, true);
		return;
	}

However, phydev->suspended is set at the *bottom* of phy_suspend(), it would have to be set at the *top* of mdio_bus_phy_suspend() for the above to be correct. Hmmm...

Thanks,

Lukas

-- >8 --

drivers/net/usb/smsc95xx.c +4 -0

index bd03e16..d351a6c 100644
@@ -1201,6 +1201,7 @@ static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
  	}
 
  	pdata->phydev->irq = phy_irq;
+	pdata->phydev->mac_managed_pm = true;
  	pdata->phydev->is_internal = is_internal_phy;
 
  	/* detect device revision as different features may be available */

@@ -1496,6 +1497,9 @@ static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
  		return ret;
  	}
 
+	if (netif_running(dev->net))
+		phy_stop(pdata->phydev);
+
  	if (pdata->suspend_flags) {
  		netdev_warn(dev->net, "error during last resume\n");
  		pdata->suspend_flags = 0;

Quoted 17 lines · Lukas Wunner

Taking a step back though, I'm wondering if there's a bigger problem here:
This is a USB device, so we stop receiving interrupts once the Interrupt Endpoint is no longer polled. But what if a PHY's interrupt is attached to a GPIO of the SoC and that interrupt is raised while the system is suspending? The interrupt handler may likewise try to reach an inaccessible (suspended) device.

The right thing to do would probably be to signal wakeup. But the PHY drivers' irq handlers instead schedule the phy_state_machine(). Perhaps we need something like the following at the top of phy_state_machine():

	if (phydev->suspended) {
		pm_wakeup_dev_event(&phydev->mdio.dev, 0, true);
		return;
	}

Quoted 3 lines

However, phydev->suspended is set at the *bottom* of phy_suspend(), it would have to be set at the *top* of mdio_bus_phy_suspend() for the above to be correct. Hmmm...

Well, your concern sounds valid, but I don't have a board with such hw configuration, so I cannot really test.

Quoted 18 lines · Marek Szyprowski

Quoted 17 lines · Lukas Wunner

Taking a step back though, I'm wondering if there's a bigger problem here:
This is a USB device, so we stop receiving interrupts once the Interrupt Endpoint is no longer polled. But what if a PHY's interrupt is attached to a GPIO of the SoC and that interrupt is raised while the system is suspending? The interrupt handler may likewise try to reach an inaccessible (suspended) device.

The right thing to do would probably be to signal wakeup. But the PHY drivers' irq handlers instead schedule the phy_state_machine(). Perhaps we need something like the following at the top of phy_state_machine():

	if (phydev->suspended) {
		pm_wakeup_dev_event(&phydev->mdio.dev, 0, true);
		return;
	}

Quoted 5 lines

Quoted 3 lines

However, phydev->suspended is set at the *bottom* of phy_suspend(), it would have to be set at the *top* of mdio_bus_phy_suspend() for the above to be correct. Hmmm...

Well, your concern sounds valid, but I don't have a board with such hw configuration, so I cannot really test.

I'm torn whether I should submit the quick fix in my last e-mail or attempt to address the deeper issue. The quick fix would ensure v5.19-rc1 isn't broken, but if possible I'd rather address the deeper issue...

Below is another patch. Would you mind testing if it fixes the problem for you? It's a replacement for the patch in my last e-mail and seeks to fix the problem for all drivers, not just smsc95xx. If you don't have time to test it, let me know and I'll just submit the quick fix in my previous e-mail.

BTW, getting a PHY interrupt on suspend seems like a corner case to me, so I'm amazed you found this and seem to be able to reproduce it 100%. Out of curiosity, is this a CI test you're performing?

Quoted 8 lines

drivers/net/phy/phy_device.c +1 -1

@@ -283,8 +283,11 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	 * may call phy routines that try to grab the same lock, and that may
 	 * lead to a deadlock.
 	 */
-	if (phydev->attached_dev && phydev->adjust_link)
+	if (phydev->attached_dev && phydev->adjust_link) {

drivers/net/phy/phy_device.c static __maybe_unused int mdio_bus_phy_suspend(struct device *dev) · line 290

Quoted 4 lines

drivers/net/phy/phy_device.c +3 -0

+		if (phy_interrupt_is_valid(phydev))
+			synchronize_irq(phydev->irq);
 		phy_stop_machine(phydev);
+	}

What is this hunk trying to achieve? As far as i know, interrupts have not been disabled. So as soon as the call to synchronize_irq() finishes, could well be another interrupt happens.

Quoted 8 lines · Andrew Lunn

Quoted 8 lines

drivers/net/phy/phy_device.c +1 -1

@@ -283,8 +283,11 @@ static __maybe_unused int mdio_bus_phy_suspend(struct device *dev)
 	 * may call phy routines that try to grab the same lock, and that may
 	 * lead to a deadlock.
 	 */
-	if (phydev->attached_dev && phydev->adjust_link)
+	if (phydev->attached_dev && phydev->adjust_link) {

Quoted 8 lines

drivers/net/phy/phy_device.c static __maybe_unused int mdio_bus_phy_suspend(struct device *dev) · line 290

Quoted 4 lines

drivers/net/phy/phy_device.c +3 -0

+		if (phy_interrupt_is_valid(phydev))
+			synchronize_irq(phydev->irq);
 		phy_stop_machine(phydev);
+	}

What is this hunk trying to achieve? As far as i know, interrupts have not been disabled. So as soon as the call to synchronize_irq() finishes, could well be another interrupt happens.

That other interrupt would bail out of phy_interrupt() because the is_prepared flag is set on the PHY's struct device, see first hunk of the patch.

The problem is that an interrupt may occur before the system sleep transition commences. phy_interrupt() will notice that is_prepared is not (yet) set, hence invokes drv->handle_interrupt(). Let's say the IRQ thread is preempted at that point, the system sleep transition is started and mdio_bus_phy_suspend() is run. It calls phy_stop_machine(), so the state machine is now stopped. Now phy_interrupt() continues, and the PHY driver's ->handle_interrupt() callback starts the state machine. Boom, that's not what we want.

So the synchronize_irq() ensures that any already running phy_interrupt() runs to completion before phy_stop_machine() is called. It doesn't matter if another interrupt occurs because then is_prepared will have been set and therefore phy_interrupt() won't call drv->handle_interrupt().

Let me know if I haven't explained it in sufficient clarity, I'll be happy to try again. :)

drivers/net/phy/phy.c +7 -0

@@ -976,6 +977,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	struct phy_driver *drv = phydev->drv;
 	irqreturn_t ret;
 
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+	    (phydev->mdio.dev.power.is_prepared ||
+	     phydev->mdio.dev.power.is_suspended)) {
+		struct net_device *netdev = phydev->attached_dev;
+
+		if (netdev) {
+			struct device *parent = netdev->dev.parent;

Quoted 11 lines

drivers/net/phy/phy.c +7 -0

@@ -976,6 +977,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
  	struct phy_driver *drv = phydev->drv;
  	irqreturn_t ret;
 
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+	    (phydev->mdio.dev.power.is_prepared ||
+	     phydev->mdio.dev.power.is_suspended)) {
+		struct net_device *netdev = phydev->attached_dev;
+
+		if (netdev) {
+			struct device *parent = netdev->dev.parent;

drivers/net/phy/phy.c static irqreturn_t phy_interrupt(int irq, void *phy_dat) · line 996

Quoted 10 lines

drivers/net/phy/phy.c +10 -0

+
+			if (netdev->wol_enabled)
+				pm_system_wakeup();
+			else if (device_may_wakeup(&netdev->dev))
+				pm_wakeup_dev_event(&netdev->dev, 0, true);
+			else if (parent && device_may_wakeup(parent))
+				pm_wakeup_dev_event(parent, 0, true);
+		}
+
+		return IRQ_HANDLED;

I'm not sure you can just throw the interrupt away. There have been issues with WoL, where the WoL signal has been applied to a PMC, not an actual interrupt. Yet the PHY driver assumes it is an interrupt. And in order for WoL to work correctly, it needs the interrupt handler to be called. We said the hardware is broken, WoL cannot work for that setup.

Here you have correct hardware, but you are throwing the interrupt away, which will have the same result. So i think you need to abort the suspend, get the bus working again, and call the interrupt handler. If this is a WoL interrupt you are supposed to be waking up anyway.

Quoted 28 lines · Andrew Lunn

Quoted 11 lines

drivers/net/phy/phy.c +7 -0

@@ -976,6 +977,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
  	struct phy_driver *drv = phydev->drv;
  	irqreturn_t ret;
 
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+	    (phydev->mdio.dev.power.is_prepared ||
+	     phydev->mdio.dev.power.is_suspended)) {
+		struct net_device *netdev = phydev->attached_dev;
+
+		if (netdev) {
+			struct device *parent = netdev->dev.parent;

drivers/net/phy/phy.c static irqreturn_t phy_interrupt(int irq, void *phy_dat) · line 996

Quoted 10 lines

drivers/net/phy/phy.c +10 -0

+
+			if (netdev->wol_enabled)
+				pm_system_wakeup();
+			else if (device_may_wakeup(&netdev->dev))
+				pm_wakeup_dev_event(&netdev->dev, 0, true);
+			else if (parent && device_may_wakeup(parent))
+				pm_wakeup_dev_event(parent, 0, true);
+		}
+
+		return IRQ_HANDLED;

I'm not sure you can just throw the interrupt away. There have been issues with WoL, where the WoL signal has been applied to a PMC, not an actual interrupt. Yet the PHY driver assumes it is an interrupt. And in order for WoL to work correctly, it needs the interrupt handler to be called. We said the hardware is broken, WoL cannot work for that setup.

Quoted 5 lines

Here you have correct hardware, but you are throwing the interrupt away, which will have the same result. So i think you need to abort the suspend, get the bus working again, and call the interrupt handler. If this is a WoL interrupt you are supposed to be waking up anyway.

This hardware doesn't support wake-on-lan. It looks somehow that it manages to throw an interrupt just a moment before the power regulator for the whole usb bus is cut off.

As for this hardware, if it does not support WOL, why are interrupts still enabled?

Quoted 11 lines

drivers/net/phy/phy.c +7 -0

@@ -976,6 +977,25 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
 	struct phy_driver *drv = phydev->drv;
 	irqreturn_t ret;
 
+	if (IS_ENABLED(CONFIG_PM_SLEEP) &&
+	    (phydev->mdio.dev.power.is_prepared ||
+	     phydev->mdio.dev.power.is_suspended)) {
+		struct net_device *netdev = phydev->attached_dev;
+
+		if (netdev) {
+			struct device *parent = netdev->dev.parent;

drivers/net/phy/phy.c static irqreturn_t phy_interrupt(int irq, void *phy_dat) · line 996

Quoted 10 lines

drivers/net/phy/phy.c +10 -0

+
+			if (netdev->wol_enabled)
+				pm_system_wakeup();
+			else if (device_may_wakeup(&netdev->dev))
+				pm_wakeup_dev_event(&netdev->dev, 0, true);
+			else if (parent && device_may_wakeup(parent))
+				pm_wakeup_dev_event(parent, 0, true);
+		}
+
+		return IRQ_HANDLED;

I'm not sure you can just throw the interrupt away. There have been issues with WoL, where the WoL signal has been applied to a PMC, not an actual interrupt. Yet the PHY driver assumes it is an interrupt. And in order for WoL to work correctly, it needs the interrupt handler to be called. We said the hardware is broken, WoL cannot work for that setup.

Here you have correct hardware, but you are throwing the interrupt away, which will have the same result. So i think you need to abort the suspend, get the bus working again, and call the interrupt handler. If this is a WoL interrupt you are supposed to be waking up anyway.

mdio_bus_phy_resume() does trigger the state machine via phy_start_machine(), so link state changes *are* detected after wakeup.

But you're saying that's not sufficient and you really want the PHY driver's IRQ handler to be called, do I understand that correctly?

Quoted 44 lines · Lukas Wunner

Quoted 23 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report.  It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

usb_suspend_both()
   usb_suspend_interface()
     smsc95xx_suspend()
       usbnet_suspend()
         __usbnet_status_stop_force() # stops interrupt polling,

                                      # link change is signaled 
before this

Quoted 7 lines

   udev->can_submit = 0               # refuse further URBs

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that.  Could you give it a spin?

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Quoted 14 lines

Taking a step back though, I'm wondering if there's a bigger problem here:
This is a USB device, so we stop receiving interrupts once the Interrupt Endpoint is no longer polled.  But what if a PHY's interrupt is attached to a GPIO of the SoC and that interrupt is raised while the system is suspending?  The interrupt handler may likewise try to reach an inaccessible (suspended) device.

The right thing to do would probably be to signal wakeup.  But the PHY drivers' irq handlers instead schedule the phy_state_machine(). Perhaps we need something like the following at the top of phy_state_machine():

Quoted 8 lines

    if (phydev->suspended) { pm_wakeup_dev_event(&phydev->mdio.dev, 0, true); return;
    }

However, phydev->suspended is set at the *bottom* of phy_suspend(), it would have to be set at the *top* of mdio_bus_phy_suspend() for the above to be correct.  Hmmm...

Well, your concern sounds valid, but I don't have a board with such hw configuration, so I cannot really test.

Quoted 29 lines

Thanks,

Lukas

-- >8 --

drivers/net/usb/smsc95xx.c +1 -0

index bd03e16..d351a6c 100644
@@ -1201,6 +1201,7 @@ static int smsc95xx_bind(struct usbnet *dev, 
 struct usb_interface *intf)
       }
         pdata->phydev->irq = phy_irq;
+    pdata->phydev->mac_managed_pm = true;
       pdata->phydev->is_internal = is_internal_phy;
         /* detect device revision as different features may be 

available */

drivers/net/usb/smsc95xx.c +2 -0

@@ -1496,6 +1497,9 @@ static int smsc95xx_suspend(struct 
 usb_interface *intf, pm_message_t message)
           return ret;
       }
   +    if (netif_running(dev->net))
+        phy_stop(pdata->phydev);
+
       if (pdata->suspend_flags) {
           netdev_warn(dev->net, "error during last resume\n");

          pdata->suspend_flags = 0;

drivers/net/usb/smsc95xx.c +0 -0

@@ -1778,6 +1782,8 @@ static int smsc95xx_resume(struct usb_interface 
 *intf)

drivers/net/usb/smsc95xx.c static int smsc95xx_resume(struct usb_interface *intf)

Quoted 7 lines

drivers/net/usb/smsc95xx.c +2 -0

       }
         phy_init_hw(pdata->phydev);
+    if (netif_running(dev->net))
+        phy_start(pdata->phydev);
         ret = usbnet_resume(intf);
       if (ret < 0)
 

Best regards

Quoted 36 lines · Marek Szyprowski

Quoted 35 lines · Lukas Wunner

Quoted 23 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

Quoted 8 lines

Quoted 5 lines

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Quoted 5 lines

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Gentle ping for the final patch...

Quoted 37 lines · Marek Szyprowski

Quoted 36 lines · Marek Szyprowski

Quoted 35 lines · Lukas Wunner

Quoted 23 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

Quoted 8 lines

Quoted 7 lines

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Gentle ping for the final patch...

Hm? Actually this issue is supposed to be fixed by mainline commit 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend").

The initial fix attempt that you're replying to should not be necessary with that commit.

Are you still seeing issues even with 1758bde2e4aa applied?
Or are you maybe using a custom downstream tree which is missing that commit?

Quoted 52 lines · Lukas Wunner

Quoted 37 lines · Marek Szyprowski

Quoted 36 lines · Marek Szyprowski

Quoted 35 lines · Lukas Wunner

Quoted 23 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

Quoted 8 lines

Quoted 7 lines

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Gentle ping for the final patch...

Quoted 8 lines

Hm? Actually this issue is supposed to be fixed by mainline commit 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend").

The initial fix attempt that you're replying to should not be necessary with that commit.

Are you still seeing issues even with 1758bde2e4aa applied?
Or are you maybe using a custom downstream tree which is missing that commit?

On Linux next-20220825 I still get the following warning during suspend/resume cycle:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1483 at drivers/net/phy/phy_device.c:323 
mdio_bus_phy_resume+0x10c/0x110
Modules linked in: exynos_gsc s5p_jpeg s5p_mfc videobuf2_dma_contig 
v4l2_mem2mem videobuf2_memops videobuf2_v4l2 videobuf2_common videodev 
mc s5p_cec
CPU: 0 PID: 1483 Comm: rtcwake Not tainted 6.0.0-rc2-next-20220825 #5482
Hardware name: Samsung Exynos (Flattened Device Tree)
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x58/0x70
  dump_stack_lvl from __warn+0xc8/0x220
  __warn from warn_slowpath_fmt+0x5c/0xb4
  warn_slowpath_fmt from mdio_bus_phy_resume+0x10c/0x110
  mdio_bus_phy_resume from dpm_run_callback+0x94/0x208
  dpm_run_callback from device_resume+0x124/0x21c
  device_resume from dpm_resume+0x108/0x278
  dpm_resume from dpm_resume_end+0xc/0x18
  dpm_resume_end from suspend_devices_and_enter+0x208/0x70c
  suspend_devices_and_enter from pm_suspend+0x364/0x430
  pm_suspend from state_store+0x68/0xc8
  state_store from kernfs_fop_write_iter+0x110/0x1d4
  kernfs_fop_write_iter from vfs_write+0x1c4/0x2ac
  vfs_write from ksys_write+0x5c/0xd4
  ksys_write from ret_fast_syscall+0x0/0x1c
Exception stack(0xf2ee5fa8 to 0xf2ee5ff0)

5fa0:                   00000004 0002b438 00000004 0002b438 00000004 00000000
5fc0: 00000004 0002b438 000291b0 00000004 0002b438 00000004 befd9c1c 00028160
5fe0: 0000006c befd9ae8 b6eb4148 b6f118a4

irq event stamp: 58381
hardirqs last  enabled at (58393): [<c019ff28>] vprintk_emit+0x320/0x344
hardirqs last disabled at (58400): [<c019fedc>] vprintk_emit+0x2d4/0x344
softirqs last  enabled at (58258): [<c0101694>] __do_softirq+0x354/0x618
softirqs last disabled at (58247): [<c012dd18>] __irq_exit_rcu+0x140/0x1ec
---[ end trace 0000000000000000 ]---

The mentioned patch fixes it.

Quoted 94 lines · Marek Szyprowski

Quoted 52 lines · Lukas Wunner

Quoted 37 lines · Marek Szyprowski

Quoted 36 lines · Marek Szyprowski

Quoted 35 lines · Lukas Wunner

Quoted 23 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 4 lines

It looks that the driver's suspend/resume operations might need some adjustments. After the system suspend/resume cycle the driver is not operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus refuses any further USB requests with -EHOSTUNREACH (-113):

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

Quoted 8 lines

Quoted 7 lines

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Gentle ping for the final patch...

Quoted 8 lines

Hm? Actually this issue is supposed to be fixed by mainline commit 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend").

The initial fix attempt that you're replying to should not be necessary with that commit.

Are you still seeing issues even with 1758bde2e4aa applied?
Or are you maybe using a custom downstream tree which is missing that commit?

On Linux next-20220825 I still get the following warning during suspend/resume cycle:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1483 at drivers/net/phy/phy_device.c:323
mdio_bus_phy_resume+0x10c/0x110
Modules linked in: exynos_gsc s5p_jpeg s5p_mfc videobuf2_dma_contig
v4l2_mem2mem videobuf2_memops videobuf2_v4l2 videobuf2_common videodev
mc s5p_cec
CPU: 0 PID: 1483 Comm: rtcwake Not tainted 6.0.0-rc2-next-20220825 #5482
Hardware name: Samsung Exynos (Flattened Device Tree)
   unwind_backtrace from show_stack+0x10/0x14
   show_stack from dump_stack_lvl+0x58/0x70
   dump_stack_lvl from __warn+0xc8/0x220
   __warn from warn_slowpath_fmt+0x5c/0xb4
   warn_slowpath_fmt from mdio_bus_phy_resume+0x10c/0x110
   mdio_bus_phy_resume from dpm_run_callback+0x94/0x208
   dpm_run_callback from device_resume+0x124/0x21c
   device_resume from dpm_resume+0x108/0x278
   dpm_resume from dpm_resume_end+0xc/0x18
   dpm_resume_end from suspend_devices_and_enter+0x208/0x70c
   suspend_devices_and_enter from pm_suspend+0x364/0x430
   pm_suspend from state_store+0x68/0xc8
   state_store from kernfs_fop_write_iter+0x110/0x1d4
   kernfs_fop_write_iter from vfs_write+0x1c4/0x2ac
   vfs_write from ksys_write+0x5c/0xd4
   ksys_write from ret_fast_syscall+0x0/0x1c
Exception stack(0xf2ee5fa8 to 0xf2ee5ff0)

5fa0:                   00000004 0002b438 00000004 0002b438 00000004 00000000
5fc0: 00000004 0002b438 000291b0 00000004 0002b438 00000004 befd9c1c 00028160
5fe0: 0000006c befd9ae8 b6eb4148 b6f118a4

Quoted 8 lines

irq event stamp: 58381
hardirqs last  enabled at (58393): [<c019ff28>] vprintk_emit+0x320/0x344
hardirqs last disabled at (58400): [<c019fedc>] vprintk_emit+0x2d4/0x344
softirqs last  enabled at (58258): [<c0101694>] __do_softirq+0x354/0x618
softirqs last disabled at (58247): [<c012dd18>] __irq_exit_rcu+0x140/0x1ec
---[ end trace 0000000000000000 ]---

The mentioned patch fixes it.

Color me confused.

With "the mentioned patch", are you referring to 1758bde2e4aa or are you referring to the little test patch in this e-mail:
https://lore.kernel.org/netdev/[email protected]/

There's a Tested-by from you on 1758bde2e4aa, so I assume the commit fixed the issue at the time. Does it still occur intermittently? Or does it occur every time? In the latter case, I'd assume some other change was made in the meantime and that other change exposed the issue again...

Quoted 125 lines · Lukas Wunner

Quoted 117 lines · Marek Szyprowski

Quoted 64 lines · Lukas Wunner

Quoted 44 lines · Marek Szyprowski

Quoted 43 lines · Marek Szyprowski

Quoted 42 lines · Lukas Wunner

Quoted 26 lines · Marek Szyprowski

This patch landed in the recent linux next-20220516 as commit 1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation
after system suspend-resume cycle. On the Odroid XU3 board I got the
following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 
09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) 
done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946
phy_state_machine+0x98/0x28c

[...]

Quoted 7 lines

It looks that the driver's suspend/resume operations might need some
adjustments. After the system suspend/resume cycle the driver is not
operational anymore. Reverting the $subject patch on top of linux next-20220516 restores ethernet operation after system suspend/resume.

Thanks a lot for the report. It seems the PHY is signaling a link change
shortly before system sleep and by the time the phy_state_machine() worker
gets around to handle it, the device has already been suspended and thus
refuses any further USB requests with -EHOSTUNREACH (-113):

[...]

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Assuming the above theory is correct, calling phy_stop_machine() after usbnet_suspend() would be sufficient to fix the issue. It cancels the phy_state_machine() worker.

The small patch below does that. Could you give it a spin?

That's it. Your analysis is right and the patch fixes the issue. Thanks!

Quoted 6 lines

Quoted 5 lines

Feel free to add:

Reported-by Marek Szyprowski <[email protected]>

Tested-by Marek Szyprowski <[email protected]>

Gentle ping for the final patch...

Hm?  Actually this issue is supposed to be fixed by mainline commit 1758bde2e4aa ("net: phy: Don't trigger state machine while in suspend").

Quoted 7 lines

The initial fix attempt that you're replying to should not be necessary
with that commit.

Are you still seeing issues even with 1758bde2e4aa applied? Or are you maybe using a custom downstream tree which is missing that commit?

On Linux next-20220825 I still get the following warning during suspend/resume cycle:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1483 at drivers/net/phy/phy_device.c:323
mdio_bus_phy_resume+0x10c/0x110
Modules linked in: exynos_gsc s5p_jpeg s5p_mfc videobuf2_dma_contig
v4l2_mem2mem videobuf2_memops videobuf2_v4l2 videobuf2_common videodev
mc s5p_cec
CPU: 0 PID: 1483 Comm: rtcwake Not tainted 6.0.0-rc2-next-20220825 
#5482
Hardware name: Samsung Exynos (Flattened Device Tree)
   unwind_backtrace from show_stack+0x10/0x14
   show_stack from dump_stack_lvl+0x58/0x70
   dump_stack_lvl from __warn+0xc8/0x220
   __warn from warn_slowpath_fmt+0x5c/0xb4
   warn_slowpath_fmt from mdio_bus_phy_resume+0x10c/0x110
   mdio_bus_phy_resume from dpm_run_callback+0x94/0x208
   dpm_run_callback from device_resume+0x124/0x21c
   device_resume from dpm_resume+0x108/0x278
   dpm_resume from dpm_resume_end+0xc/0x18
   dpm_resume_end from suspend_devices_and_enter+0x208/0x70c
   suspend_devices_and_enter from pm_suspend+0x364/0x430
   pm_suspend from state_store+0x68/0xc8
   state_store from kernfs_fop_write_iter+0x110/0x1d4
   kernfs_fop_write_iter from vfs_write+0x1c4/0x2ac
   vfs_write from ksys_write+0x5c/0xd4
   ksys_write from ret_fast_syscall+0x0/0x1c
Exception stack(0xf2ee5fa8 to 0xf2ee5ff0)

5fa0:                   00000004 0002b438 00000004 0002b438 00000004 00000000
5fc0: 00000004 0002b438 000291b0 00000004 0002b438 00000004 befd9c1c 00028160
5fe0: 0000006c befd9ae8 b6eb4148 b6f118a4

irq event stamp: 58381
hardirqs last  enabled at (58393): [<c019ff28>] 
vprintk_emit+0x320/0x344
hardirqs last disabled at (58400): [<c019fedc>] 
vprintk_emit+0x2d4/0x344
softirqs last  enabled at (58258): [<c0101694>] 
__do_softirq+0x354/0x618
softirqs last disabled at (58247): [<c012dd18>] 
__irq_exit_rcu+0x140/0x1ec
---[ end trace 0000000000000000 ]---

Quoted 1 line

The mentioned patch fixes it.

Color me confused.

With "the mentioned patch", are you referring to 1758bde2e4aa or are you referring to the little test patch in this e-mail:
https://lore.kernel.org/netdev/[email protected]/

Quoted 5 lines

There's a Tested-by from you on 1758bde2e4aa, so I assume the commit fixed the issue at the time.  Does it still occur intermittently?  Or does it occur every time?  In the latter case, I'd assume some other change was made in the meantime and that other change exposed the issue again...

The issue seems to be exposed again. On 1758bde2e4aa everything works fine and there is no warning during suspend/resume cycle. Then I've noticed that warning again while playing with current linux-next. I will check when it got broken and report again.

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

[cc += Florian]

Quoted 10 lines · Marek Szyprowski

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Quoted 4 lines

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's no need to call phy_{stop,start}() >
744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion of the fallout.

However the WARN() condition still seems too broad and causes false positives such as in your case. In particular, mdio_bus_phy_suspend() may leave the device in PHY_UP state, so that's a legal state that needs to be exempted from the WARN().

Does the issue still appear even after 6dbe852c379f?

If it does, could you test whether exempting PHY_UP silences the gratuitous WARN splat? I.e.:

Quoted 21 lines · Lukas Wunner

Quoted 10 lines · Marek Szyprowski

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Quoted 4 lines

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's no need to call phy_{stop,start}() >
744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion of the fallout.

Quoted 4 lines

However the WARN() condition still seems too broad and causes false positives such as in your case. In particular, mdio_bus_phy_suspend() may leave the device in PHY_UP state, so that's a legal state that needs to be exempted from the WARN().

How is that a legal state when the PHY should be suspended? Even if we are interrupt driven, the state machine should be stopped, does not mean that Wake-on-LAN or other activity interrupts should be disabled.

Quoted 4 lines

Does the issue still appear even after 6dbe852c379f?

If it does, could you test whether exempting PHY_UP silences the gratuitous WARN splat? I.e.:

If you allow PHY_UP, then the warning becomes effectively useless, so I don't believe this is quite what you want to do here.

Quoted 22 lines · Florian Fainelli

Quoted 21 lines · Lukas Wunner

Quoted 10 lines · Marek Szyprowski

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Quoted 4 lines

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's no need to call phy_{stop,start}() >
744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion of the fallout.

Quoted 8 lines

Quoted 4 lines

However the WARN() condition still seems too broad and causes false positives such as in your case. In particular, mdio_bus_phy_suspend() may leave the device in PHY_UP state, so that's a legal state that needs to be exempted from the WARN().

How is that a legal state when the PHY should be suspended? Even if we are interrupt driven, the state machine should be stopped, does not mean that Wake-on-LAN or other activity interrupts should be disabled.

mdio_bus_phy_suspend()
phy_stop_machine()

     phydev->state = PHY_UP  #  if (phydev->state >= PHY_UP)

So apparently PHY_UP is a legal state for a suspended PHY.

Quoted 9 lines

Quoted 6 lines

Does the issue still appear even after 6dbe852c379f?

If it does, could you test whether exempting PHY_UP silences the gratuitous WARN splat? I.e.:

If you allow PHY_UP, then the warning becomes effectively useless, so I don't believe this is quite what you want to do here.

Hm, maybe the WARN() should be dropped altogether?

Quoted 36 lines · Lukas Wunner

Quoted 22 lines · Florian Fainelli

Quoted 21 lines · Lukas Wunner

Quoted 10 lines · Marek Szyprowski

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Quoted 4 lines

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's no need to call phy_{stop,start}() >
744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion of the fallout.

Quoted 8 lines

Quoted 4 lines

However the WARN() condition still seems too broad and causes false positives such as in your case. In particular, mdio_bus_phy_suspend() may leave the device in PHY_UP state, so that's a legal state that needs to be exempted from the WARN().

How is that a legal state when the PHY should be suspended? Even if we are interrupt driven, the state machine should be stopped, does not mean that Wake-on-LAN or other activity interrupts should be disabled.

mdio_bus_phy_suspend()
phy_stop_machine()

     phydev->state = PHY_UP  #  if (phydev->state >= PHY_UP)

Quoted 1 line

So apparently PHY_UP is a legal state for a suspended PHY.

It is not clear to me why, however. Sure it does ensure that when we resume we set needs_aneg = true but this feels like a hack in the sense that we are setting the PHY in a provisional state in anticipation for what might come next.

Quoted 4 lines

Quoted 2 lines

If you allow PHY_UP, then the warning becomes effectively useless, so I don't believe this is quite what you want to do here.

Hm, maybe the WARN() should be dropped altogether?

And then be left with debugging similar problems that prompted me to submit the patch in the first place, no thank you. I guess I would rather accept that PHY_UP needs to be special cased then.

[cc += Florian]

Quoted 10 lines · Marek Szyprowski

I've finally traced what has happened. I've double checked and indeed the 1758bde2e4aa commit fixed the issue on next-20220516 kernel and as such it has been merged to linus tree. Then the commit 744d23c71af3 ("net: phy: Warn about incorrect mdio_bus_phy_resume() state") has been merged to linus tree, which triggers a new warning during the suspend/resume cycle with smsc95xx driver. Please note, that the smsc95xx still works fine regardless that warning. However it look that the commit 1758bde2e4aa only hide a real problem, which the commit 744d23c71af3 warns about.

Quoted 4 lines

Probably a proper fix for smsc95xx driver is to call phy_stop/start during suspend/resume cycle, like in similar patches for other drivers:

https://lore.kernel.org/all/[email protected]/

No, smsc95xx.c relies on mdio_bus_phy_{suspend,resume}() and there's no need to call phy_{stop,start}().

744d23c71af3 was flawed and 6dbe852c379f has already fixed a portion of the fallout.

However the WARN() condition still seems too broad and causes false positives such as in your case. In particular, mdio_bus_phy_suspend() may leave the device in PHY_UP state, so that's a legal state that needs to be exempted from the WARN().

Does the issue still appear even after 6dbe852c379f?

If it does, could you test whether exempting PHY_UP silences the gratuitous WARN splat? I.e.:

Cache the interrupt mask to avoid re-reading it from the PHY upon every interrupt.

This will simplify a subsequent commit which detects hot-removal in the interrupt handler and bails out.

Analyzing and debugging PHY transactions also becomes simpler if such redundant reads are avoided.

Last not least, interrupt overhead and latency is slightly improved.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>

If reading the Interrupt Source Flag register fails with -ENODEV, then the PHY has been hot-removed and the correct response is to bail out instead of throwing a WARN splat and attempting to suspend the PHY. The PHY should be stopped in due course anyway as the kernel asynchronously tears down the device.

Tested-by Oleksij Rempel <[email protected]> # LAN9514/9512/9500

Tested-by Ferry Toth <[email protected]> # LAN9514

Signed-off-by Lukas Wunner <[email protected]>