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

[Kai-Heng Feng]

> On Jun 19, 2020, at 17:56, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> 
> Hi,
> 
> On 6/19/20 6:16 AM, Kai-Heng Feng wrote:
>> Hi,
>>> On Jun 18, 2020, at 23:28, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>> 
>>> 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.
>> Sounds like a mechanical/hardware issue to me.
> 
> Nope, the laptop is pretty much in new state.

Ack.

> 
>> I've seen some old laptops have the same issue.
>> Swollen battery can push up the touchpad, makes it contact to touchscreen, and wakes up the system.
> 
> This is a 2-in-1 with a detachable keyboard, which is why the
> kbd/touchpad is a USB device rather then i2c-hid. Even if the
> battery were swollen this would push up the back cover of the
> tablet.

What's the behavior on Windows?
I wonder if Windows has a different wakeup policy?
Like disable remote wakeup for USB touchpad and touchscreen?


> 
>>> 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.
>> However, it's really handy to wake up the system from touchpad.
> 
> I agree this is somewhat handy, but the keyboard (space-bar) is
> usually sitting right next to it. So we can live without it,
> we really need to fix the spurious wakeup issue first, once
> that is fixed I'm fine with enabling wakeup by touchpad.

That's true. Spacebar is a good enough alternative.

> 
>>> 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.
>> But only under lid is closed?
> 
> Right.
> 
>> I wonder if it's okay to handle the case in s2idle_loop() or in userspace?
>> Lid close -> Wakeup event from touchpad -> Found the lid is closed
>> -> Turn off touchpad wakeup -> continue suspend.
> 
> I've discussed doing something about the spurious wakeup issue in
> the kernel with the the kernel input maintainer (Dmitry) here:
> 
> https://lore.kernel.org/linux-acpi/964ca07a-3da5-101f-7edf-64bdeec98a4b@xxxxxxxxxx/
> 
> He was quite clear that this must be fixed in userspace. We did
> come up with a plan for fixing this in userspace:
> 
> 1) Have udev-rules setting using hwdb for quirks which tags
> some input devices as "input-disable-wake-on-closed-lid".
> A simple udev rule could tag all i2c-hid touchpads with this and
> for the detachable USB keyboards with builtin touchpad some
> 2-in-1s have we can then use quirks in hwdb to set the tag
> on those.

Maybe we can avoid using quirks in hwdb, external USB devices should have "Removable" bit set (i.e. it's external).

> 
> 2) Teach systemd-logind which does the suspend-on-lid on modern
> GNOME3 based systems to disable wakeup on the parent of
> input-devices which have this tag set before suspending.
> 
> As mentioned the kernel can then also use this to save
> some power by disable scanning for fingers on suspend.
> 
> If you have time to work on these 2 items, that would be
> great. Once this is in place I'm fine with the suggested
> kernel change.

Ok, let me investigate a bit more.

> 
> ###
> 
> Semi-off-topic:
> 
> The thread I linked to above is about adding a new inhibit
> feature to the input system, which is intended to allow
> telling the input system system to stop listening for
> events even if userspace has the device open (or it has
> in kernel listeners) once this has landed, we can use
> the same udev-tag to also teach systemd-logind to inhibit
> e.g. the touchpad when the lid is closed but the system is
> not suspending (e.g. external monitor connected).
> Combined with some extra hid-multitouch changes this will
> again allow us to tell the touchpad to stop scanning
> for fingers saving some power.
> 
> The inhibit feature could likewise also be enabled on
> internal touchpads and keyboards when a 360 degree (yoga)
> style 2-in-1 is in tablet mode to avoid accidental key-pressed /
> touchpad touches.

I thought disabling keyboard/touchpad in tablet mode is done by system firmware.
Is it different now?

How does the kernel know it's in tablet mode?

> 
> Note the inhibit when in tablet mode thing would require
> a new: "input-inhibit-on-tablet-mode" tag. At first I
> was thinking to just have an "input-device-is-internal"
> tag, but that would e.g. also apply to the touchscreen,
> on which we do want to disable wakeup when the lid is
> closed, but not when in tablet mode.
> 
> Hmm, I guess to prepare for the inhibit stuff we should
> probably call the other tag "input-inhibit-on-closed-lid"
> rather then "input-disable-wake-on-closed-lid", and then
> systemd-logind can defer that wit should also disable wake
> (or initially only disable wake) from that. Otherwise we
> get 4 possible tags and I don't see a usecase where we
> want to inhibit but not also disable wakeup.

I'll focus on the clamshell case for now, I don't have 2-in-1 in hand right now.

> 
>>> 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.
>> I tried touch and i2c-hid touchscreen and it doesn't wake up the system.
> 
> I guess the :
> 
>        i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
> 
> Call from i2c_hid_suspend() causes that, interesting that that
> works for touchscreens but not for touchpads.
> 
> I'm pretty sure that if you comment out that line, your
> patch will cause wake-ups on touchscreens too, which
> IMHO means that your patch should maybe move the above
> call into the else of the:
> 
>        if (device_may_wakeup(&client->dev)) {
> 
> block, if we enable wakeup then it should work. This should
> probably be combined with being smarter about which devices
> to enable wakeup on by default...

Comment out that line doesn't make touchscreen have the ability to wake up the system.

My guess is that there's no ACPI GPIO connects to the touchscreen.

> 
>> However we should still handle the two different cases, probably differentiate touchpad and touchscreen in hid-multitouch.
> 
> Ack, see above.
> 
>>> 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.
>> IIRC, touchpad and touchscreen connect to different parents on all laptops I worked on.
>> So I think it's possible to enable mode 2 for touchpad, and mode 3 for touchscreen.
> 
> Ack.
> 
>> Touchpad wake is really handy, let's figure out how to enable it while covering all potential regression risks.
> 
> See above I believe we should first get the userspace bits to disable it
> when the lid is closed in place.  And even then we may need to have
> a Kconfig option to disable it for people running an older userspace,
> but I guess once the userspace bits are there, we can proceed without
> the Kconfig option and then add that later if necessary (if people are
> seeing regressions).

For now, as a comprise, can we still enable the wake up capability but disable it by default?
i.e. "device_init_wakeup(dev, false)" so user can still choose to enable touchpad wakeup at their own discretion.

Kai-Heng

> 
> Regards,
> 
> Hans
> 
> 
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>>>> ---
>>>> 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 */