* [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
@ 2023-08-10 18:48 Bartosz Golaszewski
2023-08-10 18:57 ` Bartosz Golaszewski
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2023-08-10 18:48 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
After we remove a GPIO chip that still has some requested descriptors,
gpiod_free_commit() will fail and we will never put the references to the
GPIO device and the owning module in gpiod_free().
Rework this function to:
- not warn on desc == NULL as this is a use-case on which most free
functions silently return
- put the references to desc->gdev and desc->gdev->owner unconditionally
so that the release callback actually gets called when the remaining
references are dropped by external GPIO users
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
v1 -> v2:
- add a comment about why we can't use VALIDATE_DESC_VOID()
drivers/gpio/gpiolib.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 251c875b5c34..2158067c4fd2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
void gpiod_free(struct gpio_desc *desc)
{
- if (desc && desc->gdev && gpiod_free_commit(desc)) {
- module_put(desc->gdev->owner);
- gpio_device_put(desc->gdev);
- } else {
+ /*
+ * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
+ * may already be NULL but we still want to put the references.
+ */
+ if (!desc)
+ return;
+
+ if (!gpiod_free_commit(desc))
WARN_ON(extra_checks);
- }
+
+ gpio_device_put(desc->gdev);
+ module_put(desc->gdev->owner);
}
/**
--
2.39.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
2023-08-10 18:48 [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use Bartosz Golaszewski
@ 2023-08-10 18:57 ` Bartosz Golaszewski
2023-08-11 7:16 ` Linus Walleij
2023-08-11 9:12 ` Andy Shevchenko
2023-08-11 11:57 ` Bartosz Golaszewski
2 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2023-08-10 18:57 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> After we remove a GPIO chip that still has some requested descriptors,
> gpiod_free_commit() will fail and we will never put the references to the
> GPIO device and the owning module in gpiod_free().
>
> Rework this function to:
> - not warn on desc == NULL as this is a use-case on which most free
> functions silently return
> - put the references to desc->gdev and desc->gdev->owner unconditionally
> so that the release callback actually gets called when the remaining
> references are dropped by external GPIO users
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
I forgot to pick up Linus' tag. I'll do it when applying. I should
really start using b4...
Bart
> ---
> v1 -> v2:
> - add a comment about why we can't use VALIDATE_DESC_VOID()
>
> drivers/gpio/gpiolib.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 251c875b5c34..2158067c4fd2 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>
> void gpiod_free(struct gpio_desc *desc)
> {
> - if (desc && desc->gdev && gpiod_free_commit(desc)) {
> - module_put(desc->gdev->owner);
> - gpio_device_put(desc->gdev);
> - } else {
> + /*
> + * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
> + * may already be NULL but we still want to put the references.
> + */
> + if (!desc)
> + return;
> +
> + if (!gpiod_free_commit(desc))
> WARN_ON(extra_checks);
> - }
> +
> + gpio_device_put(desc->gdev);
> + module_put(desc->gdev->owner);
> }
>
> /**
> --
> 2.39.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
2023-08-10 18:57 ` Bartosz Golaszewski
@ 2023-08-11 7:16 ` Linus Walleij
0 siblings, 0 replies; 6+ messages in thread
From: Linus Walleij @ 2023-08-11 7:16 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Andy Shevchenko, Kent Gibson, linux-gpio, linux-kernel,
Bartosz Golaszewski
On Thu, Aug 10, 2023 at 8:58 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> I forgot to pick up Linus' tag. I'll do it when applying. I should
> really start using b4...
Well with b4 you still have to issue b4 trailers -u, I almost want to
make a cron script that just loop over my b4 branches and do that
every night when I'm asleep so tags get picked up in case I miss
them.
Hm, maybe not such a bad idea.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
2023-08-10 18:48 [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use Bartosz Golaszewski
2023-08-10 18:57 ` Bartosz Golaszewski
@ 2023-08-11 9:12 ` Andy Shevchenko
2023-08-11 11:57 ` Bartosz Golaszewski
2 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2023-08-11 9:12 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linus Walleij, Kent Gibson, linux-gpio, linux-kernel,
Bartosz Golaszewski
On Thu, Aug 10, 2023 at 08:48:46PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> After we remove a GPIO chip that still has some requested descriptors,
> gpiod_free_commit() will fail and we will never put the references to the
> GPIO device and the owning module in gpiod_free().
>
> Rework this function to:
> - not warn on desc == NULL as this is a use-case on which most free
> functions silently return
> - put the references to desc->gdev and desc->gdev->owner unconditionally
> so that the release callback actually gets called when the remaining
> references are dropped by external GPIO users
...
> + /*
> + * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
> + * may already be NULL but we still want to put the references.
> + */
Clear now!
...
> + if (!gpiod_free_commit(desc))
> WARN_ON(extra_checks);
/* Always call gpiod_free_commit() */
WARN_ON(!gpiod_free_commit(desc) && extra_checks);
?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
2023-08-10 18:48 [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use Bartosz Golaszewski
2023-08-10 18:57 ` Bartosz Golaszewski
2023-08-11 9:12 ` Andy Shevchenko
@ 2023-08-11 11:57 ` Bartosz Golaszewski
2023-08-11 19:27 ` Bartosz Golaszewski
2 siblings, 1 reply; 6+ messages in thread
From: Bartosz Golaszewski @ 2023-08-11 11:57 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> After we remove a GPIO chip that still has some requested descriptors,
> gpiod_free_commit() will fail and we will never put the references to the
> GPIO device and the owning module in gpiod_free().
>
> Rework this function to:
> - not warn on desc == NULL as this is a use-case on which most free
> functions silently return
> - put the references to desc->gdev and desc->gdev->owner unconditionally
> so that the release callback actually gets called when the remaining
> references are dropped by external GPIO users
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> v1 -> v2:
> - add a comment about why we can't use VALIDATE_DESC_VOID()
>
> drivers/gpio/gpiolib.c | 16 +++++++++++-----
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 251c875b5c34..2158067c4fd2 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>
> void gpiod_free(struct gpio_desc *desc)
> {
> - if (desc && desc->gdev && gpiod_free_commit(desc)) {
> - module_put(desc->gdev->owner);
> - gpio_device_put(desc->gdev);
> - } else {
> + /*
> + * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
> + * may already be NULL but we still want to put the references.
> + */
> + if (!desc)
> + return;
> +
> + if (!gpiod_free_commit(desc))
> WARN_ON(extra_checks);
> - }
> +
> + gpio_device_put(desc->gdev);
> + module_put(desc->gdev->owner);
> }
>
> /**
> --
> 2.39.2
>
Queued for fixes.
Bartosz
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use
2023-08-11 11:57 ` Bartosz Golaszewski
@ 2023-08-11 19:27 ` Bartosz Golaszewski
0 siblings, 0 replies; 6+ messages in thread
From: Bartosz Golaszewski @ 2023-08-11 19:27 UTC (permalink / raw)
To: Linus Walleij, Andy Shevchenko, Kent Gibson
Cc: linux-gpio, linux-kernel, Bartosz Golaszewski
On Fri, Aug 11, 2023 at 1:57 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > After we remove a GPIO chip that still has some requested descriptors,
> > gpiod_free_commit() will fail and we will never put the references to the
> > GPIO device and the owning module in gpiod_free().
> >
> > Rework this function to:
> > - not warn on desc == NULL as this is a use-case on which most free
> > functions silently return
> > - put the references to desc->gdev and desc->gdev->owner unconditionally
> > so that the release callback actually gets called when the remaining
> > references are dropped by external GPIO users
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> > v1 -> v2:
> > - add a comment about why we can't use VALIDATE_DESC_VOID()
> >
> > drivers/gpio/gpiolib.c | 16 +++++++++++-----
> > 1 file changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 251c875b5c34..2158067c4fd2 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -2167,12 +2167,18 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
> >
> > void gpiod_free(struct gpio_desc *desc)
> > {
> > - if (desc && desc->gdev && gpiod_free_commit(desc)) {
> > - module_put(desc->gdev->owner);
> > - gpio_device_put(desc->gdev);
> > - } else {
> > + /*
> > + * We must not use VALIDATE_DESC_VOID() as the underlying gdev->chip
> > + * may already be NULL but we still want to put the references.
> > + */
> > + if (!desc)
> > + return;
> > +
> > + if (!gpiod_free_commit(desc))
> > WARN_ON(extra_checks);
> > - }
> > +
> > + gpio_device_put(desc->gdev);
> > + module_put(desc->gdev->owner);
> > }
> >
> > /**
> > --
> > 2.39.2
> >
>
> Queued for fixes.
>
> Bartosz
Eek, I need to back it out. I realized there's an issue with this
patch. I'll send a v3.
Bartosz
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-08-11 19:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-10 18:48 [PATCH v2] gpiolib: fix reference leaks when removing GPIO chips still in use Bartosz Golaszewski
2023-08-10 18:57 ` Bartosz Golaszewski
2023-08-11 7:16 ` Linus Walleij
2023-08-11 9:12 ` Andy Shevchenko
2023-08-11 11:57 ` Bartosz Golaszewski
2023-08-11 19:27 ` Bartosz Golaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).