Messages in this thread | | | Subject | Re: [PATCH] power: reset: at91-reset: free resources on exit path | From | Nicolas Ferre <> | Date | Wed, 31 Mar 2021 10:18:41 +0200 |
| |
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
| |