[PATCH 0/4] HID: multitouch: Disable event reporting on suspend when the device is not a wakeup-source

[PATCH 0/4] HID: multitouch: Disable event reporting on suspend when the device is not a wakeup-source

[Hans de Goede]

Hi Jiri, Benjamin,

As discussed here is a complete rewrite of my serious to have
hid-multitouch disable touch and button-press reporting on hid-mt
devices during suspend when the device is not configured as
a wakeup-source.

Regards,

Hans

Hans de Goede (4):
  HID: core: Add hid_hw_may_wakeup() function
  HID: usbhid: Implement may_wakeup ll-driver callback
  HID: logitech-dj: Implement may_wakeup ll-driver callback
  HID: multitouch: Disable event reporting on suspend when the device is
    not a wakeup-source

 drivers/hid/hid-logitech-dj.c |  8 ++++++++
 drivers/hid/hid-multitouch.c  |  3 ++-
 drivers/hid/usbhid/hid-core.c |  8 ++++++++
 include/linux/hid.h           | 18 ++++++++++++++++++
 4 files changed, 36 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/4] HID: core: Add hid_hw_may_wakeup() function

[Hans de Goede]

Add a hid_hw_may_wakeup() function, which is the equivalent of
hid_hw_may_wakeup() for hid devices.

In most cases this just returns device_may_wakeup(hdev->dev.parent),
but for some ll-drivers this is not correct. E.g. usb_hid_driver
instantiated hid devices have their parent set to the usb-interface
to which the usb_hid_driver is bound, but the power/wakeup* sysfs
attributes are part of the usb-device, which is the usb-interface's
parent.

For these special cases a new may_wakeup callback is added to
hid_ll_driver, so that ll-drivers can override the default behavior.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 include/linux/hid.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index 271021e20a3f..4d2b2212f079 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -800,6 +800,7 @@ struct hid_driver {
  * @raw_request: send raw report request to device (e.g. feature report)
  * @output_report: send output report to device
  * @idle: send idle request to device
+ * @may_wakeup: return if device may act as a wakeup source during system-suspend
  */
 struct hid_ll_driver {
 	int (*start)(struct hid_device *hdev);
@@ -824,6 +825,7 @@ struct hid_ll_driver {
 	int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len);
 
 	int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
+	bool (*may_wakeup)(struct hid_device *hdev);
 };
 
 extern struct hid_ll_driver i2c_hid_ll_driver;
@@ -1149,6 +1151,22 @@ static inline int hid_hw_idle(struct hid_device *hdev, int report, int idle,
 	return 0;
 }
 
+/**
+ * hid_may_wakeup - return if the hid device may act as a wakeup source during system-suspend
+ *
+ * @hdev: hid device
+ */
+static inline bool hid_hw_may_wakeup(struct hid_device *hdev)
+{
+	if (hdev->ll_driver->may_wakeup)
+		return hdev->ll_driver->may_wakeup(hdev);
+
+	if (hdev->dev.parent)
+		return device_may_wakeup(hdev->dev.parent);
+
+	return false;
+}
+
 /**
  * hid_hw_wait - wait for buffered io to complete
  *
-- 
2.31.1

Re: [PATCH 1/4] HID: core: Add hid_hw_may_wakeup() function

[Benjamin Tissoires]

Hi Hans,

On 5/29/21 5:14 PM, Hans de Goede wrote:
> Add a hid_hw_may_wakeup() function, which is the equivalent of
> hid_hw_may_wakeup() for hid devices.

nitpick here, but I think the second `hid_hw_may_wakeup()` is probably 
wrong.

> 
> In most cases this just returns device_may_wakeup(hdev->dev.parent),
> but for some ll-drivers this is not correct. E.g. usb_hid_driver
> instantiated hid devices have their parent set to the usb-interface
> to which the usb_hid_driver is bound, but the power/wakeup* sysfs
> attributes are part of the usb-device, which is the usb-interface's
> parent.

I never spent enough time in suspend/resume. But isn't the big consumer 
of wakeup sources be USB? Is there a point having a generic return path 
when returning false earlier will keep the same code path for all other 
transport layers (I2C and bluetooth)?

Other than those two questions, I like the series and the approach to 
not break the existing:

For the series:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

> 
> For these special cases a new may_wakeup callback is added to
> hid_ll_driver, so that ll-drivers can override the default behavior.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>   include/linux/hid.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 271021e20a3f..4d2b2212f079 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -800,6 +800,7 @@ struct hid_driver {
>    * @raw_request: send raw report request to device (e.g. feature report)
>    * @output_report: send output report to device
>    * @idle: send idle request to device
> + * @may_wakeup: return if device may act as a wakeup source during system-suspend
>    */
>   struct hid_ll_driver {
>   	int (*start)(struct hid_device *hdev);
> @@ -824,6 +825,7 @@ struct hid_ll_driver {
>   	int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len);
>   
>   	int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
> +	bool (*may_wakeup)(struct hid_device *hdev);
>   };
>   
>   extern struct hid_ll_driver i2c_hid_ll_driver;
> @@ -1149,6 +1151,22 @@ static inline int hid_hw_idle(struct hid_device *hdev, int report, int idle,
>   	return 0;
>   }
>   
> +/**
> + * hid_may_wakeup - return if the hid device may act as a wakeup source during system-suspend
> + *
> + * @hdev: hid device
> + */
> +static inline bool hid_hw_may_wakeup(struct hid_device *hdev)
> +{
> +	if (hdev->ll_driver->may_wakeup)
> +		return hdev->ll_driver->may_wakeup(hdev);
> +
> +	if (hdev->dev.parent)
> +		return device_may_wakeup(hdev->dev.parent);
> +
> +	return false;
> +}
> +
>   /**
>    * hid_hw_wait - wait for buffered io to complete
>    *
> 

Re: [PATCH 1/4] HID: core: Add hid_hw_may_wakeup() function

[Hans de Goede]

Hi,

On 6/24/21 2:16 PM, Benjamin Tissoires wrote:
> Hi Hans,
> 
> On 5/29/21 5:14 PM, Hans de Goede wrote:
>> Add a hid_hw_may_wakeup() function, which is the equivalent of
>> hid_hw_may_wakeup() for hid devices.
> 
> nitpick here, but I think the second `hid_hw_may_wakeup()` is probably wrong.

Right, the second `hid_hw_may_wakeup()` here should be
`device_may_wakeup()`. Jiri can you fix this up while applying
or do you want a new version ?

>> In most cases this just returns device_may_wakeup(hdev->dev.parent),
>> but for some ll-drivers this is not correct. E.g. usb_hid_driver
>> instantiated hid devices have their parent set to the usb-interface
>> to which the usb_hid_driver is bound, but the power/wakeup* sysfs
>> attributes are part of the usb-device, which is the usb-interface's
>> parent.
> 
> I never spent enough time in suspend/resume. But isn't the big consumer of wakeup sources be USB? Is there a point having a generic return path when returning false earlier will keep the same code path for all other transport layers (I2C and bluetooth)?

I can see why you primarily associate wakeup-sources with USB, but
pretty much any device can be a wakeup-source, e.g. embedded-controllers
typically are wakeup sources, network-cards can be wakeup-sources, etc.

One relevant HID example would be an I2C-HID attached keyboard which is
configured to wake the system from suspend on a keypress.

At least the Toshiba Click Mini L9W-B is using an I2C-HID keyboard and
I'm sure there are others.

So it seems best here to not make any assumptions and in cases where
the device which is expected to have the power/wakeup* sysfs attributes
is simply the direct parent of the hid-device call device_may_wakeup()
on hdev->dev.parent. Note that in many cases this will indeed simply
return false, which is fine. But in e.g. the i2c-hid attached keyboard
it might return true.

Regards,

Hans



> Other than those two questions, I like the series and the approach to not break the existing:
> 
> For the series:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> Cheers,
> Benjamin
> 
>>
>> For these special cases a new may_wakeup callback is added to
>> hid_ll_driver, so that ll-drivers can override the default behavior.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   include/linux/hid.h | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 271021e20a3f..4d2b2212f079 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -800,6 +800,7 @@ struct hid_driver {
>>    * @raw_request: send raw report request to device (e.g. feature report)
>>    * @output_report: send output report to device
>>    * @idle: send idle request to device
>> + * @may_wakeup: return if device may act as a wakeup source during system-suspend
>>    */
>>   struct hid_ll_driver {
>>       int (*start)(struct hid_device *hdev);
>> @@ -824,6 +825,7 @@ struct hid_ll_driver {
>>       int (*output_report) (struct hid_device *hdev, __u8 *buf, size_t len);
>>         int (*idle)(struct hid_device *hdev, int report, int idle, int reqtype);
>> +    bool (*may_wakeup)(struct hid_device *hdev);
>>   };
>>     extern struct hid_ll_driver i2c_hid_ll_driver;
>> @@ -1149,6 +1151,22 @@ static inline int hid_hw_idle(struct hid_device *hdev, int report, int idle,
>>       return 0;
>>   }
>>   +/**
>> + * hid_may_wakeup - return if the hid device may act as a wakeup source during system-suspend
>> + *
>> + * @hdev: hid device
>> + */
>> +static inline bool hid_hw_may_wakeup(struct hid_device *hdev)
>> +{
>> +    if (hdev->ll_driver->may_wakeup)
>> +        return hdev->ll_driver->may_wakeup(hdev);
>> +
>> +    if (hdev->dev.parent)
>> +        return device_may_wakeup(hdev->dev.parent);
>> +
>> +    return false;
>> +}
>> +
>>   /**
>>    * hid_hw_wait - wait for buffered io to complete
>>    *
>>
> 

Re: [PATCH 1/4] HID: core: Add hid_hw_may_wakeup() function

[Jiri Kosina]

On Thu, 24 Jun 2021, Hans de Goede wrote:

> >> Add a hid_hw_may_wakeup() function, which is the equivalent of
> >> hid_hw_may_wakeup() for hid devices.
> > 
> > nitpick here, but I think the second `hid_hw_may_wakeup()` is probably wrong.
> 
> Right, the second `hid_hw_may_wakeup()` here should be
> `device_may_wakeup()`. Jiri can you fix this up while applying
> or do you want a new version ?

No worries, I have fixed that up and applied. Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH 2/4] HID: usbhid: Implement may_wakeup ll-driver callback

[Hans de Goede]

Without a ll-driver callback hid_hw_may_wakeup() will return:
device_may_wakeup(hdev->dev.parent), usb_hid_driver instantiated hid
devices have their parent set to the usb-interface to which the
usb_hid_driver is bound, but the power/wakeup* sysfs attributes are
part of the usb-device.

Add a may_wakeup ll-driver callback which calls device_may_wakeup()
on the usb-device instead.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/usbhid/hid-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 86257ce6d619..3d792c6f1b1a 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1304,6 +1304,13 @@ static int usbhid_idle(struct hid_device *hid, int report, int idle,
 	return hid_set_idle(dev, ifnum, report, idle);
 }
 
+static bool usbhid_may_wakeup(struct hid_device *hid)
+{
+	struct usb_device *dev = hid_to_usb_dev(hid);
+
+	return device_may_wakeup(&dev->dev);
+}
+
 struct hid_ll_driver usb_hid_driver = {
 	.parse = usbhid_parse,
 	.start = usbhid_start,
@@ -1316,6 +1323,7 @@ struct hid_ll_driver usb_hid_driver = {
 	.raw_request = usbhid_raw_request,
 	.output_report = usbhid_output_report,
 	.idle = usbhid_idle,
+	.may_wakeup = usbhid_may_wakeup,
 };
 EXPORT_SYMBOL_GPL(usb_hid_driver);
 
-- 
2.31.1

[PATCH 3/4] HID: logitech-dj: Implement may_wakeup ll-driver callback

[Hans de Goede]

Without a ll-driver callback hid_hw_may_wakeup() will return:
device_may_wakeup(hdev->dev.parent), but for the hid devices
instantiated by logitech-dj for devices behind the receiver the
logitech-dj hid(pp)-device is the parent.

Add a logi_dj_ll_may_wakeup() callback which calls hid_hw_may_wakeup()
on the logitech-dj hid(pp) parent-hid-device.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-logitech-dj.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 18d37b3765f3..a0017b010c34 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1497,6 +1497,13 @@ static void logi_dj_ll_stop(struct hid_device *hid)
 	dbg_hid("%s\n", __func__);
 }
 
+static bool logi_dj_ll_may_wakeup(struct hid_device *hid)
+{
+	struct dj_device *djdev = hid->driver_data;
+	struct dj_receiver_dev *djrcv_dev = djdev->dj_receiver_dev;
+
+	return hid_hw_may_wakeup(djrcv_dev->hidpp);
+}
 
 static struct hid_ll_driver logi_dj_ll_driver = {
 	.parse = logi_dj_ll_parse,
@@ -1505,6 +1512,7 @@ static struct hid_ll_driver logi_dj_ll_driver = {
 	.open = logi_dj_ll_open,
 	.close = logi_dj_ll_close,
 	.raw_request = logi_dj_ll_raw_request,
+	.may_wakeup = logi_dj_ll_may_wakeup,
 };
 
 static int logi_dj_dj_event(struct hid_device *hdev,
-- 
2.31.1

[PATCH 4/4] HID: multitouch: Disable event reporting on suspend when the device is not a wakeup-source

[Hans de Goede]

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hid/hid-multitouch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index cfb68e443ddd..c5a7523f97ac 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1764,7 +1764,8 @@ 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) ||
+	    !hid_hw_may_wakeup(hdev))
 		mt_set_modes(hdev, HID_LATENCY_HIGH, false, false);
 	else
 		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
-- 
2.31.1