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

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

[Marco Scardovi]

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.

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>
+
  #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);
+})
+
  /**
   * 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);
+        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;
  }

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

[Mika Westerberg]

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;
>  }
> 

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

[Andy Shevchenko]

On Wed, May 06, 2026 at 01:32:15PM +0200, Mika Westerberg wrote:

> +Andy

Thanks for Cc'ing me. Marco, you have to follow the MAINTAINERS database when
submitting this. For simplicity you may use my "smart" script [1] that does
almost right (it still uses heuristics but nobody complained so far) the Cc
lists.


> On Wed, May 06, 2026 at 11:29:36AM +0200, Marco Scardovi wrote:
> > 
> > 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.

As Mika said, please, split this to the per-logical change series.

> > I've tested it (both build and functionality) against linux-next-20260430.

This is assumed, but you can put that into the cover letter.

> > Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>

...

> > +#include <linux/cleanup.h>
> > +#include <linux/slab.h>
> > +
> 
> Drop the empty line.

And follow the existing order.

...

> > +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.

They are fine, but:

- they need to be just one liner as it's done elsewhere
- the {} are redundant (see existing examples)
- they need to be added per subsystem to their headers

(so, two more patches on top of whatever this one has to be split to for
 different subsystems).

...

> >      struct acpi_gpio_event *event;
> >      irq_handler_t handler = NULL;
> >      struct gpio_desc *desc;
> > +    struct gpio_desc *desc_guard __free(free_gpio_desc) = NULL;

This way of defining is highly discouraged. See below.

> >      desc = acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");

This one should be used otherwise as

	struct gpio_desc *desc __free(free_gpio_desc) =
		acpi_request_own_gpiod(chip, agpio, 0, "ACPI:Event");

...

> > -    struct acpi_resource *ares;
> > +    struct acpi_resource *ares __free(acpi_free) = NULL;

As per above.

...

[1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh

-- 
With Best Regards,
Andy Shevchenko