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: [email protected]
Signed-off-by: Alexandru Gagniuc <[email protected]>
---
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
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: [email protected]
> Signed-off-by: Alexandru Gagniuc <[email protected]>
Please add a Fixes tag - I'm guessing it dates back to
Fixes: 21ff2e8976b1 ("r8152: support WOL")
?
--
pw-bot: cr
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
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: [email protected]
Signed-off-by: Alexandru Gagniuc <[email protected]>
---
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
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 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
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
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 Thu, Aug 10, 2023 at 04:22:16PM +0000, Alexandru Gagniuc wrote:
> From: Alan Stern <[email protected]>
>
> 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
From: Alan Stern <[email protected]>
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 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