linux-kernel - Re: [PATCH v3] HID: i2c-hid: Enable touchpad wakeup from Suspend-to-Idle

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Jun 2020 17:28:19 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>, jikos@...nel.org,
        benjamin.tissoires@...hat.com
Cc:     You-Sheng Yang <vicamo.yang@...onical.com>,
        Daniel Playfair Cal <daniel.playfair.cal@...il.com>,
        HungNien Chen <hn.chen@...dahitech.com>,
        Pavel Balan <admin@...ma.net>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] HID: i2c-hid: Enable touchpad wakeup from
 Suspend-to-Idle

Hi,

On 6/18/20 4:55 PM, Kai-Heng Feng wrote:
> Many laptops can be woken up from Suspend-to-Idle by touchpad. This is
> also the default behavior on other OSes.
> 
> So let's enable the wakeup support if the system defaults to
> Suspend-to-Idle.

I have been debugging a spurious wakeup issue on an Asus T101HA,
where the system would not stay suspended when the lid was closed.

The issue turns out to be that the touchpad is generating touch
events when the lid/display gets close to the touchpad. In this case
wakeup is already enabled by default because it is an USB device.

So I do not believe that this is a good idea, most current devices
with a HID multi-touch touchpad use i2c-hid and also use S2idle,
so this will basically enable wakeup by touchpad on almost all
current devices.

There will likely be other devices with the same issue as the T101HA,
but currently we are not seeing this issue because by default i2c-hid
devices do not have wakeup enabled. This change will thus likely cause
new spurious wakeup issues on more devices. So this seems like a
bad idea.

Also your commit message mentions touchpads, but the change
will also enable wakeup on most touchscreens out there, meaning
that just picking up a device in tablet mode and accidentally
touching the screen will wake it up.

Also hid multi-touch devices have 3 modes, see the diagrams
in Microsoft hw design guides for win8/10 touch devices:
1. Reporting events with low latency (high power mode)
2. Reporting events with high latency (lower power mode)
3. Not reporting events (lowest power mode)

I actually still need to write some patches for hid-multitouch.c
to set the mode to 2 or 3 on suspend depending on the device_may_wakeup
setting of the parent. Once that patch is written, it should
put most i2c-hid mt devices in mode 3, hopefully also helping
with Linux' relative high power consumption when a device is
suspended. With your change instead my to-be-written patch
would put the device in mode 2, which would still be an
improvement but less so.

Regards,

Hans

> 
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> ---
> v3:
>   - Use device_init_wakeup().
>   - Wording change.
> 
> v2:
>   - Fix compile error when ACPI is not enabled.
>   drivers/hid/i2c-hid/i2c-hid-core.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 294c84e136d7..dae1d072daf6 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -931,6 +931,12 @@ static void i2c_hid_acpi_fix_up_power(struct device *dev)
>   		acpi_device_fix_up_power(adev);
>   }
>   
> +static void i2c_hid_acpi_enable_wakeup(struct device *dev)
> +{
> +	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0)
> +		device_init_wakeup(dev, true);
> +}
> +
>   static const struct acpi_device_id i2c_hid_acpi_match[] = {
>   	{"ACPI0C50", 0 },
>   	{"PNP0C50", 0 },
> @@ -945,6 +951,8 @@ static inline int i2c_hid_acpi_pdata(struct i2c_client *client,
>   }
>   
>   static inline void i2c_hid_acpi_fix_up_power(struct device *dev) {}
> +
> +static inline void i2c_hid_acpi_enable_wakeup(struct device *dev) {}
>   #endif
>   
>   #ifdef CONFIG_OF
> @@ -1072,6 +1080,8 @@ static int i2c_hid_probe(struct i2c_client *client,
>   
>   	i2c_hid_acpi_fix_up_power(&client->dev);
>   
> +	i2c_hid_acpi_enable_wakeup(&client->dev);
> +
>   	device_enable_async_suspend(&client->dev);
>   
>   	/* Make sure there is something at this address */
>