[PATCH] PCI device list locking

[PATCH] PCI device list locking

[john stultz]

[-- 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.

Re: [PATCH] PCI device list locking

[Greg KH]

On Mon, Mar 04, 2002 at 03:54:11PM -0800, john stultz wrote:
> @@ -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 :(

> @@ -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?

> 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.

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
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] PCI device list locking

[john stultz]

[-- 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.

Re: [PATCH] PCI device list locking

[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.



Craig.


On Monday 04 March 2002 19:17, Greg KH wrote:
> On Mon, Mar 04, 2002 at 03:54:11PM -0800, john stultz wrote:
> > @@ -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 :(
>
> > @@ -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?
>
> > 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.
>
> 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
>
> _______________________________________________
> Kernel-janitor-discuss mailing list
> Kernel-janitor-discuss@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kernel-janitor-discuss

_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] PCI device list locking

[john stultz]

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

On Mon, 2002-03-04 at 17:12, Craig Christophel wrote:
> 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


[-- Attachment #2: pcidevlist_lock.patch --]
[-- Type: text/plain, Size: 7804 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.23
--- 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:46:15 2002
@@ -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
diff -u linux24/drivers/pci/proc.c:1.1.1.1 linux24/drivers/pci/proc.c:1.1.1.1.28.5
--- linux24/drivers/pci/proc.c:1.1.1.1	Tue Dec 18 15:49:16 2001
+++ linux24/drivers/pci/proc.c	Mon Mar  4 17:43: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_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
diff -u linux24/drivers/pci/setup-bus.c:1.1.1.1 linux24/drivers/pci/setup-bus.c:1.1.1.1.28.6
--- 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 17:43:22 2002
@@ -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
diff -u linux24/drivers/pci/setup-irq.c:1.1.1.1 linux24/drivers/pci/setup-irq.c:1.1.1.1.28.4
--- 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 17:45:13 2002
@@ -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
diff -u linux24/drivers/pnp/isapnp.c:1.1.1.2 linux24/drivers/pnp/isapnp.c:1.1.1.2.2.5
--- linux24/drivers/pnp/isapnp.c:1.1.1.2	Wed Feb 13 14:53:31 2002
+++ linux24/drivers/pnp/isapnp.c	Mon Mar  4 17:31:15 2002
@@ -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
diff -u linux24/include/linux/pci.h:1.1.1.1 linux24/include/linux/pci.h:1.1.1.1.28.10
--- linux24/include/linux/pci.h:1.1.1.1	Tue Dec 18 15:49:00 2001
+++ linux24/include/linux/pci.h	Mon Mar  4 17:31:15 2002
@@ -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.

Re: [PATCH] PCI device list locking

[Martin Diehl]

On Mon, 4 Mar 2002, Greg KH wrote:

> > +	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


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: [PATCH] PCI device list locking

[Martin Mares]

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.

> 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
-- 
Martin `MJ' Mares   <mj@ucw.cz>   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
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel