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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4798 bytes --]

On Fri, Nov 06, 2020 at 10:29:08AM +0000, Lee Jones wrote:
> 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.

I agree with Lee on this one, so I've applied the patch and touched up
the label names while at it.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]