[PATCH] r8152: Suspend USB device before shutdown when WoL is enabled

[PATCH] r8152: Suspend USB device before shutdown when WoL is enabled

[Alexandru Gagniuc]

For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
in U3 state. If it is not, and the host "disappears", the chip will
no longer respond to WoL triggers.

To resolve this, add a notifier block and register it as a reboot
notifier. When WoL is enabled, work through the usb_device struct to
get to the suspend function. Calling this function puts the link in
the correct state for WoL to function.

Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Gagniuc <alexandru.gagniuc@hp.com>
---
 drivers/net/usb/r8152.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0999a58ca9d2..5623ca5c9142 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -20,6 +20,7 @@
 #include <net/ip6_checksum.h>
 #include <uapi/linux/mdio.h>
 #include <linux/mdio.h>
+#include <linux/reboot.h>
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
 #include <linux/atomic.h>
@@ -875,6 +876,7 @@ struct r8152 {
 	struct delayed_work schedule, hw_phy_work;
 	struct mii_if_info mii;
 	struct mutex control;	/* use for hw setting */
+	struct notifier_block reboot_notifier;
 #ifdef CONFIG_PM_SLEEP
 	struct notifier_block pm_notifier;
 #endif
@@ -9609,6 +9611,25 @@ static bool rtl8152_supports_lenovo_macpassthru(struct usb_device *udev)
 	return 0;
 }
 
+/* Suspend realtek chip before system shutdown
+ *
+ * For Wake-on-LAN to work from S5, the USB link must be put in U3 state. If
+ * the host otherwise "disappears", the chip will not respond to WoL triggers.
+ */
+static int rtl8152_notify(struct notifier_block *nb, unsigned long code,
+			  void *unused)
+{
+	struct r8152 *tp = container_of(nb, struct r8152, reboot_notifier);
+	struct device *dev = &tp->udev->dev;
+
+	if (code == SYS_POWER_OFF) {
+		if (tp->saved_wolopts && dev->type->pm->suspend)
+			dev->type->pm->suspend(dev);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int rtl8152_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
@@ -9791,6 +9812,9 @@ static int rtl8152_probe(struct usb_interface *intf,
 	else
 		device_set_wakeup_enable(&udev->dev, false);
 
+	tp->reboot_notifier.notifier_call = rtl8152_notify;
+	register_reboot_notifier(&tp->reboot_notifier);
+
 	netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
 
 	return 0;
@@ -9811,6 +9835,7 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 	if (tp) {
 		rtl_set_unplug(tp);
 
+		unregister_reboot_notifier(&tp->reboot_notifier);
 		unregister_netdev(tp->netdev);
 		tasklet_kill(&tp->tx_tl);
 		cancel_delayed_work_sync(&tp->hw_phy_work);
-- 
2.39.1

Re: [PATCH] r8152: Suspend USB device before shutdown when WoL is enabled

[Jakub Kicinski]

On Thu,  6 Jul 2023 18:28:58 +0000 Alexandru Gagniuc wrote:
> For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> in U3 state. If it is not, and the host "disappears", the chip will
> no longer respond to WoL triggers.
>  
> To resolve this, add a notifier block and register it as a reboot
> notifier. When WoL is enabled, work through the usb_device struct to
> get to the suspend function. Calling this function puts the link in
> the correct state for WoL to function.

Would be good to hear from USB experts on this one, to an outside seems
like something that the bus should be doing, possibly based on some
driver opt-in..

> Cc: stable@vger.kernel.org
> Signed-off-by: Alexandru Gagniuc <alexandru.gagniuc@hp.com>

Please add a Fixes tag - I'm guessing it dates back to

Fixes: 21ff2e8976b1 ("r8152: support WOL")

?
-- 
pw-bot: cr

Re: [PATCH] r8152: Suspend USB device before shutdown when WoL is enabled

[Alan Stern]

On Fri, Jul 07, 2023 at 05:12:25PM -0700, Jakub Kicinski wrote:
> On Thu,  6 Jul 2023 18:28:58 +0000 Alexandru Gagniuc wrote:
> > For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> > in U3 state. If it is not, and the host "disappears", the chip will
> > no longer respond to WoL triggers.
> >  
> > To resolve this, add a notifier block and register it as a reboot
> > notifier. When WoL is enabled, work through the usb_device struct to
> > get to the suspend function. Calling this function puts the link in
> > the correct state for WoL to function.
> 
> Would be good to hear from USB experts on this one, to an outside seems
> like something that the bus should be doing, possibly based on some
> driver opt-in..

The USB spec does not include any discussion of what things should be 
done when the system is turned off -- it doesn't even really acknowledge 
the existence of different system-wide power states.  As a result, the 
USB subsystem never developed any support for power-off callbacks or 
anything else of the sort.

Of course, this kind of thing can always be added.  But I don't think 
there's any way to distinguish (at the USB level) between wakeup from 
S5-off and wakeup from any other low-power system state.  And the PM 
part of the device model doesn't have multiple types of "enable-wakeup" 
flags -- either a device is enabled for wakeup or it isn't.

Alan Stern

[PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alexandru Gagniuc]

For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
in U3 state. If it is not, and the host "disappears", the chip will
no longer respond to WoL triggers.

To resolve this, add a notifier block and register it as a reboot
notifier. When WoL is enabled, work through the usb_device struct to
get to the suspend function. Calling this function puts the link in
the correct state for WoL to function.

Fixes: 21ff2e8976b1 ("r8152: support WOL")
Cc: stable@vger.kernel.org
Signed-off-by: Alexandru Gagniuc <alexandru.gagniuc@hp.com>
---
Changes since v1:
    * Add "Fixes:" tag to commit message

 drivers/net/usb/r8152.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0738baa5b82e..abb82a80d262 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -20,6 +20,7 @@
 #include <net/ip6_checksum.h>
 #include <uapi/linux/mdio.h>
 #include <linux/mdio.h>
+#include <linux/reboot.h>
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
 #include <linux/atomic.h>
@@ -876,6 +877,7 @@ struct r8152 {
 	struct delayed_work schedule, hw_phy_work;
 	struct mii_if_info mii;
 	struct mutex control;	/* use for hw setting */
+	struct notifier_block reboot_notifier;
 #ifdef CONFIG_PM_SLEEP
 	struct notifier_block pm_notifier;
 #endif
@@ -9610,6 +9612,25 @@ static bool rtl8152_supports_lenovo_macpassthru(struct usb_device *udev)
 	return 0;
 }
 
+/* Suspend realtek chip before system shutdown
+ *
+ * For Wake-on-LAN to work from S5, the USB link must be put in U3 state. If
+ * the host otherwise "disappears", the chip will not respond to WoL triggers.
+ */
+static int rtl8152_notify(struct notifier_block *nb, unsigned long code,
+			  void *unused)
+{
+	struct r8152 *tp = container_of(nb, struct r8152, reboot_notifier);
+	struct device *dev = &tp->udev->dev;
+
+	if (code == SYS_POWER_OFF) {
+		if (tp->saved_wolopts && dev->type->pm->suspend)
+			dev->type->pm->suspend(dev);
+	}
+
+	return NOTIFY_DONE;
+}
+
 static int rtl8152_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
@@ -9792,6 +9813,9 @@ static int rtl8152_probe(struct usb_interface *intf,
 	else
 		device_set_wakeup_enable(&udev->dev, false);
 
+	tp->reboot_notifier.notifier_call = rtl8152_notify;
+	register_reboot_notifier(&tp->reboot_notifier);
+
 	netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
 
 	return 0;
@@ -9812,6 +9836,7 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 	if (tp) {
 		rtl_set_unplug(tp);
 
+		unregister_reboot_notifier(&tp->reboot_notifier);
 		unregister_netdev(tp->netdev);
 		tasklet_kill(&tp->tx_tl);
 		cancel_delayed_work_sync(&tp->hw_phy_work);
-- 
2.39.1

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alan Stern]

On Wed, Jul 19, 2023 at 05:37:56PM +0000, Alexandru Gagniuc wrote:
> For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> in U3 state. If it is not, and the host "disappears", the chip will
> no longer respond to WoL triggers.
> 
> To resolve this, add a notifier block and register it as a reboot
> notifier. When WoL is enabled, work through the usb_device struct to
> get to the suspend function. Calling this function puts the link in
> the correct state for WoL to function.

How do you know that the link will _remain_ in the correct state?

That is, how do you know that the shutdown processing for the USB host 
controller won't disable the link entirely, thereby preventing WoL from 
working?

Alan Stern

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Gagniuc, Alexandru]

On Wed, Jul 19, 2023 at 02:36:25PM -0400, Alan Stern wrote:
> On Wed, Jul 19, 2023 at 05:37:56PM +0000, Alexandru Gagniuc wrote:
> > For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> > in U3 state. If it is not, and the host "disappears", the chip will
> > no longer respond to WoL triggers.
> > 
> > To resolve this, add a notifier block and register it as a reboot
> > notifier. When WoL is enabled, work through the usb_device struct to
> > get to the suspend function. Calling this function puts the link in
> > the correct state for WoL to function.
> 
> How do you know that the link will _remain_ in the correct state?

The objective is to get to xhci_set_link_state() with the USB_SS_PORT_LS_U3
argument. This is achieved through usb_port_suspend() in drivers/usb/host/hub.c,
and the function is implemented in drivers/usb/host/xhci-hub.c.

This is the only path in the kernel that I am aware of for setting the U3 link
state. Given that it is part of the USB subsystem, I am fairly confident it will
show consistent behavior across platforms.
 
> That is, how do you know that the shutdown processing for the USB host 
> controller won't disable the link entirely, thereby preventing WoL from 
> working?

We are talking to the USB hub in order to set the link state. I don't see how
specifics of the host controller would influence behavior. I do expect a
controller which advertises S4/S5 in /proc/acpi/wakeup to not do anything that
would sabotage this capability. Disabling the link entirely would probalby
violate that promise.

Think of USB-C docks with a power button showing up as a HID class. The scenario
herein would disable the power button. I would take that to be a bug in the host
controller driver if the S4/S5 capability is advertised.

Alex

P.S. I sincerely apologize for the delay in my reply. The corporate email servers here
have "difficulties" with plaintext and  interleaved replies.

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alan Stern]

On Wed, Aug 02, 2023 at 02:56:43PM +0000, Gagniuc, Alexandru wrote:
> On Wed, Jul 19, 2023 at 02:36:25PM -0400, Alan Stern wrote:
> > On Wed, Jul 19, 2023 at 05:37:56PM +0000, Alexandru Gagniuc wrote:
> > > For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> > > in U3 state. If it is not, and the host "disappears", the chip will
> > > no longer respond to WoL triggers.
> > > 
> > > To resolve this, add a notifier block and register it as a reboot
> > > notifier. When WoL is enabled, work through the usb_device struct to
> > > get to the suspend function. Calling this function puts the link in
> > > the correct state for WoL to function.
> > 
> > How do you know that the link will _remain_ in the correct state?
> 
> The objective is to get to xhci_set_link_state() with the USB_SS_PORT_LS_U3
> argument. This is achieved through usb_port_suspend() in drivers/usb/host/hub.c,
> and the function is implemented in drivers/usb/host/xhci-hub.c.
> 
> This is the only path in the kernel that I am aware of for setting the U3 link
> state. Given that it is part of the USB subsystem, I am fairly confident it will
> show consistent behavior across platforms.

That does not answer my question.  I agree that making this change will 
put the link into the U3 state.  But I don't have any reason to think 
that some other software won't later put the link into some other state.

> > That is, how do you know that the shutdown processing for the USB host 
> > controller won't disable the link entirely, thereby preventing WoL from 
> > working?
> 
> We are talking to the USB hub in order to set the link state. I don't see how
> specifics of the host controller would influence behavior.

Specifics of the host controller probably won't influence behavior.  
However, specifics of the _software_ can make a big difference.

>  I do expect a
> controller which advertises S4/S5 in /proc/acpi/wakeup to not do anything that
> would sabotage this capability. Disabling the link entirely would probalby
> violate that promise.

Not if the kernel _tells_ the controller to disable the link.

> Think of USB-C docks with a power button showing up as a HID class. The scenario
> herein would disable the power button. I would take that to be a bug in the host
> controller driver if the S4/S5 capability is advertised.

Indeed.  And I am asking how you can be sure the host controller driver 
(or some other part of the software stack) doesn't have this bug.

Alan Stern

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alexandru Gagniuc]

From: Alan Stern <stern@rowland.harvard.edu>

On Wed, Aug 02, 2023 at 11:23:46AM -0400, Alan Stern wrote:
> On Wed, Aug 02, 2023 at 02:56:43PM +0000, Gagniuc, Alexandru wrote:
> > On Wed, Jul 19, 2023 at 02:36:25PM -0400, Alan Stern wrote:
> > > How do you know that the link will _remain_ in the correct state?
> > 
> > The objective is to get to xhci_set_link_state() with the USB_SS_PORT_LS_U3
> > argument. This is achieved through usb_port_suspend() in drivers/usb/host/hub.c,
> > and the function is implemented in drivers/usb/host/xhci-hub.c.
> > 
> > This is the only path in the kernel that I am aware of for setting the U3 link
> > state. Given that it is part of the USB subsystem, I am fairly confident it will
> > show consistent behavior across platforms.
> 
> That does not answer my question.  I agree that making this change will 
> put the link into the U3 state.  But I don't have any reason to think 
> that some other software won't later put the link into some other state.

I don't have a rigurous proof that the link will remain in the correct state.
The only conjecture that I can make is that no other software besides the kernel
will be running at this time. Thus, if the kernel manages to not break the link
state, things should work as intended.

> > > That is, how do you know that the shutdown processing for the USB host 
> > > controller won't disable the link entirely, thereby preventing WoL from 
> > > working?
> > 
> > We are talking to the USB hub in order to set the link state. I don't see how
> > specifics of the host controller would influence behavior.
> 
> Specifics of the host controller probably won't influence behavior.  
> However, specifics of the _software_ can make a big difference.
> 
> >  I do expect a
> > controller which advertises S4/S5 in /proc/acpi/wakeup to not do anything that
> > would sabotage this capability. Disabling the link entirely would probalby
> > violate that promise.
> 
> Not if the kernel _tells_ the controller to disable the link.
> 
> > Think of USB-C docks with a power button showing up as a HID class. The scenario
> > herein would disable the power button. I would take that to be a bug in the host
> > controller driver if the S4/S5 capability is advertised.
> 
> Indeed.  And I am asking how you can be sure the host controller driver 
> (or some other part of the software stack) doesn't have this bug.

The only way that I have to show that is empirical. I observe that WoL from S5
does not work on a device with an r8153 chip. I apply the change, and verify
that WoL from S5 now works in this scenario. What are you thinking of in terms
of being sure no current or future bug exists?

Alex

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alan Stern]

On Thu, Aug 10, 2023 at 04:22:16PM +0000, Alexandru Gagniuc wrote:
> From: Alan Stern <stern@rowland.harvard.edu>
> 
> On Wed, Aug 02, 2023 at 11:23:46AM -0400, Alan Stern wrote:
> > On Wed, Aug 02, 2023 at 02:56:43PM +0000, Gagniuc, Alexandru wrote:
> > > On Wed, Jul 19, 2023 at 02:36:25PM -0400, Alan Stern wrote:
> > > > How do you know that the link will _remain_ in the correct state?
> > > 
> > > The objective is to get to xhci_set_link_state() with the USB_SS_PORT_LS_U3
> > > argument. This is achieved through usb_port_suspend() in drivers/usb/host/hub.c,
> > > and the function is implemented in drivers/usb/host/xhci-hub.c.
> > > 
> > > This is the only path in the kernel that I am aware of for setting the U3 link
> > > state. Given that it is part of the USB subsystem, I am fairly confident it will
> > > show consistent behavior across platforms.
> > 
> > That does not answer my question.  I agree that making this change will 
> > put the link into the U3 state.  But I don't have any reason to think 
> > that some other software won't later put the link into some other state.
> 
> I don't have a rigurous proof that the link will remain in the correct state.
> The only conjecture that I can make is that no other software besides the kernel
> will be running at this time. Thus, if the kernel manages to not break the link
> state, things should work as intended.
> 
> > > > That is, how do you know that the shutdown processing for the USB host 
> > > > controller won't disable the link entirely, thereby preventing WoL from 
> > > > working?
> > > 
> > > We are talking to the USB hub in order to set the link state. I don't see how
> > > specifics of the host controller would influence behavior.
> > 
> > Specifics of the host controller probably won't influence behavior.  
> > However, specifics of the _software_ can make a big difference.
> > 
> > >  I do expect a
> > > controller which advertises S4/S5 in /proc/acpi/wakeup to not do anything that
> > > would sabotage this capability. Disabling the link entirely would probalby
> > > violate that promise.
> > 
> > Not if the kernel _tells_ the controller to disable the link.
> > 
> > > Think of USB-C docks with a power button showing up as a HID class. The scenario
> > > herein would disable the power button. I would take that to be a bug in the host
> > > controller driver if the S4/S5 capability is advertised.
> > 
> > Indeed.  And I am asking how you can be sure the host controller driver 
> > (or some other part of the software stack) doesn't have this bug.
> 
> The only way that I have to show that is empirical. I observe that WoL from S5
> does not work on a device with an r8153 chip. I apply the change, and verify
> that WoL from S5 now works in this scenario. What are you thinking of in terms
> of being sure no current or future bug exists?

I was thinking that the host controller driver's shutdown method might 
turn off power to all of the ports.

For example, in the ehci-hcd driver, ehci_shutdown() calls 
ehci_silence_controller(), which calls ehci_turn_off_all_ports().  I 
don't know if xhci-hcd does anything similar.

Alan Stern

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alexandru Gagniuc]

On Thu, Aug 10, 2023 at 01:34:39PM -0400, Alan Stern wrote:
> On Thu, Aug 10, 2023 at 04:22:16PM +0000, Alexandru Gagniuc wrote:
> > On Wed, Aug 02, 2023 at 11:23:46AM -0400, Alan Stern wrote:
> > > 
> > > Indeed.  And I am asking how you can be sure the host controller driver 
> > > (or some other part of the software stack) doesn't have this bug.
> > 
> > The only way that I have to show that is empirical. I observe that WoL from S5
> > does not work on a device with an r8153 chip. I apply the change, and verify
> > that WoL from S5 now works in this scenario. What are you thinking of in terms
> > of being sure no current or future bug exists?
> 
> I was thinking that the host controller driver's shutdown method might 
> turn off power to all of the ports.
> 
> For example, in the ehci-hcd driver, ehci_shutdown() calls 
> ehci_silence_controller(), which calls ehci_turn_off_all_ports().  I 
> don't know if xhci-hcd does anything similar.

EHCI is a different beast. I don't think EHCI (USB2.0) has the U3 link state.

The equivalent for would be xhci_shutdown(). It makes a call to
usb_disable_xhci_ports() for XHCI_SPURIOUS_REBOOT quirk. As I have not
encountered it, I don't know how it will affect the link state of other ports.
The quirk appears to switch ports to EHCI mode, rather than turn off power.

Alex

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Alan Stern]

On Thu, Aug 10, 2023 at 10:51:09PM +0000, Alexandru Gagniuc wrote:
> On Thu, Aug 10, 2023 at 01:34:39PM -0400, Alan Stern wrote:
> > I was thinking that the host controller driver's shutdown method might 
> > turn off power to all of the ports.
> > 
> > For example, in the ehci-hcd driver, ehci_shutdown() calls 
> > ehci_silence_controller(), which calls ehci_turn_off_all_ports().  I 
> > don't know if xhci-hcd does anything similar.
> 
> EHCI is a different beast. I don't think EHCI (USB2.0) has the U3 link state.

USB-2 doesn't have link states, but it does have the notion of a 
downstream port being suspended, which is effectively the same as U3.

> The equivalent for would be xhci_shutdown(). It makes a call to
> usb_disable_xhci_ports() for XHCI_SPURIOUS_REBOOT quirk. As I have not
> encountered it, I don't know how it will affect the link state of other ports.
> The quirk appears to switch ports to EHCI mode, rather than turn off power.

All right.  The important point is that the patch works for your 
situation.  I was just trying to find out how much thought you had given 
to the possibilities other people might face, if their systems aren't 
quite the same as yours.

Alan Stern

Re: [PATCH v2] r8152: Suspend USB device before shutdown when WoL is enabled

[Oliver Neukum]

On 19.07.23 19:37, Alexandru Gagniuc wrote:
> For Wake-on-LAN to work from S5 (shutdown), the USB link must be put
> in U3 state. If it is not, and the host "disappears", the chip will
> no longer respond to WoL triggers.

First, a question, does this also apply to S4?

> To resolve this, add a notifier block and register it as a reboot
> notifier. When WoL is enabled, work through the usb_device struct to
> get to the suspend function. Calling this function puts the link in
> the correct state for WoL to function.

Second, do we really want this to be done for every driver with this issue
or do we want a flag for core USB code to suspend devices when the system
goes down? UAS at least does something similar.

Third, what happens if the device is already suspended when the notifier runs?

	Regards
		Oliver