[PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Liang He]

We should use of_node_get() when a new reference of device_node
is created. It is noted that the old reference stored in
'mm_gc->gc.of_node' should also be decreased.

Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")
Signed-off-by: Liang He <windhl@126.com>
---
 drivers/gpio/gpiolib-of.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 3d6c3ffd5576..de100b0217da 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -860,7 +860,8 @@ int of_mm_gpiochip_add_data(struct device_node *np,
 	if (mm_gc->save_regs)
 		mm_gc->save_regs(mm_gc);
 
-	mm_gc->gc.of_node = np;
+	of_node_put(mm_gc->gc.of_node);
+	mm_gc->gc.of_node = of_node_get(np);
 
 	ret = gpiochip_add_data(gc, data);
 	if (ret)
@@ -868,6 +869,7 @@ int of_mm_gpiochip_add_data(struct device_node *np,
 
 	return 0;
 err2:
+	of_node_put(np);
 	iounmap(mm_gc->regs);
 err1:
 	kfree(gc->label);
-- 
2.25.1

Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Bartosz Golaszewski]

On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:
>
> We should use of_node_get() when a new reference of device_node
> is created. It is noted that the old reference stored in
> 'mm_gc->gc.of_node' should also be decreased.
>
> Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")
> Signed-off-by: Liang He <windhl@126.com>
> ---
>  drivers/gpio/gpiolib-of.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 3d6c3ffd5576..de100b0217da 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -860,7 +860,8 @@ int of_mm_gpiochip_add_data(struct device_node *np,
>         if (mm_gc->save_regs)
>                 mm_gc->save_regs(mm_gc);
>
> -       mm_gc->gc.of_node = np;
> +       of_node_put(mm_gc->gc.of_node);
> +       mm_gc->gc.of_node = of_node_get(np);
>
>         ret = gpiochip_add_data(gc, data);
>         if (ret)
> @@ -868,6 +869,7 @@ int of_mm_gpiochip_add_data(struct device_node *np,
>
>         return 0;
>  err2:
> +       of_node_put(np);
>         iounmap(mm_gc->regs);
>  err1:
>         kfree(gc->label);
> --
> 2.25.1
>

Have you noticed any issue with reference counting that made you post
this patch? We typically "borrow" the reference to the platform
device's of_node in GPIO drivers (and everywhere I can think of too).

Bart

Re:Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Liang He]

At 2022-07-05 23:40:12, "Bartosz Golaszewski" <brgl@bgdev.pl> wrote:
>On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:
>>
>> We should use of_node_get() when a new reference of device_node
>> is created. It is noted that the old reference stored in
>> 'mm_gc->gc.of_node' should also be decreased.
>>
>> Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")
>> Signed-off-by: Liang He <windhl@126.com>
>> ---
>>  drivers/gpio/gpiolib-of.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
>> index 3d6c3ffd5576..de100b0217da 100644
>> --- a/drivers/gpio/gpiolib-of.c
>> +++ b/drivers/gpio/gpiolib-of.c
>> @@ -860,7 +860,8 @@ int of_mm_gpiochip_add_data(struct device_node *np,
>>         if (mm_gc->save_regs)
>>                 mm_gc->save_regs(mm_gc);
>>
>> -       mm_gc->gc.of_node = np;
>> +       of_node_put(mm_gc->gc.of_node);
>> +       mm_gc->gc.of_node = of_node_get(np);
>>
>>         ret = gpiochip_add_data(gc, data);
>>         if (ret)
>> @@ -868,6 +869,7 @@ int of_mm_gpiochip_add_data(struct device_node *np,
>>
>>         return 0;
>>  err2:
>> +       of_node_put(np);
>>         iounmap(mm_gc->regs);
>>  err1:
>>         kfree(gc->label);
>> --
>> 2.25.1
>>
>
>Have you noticed any issue with reference counting that made you post
>this patch? We typically "borrow" the reference to the platform
>device's of_node in GPIO drivers (and everywhere I can think of too).
>
>Bart

Hi, Bart.

This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
of src file 'drivers\soc\fsl\qe\gpio.c'.

In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
which will automatically increase and decrease the refcount.

As there is a new reference escaped into the mm_gc->gc.of_node, we should increase the 
refcount, otherwise, there may be a premature free as we do not know the existence of
the reference in 'mm_gc->gc.of_node'.

If my understanding is wrong, please correct me.

Thanks,

Liang

Re: Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Andy Shevchenko]

On Wed, Jul 6, 2022 at 3:49 AM Liang He <windhl@126.com> wrote:
> At 2022-07-05 23:40:12, "Bartosz Golaszewski" <brgl@bgdev.pl> wrote:
> >On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:

...

> This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
> of src file 'drivers\soc\fsl\qe\gpio.c'.
>
> In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
> which will automatically increase and decrease the refcount.

"and decrease"


> As there is a new reference escaped into the mm_gc->gc.of_node, we should increase the
> refcount, otherwise, there may be a premature free as we do not know the existence of
> the reference in 'mm_gc->gc.of_node'.
>
> If my understanding is wrong, please correct me.

Yes, see above.

-- 
With Best Regards,
Andy Shevchenko

Re:Re: Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Liang He]

At 2022-07-06 19:52:44, "Andy Shevchenko" <andy.shevchenko@gmail.com> wrote:
>On Wed, Jul 6, 2022 at 3:49 AM Liang He <windhl@126.com> wrote:
>> At 2022-07-05 23:40:12, "Bartosz Golaszewski" <brgl@bgdev.pl> wrote:
>> >On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:
>
>...
>
>> This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
>> of src file 'drivers\soc\fsl\qe\gpio.c'.
>>
>> In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
>> which will automatically increase and decrease the refcount.
>
>"and decrease"
>
>
>> As there is a new reference escaped into the mm_gc->gc.of_node, we should increase the
>> refcount, otherwise, there may be a premature free as we do not know the existence of
>> the reference in 'mm_gc->gc.of_node'.
>>
>> If my understanding is wrong, please correct me.
>
>Yes, see above.
>
>-- 
>With Best Regards,
>Andy Shevchenko

Hi，Andy,

I think you did not get my point.

The for_each_xxx OF api has decreased the refcount, but 
there is a reference |ESCAPE| out of the local area, so we
need additional refcounting, right?

Thanks,

Liang

Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Linus Walleij]

Hi Liang,

thanks for your patch!

On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:

> We should use of_node_get() when a new reference of device_node
> is created. It is noted that the old reference stored in
> 'mm_gc->gc.of_node' should also be decreased.
>
> Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")

I doubt this is fixing that commit since it is only moving code?

> -       mm_gc->gc.of_node = np;
> +       of_node_put(mm_gc->gc.of_node);
> +       mm_gc->gc.of_node = of_node_get(np);

Aha


> This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
> of src file 'drivers\soc\fsl\qe\gpio.c'.
>
> In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
> which will automatically increase and decrease the refcount.

Put this into the commit log message!

I guess it solves the immediate problem, but the real solution is to
get rid of of_mm_gpiochip_add_data() and struct of_mm_gpio_chip
and replace all calls with instances of proper gpiochips using
[devm_]gpiochip_add_data().

Which is in our TODO file.

It's not very much using this old helper:

$ git grep of_mm_gpiochip_add_data
arch/powerpc/platforms/4xx/gpio.c:              ret =
of_mm_gpiochip_add_data(np, mm_gc, ppc4xx_gc);
arch/powerpc/platforms/8xx/cpm1.c:      return
of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
arch/powerpc/platforms/8xx/cpm1.c:      return
of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
arch/powerpc/sysdev/cpm_common.c:       return
of_mm_gpiochip_add_data(np, mm_gc, cpm2_gc);
drivers/gpio/gpio-altera.c:     ret = of_mm_gpiochip_add_data(node,
&altera_gc->mmchip, altera_gc);
drivers/gpio/gpio-mm-lantiq.c:  return
of_mm_gpiochip_add_data(pdev->dev.of_node, &chip->mmchip, chip);
drivers/gpio/gpio-mpc5200.c:    ret =
of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
drivers/gpio/gpio-mpc5200.c:    ret =
of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
drivers/soc/fsl/qe/gpio.c:              ret =
of_mm_gpiochip_add_data(np, mm_gc, qe_gc);

Those are all.

They all seem like they can be simplified a lot by using select GPIO_GENERIC
and bgpio_init() in the manner of eg drivers/gpio/gpio-ftgpio010.c replacing
a lot of boilerplate code.

If you have access to the FSL board and can test this, please consider doing
this bigger change instead, at least for that board. It is certainly making
the world a much better place than reparing the mistakes in code
using of_mm_gpio_chip.

Yours,
Linus Walleij

Re:Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Liang He]

At 2022-07-06 20:47:08, "Linus Walleij" <linus.walleij@linaro.org> wrote:
>Hi Liang,
>
>thanks for your patch!

>


Thanks! 

>On Mon, Jul 4, 2022 at 11:13 AM Liang He <windhl@126.com> wrote:
>
>> We should use of_node_get() when a new reference of device_node
>> is created. It is noted that the old reference stored in
>> 'mm_gc->gc.of_node' should also be decreased.
>>
>> Fixes: f141ed65f256 ("gpio: Move DT support code into drivers/gpio")
>
>I doubt this is fixing that commit since it is only moving code?
>
>> -       mm_gc->gc.of_node = np;
>> +       of_node_put(mm_gc->gc.of_node);
>> +       mm_gc->gc.of_node = of_node_get(np);
>
>Aha
>
>
>> This patch is based on the fact that there is a call site in function 'qe_add_gpiochips()'
>> of src file 'drivers\soc\fsl\qe\gpio.c'.
>>
>> In that function, of_mm_gpiochip_add_data() is contained in a iteration of for_each_compatible_node(),
>> which will automatically increase and decrease the refcount.
>
>Put this into the commit log message!

>


I will put it in new version patch.

>I guess it solves the immediate problem, but the real solution is to
>get rid of of_mm_gpiochip_add_data() and struct of_mm_gpio_chip
>and replace all calls with instances of proper gpiochips using
>[devm_]gpiochip_add_data().
>
>Which is in our TODO file.
>
>It's not very much using this old helper:
>
>$ git grep of_mm_gpiochip_add_data
>arch/powerpc/platforms/4xx/gpio.c:              ret =
>of_mm_gpiochip_add_data(np, mm_gc, ppc4xx_gc);
>arch/powerpc/platforms/8xx/cpm1.c:      return
>of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
>arch/powerpc/platforms/8xx/cpm1.c:      return
>of_mm_gpiochip_add_data(np, mm_gc, cpm1_gc);
>arch/powerpc/sysdev/cpm_common.c:       return
>of_mm_gpiochip_add_data(np, mm_gc, cpm2_gc);
>drivers/gpio/gpio-altera.c:     ret = of_mm_gpiochip_add_data(node,
>&altera_gc->mmchip, altera_gc);
>drivers/gpio/gpio-mm-lantiq.c:  return
>of_mm_gpiochip_add_data(pdev->dev.of_node, &chip->mmchip, chip);
>drivers/gpio/gpio-mpc5200.c:    ret =
>of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
>drivers/gpio/gpio-mpc5200.c:    ret =
>of_mm_gpiochip_add_data(ofdev->dev.of_node, &chip->mmchip, chip);
>drivers/soc/fsl/qe/gpio.c:              ret =
>of_mm_gpiochip_add_data(np, mm_gc, qe_gc);
>
>Those are all.
>
>They all seem like they can be simplified a lot by using select GPIO_GENERIC
>and bgpio_init() in the manner of eg drivers/gpio/gpio-ftgpio010.c replacing
>a lot of boilerplate code.
>
>If you have access to the FSL board and can test this, please consider doing
>this bigger change instead, at least for that board. It is certainly making
>the world a much better place than reparing the mistakes in code
>using of_mm_gpio_chip.

>>Yours,

>Linus Walleij


Hi, Linus,

First of all, thanks for your effort to review and confirm my current patch.

Second, while I would like very much to make a bigger change, it will need 
days for me to learn the whole semantic of the source code as now I only have learned
the semantic of OF APIs and can only make a small step to decide if there is a refcount bug.

But now, should I re-post this patch with the above commit log you suggested or do more
things after I can?

Thanks again, 

Liang

Re: Re: [PATCH] gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data()

[Linus Walleij]

On Wed, Jul 6, 2022 at 3:19 PM Liang He <windhl@126.com> wrote:

> At 2022-07-06 20:47:08, "Linus Walleij" <linus.walleij@linaro.org> wrote:
> Second, while I would like very much to make a bigger change, it will need
> days for me to learn the whole semantic of the source code as now I only have learned
> the semantic of OF APIs and can only make a small step to decide if there is a refcount bug.
>
> But now, should I re-post this patch with the above commit log you suggested or do more
> things after I can?

Sure no problem, just post it!

Yours,
Linus Walleij