Re: [PATCH] leds: tlc591xx: add missing of_node_put

[Julia Lawall <julia.lawall@lip6.fr>]

On Sat, 15 Jul 2017, Pavel Machek wrote:

> On Sat 2017-07-15 09:48:53, Julia Lawall wrote:
> > for_each_child_of_node performs an of_node_get on each iteration, so a
> > return from the loop requires an of_node_put.
> >
> > The semantic patch that fixes this problem is as follows
> > (http://coccinelle.lip6.fr):
> >
> > // <smpl>
> > @@
> > local idexpression n;
> > expression e,e1;
> > iterator name for_each_child_of_node;
> > @@
> >
> >  for_each_child_of_node(e1,n) {
> >    ...
> > (
> >    of_node_put(n);
> > |
> >    e = n
> > |
> >    return n;
> > |
> > +  of_node_put(n);
> > ?  return ...;
> > )
> >    ...
> >  }
> > // </smpl>
> >
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Acked-by: Pavel Machek <pavel@ucw.cz>
>
> > @@ -230,12 +230,18 @@ tlc591xx_probe(struct i2c_client *client
> >
> >  	for_each_child_of_node(np, child) {
> >  		err = of_property_read_u32(child, "reg", &reg);
> > -		if (err)
> > +		if (err) {
> > +			of_node_put(child);
> >  			return err;
> > -		if (reg < 0 || reg >= tlc591xx->max_leds)
> > +		}
> > +		if (reg < 0 || reg >= tlc591xx->max_leds) {
> > +			of_node_put(child);
> >  			return -EINVAL;
> > -		if (priv->leds[reg].active)
> > +		}
> > +		if (priv->leds[reg].active) {
> > +			of_node_put(child);
> >  			return -EINVAL;
> > +		}
>
> I'd combine last two if()s into one...

The test won't all fit on one line.  Should I do it anyway?

julia