| Message ID | 20230706182858.761311-1-alexandru.gagniuc@hp.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | r8152: Suspend USB device before shutdown when WoL is enabled | expand |
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") ?
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
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
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.
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
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
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
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
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 --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);
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(+)