[lm-sensors] [PATCH 2.6.15-rc5] hwmon: add required idr locking

[lm-sensors] [PATCH 2.6.15-rc5] hwmon: add required idr locking

[Mark M. Hoffman]

Add required locking around idr_ routines, retry the idr_pre_get/idr_get_new
pair properly, and sprinkle in some likely/unlikely for good measure.

(Lack of idr locking didn't hurt when all callers were I2C clients, as the
i2c-core serialized for us anyway.  Now that we have non I2C hwmon drivers,
this is truly necessary.)

This patch should be considered for 2.6.16.

Signed-off-by: Mark M. Hoffman <mhoffman at lightlink.com>

--- linux-2.6.16-rc5.orig/drivers/hwmon/hwmon.c
+++ linux-2.6.16-rc5/drivers/hwmon/hwmon.c
@@ -17,6 +17,7 @@
 #include <linux/idr.h>
 #include <linux/hwmon.h>
 #include <linux/gfp.h>
+#include <linux/spinlock.h>
 
 #define HWMON_ID_PREFIX "hwmon"
 #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -24,6 +25,7 @@
 static struct class *hwmon_class;
 
 static DEFINE_IDR(hwmon_idr);
+static DEFINE_SPINLOCK(idr_lock);
 
 /**
  * hwmon_device_register - register w/ hwmon sysfs class
@@ -37,20 +39,30 @@ static DEFINE_IDR(hwmon_idr);
 struct class_device *hwmon_device_register(struct device *dev)
 {
 	struct class_device *cdev;
-	int id;
+	int id, err;
 
-	if (idr_pre_get(&hwmon_idr, GFP_KERNEL) = 0)
+again:
+	if (unlikely(idr_pre_get(&hwmon_idr, GFP_KERNEL) = 0))
 		return ERR_PTR(-ENOMEM);
 
-	if (idr_get_new(&hwmon_idr, NULL, &id) < 0)
+	spin_lock(&idr_lock);
+	err = idr_get_new(&hwmon_idr, NULL, &id);
+	spin_unlock(&idr_lock);
+
+	if (unlikely(err = -EAGAIN))
+		goto again;
+	else if (unlikely(err))
 		return ERR_PTR(-ENOMEM);
 
 	id = id & MAX_ID_MASK;
 	cdev = class_device_create(hwmon_class, NULL, MKDEV(0,0), dev,
 					HWMON_ID_FORMAT, id);
 
-	if (IS_ERR(cdev))
+	if (unlikely(IS_ERR(cdev))) {
+		spin_lock(&idr_lock);
 		idr_remove(&hwmon_idr, id);
+		spin_unlock(&idr_lock);
+	}
 
 	return cdev;
 }
@@ -64,9 +76,11 @@ void hwmon_device_unregister(struct clas
 {
 	int id;
 
-	if (sscanf(cdev->class_id, HWMON_ID_FORMAT, &id) = 1) {
+	if (likely(sscanf(cdev->class_id, HWMON_ID_FORMAT, &id) = 1)) {
 		class_device_unregister(cdev);
+		spin_lock(&idr_lock);
 		idr_remove(&hwmon_idr, id);
+		spin_unlock(&idr_lock);
 	} else
 		dev_dbg(cdev->dev,
 			"hwmon_device_unregister() failed: bad class ID!\n");
-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] [PATCH 2.6.15-rc5] hwmon: add required idr locking

[Jean Delvare]

Hi Mark,

> Add required locking around idr_ routines, retry the idr_pre_get/idr_get_new
> pair properly, and sprinkle in some likely/unlikely for good measure.
> 
> (Lack of idr locking didn't hurt when all callers were I2C clients, as the
> i2c-core serialized for us anyway.  Now that we have non I2C hwmon drivers,
> this is truly necessary.)
> 
> This patch should be considered for 2.6.16.

Thanks for reporting the problem and proposing a fix. It's all correct
as far as I can tell, but I'd still have a few comments:

> -	if (idr_get_new(&hwmon_idr, NULL, &id) < 0)
> +	spin_lock(&idr_lock);
> +	err = idr_get_new(&hwmon_idr, NULL, &id);
> +	spin_unlock(&idr_lock);
> +
> +	if (unlikely(err = -EAGAIN))
> +		goto again;
> +	else if (unlikely(err))
>  		return ERR_PTR(-ENOMEM);

It has just occured to me that the error value we are returning is not
correct. Why -ENOMEM? As far as I know, idr_get_new does not allocate
memory, by design, so it's probably the more inaccurate error value we
could return.

Why don't we just return ERR_PTR(err) instead?

I realize that this isn't strictly related with your patch, the old code
returned just the same, but while we're here changing that code...

> -	if (IS_ERR(cdev))
> +	if (unlikely(IS_ERR(cdev))) {

IS_ERR() already includes an unlikely(), so this is redundant.

And you're right, let's try to get that patch in 2.6.16.

Thanks,
-- 
Jean Delvare

[lm-sensors] [PATCH 2.6.15-rc5] hwmon: add required idr locking

[Mark M. Hoffman]

Hi Jean:

* Jean Delvare <khali at linux-fr.org> [2006-03-05 18:32:27 +0100]:
> > Add required locking around idr_ routines, retry the idr_pre_get/idr_get_new
> > pair properly, and sprinkle in some likely/unlikely for good measure.
> > 
> > (Lack of idr locking didn't hurt when all callers were I2C clients, as the
> > i2c-core serialized for us anyway.  Now that we have non I2C hwmon drivers,
> > this is truly necessary.)
> > 
> > This patch should be considered for 2.6.16.
> 
> Thanks for reporting the problem and proposing a fix. It's all correct
> as far as I can tell, but I'd still have a few comments:
> 
> > -	if (idr_get_new(&hwmon_idr, NULL, &id) < 0)
> > +	spin_lock(&idr_lock);
> > +	err = idr_get_new(&hwmon_idr, NULL, &id);
> > +	spin_unlock(&idr_lock);
> > +
> > +	if (unlikely(err = -EAGAIN))
> > +		goto again;
> > +	else if (unlikely(err))
> >  		return ERR_PTR(-ENOMEM);
> 
> It has just occured to me that the error value we are returning is not
> correct. Why -ENOMEM? As far as I know, idr_get_new does not allocate
> memory, by design, so it's probably the more inaccurate error value we
> could return.
> 
> Why don't we just return ERR_PTR(err) instead?

OK.

> I realize that this isn't strictly related with your patch, the old code
> returned just the same, but while we're here changing that code...
> 
> > -	if (IS_ERR(cdev))
> > +	if (unlikely(IS_ERR(cdev))) {
> 
> IS_ERR() already includes an unlikely(), so this is redundant.

So it does - struck.

New boot-tested (on x86) patch to follow...

Regards,

-- 
Mark M. Hoffman
mhoffman at lightlink.com

[lm-sensors] [PATCH 2.6.15-rc5] hwmon: add required idr locking

[Mark M. Hoffman]

Add required locking around idr_ routines, retry the idr_pre_get/idr_get_new
pair properly, and sprinkle in some likely/unlikely for good measure.

(Lack of idr locking didn't hurt when all callers were I2C clients, as the
i2c-core serialized for us anyway.  Now that we have non I2C hwmon drivers,
this is truly necessary.)

This patch should be considered for 2.6.16.

Signed-off-by: Mark M. Hoffman <mhoffman at lightlink.com>

--- linux-2.6.16-rc5.orig/drivers/hwmon/hwmon.c
+++ linux-2.6.16-rc5/drivers/hwmon/hwmon.c
@@ -17,6 +17,7 @@
 #include <linux/idr.h>
 #include <linux/hwmon.h>
 #include <linux/gfp.h>
+#include <linux/spinlock.h>
 
 #define HWMON_ID_PREFIX "hwmon"
 #define HWMON_ID_FORMAT HWMON_ID_PREFIX "%d"
@@ -24,6 +25,7 @@
 static struct class *hwmon_class;
 
 static DEFINE_IDR(hwmon_idr);
+static DEFINE_SPINLOCK(idr_lock);
 
 /**
  * hwmon_device_register - register w/ hwmon sysfs class
@@ -37,20 +39,30 @@ static DEFINE_IDR(hwmon_idr);
 struct class_device *hwmon_device_register(struct device *dev)
 {
 	struct class_device *cdev;
-	int id;
+	int id, err;
 
-	if (idr_pre_get(&hwmon_idr, GFP_KERNEL) = 0)
+again:
+	if (unlikely(idr_pre_get(&hwmon_idr, GFP_KERNEL) = 0))
 		return ERR_PTR(-ENOMEM);
 
-	if (idr_get_new(&hwmon_idr, NULL, &id) < 0)
-		return ERR_PTR(-ENOMEM);
+	spin_lock(&idr_lock);
+	err = idr_get_new(&hwmon_idr, NULL, &id);
+	spin_unlock(&idr_lock);
+
+	if (unlikely(err = -EAGAIN))
+		goto again;
+	else if (unlikely(err))
+		return ERR_PTR(err);
 
 	id = id & MAX_ID_MASK;
 	cdev = class_device_create(hwmon_class, NULL, MKDEV(0,0), dev,
 					HWMON_ID_FORMAT, id);
 
-	if (IS_ERR(cdev))
+	if (IS_ERR(cdev)) {
+		spin_lock(&idr_lock);
 		idr_remove(&hwmon_idr, id);
+		spin_unlock(&idr_lock);
+	}
 
 	return cdev;
 }
@@ -64,9 +76,11 @@ void hwmon_device_unregister(struct clas
 {
 	int id;
 
-	if (sscanf(cdev->class_id, HWMON_ID_FORMAT, &id) = 1) {
+	if (likely(sscanf(cdev->class_id, HWMON_ID_FORMAT, &id) = 1)) {
 		class_device_unregister(cdev);
+		spin_lock(&idr_lock);
 		idr_remove(&hwmon_idr, id);
+		spin_unlock(&idr_lock);
 	} else
 		dev_dbg(cdev->dev,
 			"hwmon_device_unregister() failed: bad class ID!\n");

-- 
Mark M. Hoffman
mhoffman at lightlink.com