Re: [PATCH v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne 440
From: Mario Limonciello
Date: Mon Oct 07 2024 - 10:54:59 EST
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@xxxxxxxxxxxxx>
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@xxxxxxxxxx/
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@xxxxxxxxxxx/
?
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@xxxxxxxxxxxxx>");
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: