| Message ID | 20210209110109.906034-1-claudiu.beznea@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | power: reset: at91-reset: free resources on exit path | expand |
On 09/02/2021 at 12:01, Claudiu Beznea wrote: > Free resources on exit path (failure path of probe and remove). I'm not sure we can use this driver as a module anyway. Otherwise, it looks fine, but isn't it possible to use devm_of_iomap(), even in loop, and avoid having to deal with exit path? > Reported-by: kernel test robot <lkp@intel.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/power/reset/at91-reset.c | 25 ++++++++++++++++++++----- > 1 file changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c > index 3ff9d93a5226..2ff7833153b6 100644 > --- a/drivers/power/reset/at91-reset.c > +++ b/drivers/power/reset/at91-reset.c > @@ -206,7 +206,8 @@ static int __init at91_reset_probe(struct platform_device *pdev) > if (!reset->ramc_base[idx]) { > dev_err(&pdev->dev, "Could not map ram controller address\n"); > of_node_put(np); > - return -ENODEV; > + ret = -ENODEV; > + goto unmap; > } > idx++; > } > @@ -218,13 +219,15 @@ static int __init at91_reset_probe(struct platform_device *pdev) > reset->args = (u32)match->data; > > reset->sclk = devm_clk_get(&pdev->dev, NULL); > - if (IS_ERR(reset->sclk)) > - return PTR_ERR(reset->sclk); > + if (IS_ERR(reset->sclk)) { > + ret = PTR_ERR(reset->sclk); > + goto unmap; > + } > > ret = clk_prepare_enable(reset->sclk); > if (ret) { > dev_err(&pdev->dev, "Could not enable slow clock\n"); > - return ret; > + goto unmap; > } > > platform_set_drvdata(pdev, reset); > @@ -239,21 +242,33 @@ static int __init at91_reset_probe(struct platform_device *pdev) > ret = register_restart_handler(&reset->nb); > if (ret) { > clk_disable_unprepare(reset->sclk); > - return ret; > + goto unmap; > } > > at91_reset_status(pdev, reset->rstc_base); > > return 0; > + > +unmap: > + iounmap(reset->rstc_base); > + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) > + iounmap(reset->ramc_base[idx]); But if we keep this loop, I have the feeling that some kind of "of_node_put()" is needed as well. > + > + return ret; > } > > static int __exit at91_reset_remove(struct platform_device *pdev) > { > struct at91_reset *reset = platform_get_drvdata(pdev); > + int idx; > > unregister_restart_handler(&reset->nb); > clk_disable_unprepare(reset->sclk); > > + iounmap(reset->rstc_base); > + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) > + iounmap(reset->ramc_base[idx]); Ditto > + > return 0; > } > > -- Nicolas Ferre
On 31.03.2021 11:18, Nicolas Ferre wrote: > On 09/02/2021 at 12:01, Claudiu Beznea wrote: >> Free resources on exit path (failure path of probe and remove). > > I'm not sure we can use this driver as a module anyway. > > Otherwise, it looks fine, but isn't it possible to use devm_of_iomap(), > even in loop, and avoid having to deal with exit path? For: reset->rstc_base = of_iomap(pdev->dev.of_node, 0); it should work. For the maps in the loop I have to double check. Basically, the struct resource object to pass to devm_of_iomap() is needed and for this the pointer to a struct platform_device object corresponding to the node we look for in the loop is needed. So, I think this cannot be done this way. > >> Reported-by: kernel test robot <lkp@intel.com> >> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/power/reset/at91-reset.c | 25 ++++++++++++++++++++----- >> 1 file changed, 20 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/power/reset/at91-reset.c >> b/drivers/power/reset/at91-reset.c >> index 3ff9d93a5226..2ff7833153b6 100644 >> --- a/drivers/power/reset/at91-reset.c >> +++ b/drivers/power/reset/at91-reset.c >> @@ -206,7 +206,8 @@ static int __init at91_reset_probe(struct >> platform_device *pdev) >> if (!reset->ramc_base[idx]) { >> dev_err(&pdev->dev, "Could not map ram controller >> address\n"); >> of_node_put(np); >> - return -ENODEV; >> + ret = -ENODEV; >> + goto unmap; >> } >> idx++; >> } >> @@ -218,13 +219,15 @@ static int __init at91_reset_probe(struct >> platform_device *pdev) >> reset->args = (u32)match->data; >> reset->sclk = devm_clk_get(&pdev->dev, NULL); >> - if (IS_ERR(reset->sclk)) >> - return PTR_ERR(reset->sclk); >> + if (IS_ERR(reset->sclk)) { >> + ret = PTR_ERR(reset->sclk); >> + goto unmap; >> + } >> ret = clk_prepare_enable(reset->sclk); >> if (ret) { >> dev_err(&pdev->dev, "Could not enable slow clock\n"); >> - return ret; >> + goto unmap; >> } >> platform_set_drvdata(pdev, reset); >> @@ -239,21 +242,33 @@ static int __init at91_reset_probe(struct >> platform_device *pdev) >> ret = register_restart_handler(&reset->nb); >> if (ret) { >> clk_disable_unprepare(reset->sclk); >> - return ret; >> + goto unmap; >> } >> at91_reset_status(pdev, reset->rstc_base); >> return 0; >> + >> +unmap: >> + iounmap(reset->rstc_base); >> + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) >> + iounmap(reset->ramc_base[idx]); > > But if we keep this loop, I have the feeling that some kind of > "of_node_put()" is needed as well. No! In the loop: for_each_matching_node_and_match(np, at91_ramc_of_match, &match) { reset->ramc_lpr = (u32)match->data; reset->ramc_base[idx] = of_iomap(np, 0); if (!reset->ramc_base[idx]) { dev_err(&pdev->dev, "Could not map ram controller address\n"); of_node_put(np); ret = -ENODEV; goto unmap; } idx++; } the of_node_put() is needed only if the loop is interrupted as the macro: for_each_matching_node_and_match() is defined as follows: #define for_each_matching_node_and_match(dn, matches, match) \ for (dn = of_find_matching_node_and_match(NULL, matches, match); \ dn; dn = of_find_matching_node_and_match(dn, matches, match)) and of_find_matching_node_and_match() will return a np with refcount incremented but at the next loop step the of_find_matching_node_and_match() will be called with the same np pointer and the np refcount will be decremented. struct device_node *of_find_matching_node_and_match( struct device_node *from, const struct of_device_id *matches, const struct of_device_id **match) { // ... of_node_put(from); // ... } > >> + >> + return ret; >> } >> static int __exit at91_reset_remove(struct platform_device *pdev) >> { >> struct at91_reset *reset = platform_get_drvdata(pdev); >> + int idx; >> unregister_restart_handler(&reset->nb); >> clk_disable_unprepare(reset->sclk); >> + iounmap(reset->rstc_base); >> + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) >> + iounmap(reset->ramc_base[idx]); > > Ditto > >> + >> return 0; >> } >> > >
On 01.04.2021 17:42, Claudiu Beznea - M18063 wrote: > On 31.03.2021 11:18, Nicolas Ferre wrote: >> On 09/02/2021 at 12:01, Claudiu Beznea wrote: >>> Free resources on exit path (failure path of probe and remove). >> >> I'm not sure we can use this driver as a module anyway. >> >> Otherwise, it looks fine, but isn't it possible to use devm_of_iomap(), >> even in loop, and avoid having to deal with exit path? > > For: > reset->rstc_base = of_iomap(pdev->dev.of_node, 0); > > it should work. > > For the maps in the loop I have to double check. Basically, the struct > resource object to pass to devm_of_iomap() is needed and for this the Just realized I looked at the wrong code. Anyway, I'll double check and return. > pointer to a struct platform_device object corresponding to the node we > look for in the loop is needed. So, I think this cannot be done this way. > >> >>> Reported-by: kernel test robot <lkp@intel.com> >>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> >>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> --- >>> drivers/power/reset/at91-reset.c | 25 ++++++++++++++++++++----- >>> 1 file changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/power/reset/at91-reset.c >>> b/drivers/power/reset/at91-reset.c >>> index 3ff9d93a5226..2ff7833153b6 100644 >>> --- a/drivers/power/reset/at91-reset.c >>> +++ b/drivers/power/reset/at91-reset.c >>> @@ -206,7 +206,8 @@ static int __init at91_reset_probe(struct >>> platform_device *pdev) >>> if (!reset->ramc_base[idx]) { >>> dev_err(&pdev->dev, "Could not map ram controller >>> address\n"); >>> of_node_put(np); >>> - return -ENODEV; >>> + ret = -ENODEV; >>> + goto unmap; >>> } >>> idx++; >>> } >>> @@ -218,13 +219,15 @@ static int __init at91_reset_probe(struct >>> platform_device *pdev) >>> reset->args = (u32)match->data; >>> reset->sclk = devm_clk_get(&pdev->dev, NULL); >>> - if (IS_ERR(reset->sclk)) >>> - return PTR_ERR(reset->sclk); >>> + if (IS_ERR(reset->sclk)) { >>> + ret = PTR_ERR(reset->sclk); >>> + goto unmap; >>> + } >>> ret = clk_prepare_enable(reset->sclk); >>> if (ret) { >>> dev_err(&pdev->dev, "Could not enable slow clock\n"); >>> - return ret; >>> + goto unmap; >>> } >>> platform_set_drvdata(pdev, reset); >>> @@ -239,21 +242,33 @@ static int __init at91_reset_probe(struct >>> platform_device *pdev) >>> ret = register_restart_handler(&reset->nb); >>> if (ret) { >>> clk_disable_unprepare(reset->sclk); >>> - return ret; >>> + goto unmap; >>> } >>> at91_reset_status(pdev, reset->rstc_base); >>> return 0; >>> + >>> +unmap: >>> + iounmap(reset->rstc_base); >>> + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) >>> + iounmap(reset->ramc_base[idx]); >> >> But if we keep this loop, I have the feeling that some kind of >> "of_node_put()" is needed as well. > > No! In the loop: > > for_each_matching_node_and_match(np, at91_ramc_of_match, &match) { > reset->ramc_lpr = (u32)match->data; > reset->ramc_base[idx] = of_iomap(np, 0); > if (!reset->ramc_base[idx]) { > dev_err(&pdev->dev, "Could not map ram controller address\n"); > of_node_put(np); > ret = -ENODEV; > goto unmap; > } > idx++; > } > > the of_node_put() is needed only if the loop is interrupted as the macro: > for_each_matching_node_and_match() is defined as follows: > > #define for_each_matching_node_and_match(dn, matches, match) \ > for (dn = of_find_matching_node_and_match(NULL, matches, match); \ > dn; dn = of_find_matching_node_and_match(dn, matches, match)) > > and of_find_matching_node_and_match() will return a np with refcount > incremented but at the next loop step the of_find_matching_node_and_match() > will be called with the same np pointer and the np refcount will be > decremented. > > struct device_node *of_find_matching_node_and_match( > struct device_node *from, > const struct of_device_id *matches, > const struct of_device_id **match) > { > // ... > of_node_put(from); > // ... > } > >> >>> + >>> + return ret; >>> } >>> static int __exit at91_reset_remove(struct platform_device *pdev) >>> { >>> struct at91_reset *reset = platform_get_drvdata(pdev); >>> + int idx; >>> unregister_restart_handler(&reset->nb); >>> clk_disable_unprepare(reset->sclk); >>> + iounmap(reset->rstc_base); >>> + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) >>> + iounmap(reset->ramc_base[idx]); >> >> Ditto >> >>> + >>> return 0; >>> } >>> >> >> >
On 01/04/2021 at 16:42, Claudiu Beznea - M18063 wrote: >>> +unmap: >>> + iounmap(reset->rstc_base); >>> + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) >>> + iounmap(reset->ramc_base[idx]); >> But if we keep this loop, I have the feeling that some kind of >> "of_node_put()" is needed as well. > No! In the loop: > > for_each_matching_node_and_match(np, at91_ramc_of_match, &match) { > reset->ramc_lpr = (u32)match->data; > reset->ramc_base[idx] = of_iomap(np, 0); > if (!reset->ramc_base[idx]) { > dev_err(&pdev->dev, "Could not map ram controller address\n"); > of_node_put(np); > ret = -ENODEV; > goto unmap; > } > idx++; > } > > the of_node_put() is needed only if the loop is interrupted as the macro: > for_each_matching_node_and_match() is defined as follows: > > #define for_each_matching_node_and_match(dn, matches, match) \ > for (dn = of_find_matching_node_and_match(NULL, matches, match); \ > dn; dn = of_find_matching_node_and_match(dn, matches, match)) > > and of_find_matching_node_and_match() will return a np with refcount > incremented but at the next loop step the of_find_matching_node_and_match() > will be called with the same np pointer and the np refcount will be > decremented. > > struct device_node *of_find_matching_node_and_match( > struct device_node *from, > const struct of_device_id *matches, > const struct of_device_id **match) > { > // ... > of_node_put(from); > // ... > } Oh yes you're right Claudiu, I overlooked this one. Thanks for the in-depth explanation. Best regards, Nicolas -- Nicolas Ferre
diff --git a/drivers/power/reset/at91-reset.c b/drivers/power/reset/at91-reset.c index 3ff9d93a5226..2ff7833153b6 100644 --- a/drivers/power/reset/at91-reset.c +++ b/drivers/power/reset/at91-reset.c @@ -206,7 +206,8 @@ static int __init at91_reset_probe(struct platform_device *pdev) if (!reset->ramc_base[idx]) { dev_err(&pdev->dev, "Could not map ram controller address\n"); of_node_put(np); - return -ENODEV; + ret = -ENODEV; + goto unmap; } idx++; } @@ -218,13 +219,15 @@ static int __init at91_reset_probe(struct platform_device *pdev) reset->args = (u32)match->data; reset->sclk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(reset->sclk)) - return PTR_ERR(reset->sclk); + if (IS_ERR(reset->sclk)) { + ret = PTR_ERR(reset->sclk); + goto unmap; + } ret = clk_prepare_enable(reset->sclk); if (ret) { dev_err(&pdev->dev, "Could not enable slow clock\n"); - return ret; + goto unmap; } platform_set_drvdata(pdev, reset); @@ -239,21 +242,33 @@ static int __init at91_reset_probe(struct platform_device *pdev) ret = register_restart_handler(&reset->nb); if (ret) { clk_disable_unprepare(reset->sclk); - return ret; + goto unmap; } at91_reset_status(pdev, reset->rstc_base); return 0; + +unmap: + iounmap(reset->rstc_base); + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) + iounmap(reset->ramc_base[idx]); + + return ret; } static int __exit at91_reset_remove(struct platform_device *pdev) { struct at91_reset *reset = platform_get_drvdata(pdev); + int idx; unregister_restart_handler(&reset->nb); clk_disable_unprepare(reset->sclk); + iounmap(reset->rstc_base); + for (idx = 0; idx < ARRAY_SIZE(reset->ramc_base); idx++) + iounmap(reset->ramc_base[idx]); + return 0; }
Free resources on exit path (failure path of probe and remove). Reported-by: kernel test robot <lkp@intel.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/power/reset/at91-reset.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)