PCI device list locking

PATCH In review john stultz 5 participants 7 messages Mar 4, 2002 – Mar 10, 2002

Subsystems (2)

PCI Pnp support

lore

Share & tools

lore

john stultz

author

2002-03-04 23:54 UTC #0 lore raw

PATCH 1/1 PCI device list locking

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.

Index: linux24/drivers/pci/pci.c

drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +34 -8

diff -u linux24/drivers/pci/pci.c:1.1.1.1 linux24/drivers/pci/pci.c:1.1.1.1.28.16
@@ -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

drivers/pci/proc.c renamed from drivers/pci/proc.c:1.1.1.1 +16 -5

diff -u linux24/drivers/pci/proc.c:1.1.1.1 linux24/drivers/pci/proc.c:1.1.1.1.28.2
@@ -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

drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.3
@@ -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

drivers/pci/setup-irq.c renamed from drivers/pci/setup-irq.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-irq.c:1.1.1.1 linux24/drivers/pci/setup-irq.c:1.1.1.1.28.1
@@ -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

drivers/pnp/isapnp.c renamed from drivers/pnp/isapnp.c:1.1.1.2 +5 -1

diff -u linux24/drivers/pnp/isapnp.c:1.1.1.2 linux24/drivers/pnp/isapnp.c:1.1.1.2.2.3
@@ -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

include/linux/pci.h renamed from include/linux/pci.h:1.1.1.1 +2 -1

diff -u linux24/include/linux/pci.h:1.1.1.1 linux24/include/linux/pci.h:1.1.1.1.28.7
@@ -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.

Greg KH

2002-03-05 00:17 UTC #1 lore raw

drivers/pci/pci.c line 631

Quoted 13 lines · john stultz

drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +2 -0

@@ -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;
 }

pci_announce_device() calls the pci driver's probe() function, which can easily sleep. Not a good thing :(

drivers/pci/pci.c line 1969

Quoted 12 lines

drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +2 -0

@@ -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);

pci_fixup_device() might also be able to sleep, but I didn't go through all of the different fixup calls to make sure. Did you look at these?

Quoted 5 lines

diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.3
drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +0 -0
@@ -222,9 +222,11 @@
 		ranges.found_vga = 0;

drivers/pci/setup-bus.c line 232

Quoted 10 lines

drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +2 -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.

Can all implementations of pci_write_config_* be safely called with a spinlock held?

In short, I don't think you can use a spinlock, but rather a semaphore is probably necessary.

thanks,

greg k-h

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

john stultz

author

2002-03-05 01:05 UTC #2 lore raw

In reply to john stultz

Quoted 2 lines · Greg KH

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

Index: linux24/drivers/pci/pci.c

drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +33 -5

diff -u linux24/drivers/pci/pci.c:1.1.1.1 linux24/drivers/pci/pci.c:1.1.1.1.28.20
@@ -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

drivers/pci/proc.c renamed from drivers/pci/proc.c:1.1.1.1 +16 -5

diff -u linux24/drivers/pci/proc.c:1.1.1.1 linux24/drivers/pci/proc.c:1.1.1.1.28.3
@@ -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

drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.4
@@ -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

drivers/pci/setup-irq.c renamed from drivers/pci/setup-irq.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-irq.c:1.1.1.1 linux24/drivers/pci/setup-irq.c:1.1.1.1.28.2
@@ -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

drivers/pnp/isapnp.c renamed from drivers/pnp/isapnp.c:1.1.1.2 +5 -1

diff -u linux24/drivers/pnp/isapnp.c:1.1.1.2 linux24/drivers/pnp/isapnp.c:1.1.1.2.2.4
@@ -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

include/linux/pci.h renamed from include/linux/pci.h:1.1.1.1 +2 -1

diff -u linux24/include/linux/pci.h:1.1.1.1 linux24/include/linux/pci.h:1.1.1.1.28.9
@@ -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.

Craig Christophel

2002-03-05 01:12 UTC #3 lore raw

In reply to john stultz

Also if there is only infrequent useage of the list to change the list,
perhaps you should use a rwlock_t instead. It's just as light as a spinlock
but has the advantage of allowing multiple readers.

Craig.

Quoted 56 lines · Greg KH

drivers/pci/pci.c line 630
Quoted 12 lines · john stultz
drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +2 -0
@@ -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;
 }
pci_announce_device() calls the pci driver's probe() function, which can easily sleep. Not a good thing :(
drivers/pci/pci.c line 1969
Quoted 12 lines
drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +2 -0
@@ -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);
pci_fixup_device() might also be able to sleep, but I didn't go through all of the different fixup calls to make sure. Did you look at these?
Quoted 6 lines
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
drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +0 -0
@@ -222,9 +222,11 @@
 		ranges.found_vga = 0;
drivers/pci/setup-bus.c line 232
Quoted 10 lines
drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +2 -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.
Can all implementations of pci_write_config_* be safely called with a spinlock held?
In short, I don't think you can use a spinlock, but rather a semaphore is probably necessary.

Quoted 8 lines

thanks,
greg k-h
_______________________________________________
Kernel-janitor-discuss mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kernel-janitor-discuss

john stultz

author

2002-03-05 01:47 UTC #4 lore raw

In reply to john stultz

Quoted 3 lines · Craig Christophel

Also if there is only infrequent useage of the list to change the list,
perhaps you should use a rwlock_t instead. It's just as light as a spinlock
but has the advantage of allowing multiple readers.

How about rwsem? See attached.

I think I did this properly, but there may be a few boo-boos.

particularly:
o pci_proc_attach_device()
o pdev_enable_device()
o pdev_fixup_irq()

look like reader safe to me, but I could be wrong.

and
o pci_fixup_device()
o pci_announce_device

are writers, as I'm not sure what drv->probe or fixup->hook are doing.

thanks,
-john

Index: linux24/drivers/pci/pci.c

drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +33 -5

diff -u linux24/drivers/pci/pci.c:1.1.1.1 linux24/drivers/pci/pci.c:1.1.1.1.28.23
@@ -37,6 +37,7 @@
 
 LIST_HEAD(pci_root_buses);
 LIST_HEAD(pci_devices);
+DECLARE_RWSEM(pcidevlist_sem);
 
 /**
  * pci_find_slot - locate PCI device from a given PCI slot

@@ -55,10 +56,14 @@
 {
 	struct pci_dev *dev;
 
+	down_read(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
-		if (dev->bus->number == bus && dev->devfn == devfn)
+		if (dev->bus->number == bus && dev->devfn == devfn){
+			up_read(&pcidevlist_sem);
 			return dev;
+		}
 	}
+	up_read(&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_read(&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_read(&pcidevlist_sem);
 			return dev;
+		}
 		n = n->next;
 	}
+	up_read(&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_read(&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_read(&pcidevlist_sem);
 			return dev;
+		}
 		n = n->next;
 	}
+	up_read(&pcidevlist_sem);
 	return NULL;
 }

@@ -605,10 +621,12 @@
 	int count = 0;
 
 	list_add_tail(&drv->node, &pci_drivers);
+	down_write(&pcidevlist_sem); /* XXX: unsure of what dev->probe will do*/
 	pci_for_each_dev(dev) {
 		if (!pci_dev_driver(dev))
 			count += pci_announce_device(drv, dev);
 	}
+	up_write(&pcidevlist_sem);
 	return count;
 }

@@ -628,6 +646,7 @@
 	struct pci_dev *dev;
 
 	list_del(&drv->node);
+	down_write(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		if (dev->driver == drv) {
 			if (drv->remove)

@@ -635,6 +654,7 @@
 			dev->driver = NULL;
 		}
 	}
+	up_write(&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_write(&pcidevlist_sem);
 	list_add_tail(&dev->global_list, &pci_devices);
+	up_write(&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_write(&pcidevlist_sem); 
 	list_del(&dev->global_list);
+	up_write(&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_write(&pcidevlist_sem);
 		list_add_tail(&dev->global_list, &pci_devices);
+		up_write(&pcidevlist_sem);
 		list_add_tail(&dev->bus_list, &bus->devices);
 
 		/* Fix up broken headers */

@@ -1935,9 +1961,11 @@
 
 	pcibios_init();
 
+	down_write(&pcidevlist_sem); /*XXX: unsure what fixup->hook will do*/
 	pci_for_each_dev(dev) {
 		pci_fixup_device(PCI_FIXUP_FINAL, dev);
 	}
+	up_write(&pcidevlist_sem);
 
 #ifdef CONFIG_PM
 	pm_register(PM_PCI_DEV, 0, pci_pm_callback);

Index: linux24/drivers/pci/proc.c

drivers/pci/proc.c renamed from drivers/pci/proc.c:1.1.1.1 +16 -5

diff -u linux24/drivers/pci/proc.c:1.1.1.1 linux24/drivers/pci/proc.c:1.1.1.1.28.5
@@ -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_read(&pcidevlist_sem);
+	p = &pci_devices;
+	n = *pos;
 	while (n--) {
 		p = p->next;
-		if (p == &pci_devices)
+		if (p == &pci_devices){
+			up_read(&pcidevlist_sem);
 			return NULL;
+		}
 	}
+	up_read(&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_read(&pcidevlist_sem);
 		pci_for_each_dev(dev) {
 			pci_proc_attach_device(dev);
 		}
+		up_read(&pcidevlist_sem); 
 		entry = create_proc_entry("pci", 0, NULL);
 		if (entry)
 			entry->proc_fops = &proc_pci_operations;

Index: linux24/drivers/pci/setup-bus.c

drivers/pci/setup-bus.c renamed from drivers/pci/setup-bus.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.6
@@ -222,9 +222,11 @@
 		ranges.found_vga = 0;
 		pbus_assign_resources(b, &ranges);
 	}
+	down_read(&pcidevlist_sem); 
 	pci_for_each_dev(dev) {
 		pdev_enable_device(dev);
 	}
+	up_read(&pcidevlist_sem); 
 }
 
 /* Check whether the bridge supports I/O forwarding.

Index: linux24/drivers/pci/setup-irq.c

drivers/pci/setup-irq.c renamed from drivers/pci/setup-irq.c:1.1.1.1 +2 -0

diff -u linux24/drivers/pci/setup-irq.c:1.1.1.1 linux24/drivers/pci/setup-irq.c:1.1.1.1.28.4
@@ -65,7 +65,9 @@
 	       int (*map_irq)(struct pci_dev *, u8, u8))
 {
 	struct pci_dev *dev;
+	down_read(&pcidevlist_sem);
 	pci_for_each_dev(dev) {
 		pdev_fixup_irq(dev, swizzle, map_irq);
 	}
+	up_read(&pcidevlist_sem);
 }

Index: linux24/drivers/pnp/isapnp.c

drivers/pnp/isapnp.c renamed from drivers/pnp/isapnp.c:1.1.1.2 +5 -1

diff -u linux24/drivers/pnp/isapnp.c:1.1.1.2 linux24/drivers/pnp/isapnp.c:1.1.1.2.2.5
@@ -1685,10 +1685,14 @@
 	}
 #ifdef CONFIG_PCI
 	if (!isapnp_skip_pci_scan) {
+		down_read(&pcidevlist_sem);
 		pci_for_each_dev(dev) {
-			if (dev->irq == irq)
+			if (dev->irq == irq){
+				up_read(&pcidevlist_sem);
 				return 1;
+			}
 		}
+		up_read(&pcidevlist_sem);
 	}
 #endif
 	if (request_irq(irq, isapnp_test_handler, SA_INTERRUPT, "isapnp", NULL))

Index: linux24/include/linux/pci.h

include/linux/pci.h renamed from include/linux/pci.h:1.1.1.1 +2 -1

diff -u linux24/include/linux/pci.h:1.1.1.1 linux24/include/linux/pci.h:1.1.1.1.28.10
@@ -16,7 +16,7 @@
 
 #ifndef LINUX_PCI_H
 #define LINUX_PCI_H
-
+#include <linux/rwsem.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 rw_semaphore pcidevlist_sem;
 
 /*
  * Error values that may be returned by PCI functions.

Martin Diehl

2002-03-06 01:31 UTC #5 lore raw

In reply to john stultz

Quoted 9 lines · Greg KH

drivers/pci/pci.c line 628
Quoted 6 lines
drivers/pci/pci.c renamed from drivers/pci/pci.c:1.1.1.1 +2 -0
+	spin_lock(&pcidevlist_lock);
 	pci_for_each_dev(dev) {
 		if (!pci_dev_driver(dev))
 			count += pci_announce_device(drv, dev);
 	}
+	spin_unlock(&pcidevlist_lock);
pci_announce_device() calls the pci driver's probe() function, which can easily sleep. Not a good thing :(

Unless I missed something even a semaphore might deadlock then:

think of a pci driver for bridge-type device which has devices already attached when probed - for example cardbus (or hotplug) controller with device on the downside. If the devices are found during probe(), the driver might want to just add them in one go.

Martin

Martin Mares

2002-03-10 12:07 UTC #6 lore raw

In reply to john stultz

Hello!

When I was actively maintaining the PCI subsystem, I was becoming increasingly unhappy about the lack of locking, but I was unable to find any simple strategy which wouldn't require rewriting of many drivers.

Lots of drivers still use the old probing interface (pci_find_*). Possible
solutions: either add locking around probing loop in all drivers or (maybe better) convert all drivers to the new interface and drop the old one completely.

Quoted 6 lines

Unless I missed something even a semaphore might deadlock then:
think of a pci driver for bridge-type device which has devices already attached when probed - for example cardbus (or hotplug) controller with device on the downside. If the devices are found during probe(), the driver might want to just add them in one go.

Device addition is not a hard problem, it just suffices to define the locking rules a bit better to avoid deadlock. Removal is much worse -- you need to ensure not only nobody is working with the list, but also that nobody is using the pci_dev being removed.

Maybe we could do it this way: (assuming all drivers use the new interface)

o Introduce a global semaphore guarding all PCI lists (the global device list and all per-bus linkages). Add pci_{un,}lock_lists(). [Using any r/w-thing is probably pointless as both reads and writes to the list are very rare.]

o Make pci_for_each_dev call pci_{un,}lock_lists().

o Define that for calling pci_{insert,remove}_device() the lists must be locked by the caller.

o Define that if you want to access any pci_dev, you must either have

     the lists locked or be the owner of the device (pci_dev->driver).

o Define that when you are walking the list by pci_for_each_dev(), you can remove any device except the current one.

Unless I've missed anything, these rules seem to cover all the cases needed including probe/remove functions adding/removing devices on the downside of a bridge.

Have a nice fortnight

Signature · 8 lines

Martin `MJ' Mares   <[email protected]>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
For every complex problem, there's a solution that is simple, neat and wrong.

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel