Re: [PATCH] PCI device list locking

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

[-- Attachment #1: Type: text/plain, Size: 487 bytes --]

On Mon, 2002-03-04 at 16:17, Greg KH wrote:
> In short, I don't think you can use a spinlock, but rather a semaphore
> is probably necessary.

Sage advice indeed!

Take 2: with semaphores

Again, this is just fixes to the core pci code. Various drivers and arch
specific code still traverse the pci_devices list without the
pcidevlist_sem. If there are no complaints with this code (probably just
wishful thinking) I'll go on and clean up those unprotected traversals. 

thanks,
-john



[-- Attachment #2: pcidevlist_lock.patch --]
[-- Type: text/plain, Size: 7537 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.20
--- linux24/drivers/pci/pci.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/pci.c	Mon Mar  4 17:03:27 2002
@@ -37,6 +37,7 @@
 
 LIST_HEAD(pci_root_buses);
 LIST_HEAD(pci_devices);
+DECLARE_MUTEX(pcidevlist_sem);
 
 /**
  * pci_find_slot - locate PCI device from a given PCI slot
@@ -55,10 +56,14 @@
 {
 	struct pci_dev *dev;
 
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
-		if (dev->bus->number == bus && dev->devfn == devfn)
+		if (dev->bus->number == bus && dev->devfn == devfn){
+			up(&pcidevlist_sem);
 			return dev;
+		}
 	}
+	up(&pcidevlist_sem);
 	return NULL;
 }
 
@@ -81,17 +86,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;
+	
+	down(&pcidevlist_sem);
+	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)){
+			up(&pcidevlist_sem);
 			return dev;
+		}
 		n = n->next;
 	}
+	up(&pcidevlist_sem);
 	return NULL;
 }
 
@@ -130,14 +141,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;
 
+	down(&pcidevlist_sem);
+	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){
+			up(&pcidevlist_sem);
 			return dev;
+		}
 		n = n->next;
 	}
+	up(&pcidevlist_sem);
 	return NULL;
 }
 
@@ -605,10 +621,12 @@
 	int count = 0;
 
 	list_add_tail(&drv->node, &pci_drivers);
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		if (!pci_dev_driver(dev))
 			count += pci_announce_device(drv, dev);
 	}
+	up(&pcidevlist_sem);
 	return count;
 }
 
@@ -628,6 +646,7 @@
 	struct pci_dev *dev;
 
 	list_del(&drv->node);
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		if (dev->driver == drv) {
 			if (drv->remove)
@@ -635,6 +654,7 @@
 			dev->driver = NULL;
 		}
 	}
+	up(&pcidevlist_sem);
 }
 
 #ifdef CONFIG_HOTPLUG
@@ -716,7 +736,9 @@
 pci_insert_device(struct pci_dev *dev, struct pci_bus *bus)
 {
 	list_add_tail(&dev->bus_list, &bus->devices);
+	down(&pcidevlist_sem);
 	list_add_tail(&dev->global_list, &pci_devices);
+	up(&pcidevlist_sem);
 #ifdef CONFIG_PROC_FS
 	pci_proc_attach_device(dev);
 #endif
@@ -751,7 +773,9 @@
 		dev->driver = NULL;
 	}
 	list_del(&dev->bus_list);
+	down(&pcidevlist_sem); 
 	list_del(&dev->global_list);
+	up(&pcidevlist_sem);
 	pci_free_resources(dev);
 #ifdef CONFIG_PROC_FS
 	pci_proc_detach_device(dev);
@@ -1327,7 +1351,9 @@
 		 * Link the device to both the global PCI device chain and
 		 * the per-bus list of devices.
 		 */
+		down(&pcidevlist_sem);
 		list_add_tail(&dev->global_list, &pci_devices);
+		up(&pcidevlist_sem);
 		list_add_tail(&dev->bus_list, &bus->devices);
 
 		/* Fix up broken headers */
@@ -1935,9 +1961,11 @@
 
 	pcibios_init();
 
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		pci_fixup_device(PCI_FIXUP_FINAL, dev);
 	}
+	up(&pcidevlist_sem);
 
 #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.3
--- linux24/drivers/pci/proc.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/proc.c	Mon Mar  4 16:55:22 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;
+	down(&pcidevlist_sem);
+	p = &pci_devices;
+	n = *pos;
 	while (n--) {
 		p = p->next;
-		if (p == &pci_devices)
+		if (p == &pci_devices){
+			up(&pcidevlist_sem);
 			return NULL;
+		}
 	}
+	up(&pcidevlist_sem);
 	return p;
 }
+
+/*
+XXX: pci_seq_next() doesn't use pcidevlist_sem, make sure 
+you down 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;
+		down(&pcidevlist_sem);
 		pci_for_each_dev(dev) {
 			pci_proc_attach_device(dev);
 		}
+		up(&pcidevlist_sem);
 		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.4
--- 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 16:55:22 2002
@@ -222,9 +222,11 @@
 		ranges.found_vga = 0;
 		pbus_assign_resources(b, &ranges);
 	}
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		pdev_enable_device(dev);
 	}
+	up(&pcidevlist_sem);
 }
 
 /* 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.2
--- linux24/drivers/pci/setup-irq.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/setup-irq.c	Mon Mar  4 16:55:22 2002
@@ -65,7 +65,9 @@
 	       int (*map_irq)(struct pci_dev *, u8, u8))
 {
 	struct pci_dev *dev;
+	down(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		pdev_fixup_irq(dev, swizzle, map_irq);
 	}
+	up(&pcidevlist_sem);
 }
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.4
--- linux24/drivers/pnp/isapnp.c:1.1.1.2	Wed Feb 13 14:53:31 2002
+++ linux24/drivers/pnp/isapnp.c	Mon Mar  4 16:55:22 2002
@@ -1685,10 +1685,14 @@
 	}
 #ifdef CONFIG_PCI
 	if (!isapnp_skip_pci_scan) {
+		down(&pcidevlist_sem);
 		pci_for_each_dev(dev) {
-			if (dev->irq == irq)
+			if (dev->irq == irq){
+				up(&pcidevlist_sem);
 				return 1;
+			}
 		}
+		up(&pcidevlist_sem);
 	}
 #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.9
--- linux24/include/linux/pci.h:1.1.1.1	Tue Dec 18 15:49:00 2001
+++ linux24/include/linux/pci.h	Mon Mar  4 16:57:00 2002
@@ -16,7 +16,7 @@
 
 #ifndef LINUX_PCI_H
 #define LINUX_PCI_H
-
+#include <asm/semaphore.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 struct semaphore pcidevlist_sem;
 
 /*
  * Error values that may be returned by PCI functions.