[PATCH 0/3] Fix spurious wakes on ACPI platforms

[PATCH 0/3] Fix spurious wakes on ACPI platforms

[Raul E Rangel]

d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake") gave the kernel
the ability to control the GPIO wake bit on AMD platforms. This
uncovered various drivers that were manually managing their wake
capabilities. This is problematic on ACPI systems because the ACPI
subsystem is responsible for managing the wake capabilities for the
device. ACPI devices need to define the `_PRW` property that defines the
GPE or GPIO that will be used to wake the system, and also the power
resources that must be enabled for the device to issue a wake.

The following real world example shows the problem:
* We have an ACPI HID device that has a power resource defined in `_PR0`
  and `_PR3`. It doesn't have `_PRW` so that means the device can't wake
  the system.
* The IRQ line is active level low for this device and is pulled up by the
  power resource defined in `_PR0`/`_PR3`.

The i2c-hid driver manually sets the device as wake capable, and the wake
flag gets set on the IRQ GPIO when entering suspend. As part of suspend,
ACPI will turn off the devices power resources since the device isn't a
wake source. This immediately triggers a wake because the IRQ line is
now low.

For devices that are marked as being wake capable (via `_PRW`), they
might use GPEs for wakes, while others might use the GPIO controller
(via _AEI). We need to respect the firmware configuration so we don't
have both the GPE and GPIO triggering a wake.

So when using ACPI, the correct thing to do is remove the driver from
managing the wake configuration. The ACPI subsystem has more knowledge of
the platform topology than the driver does.

This patch series fixes a few drivers that I noticed were incorrectly
setting the GPIO wake bit on my Guybrush Chromebook. I'm sure there will
be more.

I will be OOO until the New Year, so my responses will be delayed.


Raul E Rangel (3):
  HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable
  Input: elan_i2c - Use PM subsystem to manage wake irq
  platform/chrome: cros_ec: Don't enable wake pin if ACPI managed

 drivers/hid/i2c-hid/i2c-hid-acpi.c          |  5 -----
 drivers/input/mouse/elan_i2c_core.c         | 21 +++++++--------------
 drivers/platform/chrome/cros_ec.c           |  8 ++++++--
 include/linux/platform_data/cros_ec_proto.h |  1 -
 4 files changed, 13 insertions(+), 22 deletions(-)

-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

[Raul E Rangel]

The ACPI subsystem is responsible for managing the power and wake
sources for an ACPI device. By explicitly calling
device_set_wakeup_capable, we are circumvent the ACPI subsystem and
setting wake capabilities on the device when it doesn't support it.

Take the following example:
* We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
`_PRW` so that means the device can't wake the system.
* The IRQ line is active level low for this device and is pulled up by the
power resource defined in `_PR0`/`_PR3`.

Since the i2c-hid driver has set the device as wake capable, the wake
pin gets enabled on suspend. As part of suspend, ACPI will power down
the device since it's not a wake source. When the device is powered
down, the IRQ line will drop, and it will trigger a wake event.

See the following debug log:
[   42.335804] PM: Suspending system (s2idle)
[   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
[   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
[   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
[   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
[   42.535293] PM: Wakeup unrelated to ACPI SCI
[   42.535294] PM: resume from suspend-to-idle

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index a6f0257a26de..fc311a19a19d 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
 
 	acpi_device_fix_up_power(adev);
 
-	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
-		device_set_wakeup_capable(dev, true);
-		device_set_wakeup_enable(dev, false);
-	}
-
 	return i2c_hid_core_probe(client, &ihid_acpi->ops,
 				  hid_descriptor_address);
 }
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

[Hans de Goede]

Hi,

On 12/21/21 00:43, Raul E Rangel wrote:
> The ACPI subsystem is responsible for managing the power and wake
> sources for an ACPI device. By explicitly calling
> device_set_wakeup_capable, we are circumvent the ACPI subsystem and
> setting wake capabilities on the device when it doesn't support it.
> 
> Take the following example:
> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> `_PRW` so that means the device can't wake the system.
> * The IRQ line is active level low for this device and is pulled up by the
> power resource defined in `_PR0`/`_PR3`.
> 
> Since the i2c-hid driver has set the device as wake capable, the wake
> pin gets enabled on suspend.

The IRQ pin should only have a enable_irq_wake() called on it if
something has actually requested the i2c-HID device to be a wakeup source,
the removed code claims the device is wakeup *capable*, but is also
explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup.

And i2c-hid suspend does:

        if (device_may_wakeup(&client->dev)) {
                wake_status = enable_irq_wake(client->irq);

And device_may_wakeup() checks the wakeup *enabled* setting AFAIK.

I've added Rafael to the Cc since he knows all this a lot better then me.

I have the feeling that your userspace is perhaps poking the
wakeup settings in sysfs, triggering this issue.

> As part of suspend, ACPI will power down
> the device since it's not a wake source. When the device is powered
> down, the IRQ line will drop, and it will trigger a wake event.

To me that sounds like the device is not wakeup *capable* at all, so
its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all.

Note I'm not certain about this at all, but at a first look this feels
like it is not the right fix for your problem.

Regards,

Hans


> 
> See the following debug log:
> [   42.335804] PM: Suspending system (s2idle)
> [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> [   42.535293] PM: Wakeup unrelated to ACPI SCI
> [   42.535294] PM: resume from suspend-to-idle
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> 
>  drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> index a6f0257a26de..fc311a19a19d 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>  
>  	acpi_device_fix_up_power(adev);
>  
> -	if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> -		device_set_wakeup_capable(dev, true);
> -		device_set_wakeup_enable(dev, false);
> -	}
> -
>  	return i2c_hid_core_probe(client, &ihid_acpi->ops,
>  				  hid_descriptor_address);
>  }
> 

Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

[Raul Rangel]

On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/21/21 00:43, Raul E Rangel wrote:
> > The ACPI subsystem is responsible for managing the power and wake
> > sources for an ACPI device. By explicitly calling
> > device_set_wakeup_capable, we are circumvent the ACPI subsystem and
> > setting wake capabilities on the device when it doesn't support it.
> >
> > Take the following example:
> > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> > `_PRW` so that means the device can't wake the system.
> > * The IRQ line is active level low for this device and is pulled up by the
> > power resource defined in `_PR0`/`_PR3`.
> >
> > Since the i2c-hid driver has set the device as wake capable, the wake
> > pin gets enabled on suspend.
>
> The IRQ pin should only have a enable_irq_wake() called on it if
> something has actually requested the i2c-HID device to be a wakeup source,
> the removed code claims the device is wakeup *capable*, but is also
> explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup.
>
> And i2c-hid suspend does:
>
>         if (device_may_wakeup(&client->dev)) {
>                 wake_status = enable_irq_wake(client->irq);
>
> And device_may_wakeup() checks the wakeup *enabled* setting AFAIK.
>
> I've added Rafael to the Cc since he knows all this a lot better then me.
>
> I have the feeling that your userspace is perhaps poking the
> wakeup settings in sysfs, triggering this issue.

You are correct, I added some printks in and it is userspace enabling the wake:

    [    3.280464] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: disabled
    [    3.280502] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: enabled
    [    3.280537] i2c_hid_acpi i2c-GDIX0000:00: device_wakeup_enable: start
    [    3.280541] CPU: 0 PID: 1248 Comm: powerd Not tainted 5.10.83
#151 c334d4c4185a84ded39aafcb495de6870a8e5161
    [    3.280545] Hardware name: Google Guybrush/Guybrush, BIOS
Google_Guybrush.4.15-624-g9d80a9c6aa40 12/21/2021
    [    3.280548] Call Trace:
    [    3.280554]  dump_stack+0x9c/0xe7
    [    3.280560]  device_wakeup_enable+0x136/0x172
    [    3.280564]  wakeup_store+0xbc/0xc4
    [    3.280572]  kernfs_fop_write_iter+0x10b/0x18a
    [    3.280576]  vfs_write+0x383/0x405
    [    3.280579]  ksys_write+0x74/0xd4
    [    3.280583]  do_syscall_64+0x43/0x55
    [    3.280587]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

>
> > As part of suspend, ACPI will power down
> > the device since it's not a wake source. When the device is powered
> > down, the IRQ line will drop, and it will trigger a wake event.
>
> To me that sounds like the device is not wakeup *capable* at all, so
> its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all.

The ACPI_FADT_LOW_POWER_S0 flag is a system level flag. The
system is wake capable and supports S0ix. The touchscreen device
does not support waking the system because it doesn't provide
a `_PRW`.

>
> Note I'm not certain about this at all, but at a first look this feels
> like it is not the right fix for your problem.

We can't have the `i2c-hid-acpi` driver calling `device_set_wakeup_capable`.
This will make the kernel expose the wakeup sysfs entry to userspace
regardless if the device supports wakeup or not. This would require userspace
to know that enabling this wake source will cause suspend problems and to
avoid it.

>
> Regards,
>
> Hans

Thanks for the review!

Raul

>
>
> >
> > See the following debug log:
> > [   42.335804] PM: Suspending system (s2idle)
> > [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> > [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> > [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> > [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> > [   42.535293] PM: Wakeup unrelated to ACPI SCI
> > [   42.535294] PM: resume from suspend-to-idle
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> >  drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > index a6f0257a26de..fc311a19a19d 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
> >
> >       acpi_device_fix_up_power(adev);
> >
> > -     if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> > -             device_set_wakeup_capable(dev, true);
> > -             device_set_wakeup_enable(dev, false);
> > -     }
> > -
> >       return i2c_hid_core_probe(client, &ihid_acpi->ops,
> >                                 hid_descriptor_address);
> >  }
> >
>

Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

[Hans de Goede]

Hi,

On 12/22/21 00:40, Raul Rangel wrote:
> On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/21/21 00:43, Raul E Rangel wrote:
>>> The ACPI subsystem is responsible for managing the power and wake
>>> sources for an ACPI device. By explicitly calling
>>> device_set_wakeup_capable, we are circumvent the ACPI subsystem and
>>> setting wake capabilities on the device when it doesn't support it.
>>>
>>> Take the following example:
>>> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
>>> `_PRW` so that means the device can't wake the system.
>>> * The IRQ line is active level low for this device and is pulled up by the
>>> power resource defined in `_PR0`/`_PR3`.
>>>
>>> Since the i2c-hid driver has set the device as wake capable, the wake
>>> pin gets enabled on suspend.
>>
>> The IRQ pin should only have a enable_irq_wake() called on it if
>> something has actually requested the i2c-HID device to be a wakeup source,
>> the removed code claims the device is wakeup *capable*, but is also
>> explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup.
>>
>> And i2c-hid suspend does:
>>
>>         if (device_may_wakeup(&client->dev)) {
>>                 wake_status = enable_irq_wake(client->irq);
>>
>> And device_may_wakeup() checks the wakeup *enabled* setting AFAIK.
>>
>> I've added Rafael to the Cc since he knows all this a lot better then me.
>>
>> I have the feeling that your userspace is perhaps poking the
>> wakeup settings in sysfs, triggering this issue.
> 
> You are correct, I added some printks in and it is userspace enabling the wake:
> 
>     [    3.280464] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: disabled
>     [    3.280502] i2c_hid_acpi i2c-GDIX0000:00: wakeup_store: start: enabled
>     [    3.280537] i2c_hid_acpi i2c-GDIX0000:00: device_wakeup_enable: start
>     [    3.280541] CPU: 0 PID: 1248 Comm: powerd Not tainted 5.10.83
> #151 c334d4c4185a84ded39aafcb495de6870a8e5161
>     [    3.280545] Hardware name: Google Guybrush/Guybrush, BIOS
> Google_Guybrush.4.15-624-g9d80a9c6aa40 12/21/2021
>     [    3.280548] Call Trace:
>     [    3.280554]  dump_stack+0x9c/0xe7
>     [    3.280560]  device_wakeup_enable+0x136/0x172
>     [    3.280564]  wakeup_store+0xbc/0xc4
>     [    3.280572]  kernfs_fop_write_iter+0x10b/0x18a
>     [    3.280576]  vfs_write+0x383/0x405
>     [    3.280579]  ksys_write+0x74/0xd4
>     [    3.280583]  do_syscall_64+0x43/0x55
>     [    3.280587]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
>>
>>> As part of suspend, ACPI will power down
>>> the device since it's not a wake source. When the device is powered
>>> down, the IRQ line will drop, and it will trigger a wake event.
>>
>> To me that sounds like the device is not wakeup *capable* at all, so
>> its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all.
> 
> The ACPI_FADT_LOW_POWER_S0 flag is a system level flag. The
> system is wake capable and supports S0ix. The touchscreen device
> does not support waking the system because it doesn't provide
> a `_PRW`.

I've checked a whole bunch of DSDTs from standard x86 ACPI laptops and
it seems the _PRW is only used when devices use a GPE for wakeup; not when
they are using a standard GPIO-irq as wakeup source.

To be specific _PRW seems to only be present on PCI devices and on the
ACPI pwr-button and ACPI lid switch devices.

I2C attached touchpads and touchscreens seem to never have _PRW set
(at least in the collection of DSDTs which I have).

Note that what you are doing here is in essence completely reverting
commit 203c38fbe833 ("HID: i2c-hid: Enable wakeup capability from Suspend-to-Idle")
(note when I reviewed that I deliberately asked for the wakeup to be
disabled by default to avoid regressions).

Going ahead with this change would in essence cause regressions,
breaking wakeup by touchpad on many many laptops, so NACK.

>> Note I'm not certain about this at all, but at a first look this feels
>> like it is not the right fix for your problem.
> 
> We can't have the `i2c-hid-acpi` driver calling `device_set_wakeup_capable`.
> This will make the kernel expose the wakeup sysfs entry to userspace
> regardless if the device supports wakeup or not. This would require userspace
> to know that enabling this wake source will cause suspend problems and to
> avoid it.

I'm in 2 minds about this. To me this feels like the story where a person goes
to the doctor and says "doctor if I do this-thing it hurts". To which the doctor
replies: "Well then don't do this-thing". IOW if ChromeOS would not write the
wakeup sysfs attribute then there would be no problem

If I've understood the problem correctly then the issue is with a touchscreen
right? Unlike the I2C-HID driver which does not know the difference between
a touchpad, touchscreen or say a sensor-hub. Userspace can easily query the
associated input evdev node and NOT enable wakeup on touchscreens (which seems
to be a weird thing to do regardless of if it works properly or not).
Blindly enabling wakeup on all devices seems like it is a recipe for hitting
all sort of interesting bugs; and it seems best to just avoid that in
the first place.

As I said I'm in 2 minds about this, OTOH this does feel like a hardware
bug why does the interrupt trigger when the touchscreen is put into S3 ?

To me this feels as if the interrupt line is missing a pull-up/-down or
has the wrong pull-up/-down setting requiring it to be actively driven
to the not-irq state and sending an irq signal as soon as the line is
not actively driven. Could this be a problem with the configuration of
the built-in pull-up/-downs of the GPIO pin used for the IRQ
(which can be controlled by the firmware) ?

you are right that the kernel should not export a non working wakeup sysfs
attribute, so if this cannot be fixed in firmware, then the best option is
probably to add a quirk for this model to the existing quirk list in the
I2C-HID code and to not do the device_set_wakeup_capable() call on this
specific touchscreen based on a new quirk.

> I thought of something else:
> 
>> And i2c-hid suspend does:
>>
>>         if (device_may_wakeup(&client->dev)) {
>>                 wake_status = enable_irq_wake(client->irq);
>>
> 
> I think we also need to guard the enable_irq_wake call with
> `!ACPI_COMPANION(dev)`.
> ACPI will handle enabling the correct GPIO or GPE defined in `_PRW`.
> 

Calling enable_irq_wake() is necessary for wakeup to work on devices which
use a normal GPIO IRQ rather then a GPE for wakeup.

The only case where the enable_irq_wake() should probably not be done is when
their is an ACPI_COMPANION and that ACPI companion has a valid _PRW, which
as mentioned above most (all?) I2C-HID ACPI device nodes do _not_ have.

Regards,

Hans



>>>
>>> See the following debug log:
>>> [   42.335804] PM: Suspending system (s2idle)
>>> [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
>>> [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
>>> [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
>>> [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
>>> [   42.535293] PM: Wakeup unrelated to ACPI SCI
>>> [   42.535294] PM: resume from suspend-to-idle
>>>
>>> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
>>> ---
>>>
>>>  drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
>>> index a6f0257a26de..fc311a19a19d 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
>>> @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
>>>
>>>       acpi_device_fix_up_power(adev);
>>>
>>> -     if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
>>> -             device_set_wakeup_capable(dev, true);
>>> -             device_set_wakeup_enable(dev, false);
>>> -     }
>>> -
>>>       return i2c_hid_core_probe(client, &ihid_acpi->ops,
>>>                                 hid_descriptor_address);
>>>  }
>>>
>>
> 

Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable

[Raul Rangel]

On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/21/21 00:43, Raul E Rangel wrote:
> > The ACPI subsystem is responsible for managing the power and wake
> > sources for an ACPI device. By explicitly calling
> > device_set_wakeup_capable, we are circumvent the ACPI subsystem and
> > setting wake capabilities on the device when it doesn't support it.
> >
> > Take the following example:
> > * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> > `_PRW` so that means the device can't wake the system.
> > * The IRQ line is active level low for this device and is pulled up by the
> > power resource defined in `_PR0`/`_PR3`.
> >
> > Since the i2c-hid driver has set the device as wake capable, the wake
> > pin gets enabled on suspend.
>
> The IRQ pin should only have a enable_irq_wake() called on it if
> something has actually requested the i2c-HID device to be a wakeup source,
> the removed code claims the device is wakeup *capable*, but is also
> explicitly calls device_set_wakeup_enable(dev, false), disabling wakeup.
>

I thought of something else:

> And i2c-hid suspend does:
>
>         if (device_may_wakeup(&client->dev)) {
>                 wake_status = enable_irq_wake(client->irq);
>

I think we also need to guard the enable_irq_wake call with
`!ACPI_COMPANION(dev)`.
ACPI will handle enabling the correct GPIO or GPE defined in `_PRW`.

We might also be able to remove manually calling {enable,disable}_irq_wake by
switching over to `dev_pm_set_wake_irq`. I did this for the elan_i2c
driver in the 2nd patch:
https://patchwork.kernel.org/project/linux-input/patch/20211220163823.2.Id022caf53d01112188308520915798f08a33cd3e@changeid/

> And device_may_wakeup() checks the wakeup *enabled* setting AFAIK.
>
> I've added Rafael to the Cc since he knows all this a lot better then me.
>
> I have the feeling that your userspace is perhaps poking the
> wakeup settings in sysfs, triggering this issue.
>
> > As part of suspend, ACPI will power down
> > the device since it's not a wake source. When the device is powered
> > down, the IRQ line will drop, and it will trigger a wake event.
>
> To me that sounds like the device is not wakeup *capable* at all, so
> its ACPI node should not set the ACPI_FADT_LOW_POWER_S0 flag at all.
>
> Note I'm not certain about this at all, but at a first look this feels
> like it is not the right fix for your problem.
>
> Regards,
>
> Hans
>
>
> >
> > See the following debug log:
> > [   42.335804] PM: Suspending system (s2idle)
> > [   42.340186] amd_gpio AMD0030:00: RX: Setting wake for pin 89 to enable
> > [   42.467736]     power-0416 __acpi_power_off      : Power resource [PR00] turned off
> > [   42.467739] device_pm-0280 device_set_power      : Device [H05D] transitioned to D3cold
> > [   42.475210] PM: pm_system_irq_wakeup: 11 triggered pinctrl_amd
> > [   42.535293] PM: Wakeup unrelated to ACPI SCI
> > [   42.535294] PM: resume from suspend-to-idle
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> >  drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > index a6f0257a26de..fc311a19a19d 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
> > @@ -105,11 +105,6 @@ static int i2c_hid_acpi_probe(struct i2c_client *client)
> >
> >       acpi_device_fix_up_power(adev);
> >
> > -     if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
> > -             device_set_wakeup_capable(dev, true);
> > -             device_set_wakeup_enable(dev, false);
> > -     }
> > -
> >       return i2c_hid_core_probe(client, &ihid_acpi->ops,
> >                                 hid_descriptor_address);
> >  }
> >
>

[PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul E Rangel]

The Elan I2C touchpad driver is currently manually managing the wake
IRQ. When a device is managed by device tree or ACPI it is expected that
those subsystems manage defining the wake pin and manage enabling it.

This change removes the explicit enable_irq_wake/disable_irq_wake and
relies on the PM subsystem. See device_wakeup_arm_wake_irqs.

It also registers the IRQ using dev_pm_set_wake_irq only in the case
where the device isn't DT or ACPI managed. This should preserve the
existing behavior. I'm not sure how we actually get into this state
though.

I tested this with an ACPI device that has a `_PRW` method that uses a
GPE was the wake event. I no longer see the GPIO controller being called
to enable the wake pin. This results in the GPE correctly being marked
as the wake source.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/input/mouse/elan_i2c_core.c | 21 +++++++--------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 47af62c12267..58f056ee0747 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -33,6 +33,7 @@
 #include <linux/jiffies.h>
 #include <linux/completion.h>
 #include <linux/of.h>
+#include <linux/pm_wakeirq.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <asm/unaligned.h>
@@ -86,8 +87,6 @@ struct elan_tp_data {
 	u16			fw_page_size;
 	u32			fw_signature_address;
 
-	bool			irq_wake;
-
 	u8			min_baseline;
 	u8			max_baseline;
 	bool			baseline_ready;
@@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
 	}
 
 	/*
-	 * Systems using device tree should set up wakeup via DTS,
+	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
 	 * the rest will configure device as wakeup source by default.
 	 */
-	if (!dev->of_node)
+	if (!dev->of_node && !ACPI_COMPANION(dev)) {
 		device_init_wakeup(dev, true);
+		dev_pm_set_wake_irq(dev, client->irq);
+	}
 
 	return 0;
 }
@@ -1394,13 +1395,10 @@ static int __maybe_unused elan_suspend(struct device *dev)
 
 	disable_irq(client->irq);
 
-	if (device_may_wakeup(dev)) {
+	if (device_may_wakeup(dev))
 		ret = elan_sleep(data);
-		/* Enable wake from IRQ */
-		data->irq_wake = (enable_irq_wake(client->irq) == 0);
-	} else {
+	else
 		ret = elan_disable_power(data);
-	}
 
 	mutex_unlock(&data->sysfs_mutex);
 	return ret;
@@ -1412,11 +1410,6 @@ static int __maybe_unused elan_resume(struct device *dev)
 	struct elan_tp_data *data = i2c_get_clientdata(client);
 	int error;
 
-	if (device_may_wakeup(dev) && data->irq_wake) {
-		disable_irq_wake(client->irq);
-		data->irq_wake = false;
-	}
-
 	error = elan_enable_power(data);
 	if (error) {
 		dev_err(dev, "power up when resuming failed: %d\n", error);
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

Hi Raul,

On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
>  	}
>  
>  	/*
> -	 * Systems using device tree should set up wakeup via DTS,
> +	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
>  	 * the rest will configure device as wakeup source by default.
>  	 */
> -	if (!dev->of_node)
> +	if (!dev->of_node && !ACPI_COMPANION(dev)) {

I think this will break our Rambis that use ACPI for enumeration but
actually lack _PRW. As far as I remember their trackpads were capable
of waking up the system.

I think we should remove this chunk completely and instead add necessary
code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
have additional member in struct acpi_peripheral to indicate whether
device needs to be configured for wakeup and then act upon it in
chromeos_laptop_adjust_client().

>  		device_init_wakeup(dev, true);
> +		dev_pm_set_wake_irq(dev, client->irq);
> +	}
>  
>  	return 0;
>  }

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Raul Rangel]

On Mon, Dec 20, 2021 at 7:41 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Raul,
>
> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
> > @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
> >       }
> >
> >       /*
> > -      * Systems using device tree should set up wakeup via DTS,
> > +      * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
> >        * the rest will configure device as wakeup source by default.
> >        */
> > -     if (!dev->of_node)
> > +     if (!dev->of_node && !ACPI_COMPANION(dev)) {
>
> I think this will break our Rambis that use ACPI for enumeration but
> actually lack _PRW. As far as I remember their trackpads were capable
> of waking up the system.

Looks like the _PRW was only added for the atmel touchscreen:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/rambi/acpi/touchscreen_atmel.asl;l=42

I'm assuming this was before we had the `drivers/i2c/hid` chip driver.

>
> I think we should remove this chunk completely and instead add necessary
> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
> have additional member in struct acpi_peripheral to indicate whether
> device needs to be configured for wakeup and then act upon it in
> chromeos_laptop_adjust_client().

I think that's a good idea. Should I add all the mainboards defined
here: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/coreboot/src/mainboard/google/rambi/Kconfig;l=48
?

>
> >               device_init_wakeup(dev, true);
> > +             dev_pm_set_wake_irq(dev, client->irq);
> > +     }
> >
> >       return 0;
> >  }
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Hans de Goede]

Hi,

On 12/21/21 03:41, Dmitry Torokhov wrote:
> Hi Raul,
> 
> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
>> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
>>  	}
>>  
>>  	/*
>> -	 * Systems using device tree should set up wakeup via DTS,
>> +	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
>>  	 * the rest will configure device as wakeup source by default.
>>  	 */
>> -	if (!dev->of_node)
>> +	if (!dev->of_node && !ACPI_COMPANION(dev)) {
> 
> I think this will break our Rambis that use ACPI for enumeration but
> actually lack _PRW. As far as I remember their trackpads were capable
> of waking up the system.
> 
> I think we should remove this chunk completely and instead add necessary
> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
> have additional member in struct acpi_peripheral to indicate whether
> device needs to be configured for wakeup and then act upon it in
> chromeos_laptop_adjust_client().
> 
>>  		device_init_wakeup(dev, true);
>> +		dev_pm_set_wake_irq(dev, client->irq);
>> +	}

As I already mentioned in my other reply in this thread:

https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/

AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
as such they do not have a _PRW method.

So for wakeup by elan_i2c touchpads to keep working this code is not
just necessary for some ChromeOS devices, but it is necessary on
most ACPI devices.

The problem of not making these calls on devices where a GPE is actually
used for touchpad wakeup (which at least for now is the exception not
the rule) should probably be fixed by no running this "chunk"
when the device has an ACPI_COMPANION (as this patch already checks)
*and* that ACPI_COMPANION has a valid _PRW method.

Simply removing this chunk, or taking this patch as is will very
likely lead to regressions on various x86 laptop models.

Regards,

Hans

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Dmitry Torokhov]

On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 12/21/21 03:41, Dmitry Torokhov wrote:
> > Hi Raul,
> > 
> > On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
> >> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
> >>  	}
> >>  
> >>  	/*
> >> -	 * Systems using device tree should set up wakeup via DTS,
> >> +	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
> >>  	 * the rest will configure device as wakeup source by default.
> >>  	 */
> >> -	if (!dev->of_node)
> >> +	if (!dev->of_node && !ACPI_COMPANION(dev)) {
> > 
> > I think this will break our Rambis that use ACPI for enumeration but
> > actually lack _PRW. As far as I remember their trackpads were capable
> > of waking up the system.
> > 
> > I think we should remove this chunk completely and instead add necessary
> > code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
> > have additional member in struct acpi_peripheral to indicate whether
> > device needs to be configured for wakeup and then act upon it in
> > chromeos_laptop_adjust_client().

FWIW I looked at Rambi some more and I see that it actually defines a
separate device an ACPI to handle wakeups, it is separate from the ACPI
node for the trackpad:

Scope (\_SB)
{
#ifdef BOARD_TRACKPAD_IRQ
        /* Wake device for touchpad */
        Device (TPAD)
        {
                Name (_HID, EisaId ("PNP0C0E"))
                Name (_UID, 1)
                Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })

                Name (RBUF, ResourceTemplate()
                {
                        Interrupt (ResourceConsumer, Level, ActiveLow)
                        {
                                BOARD_TRACKPAD_IRQ
                        }
                })

                Method (_CRS)
                {
                        /* Only return interrupt if I2C1 is PCI mode */
                        If (LEqual (\S1EN, 0)) {
                                Return (^RBUF)
                        }

                        /* Return empty resource template otherwise */
                        Return (ResourceTemplate() {})
                }
        }
#endif

I am not quite sure why we did this...

> > 
> >>  		device_init_wakeup(dev, true);
> >> +		dev_pm_set_wake_irq(dev, client->irq);
> >> +	}
> 
> As I already mentioned in my other reply in this thread:
> 
> https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/
> 
> AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
> as such they do not have a _PRW method.
> 
> So for wakeup by elan_i2c touchpads to keep working this code is not
> just necessary for some ChromeOS devices, but it is necessary on
> most ACPI devices.
> 
> The problem of not making these calls on devices where a GPE is actually
> used for touchpad wakeup (which at least for now is the exception not
> the rule) should probably be fixed by no running this "chunk"
> when the device has an ACPI_COMPANION (as this patch already checks)
> *and* that ACPI_COMPANION has a valid _PRW method.
> 
> Simply removing this chunk, or taking this patch as is will very
> likely lead to regressions on various x86 laptop models.

Hans, could you share a couple of DSDTs for devices that do not use GPEs
for wakeup?

For OF we already recognize that wakeup source/interrupt might differ
from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
The question is to how determine if a device is supposed to be a wakeup
source if it does not have _PRW.

Thanks.

-- 
Dmitry

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Hans de Goede]

Hi,

On 12/23/21 22:21, Dmitry Torokhov wrote:
> On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 12/21/21 03:41, Dmitry Torokhov wrote:
>>> Hi Raul,
>>>
>>> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
>>>> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
>>>>  	}
>>>>  
>>>>  	/*
>>>> -	 * Systems using device tree should set up wakeup via DTS,
>>>> +	 * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
>>>>  	 * the rest will configure device as wakeup source by default.
>>>>  	 */
>>>> -	if (!dev->of_node)
>>>> +	if (!dev->of_node && !ACPI_COMPANION(dev)) {
>>>
>>> I think this will break our Rambis that use ACPI for enumeration but
>>> actually lack _PRW. As far as I remember their trackpads were capable
>>> of waking up the system.
>>>
>>> I think we should remove this chunk completely and instead add necessary
>>> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
>>> have additional member in struct acpi_peripheral to indicate whether
>>> device needs to be configured for wakeup and then act upon it in
>>> chromeos_laptop_adjust_client().
> 
> FWIW I looked at Rambi some more and I see that it actually defines a
> separate device an ACPI to handle wakeups, it is separate from the ACPI
> node for the trackpad:
> 
> Scope (\_SB)
> {
> #ifdef BOARD_TRACKPAD_IRQ
>         /* Wake device for touchpad */
>         Device (TPAD)
>         {
>                 Name (_HID, EisaId ("PNP0C0E"))
>                 Name (_UID, 1)
>                 Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })
> 
>                 Name (RBUF, ResourceTemplate()
>                 {
>                         Interrupt (ResourceConsumer, Level, ActiveLow)
>                         {
>                                 BOARD_TRACKPAD_IRQ
>                         }
>                 })
> 
>                 Method (_CRS)
>                 {
>                         /* Only return interrupt if I2C1 is PCI mode */
>                         If (LEqual (\S1EN, 0)) {
>                                 Return (^RBUF)
>                         }
> 
>                         /* Return empty resource template otherwise */
>                         Return (ResourceTemplate() {})
>                 }
>         }
> #endif
> 
> I am not quite sure why we did this...
> 
>>>
>>>>  		device_init_wakeup(dev, true);
>>>> +		dev_pm_set_wake_irq(dev, client->irq);
>>>> +	}
>>
>> As I already mentioned in my other reply in this thread:
>>
>> https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/
>>
>> AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
>> as such they do not have a _PRW method.
>>
>> So for wakeup by elan_i2c touchpads to keep working this code is not
>> just necessary for some ChromeOS devices, but it is necessary on
>> most ACPI devices.
>>
>> The problem of not making these calls on devices where a GPE is actually
>> used for touchpad wakeup (which at least for now is the exception not
>> the rule) should probably be fixed by no running this "chunk"
>> when the device has an ACPI_COMPANION (as this patch already checks)
>> *and* that ACPI_COMPANION has a valid _PRW method.
>>
>> Simply removing this chunk, or taking this patch as is will very
>> likely lead to regressions on various x86 laptop models.
> 
> Hans, could you share a couple of DSDTs for devices that do not use GPEs
> for wakeup?
> 
> For OF we already recognize that wakeup source/interrupt might differ
> from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
> The question is to how determine if a device is supposed to be a wakeup
> source if it does not have _PRW.

With s2idle (rather then S3) we never really suspend, we just put
everything in an as low power state as possible and call halt on the
CPU and then hope that the SoC power-management-unit shuts of a whole
bunch of power-planes based on all the devices being in a low power
state.

This means that in practice with s2idle any device can be a wakeup
device since regular IRQs work fine as wakeup sources in s2idle.

This is what the s2idle support in the i2c-hid code is based on:
drivers/hid/i2c-hid/i2c-hid-acpi.c:

        if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
                device_set_wakeup_capable(dev, true);
                device_set_wakeup_enable(dev, false);
        }

So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which
uses i2c_hid_acpi as driver for its touchpad and if I echo
enabled to the wakeup attr there, then wakeup by touchpad does work.

One interesting thing there is that the touchpad ACPI node does not
have _PS0 and _PS3. Which means that the touchpad working as wakeup
device makes sense, since it can not be turned off at all.

So I guess we could extend the above check in the i2c-hid-acpi
code to read:

        if ((acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) &&
	    !adev->flags.power_manageable) {
                device_set_wakeup_capable(dev, true);
                device_set_wakeup_enable(dev, false);
        }

Because if there is a _PS3, which presumably is the case for
the troublesome touchscreen Raul is trying to fix, then we
will call that on suspend; and after that it is likely that
the device will not work as a wakeup source.

And I just checked the DSDT of a couple of devices where I'm
reasonable sure that the touchpad uses I2C-HID and none of
them define _PS0/_PS3 methods on the touchpad ACPI node.

So I think that the above suggestion should fix things
for the i2c-hid case.

I've added Kai-Heng, the author of the original change
introducing the device_set_wakeup_capable() call, to the Cc.
Kai-Heng what do you think about this ?

Raul, can you check if this resolves your issue?

FWIW here is an acpidump of the X1C8:
https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8

Regards,

Hans


p.s.

An other interesting datapoint is that despite not declaring
a _PRW method the DSDTs which I've checked do all 3 contain
an _S0W method, returning 3 or 4. Which suggests that maybe the
ACPI code should look at _S0W even when no GPE is being used?

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Kai-Heng Feng]

On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 12/23/21 22:21, Dmitry Torokhov wrote:
> > On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 12/21/21 03:41, Dmitry Torokhov wrote:
> >>> Hi Raul,
> >>>
> >>> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
> >>>> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
> >>>>    }
> >>>>
> >>>>    /*
> >>>> -   * Systems using device tree should set up wakeup via DTS,
> >>>> +   * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
> >>>>     * the rest will configure device as wakeup source by default.
> >>>>     */
> >>>> -  if (!dev->of_node)
> >>>> +  if (!dev->of_node && !ACPI_COMPANION(dev)) {
> >>>
> >>> I think this will break our Rambis that use ACPI for enumeration but
> >>> actually lack _PRW. As far as I remember their trackpads were capable
> >>> of waking up the system.
> >>>
> >>> I think we should remove this chunk completely and instead add necessary
> >>> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
> >>> have additional member in struct acpi_peripheral to indicate whether
> >>> device needs to be configured for wakeup and then act upon it in
> >>> chromeos_laptop_adjust_client().
> >
> > FWIW I looked at Rambi some more and I see that it actually defines a
> > separate device an ACPI to handle wakeups, it is separate from the ACPI
> > node for the trackpad:
> >
> > Scope (\_SB)
> > {
> > #ifdef BOARD_TRACKPAD_IRQ
> >         /* Wake device for touchpad */
> >         Device (TPAD)
> >         {
> >                 Name (_HID, EisaId ("PNP0C0E"))
> >                 Name (_UID, 1)
> >                 Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })
> >
> >                 Name (RBUF, ResourceTemplate()
> >                 {
> >                         Interrupt (ResourceConsumer, Level, ActiveLow)
> >                         {
> >                                 BOARD_TRACKPAD_IRQ
> >                         }
> >                 })
> >
> >                 Method (_CRS)
> >                 {
> >                         /* Only return interrupt if I2C1 is PCI mode */
> >                         If (LEqual (\S1EN, 0)) {
> >                                 Return (^RBUF)
> >                         }
> >
> >                         /* Return empty resource template otherwise */
> >                         Return (ResourceTemplate() {})
> >                 }
> >         }
> > #endif
> >
> > I am not quite sure why we did this...
> >
> >>>
> >>>>            device_init_wakeup(dev, true);
> >>>> +          dev_pm_set_wake_irq(dev, client->irq);
> >>>> +  }
> >>
> >> As I already mentioned in my other reply in this thread:
> >>
> >> https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/
> >>
> >> AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
> >> as such they do not have a _PRW method.
> >>
> >> So for wakeup by elan_i2c touchpads to keep working this code is not
> >> just necessary for some ChromeOS devices, but it is necessary on
> >> most ACPI devices.
> >>
> >> The problem of not making these calls on devices where a GPE is actually
> >> used for touchpad wakeup (which at least for now is the exception not
> >> the rule) should probably be fixed by no running this "chunk"
> >> when the device has an ACPI_COMPANION (as this patch already checks)
> >> *and* that ACPI_COMPANION has a valid _PRW method.
> >>
> >> Simply removing this chunk, or taking this patch as is will very
> >> likely lead to regressions on various x86 laptop models.
> >
> > Hans, could you share a couple of DSDTs for devices that do not use GPEs
> > for wakeup?
> >
> > For OF we already recognize that wakeup source/interrupt might differ
> > from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
> > The question is to how determine if a device is supposed to be a wakeup
> > source if it does not have _PRW.
>
> With s2idle (rather then S3) we never really suspend, we just put
> everything in an as low power state as possible and call halt on the
> CPU and then hope that the SoC power-management-unit shuts of a whole
> bunch of power-planes based on all the devices being in a low power
> state.
>
> This means that in practice with s2idle any device can be a wakeup
> device since regular IRQs work fine as wakeup sources in s2idle.
>
> This is what the s2idle support in the i2c-hid code is based on:
> drivers/hid/i2c-hid/i2c-hid-acpi.c:
>
>         if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
>                 device_set_wakeup_capable(dev, true);
>                 device_set_wakeup_enable(dev, false);
>         }
>
> So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which
> uses i2c_hid_acpi as driver for its touchpad and if I echo
> enabled to the wakeup attr there, then wakeup by touchpad does work.
>
> One interesting thing there is that the touchpad ACPI node does not
> have _PS0 and _PS3. Which means that the touchpad working as wakeup
> device makes sense, since it can not be turned off at all.
>
> So I guess we could extend the above check in the i2c-hid-acpi
> code to read:
>
>         if ((acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) &&
>             !adev->flags.power_manageable) {
>                 device_set_wakeup_capable(dev, true);
>                 device_set_wakeup_enable(dev, false);
>         }
>
> Because if there is a _PS3, which presumably is the case for
> the troublesome touchscreen Raul is trying to fix, then we
> will call that on suspend; and after that it is likely that
> the device will not work as a wakeup source.
>
> And I just checked the DSDT of a couple of devices where I'm
> reasonable sure that the touchpad uses I2C-HID and none of
> them define _PS0/_PS3 methods on the touchpad ACPI node.
>
> So I think that the above suggestion should fix things
> for the i2c-hid case.
>
> I've added Kai-Heng, the author of the original change
> introducing the device_set_wakeup_capable() call, to the Cc.
> Kai-Heng what do you think about this ?
>
> Raul, can you check if this resolves your issue?
>
> FWIW here is an acpidump of the X1C8:
> https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8
>
> Regards,
>
> Hans
>
>
> p.s.
>
> An other interesting datapoint is that despite not declaring
> a _PRW method the DSDTs which I've checked do all 3 contain
> an _S0W method, returning 3 or 4. Which suggests that maybe the
> ACPI code should look at _S0W even when no GPE is being used?
>

Maybe "ExclusiveAndWake" in _CRS is enough? ACPI spec says "whether it
is capable of waking the system from a low-power idle or system sleep
state" without mentioning the need for _PRW.

Kai-Heng

Re: [PATCH 2/3] Input: elan_i2c - Use PM subsystem to manage wake irq

[Hans de Goede]

Hi,

On 12/25/21 14:51, Kai-Heng Feng wrote:
> On Fri, Dec 24, 2021 at 7:11 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 12/23/21 22:21, Dmitry Torokhov wrote:
>>> On Thu, Dec 23, 2021 at 03:42:24PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 12/21/21 03:41, Dmitry Torokhov wrote:
>>>>> Hi Raul,
>>>>>
>>>>> On Mon, Dec 20, 2021 at 04:43:45PM -0700, Raul E Rangel wrote:
>>>>>> @@ -1368,11 +1367,13 @@ static int elan_probe(struct i2c_client *client,
>>>>>>    }
>>>>>>
>>>>>>    /*
>>>>>> -   * Systems using device tree should set up wakeup via DTS,
>>>>>> +   * Systems using device tree or ACPI should set up wakeup via DTS/ACPI,
>>>>>>     * the rest will configure device as wakeup source by default.
>>>>>>     */
>>>>>> -  if (!dev->of_node)
>>>>>> +  if (!dev->of_node && !ACPI_COMPANION(dev)) {
>>>>>
>>>>> I think this will break our Rambis that use ACPI for enumeration but
>>>>> actually lack _PRW. As far as I remember their trackpads were capable
>>>>> of waking up the system.
>>>>>
>>>>> I think we should remove this chunk completely and instead add necessary
>>>>> code to drivers/platform/chrome/chrome-laptop.c (I suppose we need to
>>>>> have additional member in struct acpi_peripheral to indicate whether
>>>>> device needs to be configured for wakeup and then act upon it in
>>>>> chromeos_laptop_adjust_client().
>>>
>>> FWIW I looked at Rambi some more and I see that it actually defines a
>>> separate device an ACPI to handle wakeups, it is separate from the ACPI
>>> node for the trackpad:
>>>
>>> Scope (\_SB)
>>> {
>>> #ifdef BOARD_TRACKPAD_IRQ
>>>         /* Wake device for touchpad */
>>>         Device (TPAD)
>>>         {
>>>                 Name (_HID, EisaId ("PNP0C0E"))
>>>                 Name (_UID, 1)
>>>                 Name (_PRW, Package() { BOARD_TRACKPAD_WAKE_GPIO, 0x3 })
>>>
>>>                 Name (RBUF, ResourceTemplate()
>>>                 {
>>>                         Interrupt (ResourceConsumer, Level, ActiveLow)
>>>                         {
>>>                                 BOARD_TRACKPAD_IRQ
>>>                         }
>>>                 })
>>>
>>>                 Method (_CRS)
>>>                 {
>>>                         /* Only return interrupt if I2C1 is PCI mode */
>>>                         If (LEqual (\S1EN, 0)) {
>>>                                 Return (^RBUF)
>>>                         }
>>>
>>>                         /* Return empty resource template otherwise */
>>>                         Return (ResourceTemplate() {})
>>>                 }
>>>         }
>>> #endif
>>>
>>> I am not quite sure why we did this...
>>>
>>>>>
>>>>>>            device_init_wakeup(dev, true);
>>>>>> +          dev_pm_set_wake_irq(dev, client->irq);
>>>>>> +  }
>>>>
>>>> As I already mentioned in my other reply in this thread:
>>>>
>>>> https://lore.kernel.org/linux-input/f594afab-8c1a-8821-a775-e5512e17ce8f@redhat.com/
>>>>
>>>> AFAICT most x86 ACPI laptops do not use GPEs for wakeup by touchpad and
>>>> as such they do not have a _PRW method.
>>>>
>>>> So for wakeup by elan_i2c touchpads to keep working this code is not
>>>> just necessary for some ChromeOS devices, but it is necessary on
>>>> most ACPI devices.
>>>>
>>>> The problem of not making these calls on devices where a GPE is actually
>>>> used for touchpad wakeup (which at least for now is the exception not
>>>> the rule) should probably be fixed by no running this "chunk"
>>>> when the device has an ACPI_COMPANION (as this patch already checks)
>>>> *and* that ACPI_COMPANION has a valid _PRW method.
>>>>
>>>> Simply removing this chunk, or taking this patch as is will very
>>>> likely lead to regressions on various x86 laptop models.
>>>
>>> Hans, could you share a couple of DSDTs for devices that do not use GPEs
>>> for wakeup?
>>>
>>> For OF we already recognize that wakeup source/interrupt might differ
>>> from "main" I2C interrupt, I guess we need to do similar for ACPI cases.
>>> The question is to how determine if a device is supposed to be a wakeup
>>> source if it does not have _PRW.
>>
>> With s2idle (rather then S3) we never really suspend, we just put
>> everything in an as low power state as possible and call halt on the
>> CPU and then hope that the SoC power-management-unit shuts of a whole
>> bunch of power-planes based on all the devices being in a low power
>> state.
>>
>> This means that in practice with s2idle any device can be a wakeup
>> device since regular IRQs work fine as wakeup sources in s2idle.
>>
>> This is what the s2idle support in the i2c-hid code is based on:
>> drivers/hid/i2c-hid/i2c-hid-acpi.c:
>>
>>         if (acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) {
>>                 device_set_wakeup_capable(dev, true);
>>                 device_set_wakeup_enable(dev, false);
>>         }
>>
>> So I did just test this on a Lenovo ThinkPad X1 carbon gen 8, which
>> uses i2c_hid_acpi as driver for its touchpad and if I echo
>> enabled to the wakeup attr there, then wakeup by touchpad does work.
>>
>> One interesting thing there is that the touchpad ACPI node does not
>> have _PS0 and _PS3. Which means that the touchpad working as wakeup
>> device makes sense, since it can not be turned off at all.
>>
>> So I guess we could extend the above check in the i2c-hid-acpi
>> code to read:
>>
>>         if ((acpi_gbl_FADT.flags & ACPI_FADT_LOW_POWER_S0) &&
>>             !adev->flags.power_manageable) {
>>                 device_set_wakeup_capable(dev, true);
>>                 device_set_wakeup_enable(dev, false);
>>         }
>>
>> Because if there is a _PS3, which presumably is the case for
>> the troublesome touchscreen Raul is trying to fix, then we
>> will call that on suspend; and after that it is likely that
>> the device will not work as a wakeup source.
>>
>> And I just checked the DSDT of a couple of devices where I'm
>> reasonable sure that the touchpad uses I2C-HID and none of
>> them define _PS0/_PS3 methods on the touchpad ACPI node.
>>
>> So I think that the above suggestion should fix things
>> for the i2c-hid case.
>>
>> I've added Kai-Heng, the author of the original change
>> introducing the device_set_wakeup_capable() call, to the Cc.
>> Kai-Heng what do you think about this ?
>>
>> Raul, can you check if this resolves your issue?
>>
>> FWIW here is an acpidump of the X1C8:
>> https://fedorapeople.org/~jwrdegoede/acpidump-lenovo-x1c8
>>
>> Regards,
>>
>> Hans
>>
>>
>> p.s.
>>
>> An other interesting datapoint is that despite not declaring
>> a _PRW method the DSDTs which I've checked do all 3 contain
>> an _S0W method, returning 3 or 4. Which suggests that maybe the
>> ACPI code should look at _S0W even when no GPE is being used?
>>

Sorry for being slow to respond.

> Maybe "ExclusiveAndWake" in _CRS is enough? ACPI spec says "whether it
> is capable of waking the system from a low-power idle or system sleep
> state" without mentioning the need for _PRW.

Ah yes checking for that is probable even better. We probably need to
add some ACPI helper for that though.

Regards,

hans

[PATCH 3/3] platform/chrome: cros_ec: Don't enable wake pin if ACPI managed

[Raul E Rangel]

In ACPI managed systems, the `_PRW` method defines the wake source for
the device. This could be a GPE or a GPIO not related to the IRQ. The
way the cros_ec_lpc driver works is that the irq field is optional. The
IRQ defined in the `_CRS` is only used to speed up sensor event
processing. Before this change, the SYNC_IRQ GPIO would have its wake
bit enabled. This means that we now have two wake sources defined for
the EC. This change makes the CrOS EC driver leave wake configuration
alone if the device is ACPI managed.

I tested this on guybrush and no longer see the EC SYNC IRQ enabled as a
wake source when suspending.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/platform/chrome/cros_ec.c           | 8 ++++++--
 include/linux/platform_data/cros_ec_proto.h | 1 -
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index fc5aa1525d13..81e334157338 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -9,6 +9,7 @@
  * battery charging and regulator control, firmware update.
  */
 
+#include <linux/acpi.h>
 #include <linux/of_platform.h>
 #include <linux/interrupt.h>
 #include <linux/slab.h>
@@ -336,11 +337,14 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
 		dev_dbg(ec_dev->dev, "Error %d sending suspend event to ec",
 			ret);
 
-	if (device_may_wakeup(dev))
+	/*
+	 * For non-ACPI subsystems we need to explicitly enable the wake source.
+	 * For ACPI systems, the ACPI subsystem will handle all the details.
+	 */
+	if (device_may_wakeup(dev) && !ACPI_COMPANION(ec_dev->dev))
 		ec_dev->wake_enabled = !enable_irq_wake(ec_dev->irq);
 
 	disable_irq(ec_dev->irq);
-	ec_dev->was_wake_device = ec_dev->wake_enabled;
 	ec_dev->suspended = true;
 
 	return 0;
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index 992796e40cd6..054d28ddb4c1 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -139,7 +139,6 @@ struct cros_ec_device {
 	/* These are used by other drivers that want to talk to the EC */
 	const char *phys_name;
 	struct device *dev;
-	bool was_wake_device;
 	struct class *cros_class;
 	int (*cmd_readmem)(struct cros_ec_device *ec, unsigned int offset,
 			   unsigned int bytes, void *dest);
-- 
2.34.1.307.g9b7440fafd-goog