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_ */
--

[ message continues ]

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.

--

[ message continues ]

On Tue, May 20, 2008 at 6:02 PM, Andrew Morton



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
My wrong, thanks.

Regards
dave
--

[ message continues ]

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?

--

[ message continues ]

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

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."
--

[ message continues ]

On Tue, 20 May 2008 11:36:41 -0600

rofl.

All pertinent information should be in a patch's changelog.  Then 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 ;)
--

[ message continues ]

On Wed, May 21, 2008 at 3:23 AM, Andrew Morton

--

[ message continues ]

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."
--

[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]

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?

--

[ message continues ]

On Tue, May 27, 2008 at 2:59 PM, Andrew Morton

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
--

[ message continues ]

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 ...
[ message continues ]

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
--

[ message continues ]

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
--

[ message continues ]