Re: [PATCH 1/3] HID: i2c-hid-acpi: Remove explicit device_set_wakeup_capable
From: Hans de Goede <hidden>
Date: 2021-12-21 18:49:45
Also in:
lkml
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
quoted hunk ↗ jump to hunk
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); }