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

From: Julia Lawall
Date: Sat Jul 15 2017 - 05:10:01 EST




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@xxxxxxx>
>
> Acked-by: Pavel Machek <pavel@xxxxxx>
>
> > @@ -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