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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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