As register sg_interface, the sg_add will be called, which then will
add device to sg_sysfs_class. This will cause lockdep warning,
please see following email

In this case the locks are from diffrent classi, one is sdev_class,
another is sg_sysfs_class 

Here reclassify the sg_sysfs_class for lockdep

Date: Wed, 14 May 2008 08:56:39 -0700
From: Greg KH <greg@kroah.com>
To: Andrew Morton <akpm@linux-foundation.org>,
	Dave Young <hidave.darkstar@gmail.com>
Cc: linux-scsi@vger.kernel.org, Kay Sievers <kay.sievers@vrfy.org>
Subject: Re: lockdep whine in 2.6.26-rc2-mm1
Message-ID: <20080514155639.GB28594@kroah.com>
References: <20080514000933.56adc131.akpm@linux-foundation.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20080514000933.56adc131.akpm@linux-foundation.org>
User-Agent: Mutt/1.5.16 (2007-06-09)



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

---
drivers/scsi/sg.c |    6 ++++++
1 file changed, 6 insertions(+)

diff -upr linux/drivers/scsi/sg.c linux.new/drivers/scsi/sg.c
--- linux/drivers/scsi/sg.c	2008-05-27 17:09:42.000000000 +0800
+++ linux.new/drivers/scsi/sg.c	2008-05-27 17:09:51.000000000 +0800
@@ -49,6 +49,7 @@ static int sg_version_num = 30534;	/* 2 
 #include <linux/delay.h>
 #include <linux/scatterlist.h>
 #include <linux/blktrace_api.h>
+#include <linux/lockdep.h>
 
 #include "scsi.h"
 #include <scsi/scsi_dbg.h>
@@ -67,6 +68,8 @@ static int sg_proc_init(void);
 static void sg_proc_cleanup(void);
 #endif
 
+static struct lock_class_key sg_class_key;
+
 #define SG_ALLOW_DIO_DEF 0
 #define SG_ALLOW_DIO_CODE /* compile out by commenting this define */
 
@@ -1579,6 +1582,9 @@ init_sg(void)
 		rc = PTR_ERR(sg_sysfs_class);
 		goto err_out;
         }
+
+	lockdep_set_class_and_name(&sg_sysfs_class->mutex, &sg_class_key,
+						sg_sysfs_class->name);
 	sg_sysfs_valid = 1;
 	rc = scsi_register_interface(&sg_interface);
 	if (0 == rc) {
--

[ message continues ]

This isn't really a generic solution, is it?  It only works because we
currently only have two users of the interface functions, so if we
reclassify one they look separate to lockdep.  It will fall over again
if we ever get another one.

Surely the correct fix is to initialise lockdep for the mutex the same
way we did for the semaphore in class_register() (which does exactly the
same locking without triggering lockdep)?  That way we'll also fix the
problem for other conversions of semaphore->mutex.

James



--

[ message continues ]

On Wed, May 28, 2008 at 10:40 PM, James Bottomley

Matthew & greg did the work already.

From my original idea I don't want to do this for all classes, and I
would think it as a rare case.

Regards
dave
--

[ message continues ]

Hardly ... the fact that it showed up in two different cases indicated
the problem to be more generic.

The root cause of the problem was that mutexes are part of the lockdep
infrastructure whereas semaphores aren't and the dynamic initialisation
of mutexes becomes a lockdep problem unless they're keyed and
initiallised correctly. If you want to do more semaphore->mutex
conversions, it would really be useful for you to understand what was
done and why it was necessary ... because it's going to become necessary
again in others of them.

James


--

[ message continues ]

Look, you've been told how to do this properly.  Send me the patch that
does the mutex conversion and I'll do a patch on top of that.  I have no
inclination to go wading in the cesspool of patches that is mm trying to
find yours.

-- 
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 Wed, 28 May 2008 08:49:43 -0600


Feel free to identify the cessful patches so they can be fixed or dropped.


From: Dave Young <hidave.darkstar@gmail.com>

The class_device is already removed, so do the class->sem to mutex conversion.

Signed-off-by: Dave Young <hidave.darkstar@gmail.com>
Cc: Greg KH <greg@kroah.com>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/base/class.c   |   22 +++++++++++-----------
 drivers/base/core.c    |    9 ++++-----
 include/linux/device.h |    3 ++-
 3 files changed, 17 insertions(+), 17 deletions(-)

diff -puN drivers/base/class.c~struct-class-sem-to-mutex-converting drivers/base/class.c
--- a/drivers/base/class.c~struct-class-sem-to-mutex-converting
+++ a/drivers/base/class.c
@@ -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);
 	error = kobject_set_name(&cls->subsys.kobj, "%s", cls->name);
 	if (error)
 		return error;
@@ -268,7 +268,7 @@ char *make_class_name(const char *name, 
  * 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.
  */
@@ -280,7 +280,7 @@ int class_for_each_device(struct class *
 
 	if (!class)
 		return -EINVAL;
-	down(&class->sem);
+	mutex_lock(&class->mutex);
 	list_for_each_entry(dev, &class->devices, node) {
 		if (start) {
 			if (start == dev)
@@ -293,7 +293,7 @@ int class_for_each_device(struct class *
 		if (error)
 			break;
 	}
-	up(&class->sem);
+	mutex_unlock(&class->mutex);
 
 	return error;
 }
@@ -316,7 +316,7 @@ ...
[ message continues ]