On Monday, February 24, 2014 06:00:10 PM Mika Westerberg wrote:
> The current ACPI GPIO event handling code was never tested against real
> hardware with functioning GPIO triggered events (at the time such hardware
> wasn't available). Thus it misses certain things like requesting the GPIOs
> properly, passing correct flags to the interrupt handler and so on.
>
> This patch reworks ACPI GPIO event handling so that we:
>
> 1) Use struct acpi_gpio_event for all GPIO signaled events.
> 2) Switch to use GPIO descriptor API and request GPIOs by calling
> gpiochip_request_own_desc() that we added in a previous patch.
> 3) Pass proper flags from ACPI GPIO resource to request_threaded_irq().
>
> Also instead of open-coding the _AEI iteration loop we can use
> acpi_walk_resources(). This simplifies the code a bit and fixes memory leak
> that was caused by missing kfree() for buffer returned by
> acpi_get_event_resources().
>
> Since the remove path now calls gpiochip_free_own_desc() which takes GPIO
> spinlock we need to call acpi_gpiochip_remove() outside of that lock
> (analogous to acpi_gpiochip_add() path where the lock is released before
> those funtions are called).
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> ---
> drivers/gpio/gpiolib-acpi.c | 221 ++++++++++++++++++++++++++------------------
> drivers/gpio/gpiolib.c | 3 +-
> 2 files changed, 132 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c60db4ddc166..275735f390af 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -21,7 +21,7 @@
>
> struct acpi_gpio_event {
> struct list_head node;
> - acpi_handle *evt_handle;
> + acpi_handle evt_handle;
> unsigned int pin;
> unsigned int irq;
> };
> @@ -70,9 +70,9 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>
> static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> {
> - acpi_handle handle = data;
> + struct acpi_gpio_event *event = data;
>
> - acpi_evaluate_object(handle, NULL, NULL, NULL);
> + acpi_evaluate_object(event->evt_handle, NULL, NULL, NULL);
This is a threaded irq handler, isn't it?
>
> return IRQ_HANDLED;
> }
> @@ -91,6 +91,120 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
> /* The address of this function is used as a key. */
> }
>
> +static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> + void *context)
> +{
> + struct acpi_gpio_chip *achip = context;
> + struct gpio_chip *chip = achip->chip;
> + struct acpi_resource_gpio *agpio;
> + acpi_handle handle, evt_handle;
> + struct acpi_gpio_event *event;
> + irq_handler_t handler = NULL;
> + struct gpio_desc *desc;
> + unsigned long irqflags;
> + int ret, pin, irq;
> +
> + if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> + return AE_OK;
> +
> + agpio = &ares->data.gpio;
> + if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
> + return AE_OK;
> +
> + handle = ACPI_HANDLE(chip->dev);
> + pin = agpio->pin_table[0];
> +
> + if (pin <= 255) {
> + char ev_name[5];
> + sprintf(ev_name, "_%c%02X",
> + agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> + pin);
> + if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
> + handler = acpi_gpio_irq_handler;
> + }
> + if (!handler) {
> + if (ACPI_SUCCESS(acpi_get_handle(handle, "_EVT", &evt_handle)))
> + handler = acpi_gpio_irq_handler_evt;
> + }
> + if (!handler)
> + return AE_BAD_PARAMETER;
> +
> + desc = gpiochip_get_desc(chip, pin);
> + if (IS_ERR(desc)) {
> + dev_err(chip->dev, "Failed to get GPIO descriptor\n");
> + return AE_ERROR;
> + }
> +
> + ret = gpiochip_request_own_desc(desc, "ACPI:Event");
> + if (ret) {
> + dev_err(chip->dev, "Failed to request GPIO\n");
> + return AE_ERROR;
> + }
> +
> + gpiod_direction_input(desc);
> +
> + ret = gpiod_lock_as_irq(desc);
> + if (ret) {
> + dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
> + goto fail_free_desc;
> + }
> +
> + irq = gpiod_to_irq(desc);
> + if (irq < 0) {
> + dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
> + goto fail_unlock_irq;
> + }
> +
> + irqflags = IRQF_ONESHOT;
> + if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
> + if (agpio->polarity == ACPI_ACTIVE_HIGH)
> + irqflags |= IRQF_TRIGGER_HIGH;
> + else
> + irqflags |= IRQF_TRIGGER_LOW;
> + } else {
> + switch (agpio->polarity) {
> + case ACPI_ACTIVE_HIGH:
> + irqflags |= IRQF_TRIGGER_RISING;
> + break;
> + case ACPI_ACTIVE_LOW:
> + irqflags |= IRQF_TRIGGER_FALLING;
> + break;
> + default:
> + irqflags |= IRQF_TRIGGER_RISING |
> + IRQF_TRIGGER_FALLING;
> + break;
> + }
> + }
> +
> + event = kzalloc(sizeof(*event), GFP_KERNEL);
> + if (!event)
> + goto fail_unlock_irq;
> +
> + event->evt_handle = evt_handle;
> + event->irq = irq;
> + event->pin = pin;
> +
> + ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
> + "ACPI:Event", event);
> + if (ret) {
> + dev_err(chip->dev, "Failed to setup interrupt handler for %d\n",
> + event->irq);
> + goto fail_free_event;
> + }
> +
> + list_add_tail(&event->node, &achip->events);
> + return AE_OK;
> +
> +fail_free_event:
> + kfree(event);
> +fail_unlock_irq:
> + gpiod_unlock_as_irq(desc);
> +fail_free_desc:
> + gpiochip_free_own_desc(desc);
> +
> + return AE_ERROR;
> +}
> +
> /**
> * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
> * @achip: ACPI GPIO chip
> @@ -103,104 +217,22 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
> */
> static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
> {
> - struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> struct gpio_chip *chip = achip->chip;
> - struct acpi_resource *res;
> - acpi_handle handle;
> - acpi_status status;
> - unsigned int pin;
> - int irq, ret;
> - char ev_name[5];
>
> if (!chip->dev || !chip->to_irq)
> return;
>
> - handle = ACPI_HANDLE(chip->dev);
> - if (!handle)
> - return;
> -
> INIT_LIST_HEAD(&achip->events);
> -
> - status = acpi_get_event_resources(handle, &buf);
> - if (ACPI_FAILURE(status))
> - return;
> -
> - /*
> - * If a GPIO interrupt has an ACPI event handler method, or _EVT is
> - * present, set up an interrupt handler that calls the ACPI event
> - * handler.
> - */
> - for (res = buf.pointer;
> - res && (res->type != ACPI_RESOURCE_TYPE_END_TAG);
> - res = ACPI_NEXT_RESOURCE(res)) {
> - irq_handler_t handler = NULL;
> - void *data;
> -
> - if (res->type != ACPI_RESOURCE_TYPE_GPIO ||
> - res->data.gpio.connection_type !=
> - ACPI_RESOURCE_GPIO_TYPE_INT)
> - continue;
> -
> - pin = res->data.gpio.pin_table[0];
> - if (pin > chip->ngpio)
> - continue;
> -
> - irq = chip->to_irq(chip, pin);
> - if (irq < 0)
> - continue;
> -
> - if (pin <= 255) {
> - acpi_handle ev_handle;
> -
> - sprintf(ev_name, "_%c%02X",
> - res->data.gpio.triggering ? 'E' : 'L', pin);
> - status = acpi_get_handle(handle, ev_name, &ev_handle);
> - if (ACPI_SUCCESS(status)) {
> - handler = acpi_gpio_irq_handler;
> - data = ev_handle;
> - }
> - }
> - if (!handler) {
> - struct acpi_gpio_event *event;
> - acpi_handle evt_handle;
> -
> - status = acpi_get_handle(handle, "_EVT", &evt_handle);
> - if (ACPI_FAILURE(status))
> - continue;
> -
> - event = kzalloc(sizeof(*event), GFP_KERNEL);
> - if (!event)
> - continue;
> -
> - list_add_tail(&event->node, &achip->events);
> - event->evt_handle = evt_handle;
> - event->pin = pin;
> - event->irq = irq;
> - handler = acpi_gpio_irq_handler_evt;
> - data = event;
> - }
> - if (!handler)
> - continue;
> -
> - /* Assume BIOS sets the triggering, so no flags */
> - ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
> - 0, "GPIO-signaled-ACPI-event",
> - data);
> - if (ret)
> - dev_err(chip->dev,
> - "Failed to request IRQ %d ACPI event handler\n",
> - irq);
> - }
> + acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
> + acpi_gpiochip_request_interrupt, achip);
> }
>
> /**
> - * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> + * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
> * @achip: ACPI GPIO chip
> *
> - * Free interrupts associated with the _EVT method for the given GPIO chip.
> - *
> - * The remaining ACPI event interrupts associated with the chip are freed
> - * automatically.
> + * Free interrupts associated with GPIO ACPI event method for the given
> + * GPIO chip.
> */
> static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> {
> @@ -211,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> return;
>
> list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
> - devm_free_irq(chip->dev, event->irq, event);
> + struct gpio_desc *desc;
> +
> + free_irq(event->irq, event);
> + desc = gpiochip_get_desc(chip, event->pin);
> + if (WARN_ON(IS_ERR(desc)))
> + continue;
> + gpiod_unlock_as_irq(desc);
> + gpiochip_free_own_desc(desc);
> list_del(&event->node);
> kfree(event);
> }
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 489a63524eb6..474f7d1eb7d7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1266,11 +1266,12 @@ int gpiochip_remove(struct gpio_chip *chip)
> int status = 0;
> unsigned id;
>
> + acpi_gpiochip_remove(chip);
> +
> spin_lock_irqsave(&gpio_lock, flags);
>
> gpiochip_remove_pin_ranges(chip);
> of_gpiochip_remove(chip);
> - acpi_gpiochip_remove(chip);
>
> for (id = 0; id < chip->ngpio; id++) {
> if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
>
Looks good to me.
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
