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

On Tue, Dec 21, 2021 at 11:49 AM Hans de Goede [off-list ref] wrote:

Hi,

On 12/21/21 00:43, Raul E Rangel wrote:

quoted

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.

quoted

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

quoted

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

 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);
 }