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.
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.
>
> 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.
>
> 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?
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.
>
> 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.
However we should still handle the two different cases, probably differentiate touchpad and touchscreen in hid-multitouch.
>
> 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.
Touchpad wake is really handy, let's figure out how to enable it while covering all potential regression risks.
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 */
>
