Messages in this thread | | | From | <> | Subject | Re: [PATCH] power: reset: at91-reset: free resources on exit path | Date | Thu, 1 Apr 2021 14:42:47 +0000 |
| |
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; >> } >> > >
| |