Hi, On 8/31/22 16:37, Raul Rangel wrote: > Interesting... The patch series is here: > https://patchwork.kernel.org/project/linux-input/cover/20220830231541.1135813-1-rrangel@xxxxxxxxxxxx/ > > I'll look into why you only got added to 2 of the emails. FWIW I also received the full series without problems. I'll try to reply to this soon-ish, but I have a bit of a patch backlog to process and I'm trying to process the backlog in FIFO order and this is one of the last series in the backlog ... Regards, Hans > > On Wed, Aug 31, 2022 at 5:52 AM Andy Shevchenko > <andriy.shevchenko@xxxxxxxxxxxxxxx> wrote: >> >> 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涎 DesignWare driver, you really have to >> fix your tools / submission process and try again. No review for this >> series. >> >> -- >> With Best Regards, >> Andy Shevchenko >> >> >
