Re: [PATCH] gpio: acpi: modernize resource management using cleanup.h

[Mika Westerberg <mika.westerberg@linux.intel.com>]

Hi.

+Andy

On Wed, May 06, 2026 at 11:29:36AM +0200, Marco Scardovi wrote:
> Hi everyone,
> 
> I was looking for ways to switch to modern cleanup guards and auto-freeing
> pointers to simplify error paths and synchronization in gpiolib-acpi-core.c
> so I came up with the patch you can find below.
> 
> Here you can see the main points I've worked on:
> - Use DEFINE_FREE() for gpio_desc and ACPI resources.
> - Use guard(mutex)() within the OpRegion handler loop for automatic locking.
> - Use __free() for automatic descriptor and memory cleanup.
> - Fix off-by-one error in GPIO pin bounds check.
> - Return AE_OK on out-of-range pins to allow processing other resources
>   even if one is misconfigured in firmware.
> - Use break instead of goto in OpRegion handler for cleaner control flow
>   leveraging auto-cleanup.

That should be several patches not one doing all these.

> I've tested it (both build and functionality) against linux-next-20260430.
> 
> Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 94
> +++++++++++++++++++---------------------
>  1 file changed, 43 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c
> b/drivers/gpio/gpiolib-acpi-core.c
> index eb8a40cfb7a9..19a18222b7b2 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -7,6 +7,9 @@
>   *          Mika Westerberg <mika.westerberg@linux.intel.com>
>   */
> 
> +#include <linux/cleanup.h>
> +#include <linux/slab.h>
> +

Drop the empty line.

>  #include <linux/acpi.h>
>  #include <linux/dmi.h>
>  #include <linux/errno.h>
> @@ -23,6 +26,16 @@
>  #include "gpiolib.h"
>  #include "gpiolib-acpi.h"
> 
> +DEFINE_FREE(free_gpio_desc, struct gpio_desc *, {
> +    if (_T)
> +        gpiochip_free_own_desc(_T);
> +})
> +
> +DEFINE_FREE(acpi_free, void *, {
> +    if (_T)
> +        ACPI_FREE(_T);
> +})

These are white space damaged. Also I'm not big fan of these but if Andy is
fine then works for me too. However, please test with KASAN and kmemleak
enabled that you don't break anything.

> +
>  /**
>   * struct acpi_gpio_event - ACPI GPIO event handler data
>   *
> @@ -361,6 +374,7 @@ static acpi_status acpi_gpiochip_alloc_event(struct
> acpi_resource *ares,
>      struct acpi_gpio_event *event;
>      irq_handler_t handler = NULL;
>      struct gpio_desc *desc;
> +    struct gpio_desc *desc_guard __free(free_gpio_desc) = NULL;
>      unsigned int pin;
>      int ret, irq;
> 
> @@ -370,6 +384,11 @@ static acpi_status acpi_gpiochip_alloc_event(struct
> acpi_resource *ares,
>      handle = ACPI_HANDLE(chip->parent);
>      pin = agpio->pin_table[0];
> 
> +    if (pin >= chip->ngpio) {
> +        dev_err(chip->parent, "Failed to request GPIO for pin 0x%04X, out
> of range\n", pin);

This is damaged too.

Actually let's start with a proper patch and then look the details :)

> +        return AE_OK;
> +    }
> +
>      if (pin <= 255) {
>          char ev_name[8];
>          sprintf(ev_name, "_%c%02X",
> @@ -392,31 +411,26 @@ static acpi_status acpi_gpiochip_alloc_event(struct
> acpi_resource *ares,
> 
>      desc = acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");
>      if (IS_ERR(desc)) {
> -        dev_err(chip->parent,
> -            "Failed to request GPIO for pin 0x%04X, err %pe\n",
> -            pin, desc);
> +        dev_err(chip->parent, "Failed to request GPIO for pin 0x%04X, err
> %pe\n", pin, desc);
>          return AE_OK;
>      }
> +    desc_guard = desc;
> 
>      ret = gpiochip_lock_as_irq(chip, pin);
>      if (ret) {
> -        dev_err(chip->parent,
> -            "Failed to lock GPIO pin 0x%04X as interrupt, err %d\n",
> -            pin, ret);
> -        goto fail_free_desc;
> +        dev_err(chip->parent, "Failed to lock GPIO pin 0x%04X as interrupt,
> err %d\n", pin, ret);
> +        return AE_OK;
>      }
> 
>      irq = gpiod_to_irq(desc);
>      if (irq < 0) {
> -        dev_err(chip->parent,
> -            "Failed to translate GPIO pin 0x%04X to IRQ, err %d\n",
> -            pin, irq);
> -        goto fail_unlock_irq;
> +        dev_err(chip->parent, "Failed to translate GPIO pin 0x%04X to IRQ,
> err %d\n", pin, irq);
> +        goto err_unlock;
>      }
> 
>      event = kzalloc_obj(*event);
>      if (!event)
> -        goto fail_unlock_irq;
> +        goto err_unlock;
> 
>      event->irqflags = IRQF_ONESHOT;
>      if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
> @@ -444,17 +458,15 @@ static acpi_status acpi_gpiochip_alloc_event(struct
> acpi_resource *ares,
>      event->irq = irq;
>      event->irq_is_wake = acpi_gpio_irq_is_wake(chip->parent, agpio);
>      event->pin = pin;
> -    event->desc = desc;
> +    /* Transfer ownership to event, prevent auto-free */
> +    event->desc = no_free_ptr(desc_guard);
> 
>      list_add_tail(&event->node, &acpi_gpio->events);
> 
>      return AE_OK;
> 
> -fail_unlock_irq:
> +err_unlock:
>      gpiochip_unlock_as_irq(chip, pin);
> -fail_free_desc:
> -    gpiochip_free_own_desc(desc);
> -
>      return AE_OK;
>  }
> 
> @@ -1086,7 +1098,7 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>      struct acpi_gpio_chip *achip = region_context;
>      struct gpio_chip *chip = achip->chip;
>      struct acpi_resource_gpio *agpio;
> -    struct acpi_resource *ares;
> +    struct acpi_resource *ares __free(acpi_free) = NULL;
>      u16 pin_index = address;
>      acpi_status status;
>      int length;
> @@ -1097,20 +1109,17 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>      if (ACPI_FAILURE(status))
>          return status;
> 
> -    if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
> -        ACPI_FREE(ares);
> +    if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO))
>          return AE_BAD_PARAMETER;
> -    }
> 
>      agpio = &ares->data.gpio;
> 
>      if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
> -        function == ACPI_WRITE)) {
> -        ACPI_FREE(ares);
> +        function == ACPI_WRITE))
>          return AE_BAD_PARAMETER;
> -    }
> 
>      length = min(agpio->pin_table_length, pin_index + bits);
> +    status = AE_OK;
>      for (i = pin_index; i < length; ++i) {
>          unsigned int pin = agpio->pin_table[i];
>          struct acpi_gpio_connection *conn;
> @@ -1118,7 +1127,7 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>          u16 word, shift;
>          bool found;
> 
> -        mutex_lock(&achip->conn_lock);
> +        guard(mutex)(&achip->conn_lock);
> 
>          found = false;
>          list_for_each_entry(conn, &achip->conns, node) {
> @@ -1150,17 +1159,15 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>          if (!found) {
>              desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");
>              if (IS_ERR(desc)) {
> -                mutex_unlock(&achip->conn_lock);
>                  status = AE_ERROR;
> -                goto out;
> +                break;
>              }
> 
>              conn = kzalloc_obj(*conn);
>              if (!conn) {
>                  gpiochip_free_own_desc(desc);
> -                mutex_unlock(&achip->conn_lock);
>                  status = AE_NO_MEMORY;
> -                goto out;
> +                break;
>              }
> 
>              conn->pin = pin;
> @@ -1168,8 +1175,6 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>              list_add_tail(&conn->node, &achip->conns);
>          }
> 
> -        mutex_unlock(&achip->conn_lock);
> -
>          /*
>           * For the cases when OperationRegion() consists of more than
>           * 64 bits calculate the word and bit shift to use that one to
> @@ -1188,8 +1193,6 @@ acpi_gpio_adr_space_handler(u32 function,
> acpi_physical_address address,
>          }
>      }
> 
> -out:
> -    ACPI_FREE(ares);
>      return status;
>  }
>