HID: Misc. fixes

Hi Jiri, Benjamin,

Here is v2 of what started out as a small series to fix spurious wakeups
on T101HA 2-in-1s.

This adds the discussed hid_is_usb_device() helper and uses that in:

"HID: multitouch: Disable event reporting on suspend when our parent is
not a wakeup-source"

To avoid needing to add a "depends on USB_HID" to hid-multitouch Kconfig
settings.

I've checked all other hid_is_using_ll_driver(hdev, &usb_hid_driver) callers
and the only one which can truely benefit from the new helper is the
hid-asus driver, which also deals with some I2C devices on some Asus hw.

All other drivers using hid_is_using_ll_driver(hdev, &usb_hid_driver)
are only for USB devices, so dropping the "depends on USB_HID" does not
make sense for them.

The one other driver which may benefit from the new hid_is_usb_device()
helper would be the Wacom driver which seems to also support I2C devices,
but that contains a lot of USB specific code, so I don't think we can
easily drop the "depends on USB_HID" there.

Even though this is a bit if a mixed-bag of patches, their are several
dependencies between them, so these should probably all go on a single
topic branch.

Regards,

Hans


Hans de Goede (6):
  HID: core: Remove extraneous empty line before
    EXPORT_SYMBOL_GPL(hid_check_keys_pressed)
  HID: core: Add a hid_is_usb_device() helper function
  HID: multitouch: Disable event reporting on suspend on the Asus T101HA

16 more lines

Normally the EXPORT_SYMBOL of a function immediately follows the
declaration of the function and all the other functions in hid-core.c
follow this pattern, drop the extraneous empty line before the
EXPORT_SYMBOL_GPL(hid_check_keys_pressed); line.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/hid-core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 097cb1ee3126..b593dff411a6 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -2588,7 +2588,6 @@ int hid_check_keys_pressed(struct hid_device *hid)
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
 
 static int __init hid_init(void)

-- 
2.31.1

Sometimes HID drivers want to know if the hid_device with which they
are dealing is using the usb_hid_driver. For example this is often
done to check if it is safe to cast hid_device->dev.parent to an
usb_interface like this:

struct usb_interface *intf = to_usb_interface(hdev->dev.parent);

If drivers directly call hid_is_using_ll_driver(hdev, &usb_hid_driver))
for this, then this leads to a "missing symbol usb_hid_driver"
compilation error when CONFIG_USB_HID is not enabled. Requiring
the driver to have a depends on USB_HID in their Kconfig entry
to work around this.

Add a hid_is_usb_device() helper function which drivers can use
to safely check if they are dealing with a usb_hid device without
needing to worry about the CONFIG_USB_HID setting.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/hid-core.c | 10 ++++++++++
 include/linux/hid.h    |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b593dff411a6..294c3cf05d85 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c

@@ -2590,6 +2590,16 @@ int hid_check_keys_pressed(struct hid_device *hid)
 }
 EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
 
+bool hid_is_usb_device(struct hid_device *hid)
+{
+#if IS_ENABLED(CONFIG_USB_HID)
+	return hid_is_using_ll_driver(hid, &usb_hid_driver);
+#else
+	return false;
+#endif
+}
+EXPORT_SYMBOL_GPL(hid_is_usb_device);
+
 static int __init hid_init(void)
 {
 	int ret;

diff --git a/include/linux/hid.h b/include/linux/hid.h
index ef702b3f56e3..6ceadc234132 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h

@@ -901,6 +901,7 @@ struct hid_report *hid_validate_values(struct hid_device *hid,
 void hid_setup_resolution_multiplier(struct hid_device *hid);
 int hid_open_report(struct hid_device *device);
 int hid_check_keys_pressed(struct hid_device *hid);
+bool hid_is_usb_device(struct hid_device *hid);
 int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
 bool hid_match_one_id(const struct hid_device *hdev,

-- 
2.31.1

The Asus T101HA has a problem with spurious wakeups when the lid is
closed, this is caused by the screen sitting so close to the touchpad
that the touchpad ends up reporting touch events, causing these wakeups.

Add a quirk which disables event reporting on suspend when set, and
enable this quirk for the Asus T101HA touchpad fixing the spurious
wakeups, while still allowing the device to be woken by pressing a
key on the keyboard (which is part of the same USB device).

Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/hid-multitouch.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 9d9f3e1bd5f4..cfb68e443ddd 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -70,6 +70,7 @@ MODULE_LICENSE("GPL");
 #define MT_QUIRK_WIN8_PTP_BUTTONS	BIT(18)
 #define MT_QUIRK_SEPARATE_APP_REPORT	BIT(19)
 #define MT_QUIRK_FORCE_MULTI_INPUT	BIT(20)
+#define MT_QUIRK_DISABLE_WAKEUP		BIT(21)
 
 #define MT_INPUTMODE_TOUCHSCREEN	0x02
 #define MT_INPUTMODE_TOUCHPAD		0x03

@@ -191,6 +192,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
 #define MT_CLS_EXPORT_ALL_INPUTS		0x0013
 /* reserved					0x0014 */
 #define MT_CLS_WIN_8_FORCE_MULTI_INPUT		0x0015
+#define MT_CLS_WIN_8_DISABLE_WAKEUP		0x0016
 
 /* vendor specific classes */
 #define MT_CLS_3M				0x0101

@@ -283,6 +285,15 @@ static const struct mt_class mt_classes[] = {
 			MT_QUIRK_WIN8_PTP_BUTTONS |
 			MT_QUIRK_FORCE_MULTI_INPUT,
 		.export_all_inputs = true },
+	{ .name = MT_CLS_WIN_8_DISABLE_WAKEUP,
+		.quirks = MT_QUIRK_ALWAYS_VALID |
+			MT_QUIRK_IGNORE_DUPLICATES |
+			MT_QUIRK_HOVERING |
+			MT_QUIRK_CONTACT_CNT_ACCURATE |
+			MT_QUIRK_STICKY_FINGERS |
+			MT_QUIRK_WIN8_PTP_BUTTONS |
+			MT_QUIRK_DISABLE_WAKEUP,
+		.export_all_inputs = true },
 
 	/*
 	 * vendor specific classes

@@ -759,7 +770,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 			return 1;
 		case HID_DG_CONFIDENCE:
 			if ((cls->name == MT_CLS_WIN_8 ||
-			     cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT) &&
+			     cls->name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
+			     cls->name == MT_CLS_WIN_8_DISABLE_WAKEUP) &&
 				(field->application == HID_DG_TOUCHPAD ||
 				 field->application == HID_DG_TOUCHSCREEN))
 				app->quirks |= MT_QUIRK_CONFIDENCE;

@@ -1749,8 +1761,14 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 #ifdef CONFIG_PM
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 {
+	struct mt_device *td = hid_get_drvdata(hdev);
+
 	/* High latency is desirable for power savings during S3/S0ix */
-	mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
+	if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
+		mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
+	else
+		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
+
 	return 0;
 }
 

@@ -1809,6 +1827,12 @@ static const struct hid_device_id mt_devices[] = {
 		MT_USB_DEVICE(USB_VENDOR_ID_ANTON,
 			USB_DEVICE_ID_ANTON_TOUCH_PAD) },
 
+	/* Asus T101HA */
+	{ .driver_data = MT_CLS_WIN_8_DISABLE_WAKEUP,
+		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,
+			   USB_VENDOR_ID_ASUSTEK,
+			   USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
+
 	/* Asus T304UA */
 	{ .driver_data = MT_CLS_ASUS,
 		HID_DEVICE(BUS_USB, HID_GROUP_MULTITOUCH_WIN_8,

-- 
2.31.1

Disable event reporting on suspend when our parent is not
a wakeup-source. This should help save some extra power in
this case.

Signed-off-by: Hans de Goede <redacted>
---
Changes in v2:
- Use new hid_is_usb_device() helper
---
 drivers/hid/hid-multitouch.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index cfb68e443ddd..24b4a54f7224 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c

@@ -1759,12 +1759,33 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 }
 
 #ifdef CONFIG_PM
+
+/* Check if the parent which has the power/wakeup* sysfs attributes may wake the hdev */
+static bool mt_parent_may_wake(struct hid_device *hdev)
+{
+	struct device *parent = hdev->dev.parent;
+
+	/*
+	 * USB-HID is attached to the usb_interface (our parent), the
+	 * power/wakeup* attr are part of the usb-device which is its parent.
+	 */
+	if (hid_is_usb_device(hdev) && parent)
+		parent = parent->parent;
+
+	if (parent)
+		return device_may_wakeup(parent);
+
+	/* Huh? Play it safe and keep reporting events. */
+	return true;
+}
+
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
 
 	/* High latency is desirable for power savings during S3/S0ix */
-	if (td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP)
+	if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
+	    !mt_parent_may_wake(hdev))
 		mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
 	else
 		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);

-- 
2.31.1

There is no need to use a quirk and then return -ENODEV from the
asus_probe() function to avoid that hid-asus binds to the hiddev
for the USB-interface for the hid-multitouch touchpad.

The hid-multitouch hiddev has a group of HID_GROUP_MULTITOUCH_WIN_8,
so the same result can be achieved by making the hid_device_id entry
for the dock in the asus_devices[] table only match on HID_GROUP_GENERIC
instead of having it match HID_GROUP_ANY.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/hid-asus.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 2ab22b925941..6e24f48f211c 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c

@@ -79,10 +79,9 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
 #define QUIRK_T100_KEYBOARD		BIT(6)
 #define QUIRK_T100CHI			BIT(7)
 #define QUIRK_G752_KEYBOARD		BIT(8)
-#define QUIRK_T101HA_DOCK		BIT(9)
-#define QUIRK_T90CHI			BIT(10)
-#define QUIRK_MEDION_E1239T		BIT(11)
-#define QUIRK_ROG_NKEY_KEYBOARD		BIT(12)
+#define QUIRK_T90CHI			BIT(9)
+#define QUIRK_MEDION_E1239T		BIT(10)
+#define QUIRK_ROG_NKEY_KEYBOARD		BIT(11)
 
 #define I2C_KEYBOARD_QUIRKS			(QUIRK_FIX_NOTEBOOK_REPORT | \
 						 QUIRK_NO_INIT_REPORTS | \

@@ -1072,11 +1071,6 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return ret;
 	}
 
-	/* use hid-multitouch for T101HA touchpad */
-	if (id->driver_data & QUIRK_T101HA_DOCK &&
-	    hdev->collection->usage == HID_GD_MOUSE)
-		return -ENODEV;
-
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "Asus hw start failed: %d\n", ret);

@@ -1230,8 +1224,6 @@ static const struct hid_device_id asus_devices[] = {
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
 		USB_DEVICE_ID_ASUSTEK_T100TAF_KEYBOARD),
 	  QUIRK_T100_KEYBOARD | QUIRK_NO_CONSUMER_USAGES },
-	{ HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
-		USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD), QUIRK_T101HA_DOCK },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_ASUS_AK1D) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_TURBOX, USB_DEVICE_ID_ASUS_MD_5110) },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_JESS, USB_DEVICE_ID_ASUS_MD_5112) },

@@ -1239,6 +1231,12 @@ static const struct hid_device_id asus_devices[] = {
 		USB_DEVICE_ID_ASUSTEK_T100CHI_KEYBOARD), QUIRK_T100CHI },
 	{ HID_USB_DEVICE(USB_VENDOR_ID_ITE, USB_DEVICE_ID_ITE_MEDION_E1239T),
 		QUIRK_MEDION_E1239T },
+	/*
+	 * Note bind to the HID_GROUP_GENERIC group, so that we only bind to the keyboard
+	 * part, while letting hid-multitouch.c handle the touchpad.
+	 */
+	{ HID_DEVICE(BUS_USB, HID_GROUP_GENERIC,
+		USB_VENDOR_ID_ASUSTEK, USB_DEVICE_ID_ASUSTEK_T101HA_KEYBOARD) },
 	{ }
 };
 MODULE_DEVICE_TABLE(hid, asus_devices);

-- 
2.31.1

Switch to the new hid_is_usb_device() helper.

With this new helper building without USB_HID being enabled should work,
so also change the Kconfig depends on from USB_HID to plain HID.

Signed-off-by: Hans de Goede <redacted>
---
 drivers/hid/Kconfig    | 2 +-
 drivers/hid/hid-asus.c | 6 ++----
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 786b71ef7738..6b244e67c272 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig

@@ -149,7 +149,7 @@ config HID_APPLEIR
 
 config HID_ASUS
 	tristate "Asus"
-	depends on USB_HID
+	depends on HID
 	depends on LEDS_CLASS
 	depends on ASUS_WMI || ASUS_WMI=n
 	select POWER_SUPPLY

diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
index 6e24f48f211c..41bc34aaaf09 100644
--- a/drivers/hid/hid-asus.c
+++ b/drivers/hid/hid-asus.c

@@ -1009,8 +1009,7 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (drvdata->quirks & QUIRK_IS_MULTITOUCH)
 		drvdata->tp = &asus_i2c_tp;
 
-	if ((drvdata->quirks & QUIRK_T100_KEYBOARD) &&
-	    hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if ((drvdata->quirks & QUIRK_T100_KEYBOARD) && hid_is_usb_device(hdev)) {
 		struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
 
 		if (intf->altsetting->desc.bInterfaceNumber == T100_TPAD_INTF) {

@@ -1038,8 +1037,7 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		drvdata->tp = &asus_t100chi_tp;
 	}
 
-	if ((drvdata->quirks & QUIRK_MEDION_E1239T) &&
-	    hid_is_using_ll_driver(hdev, &usb_hid_driver)) {
+	if ((drvdata->quirks & QUIRK_MEDION_E1239T) && hid_is_usb_device(hdev)) {
 		struct usb_host_interface *alt =
 			to_usb_interface(hdev->dev.parent)->altsetting;
 

-- 
2.31.1

On Wed, 5 May 2021, Hans de Goede wrote:

Hi Jiri, Benjamin,

Here is v2 of what started out as a small series to fix spurious wakeups
on T101HA 2-in-1s.

This adds the discussed hid_is_usb_device() helper and uses that in:

"HID: multitouch: Disable event reporting on suspend when our parent is
not a wakeup-source"

To avoid needing to add a "depends on USB_HID" to hid-multitouch Kconfig
settings.

I've checked all other hid_is_using_ll_driver(hdev, &usb_hid_driver) callers
and the only one which can truely benefit from the new helper is the
hid-asus driver, which also deals with some I2C devices on some Asus hw.

All other drivers using hid_is_using_ll_driver(hdev, &usb_hid_driver)
are only for USB devices, so dropping the "depends on USB_HID" does not
make sense for them.

The one other driver which may benefit from the new hid_is_usb_device()
helper would be the Wacom driver which seems to also support I2C devices,
but that contains a lot of USB specific code, so I don't think we can
easily drop the "depends on USB_HID" there.

Even though this is a bit if a mixed-bag of patches, their are several
dependencies between them, so these should probably all go on a single
topic branch.

Now in for-5.13/upstream-fixes. Thanks,

-- 
Jiri Kosina
SUSE Labs

Hi,

On 5/26/21 12:38 PM, Jiri Kosina wrote:

On Wed, 5 May 2021, Hans de Goede wrote:

quoted

Hi Jiri, Benjamin,

Here is v2 of what started out as a small series to fix spurious wakeups
on T101HA 2-in-1s.

This adds the discussed hid_is_usb_device() helper and uses that in:

"HID: multitouch: Disable event reporting on suspend when our parent is
not a wakeup-source"

To avoid needing to add a "depends on USB_HID" to hid-multitouch Kconfig
settings.

I've checked all other hid_is_using_ll_driver(hdev, &usb_hid_driver) callers
and the only one which can truely benefit from the new helper is the
hid-asus driver, which also deals with some I2C devices on some Asus hw.

All other drivers using hid_is_using_ll_driver(hdev, &usb_hid_driver)
are only for USB devices, so dropping the "depends on USB_HID" does not
make sense for them.

The one other driver which may benefit from the new hid_is_usb_device()
helper would be the Wacom driver which seems to also support I2C devices,
but that contains a lot of USB specific code, so I don't think we can
easily drop the "depends on USB_HID" there.

Even though this is a bit if a mixed-bag of patches, their are several
dependencies between them, so these should probably all go on a single
topic branch.

Now in for-5.13/upstream-fixes. Thanks,

Thank you, it seems that in the process of dropping the patches which
you had already merged from v1 of this series; and replacing them with v2,
you completely dropped:

[PATCH v2 5/6] HID: asus: Cleanup Asus T101HA keyboard-dock handling

(which was also in v1) at least I cannot find this in either one of:

https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/drivers/hid/hid-asus.c?h=for-next
https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/drivers/hid/hid-asus.c?h=for-5.13/upstream-fixes

Regards,

Hans

On Wed, 26 May 2021, Hans de Goede wrote:

quoted

quoted

Hi Jiri, Benjamin,

Here is v2 of what started out as a small series to fix spurious wakeups
on T101HA 2-in-1s.

This adds the discussed hid_is_usb_device() helper and uses that in:

"HID: multitouch: Disable event reporting on suspend when our parent is
not a wakeup-source"

To avoid needing to add a "depends on USB_HID" to hid-multitouch Kconfig
settings.

I've checked all other hid_is_using_ll_driver(hdev, &usb_hid_driver) callers
and the only one which can truely benefit from the new helper is the
hid-asus driver, which also deals with some I2C devices on some Asus hw.

All other drivers using hid_is_using_ll_driver(hdev, &usb_hid_driver)
are only for USB devices, so dropping the "depends on USB_HID" does not
make sense for them.

The one other driver which may benefit from the new hid_is_usb_device()
helper would be the Wacom driver which seems to also support I2C devices,
but that contains a lot of USB specific code, so I don't think we can
easily drop the "depends on USB_HID" there.

Even though this is a bit if a mixed-bag of patches, their are several
dependencies between them, so these should probably all go on a single
topic branch.

Now in for-5.13/upstream-fixes. Thanks,

Thank you, it seems that in the process of dropping the patches which
you had already merged from v1 of this series; and replacing them with v2,
you completely dropped:

[PATCH v2 5/6] HID: asus: Cleanup Asus T101HA keyboard-dock handling

(which was also in v1) at least I cannot find this in either one of:

2 more quoted lines

It will appear there shortly, not the full state of my tree has been 
mirrored out yet.

Thanks a lot for double-checking!

-- 
Jiri Kosina
SUSE Labs