[PATCH] PCI device list locking

[john stultz <johnstul@us.ibm.com>]

[-- Attachment #1.1: Type: text/plain, Size: 864 bytes --]

At Greg K-H's request, I've been looking into locking the pci_devices
list to protect it from being altered by hotplug while being traversed.
So far I've limited my work to just the core pci code, only adding the
pcidevlist_lock spinlock and using it to wrap all uses of
pci_for_each_dev() or pci_devices. I'm planning to go on and lock all
uses of pci_for_each_dev() in the driver and arch specific files, as
well as the bus device lists (and later the bus lists), but I wanted to
send this out for comments first. I don't really have any test cases for
sanity checking, but it boots on an 8 way and running 50 "cat
/proc/pci"s in parallel doesn't choke, so let me know if I've mistakenly
broken anything. 

Thanks 
-john 

(please CC me as I'm not sub'ed to the lists)

I apologize if this is a dup, evo crashed on send and I'm not sure if it
made it out first.

[-- Attachment #1.2: Type: text/html, Size: 1137 bytes --]

[-- Attachment #2: pcidevlist_lock.patch --]
[-- Type: text/plain, Size: 8373 bytes --]

Index: linux24/drivers/pci/pci.c
diff -u linux24/drivers/pci/pci.c:1.1.1.1 linux24/drivers/pci/pci.c:1.1.1.1.28.16
--- linux24/drivers/pci/pci.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/pci.c	Mon Mar  4 15:38:39 2002
@@ -37,6 +37,7 @@
 
 LIST_HEAD(pci_root_buses);
 LIST_HEAD(pci_devices);
+spinlock_t pcidevlist_lock = SPIN_LOCK_UNLOCKED;
 
 /**
  * pci_find_slot - locate PCI device from a given PCI slot
@@ -54,11 +55,14 @@
 pci_find_slot(unsigned int bus, unsigned int devfn)
 {
 	struct pci_dev *dev;
-
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
-		if (dev->bus->number == bus && dev->devfn == devfn)
+		if (dev->bus->number == bus && dev->devfn == devfn){
+			spin_unlock(&pcidevlist_lock);
 			return dev;
+		}
 	}
+	spin_unlock(&pcidevlist_lock);
 	return NULL;
 }
 
@@ -81,17 +85,23 @@
 		unsigned int ss_vendor, unsigned int ss_device,
 		const struct pci_dev *from)
 {
-	struct list_head *n = from ? from->global_list.next : pci_devices.next;
+	struct list_head *n;
+	
+	spin_lock(&pcidevlist_lock);
+	n = from ? from->global_list.next : pci_devices.next;
 
 	while (n != &pci_devices) {
 		struct pci_dev *dev = pci_dev_g(n);
 		if ((vendor == PCI_ANY_ID || dev->vendor == vendor) &&
 		    (device == PCI_ANY_ID || dev->device == device) &&
 		    (ss_vendor == PCI_ANY_ID || dev->subsystem_vendor == ss_vendor) &&
-		    (ss_device == PCI_ANY_ID || dev->subsystem_device == ss_device))
+		    (ss_device == PCI_ANY_ID || dev->subsystem_device == ss_device)){
+			spin_unlock(&pcidevlist_lock);
 			return dev;
+		}
 		n = n->next;
 	}
+	spin_unlock(&pcidevlist_lock);
 	return NULL;
 }
 
@@ -130,14 +140,19 @@
 struct pci_dev *
 pci_find_class(unsigned int class, const struct pci_dev *from)
 {
-	struct list_head *n = from ? from->global_list.next : pci_devices.next;
+	struct list_head *n;
 
+	spin_lock(&pcidevlist_lock);
+	n = from ? from->global_list.next : pci_devices.next;
 	while (n != &pci_devices) {
 		struct pci_dev *dev = pci_dev_g(n);
-		if (dev->class == class)
+		if (dev->class == class){
+			spin_unlock(&pcidevlist_lock);
 			return dev;
+		}
 		n = n->next;
 	}
+	spin_unlock(&pcidevlist_lock);
 	return NULL;
 }
 
@@ -605,10 +620,12 @@
 	int count = 0;
 
 	list_add_tail(&drv->node, &pci_drivers);
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		if (!pci_dev_driver(dev))
 			count += pci_announce_device(drv, dev);
 	}
+	spin_unlock(&pcidevlist_lock);
 	return count;
 }
 
@@ -628,6 +645,7 @@
 	struct pci_dev *dev;
 
 	list_del(&drv->node);
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		if (dev->driver == drv) {
 			if (drv->remove)
@@ -635,6 +653,7 @@
 			dev->driver = NULL;
 		}
 	}
+	spin_unlock(&pcidevlist_lock);
 }
 
 #ifdef CONFIG_HOTPLUG
@@ -700,7 +719,7 @@
 		if (drv->remove && pci_announce_device(drv, dev))
 			break;
 	}
-
+	
 	/* notify userspace of new hotplug device */
 	run_sbin_hotplug(dev, TRUE);
 }
@@ -716,7 +735,9 @@
 pci_insert_device(struct pci_dev *dev, struct pci_bus *bus)
 {
 	list_add_tail(&dev->bus_list, &bus->devices);
+	spin_lock(&pcidevlist_lock);
 	list_add_tail(&dev->global_list, &pci_devices);
+	spin_unlock(&pcidevlist_lock);
 #ifdef CONFIG_PROC_FS
 	pci_proc_attach_device(dev);
 #endif
@@ -751,7 +772,9 @@
 		dev->driver = NULL;
 	}
 	list_del(&dev->bus_list);
+	spin_lock(&pcidevlist_lock); 
 	list_del(&dev->global_list);
+	spin_unlock(&pcidevlist_lock);
 	pci_free_resources(dev);
 #ifdef CONFIG_PROC_FS
 	pci_proc_detach_device(dev);
@@ -1327,7 +1350,9 @@
 		 * Link the device to both the global PCI device chain and
 		 * the per-bus list of devices.
 		 */
+		spin_lock(&pcidevlist_lock);
 		list_add_tail(&dev->global_list, &pci_devices);
+		spin_unlock(&pcidevlist_lock);
 		list_add_tail(&dev->bus_list, &bus->devices);
 
 		/* Fix up broken headers */
@@ -1368,7 +1393,6 @@
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE || dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
 				max = pci_scan_bridge(bus, dev, max, pass);
 		}
-
 	/*
 	 * We've scanned the bus and so we know all about what's on
 	 * the other side of any bridges that may be on this bus plus
@@ -1935,9 +1959,11 @@
 
 	pcibios_init();
 
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		pci_fixup_device(PCI_FIXUP_FINAL, dev);
 	}
+	spin_unlock(&pcidevlist_lock);
 
 #ifdef CONFIG_PM
 	pm_register(PM_PCI_DEV, 0, pci_pm_callback);
Index: linux24/drivers/pci/proc.c
diff -u linux24/drivers/pci/proc.c:1.1.1.1 linux24/drivers/pci/proc.c:1.1.1.1.28.2
--- linux24/drivers/pci/proc.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/proc.c	Fri Mar  1 12:51:09 2002
@@ -306,17 +306,26 @@
 /* iterator */
 static void *pci_seq_start(struct seq_file *m, loff_t *pos)
 {
-	struct list_head *p = &pci_devices;
-	loff_t n = *pos;
-
-	/* XXX: surely we need some locking for traversing the list? */
+	struct list_head *p;
+	loff_t n;
+	spin_lock(&pcidevlist_lock);
+	p = &pci_devices;
+	n = *pos;
 	while (n--) {
 		p = p->next;
-		if (p == &pci_devices)
+		if (p == &pci_devices){
+			spin_unlock(&pcidevlist_lock);
 			return NULL;
+		}
 	}
+	spin_unlock(&pcidevlist_lock);
 	return p;
 }
+
+/*
+XXX: pci_seq_next() doesn't use pcidevlist_lock, make sure 
+you hold it before calling. (Currently unused?)
+*/
 static void *pci_seq_next(struct seq_file *m, void *v, loff_t *pos)
 {
 	struct list_head *p = v;
@@ -559,9 +568,11 @@
 		entry = create_proc_entry("devices", 0, proc_bus_pci_dir);
 		if (entry)
 			entry->proc_fops = &proc_bus_pci_dev_operations;
+		spin_lock(&pcidevlist_lock);
 		pci_for_each_dev(dev) {
 			pci_proc_attach_device(dev);
 		}
+		spin_unlock(&pcidevlist_lock);
 		entry = create_proc_entry("pci", 0, NULL);
 		if (entry)
 			entry->proc_fops = &proc_pci_operations;
Index: linux24/drivers/pci/setup-bus.c
diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.3
--- linux24/drivers/pci/setup-bus.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/setup-bus.c	Mon Mar  4 15:33:30 2002
@@ -222,9 +222,11 @@
 		ranges.found_vga = 0;
 		pbus_assign_resources(b, &ranges);
 	}
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		pdev_enable_device(dev);
 	}
+	spin_unlock(&pcidevlist_lock);
 }
 
 /* Check whether the bridge supports I/O forwarding.
Index: linux24/drivers/pci/setup-irq.c
diff -u linux24/drivers/pci/setup-irq.c:1.1.1.1 linux24/drivers/pci/setup-irq.c:1.1.1.1.28.1
--- linux24/drivers/pci/setup-irq.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/setup-irq.c	Fri Mar  1 14:27:55 2002
@@ -65,7 +65,9 @@
 	       int (*map_irq)(struct pci_dev *, u8, u8))
 {
 	struct pci_dev *dev;
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		pdev_fixup_irq(dev, swizzle, map_irq);
 	}
+	spin_unlock(&pcidevlist_lock);
 }
Index: linux24/drivers/pnp/isapnp.c
diff -u linux24/drivers/pnp/isapnp.c:1.1.1.2 linux24/drivers/pnp/isapnp.c:1.1.1.2.2.3
--- linux24/drivers/pnp/isapnp.c:1.1.1.2	Wed Feb 13 14:53:31 2002
+++ linux24/drivers/pnp/isapnp.c	Mon Mar  4 15:33:30 2002
@@ -1685,10 +1685,14 @@
 	}
 #ifdef CONFIG_PCI
 	if (!isapnp_skip_pci_scan) {
+		spin_lock(&pcidevlist_lock);
 		pci_for_each_dev(dev) {
-			if (dev->irq == irq)
+			if (dev->irq == irq){
+				spin_unlock(&pcidevlist_lock);
 				return 1;
+			}
 		}
+		spin_unlock(&pcidevlist_lock);
 	}
 #endif
 	if (request_irq(irq, isapnp_test_handler, SA_INTERRUPT, "isapnp", NULL))
Index: linux24/include/linux/pci.h
diff -u linux24/include/linux/pci.h:1.1.1.1 linux24/include/linux/pci.h:1.1.1.1.28.7
--- linux24/include/linux/pci.h:1.1.1.1	Tue Dec 18 15:49:00 2001
+++ linux24/include/linux/pci.h	Wed Feb 27 15:11:05 2002
@@ -16,7 +16,7 @@
 
 #ifndef LINUX_PCI_H
 #define LINUX_PCI_H
-
+#include <linux/spinlock.h>
 /*
  * Under PCI, each device has 256 bytes of configuration address space,
  * of which the first 64 bytes are standardized as follows:
@@ -435,6 +435,7 @@
 
 extern struct list_head pci_root_buses;	/* list of all known PCI buses */
 extern struct list_head pci_devices;	/* list of all devices */
+extern spinlock_t pcidevlist_lock;
 
 /*
  * Error values that may be returned by PCI functions.