Re: [PATCH] mtd: oxnas_nand: Fix error handling in oxnas_nand_probe()

[Boris Brezillon]

On Mon, 18 Sep 2017 12:24:38 +0200
Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:

> On 09/18/2017 12:14 PM, Boris Brezillon wrote:
> > Hi Alexey,
> > 
> > On Sat,  9 Sep 2017 01:00:38 +0300
> > Alexey Khoroshilov <khoroshilov@xxxxxxxxx> wrote:
> >   
> >> oxnas_nand_probe() does not disable clock on error paths.
> >> The patch adds disabling using devm interface.
> >>
> >> Found by Linux Driver Verification project (linuxtesting.org).
> >>
> >> Signed-off-by: Alexey Khoroshilov <khoroshilov@xxxxxxxxx>
> >> ---
> >>  drivers/mtd/nand/oxnas_nand.c | 22 +++++++++++++++-------
> >>  1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/mtd/nand/oxnas_nand.c b/drivers/mtd/nand/oxnas_nand.c
> >> index 1b207aac840c..8abc69a285dd 100644
> >> --- a/drivers/mtd/nand/oxnas_nand.c
> >> +++ b/drivers/mtd/nand/oxnas_nand.c
> >> @@ -103,16 +103,26 @@ static int oxnas_nand_probe(struct platform_device *pdev)
> >>  	if (IS_ERR(oxnas->io_base))
> >>  		return PTR_ERR(oxnas->io_base);
> >>  
> >> -	oxnas->clk = devm_clk_get(&pdev->dev, NULL);
> >> -	if (IS_ERR(oxnas->clk))
> >> -		oxnas->clk = NULL;
> >> -
> >>  	/* Only a single chip node is supported */
> >>  	count = of_get_child_count(np);
> >>  	if (count > 1)
> >>  		return -EINVAL;
> >>  
> >> -	clk_prepare_enable(oxnas->clk);
> >> +	oxnas->clk = devm_clk_get(&pdev->dev, NULL);
> >> +	if (IS_ERR(oxnas->clk)) {
> >> +		oxnas->clk = NULL;
> >> +	} else {
> >> +		err = clk_prepare_enable(oxnas->clk);
> >> +		if (err)
> >> +			return err;
> >> +
> >> +		err = devm_add_action_or_reset(&pdev->dev,
> >> +				(void(*)(void *))clk_disable_unprepare,
> >> +				oxnas->clk);
> >> +		if (err)
> >> +			return err;
> >> +	}
> >> +  
> > 
> > Looks like something that should be made available at the CCF level
> > either with a devm_clk_get_prepare_enable() or with a
> > devm_clk_prepare_enable() helper. Would you care proposing this change
> > to the CCF ML?
> > 
> > In the meantime can you provide a simpler fix that just adds the
> > missing clk_disable_unprepare() call in the error path?
> >   
> >>  	device_reset_optional(&pdev->dev);
> >>  
> >>  	for_each_child_of_node(np, nand_np) {
> >> @@ -167,8 +177,6 @@ static int oxnas_nand_remove(struct platform_device *pdev)
> >>  	if (oxnas->chips[0])
> >>  		nand_release(nand_to_mtd(oxnas->chips[0]));
> >>  
> >> -	clk_disable_unprepare(oxnas->clk);
> >> -
> >>  	return 0;
> >>  }
> >>    
> >   
> 
> Hi Boris,
> 
> I would also like this to be simpler, but this is the actual way to workaround the lack of advanced devm_ calls from CCF.

I did not realized that when I reviewed the patch this morning,
but the bug has already been fixed by 24c9cd8f8d26 ("mtd: oxnas_nand:
Handle clk_prepare_enable/clk_disable_unprepare."), which you acked.

> 
> Is it an issue to keep this change ?

Well, let's assume the patch was really fixing a bug. If it's
targeting an -rc or is meant to be backported, the patch should be as
simple as possible, which not the case here.
Another reason to keep the existing pattern is that, once you've added
the appropriate devm_ function to the CCF, you can automate the
adoption of this function throughout the whole kernel tree using a
coccinelle script. If you start modifying some call-site that makes yet
another pattern to detect in the cocci script.