On Mon, Nov 26, 2018 at 11:42:31AM +0100, Hans de Goede wrote:
> Commit 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event handlers
> from a late_initcall") deferred the entire acpi_gpiochip_request_interrupt
> call for each event resource.
>
> This means it also delays the gpiochip_request_own_desc(..., "ACPI:Event")
> call. This is a problem if some AML code reads the GPIO pin before we
> run the deferred acpi_gpiochip_request_interrupt, because in that case
> acpi_gpio_adr_space_handler() will already have called
> gpiochip_request_own_desc(..., "ACPI:OpRegion") causing the call from
> acpi_gpiochip_request_interrupt to fail with -EBUSY and we will fail to
> register an event handler.
>
> acpi_gpio_adr_space_handler is prepared for acpi_gpiochip_request_interrupt
> already having claimed the pin, but the other way around does not work.
>
> One example of a problem this causes, is the event handler for the OTG
> ID pin on a Prowise PT301 tablet not registering, keeping the port stuck
> in whatever mode it was in during boot and e.g. only allowing charging
> after a reboot.
>
> This commit fixes this by only deferring the request_irq call and the
> initial run of edge-triggered IRQs instead of deferring all of
> acpi_gpiochip_request_interrupt.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 78d3a92edbfb ("gpiolib-acpi: Register GpioInt ACPI event ...")
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-acpi.c | 136 +++++++++++++++++++-----------------
> 1 file changed, 73 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 55b72fbe1631..bda19373835a 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -22,8 +22,12 @@
> struct acpi_gpio_event {
> struct list_head node;
> acpi_handle handle;
> + irq_handler_t handler;
> unsigned int pin;
> unsigned int irq;
> + unsigned long irqflags;
> + bool irq_is_wake;
> + bool irq_requested;
It might be a good idea to add kernel-doc now for the struct since it
seems to be growing and it is not immediately clear what each field mean
anymore ;-)
> struct gpio_desc *desc;
> };
>
> @@ -49,10 +53,10 @@ struct acpi_gpio_chip {
>
> /*
> * For gpiochips which call acpi_gpiochip_request_interrupts() before late_init
> - * (so builtin drivers) we register the ACPI GpioInt event handlers from a
> + * (so builtin drivers) we register the ACPI GpioInt irq-handlers from a
> * late_initcall_sync handler, so that other builtin drivers can register their
> * OpRegions before the event handlers can run. This list contains gpiochips
> - * for which the acpi_gpiochip_request_interrupts() has been deferred.
> + * for which the acpi_gpiochip_request_irqs() call has been deferred.
> */
> static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
> static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
> @@ -133,8 +137,43 @@ bool acpi_gpio_get_irq_resource(struct acpi_resource *ares,
> }
> EXPORT_SYMBOL_GPL(acpi_gpio_get_irq_resource);
>
> -static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> - void *context)
> +static void acpi_gpiochip_request_irq(struct acpi_gpio_chip *acpi_gpio,
> + struct acpi_gpio_event *event)
> +{
> + int ret, value;
> +
> + ret = request_threaded_irq(event->irq, NULL, event->handler,
> + event->irqflags, "ACPI:Event", event);
> + if (ret) {
> + dev_err(acpi_gpio->chip->parent,
> + "Failed to setup interrupt handler for %d\n",
> + event->irq);
> + return;
> + }
> +
> + if (event->irq_is_wake)
> + enable_irq_wake(event->irq);
> +
> + event->irq_requested = true;
> +
> + /* Make sure we trigger the initial state of edge-triggered IRQs */
> + value = gpiod_get_raw_value_cansleep(event->desc);
> + if (((event->irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> + ((event->irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> + event->handler(event->irq, event);
> +}
> +
> +static void acpi_gpiochip_request_irqs(struct acpi_gpio_chip *acpi_gpio)
> +{
> + struct acpi_gpio_event *event;
> +
> + list_for_each_entry(event, &acpi_gpio->events, node)
> + acpi_gpiochip_request_irq(acpi_gpio, event);
> +}
> +
> +static acpi_status
> +acpi_gpiochip_request_interrupt_desc(struct acpi_resource *ares,
> + void *context)
I wonder if there could be better name for this now that it does not
request anything? Maybe acpi_gpiochip_event_alloc() or similar?
