[PATCH] gpiolib: Fix scope-based gpio_device refcounting

[PATCH] gpiolib: Fix scope-based gpio_device refcounting

[Lukas Wunner]

Commit 9e4555d1e54a ("gpiolib: add support for scope-based management to
gpio_device") sought to add scope-based gpio_device refcounting, but
erroneously forgot a negation of IS_ERR_OR_NULL().

As a result, gpio_device_put() is not called if the gpio_device pointer
is valid (meaning the ref is leaked), but only called if the pointer is
NULL or an ERR_PTR().

While at it drop a superfluous trailing semicolon.

Fixes: 9e4555d1e54a ("gpiolib: add support for scope-based management to gpio_device")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 include/linux/gpio/driver.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 0aed62f..f2878b3 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -614,7 +614,7 @@ struct gpio_device *gpio_device_find(void *data,
 void gpio_device_put(struct gpio_device *gdev);
 
 DEFINE_FREE(gpio_device_put, struct gpio_device *,
-	    if (IS_ERR_OR_NULL(_T)) gpio_device_put(_T));
+	    if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))
 
 struct device *gpio_device_to_device(struct gpio_device *gdev);
 
-- 
2.40.1

Re: [PATCH] gpiolib: Fix scope-based gpio_device refcounting

[Bartosz Golaszewski]

On Mon, Jan 15, 2024 at 4:05 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> Commit 9e4555d1e54a ("gpiolib: add support for scope-based management to
> gpio_device") sought to add scope-based gpio_device refcounting, but
> erroneously forgot a negation of IS_ERR_OR_NULL().
>

Ah good catch, thanks!

> As a result, gpio_device_put() is not called if the gpio_device pointer
> is valid (meaning the ref is leaked), but only called if the pointer is
> NULL or an ERR_PTR().
>
> While at it drop a superfluous trailing semicolon.
>

While not strictly needed here - I think it's better for readability
to have a semicolon following every statement. Any reasons for why
dropping it is better?

Bart

> Fixes: 9e4555d1e54a ("gpiolib: add support for scope-based management to gpio_device")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  include/linux/gpio/driver.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index 0aed62f..f2878b3 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -614,7 +614,7 @@ struct gpio_device *gpio_device_find(void *data,
>  void gpio_device_put(struct gpio_device *gdev);
>
>  DEFINE_FREE(gpio_device_put, struct gpio_device *,
> -           if (IS_ERR_OR_NULL(_T)) gpio_device_put(_T));
> +           if (!IS_ERR_OR_NULL(_T)) gpio_device_put(_T))
>
>  struct device *gpio_device_to_device(struct gpio_device *gdev);
>
> --
> 2.40.1
>

Re: [PATCH] gpiolib: Fix scope-based gpio_device refcounting

[Lukas Wunner]

On Mon, Jan 15, 2024 at 05:09:05PM +0100, Bartosz Golaszewski wrote:
> On Mon, Jan 15, 2024 at 4:05PM Lukas Wunner <lukas@wunner.de> wrote:
> > While at it drop a superfluous trailing semicolon.
> 
> While not strictly needed here - I think it's better for readability
> to have a semicolon following every statement. Any reasons for why
> dropping it is better?

I looked at all the DEFINE_FREE definitions in Linus' current master
and this appears to be the only one with the extraneous semicolon,
so one reason is consistency.  Another the avoidance of the illusion
that this is a proper C statement.

It's basically an empty statement so it doesn't hurt but it doesn't
provide any value either.

Thanks,

Lukas

Re: [PATCH] gpiolib: Fix scope-based gpio_device refcounting

[Bartosz Golaszewski]

On Mon, Jan 15, 2024 at 5:35 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Mon, Jan 15, 2024 at 05:09:05PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Jan 15, 2024 at 4:05PM Lukas Wunner <lukas@wunner.de> wrote:
> > > While at it drop a superfluous trailing semicolon.
> >
> > While not strictly needed here - I think it's better for readability
> > to have a semicolon following every statement. Any reasons for why
> > dropping it is better?
>
> I looked at all the DEFINE_FREE definitions in Linus' current master
> and this appears to be the only one with the extraneous semicolon,
> so one reason is consistency.  Another the avoidance of the illusion
> that this is a proper C statement.
>
> It's basically an empty statement so it doesn't hurt but it doesn't
> provide any value either.
>
> Thanks,
>
> Lukas

Fair enough, I picked it up.

Bartosz