From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mika Westerberg Subject: Re: [PATCH 5/6] gpio / ACPI: Rework ACPI GPIO event handling Date: Wed, 26 Feb 2014 11:10:34 +0200 Message-ID: <20140226091034.GX5018@intel.com> References: <1393257611-18031-1-git-send-email-mika.westerberg@linux.intel.com> <1393257611-18031-6-git-send-email-mika.westerberg@linux.intel.com> <1538177.2S9uyJK3my@vostro.rjw.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Content-Disposition: inline In-Reply-To: <1538177.2S9uyJK3my@vostro.rjw.lan> Sender: linux-kernel-owner@vger.kernel.org To: "Rafael J. Wysocki" Cc: Linus Walleij , Alexandre Courbot , Lan Tianyu , Lv Zheng , Alan Cox , Mathias Nyman , linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org List-Id: linux-acpi@vger.kernel.org On Tue, Feb 25, 2014 at 03:44:52PM +0100, Rafael J. Wysocki wrote: > 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 > > --- > > 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? Yes. > > > > 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 Thanks!