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

[Hans de Goede]

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.

> 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.

> > 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.

> > 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.

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.

###

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.

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.

> > 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...

> 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).

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 */