sysfs: fix s_active lockdep warning

Recently we met a lockdep warning from sysfs during s2ram or cpu hotplug. As reported by several people, it is something like:

[ 6967.926563] ACPI: Preparing to enter system sleep state S3
[ 6967.956156] Disabling non-boot CPUs ...
[ 6967.970401]
[ 6967.970408] =============================================
[ 6967.970419] [ INFO: possible recursive locking detected ]
[ 6967.970431] 2.6.33-rc2-git6 #27
[ 6967.970439] ---------------------------------------------
[ 6967.970450] pm-suspend/22147 is trying to acquire lock:
[ 6967.970460]  (s_active){++++.+}, at: [<c10d2941>]
sysfs_hash_and_remove+0x3d/0x4f
[ 6967.970493]
[ 6967.970497] but task is already holding lock:
[ 6967.970506]  (s_active){++++.+}, at: [<c10d4110>]

sysfs_get_active_two+0x16/0x36
[...]

Eric already provides a patch for this[1], but it still can't fix the problem. I add the missing part of Eric's patch and send these two patches together, hopefully we can fix the warning completely.

1. http://lkml.org/lkml/2010/1/10/282

Reported-by Benjamin Herrenschmidt <[email protected]>

Reported-by Larry Finger <[email protected]>

Reported-by Miles Lane <[email protected]>

Reported-by Heiko Carstens <[email protected]>

Signed-off-by WANG Cong <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Quoted 26 lines · Amerigo Wang

Recently we met a lockdep warning from sysfs during s2ram or cpu hotplug. As reported by several people, it is something like:

[ 6967.926563] ACPI: Preparing to enter system sleep state S3
[ 6967.956156] Disabling non-boot CPUs ...
[ 6967.970401]
[ 6967.970408] =============================================
[ 6967.970419] [ INFO: possible recursive locking detected ]
[ 6967.970431] 2.6.33-rc2-git6 #27
[ 6967.970439] ---------------------------------------------
[ 6967.970450] pm-suspend/22147 is trying to acquire lock:
[ 6967.970460]  (s_active){++++.+}, at: [<c10d2941>]
sysfs_hash_and_remove+0x3d/0x4f
[ 6967.970493]
[ 6967.970497] but task is already holding lock:
[ 6967.970506]  (s_active){++++.+}, at: [<c10d4110>]

sysfs_get_active_two+0x16/0x36
[...]

Eric already provides a patch for this[1], but it still can't fix the problem. I add the missing part of Eric's patch and send these two patches together, hopefully we can fix the warning completely.

1. http://lkml.org/lkml/2010/1/10/282

Quoted 8 lines

Reported-by Benjamin Herrenschmidt <[email protected]>

Reported-by Larry Finger <[email protected]>

Reported-by Miles Lane <[email protected]>

Reported-by Heiko Carstens <[email protected]>

Signed-off-by WANG Cong <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Thanks for following up on this.

I suspect we may want to create a separate class for each sysfs file instead of playing whack-a-mole and creating a subclass each time we have problems.

I don't see why the rules for one sysfs file should be the same as for any other sysfs file.

Quoted 43 lines · Eric W. Biederman

Quoted 26 lines · Amerigo Wang

Recently we met a lockdep warning from sysfs during s2ram or cpu hotplug. As reported by several people, it is something like:

[ 6967.926563] ACPI: Preparing to enter system sleep state S3
[ 6967.956156] Disabling non-boot CPUs ...
[ 6967.970401]
[ 6967.970408] =============================================
[ 6967.970419] [ INFO: possible recursive locking detected ]
[ 6967.970431] 2.6.33-rc2-git6 #27
[ 6967.970439] ---------------------------------------------
[ 6967.970450] pm-suspend/22147 is trying to acquire lock:
[ 6967.970460]  (s_active){++++.+}, at: [<c10d2941>]
sysfs_hash_and_remove+0x3d/0x4f
[ 6967.970493]
[ 6967.970497] but task is already holding lock:
[ 6967.970506]  (s_active){++++.+}, at: [<c10d4110>]

sysfs_get_active_two+0x16/0x36
[...]

Eric already provides a patch for this[1], but it still can't fix the problem. I add the missing part of Eric's patch and send these two patches together, hopefully we can fix the warning completely.

1. http://lkml.org/lkml/2010/1/10/282

Quoted 8 lines

Reported-by Benjamin Herrenschmidt <[email protected]>

Reported-by Larry Finger <[email protected]>

Reported-by Miles Lane <[email protected]>

Reported-by Heiko Carstens <[email protected]>

Signed-off-by WANG Cong <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Thanks for following up on this.

I suspect we may want to create a separate class for each sysfs file instead of playing whack-a-mole and creating a subclass each time we have problems.

Quoted 3 lines

I don't see why the rules for one sysfs file should be the same as for any other sysfs file.

I am confused, we don't know who created sysfs files unless we separate them by subclasses, the way of your patch is very straight ward.

Quoted 44 lines · Cong Wang

Quoted 43 lines · Eric W. Biederman

Quoted 26 lines · Amerigo Wang

Recently we met a lockdep warning from sysfs during s2ram or cpu hotplug. As reported by several people, it is something like:

[ 6967.926563] ACPI: Preparing to enter system sleep state S3
[ 6967.956156] Disabling non-boot CPUs ...
[ 6967.970401]
[ 6967.970408] =============================================
[ 6967.970419] [ INFO: possible recursive locking detected ]
[ 6967.970431] 2.6.33-rc2-git6 #27
[ 6967.970439] ---------------------------------------------
[ 6967.970450] pm-suspend/22147 is trying to acquire lock:
[ 6967.970460]  (s_active){++++.+}, at: [<c10d2941>]
sysfs_hash_and_remove+0x3d/0x4f
[ 6967.970493]
[ 6967.970497] but task is already holding lock:
[ 6967.970506]  (s_active){++++.+}, at: [<c10d4110>]

sysfs_get_active_two+0x16/0x36
[...]

Eric already provides a patch for this[1], but it still can't fix the problem. I add the missing part of Eric's patch and send these two patches together, hopefully we can fix the warning completely.

1. http://lkml.org/lkml/2010/1/10/282

Quoted 8 lines

Reported-by Benjamin Herrenschmidt <[email protected]>

Reported-by Larry Finger <[email protected]>

Reported-by Miles Lane <[email protected]>

Reported-by Heiko Carstens <[email protected]>

Signed-off-by WANG Cong <[email protected]>

Signed-off-by Eric W. Biederman <[email protected]>

Cc Tejun Heo <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Thanks for following up on this.

I suspect we may want to create a separate class for each sysfs file instead of playing whack-a-mole and creating a subclass each time we have problems.

Quoted 7 lines

Quoted 3 lines

I don't see why the rules for one sysfs file should be the same as for any other sysfs file.

I am confused, we don't know who created sysfs files unless we separate them by subclasses, the way of your patch is very straight ward.

The assumption is that all entities in a class are used very similarly. What I was suggesting is that it may make sense, and be simpler to have a separate __key value and thus place each sysfs file in it's class.

Doing that is a lot more for lockdep to track, but it would not produce the confusing false positives that we see now. From the reports I have seen we may have more that 16 subclasses in sysfs, and it will likely take us a while to find them all.

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

Quoted 5 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

And here I through Alan Stern had a handle on making the driver core play nice.

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

Quoted 2 lines

And here I through Alan Stern had a handle on making the driver core play nice.

It's not the driver core that is the issue here, it's that lockdep can't handle the tree structure of devices that is represented in the kernel.

I don't think it is a driver core problem, but rather, a lockdep issue.

Quoted 11 lines · Greg KH

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

And here I through Alan Stern had a handle on making the driver core play nice.

Quoted 4 lines

It's not the driver core that is the issue here, it's that lockdep can't handle the tree structure of devices that is represented in the kernel.

I don't think it is a driver core problem, but rather, a lockdep issue.

Right, we've been over that and I think I added enough lockdep annotations to make it work for the device tree. At least, Alan and I seemed to agree on that last time we talked about it.

Quoted 17 lines · Peter Zijlstra

Quoted 15 lines · Greg KH

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

And here I through Alan Stern had a handle on making the driver core play nice.

It's not the driver core that is the issue here, it's that lockdep can't handle the tree structure of devices that is represented in the kernel.

I don't think it is a driver core problem, but rather, a lockdep issue.

Quoted 3 lines

Right, we've been over that and I think I added enough lockdep annotations to make it work for the device tree. At least, Alan and I seemed to agree on that last time we talked about it.

Ah, I didn't realize that, very nice.

If so, then this sysfs lock stuff should be able to use those annotations and we shouldn't have this issue, right?

Quoted 22 lines · Greg KH

Quoted 20 lines · Peter Zijlstra

Quoted 8 lines · Greg KH

Quoted 7 lines · Peter Zijlstra

Quoted 5 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

Quoted 7 lines

Quoted 2 lines

And here I through Alan Stern had a handle on making the driver core play nice.

It's not the driver core that is the issue here, it's that lockdep can't handle the tree structure of devices that is represented in the kernel.

I don't think it is a driver core problem, but rather, a lockdep issue.

Right, we've been over that and I think I added enough lockdep annotations to make it work for the device tree. At least, Alan and I seemed to agree on that last time we talked about it.

Quoted 4 lines

Ah, I didn't realize that, very nice.

If so, then this sysfs lock stuff should be able to use those annotations and we shouldn't have this issue, right?

I really wouldn't know, I've not yet looked at sysfs to see what the particular issue is. But possibly, if you say the problem space is similar.

We get false positives when the code of a sysfs attribute synchronously removes other sysfs attributes. In general that is not safe due to hotplug etc, but there are specific instances of static sysfs entries like the pm_core where it appears to be safe.

I am not familiar with the device core lockdep issues. Are they similar?

The device tree had the problem that we could basically hold a device lock and an unspecified number of parent locks (iirc this was due to device probing, where we hold the bus lock while probing/adding child device, recursively).

If we place each dev->lock into the same class (which would naively happen), then this would lead to recursive lock warnings. The proposed solution for this is to create MAX_LOCK_DEPTH classes and assign them to the dev->lock depending on the depth in the device tree (Alan said that MAX_LOCK_DEPTH is sufficient for all practical cases).

static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];

device_add() or thereabouts would have something like:

#ifdef CONFIG_PROVE_LOCKING
	BUG_ON(dev->depth >= MAX_LOCK_DEPTH);
	lockdep_set_class(dev->lock, &dev_tree_classes[dev->depth]);

#endif

Then there was a problem were we could lock all child devices while holding the parent device lock (forgot why though), this would, on taking the second child dev->lock, again lead to recursive lock warnings.

Greg:

You have accepted Thomas's patch "drivers/base: Convert dev->sem to mutex". It generates lockdep violations galore during device probing and removal! Luckily lockdep is smart enough only to print the first occurrence. Here's what I get early on during bootup:

Quoted 12 lines

The device tree had the problem that we could basically hold a device lock and an unspecified number of parent locks (iirc this was due to device probing, where we hold the bus lock while probing/adding child device, recursively).

If we place each dev->lock into the same class (which would naively happen), then this would lead to recursive lock warnings. The proposed solution for this is to create MAX_LOCK_DEPTH classes and assign them to the dev->lock depending on the depth in the device tree (Alan said that MAX_LOCK_DEPTH is sufficient for all practical cases).

Quoted 8 lines

static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];

device_add() or thereabouts would have something like:

#ifdef CONFIG_PROVE_LOCKING
	BUG_ON(dev->depth >= MAX_LOCK_DEPTH);
	lockdep_set_class(dev->lock, &dev_tree_classes[dev->depth]);

#endif

Unfortunately this doesn't really work. Here is a patch implementing the scheme:

Index: usb-2.6/drivers/base/core.c
===================================================================

drivers/base/core.c +22 -0

@@ -22,6 +22,7 @@
 #include <linux/kallsyms.h>
 #include <linux/mutex.h>
 #include <linux/async.h>
+#include <linux/sched.h>
 
 #include "base.h"
 #include "power/power.h"

@@ -671,6 +672,26 @@ static void setup_parent(struct device *
 		dev->kobj.parent = kobj;
 }
 
+#ifdef CONFIG_PROVE_LOCKING
+static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];
+
+static void setup_mutex_depth(struct device *dev, struct device *parent)
+{
+	int depth = 0;
+
+	/* Dynamically determine the device's depth in the device tree */
+	while (parent) {
+		++depth;
+		parent = parent->parent;
+	}
+	BUG_ON(depth > MAX_LOCK_DEPTH);
+	lockdep_set_class(&dev->mutex, &dev_tree_classes[depth]);
+}
+#else
+static inline void setup_mutex_depth(struct device *dev,
+		struct device *parent) {}
+#endif
+
 static int device_add_class_symlinks(struct device *dev)
 {
 	int error;

@@ -912,6 +933,7 @@ int device_add(struct device *dev)
 
 	parent = get_device(dev->parent);
 	setup_parent(dev, parent);
+	setup_mutex_depth(dev, parent);
 
 	/* use parent numa_node */
 	if (parent)

This doesn't address the fact that we really have multiple device trees (for example, class devices are handled separately from normal devices). With the above patch installed, I still get lockdep violations farther on during boot:

Quoted 4 lines

This doesn't address the fact that we really have multiple device trees (for example, class devices are handled separately from normal devices). With the above patch installed, I still get lockdep violations farther on during boot:

<snip lockdep splat>

Hmm, so you have multiple interacting trees? I had understood you only had a single device tree.

So how many trees are there, is that fixed?
Does the device know what tree it is going to end up in?

If yes, then you can extend the setup_mutex_depth() function to pick a different class stack for each tree.

Quoted 9 lines · Peter Zijlstra

Quoted 4 lines

This doesn't address the fact that we really have multiple device trees (for example, class devices are handled separately from normal devices). With the above patch installed, I still get lockdep violations farther on during boot:

<snip lockdep splat>

Hmm, so you have multiple interacting trees? I had understood you only had a single device tree.

The real situation is kind of complicated, and I'm not familiar with all the details. But it's certainly true that a driver will want to work with (and lock!) multiple struct device's that don't have a parent-child relation in the tree. The simplest example is regular devices together with class devices, and another might be PCI devices together with their "shadow" ACPI devices.

Quoted 2 lines

So how many trees are there, is that fixed?
Does the device know what tree it is going to end up in?

The driver generally knows, but AFAIK that information is not passed back to the driver core. At least, not directly -- you might say that it could be deduced from the parent pointer, assuming the core already knows all about the parent.

Quoted 2 lines

If yes, then you can extend the setup_mutex_depth() function to pick a different class stack for each tree.

Maybe this could be done.

But for now perhaps a compromise is in
order. We could make the switch from semaphores to mutexes while avoiding lockdep issues by assigning the device mutexes to a "don't-verify" class. Is there such a thing, or could it be added?

Right, so this device stuff is much more complicated than I was led to believe ;-)

So the device core doesn't know, so how are you guys making sure there really are no deadlocks hidden in there somewhere?

Quoted 4 lines

But for now perhaps a compromise is in
order. We could make the switch from semaphores to mutexes while avoiding lockdep issues by assigning the device mutexes to a "don't-verify" class. Is there such a thing, or could it be added?

Something like the below might work, but it should go along with a checkpatch.pl mod to ensure we don't grow any new users (just don't feel like brushing up my perl fu enough to actually make sense of that script)

Quoted 2 lines · Peter Zijlstra

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated? :-)

Quoted 7 lines · Alan Stern

Quoted 2 lines · Peter Zijlstra

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated? :-)

Well, regular tree's aren't all that complicated, but multiple inter-locking trees is a whole different story indeed.

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

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Alan Stern

Quoted 2 lines · Peter Zijlstra

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated? :-)

Well, regular tree's aren't all that complicated, but multiple inter-locking trees is a whole different story indeed.

I ever tried converting device semaphore to mutex, but failed with same issue.

At least now there's no lockdep solution for it, so I recommend revert the mutex converting patch.

following lockdep warning with rc6-mm1:

[    0.397123] [    0.397124] =============================================
[    0.397359] [ INFO: possible recursive locking detected ]
[    0.397480] 2.6.33-rc6-mm1 #1
[    0.397596] ---------------------------------------------
[    0.397717] swapper/1 is trying to acquire lock:
[    0.397836]  (&dev->mutex){+.+...}, at: [<c12662e4>] __driver_attach+0x38/0x63
[    0.398162] [    0.398162] but task is already holding lock:
[    0.398393]  (&dev->mutex){+.+...}, at: [<c12662d8>] __driver_attach+0x2c/0x63
[    0.399999] 

Quoted 25 lines · Dave Young

Quoted 5 lines · Peter Zijlstra

Quoted 3 lines · Alan Stern

On Fri, 5 Feb 2010, Peter Zijlstra wrote:

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated?  :-)

Well, regular tree's aren't all that complicated, but multiple inter-locking trees is a whole different story indeed.

I ever tried converting device semaphore to mutex, but failed with same issue.

At least now there's no lockdep solution for it, so I recommend revert the mutex converting patch.

following lockdep warning with rc6-mm1:

Quoted 12 lines

[    0.397123] [    0.397124]
=============================================
[    0.397359] [ INFO: possible recursive locking detected ] [    0.397480] 2.6.33-rc6-mm1 #1
[    0.397596] --------------------------------------------- [    0.397717] swapper/1 is trying to acquire lock:
[    0.397836]  (&dev->mutex){+.+...}, at: [<c12662e4>] __driver_attach+0x38/0x63
[    0.398162] [    0.398162] but task is already holding lock:
[    0.398393]  (&dev->mutex){+.+...}, at: [<c12662d8>] __driver_attach+0x2c/0x63
[    0.399999]

Alan already provided a patch for this issue earlier in this thread.

Thanks.

Quoted 32 lines · Cong Wang

Quoted 31 lines · Dave Young

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Alan Stern

Quoted 2 lines · Peter Zijlstra

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated? :-)

Well, regular tree's aren't all that complicated, but multiple inter-locking trees is a whole different story indeed.

I ever tried converting device semaphore to mutex, but failed with same issue.

At least now there's no lockdep solution for it, so I recommend revert the mutex converting patch.

following lockdep warning with rc6-mm1:

[    0.397123] [    0.397124]
=============================================
[    0.397359] [ INFO: possible recursive locking detected ]
[    0.397480] 2.6.33-rc6-mm1 #1
[    0.397596] ---------------------------------------------
[    0.397717] swapper/1 is trying to acquire lock:
[    0.397836]  (&dev->mutex){+.+...}, at: [<c12662e4>]
__driver_attach+0x38/0x63
[    0.398162] [    0.398162] but task is already holding lock:
[    0.398393]  (&dev->mutex){+.+...}, at: [<c12662d8>]
__driver_attach+0x2c/0x63
[    0.399999]

Quoted 1 line

Alan already provided a patch for this issue earlier in this thread.

Yes, but device locks can not be classified with regular tree style.

Please read the whole thread.

Quoted 2 lines · Peter Zijlstra

Right, so this device stuff is much more complicated than I was led to believe ;-)

Haven't I told you all along that tree-structured locking is complicated? :-)

Quoted 2 lines

So the device core doesn't know, so how are you guys making sure there really are no deadlocks hidden in there somewhere?

In the code I've seen, deadlocks are avoided by always taking the locks in the same order. But who knows? Maybe there _are_ some hidden deadlocks lurking. For now we can't rely on lockdep to find them, though, because it gets sidetracked by all the false positives.

Quoted 4 lines

But for now perhaps a compromise is in
order. We could make the switch from semaphores to mutexes while avoiding lockdep issues by assigning the device mutexes to a "don't-verify" class. Is there such a thing, or could it be added?

Something like the below might work, but it should go along with a checkpatch.pl mod to ensure we don't grow any new users (just don't feel like brushing up my perl fu enough to actually make sense of that script)

Greg:

You have accepted Thomas's patch "drivers/base: Convert dev->sem to mutex". It generates lockdep violations galore during device probing and removal! Luckily lockdep is smart enough only to print the first occurrence. Here's what I get early on during bootup:

[    0.149911] ACPI: EC: Look up EC in DSDT
[    0.170665] ACPI: Executed 1 blocks of module-level executable AML code
[    0.198111] ACPI: Interpreter enabled
[    0.198267] ACPI: (supports S0 S3 S4 S5)
[    0.198802] ACPI: Using IOAPIC for interrupt routing
[    0.266493] 
[    0.266496] =============================================
[    0.266775] [ INFO: possible recursive locking detected ]
[    0.266917] 2.6.33-rc6 #1
[    0.267051] ---------------------------------------------
[    0.267192] swapper/1 is trying to acquire lock:
[    0.267332]  (&dev->mutex){+.+...}, at: [<c11496be>] __driver_attach+0x38/0x63
[    0.267683] 
[    0.267685] but task is already holding lock:
[    0.267953]  (&dev->mutex){+.+...}, at: [<c11496b2>] __driver_attach+0x2c/0x63
[    0.268000] 
[    0.268000] other info that might help us debug this:
[    0.268000] 1 lock held by swapper/1:
[    0.268000]  #0:  (&dev->mutex){+.+...}, at: [<c11496b2>] __driver_attach+0x2c/0x63
[    0.268000] 
[    0.268000] stack backtrace:
[    0.268000] Pid: 1, comm: swapper Not tainted 2.6.33-rc6 #1
[    0.268000] Call Trace:
[    0.268000]  [<c11c819e>] ? printk+0xf/0x11
[    0.268000]  [<c1041c9b>] __lock_acquire+0x804/0xb47
[    0.268000]  [<c10b2026>] ? sysfs_addrm_finish+0x19/0xe2
[    0.268000]  [<c1042020>] lock_acquire+0x42/0x59
[    0.268000]  [<c11496be>] ? __driver_attach+0x38/0x63
[    0.268000]  [<c11c90c6>] __mutex_lock_common+0x39/0x38f
[    0.268000]  [<c11496be>] ? __driver_attach+0x38/0x63
[    0.268000]  [<c11c94ab>] mutex_lock_nested+0x2b/0x33
[    0.268000]  [<c11496be>] ? __driver_attach+0x38/0x63
[    0.268000]  [<c11496be>] __driver_attach+0x38/0x63
[    0.268000]  [<c1148e0a>] bus_for_each_dev+0x3d/0x67
[    0.268000]  [<c11494cf>] driver_attach+0x14/0x16
[    0.268000]  [<c1149686>] ? __driver_attach+0x0/0x63
[    0.268000]  [<c11491c1>] bus_add_driver+0x92/0x1c5
[    0.268000]  [<c114990f>] driver_register+0x79/0xe0
[    0.268000]  [<c1106d32>] acpi_bus_register_driver+0x3a/0x3c
[    0.268000]  [<c131999f>] acpi_power_init+0x3f/0x5e
[    0.268000]  [<c1319422>] acpi_init+0x28e/0x2c8
[    0.268000]  [<c1319194>] ? acpi_init+0x0/0x2c8
[    0.268000]  [<c1001139>] do_one_initcall+0x4c/0x136
[    0.268000]  [<c130130b>] kernel_init+0x11c/0x16d
[    0.268000]  [<c13011ef>] ? kernel_init+0x0/0x16d
[    0.268000]  [<c1002cba>] kernel_thread_helper+0x6/0x10
[    0.268485] ACPI: Power Resource [GFAN] (on)

Quoted 11 lines · Peter Zijlstra

The device tree had the problem that we could basically hold a device lock and an unspecified number of parent locks (iirc this was due to device probing, where we hold the bus lock while probing/adding child device, recursively).

If we place each dev->lock into the same class (which would naively happen), then this would lead to recursive lock warnings. The proposed solution for this is to create MAX_LOCK_DEPTH classes and assign them to the dev->lock depending on the depth in the device tree (Alan said that MAX_LOCK_DEPTH is sufficient for all practical cases).

Quoted 8 lines

static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];

device_add() or thereabouts would have something like:

#ifdef CONFIG_PROVE_LOCKING
	BUG_ON(dev->depth >= MAX_LOCK_DEPTH);
	lockdep_set_class(dev->lock, &dev_tree_classes[dev->depth]);

#endif

Unfortunately this doesn't really work. Here is a patch implementing the scheme:

Index: usb-2.6/drivers/base/core.c
===================================================================

drivers/base/core.c +22 -0

@@ -22,6 +22,7 @@
 #include <linux/kallsyms.h>
 #include <linux/mutex.h>
 #include <linux/async.h>
+#include <linux/sched.h>
 
 #include "base.h"
 #include "power/power.h"

@@ -671,6 +672,26 @@ static void setup_parent(struct device *
 		dev->kobj.parent = kobj;
 }
 
+#ifdef CONFIG_PROVE_LOCKING
+static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];
+
+static void setup_mutex_depth(struct device *dev, struct device *parent)
+{
+	int depth = 0;
+
+	/* Dynamically determine the device's depth in the device tree */
+	while (parent) {
+		++depth;
+		parent = parent->parent;
+	}
+	BUG_ON(depth > MAX_LOCK_DEPTH);
+	lockdep_set_class(&dev->mutex, &dev_tree_classes[depth]);
+}
+#else
+static inline void setup_mutex_depth(struct device *dev,
+		struct device *parent) {}
+#endif
+
 static int device_add_class_symlinks(struct device *dev)
 {
 	int error;

@@ -912,6 +933,7 @@ int device_add(struct device *dev)
 
 	parent = get_device(dev->parent);
 	setup_parent(dev, parent);
+	setup_mutex_depth(dev, parent);
 
 	/* use parent numa_node */
 	if (parent)

This doesn't address the fact that we really have multiple device trees (for example, class devices are handled separately from normal devices). With the above patch installed, I still get lockdep violations farther on during boot:

[    0.272332] pci_bus 0000:00: on NUMA node 0
[    0.272355] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0._PRT]
[    0.273503] ACPI: PCI Interrupt Routing Table [\_SB_.PCI0.P0P4._PRT]
[    0.279205] 
[    0.279208] =============================================
[    0.279485] [ INFO: possible recursive locking detected ]
[    0.279628] 2.6.33-rc6 #2
[    0.279763] ---------------------------------------------
[    0.279905] swapper/1 is trying to acquire lock:
[    0.280000]  (&dev_tree_classes[depth]#2){+.+.+.}, at: [<c1149776>] device_attach+0x14/0x6e
[    0.280000] 
[    0.280000] but task is already holding lock:
[    0.280000]  (&dev_tree_classes[depth]#2){+.+.+.}, at: [<c11496de>] __driver_attach+0x2c/0x63
[    0.280000] 
[    0.280000] other info that might help us debug this:
[    0.280000] 2 locks held by swapper/1:
[    0.280000]  #0:  (&dev_tree_classes[depth]#2){+.+.+.}, at: [<c11496de>] __driver_attach+0x2c/0x63
[    0.280000]  #1:  (&dev_tree_classes[depth]#3){+.+.+.}, at: [<c11496ea>] __driver_attach+0x38/0x63
[    0.280000] 
[    0.280000] stack backtrace:
[    0.280000] Pid: 1, comm: swapper Not tainted 2.6.33-rc6 #2
[    0.280000] Call Trace:
[    0.280000]  [<c11c81ce>] ? printk+0xf/0x11
[    0.280000]  [<c1041c9b>] __lock_acquire+0x804/0xb47
[    0.280000]  [<c101a73d>] ? spin_unlock_irqrestore+0x8/0xa
[    0.280000]  [<c101a891>] ? __wake_up+0x32/0x3b
[    0.280000]  [<c1042020>] lock_acquire+0x42/0x59
[    0.280000]  [<c1149776>] ? device_attach+0x14/0x6e
[    0.280000]  [<c11c90f6>] __mutex_lock_common+0x39/0x38f
[    0.280000]  [<c1149776>] ? device_attach+0x14/0x6e
[    0.280000]  [<c1040e2e>] ? trace_hardirqs_on+0xb/0xd
[    0.280000]  [<c10ed5a7>] ? kobject_uevent_env+0x2e9/0x30a
[    0.280000]  [<c10ed5a7>] ? kobject_uevent_env+0x2e9/0x30a
[    0.280000]  [<c11c94db>] mutex_lock_nested+0x2b/0x33
[    0.280000]  [<c1149776>] ? device_attach+0x14/0x6e
[    0.280000]  [<c1149776>] device_attach+0x14/0x6e
[    0.280000]  [<c1148aa1>] bus_probe_device+0x1b/0x30
[    0.280000]  [<c1147b6c>] device_add+0x310/0x458
[    0.280000]  [<c10f96ac>] pci_bus_add_device+0xf/0x30
[    0.280000]  [<c10f96f0>] pci_bus_add_devices+0x23/0xdd
[    0.280000]  [<c11c011b>] ? acpi_pci_root_add+0x1cf/0x1ff
[    0.280000]  [<c11088a3>] acpi_pci_root_start+0x11/0x15
[    0.280000]  [<c1106370>] acpi_start_single_object+0x1e/0x3f
[    0.280000]  [<c11064a9>] acpi_device_probe+0x78/0xf4
[    0.280000]  [<c1149632>] driver_probe_device+0x87/0x107
[    0.280000]  [<c11496f9>] __driver_attach+0x47/0x63
[    0.280000]  [<c1148e36>] bus_for_each_dev+0x3d/0x67
[    0.280000]  [<c11494fb>] driver_attach+0x14/0x16
[    0.280000]  [<c11496b2>] ? __driver_attach+0x0/0x63
[    0.280000]  [<c11491ed>] bus_add_driver+0x92/0x1c5
[    0.280000]  [<c1319798>] ? acpi_pci_root_init+0x0/0x25
[    0.280000]  [<c114993b>] driver_register+0x79/0xe0
[    0.280000]  [<c1319798>] ? acpi_pci_root_init+0x0/0x25
[    0.280000]  [<c1106d32>] acpi_bus_register_driver+0x3a/0x3c
[    0.280000]  [<c13197ae>] acpi_pci_root_init+0x16/0x25
[    0.280000]  [<c1001139>] do_one_initcall+0x4c/0x136
[    0.280000]  [<c130130b>] kernel_init+0x11c/0x16d
[    0.280000]  [<c13011ef>] ? kernel_init+0x0/0x16d
[    0.280000]  [<c1002cba>] kernel_thread_helper+0x6/0x10
[    0.328206] ACPI: PCI Interrupt Link [LNKA] (IRQs 3 4 5 6 7 *10 11 12 14 15)
[    0.329223] ACPI: PCI Interrupt Link [LNKB] (IRQs 3 4 *5 6 7 10 11 12 14 15)

Quoted 4 lines

Then there was a problem were we could lock all child devices while holding the parent device lock (forgot why though), this would, on taking the second child dev->lock, again lead to recursive lock warnings.

AFAIK, the code that used to do this is no longer present. There may be other places where it is still done, but I'm not aware of any.

However in view of the other difficulties, it still doesn't seem possible to make device mutexes work with lockdep. I suggest removing Thomas's patch.

Quoted 3 lines

However in view of the other difficulties, it still doesn't seem possible to make device mutexes work with lockdep. I suggest removing Thomas's patch.

Unless Thomas or anyone else thinks of something that can solve these problems, I will do so.

Quoted 6 lines · Greg KH

Quoted 3 lines

However in view of the other difficulties, it still doesn't seem possible to make device mutexes work with lockdep. I suggest removing Thomas's patch.

Unless Thomas or anyone else thinks of something that can solve these problems, I will do so.

Hmm, I have not seen those lockdep splats on -rt where we have the mutex conversion for quite some time already. Need to look at it.

Quoted 8 lines · Thomas Gleixner

Quoted 6 lines · Greg KH

Quoted 3 lines

However in view of the other difficulties, it still doesn't seem possible to make device mutexes work with lockdep. I suggest removing Thomas's patch.

Unless Thomas or anyone else thinks of something that can solve these problems, I will do so.

Quoted 2 lines

Hmm, I have not seen those lockdep splats on -rt where we have the mutex conversion for quite some time already. Need to look at it.

Thomas, did you ever figure out what was going on? It would be nice to know why the -rt kernels don't have any lockdep problems associated with the device mutexes.

+#include <linux/sched.h>

Quoted 1 line · Alan Stern

+#include <linux/sched.h>

#include <linux/lockdep.h>

should be sufficient I think.

Quoted 5 lines · Peter Zijlstra

Quoted 1 line · Alan Stern

+#include <linux/sched.h>

#include <linux/lockdep.h>

should be sufficient I think.

No, it's not. It leaves MAX_LOCK_DEPTH undeclared. Beats me why that symbol ended up in sched.h...

Quoted 6 lines · Alan Stern

Quoted 4 lines · Peter Zijlstra

Quoted 1 line · Alan Stern

+#include <linux/sched.h>

#include <linux/lockdep.h>

Quoted 5 lines

Quoted 1 line

should be sufficient I think.

No, it's not. It leaves MAX_LOCK_DEPTH undeclared. Beats me why that symbol ended up in sched.h...

because task_struct->held_locks uses it, but it seems better to move it into lockdep.h...

Quoted 6 lines · Eric W. Biederman

We get false positives when the code of a sysfs attribute synchronously removes other sysfs attributes. In general that is not safe due to hotplug etc, but there are specific instances of static sysfs entries like the pm_core where it appears to be safe.

I am not familiar with the device core lockdep issues. Are they similar?

The device tree had the problem that we could basically hold a device lock and an unspecified number of parent locks (iirc this was due to device probing, where we hold the bus lock while probing/adding child device, recursively).

If we place each dev->lock into the same class (which would naively happen), then this would lead to recursive lock warnings. The proposed solution for this is to create MAX_LOCK_DEPTH classes and assign them to the dev->lock depending on the depth in the device tree (Alan said that MAX_LOCK_DEPTH is sufficient for all practical cases).

static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];

device_add() or thereabouts would have something like:

#ifdef CONFIG_PROVE_LOCKING
	BUG_ON(dev->depth >= MAX_LOCK_DEPTH);
	lockdep_set_class(dev->lock, &dev_tree_classes[dev->depth]);

#endif

Quoted 6 lines · Eric W. Biederman

We get false positives when the code of a sysfs attribute synchronously removes other sysfs attributes. In general that is not safe due to hotplug etc, but there are specific instances of static sysfs entries like the pm_core where it appears to be safe.

I am not familiar with the device core lockdep issues. Are they similar?

The device tree had the problem that we could basically hold a device lock and an unspecified number of parent locks (iirc this was due to device probing, where we hold the bus lock while probing/adding child device, recursively).

If we place each dev->lock into the same class (which would naively happen), then this would lead to recursive lock warnings. The proposed solution for this is to create MAX_LOCK_DEPTH classes and assign them to the dev->lock depending on the depth in the device tree (Alan said that MAX_LOCK_DEPTH is sufficient for all practical cases).

static struct lock_class_key dev_tree_classes[MAX_LOCK_DEPTH];

device_add() or thereabouts would have something like:

#ifdef CONFIG_PROVE_LOCKING
	BUG_ON(dev->depth >= MAX_LOCK_DEPTH);
	lockdep_set_class(dev->lock, &dev_tree_classes[dev->depth]);

#endif

Then there was a problem were we could lock all child devices while holding the parent device lock (forgot why though), this would, on taking the second child dev->lock, again lead to recursive lock warnings.

We have an annotation for that: lock_nest_lock (currently only spin_lock_nest_lock exists, but mutex_lock_nest_lock is easily created), and this would allow you to do things like:

	mutex_lock(&parent->lock);
	for_each_device_child(child, parent) {
		mutex_lock_nest_lock(&child->lock, &parent->lock);
		...
	}

I hope this helps in figuring out the sysfs case..

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

I suggest we just remove the original patch, as it seems to be causing way too many problems.

Any objections to that?

Quoted 9 lines · Greg KH

Heh, this whole mess is the very reason we didn't add lockdep support to the driver core. Nested devices that all look alike from the driver core, are really different objects and the locking lifetimes are separate, but lockdep can't see that.

I suggest we just remove the original patch, as it seems to be causing way too many problems.

Any objections to that?

I think the hit rate for real problems has been about 25-50%. Of the false positives a lot of those have been, code that is at least questionable.

Furthermore there are problems we can find this way that we won't know about any other way. Unfortunately I haven't had much time to do anything kernel related lately, or I would have done more with this. My comment was about simply about finding a good way to increase the signal to noise ration so investigations can reasonably start with the presumption that code lockdep is complaining about real problems.

The deadlocks that we can hit in sysfs are very nasty to find, they have persisted for years, and they pop back up after they are fixed. So far the pain from lockdep annotations seems a lot lower.

Right now annotating with subclasses as Amerigo is attempting will work, and remove the false positives. I was simply hoping to find a faster way to get there.

So yes, I do object to removing the original patch. Let's put in the work to find a good path to remove the handful of cases that cause false positives.

I suspect we may want to create a separate class for each sysfs file instead of playing whack-a-mole and creating a subclass each time we have problems.