Hi, > On Jul 3, 2020, at 20:27, Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 7/1/20 8:46 AM, Kai-Heng Feng wrote: >>> 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? > > I'm afraid I no longer have Windows on the device, so I cannot > test this. I guess that Windows disables the builtin touchpad and > touchscreenn when the lid is closed. That is sensible to do from > a power-management pov even when not suspending, but just closing > the lid and using an external monitor + kbd. Ok. This seems worth further investigation. > >>>>> 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). > > At the USB level there only is a somewhat reliable "removal" bit > for storage devices, defined at the storage protocol level. > > For other devices there is: > > cat /sys/bus/usb/devices/1-8/removable > > This comes from the ACPI tables parsed by: > drivers/usb/core/usb-acpi.c > > Which set the port's connect_type variable. Actually most laptops I've used correctly marked internally connected USB 2 device with Removable bit. And that's the reason why USB2 LPM, which only enables when Removable == 1, caused many issues for us. > > I've tested this on a bunch of 2-in-1s with detachable USB > keyboards and it is almost completely random and not at > all related to the reality (both for the kbd-dock connector > and for other USB ports) this info is so unreliable it is > pretty much useless. Ok, that means USB2 LPM will be inadvertently enabled for those devices... Doesn't sound right. > > And the kbd-dock is special anyways, the dock is removable, > but when docked / the 2-in-1 is in laptop mode, the touchpad > should be treated as an internal touchpad, as it will be > covered by the display when the lid is closed. > > Note I'm mostly referring to 2-in-1's which turn into normal > clamshell laptops when docked into their keyboard dock, > not those with the flimsy type-covers. > > Good example's of the type I mean are the Asus T100 series > and the Lenovo Miix 300 / 310 / 320. Ok, good to know the difference. > >>> 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? > > It is in some cases but not always. > >> How does the kernel know it's in tablet mode? > > There are various interfaces for this. I've recently submitted > a series of patches upstream adding an input dev with SW_TABLET > functionality to most 2-in-1s: > > -I've done a bunch of fixes making the intel-vbtn code export > SW_TABLET on more devices > > -I've added support for the INT33D3 ACPI device to > the soc_button_array driver, this device offers direct access > to a GPIO for SW_TABLET mode (when it has a non empty resource list) > > -I've added SW_TABLET support for HP devices using HP's custom WMI > method for this to the hp_wmi driver. Thanks for implementing this. Seems like more and more devices will use software-based implementation for disabling input device. > > >>> 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. > > Ok, for the clamshell case it should be easy to have a few' > simple universal (based on the bus/connection type of the touchpad) > udev rules to tag them as "input-disable-wake-on-closed-lid". > > Because of the Bay/Cherry Trail hw-enablement I've been doing as > a personal side project I have quite a few 2-in-1s, so I can > build on top of that adding a hwdb template + some initial > entries for tagging the touchpads in detachable keyboards the > same way. Ack. > > >>> >>>>> 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. > > But there is, i2c-hid devices always have an interrupt line, > possibly the touchpad gets powered of on transition to D3 though. > > Yes that is probably it, many touchscreens have a _PS3 method > like this: > > Method (_PS3, 0, Serialized) // _PS3: Power State 3 > { > If ((^^^I2C7.AVBL == One)) > { > DATA = 0x1C > ^^^I2C7.DL13 = BUFF /* \_SB_.PCI0.I2C6.TCS0.BUFF */ > } > } > > Which tells the PMIC to turn of the LDO supplying the touchscreen, > so that means no wake from suspend by the touchscreen. > > I'm pretty sure you will find something similar in the ACPI > node describing the touchscreen on the device you are testing > with. > > If the ACPI tables do not explicitly turn off the device on D3, > then it would be good to disable wake, and it cannot hurt on > devices which do completely power down the touchscreen. So > extending the "input-disable-wake-on-closed-lid" stuff to the > touchscreen is probably a good idea. But for starters lets just > focus on touchpads. Ok, thanks for the explanation! > >>>> 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. > > Yes that sounds reasonable. Thanks, will send a new revision. Kai-Heng > > Regards, > > Hans > > > >> 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 */
