Re: [PATCH 2/2] platform/x86/hp: Avoid spurious wakeup on HP ProOne 440

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 5, 2024 at 4:56 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Kai-Heng Feng,
>
> On 9/5/24 6:24 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. If the touchscreen is present, wait a while to let the USB disconnect
> >    event fire.
> > 3. Since the disconnect event already happened, the xhci's suspend
> >    routine won't be interrupted anymore.
>
> You only set the suspend-handler from the dmi-quirk callback, so it
> is only ever set on the affected laptop-model. So this can be simplified
> by simply always doing the msleep(200) on suspend, instead of poking at
> USB system internals to find out if the touchscreen is there on suspend.
>
> I guess there may be versions of this specific laptop with/without
> the touchscreen, but I'm not overly worried about adding a 200 ms delay
> on just 1 model laptop for the versions which don't have a touchscreen.

My original idea is not to impose the delay for unaffected systems.
But yes not poking at USB subsystem can make things much easier.

>
> A bigger worry which I have is that we are going to see the same problem
> on other vendor's laptops. So I think in the end we may need something
> done in a more generic manner, in e.g. the drm subsystem (since this
> is display related). For now lets go with this fix and when we hit similar
> cases we can figure out what a generic fix will look like.

Right now this is the only model I've seen that exhibit such behavior.
So I am not overly worried about such problem.

>
> One more comment inline below.
>
>
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> >  drivers/platform/x86/hp/hp-wmi.c | 104 ++++++++++++++++++++++++++++++-
> >  1 file changed, 103 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> > index 876e0a97cee1..80fc3ee4deaf 100644
> > --- a/drivers/platform/x86/hp/hp-wmi.c
> > +++ b/drivers/platform/x86/hp/hp-wmi.c
> > @@ -30,6 +30,9 @@
> >  #include <linux/rfkill.h>
> >  #include <linux/string.h>
> >  #include <linux/dmi.h>
> > +#include <linux/delay.h>
> > +#include <linux/pci.h>
> > +#include <linux/usb.h>
> >
> >  MODULE_AUTHOR("Matthew Garrett <mjg59@xxxxxxxxxxxxx>");
> >  MODULE_DESCRIPTION("HP laptop WMI driver");
> > @@ -1708,6 +1711,52 @@ static void __exit hp_wmi_bios_remove(struct platform_device *device)
> >               platform_profile_remove();
> >  }
> >
> > +static int hp_wmi_suspend_handler(struct device *device)
> > +{
> > +     acpi_handle handle;
> > +     struct acpi_device *adev;
> > +     struct device *physdev;
> > +     struct usb_port *port_dev;
> > +     struct usb_device *udev;
> > +     acpi_status status;
> > +     bool found = false;
> > +
> > +     /* The USB touchscreen device always connects to HS11 */
> > +     status = acpi_get_handle(NULL, "\\_SB.PC00.XHCI.RHUB.HS11", &handle);
> > +     if (ACPI_FAILURE(status))
> > +             return 0;
> > +
> > +     adev = acpi_fetch_acpi_dev(handle);
> > +     if (!adev)
> > +             return 0;
> > +
> > +     physdev = get_device(acpi_get_first_physical_node(adev));
> > +     if (!physdev)
> > +             return 0;
> > +
> > +     port_dev = to_usb_port(physdev);
> > +     if (port_dev->state == USB_STATE_NOTATTACHED)
> > +             return 0;
> > +
> > +     udev = port_dev->child;
> > +
> > +     if (udev) {
>
> This is racy. Often desktop environments will turn off the display
> before doing a system-suspend, so the touchscreen is already disconnected
> at this point but the USB subsystem may not have processed it yet.
>
> What if the USB subsystem processes the disconnect exactly at this point?
>
> Then your port_dev->child pointer is no longer valid and your passing
> a pointer to free-ed mem to usb_get_dev()
>
> As I said above since this code only runs on 1 model based on a DMI
> match just simplify this entire function to a single "msleep(200)"
> and be done with it.

OK, will scratch the poking part in next revision. Thanks for the review.

Kai-Heng

>
> Regards,
>
> Hans
>
>
> > +             usb_get_dev(udev);
> > +             if (le16_to_cpu(udev->descriptor.idVendor) == 0x1fd2 &&
> > +                 le16_to_cpu(udev->descriptor.idProduct) == 0x8102) {
> > +                     dev_dbg(&hp_wmi_platform_dev->dev, "LG Melfas touchscreen found\n");
> > +                     found = true;
> > +             }
> > +             usb_put_dev(udev);
> > +
> > +             /* Let the xhci have time to handle disconnect event */
> > +             if (found)
> > +                     msleep(200);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  static int hp_wmi_resume_handler(struct device *device)
> >  {
> >       /*
> > @@ -1745,7 +1794,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 +1920,57 @@ 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_info(&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_info(&hp_wmi_platform_dev->dev, "Suspend after %s\n",
> > +                              pci_name(vga));
> > +             else {
> > +                     device_link_del(xhci_link);
> > +                     return 1;
> > +             }
> > +     }
> > +
> > +
> > +     /* During system bootup, the display and the USB touchscreen device can
> > +      * be on and off several times, so the device may not be present during
> > +      * hp-wmi's probe routine. Try to find the device in suspend routine
> > +      * instead.
> > +      */
> > +     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 +2009,8 @@ static int __init hp_wmi_init(void)
> >                       goto err_unregister_device;
> >       }
> >
> > +     dmi_check_system(hp_wmi_quirk_table);
> > +
> >       return 0;
> >
> >  err_unregister_device:
>





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux