| Message ID | 20240906053047.459036-1-kai.heng.feng@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne 440 | expand |
Hi, On 9/6/24 7:30 AM, Kai-Heng Feng wrote: > The HP ProOne 440 has a power saving design that when the display is > off, it also cuts the USB touchscreen device's power off. > > This can cause system early wakeup because cutting the power off the > touchscreen device creates a disconnect event and prevent the system > from suspending: > [ 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 > > So add a quirk to make sure the following is happening: > 1. Let the i915 driver suspend first, to ensure the display is off so > system also cuts the USB touchscreen's power. > 2. Wait a while to let the USB disconnect event fire and get handled. > 3. Since the disconnect event already happened, the xhci's suspend > routine won't be interrupted anymore. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Thanks, patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Ilpo, do you plan to do another fixes pull-request for 6.11, or shall I add this to for-next to target 6.12-rc1 ? Either way works for me. If you plan to do another fixes pull-request, note that I plan to post a v2 of the panasonic patches this Monday. Regards, Hans > --- > v3: > - Use dev_dbg() instead of dev_info(). > > v2: > - Remove the part that searching for the touchscreen device. > - Wording. > > drivers/platform/x86/hp/hp-wmi.c | 59 +++++++++++++++++++++++++++++++- > 1 file changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c > index 876e0a97cee1..92cb02b50dfc 100644 > --- a/drivers/platform/x86/hp/hp-wmi.c > +++ b/drivers/platform/x86/hp/hp-wmi.c > @@ -30,6 +30,8 @@ > #include <linux/rfkill.h> > #include <linux/string.h> > #include <linux/dmi.h> > +#include <linux/delay.h> > +#include <linux/pci.h> > > MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>"); > MODULE_DESCRIPTION("HP laptop WMI driver"); > @@ -1708,6 +1710,14 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device) > platform_profile_remove(); > } > > +static int hp_wmi_suspend_handler(struct device *device) > +{ > + /* Let the xhci have time to handle disconnect event */ > + msleep(200); > + > + return 0; > +} > + > static int hp_wmi_resume_handler(struct device *device) > { > /* > @@ -1745,7 +1755,7 @@ static int hp_wmi_resume_handler(struct device *device) > return 0; > } > > -static const struct dev_pm_ops hp_wmi_pm_ops = { > +static struct dev_pm_ops hp_wmi_pm_ops = { > .resume = hp_wmi_resume_handler, > .restore = hp_wmi_resume_handler, > }; > @@ -1871,6 +1881,51 @@ static int hp_wmi_hwmon_init(void) > return 0; > } > > +static int lg_usb_touchscreen_quirk(const struct dmi_system_id *id) > +{ > + struct pci_dev *vga, *xhci; > + struct device_link *vga_link, *xhci_link; > + > + vga = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL); > + > + xhci = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL); > + > + if (vga && xhci) { > + xhci_link = device_link_add(&hp_wmi_platform_dev->dev, &xhci->dev, > + DL_FLAG_STATELESS); > + if (xhci_link) > + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend before %s\n", > + pci_name(xhci)); > + else > + return 1; > + > + vga_link = device_link_add(&vga->dev, &hp_wmi_platform_dev->dev, > + DL_FLAG_STATELESS); > + if (vga_link) > + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend after %s\n", > + pci_name(vga)); > + else { > + device_link_del(xhci_link); > + return 1; > + } > + } > + > + hp_wmi_pm_ops.suspend = hp_wmi_suspend_handler; > + > + return 1; > +} > + > +static const struct dmi_system_id hp_wmi_quirk_table[] = { > + { > + .callback = lg_usb_touchscreen_quirk, > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > + DMI_MATCH(DMI_PRODUCT_NAME, "HP ProOne 440 23.8 inch G9 All-in-One Desktop PC"), > + }, > + }, > + {} > +}; > + > static int __init hp_wmi_init(void) > { > int event_capable = wmi_has_guid(HPWMI_EVENT_GUID); > @@ -1909,6 +1964,8 @@ static int __init hp_wmi_init(void) > goto err_unregister_device; > } > > + dmi_check_system(hp_wmi_quirk_table); > + > return 0; > > err_unregister_device:
On Fri, Sep 06, 2024 at 01:30:47PM +0800, Kai-Heng Feng wrote: > The HP ProOne 440 has a power saving design that when the display is > off, it also cuts the USB touchscreen device's power off. > > This can cause system early wakeup because cutting the power off the > touchscreen device creates a disconnect event and prevent the system > from suspending: Is the touchscreen device connected directly to the root hub? If it is then it looks like there's a separate bug here, which needs to be fixed. > [ 445.814574] hub 2-0:1.0: hub_suspend > [ 445.814652] usb usb2: bus suspend, wakeup 0 Since the wakeup flag is set to 0, the root hub should not generate a wakeup request when a port-status-change event happens. > [ 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 But it did. This appears to be a bug in one of the xhci-hcd suspend routines. Alternatively, if the touchscreen device is connected to an intermediate hub then that intermediate hub should not be allowed to generate wakeup events. That's determined by userspace, though, not by the kernel. Alan Stern
HI, On 9/6/24 8:39 PM, Ilpo Järvinen wrote: > On Fri, 6 Sep 2024, Hans de Goede wrote: >> On 9/6/24 7:30 AM, Kai-Heng Feng wrote: >>> The HP ProOne 440 has a power saving design that when the display is >>> off, it also cuts the USB touchscreen device's power off. >>> >>> This can cause system early wakeup because cutting the power off the >>> touchscreen device creates a disconnect event and prevent the system >>> from suspending: >>> [ 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 >>> >>> So add a quirk to make sure the following is happening: >>> 1. Let the i915 driver suspend first, to ensure the display is off so >>> system also cuts the USB touchscreen's power. >>> 2. Wait a while to let the USB disconnect event fire and get handled. >>> 3. Since the disconnect event already happened, the xhci's suspend >>> routine won't be interrupted anymore. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> Thanks, patch looks good to me: >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> >> Ilpo, do you plan to do another fixes pull-request for 6.11, >> or shall I add this to for-next to target 6.12-rc1 ? >> >> Either way works for me. If you plan to do another fixes >> pull-request, note that I plan to post a v2 of the panasonic >> patches this Monday. > > Hi Hans, > > I was thinking that perhaps one more is necessary the next week. Ok sounds good, but given Alan's remarks lets hold of on merging this one until we are sure this is not something which can / should be fixed on the USB side, or with a hwdb entry to change the hub wakeup setting for the hub to which the touchscreen is attached. Regards, Hans
On Fri, Sep 6, 2024 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Fri, Sep 06, 2024 at 01:30:47PM +0800, Kai-Heng Feng wrote: > > The HP ProOne 440 has a power saving design that when the display is > > off, it also cuts the USB touchscreen device's power off. > > > > This can cause system early wakeup because cutting the power off the > > touchscreen device creates a disconnect event and prevent the system > > from suspending: > > Is the touchscreen device connected directly to the root hub? If it is > then it looks like there's a separate bug here, which needs to be fixed. > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > Since the wakeup flag is set to 0, the root hub should not generate a > wakeup request when a port-status-change event happens. The disconnect event itself should not generate a wake request, but the interrupt itself still needs to be handled. > > > [ 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 > > But it did. This appears to be a bug in one of the xhci-hcd suspend > routines. So should the xhci-hcd delay all interrupt handling after system resume? > > Alternatively, if the touchscreen device is connected to an intermediate > hub then that intermediate hub should not be allowed to generate wakeup > events. That's determined by userspace, though, not by the kernel. It's connected to roothub. Kai-Heng > > Alan Stern
On Mon, Sep 09, 2024 at 11:05:05AM +0800, Kai-Heng Feng wrote: > On Fri, Sep 6, 2024 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > On Fri, Sep 06, 2024 at 01:30:47PM +0800, Kai-Heng Feng wrote: > > > The HP ProOne 440 has a power saving design that when the display is > > > off, it also cuts the USB touchscreen device's power off. > > > > > > This can cause system early wakeup because cutting the power off the > > > touchscreen device creates a disconnect event and prevent the system > > > from suspending: > > > > Is the touchscreen device connected directly to the root hub? If it is > > then it looks like there's a separate bug here, which needs to be fixed. > > > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > > Since the wakeup flag is set to 0, the root hub should not generate a > > wakeup request when a port-status-change event happens. > > The disconnect event itself should not generate a wake request, but > the interrupt itself still needs to be handled. > > > > > > [ 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 > > > > But it did. This appears to be a bug in one of the xhci-hcd suspend > > routines. I failed to notice before that the suspend message message above is for bus 2 whereas the port change event here is on bus 1. Nevertheless, I assume that bus 1 was suspended with wakeup = 0, so the idea is the same. > So should the xhci-hcd delay all interrupt handling after system resume? It depends on how the hardware works; I don't know the details. The best approach would be: when suspending the root hub with wakeup = 0, the driver will tell the hardware not to generate interrupt requests for port-change events (and then re-enable those interrupt requests when the root hub is resumed, later on). If that's not possible, another possibility is that the driver could handle the interrupt and clear the hardware's port-change status bit but then not ask for the root hub to be resumed. However, this would probably be more difficult to get right. Alan Stern
On Tue, Sep 10, 2024 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, Sep 10, 2024 at 11:33:02AM +0800, Kai-Heng Feng wrote: > > On Mon, Sep 9, 2024 at 10:39 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > On Mon, Sep 09, 2024 at 11:05:05AM +0800, Kai-Heng Feng wrote: > > > > On Fri, Sep 6, 2024 at 10:22 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > > > > > > > > > On Fri, Sep 06, 2024 at 01:30:47PM +0800, Kai-Heng Feng wrote: > > > > > > The HP ProOne 440 has a power saving design that when the display is > > > > > > off, it also cuts the USB touchscreen device's power off. > > > > > > > > > > > > This can cause system early wakeup because cutting the power off the > > > > > > touchscreen device creates a disconnect event and prevent the system > > > > > > from suspending: > > > > > > > > > > Is the touchscreen device connected directly to the root hub? If it is > > > > > then it looks like there's a separate bug here, which needs to be fixed. > > > > > > > > > > > [ 445.814574] hub 2-0:1.0: hub_suspend > > > > > > [ 445.814652] usb usb2: bus suspend, wakeup 0 > > > > > > > > > > Since the wakeup flag is set to 0, the root hub should not generate a > > > > > wakeup request when a port-status-change event happens. > > > > > > > > The disconnect event itself should not generate a wake request, but > > > > the interrupt itself still needs to be handled. > > > > > > > > > > > > > > > [ 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 > > > > > > > > > > But it did. This appears to be a bug in one of the xhci-hcd suspend > > > > > routines. > > > > > > I failed to notice before that the suspend message message above is for > > > bus 2 whereas the port change event here is on bus 1. Nevertheless, I > > > assume that bus 1 was suspended with wakeup = 0, so the idea is the > > > same. > > > > Yes the bus 1 was already suspended. > > > > > > > > > So should the xhci-hcd delay all interrupt handling after system resume? > > > > > > It depends on how the hardware works; I don't know the details. The > > > best approach would be: when suspending the root hub with wakeup = 0, > > > the driver will tell the hardware not to generate interrupt requests for > > > port-change events (and then re-enable those interrupt requests when the > > > root hub is resumed, later on). > > > > So the XHCI_CMD_EIE needs to be cleared in prepare callback to ensure > > there's no interrupt in suspend callback. > > Not in the prepare callback. Clear it during the suspend callback. > > But now reading this and the earlier section, I realize what the problem > is. There's only one bit in the command register to control IRQ > generation, so you can't turn off interrupt requests for bus 1 (the > legacy USB-2 bus) without also turning them off for bus 2 (the USB-3 > bus). > > > Maybe this can be done, but this seems to greatly alter the xHCI suspend flow. > Yes, this approach isn't feasible. > > > > If that's not possible, another possibility is that the driver could > > > handle the interrupt and clear the hardware's port-change status bit but > > > then not ask for the root hub to be resumed. However, this would > > > probably be more difficult to get right. > > > > IIUC the portsc status bit gets cleared after roothub is resumed. So > > this also brings not insignificant change. > > Not sure if its the best approach. > > It should be possible for this to work. Just make the interrupt > handler skip calling usb_hcd_resume_root_hub() if wakeup is not enabled > for the root hub getting the port-status change. When the root hub > resumes as part of the system resume later on, the hub driver will check > and see that a connect change occurred. This can work. But should the change be made in usb_hcd_resume_root_hub() or by the caller? The issue can potentially happen to all USB controllers, not just xHCI. Kai-Heng > > Alan Stern
On Thu, Sep 12, 2024 at 02:28:15PM +0800, Kai-Heng Feng wrote: > On Tue, Sep 10, 2024 at 9:13 PM Alan Stern <stern@rowland.harvard.edu> wrote: > > It should be possible for this to work. Just make the interrupt > > handler skip calling usb_hcd_resume_root_hub() if wakeup is not enabled > > for the root hub getting the port-status change. When the root hub > > resumes as part of the system resume later on, the hub driver will check > > and see that a connect change occurred. > > This can work. But should the change be made in > usb_hcd_resume_root_hub() or by the caller? > The issue can potentially happen to all USB controllers, not just xHCI. True. However, we need to make sure that remote wakeup continues to work properly. This means that the handler should skip calling usb_hcd_resume_root_hub() (when the root hub is suspended with wakeup = 0) for port connect/disconnect changes or for port overcurrent changes. But it should _not_ skip calling usb_hcd_resume_root_hub() for port resume events (i.e., wakeup requests received from downstream). usb_hcd_resume_root_hub() does not have enough information to know the reason for the resume; only the interrupt handler does. Have you been following the discussion in this other email thread? https://lore.kernel.org/linux-usb/20240906030548.845115-1-duanchenghao@kylinos.cn/ It seems very similar to the problem you are trying to fix. Alan Stern
Hi Kai-Heng, On 6-Sep-24 7:30 AM, Kai-Heng Feng wrote: > The HP ProOne 440 has a power saving design that when the display is > off, it also cuts the USB touchscreen device's power off. > > This can cause system early wakeup because cutting the power off the > touchscreen device creates a disconnect event and prevent the system > from suspending: > [ 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 > > So add a quirk to make sure the following is happening: > 1. Let the i915 driver suspend first, to ensure the display is off so > system also cuts the USB touchscreen's power. > 2. Wait a while to let the USB disconnect event fire and get handled. > 3. Since the disconnect event already happened, the xhci's suspend > routine won't be interrupted anymore. > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> I was wondering if there is any progress in trying to come up with a more generic fix at the USB hub level for this as discussed in other emails in this thread ? Also have you seen this series: [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/ ? I wonder if that is relevant. If the touchscreen gets turned off when the GPU enters D3 then this will not help, but if it gets turned off by the system wide Display Off call as described in that series then that series + extending patch 3 to maybe also include the HP ProOne 440 might be another (cleaner) way to fix this ? Regards, Hans > --- > v3: > - Use dev_dbg() instead of dev_info(). > > v2: > - Remove the part that searching for the touchscreen device. > - Wording. > > drivers/platform/x86/hp/hp-wmi.c | 59 +++++++++++++++++++++++++++++++- > 1 file changed, 58 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c > index 876e0a97cee1..92cb02b50dfc 100644 > --- a/drivers/platform/x86/hp/hp-wmi.c > +++ b/drivers/platform/x86/hp/hp-wmi.c > @@ -30,6 +30,8 @@ > #include <linux/rfkill.h> > #include <linux/string.h> > #include <linux/dmi.h> > +#include <linux/delay.h> > +#include <linux/pci.h> > > MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>"); > MODULE_DESCRIPTION("HP laptop WMI driver"); > @@ -1708,6 +1710,14 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device) > platform_profile_remove(); > } > > +static int hp_wmi_suspend_handler(struct device *device) > +{ > + /* Let the xhci have time to handle disconnect event */ > + msleep(200); > + > + return 0; > +} > + > static int hp_wmi_resume_handler(struct device *device) > { > /* > @@ -1745,7 +1755,7 @@ static int hp_wmi_resume_handler(struct device *device) > return 0; > } > > -static const struct dev_pm_ops hp_wmi_pm_ops = { > +static struct dev_pm_ops hp_wmi_pm_ops = { > .resume = hp_wmi_resume_handler, > .restore = hp_wmi_resume_handler, > }; > @@ -1871,6 +1881,51 @@ static int hp_wmi_hwmon_init(void) > return 0; > } > > +static int lg_usb_touchscreen_quirk(const struct dmi_system_id *id) > +{ > + struct pci_dev *vga, *xhci; > + struct device_link *vga_link, *xhci_link; > + > + vga = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL); > + > + xhci = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL); > + > + if (vga && xhci) { > + xhci_link = device_link_add(&hp_wmi_platform_dev->dev, &xhci->dev, > + DL_FLAG_STATELESS); > + if (xhci_link) > + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend before %s\n", > + pci_name(xhci)); > + else > + return 1; > + > + vga_link = device_link_add(&vga->dev, &hp_wmi_platform_dev->dev, > + DL_FLAG_STATELESS); > + if (vga_link) > + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend after %s\n", > + pci_name(vga)); > + else { > + device_link_del(xhci_link); > + return 1; > + } > + } > + > + hp_wmi_pm_ops.suspend = hp_wmi_suspend_handler; > + > + return 1; > +} > + > +static const struct dmi_system_id hp_wmi_quirk_table[] = { > + { > + .callback = lg_usb_touchscreen_quirk, > + .matches = { > + DMI_MATCH(DMI_SYS_VENDOR, "HP"), > + DMI_MATCH(DMI_PRODUCT_NAME, "HP ProOne 440 23.8 inch G9 All-in-One Desktop PC"), > + }, > + }, > + {} > +}; > + > static int __init hp_wmi_init(void) > { > int event_capable = wmi_has_guid(HPWMI_EVENT_GUID); > @@ -1909,6 +1964,8 @@ static int __init hp_wmi_init(void) > goto err_unregister_device; > } > > + dmi_check_system(hp_wmi_quirk_table); > + > return 0; > > err_unregister_device:
Hi Hans, On 2024/10/5 10:25 PM, Hans de Goede wrote: > Hi Kai-Heng, > > On 6-Sep-24 7:30 AM, Kai-Heng Feng wrote: >> The HP ProOne 440 has a power saving design that when the display is >> off, it also cuts the USB touchscreen device's power off. >> >> This can cause system early wakeup because cutting the power off the >> touchscreen device creates a disconnect event and prevent the system >> from suspending: >> [ 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 >> >> So add a quirk to make sure the following is happening: >> 1. Let the i915 driver suspend first, to ensure the display is off so >> system also cuts the USB touchscreen's power. >> 2. Wait a while to let the USB disconnect event fire and get handled. >> 3. Since the disconnect event already happened, the xhci's suspend >> routine won't be interrupted anymore. >> >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > I was wondering if there is any progress in trying to come up with > a more generic fix at the USB hub level for this as discussed in > other emails in this thread ? This patch fixes this issue and IMO quite generic: https://lore.kernel.org/linux-usb/20240906030548.845115-1-duanchenghao@kylinos.cn/ > > Also have you seen this series: > > [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) > https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1-lkml@antheas.dev/ > > ? > > I wonder if that is relevant. If the touchscreen gets turned off when > the GPU enters D3 then this will not help, but if it gets turned off > by the system wide Display Off call as described in that series then > that series + extending patch 3 to maybe also include the HP ProOne 440 > might be another (cleaner) way to fix this ? The series won't help. The display was turned off when i915 turning off CRTCs, so it's much earlier than the LPI's Display Off. If the the touchsreen is turned off by Display Off, then the issue shouldn't exist at all, as .suspend_noirq for xHCI is already called. Kai-Heng > > Regards, > > Hans > > > > > > > > > > > > > > > > > >> --- >> v3: >> - Use dev_dbg() instead of dev_info(). >> >> v2: >> - Remove the part that searching for the touchscreen device. >> - Wording. >> >> drivers/platform/x86/hp/hp-wmi.c | 59 +++++++++++++++++++++++++++++++- >> 1 file changed, 58 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c >> index 876e0a97cee1..92cb02b50dfc 100644 >> --- a/drivers/platform/x86/hp/hp-wmi.c >> +++ b/drivers/platform/x86/hp/hp-wmi.c >> @@ -30,6 +30,8 @@ >> #include <linux/rfkill.h> >> #include <linux/string.h> >> #include <linux/dmi.h> >> +#include <linux/delay.h> >> +#include <linux/pci.h> >> >> MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>"); >> MODULE_DESCRIPTION("HP laptop WMI driver"); >> @@ -1708,6 +1710,14 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device) >> platform_profile_remove(); >> } >> >> +static int hp_wmi_suspend_handler(struct device *device) >> +{ >> + /* Let the xhci have time to handle disconnect event */ >> + msleep(200); >> + >> + return 0; >> +} >> + >> static int hp_wmi_resume_handler(struct device *device) >> { >> /* >> @@ -1745,7 +1755,7 @@ static int hp_wmi_resume_handler(struct device *device) >> return 0; >> } >> >> -static const struct dev_pm_ops hp_wmi_pm_ops = { >> +static struct dev_pm_ops hp_wmi_pm_ops = { >> .resume = hp_wmi_resume_handler, >> .restore = hp_wmi_resume_handler, >> }; >> @@ -1871,6 +1881,51 @@ static int hp_wmi_hwmon_init(void) >> return 0; >> } >> >> +static int lg_usb_touchscreen_quirk(const struct dmi_system_id *id) >> +{ >> + struct pci_dev *vga, *xhci; >> + struct device_link *vga_link, *xhci_link; >> + >> + vga = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL); >> + >> + xhci = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL); >> + >> + if (vga && xhci) { >> + xhci_link = device_link_add(&hp_wmi_platform_dev->dev, &xhci->dev, >> + DL_FLAG_STATELESS); >> + if (xhci_link) >> + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend before %s\n", >> + pci_name(xhci)); >> + else >> + return 1; >> + >> + vga_link = device_link_add(&vga->dev, &hp_wmi_platform_dev->dev, >> + DL_FLAG_STATELESS); >> + if (vga_link) >> + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend after %s\n", >> + pci_name(vga)); >> + else { >> + device_link_del(xhci_link); >> + return 1; >> + } >> + } >> + >> + hp_wmi_pm_ops.suspend = hp_wmi_suspend_handler; >> + >> + return 1; >> +} >> + >> +static const struct dmi_system_id hp_wmi_quirk_table[] = { >> + { >> + .callback = lg_usb_touchscreen_quirk, >> + .matches = { >> + DMI_MATCH(DMI_SYS_VENDOR, "HP"), >> + DMI_MATCH(DMI_PRODUCT_NAME, "HP ProOne 440 23.8 inch G9 All-in-One Desktop PC"), >> + }, >> + }, >> + {} >> +}; >> + >> static int __init hp_wmi_init(void) >> { >> int event_capable = wmi_has_guid(HPWMI_EVENT_GUID); >> @@ -1909,6 +1964,8 @@ static int __init hp_wmi_init(void) >> goto err_unregister_device; >> } >> >> + dmi_check_system(hp_wmi_quirk_table); >> + >> return 0; >> >> err_unregister_device: >
On 10/6/2024 23:22, Kai-Heng Feng wrote: > Hi Hans, > > On 2024/10/5 10:25 PM, Hans de Goede wrote: >> Hi Kai-Heng, >> >> On 6-Sep-24 7:30 AM, Kai-Heng Feng wrote: >>> The HP ProOne 440 has a power saving design that when the display is >>> off, it also cuts the USB touchscreen device's power off. >>> >>> This can cause system early wakeup because cutting the power off the >>> touchscreen device creates a disconnect event and prevent the system >>> from suspending: >>> [ 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 >>> >>> So add a quirk to make sure the following is happening: >>> 1. Let the i915 driver suspend first, to ensure the display is off so >>> system also cuts the USB touchscreen's power. >>> 2. Wait a while to let the USB disconnect event fire and get handled. >>> 3. Since the disconnect event already happened, the xhci's suspend >>> routine won't be interrupted anymore. >>> >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> I was wondering if there is any progress in trying to come up with >> a more generic fix at the USB hub level for this as discussed in >> other emails in this thread ? > > This patch fixes this issue and IMO quite generic: > https://lore.kernel.org/linux-usb/20240906030548.845115-1- > duanchenghao@kylinos.cn/ > >> >> Also have you seen this series: >> >> [PATCH v2 0/5] acpi/x86: s2idle: move Display off/on calls outside >> suspend (fixes ROG Ally suspend) >> https://lore.kernel.org/platform-driver-x86/20240922172258.48435-1- >> lkml@antheas.dev/ >> >> ? >> >> I wonder if that is relevant. If the touchscreen gets turned off when >> the GPU enters D3 then this will not help, but if it gets turned off >> by the system wide Display Off call as described in that series then >> that series + extending patch 3 to maybe also include the HP ProOne 440 >> might be another (cleaner) way to fix this ? > > The series won't help. The display was turned off when i915 turning off > CRTCs, so it's much earlier than the LPI's Display Off. > > If the the touchsreen is turned off by Display Off, then the issue > shouldn't exist at all, as .suspend_noirq for xHCI is already called. Just FWIW the original PoC I that I did [1] that the series was based on did it in a DRM suspend callback. I don't think it materially changes things though because you still would have to get the device ordering between XHCI and DRM correct. [1] https://git.kernel.org/pub/scm/linux/kernel/git/superm1/linux.git/log/?h=superm1/dsm-screen-on-off > > Kai-Heng > >> >> Regards, >> >> Hans >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >>> --- >>> v3: >>> - Use dev_dbg() instead of dev_info(). >>> >>> v2: >>> - Remove the part that searching for the touchscreen device. >>> - Wording. >>> >>> drivers/platform/x86/hp/hp-wmi.c | 59 +++++++++++++++++++++++++++++++- >>> 1 file changed, 58 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/ >>> hp/hp-wmi.c >>> index 876e0a97cee1..92cb02b50dfc 100644 >>> --- a/drivers/platform/x86/hp/hp-wmi.c >>> +++ b/drivers/platform/x86/hp/hp-wmi.c >>> @@ -30,6 +30,8 @@ >>> #include <linux/rfkill.h> >>> #include <linux/string.h> >>> #include <linux/dmi.h> >>> +#include <linux/delay.h> >>> +#include <linux/pci.h> >>> MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>"); >>> MODULE_DESCRIPTION("HP laptop WMI driver"); >>> @@ -1708,6 +1710,14 @@ static void __exit hp_wmi_bios_remove(struct >>> platform_device *device) >>> platform_profile_remove(); >>> } >>> +static int hp_wmi_suspend_handler(struct device *device) >>> +{ >>> + /* Let the xhci have time to handle disconnect event */ >>> + msleep(200); >>> + >>> + return 0; >>> +} >>> + >>> static int hp_wmi_resume_handler(struct device *device) >>> { >>> /* >>> @@ -1745,7 +1755,7 @@ static int hp_wmi_resume_handler(struct device >>> *device) >>> return 0; >>> } >>> -static const struct dev_pm_ops hp_wmi_pm_ops = { >>> +static struct dev_pm_ops hp_wmi_pm_ops = { >>> .resume = hp_wmi_resume_handler, >>> .restore = hp_wmi_resume_handler, >>> }; >>> @@ -1871,6 +1881,51 @@ static int hp_wmi_hwmon_init(void) >>> return 0; >>> } >>> +static int lg_usb_touchscreen_quirk(const struct dmi_system_id *id) >>> +{ >>> + struct pci_dev *vga, *xhci; >>> + struct device_link *vga_link, *xhci_link; >>> + >>> + vga = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL); >>> + >>> + xhci = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL); >>> + >>> + if (vga && xhci) { >>> + xhci_link = device_link_add(&hp_wmi_platform_dev->dev, >>> &xhci->dev, >>> + DL_FLAG_STATELESS); >>> + if (xhci_link) >>> + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend before %s\n", >>> + pci_name(xhci)); >>> + else >>> + return 1; >>> + >>> + vga_link = device_link_add(&vga->dev, &hp_wmi_platform_dev- >>> >dev, >>> + DL_FLAG_STATELESS); >>> + if (vga_link) >>> + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend after %s\n", >>> + pci_name(vga)); >>> + else { >>> + device_link_del(xhci_link); >>> + return 1; >>> + } >>> + } >>> + >>> + hp_wmi_pm_ops.suspend = hp_wmi_suspend_handler; >>> + >>> + return 1; >>> +} >>> + >>> +static const struct dmi_system_id hp_wmi_quirk_table[] = { >>> + { >>> + .callback = lg_usb_touchscreen_quirk, >>> + .matches = { >>> + DMI_MATCH(DMI_SYS_VENDOR, "HP"), >>> + DMI_MATCH(DMI_PRODUCT_NAME, "HP ProOne 440 23.8 inch G9 >>> All-in-One Desktop PC"), >>> + }, >>> + }, >>> + {} >>> +}; >>> + >>> static int __init hp_wmi_init(void) >>> { >>> int event_capable = wmi_has_guid(HPWMI_EVENT_GUID); >>> @@ -1909,6 +1964,8 @@ static int __init hp_wmi_init(void) >>> goto err_unregister_device; >>> } >>> + dmi_check_system(hp_wmi_quirk_table); >>> + >>> return 0; >>> err_unregister_device: >> > >
diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c index 876e0a97cee1..92cb02b50dfc 100644 --- a/drivers/platform/x86/hp/hp-wmi.c +++ b/drivers/platform/x86/hp/hp-wmi.c @@ -30,6 +30,8 @@ #include <linux/rfkill.h> #include <linux/string.h> #include <linux/dmi.h> +#include <linux/delay.h> +#include <linux/pci.h> MODULE_AUTHOR("Matthew Garrett <mjg59@srcf.ucam.org>"); MODULE_DESCRIPTION("HP laptop WMI driver"); @@ -1708,6 +1710,14 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device) platform_profile_remove(); } +static int hp_wmi_suspend_handler(struct device *device) +{ + /* Let the xhci have time to handle disconnect event */ + msleep(200); + + return 0; +} + static int hp_wmi_resume_handler(struct device *device) { /* @@ -1745,7 +1755,7 @@ static int hp_wmi_resume_handler(struct device *device) return 0; } -static const struct dev_pm_ops hp_wmi_pm_ops = { +static struct dev_pm_ops hp_wmi_pm_ops = { .resume = hp_wmi_resume_handler, .restore = hp_wmi_resume_handler, }; @@ -1871,6 +1881,51 @@ static int hp_wmi_hwmon_init(void) return 0; } +static int lg_usb_touchscreen_quirk(const struct dmi_system_id *id) +{ + struct pci_dev *vga, *xhci; + struct device_link *vga_link, *xhci_link; + + vga = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, NULL); + + xhci = pci_get_class(PCI_CLASS_SERIAL_USB_XHCI, NULL); + + if (vga && xhci) { + xhci_link = device_link_add(&hp_wmi_platform_dev->dev, &xhci->dev, + DL_FLAG_STATELESS); + if (xhci_link) + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend before %s\n", + pci_name(xhci)); + else + return 1; + + vga_link = device_link_add(&vga->dev, &hp_wmi_platform_dev->dev, + DL_FLAG_STATELESS); + if (vga_link) + dev_dbg(&hp_wmi_platform_dev->dev, "Suspend after %s\n", + pci_name(vga)); + else { + device_link_del(xhci_link); + return 1; + } + } + + hp_wmi_pm_ops.suspend = hp_wmi_suspend_handler; + + return 1; +} + +static const struct dmi_system_id hp_wmi_quirk_table[] = { + { + .callback = lg_usb_touchscreen_quirk, + .matches = { + DMI_MATCH(DMI_SYS_VENDOR, "HP"), + DMI_MATCH(DMI_PRODUCT_NAME, "HP ProOne 440 23.8 inch G9 All-in-One Desktop PC"), + }, + }, + {} +}; + static int __init hp_wmi_init(void) { int event_capable = wmi_has_guid(HPWMI_EVENT_GUID); @@ -1909,6 +1964,8 @@ static int __init hp_wmi_init(void) goto err_unregister_device; } + dmi_check_system(hp_wmi_quirk_table); + return 0; err_unregister_device:
The HP ProOne 440 has a power saving design that when the display is off, it also cuts the USB touchscreen device's power off. This can cause system early wakeup because cutting the power off the touchscreen device creates a disconnect event and prevent the system from suspending: [ 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 So add a quirk to make sure the following is happening: 1. Let the i915 driver suspend first, to ensure the display is off so system also cuts the USB touchscreen's power. 2. Wait a while to let the USB disconnect event fire and get handled. 3. Since the disconnect event already happened, the xhci's suspend routine won't be interrupted anymore. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- v3: - Use dev_dbg() instead of dev_info(). v2: - Remove the part that searching for the touchscreen device. - Wording. drivers/platform/x86/hp/hp-wmi.c | 59 +++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-)