Re: [PATCH 1/3] ACPI: x86: Introduce an acpi_quirk_skip_gpio_event_handlers() helper

[Hans de Goede <hdegoede@redhat.com>]

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