of_node_put is needed before discarding a value received from
of_find_node_by_name, eg in error handling code or when the device
node is no longer used.
The semantic match that catches the bug is as follows:
(http://www.emn.fr/x-info/coccinelle/)
// <smpl>
@r exists@
local idexpression struct device_node *n;
position p1, p2;
statement S1,S2;
expression E,E1;
expression *ptr != NULL;
@@
(
if (!(n@p1 = of_find_node_by_name(...))) S1
|
n@p1 = of_find_node_by_name(...)
)
<... when != of_node_put(n)
when != if (...) { <+... of_node_put(n) ...+> }
when != true !n || ...
when != n = E
when != E = n
if (!n || ...) S2
...>
(
return \(0\|<+...n...+>\|ptr\);
|
return@p2 ...;
|
n = E1
|
E1 = n
)
@script:python@
p1 << r.p1;
p2 << r.p2;
@@
print "* file: %s of_find_node_by_name %s return %s" % (p1[0].file,p1[0].line,p2[0].line)
// </smpl>
Signed-off-by: Nicolas Palix <[email protected]>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/hwmon/ams/ams-core.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/hwmon/ams/ams-core.c b/drivers/hwmon/ams/ams-core.c
index 6c9ace1..e612af9 100644
--- a/drivers/hwmon/ams/ams-core.c
+++ b/drivers/hwmon/ams/ams-core.c
@@ -202,6 +202,7 @@ int __init ams_init(void)
if (np && of_device_is_compatible(np, "AAPL,accelerometer_1"))
/* Found I2C motion sensor */
return ams_i2c_init(np);
+ of_node_put(np);
#endif
#ifdef CONFIG_SENSORS_AMS_PMU
@@ -209,6 +210,7 @@ int __init ams_init(void)
if (np && of_device_is_compatible(np, "sms"))
/* Found PMU motion sensor */
return ams_pmu_init(np);
+ of_node_put(np);
#endif
return -ENODEV;
}
--
Nicolas Palix
On Tue, 2 Dec 2008 14:48:13 +0100 Nicolas Palix <[email protected]> wrote:
>
> of_node_put is needed before discarding a value received from
> of_find_node_by_name, eg in error handling code or when the device
> node is no longer used.
>
> Signed-off-by: Nicolas Palix <[email protected]>
> Signed-off-by: Julia Lawall <[email protected]>
> ---
> drivers/hwmon/ams/ams-core.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hwmon/ams/ams-core.c b/drivers/hwmon/ams/ams-core.c
> index 6c9ace1..e612af9 100644
> --- a/drivers/hwmon/ams/ams-core.c
> +++ b/drivers/hwmon/ams/ams-core.c
> @@ -202,6 +202,7 @@ int __init ams_init(void)
> if (np && of_device_is_compatible(np, "AAPL,accelerometer_1"))
> /* Found I2C motion sensor */
> return ams_i2c_init(np);
> + of_node_put(np);
This is wrong because if CONFIG_SENSORS_AMS_PMU is set, np is used again
below.
> #endif
>
> #ifdef CONFIG_SENSORS_AMS_PMU
> @@ -209,6 +210,7 @@ int __init ams_init(void)
> if (np && of_device_is_compatible(np, "sms"))
> /* Found PMU motion sensor */
> return ams_pmu_init(np);
> + of_node_put(np);
> #endif
> return -ENODEV;
> }
Also, I don't like this code since each of ams_i2c_init and ams_pmu_init
keep a reference to np, so they should be doing an of_node_get(np) and
this code above should be unconditionally doing an of_node_put(np). This
is not your problem, though.
--
Cheers,
Stephen Rothwell [email protected]
http://www.canb.auug.org.au/~sfr/
On Wednesday 03 December 2008 00:30:51 Stephen Rothwell wrote:
> On Tue, 2 Dec 2008 14:48:13 +0100 Nicolas Palix <[email protected]> wrote:
> This is wrong because if CONFIG_SENSORS_AMS_PMU is set, np is used again
> below.
>
np is indeed reused in the second #ifdef, but there it is immediately
assigned a new value. So of_node_put is needed in the first #ifdef to
decrement the reference count on the value it has there.
> Also, I don't like this code since each of ams_i2c_init and ams_pmu_init
> keep a reference to np, so they should be doing an of_node_get(np) and
> this code above should be unconditionally doing an of_node_put(np). This
> is not your problem, though.
>
Following your remarks, I changed the patch. ams_i2c_init and ams_pmu_init
still keep a reference but they're now doing a call to of_node_get in their
init functions. The reference is then released in the corresponding exit
function or in error handling code.
Finally, ams_init releases the np reference for each of the two
of_find_node_by_name calls.
Regards,
Signed-off-by: Nicolas Palix <[email protected]>
Signed-off-by: Julia Lawall <[email protected]>
---
ams-core.c | 17 +++++++++++++----
ams-i2c.c | 7 ++++++-
ams-pmu.c | 13 ++++++++++---
3 files changed, 29 insertions(+), 8 deletions(-)
diff --git a/drivers/hwmon/ams/ams-core.c b/drivers/hwmon/ams/ams-core.c
index 6c9ace1..9856c3f 100644
--- a/drivers/hwmon/ams/ams-core.c
+++ b/drivers/hwmon/ams/ams-core.c
@@ -192,6 +192,7 @@ release_freefall:
int __init ams_init(void)
{
struct device_node *np;
+ int ret;
spin_lock_init(&ams_info.irq_lock);
mutex_init(&ams_info.lock);
@@ -199,16 +200,24 @@ int __init ams_init(void)
#ifdef CONFIG_SENSORS_AMS_I2C
np = of_find_node_by_name(NULL, "accelerometer");
- if (np && of_device_is_compatible(np, "AAPL,accelerometer_1"))
+ if (np && of_device_is_compatible(np, "AAPL,accelerometer_1")) {
/* Found I2C motion sensor */
- return ams_i2c_init(np);
+ ret = ams_i2c_init(np);
+ of_node_put(np);
+ return ret;
+ }
+ of_node_put(np);
#endif
#ifdef CONFIG_SENSORS_AMS_PMU
np = of_find_node_by_name(NULL, "sms");
- if (np && of_device_is_compatible(np, "sms"))
+ if (np && of_device_is_compatible(np, "sms")) {
/* Found PMU motion sensor */
- return ams_pmu_init(np);
+ ret = ams_pmu_init(np);
+ of_node_put(np);
+ return ret;
+ }
+ of_node_put(np);
#endif
return -ENODEV;
}
diff --git a/drivers/hwmon/ams/ams-i2c.c b/drivers/hwmon/ams/ams-i2c.c
index 2cbf8a6..636db87 100644
--- a/drivers/hwmon/ams/ams-i2c.c
+++ b/drivers/hwmon/ams/ams-i2c.c
@@ -255,6 +255,9 @@ static int ams_i2c_remove(struct i2c_client *client)
static void ams_i2c_exit(void)
{
i2c_del_driver(&ams_i2c_driver);
+
+ /* Release device node */
+ of_node_put(ams_info.of_node);
}
int __init ams_i2c_init(struct device_node *np)
@@ -262,7 +265,7 @@ int __init ams_i2c_init(struct device_node *np)
int result;
/* Set implementation stuff */
- ams_info.of_node = np;
+ ams_info.of_node = of_node_get(np);
ams_info.exit = ams_i2c_exit;
ams_info.get_vendor = ams_i2c_get_vendor;
ams_info.get_xyz = ams_i2c_get_xyz;
@@ -270,6 +273,8 @@ int __init ams_i2c_init(struct device_node *np)
ams_info.bustype = BUS_I2C;
result = i2c_add_driver(&ams_i2c_driver);
+ if (result < 0)
+ of_node_put(ams_info.of_node);
return result;
}
diff --git a/drivers/hwmon/ams/ams-pmu.c b/drivers/hwmon/ams/ams-pmu.c
index fb18b3d..babf3d5 100644
--- a/drivers/hwmon/ams/ams-pmu.c
+++ b/drivers/hwmon/ams/ams-pmu.c
@@ -141,6 +141,9 @@ static void ams_pmu_exit(void)
ams_info.has_device = 0;
+ /* Release device node */
+ of_node_put(ams_info.of_node);
+
printk(KERN_INFO "ams: Unloading\n");
}
@@ -150,7 +153,7 @@ int __init ams_pmu_init(struct device_node *np)
int result;
/* Set implementation stuff */
- ams_info.of_node = np;
+ ams_info.of_node = of_node_get(np);
ams_info.exit = ams_pmu_exit;
ams_info.get_vendor = ams_pmu_get_vendor;
ams_info.get_xyz = ams_pmu_get_xyz;
@@ -159,8 +162,10 @@ int __init ams_pmu_init(struct device_node *np)
/* Get PMU command, should be 0x4e, but we can never know */
prop = of_get_property(ams_info.of_node, "reg", NULL);
- if (!prop)
+ if (!prop) {
+ of_node_put(ams_info.of_node);
return -ENODEV;
+ }
ams_pmu_cmd = ((*prop) >> 8) & 0xff;
@@ -171,8 +176,10 @@ int __init ams_pmu_init(struct device_node *np)
ams_pmu_clear_irq(AMS_IRQ_ALL);
result = ams_sensor_attach();
- if (result < 0)
+ if (result < 0) {
+ of_node_put(ams_info.of_node);
return result;
+ }
/* Set default values */
ams_pmu_set_register(AMS_FF_LOW_LIMIT, 0x15);