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@xxxxxxxxx>
> > > ---
> > > 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", ®);
> > > - 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
