PCI core learns 'hotplug'

A while ago, Darrick Wong posted a patch for fakephp that kicked off some controversy:

	http://thread.gmane.org/gmane.linux.kernel/761944

The issue was that I broke the fakephp interface back in the 2.6.27 timeframe. After some discussion on the lists, Trent Piepho sent some patches, and I proposed a solution incorporating those patches.

This is my first cut at making everyone happy. In summary, it:

- introduces /sys/bus/pci/devices/.../remove for function level hot-remove

- introduces /sys/bus/pci/devices/.../rescan to rescan the PCI hierarchy, starting at that device and descending to all children

- introduces /sys/bus/pci/rescan to rescan the entire PCI hierarchy

- restores the pre-2.6.27 fakephp interface for userspace compatability

Some notes:

- I did review Rolf Eike Beer's dummyphp patch at his request. It won't work for the same reasons that we cannot fix the existing fakephp driver -- both use the PCI hotplug API, namely pci_hp_register(), which enforces the semantic of "only one slots/ entry per physical slot", which clearly does not allow multiple entries for multi-function devices. One side note, Trent's new fakephp driver is also much smaller and cleaner, but the real reason is dummyphp uses the PCI hotplug API.

I hope this reasoning is fair, but if folks disagree, I'm happy to discuss

- I've been testing this patchset on my ia64 machines, which Linus has called "an insane mess of PCI bridges"[1], and it seems to work well. I'm just starting to test on some x86 machines, and have been noticing some issues with BAR collisions, so this is definitely a work-in-progress.

- Also, you might be wondering, "how does this patchset interact with existing hotplug drivers, like acpiphp or pciehp?" The answer is, "poorly". :(

If you use the new PCI core removal/rescan and then try to modify the slot using acpiphp, you get an oops. My impression is that this behavior is the same as pre-2.6.27, where you could have loaded fakephp and acpiphp, removed the device with fakephp, and encountered an oops with acpiphp.

2.6.27 enforced the rule of "one hotplug driver per slot", at the cost of the fakephp breakage (which started this whole discussion).

So, I'm not sure what to do about this. The way that we remove devices today, using pci_remove_bus_device() doesn't lend itself to safety very well, since it will just start removing devices from the bus without checking anything.

Maybe we need some other API, or maybe we just live with the limitation of, "if you use PCI core hotplug, don't use the other hotplug drivers and vice versa".

- Finally, a note to Jesse, even if this patchset is rejected, I'd recommend taking the ASPM fix, as that is just a pure bugfix. It could probably go into .29.

Thanks. Comments welcome.

/ac

1: http://lkml.org/lkml/2008/12/13/196
---

Alex Chiang (9):
PCI: more whitespace cleanups
PCI Hotplug: schedule fakephp for feature removal PCI Hotplug: rename legacy_fakephp to fakephp
PCI: Introduce /sys/bus/pci/devices/.../rescan
PCI: Introduce /sys/bus/pci/rescan
PCI: Introduce /sys/bus/pci/devices/.../remove
PCI: properly clean up ASPM link state on device remove
PCI: always scan child buses
PCI: don't scan existing devices

Trent Piepho (1):
PCI Hotplug: restore fakephp interface with complete reimplementation

 Documentation/ABI/testing/sysfs-bus-pci    |   27 ++
 Documentation/feature-removal-schedule.txt |   32 ++
 drivers/pci/hotplug/fakephp.c              |  438 +++++++---------------------
 drivers/pci/pci-sysfs.c                    |   98 ++++++
 drivers/pci/pcie/aspm.c                    |    4 
 drivers/pci/probe.c                        |   68 +++-
 drivers/pci/remove.c                       |    4 
 7 files changed, 303 insertions(+), 368 deletions(-)

pci_scan_slot is supposed to add newly discovered devices to pci_bus->devices, but doesn't check to see if the device has already been added. This can cause problems if we ever want to use this interface to rescan the PCI bus.

---

 drivers/pci/probe.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

index 55ec44a..a9e17e2 100644
@@ -1035,6 +1035,17 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 	for (func = 0; func < 8; func++, devfn++) {
 		struct pci_dev *dev;
 
+		dev = pci_get_slot(bus, devfn);
+		if (dev) {
+			if (dev->multifunction) {
+				pci_dev_put(dev);
+				continue;
+			} else {
+				pci_dev_put(dev);
+				break;
+			}
+		}
+
 		dev = pci_scan_single_device(bus, devfn);
 		if (dev) {
 			nr++;

While scanning bridges, we stop our scan if we encounter a bus that we've seen before, to work around some buggy chipsets. This is a good idea, but prevents us from fully scanning the PCI bus at a future time (to find newly hot-added devices, for example).

Change the logic so that we skip _re-adding_ an existing bus that we've seen before, but also allow the scan to descend to all child buses.

Now that we're potentially scanning our child buses again, we also need to be sure not to attempt re-initializing their BARs so we avoid that.

This patch lays the groundwork to allow the user to issue a rescan of the PCI bus at any time.

---

 drivers/pci/probe.c |   33 +++++++++++++++++++--------------
 1 files changed, 19 insertions(+), 14 deletions(-)

index a9e17e2..9cf810e 100644
@@ -511,21 +511,21 @@ int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 
 		/*
 		 * If we already got to this bus through a different bridge,
-		 * ignore it.  This can happen with the i450NX chipset.
+		 * don't re-add it. This can happen with the i450NX chipset.
+		 *
+		 * However, we continue to descend down the hierarchy and
+		 * scan remaining child buses.
 		 */
-		if (pci_find_bus(pci_domain_nr(bus), busnr)) {
-			dev_info(&dev->dev, "bus %04x:%02x already known\n",
-				 pci_domain_nr(bus), busnr);
-			goto out;
+		child = pci_find_bus(pci_domain_nr(bus), busnr);
+		if (!child) {
+			child = pci_add_new_bus(bus, dev, busnr);
+			if (!child)
+				goto out;
+			child->primary = buses & 0xFF;
+			child->subordinate = (buses >> 16) & 0xFF;
+			child->bridge_ctl = bctl;
 		}
 
-		child = pci_add_new_bus(bus, dev, busnr);
-		if (!child)
-			goto out;
-		child->primary = buses & 0xFF;
-		child->subordinate = (buses >> 16) & 0xFF;
-		child->bridge_ctl = bctl;
-
 		cmax = pci_scan_child_bus(child);
 		if (cmax > max)
 			max = cmax;

@@ -1089,8 +1089,13 @@ unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus)
 	 * After performing arch-dependent fixup of the bus, look behind
 	 * all PCI-to-PCI bridges on this bus.
 	 */
-	pr_debug("PCI: Fixups for bus %04x:%02x\n", pci_domain_nr(bus), bus->number);
-	pcibios_fixup_bus(bus);
+	if (!bus->is_added) {
+		pr_debug("PCI: Fixups for bus %04x:%02x\n", pci_domain_nr(bus), bus->number);
+		pcibios_fixup_bus(bus);
+		if (bus->parent == NULL)
+			bus->is_added = 1;
+	}
+
 	for (pass=0; pass < 2; pass++)
 		list_for_each_entry(dev, &bus->devices, bus_list) {
 			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||

We only want to disable ASPM when the last function is removed from the parent's device list. We determine this by checking to see if the parent's device list is completely empty.

Unfortunately, we never hit that code because the parent is considered an upstream port, and never had an ASPM link_state associated with it.

The early check for !link_state causes us to return early, we never discover that our device list is empty, and thus we never remove the downstream ports' link_state nodes.

Instead of checking to see if the parent's device list is empty, we can check to see if we are the last device on the list, and if so, then we know that we can clean up properly.

---

 drivers/pci/pcie/aspm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 586b6f7..b0367f1 100644
@@ -718,9 +718,9 @@ void pcie_aspm_exit_link_state(struct pci_dev *pdev)
 
 	/*
 	 * All PCIe functions are in one slot, remove one function will remove
-	 * the the whole slot, so just wait
+	 * the whole slot, so just wait until we are the last function left.
 	 */
-	if (!list_empty(&parent->subordinate->devices))
+	if (!list_is_last(&pdev->bus_list, &parent->subordinate->devices))
 		goto out;
 
 	/* All functions are removed, so just disable ASPM for the link */

Applied to my for-linus branch, thanks. Let me know if you have a problem with it (fast) Shaohua! :)

Jesse Barnes, Intel Open Source Technology Center

* Jesse Barnes <[email protected]>:

Thanks Jesse.

I'm cc'ing [email protected] as well (which I should have done in the first place). This fixes a bug that existed since ASPM support was added (7d715a6c1), and git name-rev tells me that we've had it since 2.6.26.

So this patch is a candidate for 2.6.27 and 2.6.28 stable branches.

Thanks.

/ac

Now queued up.

thanks,

greg k-h

This patch adds an attribute named "remove" to a PCI device's sysfs directory. Writing a non-zero value to this attribute will remove the PCI device and any of its children.

Code was originally written by Trent Piepho, with a small cleanup and additional documentation from me.

---

 Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++++
 drivers/pci/pci-sysfs.c                 |   30 ++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 0 deletions(-)

index ceddcff..faa21de 100644
@@ -1,3 +1,11 @@
+What:		/sys/bus/pci/devices/.../remove
+Date:		January 2009
+Contact:	Linux PCI developers <[email protected]>
+Description:
+		Writing a non-zero value to this attribute will
+		hot-remove the PCI device and any of its children.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <[email protected]>

index db7ec14..603a32d 100644
@@ -219,6 +219,33 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+#ifdef CONFIG_HOTPLUG
+static void remove_callback(void *data)
+{
+	pci_remove_bus_device((struct pci_dev *)data);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val)
+		sysfs_schedule_callback(&dev->kobj, remove_callback, pdev,
+		                        THIS_MODULE);
+
+	return count;
+}
+#endif
+
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),

@@ -237,6 +264,9 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR(broken_parity_status,(S_IRUGO|S_IWUSR),
 		broken_parity_status_show,broken_parity_status_store),
 	__ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store),
+#ifdef CONFIG_HOTPLUG
+	__ATTR(remove, S_IWUSR, NULL, remove_store),
+#endif
 	__ATTR_NULL,
 };
 

This interface allows the user to force a rescan of all PCI buses in system, and rediscover devices that have been removed earlier.

---

 Documentation/ABI/testing/sysfs-bus-pci |    9 +++++++++
 drivers/pci/pci-sysfs.c                 |   28 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)

index faa21de..8c0abc7 100644
@@ -1,3 +1,12 @@
+What:		/sys/bus/pci/rescan
+Date:		January 2009
+Contact:	Linux PCI developers <[email protected]>
+Description:
+		Writing a non-zero value to this attribute will
+		force a rescan of all PCI buses in the system, and
+		re-discover previously removed devices.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../remove
 Date:		January 2009
 Contact:	Linux PCI developers <[email protected]>

index 603a32d..c300242 100644
@@ -986,12 +986,40 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
 	}
 }
 
+#ifdef CONFIG_HOTPLUG
+static ssize_t bus_rescan_store(struct bus_type *bus, const char *buf, size_t count)
+{
+	unsigned long val;
+	struct pci_bus *b = NULL;
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val)
+		while ((b = pci_find_next_bus(b)) != NULL) {
+			pci_scan_child_bus(b);
+			pci_bus_add_devices(b);
+		}
+
+	return count;
+}
+static BUS_ATTR(rescan, S_IWUSR, NULL, bus_rescan_store);
+#endif
+
 static int __init pci_sysfs_init(void)
 {
 	struct pci_dev *pdev = NULL;
 	int retval;
 
 	sysfs_initialized = 1;
+#ifdef CONFIG_HOTPLUG
+	retval = bus_create_file(&pci_bus_type, &bus_attr_rescan);
+	if (retval)
+		return retval;
+#endif
 	for_each_pci_dev(pdev) {
 		retval = pci_create_sysfs_dev_files(pdev);
 		if (retval) {

This interface allows the user to force a rescan of the device's parent bus and all subordinate buses, and rediscover devices removed earlier from this part of the device tree.

---

 Documentation/ABI/testing/sysfs-bus-pci |   10 ++++++++++
 drivers/pci/pci-sysfs.c                 |   22 ++++++++++++++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

index 8c0abc7..f59e963 100644
@@ -15,6 +15,16 @@ Description:
 		hot-remove the PCI device and any of its children.
 		Depends on CONFIG_HOTPLUG.
 
+What:		/sys/bus/pci/devices/.../rescan
+Date:		January 2009
+Contact:	Linux PCI developers <[email protected]>
+Description:
+		Writing a non-zero value to this attribute will
+		force a rescan of the device's parent bus and all
+		child buses, and re-discover devices removed earlier
+		from this part of the device tree.
+		Depends on CONFIG_HOTPLUG.
+
 What:		/sys/bus/pci/devices/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <[email protected]>

index c300242..dcbc8bc 100644
@@ -220,6 +220,27 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
 }
 
 #ifdef CONFIG_HOTPLUG
+static ssize_t
+dev_rescan_store(struct device *dev, struct device_attribute *attr,
+	      const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val) {
+		pci_scan_child_bus(pdev->bus);
+		pci_bus_add_devices(pdev->bus);
+	}
+
+	return count;
+}
+
 static void remove_callback(void *data)
 {
 	pci_remove_bus_device((struct pci_dev *)data);

@@ -266,6 +287,7 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store),
 #ifdef CONFIG_HOTPLUG
 	__ATTR(remove, S_IWUSR, NULL, remove_store),
+	__ATTR(rescan, S_IWUSR, NULL, dev_rescan_store),
 #endif
 	__ATTR_NULL,
 };

From: Trent Piepho <[email protected]>

A complete re-implementation of fakephp is necessary if it is to present its former interface (pre-2.6.27, when it broke). The reason is that PCI hotplug drivers call pci_hp_register(), which enforces the rule that only one /sys/bus/pci/slots/ file may be created per physical slot.

The change breaks the old fakephp's assumption that it could create a file per function. So we re-implement fakephp to avoid using the standard PCI hotplug API so that we can restore the old fakephp user interface.

It puts entries in /sys/bus/pci/slots with the names of all PCI devices/functions, exactly symmetrical to what is shown in /sys/bus/pci/devices. Each slots/ entry has a "power" attribute, which works the same way as the fakephp driver's power attribute has worked.

There are a few improvements over old fakephp, which couldn't handle PCI devices being added or removed via a means outside of fakephp's knowledge. If a device was added another way, old fakephp didn't notice and didn't create the fake slot for it. If a device was removed another way, old fakephp didn't delete the fake slot for it (and accessing the stale slot caused an oops).

The new implementation overcomes these limitations. As a consequence, removing a bridge with other devices behind it now works as well, which is something else old fakephp couldn't do previously.

This duplicates a tiny bit of the code in the PCI core that does this same function. Re-using that code ends up being more complex than duplicating it, and it makes code in the PCI core more ugly just to support this legacy fakephp interface compatibility layer.

---

 drivers/pci/hotplug/Makefile         |    2 
 drivers/pci/hotplug/fakephp.c        |  385 ----------------------------------
 drivers/pci/hotplug/legacy_fakephp.c |  163 ++++++++++++++
 3 files changed, 164 insertions(+), 386 deletions(-)
 delete mode 100644 drivers/pci/hotplug/fakephp.c
 create mode 100644 drivers/pci/hotplug/legacy_fakephp.c

index e31fb91..5a86728 100644
@@ -16,7 +16,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
 obj-$(CONFIG_HOTPLUG_PCI_SGI)		+= sgi_hotplug.o
 
 # Link this last so it doesn't claim devices that have a real hotplug driver
-obj-$(CONFIG_HOTPLUG_PCI_FAKE)		+= fakephp.o
+obj-$(CONFIG_HOTPLUG_PCI_FAKE)		+= legacy_fakephp.o
 
 pci_hotplug-objs	:=	pci_hotplug_core.o
 

We wanted to replace fakephp wholesale, so rename legacy_fakephp back to fakephp. Yes, this is a silly commit, but it produces a much easier patch to read and review.

---

 drivers/pci/hotplug/Makefile         |    2 
 drivers/pci/hotplug/fakephp.c        |  163 ++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/legacy_fakephp.c |  163 ----------------------------------
 3 files changed, 164 insertions(+), 164 deletions(-)
 create mode 100644 drivers/pci/hotplug/fakephp.c
 delete mode 100644 drivers/pci/hotplug/legacy_fakephp.c

index 5a86728..e31fb91 100644
@@ -16,7 +16,7 @@ obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
 obj-$(CONFIG_HOTPLUG_PCI_SGI)		+= sgi_hotplug.o
 
 # Link this last so it doesn't claim devices that have a real hotplug driver
-obj-$(CONFIG_HOTPLUG_PCI_FAKE)		+= legacy_fakephp.o
+obj-$(CONFIG_HOTPLUG_PCI_FAKE)		+= fakephp.o
 
 pci_hotplug-objs	:=	pci_hotplug_core.o
 

Now that the PCI core is capable of function-level remove and rescan as well as bus-level rescan, there's no functional need to keep fakephp anymore.

We keep it around for userspace compatibility reasons, schedule removal in three years.

---

 Documentation/feature-removal-schedule.txt |   32 ++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

index 5ddbe35..fc3505b 100644
@@ -335,3 +335,35 @@ Why:	In 2.6.18 the Secmark concept was introduced to replace the "compat_net"
 	Secmark, it is time to deprecate the older mechanism and start the
 	process of removing the old code.
 Who:	Paul Moore <[email protected]>
+
+---------------------------
+
+What:	fakephp and associated sysfs files in /sys/bus/pci/slots/
+When:	2011
+Why:	In 2.6.27, the semantics of /sys/bus/pci/slots was redefined to
+	represent a machine's physical PCI slots. The change in semantics
+	had userspace implications, as the hotplug core no longer allowed
+	drivers to create multiple sysfs files per physical slot (required
+	for multi-function devices, e.g.). fakephp was seen as a developer's
+	tool only, and its interface changed. Too late, we learned that
+	there were some users of the fakephp interface.
+
+	In 2.6.30, the original fakephp interface was restored. At the same
+	time, the PCI core gained the ability that fakephp provided, namely
+	function-level hot-remove and hot-add.
+
+	Since the PCI core now provides the same functionality, exposed in:
+
+		/sys/bus/pci/rescan
+		/sys/bus/pci/devices/.../remove
+		/sys/bus/pci/devices/.../rescan
+
+	there is no functional reason to maintain fakephp as well.
+
+	We will keep the existing module so that 'modprobe fakephp' will
+	present the old /sys/bus/pci/slots/... interface for compatibility,
+	but users are urged to migrate their applications to the API above.
+
+	After a reasonable transition period, we will remove the legacy
+	fakephp interface.
+Who:	Alex Chiang <[email protected]>

Clean up some stray whitespace issues.

---

 drivers/pci/pci-sysfs.c |   18 +++++++++---------
 drivers/pci/probe.c     |   24 +++++++++++++-----------
 drivers/pci/remove.c    |    4 ++--
 3 files changed, 24 insertions(+), 22 deletions(-)

index dcbc8bc..adc6b86 100644
@@ -10,7 +10,7 @@
  *
  * File attributes for PCI devices
  *
- * Modeled after usb's driverfs.c 
+ * Modeled after usb's driverfs.c
  *
  */
 

@@ -70,7 +70,7 @@ static ssize_t broken_parity_status_store(struct device *dev,
 
 static ssize_t local_cpus_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
-{		
+{
 	const struct cpumask *mask;
 	int len;
 

@@ -380,13 +380,13 @@ pci_write_config(struct kobject *kobj, struct bin_attribute *bin_attr,
 		size = dev->cfg_size - off;
 		count = size;
 	}
-	
+
 	if ((off & 1) && size) {
 		pci_user_write_config_byte(dev, off, data[off - init_off]);
 		off++;
 		size--;
 	}
-	
+
 	if ((off & 3) && size > 2) {
 		u16 val = data[off - init_off];
 		val |= (u16) data[off - init_off + 1] << 8;

@@ -404,7 +404,7 @@ pci_write_config(struct kobject *kobj, struct bin_attribute *bin_attr,
 		off += 4;
 		size -= 4;
 	}
-	
+
 	if (size >= 2) {
 		u16 val = data[off - init_off];
 		val |= (u16) data[off - init_off + 1] << 8;

@@ -818,21 +818,21 @@ pci_read_rom(struct kobject *kobj, struct bin_attribute *bin_attr,
 
 	if (!pdev->rom_attr_enabled)
 		return -EINVAL;
-	
+
 	rom = pci_map_rom(pdev, &size);	/* size starts out as PCI window size */
 	if (!rom)
 		return 0;
-		
+
 	if (off >= size)
 		count = 0;
 	else {
 		if (off + count > size)
 			count = size - off;
-		
+
 		memcpy_fromio(buf, rom + off, count);
 	}
 	pci_unmap_rom(pdev, rom);
-		
+
 	return count;
 }
 

index 9cf810e..7399d4e 100644
@@ -494,7 +494,7 @@ int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 	}
 
 	/* Disable MasterAbortMode during probing to avoid reporting
-	   of bus errors (in some architectures) */ 
+	   of bus errors (in some architectures) */
 	pci_read_config_word(dev, PCI_BRIDGE_CONTROL, &bctl);
 	pci_write_config_word(dev, PCI_BRIDGE_CONTROL,
 			      bctl & ~PCI_BRIDGE_CTL_MASTER_ABORT);

@@ -570,7 +570,7 @@ int __devinit pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 			buses &= ~0xff000000;
 			buses |= CARDBUS_LATENCY_TIMER << 24;
 		}
-			
+
 		/*
 		 * We need to blast all three values with a single write.
 		 */

@@ -680,7 +680,7 @@ static void pci_read_irq(struct pci_dev *dev)
  * pci_setup_device - fill in class and map information of a device
  * @dev: the device structure to fill
  *
- * Initialize the device structure with information about the device's 
+ * Initialize the device structure with information about the device's
  * vendor,class,memory and IO-space addresses,IRQ lines etc.
  * Called at initialisation of the PCI subsystem and by CardBus services.
  * Returns 0 on success and -1 if unknown type of device (not normal, bridge

@@ -720,8 +720,8 @@ static int pci_setup_device(struct pci_dev * dev)
 		pci_read_config_word(dev, PCI_SUBSYSTEM_ID, &dev->subsystem_device);
 
 		/*
-		 *	Do the ugly legacy mode stuff here rather than broken chip
-		 *	quirk code. Legacy mode ATA controllers have fixed
+		 *	Do the ugly legacy mode stuff here rather than broken
+		 *	chip quirk code. Legacy mode ATA controllers have fixed
 		 *	addresses. These are not always echoed in BAR0-3, and
 		 *	BAR0-3 in a few cases contain junk!
 		 */

@@ -752,7 +752,7 @@ static int pci_setup_device(struct pci_dev * dev)
 			goto bad;
 		/* The PCI-to-PCI bridge spec requires that subtractive
 		   decoding (i.e. transparent) bridge must have programming
-		   interface code of 0x01. */ 
+		   interface code of 0x01. */
 		pci_read_irq(dev);
 		dev->transparent = ((dev->class & 0xff) == 1);
 		pci_read_bases(dev, 2, PCI_ROM_ADDRESS1);

@@ -1051,14 +1051,14 @@ int pci_scan_slot(struct pci_bus *bus, int devfn)
 			nr++;
 
 			/*
-		 	 * If this is a single function device,
-		 	 * don't scan past the first function.
-		 	 */
+			 * If this is a single function device,
+			 * don't scan past the first function.
+			 */
 			if (!dev->multifunction) {
 				if (func > 0) {
 					dev->multifunction = 1;
 				} else {
- 					break;
+					break;
 				}
 			}
 		} else {

@@ -1140,7 +1140,9 @@ struct pci_bus * pci_create_bus(struct device *parent,
 	b->ops = ops;
 
 	if (pci_find_bus(pci_domain_nr(b), bus)) {
-		/* If we already got to this bus through a different bridge, ignore it */
+		/* If we already got to this bus through a different bridge,
+		 * ignore it
+		 */
 		pr_debug("PCI: Bus %04x:%02x already known\n", pci_domain_nr(b), bus);
 		goto err_out;
 	}

index 042e089..ba58128 100644
@@ -7,7 +7,7 @@ static void pci_free_resources(struct pci_dev *dev)
 {
 	int i;
 
- 	msi_remove_pci_irq_vectors(dev);
+	msi_remove_pci_irq_vectors(dev);
 
 	pci_cleanup_rom(dev);
 	for (i = 0; i < PCI_NUM_RESOURCES; i++) {

@@ -49,7 +49,7 @@ static void pci_destroy_dev(struct pci_dev *dev)
  * pci_remove_device_safe - remove an unused hotplug device
  * @dev: the device to remove
  *
- * Delete the device structure from the device lists and 
+ * Delete the device structure from the device lists and
  * notify userspace (/sbin/hotplug), but only if the device
  * in question is not being used by a driver.
  * Returns 0 on success.

I also continued to work on my patches, but then my reasons for caring about PCI hotplug disappeared due to the current economic climate.

I updated my "remove" patch to include documentation. I created a patch that added "/sys/bus/pci/scan", but not the per-device version. And I updated my new fakephp driver to support rescanning.

Everything worked, but when a bridge was rescanned there would be annoying warning messages. I never got around to figuring about what to do about that. It seems like the code that assigns bridge resources wasn't intended to handle bridges that already had resources assigned to them, though it does work.

Maybe your series can use my latest patches for removal and legacy_fakephp? It sounds like your patches for rescanning do more than mine.

Does it not work, or is it just warnings? I didn't have any problems with resources ending up unassigned, but I did get warnings. I think there was also an issue with removed and rescanned devices' resources' ->parent pointers not being the same as they were before removal. Which doesn't seem to matter any, but made me feel like the code wasn't right yet.

I came to the same conclusion. fakephp or acpiphp will oops if you use the other to remove a pci device. The drivers just aren't designed to handle a pci device being removed out from under them.

My new fakephp driver seems to handle this ok, maybe other php drivers could do the same thing?

This patch adds an attribute named "remove" to a PCI device's sysfs directory. Writing a non-zero value to this attribute will remove the PCI device and any children of it.

---
 Documentation/filesystems/sysfs-pci.txt |    9 +++++++++
 drivers/pci/pci-sysfs.c                 |   26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

index 68ef488..54e014f 100644
@@ -11,6 +11,7 @@ that support it.  For example, a given bus might look like this:
      |   |-- device
      |   |-- irq
      |   |-- local_cpus
+     |   |-- remove
      |   |-- resource
      |   |-- resource0
      |   |-- resource1

@@ -34,6 +35,7 @@ files, each with their own function.
        device		   PCI device (ascii, ro)
        irq		   IRQ number (ascii, ro)
        local_cpus	   nearby CPU mask (cpumask, ro)
+       remove		   remove device from kernel's list (ascii, wo)
        resource		   PCI resource host addresses (ascii, ro)
        resource0..N	   PCI resource N, if present (binary, mmap)
        resource0_wc..N_wc  PCI WC map resource N, if prefetchable (binary, mmap)

@@ -44,6 +46,7 @@ files, each with their own function.
 
   ro - read only file
   rw - file is readable and writable
+  wo - write only file
   mmap - file is mmapable
   ascii - file contains ascii text
   binary - file contains binary data

@@ -62,6 +65,12 @@ ROM file, if available.  It's disabled by default, however, so applications
 should write the string "1" to the file to enable it before attempting a read
 call, and disable it following the access by writing "0" to the file.
 
+The 'remove' file is used to remove the PCI device, by writing a non-zero
+integer to the file.  This does not involve any kind of hot-plug functionality,
+e.g. powering off the device.  The device is removed from the kernel's list of
+PCI devices, the sysfs directory for it removed, and the device will be removed
+from any drivers attached to it.
+
 Accessing legacy resources through sysfs
 ----------------------------------------
 

index 3ddee7f..d422f37 100644
@@ -219,6 +219,31 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,
 	return count;
 }
 
+static void remove_callback(void *data)
+{
+	pci_remove_bus_device((struct pci_dev *)data);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	struct pci_dev *pdev = to_pci_dev(dev);
+	unsigned long val;
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (val)
+		sysfs_schedule_callback(&dev->kobj, remove_callback, pdev,
+		                        THIS_MODULE);
+
+	return count;
+}
+
 struct device_attribute pci_dev_attrs[] = {
 	__ATTR_RO(resource),
 	__ATTR_RO(vendor),

@@ -237,6 +262,7 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR(broken_parity_status,(S_IRUGO|S_IWUSR),
 		broken_parity_status_show,broken_parity_status_store),
 	__ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store),
+	__ATTR(remove, S_IWUSR, NULL, remove_store),
 	__ATTR_NULL,
 };
 

1.5.4.3

* Trent Piepho <[email protected]>:

I've fold this almost verbatim into my patch series, changing the patch title, adding ABI documentation, and protecting the remove calls with #ifdef CONFIG_HOTPLUG:

Otherwise, it's untouched.

Author: Trent Piepho <[email protected]>
Date:   Tue Feb 10 15:02:54 2009 -0700

PCI: Introduce /sys/bus/pci/devices/.../remove

This patch adds an attribute named "remove" to a PCI device's sysfs directory. Writing a non-zero value to this attribute will remove the PCI device and any children of it.

Thanks.

/ac

This adds a bus attribute named "scan" to the PCI bus, which will appear in sysfs at "/sys/bus/pci/scan". Writing to this file will trigger a rescan of all PCI busses.

To do this pci_scan_single_device() is modified to first check if the device to be scanned is already known to Linux. In which case it just returns the existing device. The old behavior was to create a new device anyway that would conflict with the existing one, causing all manner of Bad Things to happen.

pci_scan_slot() has been rewritten to be less complex and will now return the number of *new* devices found. It didn't used to work to call pci_scan_slot() on a previously scanned slot, so returning all devices found was effectivly the same as returning new devices.

---
 drivers/pci/pci-driver.c |    1 +
 drivers/pci/pci-sysfs.c  |   14 +++++++++++
 drivers/pci/pci.h        |    6 +++++
 drivers/pci/probe.c      |   55 +++++++++++++++++++++++++++------------------
 4 files changed, 54 insertions(+), 22 deletions(-)

index 2bec651..fcfd445 100644
@@ -847,6 +847,7 @@ struct bus_type pci_bus_type = {
 	.remove		= pci_device_remove,
 	.shutdown	= pci_device_shutdown,
 	.dev_attrs	= pci_dev_attrs,
+	.bus_attrs	= pci_bus_attrs,
 	.pm		= PCI_PM_OPS_PTR,
 };
 

index d422f37..0781ab7 100644
@@ -266,6 +266,20 @@ struct device_attribute pci_dev_attrs[] = {
 	__ATTR_NULL,
 };
 
+#ifdef CONFIG_HOTPLUG
+static ssize_t __ref scan(struct bus_type *type, const char *buf, size_t count)
+{
+	pci_rescan_busses();
+
+	return count;
+}
+
+struct bus_attribute pci_bus_attrs[] = {
+	__ATTR(scan, S_IWUSR, NULL, scan),
+	__ATTR_NULL
+};
+#endif
+
 static ssize_t
 pci_read_config(struct kobject *kobj, struct bin_attribute *bin_attr,
 		char *buf, loff_t off, size_t count)

index 65deed8..49d7bb4 100644
@@ -87,6 +87,7 @@ static inline int pci_proc_detach_bus(struct pci_bus *bus) { return 0; }
 
 /* Functions for PCI Hotplug drivers to use */
 extern unsigned int pci_do_scan_bus(struct pci_bus *bus);
+extern void pci_rescan_busses(void) __devinit;
 
 #ifdef HAVE_PCI_LEGACY
 extern void pci_create_legacy_files(struct pci_bus *bus);

@@ -128,6 +129,11 @@ extern int pcie_mch_quirk;
 extern struct device_attribute pci_dev_attrs[];
 extern struct device_attribute dev_attr_cpuaffinity;
 extern struct device_attribute dev_attr_cpulistaffinity;
+#ifdef CONFIG_HOTPLUG
+extern struct bus_attribute pci_bus_attrs[];
+#else
+#define pci_bus_attrs NULL
+#endif
 
 /**
  * pci_match_one_device - Tell if a PCI device structure has a matching

1.5.4.3

I will definitely take some ideas from this patch and incorporate them into mine.

* Trent Piepho <[email protected]>:

I prefer the name "rescan".

This module's interface is compatible with what fakephp's interface has been.

It puts entries in /sys/bus/pci/slots with the names of all PCI devices/functions. Each one has a "power" attribute, which works the same way as the fakephp driver's power attribute has worked.

There are a few improvements over fakephp, which couldn't handle PCI devices being added or removed via a means other than fakephp's actions. If a device is added another way, fakephp doesn't notice it and doesn't create the fake slot for it. If a device was removed another way, fakephp doesn't delete the fake slot for it (and accessing the stale slot will cause an oops). These problems are fixed. As a consequence of this, removing a bridge with other devices behind it works as well, which is something else fakephp couldn't do.

This patch duplicates a tiny bit of the code in the PCI core that does this same function. Re-using that code ends up being more complex than duplicating it, and it makes code in the PCI core more ugly just to support this legacy fakephp interface compatibility layer.

---
 drivers/pci/hotplug/Kconfig          |    5 +
 drivers/pci/hotplug/Makefile         |    1 +
 drivers/pci/hotplug/legacy_fakephp.c |  168 ++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pci/hotplug/legacy_fakephp.c

index eacfb13..d26bc68 100644
@@ -39,6 +39,11 @@ config HOTPLUG_PCI_FAKE
 
 	  When in doubt, say N.
 
+config HOTPLUG_PCI_LEGACY_FAKE
+	tristate "Legacy Fake PCI Hotplug driver"
+	help
+	  Provides an interface like the fakephp driver used to.
+
 config HOTPLUG_PCI_COMPAQ
 	tristate "Compaq PCI Hotplug driver"
 	depends on X86 && PCI_BIOS && PCI_LEGACY

index 9bdbe1a..9ad009d 100644
@@ -14,6 +14,7 @@ obj-$(CONFIG_HOTPLUG_PCI_SHPC)		+= shpchp.o
 obj-$(CONFIG_HOTPLUG_PCI_RPA)		+= rpaphp.o
 obj-$(CONFIG_HOTPLUG_PCI_RPA_DLPAR)	+= rpadlpar_io.o
 obj-$(CONFIG_HOTPLUG_PCI_SGI)		+= sgi_hotplug.o
+obj-$(CONFIG_HOTPLUG_PCI_LEGACY_FAKE)	+= legacy_fakephp.o
 
 # Link this last so it doesn't claim devices that have a real hotplug driver
 obj-$(CONFIG_HOTPLUG_PCI_FAKE)		+= fakephp.o

1.5.4.3

Hi Trent,

Sorry for the very long delay. I've been swamped with other things. :-/

* Trent Piepho <[email protected]>:

:(

I will incorporate your patch for removal (and replace mine).

I've already incorporated your legacy_fakephp patch (although I took the liberty of just replacing fakephp wholesale).

When (if? :) I work out the kinks, you'll get authorship credit for both the above.

I'm actually getting errors on my x86 machine:

pci 0000:04:01.0: BAR 8: bogus alignment [0xfa000000-0xfbffffff] flags 0x200
pci 0000:04:01.0: BAR 9: bogus alignment [0xd1100000-0xd11fffff] flags 0x1201
...
pci 0000:07:00.0: BAR 8: bogus alignment [0xfa000000-0xfbffffff] flags 0x200
pci 0000:07:00.0: BAR 9: bogus alignment [0xd1100000-0xd11fffff] flags 0x1201
...
pcieport-driver 0000:04:01.0: irq 57 for MSI/MSI-X
pcieport-driver 0000:04:01.0: device not available because of BAR 8 [0xfa000000-0xfbffffff] collisions
pcieport-driver: probe of 0000:04:01.0 failed with error -22

Obviously, this needs to be figured out before going into mainline, and even then, I think it might need some soak time in -mm...

Yeah, I saw what you did by registering a bus notifier, nice trick.

I don't think every driver wants to do this, it might be better to change the hotplug core to require some sort of callback that gets called when some other driver removes a device.

Thanks.

/ac