linux-kernel - Re: [PATCH] leds: tlc591xx: add missing of_node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.20.1707151109180.2838@hadrien>
Date:   Sat, 15 Jul 2017 11:09:52 +0200 (CEST)
From:   Julia Lawall <julia.lawall@...6.fr>
To:     Pavel Machek <pavel@....cz>
cc:     Richard Purdie <rpurdie@...ys.net>,
        kernel-janitors@...r.kernel.org,
        Jacek Anaszewski <jacek.anaszewski@...il.com>,
        linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: tlc591xx: add missing of_node_put



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@...6.fr>
>
> Acked-by: Pavel Machek <pavel@....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