Re: [PATCH] pwm: sti: fix error handling

[Lee Jones <lee.jones@linaro.org>]

On Fri, 06 Nov 2020, Uwe Kleine-König wrote:

> Hello Lee,
> 
> On Fri, Nov 06, 2020 at 08:29:14AM +0000, Lee Jones wrote:
> > On Tue, 13 Oct 2020, Uwe Kleine-König wrote:
> > 
> > > This commit fixes several faults:
> > > 
> > >  - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
> > >    by calling clk_put().
> > >  - A clk that was prepared must be unprepared.
> > >  - The .remove callback isn't supposed to call pwm_disable().
> > >  - The necessary resources needed by the PWM must not be freed before
> > >    pwmchip_remove() was called.
> > > 
> > > Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
> > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > ---
> > >  drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 37 insertions(+), 12 deletions(-)
> > 
> > Sorry for the delay, this ended up in spam.
> > 
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > index 1508616d794c..f89f8cbdfdfc 100644
> > > --- a/drivers/pwm/pwm-sti.c
> > > +++ b/drivers/pwm/pwm-sti.c
> > > @@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > >  	ret = clk_prepare(pc->pwm_clk);
> > >  	if (ret) {
> > >  		dev_err(dev, "failed to prepare clock\n");
> > > -		return ret;
> > > +		goto err_pwm_clk_prepare;
> > 
> > I would prefer these to indicate the intention, rather than were they
> > were called from.  So err_put_cpt_clk for this one, etc.
> 
> This might be subjective, but I like it better the way I did it. My
> pattern is:
> 
> 	ret = get_resource_A();
> 	if (ret)
> 		goto err_A;
> 
> 	ret = get_resource_B();
> 	if (ret)
> 		goto err_B;
> 
> 	...
> 
> 	put_resource_B();
> err_B:
> 	
> 	put_resource_A();
> err_A:
> 
> 	return ret;
> 
> This way just looking at on get_resource_$X block it is obvious that the
> picked label is right and in the error handling blocks that's obvious,
> too.
> 
> However with the (admittedly more common) style you prefer it is:
> 
> 	ret = get_resource_A();
> 	if (ret)
> 		goto return_ret; // or just: return ret
> 
> 	ret = get_resource_B();
> 	if (ret)
> 		goto put_A;
> 
> 	...
> 
> put_B:
> 	put_resource_B();
> 
> put_A:
> 	put_resource_A();
> 
> return_ret:
> 	return ret;
> 
> You have to check the previous block to see that put_A is right for
> the error path of get_resource_B(). In this trivial example you have to
> look back 6 instead of 2 lines. For more complex stuff it tends to be
> 3 lines for my approach (one more for the error message, and so still in
> the same logical block) but might be considerably bigger for the common
> approach. The usual amount of context in patches is 3 lines. And if you
> add another resource allocation between A and B you have to adapt the
> error path in B which is somewhat unrelated. So a patch adding A2 looks
> for my approach:
> 
> @@ ...
>  	if (ret)
>  		goto err_A;
>  
> +	ret = get_resource_A2();
> +	if (ret)
> +		goto err_A2;
> +
>  	ret = get_resource_B();
>  	if (ret)
>  		goto err_B;
> @@ ...
>  	put_resource_B();
>  err_B:
>  	
> +	put_resource_A2();
> +err_A2:
> +
>  	put_resource_A()
>  err_A:
>  
> Here you see that the right error label is used in the new error path
> and that it is placed correctly between err_B and err_A.
> 
> For your preferred approach the patch looks as follows:
> 
> @@ ...
>  	if (ret)
>  		goto return_ret;
>  
> +	ret = get_resource_A2();
> +	if (ret)
> +		goto put_A;
> +
>  	ret = get_resource_B();
>  	if (ret)
> -		goto put_A;
> +		goto put_A2;
>  
>  	...
> @@ ...
>  put_B: 
>  	put_resource_B();
>  
> +put_A2:
> +	put_resource_A2;
> +
>  put_A:
>  	put_resource_A();
>  
> Note you cannot see by just looking at the patch that goto put_A is
> right. (Well, if you assume that the old code is correct see that just
> before put_A B is freed which matches what just happens after your new
> get_resource_A2, but that's considerably more complicated.) Also you
> have to modify the goto for B.
> 
> This is in my eyes ugly enough to justify my preference.

Wow, you sure put a lot of effort into that. :)

I'll leave it up to ST and Thierry to have the final say.

> > > +err_pwmchip_add:
> > > +
> > > +	if (cdata->cpt_num_devs) {
> > 
> > No need for the evaluations, clk_*() calls are NULL safe.
> 
> Yes, I added that to match the allocation pattern. I don't feel strong

Right, but my point is, you don't have to.

It's added complexity/lines for no clear value.

I would personally simplify it all and remove them.

It also saves the ugliness of goto'ing into the middle of an if().

> here. (But as a side note: I don't like the gotos in .probe. They are
> not used to do (plain) error handling and so they complicate following
> the code flow.)

I think error handling is where gotos excel personally.
-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog