| Message ID | 20220704091313.277567-1-windhl@126.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpiolib-of: Fix refcount bugs in of_mm_gpiochip_add_data() | expand |
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
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
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.
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
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
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
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
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);
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(-)