| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Yyw12S3pstNrbGFO@smile.fi.intel.com>
Date: Thu, 22 Sep 2022 13:15:53 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Raul E Rangel <rrangel@...omium.org>
Cc: linux-acpi@...r.kernel.org, linux-input@...r.kernel.org,
rafael@...nel.org, timvp@...gle.com,
mika.westerberg@...ux.intel.com, dmitry.torokhov@...il.com,
jingle.wu@....com.tw, hdegoede@...hat.com,
mario.limonciello@....com, linus.walleij@...aro.org,
Alistair Francis <alistair@...stair23.me>,
Angela Czubak <acz@...ihalf.com>,
Bartosz Golaszewski <brgl@...ev.pl>,
Bartosz Szczepanek <bsz@...ihalf.com>,
Benjamin Tissoires <benjamin.tissoires@...hat.com>,
Cai Huoqing <cai.huoqing@...ux.dev>,
Dan Williams <dan.j.williams@...el.com>,
Douglas Anderson <dianders@...omium.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Guenter Roeck <groeck@...omium.org>,
Jiri Kosina <jikos@...nel.org>,
Johnny Chuang <johnny.chuang.emc@...il.com>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Len Brown <lenb@...nel.org>,
Matthias Kaehlcke <mka@...omium.org>,
Nathan Chancellor <nathan@...nel.org>,
Nick Desaulniers <ndesaulniers@...gle.com>,
Rob Herring <robh@...nel.org>,
Terry Bowman <terry.bowman@....com>, Tom Rix <trix@...hat.com>,
Wolfram Sang <wsa@...nel.org>,
Yang Li <yang.lee@...ux.alibaba.com>,
linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org,
linux-kernel@...r.kernel.org, llvm@...ts.linux.dev
Subject: Re: [PATCH v5 00/13] acpi: i2c: Use SharedAndWake and
ExclusiveAndWake to enable wake irq
On Wed, Sep 21, 2022 at 09:51:52AM -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 Interrupt/GpioInt. This is accomplished by:
> * Migrating some of the i2c drivers over to using the PM subsystem to
> manage the wake IRQ.
> * Expose the wake_capable bit from the ACPI Interrupt/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'm sure this will surface some devices where the IRQs were not
> correctly marked as wake capable. Ideally the firmware can be fixed, but
> if not we can work around this in the kernel by providing a board
> specific `struct i2c_board_info` with the `I2C_CLIENT_WAKE` flag set.
> See `chromeos_laptop.c` for an example of matching DMI properties and
> setting the `I2C_CLIENT_WAKE` override.
>
> Thanks,
> Raul
>
> Changes in v5:
> - Added Acked-by: Benjamin Tissoires
> - Removed clang-format white space changes
> - Check irq return value before updating wake_capable pointer
You are too fast with a new versions... I have comment on the v4 and I believe
it applicable here (not settled yet).
> - Go back to using adev->wakeup.flags.valid to keep the diff cleaner
> - Fix a typo in comment
>
> Changes in v4:
> - Added Reviewed-by
> - Reformatted with 96 char limit
> - Added Reviewed-by
> - Reformatted with 96 char limit
> - Removed unnecessary !!
> - Removed unrelated whitespace change
> - Added Reviewed-by
> - Renamed i2c_acpi_add_resource to i2c_acpi_add_irq_resource
> - Expanded logic in i2c_acpi_add_i2c_resource to make it easier to read
>
> Changes in v3:
> - Kept `acpi_dev_gpio_irq_get_by` unchanged to avoid having to touch
> unrelated drivers.
> - Converted wake_capable parameter to bool.
> - Fixed bad indent
> - Convert wake_capable to bool
> - Only update wake_capable pointer once
> - Move wake_capable local into local block
>
> Changes in v2:
> - Added elants_i2c to series
> - Added raydium_ts_i2c to series
> - Fixed call site in mlxbf_gige_probe
> - Added ability to extract wake bit from Interrupt/IRQ resources
> - Look at wake_cabple bit for IRQ/Interrupt resources
> - I chose not to keep the legacy code around since systems without DT or ACPI should be rare.
>
> Raul E Rangel (13):
> HID: i2c-hid: Use PM subsystem to manage wake irq
> Input: elan_i2c - Use PM subsystem to manage wake irq
> Input: elants_i2c - Use PM subsystem to manage wake irq
> Input: raydium_ts_i2c - Use PM subsystem to manage wake irq
> gpiolib: acpi: Add wake_capable variants of acpi_dev_gpio_irq_get
> ACPI: resources: Add wake_capable parameter to acpi_dev_irq_flags
> i2c: acpi: Use ACPI wake capability bit to set wake_irq
> ACPI: PM: Take wake IRQ into consideration when entering
> suspend-to-idle
> HID: i2c-hid: acpi: Stop setting wakeup_capable
> HID: i2c-hid: Don't set wake_capable and wake_irq
> Input: elan_i2c - Don't set wake_capable and wake_irq
> Input: elants_i2c - Don't set wake_capable and wake_irq
> Input: raydium_ts_i2c - Don't set wake_capable and wake_irq
>
> drivers/acpi/device_pm.c | 19 +++++++++--
> drivers/acpi/irq.c | 8 +++--
> drivers/acpi/resource.c | 16 +++++++---
> drivers/gpio/gpiolib-acpi.c | 15 +++++++--
> drivers/gpio/gpiolib-acpi.h | 2 ++
> drivers/hid/i2c-hid/i2c-hid-acpi.c | 5 ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 24 ++------------
> drivers/i2c/i2c-core-acpi.c | 37 ++++++++++++++++------
> drivers/i2c/i2c-core-base.c | 6 +++-
> drivers/i2c/i2c-core.h | 4 +--
> drivers/input/mouse/elan_i2c_core.c | 15 +--------
> drivers/input/touchscreen/elants_i2c.c | 13 ++------
> drivers/input/touchscreen/raydium_i2c_ts.c | 7 +---
> drivers/pnp/pnpacpi/rsparser.c | 7 ++--
> include/linux/acpi.h | 23 +++++++++++---
> include/linux/ioport.h | 3 +-
> 16 files changed, 112 insertions(+), 92 deletions(-)
>
> --
> 2.37.3.968.ga6b4b080e4-goog
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists