[PATCH 1/3][-mm] add class_reclassify macro

[PATCH 1/3][-mm] add class_reclassify macro

[Dave Young]

Converting class semaphore to mutex cause lockdep warnings due to
class_interface_register/unregister will possible call device_add/del

For the class_interface users here add a class_reclassify macro to
reclassify the lock class of their struct class.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>

---
include/linux/device.h |    7 +++++++
1 file changed, 7 insertions(+)

--- linux/include/linux/device.h	2008-05-19 12:29:54.000000000 +0800
+++ linux.new/include/linux/device.h	2008-05-19 14:42:25.000000000 +0800
@@ -529,4 +529,11 @@ extern const char *dev_driver_string(str
 	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
 #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
 	MODULE_ALIAS("char-major-" __stringify(major) "-*")
+
+#define class_reclassify(class)						\
+do {									\
+	static struct lock_class_key	class_key;			\
+	lockdep_set_class_and_name(&(class)->mutex, &class_key,		\
+				 (class)->name);			\
+} while (0)
 #endif /* _DEVICE_H_ */

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Andrew Morton]

On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:

> Converting class semaphore to mutex cause lockdep warnings due to
> class_interface_register/unregister will possible call device_add/del

Shouldn't we just fix that?

> For the class_interface users here add a class_reclassify macro to
> reclassify the lock class of their struct class.
> 
> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
> 
> ---
> include/linux/device.h |    7 +++++++
> 1 file changed, 7 insertions(+)
> 
> --- linux/include/linux/device.h	2008-05-19 12:29:54.000000000 +0800
> +++ linux.new/include/linux/device.h	2008-05-19 14:42:25.000000000 +0800
> @@ -529,4 +529,11 @@ extern const char *dev_driver_string(str
>  	MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>  #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
>  	MODULE_ALIAS("char-major-" __stringify(major) "-*")
> +
> +#define class_reclassify(class)						\
> +do {									\
> +	static struct lock_class_key	class_key;			\
> +	lockdep_set_class_and_name(&(class)->mutex, &class_key,		\
> +				 (class)->name);			\
> +} while (0)
>  #endif /* _DEVICE_H_ */

I think it would need a lavish comment explaining what it is for, and
the reasons for its existence.

Perhaps this should be a kernel-wide thing in lockdep.h.  But then that
would invite people to use it, and it looks like a bad thing.

device.h does not include lockdep.h, so putting this here assumes that
callees have already included lockdep.h.  This is the sort of
assumption which leads to compilation errors.

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Dave Young]

On Tue, May 20, 2008 at 6:02 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
>
>> Converting class semaphore to mutex cause lockdep warnings due to
>> class_interface_register/unregister will possible call device_add/del
>
> Shouldn't we just fix that?

Andrew, could you tell more?

>
>> For the class_interface users here add a class_reclassify macro to
>> reclassify the lock class of their struct class.
>>
>> Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
>>
>> ---
>> include/linux/device.h |    7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> --- linux/include/linux/device.h      2008-05-19 12:29:54.000000000 +0800
>> +++ linux.new/include/linux/device.h  2008-05-19 14:42:25.000000000 +0800
>> @@ -529,4 +529,11 @@ extern const char *dev_driver_string(str
>>       MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
>>  #define MODULE_ALIAS_CHARDEV_MAJOR(major) \
>>       MODULE_ALIAS("char-major-" __stringify(major) "-*")
>> +
>> +#define class_reclassify(class)                                              \
>> +do {                                                                 \
>> +     static struct lock_class_key    class_key;                      \
>> +     lockdep_set_class_and_name(&(class)->mutex, &class_key,         \
>> +                              (class)->name);                        \
>> +} while (0)
>>  #endif /* _DEVICE_H_ */
>
> I think it would need a lavish comment explaining what it is for, and
> the reasons for its existence.

Yes, will do.

>
> Perhaps this should be a kernel-wide thing in lockdep.h.  But then that
> would invite people to use it, and it looks like a bad thing.

I agree.
There's wellmeant lockdep warnings for some safe code logic every some time.
Yes, this macro violates the lockdep class-based rules, but it can act
as just supplementary. And it could help some users.

>
> device.h does not include lockdep.h, so putting this here assumes that
> callees have already included lockdep.h.  This is the sort of
> assumption which leads to compilation errors.
>
My wrong, thanks.

Regards
dave

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Andrew Morton]

On Tue, 20 May 2008 19:05:21 +0800 "Dave Young" <hidave.darkstar@gmail.com> wrote:

> On Tue, May 20, 2008 at 6:02 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
> >
> >> Converting class semaphore to mutex cause lockdep warnings due to
> >> class_interface_register/unregister will possible call device_add/del
> >
> > Shouldn't we just fix that?
> 
> Andrew, could you tell more?

Well what are these lockdep warnings?  Normally such a warning means that
we have a locking bug. I _assume_ that you've determined that the warnings
are false-positives?

The warning which Mariusz Kozlowski discovered ("Subject: Re:
2.6.26-rc2-mm1: possible circular locking dependency detected") was
triggered by the "class semaphore to mutex" conversion and it looks
like a real bug to me.  Would your patch prevent warnings such as that
one from being available to us?

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Matthew Wilcox]

On Tue, May 20, 2008 at 10:30:45AM -0700, Andrew Morton wrote:
> Well what are these lockdep warnings?  Normally such a warning means that
> we have a locking bug. I _assume_ that you've determined that the warnings
> are false-positives?

Andrew, we already discussed this on the thread you started that you
then ignored ...

> The warning which Mariusz Kozlowski discovered ("Subject: Re:
> 2.6.26-rc2-mm1: possible circular locking dependency detected") was
> triggered by the "class semaphore to mutex" conversion and it looks
> like a real bug to me.  Would your patch prevent warnings such as that
> one from being available to us?

The problem is that you add one type of class which then adds devices
that are of another class.  This is not a bug.  My proposal is to give
each sysfs class its own lock class; Dave's is to only do it for the
two classes he knows about that do this.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Andrew Morton]

On Tue, 20 May 2008 11:36:41 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Tue, May 20, 2008 at 10:30:45AM -0700, Andrew Morton wrote:
> > Well what are these lockdep warnings?  Normally such a warning means that
> > we have a locking bug. I _assume_ that you've determined that the warnings
> > are false-positives?
> 
> Andrew, we already discussed this on the thread you started that you
> then ignored ...

rofl.

All pertinent information should be in a patch's changelog.  Then this
sort of confusion will not occur.

> > The warning which Mariusz Kozlowski discovered ("Subject: Re:
> > 2.6.26-rc2-mm1: possible circular locking dependency detected") was
> > triggered by the "class semaphore to mutex" conversion and it looks
> > like a real bug to me.  Would your patch prevent warnings such as that
> > one from being available to us?
> 
> The problem is that you add one type of class which then adds devices
> that are of another class.  This is not a bug.  My proposal is to give
> each sysfs class its own lock class; Dave's is to only do it for the
> two classes he knows about that do this.

Well that sounds reasonable.  I'm not sure that we should introduce
generic-looking helper infrastructure to do it, however.

Anyway I'll happily sit back and let you guys and Greg sort this one out ;)

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Dave Young]

On Wed, May 21, 2008 at 3:23 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 20 May 2008 11:36:41 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
>
>> On Tue, May 20, 2008 at 10:30:45AM -0700, Andrew Morton wrote:
>> > Well what are these lockdep warnings?  Normally such a warning means that
>> > we have a locking bug. I _assume_ that you've determined that the warnings
>> > are false-positives?
>>
>> Andrew, we already discussed this on the thread you started that you
>> then ignored ...
>
> rofl.
>
> All pertinent information should be in a patch's changelog.  Then this
> sort of confusion will not occur.

My wrong. should do this in advance.

>
>> > The warning which Mariusz Kozlowski discovered ("Subject: Re:
>> > 2.6.26-rc2-mm1: possible circular locking dependency detected") was
>> > triggered by the "class semaphore to mutex" conversion and it looks
>> > like a real bug to me.  Would your patch prevent warnings such as that
>> > one from being available to us?
>>
>> The problem is that you add one type of class which then adds devices
>> that are of another class.  This is not a bug.  My proposal is to give
>> each sysfs class its own lock class; Dave's is to only do it for the
>> two classes he knows about that do this.
>
> Well that sounds reasonable.  I'm not sure that we should introduce
> generic-looking helper infrastructure to do it, however.
>
> Anyway I'll happily sit back and let you guys and Greg sort this one out ;)
>

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Matthew Wilcox]

On Tue, May 20, 2008 at 03:02:32AM -0700, Andrew Morton wrote:
> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
> 
> > Converting class semaphore to mutex cause lockdep warnings due to
> > class_interface_register/unregister will possible call device_add/del
> 
> Shouldn't we just fix that?

That's what I suggested (in a thread you were cc'd on, but probably
aren't reading).  I don't like this reclassify thing either.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Greg KH]

On Tue, May 20, 2008 at 03:02:32AM -0700, Andrew Morton wrote:
> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
> 
> > Converting class semaphore to mutex cause lockdep warnings due to
> > class_interface_register/unregister will possible call device_add/del
> 
> Shouldn't we just fix that?

Um, no, that's a "feature" that some types of hardware and interfaces
require.

This is one reason I really don't like this type of conversion, it's
causing lots of problems for no known gain.

So I would just recommend dropping this patch set, the current "convert
class semaphore to a mutex" patch in the -mm tree is already causing
lockdep warnings, and trying to do something like this isn't really
going to solve the root problem here.

thanks,

greg k-h

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Dave Young]

On Wed, May 21, 2008 at 1:21 AM, Greg KH <greg@kroah.com> wrote:
> On Tue, May 20, 2008 at 03:02:32AM -0700, Andrew Morton wrote:
>> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
>>
>> > Converting class semaphore to mutex cause lockdep warnings due to
>> > class_interface_register/unregister will possible call device_add/del
>>
>> Shouldn't we just fix that?
>
> Um, no, that's a "feature" that some types of hardware and interfaces
> require.
>
> This is one reason I really don't like this type of conversion, it's
> causing lots of problems for no known gain.
>
> So I would just recommend dropping this patch set, the current "convert
> class semaphore to a mutex" patch in the -mm tree is already causing
> lockdep warnings, and trying to do something like this isn't really
> going to solve the root problem here.

At last, I decide to give up.

Andrew, I could not do more for this issue now, you can drop the
conversion patch if there's no suitable fix from others.

Thanks
Dave

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Andrew Morton]

On Tue, 27 May 2008 14:42:51 +0800 "Dave Young" <hidave.darkstar@gmail.com> wrote:

> On Wed, May 21, 2008 at 1:21 AM, Greg KH <greg@kroah.com> wrote:
> > On Tue, May 20, 2008 at 03:02:32AM -0700, Andrew Morton wrote:
> >> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
> >>
> >> > Converting class semaphore to mutex cause lockdep warnings due to
> >> > class_interface_register/unregister will possible call device_add/del
> >>
> >> Shouldn't we just fix that?
> >
> > Um, no, that's a "feature" that some types of hardware and interfaces
> > require.
> >
> > This is one reason I really don't like this type of conversion, it's
> > causing lots of problems for no known gain.
> >
> > So I would just recommend dropping this patch set, the current "convert
> > class semaphore to a mutex" patch in the -mm tree is already causing
> > lockdep warnings, and trying to do something like this isn't really
> > going to solve the root problem here.
> 
> At last, I decide to give up.
> 
> Andrew, I could not do more for this issue now, you can drop the
> conversion patch if there's no suitable fix from others.
> 

If that semaphore is being used as a mutex then we should convert it to
a mutex (dammit).

Leaving it implemented as a semphore is not the proper way of
suppressing the lockdep warnings.  It would be better to convert it to
a mutex then add suitable (and suitably commented) open-coded lockdep
annotations to suppress the runtime warnings.

And afaik that's pretty much what your patch did, except you added that
unpopular macro.  If instead of the macro we were to convert that patch
to add open-coded lockdep annotation, what problems remain?

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Dave Young]

On Tue, May 27, 2008 at 2:59 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Tue, 27 May 2008 14:42:51 +0800 "Dave Young" <hidave.darkstar@gmail.com> wrote:
>
>> On Wed, May 21, 2008 at 1:21 AM, Greg KH <greg@kroah.com> wrote:
>> > On Tue, May 20, 2008 at 03:02:32AM -0700, Andrew Morton wrote:
>> >> On Tue, 20 May 2008 17:55:54 +0800 Dave Young <hidave.darkstar@gmail.com> wrote:
>> >>
>> >> > Converting class semaphore to mutex cause lockdep warnings due to
>> >> > class_interface_register/unregister will possible call device_add/del
>> >>
>> >> Shouldn't we just fix that?
>> >
>> > Um, no, that's a "feature" that some types of hardware and interfaces
>> > require.
>> >
>> > This is one reason I really don't like this type of conversion, it's
>> > causing lots of problems for no known gain.
>> >
>> > So I would just recommend dropping this patch set, the current "convert
>> > class semaphore to a mutex" patch in the -mm tree is already causing
>> > lockdep warnings, and trying to do something like this isn't really
>> > going to solve the root problem here.
>>
>> At last, I decide to give up.
>>
>> Andrew, I could not do more for this issue now, you can drop the
>> conversion patch if there's no suitable fix from others.
>>
>
> If that semaphore is being used as a mutex then we should convert it to
> a mutex (dammit).
>
> Leaving it implemented as a semphore is not the proper way of
> suppressing the lockdep warnings.  It would be better to convert it to
> a mutex then add suitable (and suitably commented) open-coded lockdep
> annotations to suppress the runtime warnings.

I agree.

>
> And afaik that's pretty much what your patch did, except you added that
> unpopular macro.  If instead of the macro we were to convert that patch
> to add open-coded lockdep annotation, what problems remain?
>
>
Actually I want a best fix which could be accepted by all of you, and
by me. It's not so easy.

I can give another try without the macro. Thanks.

---
Regards
dave

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Matthew Wilcox]

On Mon, May 26, 2008 at 11:59:34PM -0700, Andrew Morton wrote:
> If that semaphore is being used as a mutex then we should convert it to
> a mutex (dammit).

Right.

> Leaving it implemented as a semphore is not the proper way of
> suppressing the lockdep warnings.  It would be better to convert it to
> a mutex then add suitable (and suitably commented) open-coded lockdep
> annotations to suppress the runtime warnings.

We don't even have to go that far.  Here's all that's needed:

diff -u a/drivers/base/class.c b/drivers/base/class.c
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -134,7 +134,7 @@
 	}
 }
 
-int class_register(struct class *cls)
+int __class_register(struct class *cls, struct lock_class_key *key)
 {
 	int error;
 
@@ -143,7 +143,7 @@
 	INIT_LIST_HEAD(&cls->devices);
 	INIT_LIST_HEAD(&cls->interfaces);
 	kset_init(&cls->class_dirs);
-	mutex_init(&cls->mutex);
+	__mutex_init(&cls->mutex, "struct class mutex", key);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -389,7 +389,7 @@
 
 EXPORT_SYMBOL_GPL(class_create_file);
 EXPORT_SYMBOL_GPL(class_remove_file);
-EXPORT_SYMBOL_GPL(class_register);
+EXPORT_SYMBOL_GPL(__class_register);
 EXPORT_SYMBOL_GPL(class_unregister);
 EXPORT_SYMBOL_GPL(class_create);
 EXPORT_SYMBOL_GPL(class_destroy);
diff -u a/include/linux/device.h b/include/linux/device.h
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -16,6 +16,7 @@
 #include <linux/kobject.h>
 #include <linux/klist.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/module.h>
@@ -200,7 +201,14 @@
 	int (*resume)(struct device *dev);
 };
 
-extern int __must_check class_register(struct class *class);
+#define class_register(class)			\
+({						\
+	static struct lock_class_key __key;	\
+	__class_register(class, &__key);	\
+})
+
+extern int __must_check __class_register(struct class *class,
+					struct lock_class_key *key);
 extern void class_unregister(struct class *class);
 extern int class_for_each_device(struct class *class, void *data,
 				 int (*fn)(struct device *dev, void *data));

And here's a replacement patch for what's in your tree (to improve git
bisectability):

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/base/class.c b/drivers/base/class.c
index e085af0..d28cb5f 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -134,7 +134,7 @@ static void remove_class_attrs(struct class *cls)
 	}
 }
 
-int class_register(struct class *cls)
+int __class_register(struct class *cls, struct lock_class_key *key)
 {
 	int error;
 
@@ -143,7 +143,7 @@ int class_register(struct class *cls)
 	INIT_LIST_HEAD(&cls->devices);
 	INIT_LIST_HEAD(&cls->interfaces);
 	kset_init(&cls->class_dirs);
-	init_MUTEX(&cls->sem);
+	__mutex_init(&cls->mutex, "struct class mutex", key);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -261,7 +261,7 @@ char *make_class_name(const char *name, struct kobject *kobj)
  * We check the return of @fn each time. If it returns anything
  * other than 0, we break out and return that value.
  *
- * Note, we hold class->sem in this function, so it can not be
+ * Note, we hold class->mutex in this function, so it can not be
  * re-acquired in @fn, otherwise it will self-deadlocking. For
  * example, calls to add or remove class members would be verboten.
  */
@@ -273,7 +273,7 @@ int class_for_each_device(struct class *class, void *data,
 
 	if (!class)
 		return -EINVAL;
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev, &class->devices, node) {
 		dev = get_device(dev);
 		if (dev) {
@@ -284,7 +284,7 @@ int class_for_each_device(struct class *class, void *data,
 		if (error)
 			break;
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return error;
 }
@@ -306,7 +306,7 @@ EXPORT_SYMBOL_GPL(class_for_each_device);
  *
  * Note, you will need to drop the reference with put_device() after use.
  *
- * We hold class->sem in this function, so it can not be
+ * We hold class->mutex in this function, so it can not be
  * re-acquired in @match, otherwise it will self-deadlocking. For
  * example, calls to add or remove class members would be verboten.
  */
@@ -319,7 +319,7 @@ struct device *class_find_device(struct class *class, void *data,
 	if (!class)
 		return NULL;
 
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev, &class->devices, node) {
 		dev = get_device(dev);
 		if (dev) {
@@ -331,7 +331,7 @@ struct device *class_find_device(struct class *class, void *data,
 		} else
 			break;
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return found ? dev : NULL;
 }
@@ -349,13 +349,13 @@ int class_interface_register(struct class_interface *class_intf)
 	if (!parent)
 		return -EINVAL;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_add_tail(&class_intf->node, &parent->interfaces);
 	if (class_intf->add_dev) {
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->add_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	return 0;
 }
@@ -368,13 +368,13 @@ void class_interface_unregister(struct class_interface *class_intf)
 	if (!parent)
 		return;
 
-	down(&parent->sem);
+	mutex_lock(&parent->mutex);
 	list_del_init(&class_intf->node);
 	if (class_intf->remove_dev) {
 		list_for_each_entry(dev, &parent->devices, node)
 			class_intf->remove_dev(dev, class_intf);
 	}
-	up(&parent->sem);
+	mutex_unlock(&parent->mutex);
 
 	class_put(parent);
 }
@@ -389,7 +389,7 @@ int __init classes_init(void)
 
 EXPORT_SYMBOL_GPL(class_create_file);
 EXPORT_SYMBOL_GPL(class_remove_file);
-EXPORT_SYMBOL_GPL(class_register);
+EXPORT_SYMBOL_GPL(__class_register);
 EXPORT_SYMBOL_GPL(class_unregister);
 EXPORT_SYMBOL_GPL(class_create);
 EXPORT_SYMBOL_GPL(class_destroy);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 72eccae..068d3de 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -20,7 +20,6 @@
 #include <linux/notifier.h>
 #include <linux/genhd.h>
 #include <linux/kallsyms.h>
-#include <linux/semaphore.h>
 
 #include "base.h"
 #include "power/power.h"
@@ -833,7 +832,7 @@ int device_add(struct device *dev)
 		klist_add_tail(&dev->knode_parent, &parent->klist_children);
 
 	if (dev->class) {
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* tie the class to the device */
 		list_add_tail(&dev->node, &dev->class->devices);
 
@@ -841,7 +840,7 @@ int device_add(struct device *dev)
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->add_dev)
 				class_intf->add_dev(dev, class_intf);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
  Done:
 	put_device(dev);
@@ -937,14 +936,14 @@ void device_del(struct device *dev)
 	if (dev->class) {
 		device_remove_class_symlinks(dev);
 
-		down(&dev->class->sem);
+		mutex_lock(&dev->class->mutex);
 		/* notify any interfaces that the device is now gone */
 		list_for_each_entry(class_intf, &dev->class->interfaces, node)
 			if (class_intf->remove_dev)
 				class_intf->remove_dev(dev, class_intf);
 		/* remove the device from the class list */
 		list_del_init(&dev->node);
-		up(&dev->class->sem);
+		mutex_unlock(&dev->class->mutex);
 	}
 	device_remove_file(dev, &uevent_attr);
 	device_remove_attrs(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 14616e8..718f4c0 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -16,10 +16,12 @@
 #include <linux/kobject.h>
 #include <linux/klist.h>
 #include <linux/list.h>
+#include <linux/lockdep.h>
 #include <linux/compiler.h>
 #include <linux/types.h>
 #include <linux/module.h>
 #include <linux/pm.h>
+#include <linux/mutex.h>
 #include <linux/semaphore.h>
 #include <asm/atomic.h>
 #include <asm/device.h>
@@ -186,7 +188,7 @@ struct class {
 	struct list_head	devices;
 	struct list_head	interfaces;
 	struct kset		class_dirs;
-	struct semaphore	sem; /* locks children, devices, interfaces */
+	struct mutex		mutex; /* locks devices, interfaces */
 	struct class_attribute		*class_attrs;
 	struct device_attribute		*dev_attrs;
 
@@ -199,7 +201,14 @@ struct class {
 	int (*resume)(struct device *dev);
 };
 
-extern int __must_check class_register(struct class *class);
+#define class_register(class)			\
+({						\
+	static struct lock_class_key __key;	\
+	__class_register(class, &__key);	\
+})
+
+extern int __must_check __class_register(struct class *class,
+					struct lock_class_key *key);
 extern void class_unregister(struct class *class);
 extern int class_for_each_device(struct class *class, void *data,
 				 int (*fn)(struct device *dev, void *data));

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Greg KH]

On Wed, May 28, 2008 at 09:48:40AM -0600, Matthew Wilcox wrote:
> On Mon, May 26, 2008 at 11:59:34PM -0700, Andrew Morton wrote:
> > If that semaphore is being used as a mutex then we should convert it to
> > a mutex (dammit).
> 
> Right.
> 
> > Leaving it implemented as a semphore is not the proper way of
> > suppressing the lockdep warnings.  It would be better to convert it to
> > a mutex then add suitable (and suitably commented) open-coded lockdep
> > annotations to suppress the runtime warnings.
> 
> We don't even have to go that far.  Here's all that's needed:
> 
> diff -u a/drivers/base/class.c b/drivers/base/class.c
> --- a/drivers/base/class.c
> +++ b/drivers/base/class.c
> @@ -134,7 +134,7 @@
>  	}
>  }
>  
> -int class_register(struct class *cls)
> +int __class_register(struct class *cls, struct lock_class_key *key)

Sorry, this will not work properly, as class_create() is now more
commonly called, and it calls class_register() from within the driver
core.  So there would be a lot of classes with the same "key" because of
this.

So try changing class_create also.

thanks,

greg k-h

Re: [PATCH 1/3][-mm] add class_reclassify macro

[Greg KH]

On Wed, May 28, 2008 at 09:06:37AM -0700, Greg KH wrote:
> On Wed, May 28, 2008 at 09:48:40AM -0600, Matthew Wilcox wrote:
> > On Mon, May 26, 2008 at 11:59:34PM -0700, Andrew Morton wrote:
> > > If that semaphore is being used as a mutex then we should convert it to
> > > a mutex (dammit).
> > 
> > Right.
> > 
> > > Leaving it implemented as a semphore is not the proper way of
> > > suppressing the lockdep warnings.  It would be better to convert it to
> > > a mutex then add suitable (and suitably commented) open-coded lockdep
> > > annotations to suppress the runtime warnings.
> > 
> > We don't even have to go that far.  Here's all that's needed:
> > 
> > diff -u a/drivers/base/class.c b/drivers/base/class.c
> > --- a/drivers/base/class.c
> > +++ b/drivers/base/class.c
> > @@ -134,7 +134,7 @@
> >  	}
> >  }
> >  
> > -int class_register(struct class *cls)
> > +int __class_register(struct class *cls, struct lock_class_key *key)
> 
> Sorry, this will not work properly, as class_create() is now more
> commonly called, and it calls class_register() from within the driver
> core.  So there would be a lot of classes with the same "key" because of
> this.
> 
> So try changing class_create also.

Oh forget it, I'm messing around in this area anyway, so all of these
changes are not going to apply to tomorrow's -next tree at all.

I'll take this and get it working, give me a bit...

thanks,

greg k-h