On Tue, Aug 30, 2022 at 05:15:33PM -0600, Raul E Rangel wrote:
> Today, i2c drivers are making the assumption that their IRQs can also
> be used as wake IRQs. This isn't always the case and it can lead to
> spurious wakes. This has recently started to affect AMD Chromebooks.
> With the introduction of
> d62bd5ce12d7 ("pinctrl: amd: Implement irq_set_wake"), the AMD GPIO
> controller gained the capability to set the wake bit on each GPIO. The
> ACPI specification defines two ways to inform the system if a device is
> wake capable:
> 1) The _PRW object defines the GPE that can be used to wake the system.
> 2) Setting ExclusiveAndWake or SharedAndWake in the _CRS GpioInt.
>
> Currently only the first method is supported. The i2c drivers don't have
> any indication that the IRQ is wake capable, so they guess. This causes
> spurious interrupts, for example:
> * We have an ACPI HID device that has `_PR0` and `_PR3`. It doesn't have
> `_PRW` or `ExclusiveAndWake` 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 driver will (incorrectly) arm the GPIO for wake by calling
> `enable_irq_wake` as part of its suspend hook.
> * ACPI will power down the device since it doesn't have a wake GPE
> associated with it.
> * 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
>
> In order to fix this, we need to take into account the wake capable bit
> defined on the GpioInt. This is accomplished by:
> * Migrating some of the i2c drivers over to using the PM subsystem to
> manage the wake IRQ. max8925-i2c, elants_i2c, and raydium_i2c_ts still
> need to be migrated, I can do that depending on the feedback to this
> patch series.
> * Expose the wake_capable bit from the ACPI GpioInt resource to the
> i2c core.
> * Use the wake_capable bit in the i2c core to call
> `dev_pm_set_wake_irq`. This reuses the existing device tree flow.
> * Make the i2c drivers stop calling `dev_pm_set_wake_irq` since it's now
> handled by the i2c core.
> * Make the ACPI device PM system aware of the wake_irq. This is
> necessary so the device doesn't incorrectly get powered down when a
> wake_irq is enabled.
>
> I've tested this code with various combinations of having _PRW,
> ExclusiveAndWake and power resources all defined or not defined, but it
> would be great if others could test this out on their hardware.
I have got only cover letter and a single patch (#3). What's going on?
Note: I'm also reviewer of I²C DesignWare driver, you really have to
fix your tools / submission process and try again. No review for this
series.
--
With Best Regards,
Andy Shevchenko
