Re: [PATCH] input: keyboard: cap11xx: Add missing of_node_put

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Thu, Jan 28, 2016 at 10:16:39AM +0100, Julia Lawall wrote:
> 
> 
> On Wed, 27 Jan 2016, Dmitry Torokhov wrote:
> 
> > On Mon, Jan 25, 2016 at 09:01:21PM +0530, Amitoj Kaur Chawla wrote:
> > > for_each_child_of_node performs an of_node_get on each iteration, so
> > > to break out of the loop an of_node_put is required.
> > >
> > > Found using Coccinelle. The semantic patch used for this is as follows:
> > >
> > > // <smpl>
> > > @@
> > > expression e;
> > > local idexpression n;
> > > @@
> > >
> > >  for_each_child_of_node(..., n) {
> > >    ... when != of_node_put(n)
> > >        when != e = n
> > > (
> > >    return n;
> > > |
> > > +  of_node_put(n);
> > > ?  return ...;
> > > )
> > >    ...
> > >  }
> > > // </smpl
> > >
> > > Signed-off-by: Amitoj Kaur Chawla <amitoj1606@gmail.com>
> > > ---
> > >  drivers/input/keyboard/cap11xx.c | 12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/cap11xx.c b/drivers/input/keyboard/cap11xx.c
> > > index 378db10..27cd7df 100644
> > > --- a/drivers/input/keyboard/cap11xx.c
> > > +++ b/drivers/input/keyboard/cap11xx.c
> > > @@ -304,8 +304,10 @@ static int cap11xx_init_leds(struct device *dev,
> > >  		led->cdev.brightness = LED_OFF;
> > >
> > >  		error = of_property_read_u32(child, "reg", &reg);
> > > -		if (error != 0 || reg >= num_leds)
> > > -			return -EINVAL;
> > > +		if (error != 0 || reg >= num_leds) {
> > > +			error = -EINVAL;
> > > +			goto putchild;
> >
> > Instead of jumping to a label I added of_node_put here and also below
> > and applied, thank you.
> 
> Do you have a general strategy for this?
> 
> I asked Arnd Bergmann, and he said that if things were shared and if all
> failures later in the function could use the shared label, then one should
> use a label.  But I can see that there could be different opinions about
> it.  Maybe two instances is not enough for sharing?  Maybe the fact that
> the need for this error handling is limited to the loop means that there
> should never be sharing?

I do not think I can formalize the rule well, it is a bit of everything:

- the function is devm-ised and I do not like mixing "goto err" style of
  cleanups with automatic devm cleanup
- there was no "goto err*" in the function before the change
- as you mentioned the cleanup "belongs" to the loop
- there were only 2 instances where we needed to do cleanup
- amount of cleanup was minimal

As a side not I am unhappy with this API as it is very similar
list_for_each and for_each_set_bit, etc, so needing to drop reference
when breaking/returning is quite unexpected ;(

Thanks.

-- 
Dmitry