On Thu, Sep 5, 2024 at 1:18 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 05, 2024 at 12:24: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:
> > [ 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.
> >
> > 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);
>
> That's a brave cast, how do you "know" this is this device type?
Assuming the the physical device of "\_SB.PC00.XHCI.RHUB.HS11" is a usb_port.
But yes there's no guarantee.
>
> > + if (port_dev->state == USB_STATE_NOTATTACHED)
> > + return 0;
> > +
> > + udev = port_dev->child;
> > +
> > + if (udev) {
> > + 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);
>
> why 200?
A minimal delay that works. We can make it longer too.
>
> > + }
> > +
> > + 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));
>
> When drivers work properly, they are quiet, was this ment to be
> dev_dbg()?
Sure, will change it to dev_dbg() next revision.
>
> > + 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));
>
> Same here.
Thanks for the review.
Kai-Heng
>
> thanks,
>
> greg k-h
