Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758387Ab0BDQfc (ORCPT ); Thu, 4 Feb 2010 11:35:32 -0500 Received: from iolanthe.rowland.org ([192.131.102.54]:59962 "HELO iolanthe.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1758375Ab0BDQfa (ORCPT ); Thu, 4 Feb 2010 11:35:30 -0500 Date: Thu, 4 Feb 2010 11:35:28 -0500 (EST) From: Alan Stern X-X-Sender: stern@iolanthe.rowland.org To: Peter Zijlstra cc: "Eric W. Biederman" , Greg KH , Thomas Gleixner , Cong Wang , Kernel development list , Tejun Heo , Miles Lane , Heiko Carstens , Benjamin Herrenschmidt , Larry Finger , Andrew Morton Subject: Re: [Patch 0/2] sysfs: fix s_active lockdep warning In-Reply-To: <1265283496.24455.1939.camel@laptop> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9381 Lines: 217 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: [] __driver_attach+0x38/0x63 [ 0.267683] [ 0.267685] but task is already holding lock: [ 0.267953] (&dev->mutex){+.+...}, at: [] __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: [] __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] [] ? printk+0xf/0x11 [ 0.268000] [] __lock_acquire+0x804/0xb47 [ 0.268000] [] ? sysfs_addrm_finish+0x19/0xe2 [ 0.268000] [] lock_acquire+0x42/0x59 [ 0.268000] [] ? __driver_attach+0x38/0x63 [ 0.268000] [] __mutex_lock_common+0x39/0x38f [ 0.268000] [] ? __driver_attach+0x38/0x63 [ 0.268000] [] mutex_lock_nested+0x2b/0x33 [ 0.268000] [] ? __driver_attach+0x38/0x63 [ 0.268000] [] __driver_attach+0x38/0x63 [ 0.268000] [] bus_for_each_dev+0x3d/0x67 [ 0.268000] [] driver_attach+0x14/0x16 [ 0.268000] [] ? __driver_attach+0x0/0x63 [ 0.268000] [] bus_add_driver+0x92/0x1c5 [ 0.268000] [] driver_register+0x79/0xe0 [ 0.268000] [] acpi_bus_register_driver+0x3a/0x3c [ 0.268000] [] acpi_power_init+0x3f/0x5e [ 0.268000] [] acpi_init+0x28e/0x2c8 [ 0.268000] [] ? acpi_init+0x0/0x2c8 [ 0.268000] [] do_one_initcall+0x4c/0x136 [ 0.268000] [] kernel_init+0x11c/0x16d [ 0.268000] [] ? kernel_init+0x0/0x16d [ 0.268000] [] kernel_thread_helper+0x6/0x10 [ 0.268485] ACPI: Power Resource [GFAN] (on) On Thu, 4 Feb 2010, Peter Zijlstra wrote: > 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 Unfortunately this doesn't really work. Here is a patch implementing the scheme: Index: usb-2.6/drivers/base/core.c =================================================================== --- usb-2.6.orig/drivers/base/core.c +++ usb-2.6/drivers/base/core.c @@ -22,6 +22,7 @@ #include #include #include +#include #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: [] device_attach+0x14/0x6e [ 0.280000] [ 0.280000] but task is already holding lock: [ 0.280000] (&dev_tree_classes[depth]#2){+.+.+.}, at: [] __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: [] __driver_attach+0x2c/0x63 [ 0.280000] #1: (&dev_tree_classes[depth]#3){+.+.+.}, at: [] __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] [] ? printk+0xf/0x11 [ 0.280000] [] __lock_acquire+0x804/0xb47 [ 0.280000] [] ? spin_unlock_irqrestore+0x8/0xa [ 0.280000] [] ? __wake_up+0x32/0x3b [ 0.280000] [] lock_acquire+0x42/0x59 [ 0.280000] [] ? device_attach+0x14/0x6e [ 0.280000] [] __mutex_lock_common+0x39/0x38f [ 0.280000] [] ? device_attach+0x14/0x6e [ 0.280000] [] ? trace_hardirqs_on+0xb/0xd [ 0.280000] [] ? kobject_uevent_env+0x2e9/0x30a [ 0.280000] [] ? kobject_uevent_env+0x2e9/0x30a [ 0.280000] [] mutex_lock_nested+0x2b/0x33 [ 0.280000] [] ? device_attach+0x14/0x6e [ 0.280000] [] device_attach+0x14/0x6e [ 0.280000] [] bus_probe_device+0x1b/0x30 [ 0.280000] [] device_add+0x310/0x458 [ 0.280000] [] pci_bus_add_device+0xf/0x30 [ 0.280000] [] pci_bus_add_devices+0x23/0xdd [ 0.280000] [] ? acpi_pci_root_add+0x1cf/0x1ff [ 0.280000] [] acpi_pci_root_start+0x11/0x15 [ 0.280000] [] acpi_start_single_object+0x1e/0x3f [ 0.280000] [] acpi_device_probe+0x78/0xf4 [ 0.280000] [] driver_probe_device+0x87/0x107 [ 0.280000] [] __driver_attach+0x47/0x63 [ 0.280000] [] bus_for_each_dev+0x3d/0x67 [ 0.280000] [] driver_attach+0x14/0x16 [ 0.280000] [] ? __driver_attach+0x0/0x63 [ 0.280000] [] bus_add_driver+0x92/0x1c5 [ 0.280000] [] ? acpi_pci_root_init+0x0/0x25 [ 0.280000] [] driver_register+0x79/0xe0 [ 0.280000] [] ? acpi_pci_root_init+0x0/0x25 [ 0.280000] [] acpi_bus_register_driver+0x3a/0x3c [ 0.280000] [] acpi_pci_root_init+0x16/0x25 [ 0.280000] [] do_one_initcall+0x4c/0x136 [ 0.280000] [] kernel_init+0x11c/0x16d [ 0.280000] [] ? kernel_init+0x0/0x16d [ 0.280000] [] 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) > 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/