Message-ID: <a2449e98-1258-4308-b246-b755e7a3708f@amd.com>
Date: Mon, 7 Oct 2024 09:53:35 -0500
From: Mario Limonciello <mario.limonciello@....com>
To: Kai-Heng Feng <kaihengf@...dia.com>, Hans de Goede <hdegoede@...hat.com>,
 ilpo.jarvinen@...ux.intel.com, gregkh@...uxfoundation.org,
 jorge.lopez2@...com
Cc: acelan.kao@...onical.com, platform-driver-x86@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v3] platform/x86/hp: Avoid spurious wakeup on HP ProOne
 440

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@...onical.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@...inos.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@...heas.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@...f.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:
>>
> 
> 

Powered by blists - more mailing lists