From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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
On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
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
>
On Thu, Aug 10, 2023 at 8:58 PM Bartosz Golaszewski <[email protected]> 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
On Thu, Aug 10, 2023 at 08:48:46PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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
On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <[email protected]> wrote:
>
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
On Fri, Aug 11, 2023 at 1:57 PM Bartosz Golaszewski <[email protected]> wrote:
>
> On Thu, Aug 10, 2023 at 8:48 PM Bartosz Golaszewski <[email protected]> wrote:
> >
> > From: Bartosz Golaszewski <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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