[PATCH linux-next] hwmon/ams: Add missing of_node_put in ams-core.c

[PATCH linux-next] hwmon/ams: Add missing of_node_put in ams-core.c

[Nicolas Palix]

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 <npalix@diku.dk>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
 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

Re: [PATCH linux-next] hwmon/ams: Add missing of_node_put in ams-core.c

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1561 bytes --]

On Tue, 2 Dec 2008 14:48:13 +0100 Nicolas Palix <npalix@diku.dk> 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 <npalix@diku.dk>
> Signed-off-by: Julia Lawall <julia@diku.dk>
> ---
>  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                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH linux-next] hwmon/ams: Add missing of_node_put in ams-core.c

[Nicolas Palix]

On Wednesday 03 December 2008 00:30:51 Stephen Rothwell wrote:
> On Tue, 2 Dec 2008 14:48:13 +0100 Nicolas Palix <npalix@diku.dk> 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 <npalix@diku.dk>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
 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);