| Message ID | 20230218103235.6934-2-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI: x86: Introduce an acpi_quirk_skip_gpio_event_handlers() helper | expand |
On Sat, Feb 18, 2023 at 11:32:33AM +0100, Hans de Goede wrote: > x86 ACPI boards which ship with only Android as their factory image usually > have pretty broken ACPI tables, relying on everything being hardcoded in > the factory kernel image and often disabling parts of the ACPI enumeration > kernel code to avoid the broken tables causing issues. > > Part of this broken ACPI code is that sometimes these boards have _AEI > ACPI GPIO event handlers which are broken. > > So far this has been dealt with in the platform/x86/x86-android-tablets.c > module, which contains various workarounds for these devices, by it calling > acpi_gpiochip_free_interrupts() on gpiochip-s with troublesome handlers to > disable the handlers. > > But in some cases this is too late, if the handlers are of the edge type > then gpiolib-acpi.c's code will already have run them at boot. > This can cause issues such as GPIOs ending up as owned by "ACPI:OpRegion", > making them unavailable for drivers which actually need them. > > Boards with these broken ACPI tables are already listed in > drivers/acpi/x86/utils.c for e.g. acpi_quirk_skip_i2c_client_enumeration(). > Extend the quirks mechanism for a new acpi_quirk_skip_gpio_event_handlers() > helper, this re-uses the DMI-ids rather then having to duplicate the same > DMI table in gpiolib-acpi.c . > > Also add the new ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS quirk to existing > boards with troublesome ACPI gpio event handlers, so that the current > acpi_gpiochip_free_interrupts() hack can be removed from > x86-android-tablets.c . I'm wondering if we can teach acpi_gpio_in_ignore_list() to handle this. P.S. Why do we lock an IRQ before checking acpi_gpio_in_ignore_list() and why do we not free that if the IRQ is in ignore list?
Hi, On 2/20/23 14:34, Andy Shevchenko wrote: > On Sat, Feb 18, 2023 at 11:32:33AM +0100, Hans de Goede wrote: >> x86 ACPI boards which ship with only Android as their factory image usually >> have pretty broken ACPI tables, relying on everything being hardcoded in >> the factory kernel image and often disabling parts of the ACPI enumeration >> kernel code to avoid the broken tables causing issues. >> >> Part of this broken ACPI code is that sometimes these boards have _AEI >> ACPI GPIO event handlers which are broken. >> >> So far this has been dealt with in the platform/x86/x86-android-tablets.c >> module, which contains various workarounds for these devices, by it calling >> acpi_gpiochip_free_interrupts() on gpiochip-s with troublesome handlers to >> disable the handlers. >> >> But in some cases this is too late, if the handlers are of the edge type >> then gpiolib-acpi.c's code will already have run them at boot. >> This can cause issues such as GPIOs ending up as owned by "ACPI:OpRegion", >> making them unavailable for drivers which actually need them. >> >> Boards with these broken ACPI tables are already listed in >> drivers/acpi/x86/utils.c for e.g. acpi_quirk_skip_i2c_client_enumeration(). >> Extend the quirks mechanism for a new acpi_quirk_skip_gpio_event_handlers() >> helper, this re-uses the DMI-ids rather then having to duplicate the same >> DMI table in gpiolib-acpi.c . >> >> Also add the new ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS quirk to existing >> boards with troublesome ACPI gpio event handlers, so that the current >> acpi_gpiochip_free_interrupts() hack can be removed from >> x86-android-tablets.c . > > I'm wondering if we can teach acpi_gpio_in_ignore_list() to handle this. You mean have it call acpi_quirk_skip_gpio_event_handlers(), or you mean extend the DMI matchs inside drivers/gpio/gpiolib-acpi.c to cover these cases ? These devices with severely broken DSDTs already need a bunch of other acpi handling quirks. So the idea is to re-use the existing quirk mechanism for these to avoid having to have DMI match table entries for a single model in various different places. > P.S. Why do we lock an IRQ before checking acpi_gpio_in_ignore_list() and > why do we not free that if the IRQ is in ignore list? The idea was to do the test after other things which can fail, so that if there are other reasons to skip the GPIO we don't do the test + dev_xxx msg. But you are right, we should either unlock it when ignoring it, or move the acpi_gpio_in_ignore_list() list check up. I guess just moving the check up is better, shall I prepare a patch for this? Regards, Hans
On Mon, Feb 20, 2023 at 04:23:33PM +0100, Hans de Goede wrote: > On 2/20/23 14:34, Andy Shevchenko wrote: > > On Sat, Feb 18, 2023 at 11:32:33AM +0100, Hans de Goede wrote: > >> x86 ACPI boards which ship with only Android as their factory image usually > >> have pretty broken ACPI tables, relying on everything being hardcoded in > >> the factory kernel image and often disabling parts of the ACPI enumeration > >> kernel code to avoid the broken tables causing issues. > >> > >> Part of this broken ACPI code is that sometimes these boards have _AEI > >> ACPI GPIO event handlers which are broken. > >> > >> So far this has been dealt with in the platform/x86/x86-android-tablets.c > >> module, which contains various workarounds for these devices, by it calling > >> acpi_gpiochip_free_interrupts() on gpiochip-s with troublesome handlers to > >> disable the handlers. > >> > >> But in some cases this is too late, if the handlers are of the edge type > >> then gpiolib-acpi.c's code will already have run them at boot. > >> This can cause issues such as GPIOs ending up as owned by "ACPI:OpRegion", > >> making them unavailable for drivers which actually need them. > >> > >> Boards with these broken ACPI tables are already listed in > >> drivers/acpi/x86/utils.c for e.g. acpi_quirk_skip_i2c_client_enumeration(). > >> Extend the quirks mechanism for a new acpi_quirk_skip_gpio_event_handlers() > >> helper, this re-uses the DMI-ids rather then having to duplicate the same > >> DMI table in gpiolib-acpi.c . > >> > >> Also add the new ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS quirk to existing > >> boards with troublesome ACPI gpio event handlers, so that the current > >> acpi_gpiochip_free_interrupts() hack can be removed from > >> x86-android-tablets.c . > > > > I'm wondering if we can teach acpi_gpio_in_ignore_list() to handle this. > > You mean have it call acpi_quirk_skip_gpio_event_handlers(), or you mean > extend the DMI matchs inside drivers/gpio/gpiolib-acpi.c to cover these > cases ? > > These devices with severely broken DSDTs already need a bunch of > other acpi handling quirks. So the idea is to re-use the existing > quirk mechanism for these to avoid having to have DMI match table > entries for a single model in various different places. I don't like growing amount of compile dependencies between these modules. (Yes, I'm aware about stubs.) Can we maybe move other quirks out from gpiolib-acpi.c to something like PDx86 or another existing board files (with quirks)? > > P.S. Why do we lock an IRQ before checking acpi_gpio_in_ignore_list() and > > why do we not free that if the IRQ is in ignore list? > > The idea was to do the test after other things which can fail, so that > if there are other reasons to skip the GPIO we don't do the test + > dev_xxx msg. But you are right, we should either unlock it when ignoring > it, or move the acpi_gpio_in_ignore_list() list check up. > > I guess just moving the check up is better, shall I prepare a patch for this? Yes, please.
Hi, On 2/20/23 16:35, Andy Shevchenko wrote: > On Mon, Feb 20, 2023 at 04:23:33PM +0100, Hans de Goede wrote: >> On 2/20/23 14:34, Andy Shevchenko wrote: >>> On Sat, Feb 18, 2023 at 11:32:33AM +0100, Hans de Goede wrote: >>>> x86 ACPI boards which ship with only Android as their factory image usually >>>> have pretty broken ACPI tables, relying on everything being hardcoded in >>>> the factory kernel image and often disabling parts of the ACPI enumeration >>>> kernel code to avoid the broken tables causing issues. >>>> >>>> Part of this broken ACPI code is that sometimes these boards have _AEI >>>> ACPI GPIO event handlers which are broken. >>>> >>>> So far this has been dealt with in the platform/x86/x86-android-tablets.c >>>> module, which contains various workarounds for these devices, by it calling >>>> acpi_gpiochip_free_interrupts() on gpiochip-s with troublesome handlers to >>>> disable the handlers. >>>> >>>> But in some cases this is too late, if the handlers are of the edge type >>>> then gpiolib-acpi.c's code will already have run them at boot. >>>> This can cause issues such as GPIOs ending up as owned by "ACPI:OpRegion", >>>> making them unavailable for drivers which actually need them. >>>> >>>> Boards with these broken ACPI tables are already listed in >>>> drivers/acpi/x86/utils.c for e.g. acpi_quirk_skip_i2c_client_enumeration(). >>>> Extend the quirks mechanism for a new acpi_quirk_skip_gpio_event_handlers() >>>> helper, this re-uses the DMI-ids rather then having to duplicate the same >>>> DMI table in gpiolib-acpi.c . >>>> >>>> Also add the new ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS quirk to existing >>>> boards with troublesome ACPI gpio event handlers, so that the current >>>> acpi_gpiochip_free_interrupts() hack can be removed from >>>> x86-android-tablets.c . >>> >>> I'm wondering if we can teach acpi_gpio_in_ignore_list() to handle this. >> >> You mean have it call acpi_quirk_skip_gpio_event_handlers(), or you mean >> extend the DMI matchs inside drivers/gpio/gpiolib-acpi.c to cover these >> cases ? >> >> These devices with severely broken DSDTs already need a bunch of >> other acpi handling quirks. So the idea is to re-use the existing >> quirk mechanism for these to avoid having to have DMI match table >> entries for a single model in various different places. > > I don't like growing amount of compile dependencies between these modules. > (Yes, I'm aware about stubs.) gpiolib-acpi.c already depends on CONFIG_ACPI and is not build when this is not set. So this does not add any new dependencies. IOW I don't see the problem here ? (also for this reason there is no stub for the new acpi_quirk_skip_gpio_event_handlers() helper) > Can we maybe move other quirks out from gpiolib-acpi.c to something like > PDx86 or another existing board files (with quirks)? I don't really see a clean way to move these. >>> P.S. Why do we lock an IRQ before checking acpi_gpio_in_ignore_list() and >>> why do we not free that if the IRQ is in ignore list? >> >> The idea was to do the test after other things which can fail, so that >> if there are other reasons to skip the GPIO we don't do the test + >> dev_xxx msg. But you are right, we should either unlock it when ignoring >> it, or move the acpi_gpio_in_ignore_list() list check up. >> >> I guess just moving the check up is better, shall I prepare a patch for this? > > Yes, please. Ok will do. Regards, hans
diff --git a/drivers/acpi/x86/utils.c b/drivers/acpi/x86/utils.c index 4e816bb402f6..4a6f3a6726d0 100644 --- a/drivers/acpi/x86/utils.c +++ b/drivers/acpi/x86/utils.c @@ -262,6 +262,7 @@ bool force_storage_d3(void) #define ACPI_QUIRK_UART1_TTY_UART2_SKIP BIT(1) #define ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY BIT(2) #define ACPI_QUIRK_USE_ACPI_AC_AND_BATTERY BIT(3) +#define ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS BIT(4) static const struct dmi_system_id acpi_quirk_skip_dmi_ids[] = { /* @@ -297,7 +298,8 @@ static const struct dmi_system_id acpi_quirk_skip_dmi_ids[] = { }, .driver_data = (void *)(ACPI_QUIRK_SKIP_I2C_CLIENTS | ACPI_QUIRK_UART1_TTY_UART2_SKIP | - ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY), + ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY | + ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS), }, { .matches = { @@ -305,7 +307,8 @@ static const struct dmi_system_id acpi_quirk_skip_dmi_ids[] = { DMI_MATCH(DMI_PRODUCT_NAME, "TF103C"), }, .driver_data = (void *)(ACPI_QUIRK_SKIP_I2C_CLIENTS | - ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY), + ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY | + ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS), }, { /* Lenovo Yoga Tablet 2 1050F/L */ @@ -347,7 +350,8 @@ static const struct dmi_system_id acpi_quirk_skip_dmi_ids[] = { DMI_MATCH(DMI_PRODUCT_NAME, "M890BAP"), }, .driver_data = (void *)(ACPI_QUIRK_SKIP_I2C_CLIENTS | - ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY), + ACPI_QUIRK_SKIP_ACPI_AC_AND_BATTERY | + ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS), }, { /* Whitelabel (sold as various brands) TM800A550L */ @@ -424,6 +428,20 @@ int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *s return 0; } EXPORT_SYMBOL_GPL(acpi_quirk_skip_serdev_enumeration); + +bool acpi_quirk_skip_gpio_event_handlers(void) +{ + const struct dmi_system_id *dmi_id; + long quirks; + + dmi_id = dmi_first_match(acpi_quirk_skip_dmi_ids); + if (!dmi_id) + return false; + + quirks = (unsigned long)dmi_id->driver_data; + return (quirks & ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS); +} +EXPORT_SYMBOL_GPL(acpi_quirk_skip_gpio_event_handlers); #endif /* Lists of PMIC ACPI HIDs with an (often better) native charger driver */ diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index 17c53f484280..6041768bb72b 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -536,6 +536,9 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) if (ACPI_FAILURE(status)) return; + if (acpi_quirk_skip_gpio_event_handlers()) + return; + acpi_walk_resources(handle, METHOD_NAME__AEI, acpi_gpiochip_alloc_event, acpi_gpio); diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index e44be31115a6..d69545cd6a48 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -656,6 +656,7 @@ static inline bool acpi_quirk_skip_acpi_ac_and_battery(void) #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS) bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev); int acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip); +bool acpi_quirk_skip_gpio_event_handlers(void); #else static inline bool acpi_quirk_skip_i2c_client_enumeration(struct acpi_device *adev) { @@ -667,6 +668,10 @@ acpi_quirk_skip_serdev_enumeration(struct device *controller_parent, bool *skip) *skip = false; return 0; } +static inline bool acpi_quirk_skip_gpio_event_handlers(void) +{ + return false; +} #endif #ifdef CONFIG_PM
x86 ACPI boards which ship with only Android as their factory image usually have pretty broken ACPI tables, relying on everything being hardcoded in the factory kernel image and often disabling parts of the ACPI enumeration kernel code to avoid the broken tables causing issues. Part of this broken ACPI code is that sometimes these boards have _AEI ACPI GPIO event handlers which are broken. So far this has been dealt with in the platform/x86/x86-android-tablets.c module, which contains various workarounds for these devices, by it calling acpi_gpiochip_free_interrupts() on gpiochip-s with troublesome handlers to disable the handlers. But in some cases this is too late, if the handlers are of the edge type then gpiolib-acpi.c's code will already have run them at boot. This can cause issues such as GPIOs ending up as owned by "ACPI:OpRegion", making them unavailable for drivers which actually need them. Boards with these broken ACPI tables are already listed in drivers/acpi/x86/utils.c for e.g. acpi_quirk_skip_i2c_client_enumeration(). Extend the quirks mechanism for a new acpi_quirk_skip_gpio_event_handlers() helper, this re-uses the DMI-ids rather then having to duplicate the same DMI table in gpiolib-acpi.c . Also add the new ACPI_QUIRK_SKIP_GPIO_EVENT_HANDLERS quirk to existing boards with troublesome ACPI gpio event handlers, so that the current acpi_gpiochip_free_interrupts() hack can be removed from x86-android-tablets.c . Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/acpi/x86/utils.c | 24 +++++++++++++++++++++--- drivers/gpio/gpiolib-acpi.c | 3 +++ include/acpi/acpi_bus.h | 5 +++++ 3 files changed, 29 insertions(+), 3 deletions(-)