* [PATCH] input: keyboard: cap11xx: Add missing of_node_put
@ 2016-01-25 15:31 Amitoj Kaur Chawla
2016-01-27 23:48 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Amitoj Kaur Chawla @ 2016-01-25 15:31 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel; +Cc: julia.lawall
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", ®);
- if (error != 0 || reg >= num_leds)
- return -EINVAL;
+ if (error != 0 || reg >= num_leds) {
+ error = -EINVAL;
+ goto putchild;
+ }
led->reg = reg;
led->priv = priv;
@@ -314,13 +316,17 @@ static int cap11xx_init_leds(struct device *dev,
error = devm_led_classdev_register(dev, &led->cdev);
if (error)
- return error;
+ goto putchild;
priv->num_leds++;
led++;
}
return 0;
+
+putchild:
+ of_node_put(child);
+ return error;
}
#else
static int cap11xx_init_leds(struct device *dev,
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] input: keyboard: cap11xx: Add missing of_node_put
2016-01-25 15:31 [PATCH] input: keyboard: cap11xx: Add missing of_node_put Amitoj Kaur Chawla
@ 2016-01-27 23:48 ` Dmitry Torokhov
2016-01-28 9:16 ` Julia Lawall
0 siblings, 1 reply; 4+ messages in thread
From: Dmitry Torokhov @ 2016-01-27 23:48 UTC (permalink / raw)
To: Amitoj Kaur Chawla; +Cc: linux-input, linux-kernel, julia.lawall
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", ®);
> - 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.
I believe there is another input driver that returns from
for_each_child_of_node() without dropping reference.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: keyboard: cap11xx: Add missing of_node_put
2016-01-27 23:48 ` Dmitry Torokhov
@ 2016-01-28 9:16 ` Julia Lawall
2016-01-28 18:51 ` Dmitry Torokhov
0 siblings, 1 reply; 4+ messages in thread
From: Julia Lawall @ 2016-01-28 9:16 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Amitoj Kaur Chawla, linux-input, linux-kernel
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", ®);
> > - 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?
thanks,
julia
>
> I believe there is another input driver that returns from
> for_each_child_of_node() without dropping reference.
>
> Thanks.
>
> --
> Dmitry
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] input: keyboard: cap11xx: Add missing of_node_put
2016-01-28 9:16 ` Julia Lawall
@ 2016-01-28 18:51 ` Dmitry Torokhov
0 siblings, 0 replies; 4+ messages in thread
From: Dmitry Torokhov @ 2016-01-28 18:51 UTC (permalink / raw)
To: Julia Lawall; +Cc: Amitoj Kaur Chawla, linux-input, linux-kernel
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", ®);
> > > - 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-01-28 18:51 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-25 15:31 [PATCH] input: keyboard: cap11xx: Add missing of_node_put Amitoj Kaur Chawla
2016-01-27 23:48 ` Dmitry Torokhov
2016-01-28 9:16 ` Julia Lawall
2016-01-28 18:51 ` Dmitry Torokhov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.