| Message ID | 20250131100630.342995-1-chenhuacai@loongson.cn (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | USB: core: Enable root_hub's remote wakeup for wakeup sources | expand |
On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > Now we only enable the remote wakeup function for the USB wakeup source > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > enough to enable the S3 wakeup function for USB keyboards, so we also > enable the root_hub's remote wakeup (and disable it on error). Frankly > this is unnecessary for XHCI, but enable it unconditionally make code > simple and seems harmless. > > Cc: stable@vger.kernel.org > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> What commit id does this fix? > --- > drivers/usb/core/hub.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index c3f839637cb5..efd6374ccd1d 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -3480,6 +3480,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > if (PMSG_IS_AUTO(msg)) > goto err_wakeup; > } > + usb_enable_remote_wakeup(udev->bus->root_hub); > } > > /* disable USB2 hardware LPM */ > @@ -3543,8 +3544,10 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > /* Try to enable USB2 hardware LPM again */ > usb_enable_usb2_hardware_lpm(udev); > > - if (udev->do_remote_wakeup) > + if (udev->do_remote_wakeup) { > (void) usb_disable_remote_wakeup(udev); > + (void) usb_disable_remote_wakeup(udev->bus->root_hub); This feels wrong, what about all of the devices inbetween this device and the root hub? thanks, greg k-h
Hi, Greg, On Fri, Jan 31, 2025 at 6:49 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > > Now we only enable the remote wakeup function for the USB wakeup source > > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > > enough to enable the S3 wakeup function for USB keyboards, so we also > > enable the root_hub's remote wakeup (and disable it on error). Frankly > > this is unnecessary for XHCI, but enable it unconditionally make code > > simple and seems harmless. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> > > What commit id does this fix? It seems this problem exist from the first place (at least >=4.19). > > > --- > > drivers/usb/core/hub.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > > index c3f839637cb5..efd6374ccd1d 100644 > > --- a/drivers/usb/core/hub.c > > +++ b/drivers/usb/core/hub.c > > @@ -3480,6 +3480,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > > if (PMSG_IS_AUTO(msg)) > > goto err_wakeup; > > } > > + usb_enable_remote_wakeup(udev->bus->root_hub); > > } > > > > /* disable USB2 hardware LPM */ > > @@ -3543,8 +3544,10 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) > > /* Try to enable USB2 hardware LPM again */ > > usb_enable_usb2_hardware_lpm(udev); > > > > - if (udev->do_remote_wakeup) > > + if (udev->do_remote_wakeup) { > > (void) usb_disable_remote_wakeup(udev); > > + (void) usb_disable_remote_wakeup(udev->bus->root_hub); > > This feels wrong, what about all of the devices inbetween this device > and the root hub? Yes, if there are other hubs between the root hub and keyboard, this patch still cannot fix the wakeup problem. I have tried to enable every hub in the link, but failed. Because I found many hubs lost power during suspend. So this patch can only fixes the most usual cases. Huacai > > thanks, > > greg k-h
On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > Now we only enable the remote wakeup function for the USB wakeup source > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > enough to enable the S3 wakeup function for USB keyboards, Why do you say this? It was enough on my system with an EHCI/UHCI controller when I wrote that code. What hardware do you have that isn't working? > so we also > enable the root_hub's remote wakeup (and disable it on error). Frankly > this is unnecessary for XHCI, but enable it unconditionally make code > simple and seems harmless. This does not make sense. For hubs (including root hubs), enabling remote wakeup means that the hub will generate a wakeup request when there is a connect, disconnect, or over-current change. That's not what you want to do, is it? And it has nothing to do with how the hub handles wakeup requests received from downstream devices. You need to explain what's going on here in much more detail. What exactly is going wrong, and why? What is the hardware actually doing, as compared to what we expect it to do? Alan Stern
Hi, Alan, On Fri, Jan 31, 2025 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > > Now we only enable the remote wakeup function for the USB wakeup source > > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > > enough to enable the S3 wakeup function for USB keyboards, > > Why do you say this? It was enough on my system with an EHCI/UHCI > controller when I wrote that code. What hardware do you have that isn't > working? > > > so we also > > enable the root_hub's remote wakeup (and disable it on error). Frankly > > this is unnecessary for XHCI, but enable it unconditionally make code > > simple and seems harmless. > > This does not make sense. For hubs (including root hubs), enabling > remote wakeup means that the hub will generate a wakeup request when > there is a connect, disconnect, or over-current change. That's not what > you want to do, is it? And it has nothing to do with how the hub > handles wakeup requests received from downstream devices. > > You need to explain what's going on here in much more detail. What > exactly is going wrong, and why? What is the hardware actually doing, > as compared to what we expect it to do? OK, let me tell a long story: At first, someone reported that on Loongson platform we cannot wake up S3 with a USB keyboard, but no problem on x86. At that time we thought this was a platform-specific problem. After that we have done many experiments, then we found that if the keyboard is connected to a XHCI controller, it can wake up, but cannot wake up if it is connected to a non-XHCI controller, no matter on x86 or on Loongson. We are not familiar with USB protocol, this is just observed from experiments. You are probably right that enabling remote wakeup on a hub means it can generate wakeup requests rather than forward downstream devices' requests. But from experiments we found that if we enable the "wakeup" knob of the root_hub via sysfs, then a keyboard becomes able to wake up S3 (for non-XHCI controllers). So we guess that the enablement also enables forwarding. So maybe this is an implementation-specific problem (but most implementations have problems)? This patch itself just emulates the enablement of root_hub's remote wakeup automatically (then we needn't operate on sysfs). Huacai > > Alan Stern
On Sat, Feb 01, 2025 at 02:42:43PM +0800, Huacai Chen wrote: > Hi, Alan, > > On Fri, Jan 31, 2025 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > > > Now we only enable the remote wakeup function for the USB wakeup source > > > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > > > enough to enable the S3 wakeup function for USB keyboards, > > > > Why do you say this? It was enough on my system with an EHCI/UHCI > > controller when I wrote that code. What hardware do you have that isn't > > working? > > > > > so we also > > > enable the root_hub's remote wakeup (and disable it on error). Frankly > > > this is unnecessary for XHCI, but enable it unconditionally make code > > > simple and seems harmless. > > > > This does not make sense. For hubs (including root hubs), enabling > > remote wakeup means that the hub will generate a wakeup request when > > there is a connect, disconnect, or over-current change. That's not what > > you want to do, is it? And it has nothing to do with how the hub > > handles wakeup requests received from downstream devices. > > > > You need to explain what's going on here in much more detail. What > > exactly is going wrong, and why? What is the hardware actually doing, > > as compared to what we expect it to do? > OK, let me tell a long story: > > At first, someone reported that on Loongson platform we cannot wake up > S3 with a USB keyboard, but no problem on x86. At that time we thought > this was a platform-specific problem. > > After that we have done many experiments, then we found that if the > keyboard is connected to a XHCI controller, it can wake up, but cannot > wake up if it is connected to a non-XHCI controller, no matter on x86 > or on Loongson. We are not familiar with USB protocol, this is just > observed from experiments. > > You are probably right that enabling remote wakeup on a hub means it > can generate wakeup requests rather than forward downstream devices' > requests. But from experiments we found that if we enable the "wakeup" > knob of the root_hub via sysfs, then a keyboard becomes able to wake > up S3 (for non-XHCI controllers). So we guess that the enablement also > enables forwarding. So maybe this is an implementation-specific > problem (but most implementations have problems)? > > This patch itself just emulates the enablement of root_hub's remote > wakeup automatically (then we needn't operate on sysfs). I'll run some experiments on my system. Maybe you're right about the problem, but your proposed solution looks wrong. Alan Stern
On Sun, Feb 2, 2025 at 12:55 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Sat, Feb 01, 2025 at 02:42:43PM +0800, Huacai Chen wrote: > > Hi, Alan, > > > > On Fri, Jan 31, 2025 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > > > > Now we only enable the remote wakeup function for the USB wakeup source > > > > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > > > > enough to enable the S3 wakeup function for USB keyboards, > > > > > > Why do you say this? It was enough on my system with an EHCI/UHCI > > > controller when I wrote that code. What hardware do you have that isn't > > > working? > > > > > > > so we also > > > > enable the root_hub's remote wakeup (and disable it on error). Frankly > > > > this is unnecessary for XHCI, but enable it unconditionally make code > > > > simple and seems harmless. > > > > > > This does not make sense. For hubs (including root hubs), enabling > > > remote wakeup means that the hub will generate a wakeup request when > > > there is a connect, disconnect, or over-current change. That's not what > > > you want to do, is it? And it has nothing to do with how the hub > > > handles wakeup requests received from downstream devices. > > > > > > You need to explain what's going on here in much more detail. What > > > exactly is going wrong, and why? What is the hardware actually doing, > > > as compared to what we expect it to do? > > OK, let me tell a long story: > > > > At first, someone reported that on Loongson platform we cannot wake up > > S3 with a USB keyboard, but no problem on x86. At that time we thought > > this was a platform-specific problem. > > > > After that we have done many experiments, then we found that if the > > keyboard is connected to a XHCI controller, it can wake up, but cannot > > wake up if it is connected to a non-XHCI controller, no matter on x86 > > or on Loongson. We are not familiar with USB protocol, this is just > > observed from experiments. > > > > You are probably right that enabling remote wakeup on a hub means it > > can generate wakeup requests rather than forward downstream devices' > > requests. But from experiments we found that if we enable the "wakeup" > > knob of the root_hub via sysfs, then a keyboard becomes able to wake > > up S3 (for non-XHCI controllers). So we guess that the enablement also > > enables forwarding. So maybe this is an implementation-specific > > problem (but most implementations have problems)? > > > > This patch itself just emulates the enablement of root_hub's remote > > wakeup automatically (then we needn't operate on sysfs). > > I'll run some experiments on my system. Maybe you're right about the > problem, but your proposed solution looks wrong. OK, I'm glad to see a better solution. :) Huacai > > Alan Stern
On Sat, Feb 01, 2025 at 11:55:03AM -0500, Alan Stern wrote: > On Sat, Feb 01, 2025 at 02:42:43PM +0800, Huacai Chen wrote: > > Hi, Alan, > > > > On Fri, Jan 31, 2025 at 11:17 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Fri, Jan 31, 2025 at 06:06:30PM +0800, Huacai Chen wrote: > > > > Now we only enable the remote wakeup function for the USB wakeup source > > > > itself at usb_port_suspend(). But on pre-XHCI controllers this is not > > > > enough to enable the S3 wakeup function for USB keyboards, > > > > > > Why do you say this? It was enough on my system with an EHCI/UHCI > > > controller when I wrote that code. What hardware do you have that isn't > > > working? > > > > > > > so we also > > > > enable the root_hub's remote wakeup (and disable it on error). Frankly > > > > this is unnecessary for XHCI, but enable it unconditionally make code > > > > simple and seems harmless. > > > > > > This does not make sense. For hubs (including root hubs), enabling > > > remote wakeup means that the hub will generate a wakeup request when > > > there is a connect, disconnect, or over-current change. That's not what > > > you want to do, is it? And it has nothing to do with how the hub > > > handles wakeup requests received from downstream devices. > > > > > > You need to explain what's going on here in much more detail. What > > > exactly is going wrong, and why? What is the hardware actually doing, > > > as compared to what we expect it to do? > > OK, let me tell a long story: > > > > At first, someone reported that on Loongson platform we cannot wake up > > S3 with a USB keyboard, but no problem on x86. At that time we thought > > this was a platform-specific problem. > > > > After that we have done many experiments, then we found that if the > > keyboard is connected to a XHCI controller, it can wake up, but cannot > > wake up if it is connected to a non-XHCI controller, no matter on x86 > > or on Loongson. We are not familiar with USB protocol, this is just > > observed from experiments. > > > > You are probably right that enabling remote wakeup on a hub means it > > can generate wakeup requests rather than forward downstream devices' > > requests. But from experiments we found that if we enable the "wakeup" > > knob of the root_hub via sysfs, then a keyboard becomes able to wake > > up S3 (for non-XHCI controllers). So we guess that the enablement also > > enables forwarding. So maybe this is an implementation-specific > > problem (but most implementations have problems)? > > > > This patch itself just emulates the enablement of root_hub's remote > > wakeup automatically (then we needn't operate on sysfs). > > I'll run some experiments on my system. Maybe you're right about the > problem, but your proposed solution looks wrong. I just tried running the experiment on my system. I enabled wakeup for the mouse device, made sure it was disabled for the intermediate hub and the root hub, and made sure it was enabled for the host controller. (Those last three are the default settings.) Then I put the system in S3 suspend by writing "mem" to /sys/power/state, and when the system was asleep I pressed one of the mouse buttons -- and the system woke up. This was done under a 6.12.10 kernel, with an EHCI host controller, not xHCI. So it seems like something is wrong with your system in particular, not the core USB code in general. What type of host controller is your mouse attached to? Have you tested whether the mouse is able to wake up from runtime suspend, as opposed to S3 suspend? Alan Stern
Hi Alan, Huacai, <snip> > I just tried running the experiment on my system. I enabled wakeup for > the mouse device, made sure it was disabled for the intermediate hub and > the root hub, and made sure it was enabled for the host controller. > (Those last three are the default settings.) Then I put the system in > S3 suspend by writing "mem" to /sys/power/state, and when the system was > asleep I pressed one of the mouse buttons -- and the system woke up. > This was done under a 6.12.10 kernel, with an EHCI host controller, not > xHCI. > > So it seems like something is wrong with your system in particular, not > the core USB code in general. What type of host controller is your > mouse attached to? Have you tested whether the mouse is able to wake up > from runtime suspend, as opposed to S3 suspend? > Just to chime in with my own test results. I was looking at this with Huacai a few days back and we suspected that this had something to do with particular systems, as you have found; we also suspected that if a keyboard was connected to a non-xHCI controller, it would fail to wake up the system. I conducted a simple experiment on my Lenovo ThinkPad X200s, which does not come with any USB 3.0 port. Here are my findings: 1. With upstream code, the system would not wake up with neither the internal nor the external keyboards. One exception being the Fn key on the internal keyboard, which would wake up the system (but I suspect that this is EC behaviour). This behaviour is consistent across any USB port on the laptop and, regardless if the external keyboard was connected to the laptop itself or via a hub. 2. With Huacai's code, I was able to wake up the laptop with an external keyboard in all the scenarios listed in (1). The internal keyboard still failed to wake up the system unless I strike the Fn key. I should note, however, that the internal keyboard is not connected via USB so it's probably irrelevant information anyway. As for mice, it seems that the kernel disables wake-up via USB mice and enables wake-up via USB keyboards. This is also consistent with your findings. Best Regards, Mingcong Bai > Alan Stern >
On Tue, Feb 04, 2025 at 12:01:37AM +0800, Mingcong Bai wrote: > Hi Alan, Huacai, > > <snip> > > > I just tried running the experiment on my system. I enabled wakeup for > > the mouse device, made sure it was disabled for the intermediate hub and > > the root hub, and made sure it was enabled for the host controller. > > (Those last three are the default settings.) Then I put the system in > > S3 suspend by writing "mem" to /sys/power/state, and when the system was > > asleep I pressed one of the mouse buttons -- and the system woke up. > > This was done under a 6.12.10 kernel, with an EHCI host controller, not > > xHCI. > > > > So it seems like something is wrong with your system in particular, not > > the core USB code in general. What type of host controller is your > > mouse attached to? Have you tested whether the mouse is able to wake up > > from runtime suspend, as opposed to S3 suspend? > > > > Just to chime in with my own test results. I was looking at this with Huacai > a few days back and we suspected that this had something to do with > particular systems, as you have found; we also suspected that if a keyboard > was connected to a non-xHCI controller, it would fail to wake up the system. > > I conducted a simple experiment on my Lenovo ThinkPad X200s, which does not > come with any USB 3.0 port. Here are my findings: What sort of USB controller does the X200s have? Is the controller enabled for wakeup? What happens with runtime suspend rather than S3 suspend? > 1. With upstream code, the system would not wake up with neither the > internal nor the external keyboards. One exception being the Fn key on the > internal keyboard, which would wake up the system (but I suspect that this > is EC behaviour). This behaviour is consistent across any USB port on the > laptop and, regardless if the external keyboard was connected to the laptop > itself or via a hub. > > 2. With Huacai's code, I was able to wake up the laptop with an external > keyboard in all the scenarios listed in (1). The internal keyboard still > failed to wake up the system unless I strike the Fn key. > > I should note, however, that the internal keyboard is not connected via USB > so it's probably irrelevant information anyway. > > As for mice, it seems that the kernel disables wake-up via USB mice and > enables wake-up via USB keyboards. This is also consistent with your > findings. Yes, and of course you can manually change the default wakeup settings whenever you want. Alan Stern
Hi Alan, 在 2025/2/4 00:15, Alan Stern 写道: > On Tue, Feb 04, 2025 at 12:01:37AM +0800, Mingcong Bai wrote: >> Hi Alan, Huacai, >> >> <snip> >> >>> I just tried running the experiment on my system. I enabled wakeup for >>> the mouse device, made sure it was disabled for the intermediate hub and >>> the root hub, and made sure it was enabled for the host controller. >>> (Those last three are the default settings.) Then I put the system in >>> S3 suspend by writing "mem" to /sys/power/state, and when the system was >>> asleep I pressed one of the mouse buttons -- and the system woke up. >>> This was done under a 6.12.10 kernel, with an EHCI host controller, not >>> xHCI. >>> >>> So it seems like something is wrong with your system in particular, not >>> the core USB code in general. What type of host controller is your >>> mouse attached to? Have you tested whether the mouse is able to wake up >>> from runtime suspend, as opposed to S3 suspend? >>> >> >> Just to chime in with my own test results. I was looking at this with Huacai >> a few days back and we suspected that this had something to do with >> particular systems, as you have found; we also suspected that if a keyboard >> was connected to a non-xHCI controller, it would fail to wake up the system. >> >> I conducted a simple experiment on my Lenovo ThinkPad X200s, which does not >> come with any USB 3.0 port. Here are my findings: > > What sort of USB controller does the X200s have? Is the controller > enabled for wakeup? > > What happens with runtime suspend rather than S3 suspend? It has the Intel Corporation 82801I (ICH9 Family) USB UCHI/USB2 EHCI controller with PCI IDs 17aa:20f0/17aa:20f1. The hub is a Genesys Logic, Inc. Hub with an ID of 05e3:0610 - this is an xHCI hub. Sorry but the record here is going to get a bit messy... But let's start with a kernel with Huacai's patch. === Kernel + Huacai's Patch === 1. If I plug in the external keyboard via the hub, /sys/bus/usb/devices/usb1, power/state is set to enabled. For the hub, corresponding to usb1/1-1, power/wakeup is set to disabled. 2. If I plug the keyboard directly into the chassis, usb1/power/wakeup is set to disabled, but usb1/1-1/power/wakeup is set to enabled. The system wakes up via external keyboard plugged directly into the chassis **or** the hub either way, regardless if I used S3 or runtime suspend (which I assume to be echo freeze > /sys/power/state). === Kernel w/o Huacai's Patch === The controller where I plugged in the USB hub, /sys/bus/usb/devices/usb1 and the hub, corresponding to usb1/1-1, their power/wakeup entries are both set to disabled. Same for when I have the patch applied. However, if I plug the external keyboard into the chassis, it would fail to wakeup regardless of S3/runtime suspend (freeze). If I plug the external keyboard via that USB Hub though, it would wake up the machine. The findings are consistent between S3/freeze. > >> 1. With upstream code, the system would not wake up with neither the >> internal nor the external keyboards. One exception being the Fn key on the >> internal keyboard, which would wake up the system (but I suspect that this >> is EC behaviour). This behaviour is consistent across any USB port on the >> laptop and, regardless if the external keyboard was connected to the laptop >> itself or via a hub. >> >> 2. With Huacai's code, I was able to wake up the laptop with an external >> keyboard in all the scenarios listed in (1). The internal keyboard still >> failed to wake up the system unless I strike the Fn key. >> >> I should note, however, that the internal keyboard is not connected via USB >> so it's probably irrelevant information anyway. >> >> As for mice, it seems that the kernel disables wake-up via USB mice and >> enables wake-up via USB keyboards. This is also consistent with your >> findings. > > Yes, and of course you can manually change the default wakeup settings > whenever you want. > > Alan Stern Best Regards, Mingcong Bai
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index c3f839637cb5..efd6374ccd1d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -3480,6 +3480,7 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) if (PMSG_IS_AUTO(msg)) goto err_wakeup; } + usb_enable_remote_wakeup(udev->bus->root_hub); } /* disable USB2 hardware LPM */ @@ -3543,8 +3544,10 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg) /* Try to enable USB2 hardware LPM again */ usb_enable_usb2_hardware_lpm(udev); - if (udev->do_remote_wakeup) + if (udev->do_remote_wakeup) { (void) usb_disable_remote_wakeup(udev); + (void) usb_disable_remote_wakeup(udev->bus->root_hub); + } err_wakeup: /* System sleep transitions should never fail */
Now we only enable the remote wakeup function for the USB wakeup source itself at usb_port_suspend(). But on pre-XHCI controllers this is not enough to enable the S3 wakeup function for USB keyboards, so we also enable the root_hub's remote wakeup (and disable it on error). Frankly this is unnecessary for XHCI, but enable it unconditionally make code simple and seems harmless. Cc: stable@vger.kernel.org Signed-off-by: Huacai Chen <chenhuacai@loongson.cn> --- drivers/usb/core/hub.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)