Skip to contentSkip to conversation

hwmon/ams: Add missing of_node_put in ams-core.c

PATCH In review Nicolas Palix 2 participants 3 messages Dec 2, 2008 – Dec 5, 2008

Subsystems (2)
Kernel janitors Hardware monitoring
lore
Share & tools
lore

Nicolas Palix

author
#0 lore raw

PATCH 1/1 hwmon/ams: Add missing of_node_put in ams-core.c

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(-)
drivers/hwmon/ams/ams-core.c +2 -0
index 6c9ace1..e612af9 100644
@@ -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;
 }
Signature · 1 line
Nicolas Palix
#1 lore raw
Quoted 16 lines · 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(-)
drivers/hwmon/ams/ams-core.c +0 -0
index 6c9ace1..e612af9 100644

drivers/hwmon/ams/ams-core.c int __init ams_init(void) · line 205

Quoted 4 lines
drivers/hwmon/ams/ams-core.c +1 -0
@@ -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.

drivers/hwmon/ams/ams-core.c int __init ams_init(void) · line 216

Quoted 11 lines
drivers/hwmon/ams/ams-core.c +1 -0
 #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.

Signature · 3 lines
Cheers,
Stephen Rothwell                    [email protected]
http://www.canb.auug.org.au/~sfr/
author
#2 lore raw
Quoted 4 lines · 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.

Quoted 5 lines

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(-)
drivers/hwmon/ams/ams-core.c +13 -4
index 6c9ace1..9856c3f 100644
@@ -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;
 }
drivers/hwmon/ams/ams-i2c.c +6 -1
index 2cbf8a6..636db87 100644
@@ -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;
 }
drivers/hwmon/ams/ams-pmu.c +10 -3
index fb18b3d..babf3d5 100644
@@ -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);

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html