| Message ID | 20230817093305.212821-1-kai.heng.feng@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xhci: Disable connect, disconnect and over-current wakeup on system suspend | expand |
On 17.08.23 11:33, Kai-Heng Feng wrote: Hi, > So for system-wide suspend, also disable connect, disconnect and > over-current wakeup to prevent spurious wakeup. isn't this breaking the ability to effectively use your root hub as a source of system wakeups? That is, even if you want the system to wake up if somebody attaches a new device, it no longer works? Regards Oliver
On 17.8.2023 14.18, Oliver Neukum wrote: > On 17.08.23 11:33, Kai-Heng Feng wrote: > > Hi, > >> So for system-wide suspend, also disable connect, disconnect and >> over-current wakeup to prevent spurious wakeup. > > isn't this breaking the ability to effectively use your root hub > as a source of system wakeups? That is, even if you want the > system to wake up if somebody attaches a new device, it no longer works? > I got the same concern about this. -Mathias
On Thu, Aug 17, 2023 at 8:52 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 17.8.2023 14.18, Oliver Neukum wrote: > > On 17.08.23 11:33, Kai-Heng Feng wrote: > > > > Hi, > > > >> So for system-wide suspend, also disable connect, disconnect and > >> over-current wakeup to prevent spurious wakeup. > > > > isn't this breaking the ability to effectively use your root hub > > as a source of system wakeups? That is, even if you want the > > system to wake up if somebody attaches a new device, it no longer works? > > > > I got the same concern about this. Per my test, it doesn't work with or without this change. This applies to disconnection too, disconnecting USB devices doesn't wake the system up. Furthermore, if the newly attached device is a USB keyboard, pressing it doesn't wake the system up either. Probably because remote wakeup isn't configured when the system is suspended. Kai-Heng > > -Mathias
On Thu, Aug 17, 2023 at 09:13:55PM +0800, Kai-Heng Feng wrote: > On Thu, Aug 17, 2023 at 8:52 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: > > > > On 17.8.2023 14.18, Oliver Neukum wrote: > > > On 17.08.23 11:33, Kai-Heng Feng wrote: > > > > > > Hi, > > > > > >> So for system-wide suspend, also disable connect, disconnect and > > >> over-current wakeup to prevent spurious wakeup. > > > > > > isn't this breaking the ability to effectively use your root hub > > > as a source of system wakeups? That is, even if you want the > > > system to wake up if somebody attaches a new device, it no longer works? > > > > > > > I got the same concern about this. > > Per my test, it doesn't work with or without this change. This applies > to disconnection too, disconnecting USB devices doesn't wake the > system up. > Furthermore, if the newly attached device is a USB keyboard, pressing > it doesn't wake the system up either. Probably because remote wakeup > isn't configured when the system is suspended. If remote wakeup isn't enabled then the do_wakeup variable will be 0, so your patch wouldn't make any difference. The question is what happens when remote wakeup _is_ enabled. Did you check the settings in the controller's and root hub's power/wakeup sysfs files? Alan Stern
On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > system: > [ 445.814574] hub 2-0:1.0: hub_suspend > [ 445.814652] usb usb2: bus suspend, wakeup 0 > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 What is the meaning of the 0x202a0 bits? What caused this wakeup? > [ 445.824639] xhci_hcd 0000:00:14.0: resume root hub > [ 445.824651] xhci_hcd 0000:00:14.0: handle_port_status: starting usb1 port polling. > [ 445.844039] xhci_hcd 0000:00:14.0: PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x20 returns -16 > [ 445.844058] xhci_hcd 0000:00:14.0: PM: dpm_run_callback(): pci_pm_suspend+0x0/0x1c0 returns -16 > [ 445.844072] xhci_hcd 0000:00:14.0: PM: failed to suspend async: error -16 > [ 446.276101] PM: Some devices failed to suspend, or early wake event detected > > The system is designed to let display and touchpanel share the same > power source, so when the display becomes off, the USB touchpanel also > lost its power and disconnect itself from USB bus. That doesn't play > well when most Desktop Environment lock and turnoff the display right > before entering system suspend. I don't see why that should cause any trouble. The display gets locked and turned off, the touchpanel disconnects from the USB bus, and then the system goes into suspend. Why would there be a wakeup signal at this point? > So for system-wide suspend, also disable connect, disconnect and > over-current wakeup to prevent spurious wakeup. Whether to disable these things is part of the userspace policy. The kernel should not make the decision; the user does by enabling or disabling wakeups. Alan Stern > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > --- > drivers/usb/host/xhci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > index fae994f679d4..dc499100efa6 100644 > --- a/drivers/usb/host/xhci.c > +++ b/drivers/usb/host/xhci.c > @@ -16,6 +16,7 @@ > #include <linux/module.h> > #include <linux/moduleparam.h> > #include <linux/slab.h> > +#include <linux/suspend.h> > #include <linux/dmi.h> > #include <linux/dma-mapping.h> > > @@ -789,7 +790,7 @@ static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci, > t2 = t1; > > /* clear wake bits if do_wake is not set */ > - if (!do_wakeup) > + if (!do_wakeup || pm_suspend_target_state != PM_SUSPEND_ON) > t2 &= ~PORT_WAKE_BITS; > > /* Don't touch csc bit if connected or connect change is set */ > -- > 2.34.1 >
On Thu, Aug 17, 2023 at 10:07:37AM -0400, Alan Stern wrote: > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > system: > > [ 445.814574] hub 2-0:1.0: hub_suspend > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > What is the meaning of the 0x202a0 bits? What caused this wakeup? And more to the point, given that the previous line says "wakeup 0", why should any port change event cause a wakeup? Alan Stern
On Thu, Aug 17, 2023 at 10:03 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Aug 17, 2023 at 09:13:55PM +0800, Kai-Heng Feng wrote: > > On Thu, Aug 17, 2023 at 8:52 PM Mathias Nyman > > <mathias.nyman@linux.intel.com> wrote: > > > > > > On 17.8.2023 14.18, Oliver Neukum wrote: > > > > On 17.08.23 11:33, Kai-Heng Feng wrote: > > > > > > > > Hi, > > > > > > > >> So for system-wide suspend, also disable connect, disconnect and > > > >> over-current wakeup to prevent spurious wakeup. > > > > > > > > isn't this breaking the ability to effectively use your root hub > > > > as a source of system wakeups? That is, even if you want the > > > > system to wake up if somebody attaches a new device, it no longer works? > > > > > > > > > > I got the same concern about this. > > > > Per my test, it doesn't work with or without this change. This applies > > to disconnection too, disconnecting USB devices doesn't wake the > > system up. > > Furthermore, if the newly attached device is a USB keyboard, pressing > > it doesn't wake the system up either. Probably because remote wakeup > > isn't configured when the system is suspended. > > If remote wakeup isn't enabled then the do_wakeup variable will be 0, > so your patch wouldn't make any difference. The question is what > happens when remote wakeup _is_ enabled. Nothing happens either per my testing. For USB keyboard, the remote wakeup is enabled, unplugging it when suspend is suspended doesn't wake the system up, despite of PORT_WKDISC_E being set. Plugging it back doesn't wake the system up either, despite of PORT_WKCONN_E. > > Did you check the settings in the controller's and root hub's > power/wakeup sysfs files? Yes. It's all correct as keyboard press can wake the system up. Kai-Heng > > Alan Stern
On Thu, Aug 17, 2023 at 10:07 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > system: > > [ 445.814574] hub 2-0:1.0: hub_suspend > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > What is the meaning of the 0x202a0 bits? What caused this wakeup? The USB touchpanel is disconnecting from the USB bus. CCS is 0. > > > [ 445.824639] xhci_hcd 0000:00:14.0: resume root hub > > [ 445.824651] xhci_hcd 0000:00:14.0: handle_port_status: starting usb1 port polling. > > [ 445.844039] xhci_hcd 0000:00:14.0: PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x20 returns -16 > > [ 445.844058] xhci_hcd 0000:00:14.0: PM: dpm_run_callback(): pci_pm_suspend+0x0/0x1c0 returns -16 > > [ 445.844072] xhci_hcd 0000:00:14.0: PM: failed to suspend async: error -16 > > [ 446.276101] PM: Some devices failed to suspend, or early wake event detected > > > > The system is designed to let display and touchpanel share the same > > power source, so when the display becomes off, the USB touchpanel also > > lost its power and disconnect itself from USB bus. That doesn't play > > well when most Desktop Environment lock and turnoff the display right > > before entering system suspend. > > I don't see why that should cause any trouble. The display gets locked > and turned off, the touchpanel disconnects from the USB bus, and then > the system goes into suspend. Why would there be a wakeup signal at > this point? The disconnecting can happens during the system suspend process, so the suspend process is aborted. > > > So for system-wide suspend, also disable connect, disconnect and > > over-current wakeup to prevent spurious wakeup. > > Whether to disable these things is part of the userspace policy. The > kernel should not make the decision; the user does by enabling or > disabling wakeups. The power/wakeup is already disabled. The disconnecting event is from roothub and if roothub wakeup is disabled, other USB devices lose the ability to wake the system up from system suspend. Kai-Heng > > Alan Stern > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > --- > > drivers/usb/host/xhci.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c > > index fae994f679d4..dc499100efa6 100644 > > --- a/drivers/usb/host/xhci.c > > +++ b/drivers/usb/host/xhci.c > > @@ -16,6 +16,7 @@ > > #include <linux/module.h> > > #include <linux/moduleparam.h> > > #include <linux/slab.h> > > +#include <linux/suspend.h> > > #include <linux/dmi.h> > > #include <linux/dma-mapping.h> > > > > @@ -789,7 +790,7 @@ static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci, > > t2 = t1; > > > > /* clear wake bits if do_wake is not set */ > > - if (!do_wakeup) > > + if (!do_wakeup || pm_suspend_target_state != PM_SUSPEND_ON) > > t2 &= ~PORT_WAKE_BITS; > > > > /* Don't touch csc bit if connected or connect change is set */ > > -- > > 2.34.1 > >
On Thu, Aug 17, 2023 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, Aug 17, 2023 at 10:07:37AM -0400, Alan Stern wrote: > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > > system: > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > > > What is the meaning of the 0x202a0 bits? What caused this wakeup? > > And more to the point, given that the previous line says "wakeup 0", why > should any port change event cause a wakeup? I think the controller and roothub have to deal with the interrupt about disconnecting regardless of the remote wakeup setting. Kai-Heng > > Alan Stern
On Fri, Aug 18, 2023 at 08:01:54AM +0800, Kai-Heng Feng wrote: > On Thu, Aug 17, 2023 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, Aug 17, 2023 at 10:07:37AM -0400, Alan Stern wrote: > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > > > system: > > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > > > > > What is the meaning of the 0x202a0 bits? What caused this wakeup? > > > > And more to the point, given that the previous line says "wakeup 0", why > > should any port change event cause a wakeup? > > I think the controller and roothub have to deal with the interrupt > about disconnecting regardless of the remote wakeup setting. This seems to contradict what you wrote in an earlier email: > > If remote wakeup isn't enabled then the do_wakeup variable will be 0, > > so your patch wouldn't make any difference. The question is what > > happens when remote wakeup _is_ enabled. > > Nothing happens either per my testing. > > For USB keyboard, the remote wakeup is enabled, unplugging it when > suspend is suspended doesn't wake the system up, despite of PORT_WKDISC_E being set. > Plugging it back doesn't wake the system up either, despite of PORT_WKCONN_E. You appear to be saying that when wakeup is disabled, unplugging a device will wake up the system -- but when wakeup is enabled, unplugging a device will not wake up the system! Is that really what you meant? It doesn't make sense. Probably means there's a bug in the HCD. The point I'm trying to get at is that if wakeups are disabled for both the host controller and the root hub then _nothing_ should generate an interrupt or wakeup request. Not pressing a key, not unplugging a device... nothing. But if wakeup _is_ enabled for both the controller and the root hub, then any of those actions should generate an interrupt and wake up the system. If wakeup is enabled for the host controller but not for the root hub, then unplugging a device from the root hub should not generate a wakeup request or an interrupt. But things like pressing a key on a wakeup-enabled keyboard should. (In other words, the root hub shouldn't generate any wakeup requests on its own but it should relay wakeup requests that it receives from downstream devices.) However, it's understandable if the system doesn't behave properly in this case since it's kind of an odd situation. Alan Stern
On Fri, Aug 18, 2023 at 08:00:39AM +0800, Kai-Heng Feng wrote: > On Thu, Aug 17, 2023 at 10:07 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > The system is designed to let display and touchpanel share the same > > > power source, so when the display becomes off, the USB touchpanel also > > > lost its power and disconnect itself from USB bus. That doesn't play > > > well when most Desktop Environment lock and turnoff the display right > > > before entering system suspend. > > > > I don't see why that should cause any trouble. The display gets locked > > and turned off, the touchpanel disconnects from the USB bus, and then > > the system goes into suspend. Why would there be a wakeup signal at > > this point? > > The disconnecting can happens during the system suspend process, so > the suspend process is aborted. Maybe these systems need to add a little delay when the display is turned off, in order to give the touchpanel time to disconnect before the system suspend begins. > > > So for system-wide suspend, also disable connect, disconnect and > > > over-current wakeup to prevent spurious wakeup. > > > > Whether to disable these things is part of the userspace policy. The > > kernel should not make the decision; the user does by enabling or > > disabling wakeups. > > The power/wakeup is already disabled. In that case the root hub should not generate a wakeup request in response to the touchpanel disconnecting. > The disconnecting event is from roothub and if roothub wakeup is > disabled, other USB devices lose the ability to wake the system up > from system suspend. That shouldn't happen either. Disabling wakeup on the root hub should not prevent the root hub from relaying wakeup requests it receives from downstream devices. It should merely prevent the root hub from generating its own wakeup requests for connects, disconnects, and over-current events. It sounds like the xhci root-hub code isn't doing the right thing, at least, not on your systems. Alan Stern
On Fri, Aug 18, 2023 at 11:08 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Aug 18, 2023 at 08:01:54AM +0800, Kai-Heng Feng wrote: > > On Thu, Aug 17, 2023 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Thu, Aug 17, 2023 at 10:07:37AM -0400, Alan Stern wrote: > > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > > > > system: > > > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > > > > > > > What is the meaning of the 0x202a0 bits? What caused this wakeup? > > > > > > And more to the point, given that the previous line says "wakeup 0", why > > > should any port change event cause a wakeup? > > > > I think the controller and roothub have to deal with the interrupt > > about disconnecting regardless of the remote wakeup setting. > > This seems to contradict what you wrote in an earlier email: Nothing is contradicting, what I mentioned is the wakeup of USB touchpanel, not the controller or root hub. > > > > If remote wakeup isn't enabled then the do_wakeup variable will be 0, > > > so your patch wouldn't make any difference. The question is what > > > happens when remote wakeup _is_ enabled. > > > > Nothing happens either per my testing. > > > > For USB keyboard, the remote wakeup is enabled, unplugging it when > > suspend is suspended doesn't wake the system up, despite of PORT_WKDISC_E being set. > > Plugging it back doesn't wake the system up either, despite of PORT_WKCONN_E. > > You appear to be saying that when wakeup is disabled, unplugging a > device will wake up the system -- but when wakeup is enabled, unplugging > a device will not wake up the system! No, what I was saying is that when PORT_WKCONN_E and PORT_WKDISC_E are set, plugging/unplugging USB doesn't wake up the system from suspended state. What it really does for this case is to hinder the suspending process. > > Is that really what you meant? It doesn't make sense. Probably means > there's a bug in the HCD. > > The point I'm trying to get at is that if wakeups are disabled for both > the host controller and the root hub then _nothing_ should generate an > interrupt or wakeup request. Not pressing a key, not unplugging a > device... nothing. But if wakeup _is_ enabled for both the controller > and the root hub, then any of those actions should generate an interrupt > and wake up the system. Like above, the wakeup I mentioned is on the USB touchpanel itself, not on the controller and roothub. There's no IRQ generated when controller's wakeup is disabled. > > If wakeup is enabled for the host controller but not for the root hub, > then unplugging a device from the root hub should not generate a wakeup > request or an interrupt. But things like pressing a key on a > wakeup-enabled keyboard should. (In other words, the root hub shouldn't > generate any wakeup requests on its own but it should relay wakeup > requests that it receives from downstream devices.) However, it's > understandable if the system doesn't behave properly in this case since > it's kind of an odd situation. Do you mean when the system is suspended, or system is still suspending? The issue only happens when the system is suspending. Kai-Heng > > Alan Stern
On Fri, Aug 18, 2023 at 11:19 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Aug 18, 2023 at 08:00:39AM +0800, Kai-Heng Feng wrote: > > On Thu, Aug 17, 2023 at 10:07 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > > The system is designed to let display and touchpanel share the same > > > > power source, so when the display becomes off, the USB touchpanel also > > > > lost its power and disconnect itself from USB bus. That doesn't play > > > > well when most Desktop Environment lock and turnoff the display right > > > > before entering system suspend. > > > > > > I don't see why that should cause any trouble. The display gets locked > > > and turned off, the touchpanel disconnects from the USB bus, and then > > > the system goes into suspend. Why would there be a wakeup signal at > > > this point? > > > > The disconnecting can happens during the system suspend process, so > > the suspend process is aborted. > > Maybe these systems need to add a little delay when the display is > turned off, in order to give the touchpanel time to disconnect before > the system suspend begins. Unfortunately the hardware can't be changed. > > > > > So for system-wide suspend, also disable connect, disconnect and > > > > over-current wakeup to prevent spurious wakeup. > > > > > > Whether to disable these things is part of the userspace policy. The > > > kernel should not make the decision; the user does by enabling or > > > disabling wakeups. > > > > The power/wakeup is already disabled. > > In that case the root hub should not generate a wakeup request in > response to the touchpanel disconnecting. Here's the wakeup setting when the issue happens: controller - wakeup enabled root hub: wakeup disabled touchpanel: wakeup disabled > > > The disconnecting event is from roothub and if roothub wakeup is > > disabled, other USB devices lose the ability to wake the system up > > from system suspend. > > That shouldn't happen either. Disabling wakeup on the root hub should > not prevent the root hub from relaying wakeup requests it receives from > downstream devices. It should merely prevent the root hub from > generating its own wakeup requests for connects, disconnects, and > over-current events. Sorry, it was meant to be the xHCI controller. The didn't make the difference clear. > > It sounds like the xhci root-hub code isn't doing the right thing, at > least, not on your systems. I still don't fully understand why removing PORT_WAKE_BITS is not right in this case. Kai-Heng > > Alan Stern
On Fri, Aug 18, 2023 at 10:12:25PM +0800, Kai-Heng Feng wrote: > On Fri, Aug 18, 2023 at 11:08 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Aug 18, 2023 at 08:01:54AM +0800, Kai-Heng Feng wrote: > > > On Thu, Aug 17, 2023 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > On Thu, Aug 17, 2023 at 10:07:37AM -0400, Alan Stern wrote: > > > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > > > > HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the > > > > > > system: > > > > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > > > > [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 > > > > > > > > > > What is the meaning of the 0x202a0 bits? What caused this wakeup? > > > > > > > > And more to the point, given that the previous line says "wakeup 0", why > > > > should any port change event cause a wakeup? > > > > > > I think the controller and roothub have to deal with the interrupt > > > about disconnecting regardless of the remote wakeup setting. > > > > This seems to contradict what you wrote in an earlier email: > > Nothing is contradicting, what I mentioned is the wakeup of USB > touchpanel, not the controller or root hub. Now I'm getting even more confused. How can the touchpanel generate a wakeup request while it is powered off? > > > > If remote wakeup isn't enabled then the do_wakeup variable will be 0, > > > > so your patch wouldn't make any difference. The question is what > > > > happens when remote wakeup _is_ enabled. > > > > > > Nothing happens either per my testing. > > > > > > For USB keyboard, the remote wakeup is enabled, unplugging it when > > > suspend is suspended doesn't wake the system up, despite of PORT_WKDISC_E being set. > > > Plugging it back doesn't wake the system up either, despite of PORT_WKCONN_E. > > > > You appear to be saying that when wakeup is disabled, unplugging a > > device will wake up the system -- but when wakeup is enabled, unplugging > > a device will not wake up the system! > > No, what I was saying is that when PORT_WKCONN_E and PORT_WKDISC_E are > set, plugging/unplugging USB doesn't wake up the system from suspended > state. Setting those bits tells the controller hardware what to do when a connect or disconnect event occurs. Whether the controller's actions will end up waking up the system depends on other settings, such as the PCI/ACPI PME# settings. However, if those bits are not set then the controller should not generate a wakeup request in response to a connect or disconnect event. > What it really does for this case is to hinder the suspending process. Do you mean that if bits are set then the host controller generates a wakeup signal when a connect or disconnect occurs while the system is suspending? And this wakeup signal causes the system suspend to be aborted? Or do you mean that if the bits are clear then somehow the system suspend gets aborted? > > The point I'm trying to get at is that if wakeups are disabled for both > > the host controller and the root hub then _nothing_ should generate an > > interrupt or wakeup request. Not pressing a key, not unplugging a > > device... nothing. But if wakeup _is_ enabled for both the controller > > and the root hub, then any of those actions should generate an interrupt > > and wake up the system. > > Like above, the wakeup I mentioned is on the USB touchpanel itself, > not on the controller and roothub. Do you mean that the touchpanel sends a wakeup signal through some other route, not over the USB bus? Or do you mean that the touchpanel generates a wakeup signal and sends it over the USB bus to the root hub? > There's no IRQ generated when controller's wakeup is disabled. That's what you would expect. > > If wakeup is enabled for the host controller but not for the root hub, > > then unplugging a device from the root hub should not generate a wakeup > > request or an interrupt. But things like pressing a key on a > > wakeup-enabled keyboard should. (In other words, the root hub shouldn't > > generate any wakeup requests on its own but it should relay wakeup > > requests that it receives from downstream devices.) However, it's > > understandable if the system doesn't behave properly in this case since > > it's kind of an odd situation. > > Do you mean when the system is suspended, or system is still suspending? Both. The above applies any time the root hub is suspended. > The issue only happens when the system is suspending. What you need is something you can test manually, so that you can really tell what's going on. For example, you could add a 10-second delay at some point in the xhci suspend routine and try plugging or unplugging a device during that delay. What happens if the delay is at the start of the suspend routine? What happens if the delay is at the end? At what point in the routine does the behavior change? What do the port status bits show? You get the idea. Alan Stern
On Fri, Aug 18, 2023 at 10:19:27PM +0800, Kai-Heng Feng wrote: > On Fri, Aug 18, 2023 at 11:19 AM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Aug 18, 2023 at 08:00:39AM +0800, Kai-Heng Feng wrote: > > > On Thu, Aug 17, 2023 at 10:07 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > On Thu, Aug 17, 2023 at 05:33:05PM +0800, Kai-Heng Feng wrote: > > > > > The system is designed to let display and touchpanel share the same > > > > > power source, so when the display becomes off, the USB touchpanel also > > > > > lost its power and disconnect itself from USB bus. That doesn't play > > > > > well when most Desktop Environment lock and turnoff the display right > > > > > before entering system suspend. > > > > > > > > I don't see why that should cause any trouble. The display gets locked > > > > and turned off, the touchpanel disconnects from the USB bus, and then > > > > the system goes into suspend. Why would there be a wakeup signal at > > > > this point? > > > > > > The disconnecting can happens during the system suspend process, so > > > the suspend process is aborted. > > > > Maybe these systems need to add a little delay when the display is > > turned off, in order to give the touchpanel time to disconnect before > > the system suspend begins. > > Unfortunately the hardware can't be changed. No, but the software can. Change the kernel routine that controls the display's power, so that when it's turning the power off it delays a little before returning. > > > > > So for system-wide suspend, also disable connect, disconnect and > > > > > over-current wakeup to prevent spurious wakeup. > > > > > > > > Whether to disable these things is part of the userspace policy. The > > > > kernel should not make the decision; the user does by enabling or > > > > disabling wakeups. > > > > > > The power/wakeup is already disabled. > > > > In that case the root hub should not generate a wakeup request in > > response to the touchpanel disconnecting. > > Here's the wakeup setting when the issue happens: > controller - wakeup enabled > root hub: wakeup disabled > touchpanel: wakeup disabled Your patch turns off the PORT_WAKE_BITS in xhci_disable_hub_port_wake(), right? (Incidentally, the patch changes the code but doesn't change the immediately preceding comment, which will cause the comment to be wrong.) But if the root hub was suspended with wakeup disabled then those bits should already be off. So I don't see why your patch causes any change in behavior. > > > The disconnecting event is from roothub and if roothub wakeup is > > > disabled, other USB devices lose the ability to wake the system up > > > from system suspend. > > > > That shouldn't happen either. Disabling wakeup on the root hub should > > not prevent the root hub from relaying wakeup requests it receives from > > downstream devices. It should merely prevent the root hub from > > generating its own wakeup requests for connects, disconnects, and > > over-current events. > > Sorry, it was meant to be the xHCI controller. The didn't make the > difference clear. > > > > > It sounds like the xhci root-hub code isn't doing the right thing, at > > least, not on your systems. > > I still don't fully understand why removing PORT_WAKE_BITS is not > right in this case. The decision about whether to turn those bits on or off should depend on the wakeup settings. They should be on if wakeup is enabled and off if wakeup is disabled. Your patch doesn't work that way; it turns the bits off even when wakeup is enabled. Alan Stern
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index fae994f679d4..dc499100efa6 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -16,6 +16,7 @@ #include <linux/module.h> #include <linux/moduleparam.h> #include <linux/slab.h> +#include <linux/suspend.h> #include <linux/dmi.h> #include <linux/dma-mapping.h> @@ -789,7 +790,7 @@ static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci, t2 = t1; /* clear wake bits if do_wake is not set */ - if (!do_wakeup) + if (!do_wakeup || pm_suspend_target_state != PM_SUSPEND_ON) t2 &= ~PORT_WAKE_BITS; /* Don't touch csc bit if connected or connect change is set */
HP ProOne 440 G10 AIO sometimes cannot suspend as xHCI wakes up the system: [ 445.814574] hub 2-0:1.0: hub_suspend [ 445.814652] usb usb2: bus suspend, wakeup 0 [ 445.824629] xhci_hcd 0000:00:14.0: Port change event, 1-11, id 11, portsc: 0x202a0 [ 445.824639] xhci_hcd 0000:00:14.0: resume root hub [ 445.824651] xhci_hcd 0000:00:14.0: handle_port_status: starting usb1 port polling. [ 445.844039] xhci_hcd 0000:00:14.0: PM: pci_pm_suspend(): hcd_pci_suspend+0x0/0x20 returns -16 [ 445.844058] xhci_hcd 0000:00:14.0: PM: dpm_run_callback(): pci_pm_suspend+0x0/0x1c0 returns -16 [ 445.844072] xhci_hcd 0000:00:14.0: PM: failed to suspend async: error -16 [ 446.276101] PM: Some devices failed to suspend, or early wake event detected The system is designed to let display and touchpanel share the same power source, so when the display becomes off, the USB touchpanel also lost its power and disconnect itself from USB bus. That doesn't play well when most Desktop Environment lock and turnoff the display right before entering system suspend. So for system-wide suspend, also disable connect, disconnect and over-current wakeup to prevent spurious wakeup. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/usb/host/xhci.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)