diff mbox series

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

Message ID 20230719173756.380829-1-alexandru.gagniuc@hp.com
State New
Headers
Series [v2] r8152: Suspend USB device before shutdown when WoL is enabled |

Commit Message

Alexandru Gagniuc July 19, 2023, 5:37 p.m. UTC 
  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(+)

Comments

Alan Stern July 19, 2023, 6:36 p.m. UTC | #1 
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
Oliver Neukum July 20, 2023, 8:39 a.m. UTC | #2 
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
Alexandru Gagniuc Aug. 2, 2023, 2:56 p.m. UTC | #3 
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.
Alan Stern Aug. 2, 2023, 3:23 p.m. UTC | #4 
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
Alexandru Gagniuc Aug. 10, 2023, 4:22 p.m. UTC | #5 
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
Alan Stern Aug. 10, 2023, 5:34 p.m. UTC | #6 
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
Alexandru Gagniuc Aug. 10, 2023, 10:51 p.m. UTC | #7 
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
Alan Stern Aug. 11, 2023, 1:51 a.m. UTC | #8 
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
diff mbox series

Patch

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);