[PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[Kai-Heng Feng]

Move struct usb_port to linux/usb.h so other subsystems can use it too.

The user will be introduced in next patch.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
 drivers/usb/core/hub.h | 45 -------------------------------------
 drivers/usb/core/usb.h |  6 -----
 include/linux/usb.h    | 51 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index e6ae73f8a95d..8b488c247a1e 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -77,51 +77,6 @@ struct usb_hub {
 	struct list_head        onboard_devs;
 };
 
-/**
- * struct usb port - kernel's representation of a usb port
- * @child: usb device attached to the port
- * @dev: generic device interface
- * @port_owner: port's owner
- * @peer: related usb2 and usb3 ports (share the same connector)
- * @connector: USB Type-C connector
- * @req: default pm qos request for hubs without port power control
- * @connect_type: port's connect type
- * @state: device state of the usb device attached to the port
- * @state_kn: kernfs_node of the sysfs attribute that accesses @state
- * @location: opaque representation of platform connector location
- * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
- * @portnum: port index num based one
- * @is_superspeed cache super-speed status
- * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted.
- * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted.
- * @early_stop: whether port initialization will be stopped earlier.
- * @ignore_event: whether events of the port are ignored.
- */
-struct usb_port {
-	struct usb_device *child;
-	struct device dev;
-	struct usb_dev_state *port_owner;
-	struct usb_port *peer;
-	struct typec_connector *connector;
-	struct dev_pm_qos_request *req;
-	enum usb_port_connect_type connect_type;
-	enum usb_device_state state;
-	struct kernfs_node *state_kn;
-	usb_port_location_t location;
-	struct mutex status_lock;
-	u32 over_current_count;
-	u8 portnum;
-	u32 quirks;
-	unsigned int early_stop:1;
-	unsigned int ignore_event:1;
-	unsigned int is_superspeed:1;
-	unsigned int usb3_lpm_u1_permit:1;
-	unsigned int usb3_lpm_u2_permit:1;
-};
-
-#define to_usb_port(_dev) \
-	container_of(_dev, struct usb_port, dev)
-
 extern int usb_hub_create_port_device(struct usb_hub *hub,
 		int port1);
 extern void usb_hub_remove_port_device(struct usb_hub *hub,
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index b8324ea05b20..3f45b6978660 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -192,12 +192,6 @@ extern const struct file_operations usbdev_file_operations;
 extern int usb_devio_init(void);
 extern void usb_devio_cleanup(void);
 
-/*
- * Firmware specific cookie identifying a port's location. '0' == no location
- * data available
- */
-typedef u32 usb_port_location_t;
-
 /* internal notify stuff */
 extern void usb_notify_add_device(struct usb_device *udev);
 extern void usb_notify_remove_device(struct usb_device *udev);
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 832997a9da0a..1a8f94cc2bc1 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -772,6 +772,57 @@ extern void usb_queue_reset_device(struct usb_interface *dev);
 
 extern struct device *usb_intf_get_dma_device(struct usb_interface *intf);
 
+/*
+ * Firmware specific cookie identifying a port's location. '0' == no location
+ * data available
+ */
+typedef u32 usb_port_location_t;
+
+/**
+ * struct usb port - kernel's representation of a usb port
+ * @child: usb device attached to the port
+ * @dev: generic device interface
+ * @port_owner: port's owner
+ * @peer: related usb2 and usb3 ports (share the same connector)
+ * @connector: USB Type-C connector
+ * @req: default pm qos request for hubs without port power control
+ * @connect_type: port's connect type
+ * @state: device state of the usb device attached to the port
+ * @state_kn: kernfs_node of the sysfs attribute that accesses @state
+ * @location: opaque representation of platform connector location
+ * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
+ * @portnum: port index num based one
+ * @is_superspeed cache super-speed status
+ * @usb3_lpm_u1_permit: whether USB3 U1 LPM is permitted.
+ * @usb3_lpm_u2_permit: whether USB3 U2 LPM is permitted.
+ * @early_stop: whether port initialization will be stopped earlier.
+ * @ignore_event: whether events of the port are ignored.
+ */
+struct usb_port {
+	struct usb_device *child;
+	struct device dev;
+	struct usb_dev_state *port_owner;
+	struct usb_port *peer;
+	struct typec_connector *connector;
+	struct dev_pm_qos_request *req;
+	enum usb_port_connect_type connect_type;
+	enum usb_device_state state;
+	struct kernfs_node *state_kn;
+	usb_port_location_t location;
+	struct mutex status_lock;
+	u32 over_current_count;
+	u8 portnum;
+	u32 quirks;
+	unsigned int early_stop:1;
+	unsigned int ignore_event:1;
+	unsigned int is_superspeed:1;
+	unsigned int usb3_lpm_u1_permit:1;
+	unsigned int usb3_lpm_u2_permit:1;
+};
+
+#define to_usb_port(_dev) \
+	container_of(_dev, struct usb_port, dev)
+
 #ifdef CONFIG_ACPI
 extern int usb_acpi_set_power_state(struct usb_device *hdev, int index,
 	bool enable);
-- 
2.43.0

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

[Kai-Heng Feng]

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@canonical.com>
---
 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@srcf.ucam.org>");
 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) {
+		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:
-- 
2.43.0

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

[Greg KH]

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@canonical.com>
> ---
>  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@srcf.ucam.org>");
>  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?

> +	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?

> +	}
> +
> +	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()?

> +		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,

greg k-h

Re: [PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[Greg KH]

On Thu, Sep 05, 2024 at 12:24:46PM +0800, Kai-Heng Feng wrote:
> Move struct usb_port to linux/usb.h so other subsystems can use it too.

These really are "internal to the usb core" functions and variables, I
am loath to export them as it requires that you "know" what the device
type is of something without any recorse if you get it wrong.  I
commented on patch 2/2 about that.

Could we provide a usb core function for you instead to help out?  What
exactly are you trying to get access to on the USB bus that you need to
use here, the port or the device?

thanks,

greg k-h

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

[Kai-Heng Feng]

On Thu, Sep 5, 2024 at 1:18 PM Greg KH <gregkh@linuxfoundation.org> 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@canonical.com>
> > ---
> >  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@srcf.ucam.org>");
> >  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

Re: [PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[Kai-Heng Feng]

On Thu, Sep 5, 2024 at 1:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 05, 2024 at 12:24:46PM +0800, Kai-Heng Feng wrote:
> > Move struct usb_port to linux/usb.h so other subsystems can use it too.
>
> These really are "internal to the usb core" functions and variables, I
> am loath to export them as it requires that you "know" what the device
> type is of something without any recorse if you get it wrong.  I
> commented on patch 2/2 about that.
>
> Could we provide a usb core function for you instead to help out?  What
> exactly are you trying to get access to on the USB bus that you need to
> use here, the port or the device?

The device so the quirk can check its vendor and product id.

That means a function or helper that can return USB port/device from
an ACPI path.

Kai-Heng

>
> thanks,
>
> greg k-h

Re: [PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[Greg KH]

On Thu, Sep 05, 2024 at 02:24:31PM +0800, Kai-Heng Feng wrote:
> On Thu, Sep 5, 2024 at 1:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Sep 05, 2024 at 12:24:46PM +0800, Kai-Heng Feng wrote:
> > > Move struct usb_port to linux/usb.h so other subsystems can use it too.
> >
> > These really are "internal to the usb core" functions and variables, I
> > am loath to export them as it requires that you "know" what the device
> > type is of something without any recorse if you get it wrong.  I
> > commented on patch 2/2 about that.
> >
> > Could we provide a usb core function for you instead to help out?  What
> > exactly are you trying to get access to on the USB bus that you need to
> > use here, the port or the device?
> 
> The device so the quirk can check its vendor and product id.
> 
> That means a function or helper that can return USB port/device from
> an ACPI path.

So you don't want the port, only the device.  Why not just search the
bus for a device, you don't care where in the acpi path it lives, right?
Or better yet, do this in the driver for the device itself, that's when
you know you have the right device.

thanks,

greg k-h

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

[Hans de Goede]

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.

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.

One more comment inline below.


> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
>  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@srcf.ucam.org>");
>  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.

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:

Re: [PATCH 1/2] usb: linux/usb.h: Move USB port definition to usb.h

[Kai-Heng Feng]

On Thu, Sep 5, 2024 at 2:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Thu, Sep 05, 2024 at 02:24:31PM +0800, Kai-Heng Feng wrote:
> > On Thu, Sep 5, 2024 at 1:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Sep 05, 2024 at 12:24:46PM +0800, Kai-Heng Feng wrote:
> > > > Move struct usb_port to linux/usb.h so other subsystems can use it too.
> > >
> > > These really are "internal to the usb core" functions and variables, I
> > > am loath to export them as it requires that you "know" what the device
> > > type is of something without any recorse if you get it wrong.  I
> > > commented on patch 2/2 about that.
> > >
> > > Could we provide a usb core function for you instead to help out?  What
> > > exactly are you trying to get access to on the USB bus that you need to
> > > use here, the port or the device?
> >
> > The device so the quirk can check its vendor and product id.
> >
> > That means a function or helper that can return USB port/device from
> > an ACPI path.
>
> So you don't want the port, only the device.  Why not just search the
> bus for a device, you don't care where in the acpi path it lives, right?
> Or better yet, do this in the driver for the device itself, that's when
> you know you have the right device.

The crucial part of the quirk is the device links between VGA and USB
controller to enforce suspend order, it can be hard to create and
maintain the links when the USB device can disappear at any moment.

Kai-Heng

>
> thanks,
>
> greg k-h

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

[Kai-Heng Feng]

On Thu, Sep 5, 2024 at 4:56 PM Hans de Goede <hdegoede@redhat.com> 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@canonical.com>
> > ---
> >  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@srcf.ucam.org>");
> >  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:
>