Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734006 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mtd: oxnas_nand: Fix error handling in  oxnas_nand_probe() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-09-18 12:20 +0200
    Re: [PATCH] mtd: oxnas_nand: Fix error handling in oxnas_nand_probe() Neil Armstrong <narmstrong@baylibre.com> - 2017-09-18 12:30 +0200
      Re: [PATCH] mtd: oxnas_nand: Fix error handling in  oxnas_nand_probe() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-09-18 14:30 +0200

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

Hi Alexey,

On Sat,  9 Sep 2017 01:00:38 +0300
Alexey Khoroshilov <khoroshilov@ispras.ru> 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@ispras.ru>
> ---
>  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;
>  }
>  

[toc] | [next] | [standalone]

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

On 09/18/2017 12:14 PM, Boris Brezillon wrote:
> Hi Alexey,
> 
> On Sat,  9 Sep 2017 01:00:38 +0300
> Alexey Khoroshilov <khoroshilov@ispras.ru> 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@ispras.ru>
>> ---
>>  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.

Is it an issue to keep this change ?

Neil

[toc] | [prev] | [next] | [standalone]

#1734091

On Mon, 18 Sep 2017 12:24:38 +0200
Neil Armstrong <narmstrong@baylibre.com> 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@ispras.ru> 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@ispras.ru>
> >> ---
> >>  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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web