PCI core learns hotplug

This is v3 of the patch series that introduces function-level hotplug to the PCI core. It is against Jesse's linux-next branch.

I've been doing lots of testing, and found and fixed several small buglets, both in my code and elsewhere in PCI. In order to test this patchset, you will also need these patches:

	http://thread.gmane.org/gmane.linux.kernel.pci/3437
	http://lkml.org/lkml/2009/3/7/173

There is still one major bug somewhere that shows up only when using the PCIe portdriver (that is, any time PCIe support is built into the kernel). You get an oops during multiple remove/rescan cycles, especially on devices with an internal bridge.

This symptom does not appear on the exact same hardware topology, removing and rescanning the exact same device, when not using the PCIe port driver.

I'm still not sure what's going on, so if anyone wants to play with this series, I'd truly appreciate any help.

Thanks.

/ac

v2 -> v3:
- properly remove device with internal bridge
- added Kenji Kaneshige's pci_is_root_bus() interface
- dropped whitespace cleanups for another time

v1 -> v2:
- incorporated lots of Trent Piepho's work
- beefed up pci_do_scan_bus as heavy lifter for rescanning
- small bugfixes folded into earlier patches to get everything working
---

Alex Chiang (7):
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/devices/.../remove
PCI: Introduce /sys/bus/pci/rescan
PCI: beef up pci_do_scan_bus()
PCI: always scan child buses

Kenji Kaneshige (1):
PCI: pci_is_root_bus helper

Trent Piepho (3):
PCI Hotplug: restore fakephp interface with complete reimplementation
PCI: pci_scan_slot() returns newly found devices
PCI: don't scan existing devices

 Documentation/ABI/testing/sysfs-bus-pci    |   27 ++
 Documentation/feature-removal-schedule.txt |   32 ++
 Documentation/filesystems/sysfs-pci.txt    |    9 +
 drivers/pci/hotplug-pci.c                  |   16 +
 drivers/pci/hotplug/fakephp.c              |  443 +++++++---------------------
 drivers/pci/pci-sysfs.c                    |   89 ++++++
 drivers/pci/probe.c                        |   74 ++---
 include/linux/pci.h                        |    9 +
 8 files changed, 321 insertions(+), 378 deletions(-)

From: Kenji Kaneshige <[email protected]>

Introduce pci_is_root_bus helper function. This will help make code more consistent, as well as prevent incorrect assumptions (such as pci_bus->self == NULL on a root bus, which is _not_ always true).

---

 include/linux/pci.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

index 7baf2a5..a31f935 100644
@@ -341,6 +341,15 @@ struct pci_bus {
 #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)
 #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
 
+/*
+ * Returns true if the pci bus is root (behind host-pci bridge),
+ * false otherwise
+ */
+static inline bool pci_is_root_bus(struct pci_bus *pbus)
+{
+	return !(pbus->parent);
+}
+
 #ifdef CONFIG_PCI_MSI
 static inline bool pci_dev_msi_enabled(struct pci_dev *pci_dev)
 {

From: Trent Piepho <[email protected]>

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

If the device is already added, just return it.

---

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

index 55ec44a..66b5d1c 100644
@@ -1006,6 +1006,12 @@ struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
 {
 	struct pci_dev *dev;
 
+	dev = pci_get_slot(bus, devfn);
+	if (dev) {
+		pci_dev_put(dev);
+		return dev;
+	}
+
 	dev = pci_scan_device(bus, devfn);
 	if (!dev)
 		return NULL;

From: Trent Piepho <[email protected]>

pci_scan_slot() has been rewritten to be less complex and will now return the number of *new* devices found.

Existing callers need not worry because they already assume that they can't call pci_scan_slot() on an already-scanned slot.

Thus, there is no semantic change for existing callers: returning newly found devices (this patch) is exactly equal to returning all found devices (before this patch).

This patch adds some more groundwork to allow us to rescan the PCI bus during runtime to discover newly added devices.

---

 drivers/pci/probe.c |   35 +++++++++++++----------------------
 1 files changed, 13 insertions(+), 22 deletions(-)

index 66b5d1c..d392813 100644
@@ -1030,35 +1030,26 @@ EXPORT_SYMBOL(pci_scan_single_device);
  * Scan a PCI slot on the specified PCI bus for devices, adding
  * discovered devices to the @bus->devices list.  New devices
  * will not have is_added set.
+ *
+ * Returns the number of new devices found.
  */
 int pci_scan_slot(struct pci_bus *bus, int devfn)
 {
-	int func, nr = 0;
-	int scan_all_fns;
-
-	scan_all_fns = pcibios_scan_all_fns(bus, devfn);
-
-	for (func = 0; func < 8; func++, devfn++) {
-		struct pci_dev *dev;
+	int fn, nr = 0;
+	struct pci_dev *dev;
 
-		dev = pci_scan_single_device(bus, devfn);
-		if (dev) {
+	if ((dev = pci_scan_single_device(bus, devfn)))
+		if (!dev->is_added)	/* new device? */
 			nr++;
 
-			/*
-		 	 * 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;
-				}
+	if ((dev && dev->multifunction) ||
+	    (!dev && pcibios_scan_all_fns(bus, devfn))) {
+		for (fn = 1; fn < 8; fn++) {
+			if ((dev = pci_scan_single_device(bus, devfn + fn))) {
+				if (!dev->is_added)
+					nr++;
+				dev->multifunction = 1;
 			}
-		} else {
-			if (func == 0 && !scan_all_fns)
-				break;
 		}
 	}
 

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 d392813..a3959ce 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;

@@ -1075,8 +1075,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 (pci_is_root_bus(bus))
+			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 have a nice interface for re-scanning a PCI bus which will discover newly added devices, add them to the device tree, and enable them properly.

Ensure that the bridge resources are properly sized and assigned during the rescan.

---

 drivers/pci/hotplug-pci.c |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

index 4d4a644..33ab2d2 100644
@@ -6,13 +6,21 @@
 
 unsigned int __devinit pci_do_scan_bus(struct pci_bus *bus)
 {
-	unsigned int max;
+	unsigned int max, pass;
+	struct pci_dev *dev;
 
 	max = pci_scan_child_bus(bus);
 
-	/*
-	 * Make the discovered devices available.
-	 */
+	for (pass=0; pass < 2; pass++)
+		list_for_each_entry(dev, &bus->devices, bus_list) {
+			if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
+			    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
+				if (pass && dev->subordinate)
+					pci_bus_size_bridges(dev->subordinate);
+		}
+
+	pci_bus_assign_resources(bus);
+	pci_enable_bridges(bus);
 	pci_bus_add_devices(bus);
 
 	return max;

Hello Kenji-san,

* Kenji Kaneshige <[email protected]>:

Yes, I don't know why I had it in there originally. I can remove that.

I hadn't gotten around to verifying/fixing existing callers of pci_do_scan_bus yet. I was focusing on the core first.

There aren't too many callers, but unfortunately, I don't have any hardware that actually uses the existing drivers.

I seem to recall that your machines support shpchp. Would you mind testing this patch and telling me if your machine still behaves properly?

I looked at shpchp_configure_device and I think that simply scanning the device's parent bus should work.

Thanks.

/ac

index aa315e5..7e8457b 100644
@@ -110,11 +110,7 @@ int __ref shpchp_configure_device(struct slot *p_slot)
 		return -EINVAL;
 	}
 
-	num = pci_scan_slot(parent, PCI_DEVFN(p_slot->device, 0));
-	if (num == 0) {
-		ctrl_err(ctrl, "No new device found\n");
-		return -ENODEV;
-	}
+	pci_do_scan_bus(parent);
 
 	for (fn = 0; fn < 8; fn++) {
 		dev = pci_get_slot(parent, PCI_DEVFN(p_slot->device, fn));

@@ -126,40 +122,10 @@ int __ref shpchp_configure_device(struct slot *p_slot)
 			pci_dev_put(dev);
 			continue;
 		}
-		if ((dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) ||
-				(dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)) {
-			/* Find an unused bus number for the new bridge */
-			struct pci_bus *child;
-			unsigned char busnr, start = parent->secondary;
-			unsigned char end = parent->subordinate;
-			for (busnr = start; busnr <= end; busnr++) {
-				if (!pci_find_bus(pci_domain_nr(parent),
-							busnr))
-					break;
-			}
-			if (busnr > end) {
-				ctrl_err(ctrl,
-					 "No free bus for hot-added bridge\n");
-				pci_dev_put(dev);
-				continue;
-			}
-			child = pci_add_new_bus(parent, dev, busnr);
-			if (!child) {
-				ctrl_err(ctrl, "Cannot add new bus for %s\n",
-					 pci_name(dev));
-				pci_dev_put(dev);
-				continue;
-			}
-			child->subordinate = pci_do_scan_bus(child);
-			pci_bus_size_bridges(child);
-		}
 		program_fw_provided_values(dev);
 		pci_dev_put(dev);
 	}
 
-	pci_bus_assign_resources(parent);
-	pci_bus_add_devices(parent);
-	pci_enable_bridges(parent);
 	return 0;
 }
 

I have machines that support shpchp. But unfortunately I don't have any adapter card that contains bridge, which is needed to test your change.

Ok, I'll try it.

Thanks,
Kenji Kaneshige

Hello Kenji-san,

* Kenji Kaneshige <[email protected]>:

You're right.

The more I think about it though, the more I think that even without the below patch to clean up the callers of pci_do_scan_bus, we should be ok, because:

- all the old code (which I removed below) existed because the old PCI core would refuse to scan PCI buses that had already been discovered

- that meant that it would never descend past a known bridge to try and find new child bridges

- that meant that hotplug drivers had to manually discover new bridges and add them, essentially duplicating functionality in pci_scan_bridge

This patch series allows the PCI core to scan existing bridges and descend down into the children every time, looking for new bridges and devices, so all the code in shpchp, cpcihp, and other callers of pci_do_scan_bus shouldn't be necessary anymore.

Also, if we do add new bridges once manually in shpchp, and then call the new pci_do_scan_bus again, we will _not_ add devices twice because the core should check each bridge and device for struct pci_dev.is_added.

So anyway, I think that cleaning up the callers of pci_do_scan_bus is a good idea, but multiple calls to the interface definitely should not result in problems. If they do, then that's a bug in my patch series.

I set up a git tree to make it easier to test:

git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git

The 'test-20090313' branch contains all the latest fixes to enable this patch series. It does not contain the patch below, so you will have to apply it by hand.

Thanks for testing!

/ac

I'm sorry, but I didn't have enough time to try your patch on my environment. So I'm still just looking at the code.

I looked at shpchp_configure_device() from the view point of bridge hot-add. I think it is broken regardless of your change because it calls pci_bus_add_devices() (through pci_do_scan_bus) before assigning resources. So I think it must be changed regardless of your change. But it's a little difficult for me because I don't have any test environment as I mentioned before.

But I'm still worrying about your change against pci_do_scan_bus(). Without your change, pci_do_scan_bus() scans child buses and add devices without assigning resources. I guess that it means existing callers of pci_do_scan_bus() have some mechanism to assign resource by theirselves and they don't expect pci_do_scan_bus() assigns resources.

By the way, I have one question about rescan. Please suppose that we enable the bridge(B) and its children using rescan interface in the picture below.

                  |
-------------------------------------- parent bus
        |                  |
    bridge(A)          bridge(B)
    (working)        (Not working)
        |                  |
  -------------      -------------
   |         |        |         |
  dev       dev      dev       dev
(working) (working)   (Not working)

In this case, your rescan mechanism calls pci_do_scan_bus() for
parent bus, and pci_do_scan_bus() calls pci_bus_assign_resources()
for parent bus. My question is, does pci_bus_assign_resources() do
nothing against bridge(A) that is currently working? I guess pci_bus_assign_resources() would update some registers of bridge(A) and it would breaks currently working devices.

Thanks,
Kenji Kaneshige

* Kenji Kaneshige <[email protected]>:

Ok.

Hm, what you say makes sense.

I managed to find a very old machine supported by cpqphp, and also found a card with a bridge.

cpqhp_configure_device() follows a similar algorithm to shpchp_configure_device(). I'm just starting my testing now, and there is good news and bad news.

The bad news is that although cpqphp loads successfully, and we can successfully offline a card, we cannot online it again afterwards due to BAR collisions. This failure occurs even without my changes (2.6.27 kernel), and I haven't had time to track the regression down yet.

We do discover the bridge on the device correctly and it is added back into the device tree correctly, but we can't use it because it's not programmed correctly.

The good news is, after rewriting cpqphp_configure_device() to resemble the shpchp patch I gave you, we still discover the bridge correctly and add it back into the device tree in the proper place. We no longer get BAR collisions, but we fail in a slightly different way.

At least I'm not introducing a new regression in cpqphp, and I suspect shpchp will be similar.

I looked through shpchp and couldn't find this assumption. Is it stored in the struct controller, under mmio_base and mmio_size?

I am motivated to get this patch series into 2.6.30 for several reasons, so I think for now, I will not change pci_do_scan_bus(). Instead, I'll create a new interface that only the PCI core will use, and leave the drivers alone.

Over time, we can migrate the drivers to the PCI core interface.

This is a very good catch, thank you.

I added another patch to prevent this situation. We now check to see if the bridge is already added inside of pci_setup_bridge().

Thanks.

/ac

Yes, shpchp doesn't have this assumption. As I mentioned in the previous e-mail, I think shpchp's bridge hot-add code is broken even without your change. I'm worrying about the other hotplug drivers such as cpqphp, cpcihp, rpaphp and ibmphp, though I don't have any knowledge about those hotplug drivers.

I think it's much safer way.

Sounds good to me.

Thanks,
Kenji Kaneshige

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                 |   26 ++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)

index e638e15..ea4aee2 100644
@@ -41,6 +41,15 @@ Description:
 		for the device and attempt to bind to it.  For example:
 		# echo "8086 10f5" > /sys/bus/pci/drivers/foo/new_id
 
+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/.../vpd
 Date:		February 2008
 Contact:	Ben Hutchings <[email protected]>

index 1c89298..a29fc3b 100644
@@ -971,12 +971,38 @@ 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_do_scan_bus(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 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.

Trent Piepho wrote the original implementation and documentation.

---

 Documentation/ABI/testing/sysfs-bus-pci |    8 ++++++
 Documentation/filesystems/sysfs-pci.txt |    9 ++++++
 drivers/pci/pci-sysfs.c                 |   43 +++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 0 deletions(-)

index ea4aee2..5b1ddde 100644
@@ -50,6 +50,14 @@ Description:
 		re-discover previously removed devices.
 		Depends on CONFIG_HOTPLUG.
 
+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 9f8740c..89c15dc 100644
@@ -12,6 +12,7 @@ that support it.  For example, a given bus might look like this:
      |   |-- enable
      |   |-- irq
      |   |-- local_cpus
+     |   |-- remove
      |   |-- resource
      |   |-- resource0
      |   |-- resource1

@@ -36,6 +37,7 @@ files, each with their own function.
        enable	           Whether the device is enabled (ascii, rw)
        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)

@@ -46,6 +48,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

@@ -73,6 +76,12 @@ that the device must be enabled for a rom read to return data succesfully.
 In the event a driver is not bound to the device, it can be enabled using the
 'enable' file, documented above.
 
+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 is removed, and the device will be
+removed from any drivers attached to it.
+
 Accessing legacy resources through sysfs
 ----------------------------------------
 

This is an updated version of this patch. It fixes a bug where we would remove a bridge even if it still had children.

Now we check for children before attempting to remove the bridge.

/ac

commit 3e9eed2b7f4dd6f8f0df98d6bfae71c7461dbdf2

Author: Alex Chiang <[email protected]>
Date:   Mon Mar 9 12:43:10 2009 -0600

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.

Trent Piepho wrote the original implementation and documentation.

index ea4aee2..5b1ddde 100644
@@ -50,6 +50,14 @@ Description:
 		re-discover previously removed devices.
 		Depends on CONFIG_HOTPLUG.
 
+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 9f8740c..89c15dc 100644
@@ -12,6 +12,7 @@ that support it.  For example, a given bus might look like this:
      |   |-- enable
      |   |-- irq
      |   |-- local_cpus
+     |   |-- remove
      |   |-- resource
      |   |-- resource0
      |   |-- resource1

@@ -36,6 +37,7 @@ files, each with their own function.
        enable	           Whether the device is enabled (ascii, rw)
        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)

@@ -46,6 +48,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

@@ -73,6 +76,12 @@ that the device must be enabled for a rom read to return data succesfully.
 In the event a driver is not bound to the device, it can be enabled using the
 'enable' file, documented above.
 
+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 is removed, and the device will be
+removed from any drivers attached to it.
+
 Accessing legacy resources through sysfs
 ----------------------------------------
 

* Alex Chiang <[email protected]>:

This is another updated version of the patch.

It adds a mutex for the sysfs callback function.

This mutex, along with another sysfs patch that I plan on posting soon, should help prevent an oops that a crazy user might induce with the following loop:

	# while true ; do echo 1 > /sys/bus/pci/devices/.../remove ; done

I'm posting this updated version so that Vegard can drop it into his test setup that he had.

When I post v4 of this entire patch series, I'll have a full changelog, etc.

Thanks.

/ac

commit 53cb428d7a26be7b3ea38f6d7f30cd33cc1e6b4a

Author: Alex Chiang <[email protected]>
Date:   Tue Mar 10 16:32:45 2009 -0600

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.

Trent Piepho wrote the original implementation and documentation.

index ea4aee2..5b1ddde 100644
@@ -50,6 +50,14 @@ Description:
 		re-discover previously removed devices.
 		Depends on CONFIG_HOTPLUG.
 
+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 9f8740c..26e4b8b 100644
@@ -12,6 +12,7 @@ that support it.  For example, a given bus might look like this:
      |   |-- enable
      |   |-- irq
      |   |-- local_cpus
+     |   |-- remove
      |   |-- resource
      |   |-- resource0
      |   |-- resource1

@@ -36,6 +37,7 @@ files, each with their own function.
        enable	           Whether the device is enabled (ascii, rw)
        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)

@@ -46,6 +48,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

@@ -73,6 +76,13 @@ that the device must be enabled for a rom read to return data succesfully.
 In the event a driver is not bound to the device, it can be enabled using the
 'enable' file, documented above.
 
+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 is removed, and the device will be
+removed from any drivers attached to it. Removal of PCI root buses is
+disallowed.
+
 Accessing legacy resources through sysfs
 ----------------------------------------
 

* Alex Chiang <[email protected]>:

Yet another updated version. Removes an extra pci_dev_put that was causing us to reference the pci_dev after it was freed (which showed up when slab poisoning debugging was turned on).

This iteration plus the sysfs rfc patch I posted earlier today survives Vegard's test cases.

	http://lkml.org/lkml/2009/3/10/476

Need to try and test the entire series under under kmemcheck once I figure out how to do that.

Thanks.

/ac

commit 22222eefb1535eca972e60f60e6a44bbada6bd35

Author: Alex Chiang <[email protected]>
Date:   Tue Mar 10 22:01:32 2009 -0600

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.

Trent Piepho wrote the original implementation and documentation.

index ea4aee2..5b1ddde 100644
@@ -50,6 +50,14 @@ Description:
 		re-discover previously removed devices.
 		Depends on CONFIG_HOTPLUG.
 
+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 9f8740c..26e4b8b 100644
@@ -12,6 +12,7 @@ that support it.  For example, a given bus might look like this:
      |   |-- enable
      |   |-- irq
      |   |-- local_cpus
+     |   |-- remove
      |   |-- resource
      |   |-- resource0
      |   |-- resource1

@@ -36,6 +37,7 @@ files, each with their own function.
        enable	           Whether the device is enabled (ascii, rw)
        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)

@@ -46,6 +48,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

@@ -73,6 +76,13 @@ that the device must be enabled for a rom read to return data succesfully.
 In the event a driver is not bound to the device, it can be enabled using the
 'enable' file, documented above.
 
+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 is removed, and the device will be
+removed from any drivers attached to it. Removal of PCI root buses is
+disallowed.
+
 Accessing legacy resources through sysfs
 ----------------------------------------
 

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                 |   20 ++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

index 5b1ddde..d33840e 100644
@@ -58,6 +58,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 2f85383..be66dbd 100644
@@ -220,6 +220,25 @@ 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_do_scan_bus(pdev->bus);
+
+	return count;
+}
+
 static void remove_callback(void *data)
 {
 	int bridge = 0;

@@ -279,6 +298,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        |  395 ----------------------------------
 drivers/pci/hotplug/legacy_fakephp.c |  162 ++++++++++++++
 3 files changed, 163 insertions(+), 396 deletions(-)
 delete mode 100644 drivers/pci/hotplug/fakephp.c
 create mode 100644 drivers/pci/hotplug/legacy_fakephp.c

index 2aa117c..3314fe8 100644
@@ -20,7 +20,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        |  162 ++++++++++++++++++++++++++++++++++
 drivers/pci/hotplug/legacy_fakephp.c |  162 ----------------------------------
 3 files changed, 163 insertions(+), 163 deletions(-)
 create mode 100644 drivers/pci/hotplug/fakephp.c
 delete mode 100644 drivers/pci/hotplug/legacy_fakephp.c

index 3314fe8..2aa117c 100644
@@ -20,7 +20,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]>

* Alex Chiang <[email protected]>:

Got it, we had a double-free in the PCIe port driver which was causing all sorts of problems.

I fixed that and now this patch series is stable enough for others to actually apply and test. As of now, there are no known bugs.

Of course, I'm going to keep testing and try to find some more bugs. :)

As a reminder, if you want to play with this series, you'll also need these two patches:

And now this third patch:

	http://thread.gmane.org/gmane.linux.kernel.pci/3524

Finally, patch 07/11 needs to be updated. I'll post a reply to that mail with the updated patch.

Thanks.

/ac

2009/3/9 Alex Chiang <[email protected]>:

Hi,

I got this crash:

[  279.029673] BUG: unable to handle kernel NULL pointer dereference
at 0000000000000008
[  279.030011] IP: [<ffffffff811fce96>] pci_remove_bus_device+0x56/0xe0
[  279.030011] PGD 3e47e067 PUD 3e4d1067 PMD 0
[  279.030011] Oops: 0002 [#1] SMP
[  279.030011] last sysfs file: /sys/devices/pci0000:00/0000:00:00.0/remove
[  279.030011] CPU 0
[  279.030011] Pid: 6, comm: events/0 Not tainted 2.6.29-rc6 #361 945P-A
[  279.030011] RIP: 0010:[<ffffffff811fce96>]  [<ffffffff811fce96>]
pci_remove_bus_device+0x56/0xe0
[  279.030011] RSP: 0018:ffff88003f8bde30  EFLAGS: 00010286
[  279.030011] RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffffff817ab9b8
[  279.030011] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff817ab9b0
[  279.030011] RBP: ffff88003f8bde50 R08: 00000000002ec000 R09: 0000000000000000
[  279.030011] R10: ffff88003d9fd7c0 R11: 0000000000000040 R12: ffff88003d929800
[  279.030011] R13: ffff88003d929800 R14: ffff88003f80a908 R15: ffff88003f8adf00
[  279.030011] FS:  0000000000000000(0000) GS:ffff8800019f1000(0000)
knlGS:0000000000000000
[  279.030011] CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
[  279.030011] CR2: ffff88003e4d1000 CR3: 000000003e452000 CR4: 00000000000006a0
[  279.030011] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  279.030011] DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
[  279.030011] Process events/0 (pid: 6, threadinfo ffff88003f8bc000,
task ffff88003f8a2350)
[  279.030011] Stack:
[  279.030011]  ffffffffffffffff ffff88003d929800 ffff88003d9de800
ffff88003f80a908
[  279.030011]  ffff88003f8bde70 ffffffff81202f7d 0000000000000010
ffff88003d9de820
[  279.030011]  ffff88003f8bde90 ffffffff8112503f ffff88003f80a900
ffffffff81125020
[  279.030011] Call Trace:
[  279.030011]  [<ffffffff81202f7d>] remove_callback+0x3d/0x60
[  279.030011]  [<ffffffff8112503f>] sysfs_schedule_callback_work+0x1f/0x40
[  279.030011]  [<ffffffff81125020>] ? sysfs_schedule_callback_work+0x0/0x40
[  279.030011]  [<ffffffff81055510>] run_workqueue+0x70/0x130
[  279.030011]  [<ffffffff81055677>] worker_thread+0xa7/0x120
[  279.030011]  [<ffffffff810597f0>] ? autoremove_wake_function+0x0/0x40
[  279.030011]  [<ffffffff810555d0>] ? worker_thread+0x0/0x120
[  279.030011]  [<ffffffff810593d9>] kthread+0x49/0x90
[  279.030011]  [<ffffffff8100d45a>] child_rip+0xa/0x20
[  279.030011]  [<ffffffff81059390>] ? kthread+0x0/0x90
[  279.030011]  [<ffffffff8100d450>] ? child_rip+0x0/0x20
[  279.030011] Code: 00 00 00 4c 89 ef 4d 89 ec 31 db e8 75 fe ff ff
48 c7 c7 b0 b9 7a 81 e8 f9 f8 3a 00 49 8b 55 00 49 8b
 45 08 48 c7 c7 b0 b9 7a 81 <48> 89 42 08 48 89 10 49 c7 45 08 00 00
00 00 49 c7 45 00 00 00
[  279.030011] RIP  [<ffffffff811fce96>] pci_remove_bus_device+0x56/0xe0
[  279.030011]  RSP <ffff88003f8bde30>
[  279.030011] CR2: 0000000000000008
[  279.291933] ---[ end trace 4ba18f2857f89768 ]---

It was with this patch queue on top of pci/linux-next (487e348b0ff23e061f60010477a664ea378c1b30):

PCIe: portdrv: call pci_disable_device during remove
PCIe: AER: during disable, check subordinate before walking PCIe portdrv: eliminate double kfree in remove path
PCI Hotplug: schedule fakephp for feature removal
PCI Hotplug: rename legacy_fakephp to fakephp
PCI Hotplug: restore fakephp interface with complete reimplementation
PCI: Introduce /sys/bus/pci/devices/.../rescan
PCI: Introduce /sys/bus/pci/devices/.../remove (new version)
PCI: Introduce /sys/bus/pci/rescan
PCI: beef up pci_do_scan_bus()
PCI: always scan child buses
PCI: pci_scan_slot() returns newly found devices
PCI: don't scan existing devices
PCI: pci_is_root_bus helper

It reproduces reliably if I do this:

$ while true; do echo 1 > /sys/bus/pci/devices/0000\:00\:00.0/remove; done

Line numbers:

$ addr2line -e vmlinux -i ffffffff811fce96
include/linux/list.h:92
include/linux/list.h:105
drivers/pci/remove.c:40
drivers/pci/remove.c:106

And this is my drivers/pci/remove.c:

33 static void pci_destroy_dev(struct pci_dev *dev) 34 {
35 pci_stop_dev(dev);
36
37 /* Remove the device from the device lists, and prevent any further
38 * list accesses from this device */

 39         down_write(&pci_bus_sem);
 40         list_del(&dev->bus_list);
 41         dev->bus_list.next = dev->bus_list.prev = NULL;
 42         up_write(&pci_bus_sem);

43
44 pci_free_resources(dev);
45 pci_dev_put(dev);
46 }

Vegard

"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Hi Vegard,

First, thanks for testing!

* Vegard Nossum <[email protected]>:

[...]

I was going to ask for lspci -v output so that I could see what device 0000:00:00.0 might be, but I was able to reproduce something similar on my machine.

Can I ask why you're doing the above with a while loop? Just to torture the code? Or something else?

I haven't started investigating too closely yet, but it looks like I need some locking in there.

Thanks.

/ac

[root@tahitifp1 pci]# while true ; do echo 1 > devices/0000\:04\:00.0/remove ; done
kobject: '0000:06:00.0' (e000000181703120): kobject_uevent_env

------------[ cut here ]------------
WARNING: at fs/sysfs/group.c:138 sysfs_remove_group+0x80/0x180()
Hardware name: server BL860c
sysfs group a000000101432580 not found for kobject '0000:06:00.0'
Modules linked in: binfmt_misc dm_multipath pci_slot sg shpchp pci_hotplug dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod uhci_hcd ohci_hcd ehci_hcd usbcore

Call Trace:
 [<a0000001000146d0>] show_stack+0x50/0xa0
                                sp=e00000018131fae0 bsp=e000000181311228
 [<a000000100014750>] dump_stack+0x30/0x60
                                sp=e00000018131fcb0 bsp=e000000181311210
 [<a00000010009a0f0>] warn_slowpath+0x130/0x180
                                sp=e00000018131fcb0 bsp=e0000001813111a8
 [<a0000001002336a0>] sysfs_remove_group+0x80/0x180
                                sp=e00000018131fdd0 bsp=e000000181311178
 [<a000000100513410>] dpm_sysfs_remove+0x30/0x60
                                sp=e00000018131fdd0 bsp=e000000181311158
 [<a0000001005041d0>] device_del+0x70/0x3a0
                                sp=e00000018131fdd0 bsp=e000000181311120
 [<a0000001005045d0>] device_unregister+0xd0/0x100
                                sp=e00000018131fdd0 bsp=e000000181311100
 [<a000000100409cd0>] pci_stop_dev+0x70/0x100
                                sp=e00000018131fdd0 bsp=e0000001813110d8
 [<a000000100409f40>] pci_remove_bus_device+0x80/0x180
                                sp=e00000018131fdd0 bsp=e0000001813110a8
 [<a00000010040a0a0>] pci_remove_behind_bridge+0x60/0xc0
                                sp=e00000018131fdd0 bsp=e000000181311080
 [<a000000100409f00>] pci_remove_bus_device+0x40/0x180
                                sp=e00000018131fdd0 bsp=e000000181311050
 [<a00000010040a0a0>] pci_remove_behind_bridge+0x60/0xc0
                                sp=e00000018131fdd0 bsp=e000000181311028
 [<a000000100409f00>] pci_remove_bus_device+0x40/0x180
                                sp=e00000018131fdd0 bsp=e000000181310ff0
 [<a000000100415bc0>] remove_callback+0x40/0xc0
                                sp=e00000018131fdd0 bsp=e000000181310fc8
 [<a00000010022edb0>] sysfs_schedule_callback_work+0x50/0xc0
                                sp=e00000018131fdd0 bsp=e000000181310fa0
 [<a0000001000c1150>] run_workqueue+0x1f0/0x340
                                sp=e00000018131fdd0 bsp=e000000181310f60
 [<a0000001000c13e0>] worker_thread+0x140/0x180
                                sp=e00000018131fdd0 bsp=e000000181310f38
 [<a0000001000c9ba0>] kthread+0xa0/0x120
                                sp=e00000018131fe30 bsp=e000000181310f08
 [<a000000100016690>] kernel_thread_helper+0xd0/0x100
                                sp=e00000018131fe30 bsp=e000000181310ee0
 [<a00000010000a4c0>] start_kernel_thread+0x20/0x40
                                sp=e00000018131fe30 bsp=e000000181310ee0
---[ end trace 9397c0de832fd5ba ]---
Unable to handle kernel NULL pointer dereference (address 0000000000000020)
events/4[32]: Oops 8813272891392 [1]
Modules linked in: binfmt_misc dm_multipath pci_slot sg shpchp pci_hotplug dm_snapshot dm_zero dm_mirror dm_region_hash dm_log dm_mod uhci_hcd ohci_hcd ehci_hcd usbcore

Pid: 32, CPU 4, comm: events/4
psr : 00001010085a6010 ifs : 800000000000038a ip : [<a0000001009059a0>] Tainted: G W (2.6.29-rc4)
ip is at klist_put+0x40/0x160
unat: 0000000000000000 pfs : 000000000000038a rsc : 0000000000000003
rnat: e00000018131fcb0 bsps: 9397c0de832fd5ba pr : 0000000000005a41
ldrs: 0000000000000000 ccv : 0000000000000000 fpsr: 0009804c8a70433f csd : 0000000000000000 ssd : 0000000000000000
b0 : a000000100905990 b6 : a000000100044250 b7 : a00000010000cd10 f6 : 000000000000000000000 f7 : 1003e9e3779b97f4a7c16 f8 : 1003e0a00000010000a4c f9 : 1003e0000000000000056 f10 : 1003e000000000000025a f11 : 1003e6db6db6db6db6db7 r1 : a0000001016d1360 r2 : a0000001014f5f50 r3 : a0000001014f5188 r8 : 0000000000000000 r9 : 0000000000000000 r10 : 0000000000000200 r11 : 0000000000000000 r12 : e00000018131fdd0 r13 : e000000181310000 r14 : 0000000000000020 r15 : 0000000000000009 r16 : 0000000000000000 r17 : a0000001014f5f50 r18 : 0000000000000200 r19 : 000000000007aca1 r20 : fffffffffff7aca1 r21 : a0000001014f5f90 r22 : a0000001014f5f94 r23 : 000000000000000a r24 : 000000000000000a r25 : a000000102b76a98 r26 : 000000000007aca0 r27 : 00000010085a2010 r28 : 00000000000fffff r29 : a0000001014f4e58 r30 : 0000000000000000 r31 : a000000101409818

Call Trace:
 [<a0000001000146d0>] show_stack+0x50/0xa0
                                sp=e00000018131f9a0 bsp=e0000001813112c0
 [<a000000100014fb0>] show_regs+0x830/0x860
                                sp=e00000018131fb70 bsp=e000000181311278
 [<a000000100039bc0>] die+0x1c0/0x2c0
                                sp=e00000018131fb70 bsp=e000000181311230
 [<a000000100063e50>] ia64_do_page_fault+0x830/0x960
                                sp=e00000018131fb70 bsp=e0000001813111c8
 [<a00000010000c700>] ia64_native_leave_kernel+0x0/0x270
                                sp=e00000018131fc00 bsp=e0000001813111c8
 [<a0000001009059a0>] klist_put+0x40/0x160
                                sp=e00000018131fdd0 bsp=e000000181311178
 [<a000000100905af0>] klist_del+0x30/0x60
                                sp=e00000018131fdd0 bsp=e000000181311158
 [<a0000001005041f0>] device_del+0x90/0x3a0
                                sp=e00000018131fdd0 bsp=e000000181311120
 [<a0000001005045d0>] device_unregister+0xd0/0x100
                                sp=e00000018131fdd0 bsp=e000000181311100
 [<a000000100409cd0>] pci_stop_dev+0x70/0x100
                                sp=e00000018131fdd0 bsp=e0000001813110d8
 [<a000000100409f40>] pci_remove_bus_device+0x80/0x180
                                sp=e00000018131fdd0 bsp=e0000001813110a8
 [<a00000010040a0a0>] pci_remove_behind_bridge+0x60/0xc0
                                sp=e00000018131fdd0 bsp=e000000181311080
 [<a000000100409f00>] pci_remove_bus_device+0x40/0x180
                                sp=e00000018131fdd0 bsp=e000000181311050
 [<a00000010040a0a0>] pci_remove_behind_bridge+0x60/0xc0
                                sp=e00000018131fdd0 bsp=e000000181311028
 [<a000000100409f00>] pci_remove_bus_device+0x40/0x180
                                sp=e00000018131fdd0 bsp=e000000181310ff0
 [<a000000100415bc0>] remove_callback+0x40/0xc0
                                sp=e00000018131fdd0 bsp=e000000181310fc8
 [<a00000010022edb0>] sysfs_schedule_callback_work+0x50/0xc0
                                sp=e00000018131fdd0 bsp=e000000181310fa0
 [<a0000001000c1150>] run_workqueue+0x1f0/0x340
                                sp=e00000018131fdd0 bsp=e000000181310f60
 [<a0000001000c13e0>] worker_thread+0x140/0x180
                                sp=e00000018131fdd0 bsp=e000000181310f38
 [<a0000001000c9ba0>] kthread+0xa0/0x120
                                sp=e00000018131fe30 bsp=e000000181310f08
 [<a000000100016690>] kernel_thread_helper+0xd0/0x100
                                sp=e00000018131fe30 bsp=e000000181310ee0
 [<a00000010000a4c0>] start_kernel_thread+0x20/0x40
                                sp=e00000018131fe30 bsp=e000000181310ee0

kobject: '0000:06:00.0' (e000000181703120): fill_kobj_path: path = '/devices/pci0000:03/0000:03:00.0/0000:04:00.0/0000:05:02.0/0000:06:00.0'

2009/3/9 Alex Chiang <[email protected]>:

Yes, purely for the purpose of torture ;-)

I also found one more use-after-free error using kmemcheck:

[  263.258025] WARNING: kmemcheck: Caught 8-bit read from freed memory
(ffff88003d8f315c)
[  263.266131] 80e1803f0088ffff20d67b81ffffffff0000000000000000000000000d000000
[  263.275104]  f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f f
[  263.284053]                                                          ^
[  263.290696]
[  263.292303] Pid: 881, comm: udevd Not tainted 2.6.29-rc6 #361 945P-A
[  263.298770] RIP: 0010:[<ffffffff811eb501>]  [<ffffffff811eb501>]
kobject_put+0x11/0x60
[  263.306938] RSP: 0018:ffff88003f8bde60  EFLAGS: 00010282
[  263.312367] RAX: 0000000000000001 RBX: ffff88003d8f3120 RCX: 0000000000000000
[  263.319616] RDX: 0000000000000001 RSI: 0000000000000003 RDI: ffff88003d8f3120
[  263.326865] RBP: ffff88003f8bde70 R08: 00000000002ec000 R09: 0000000000000000
[  263.334114] R10: ffff88003d95fdc0 R11: 0000000000000010 R12: ffff88003d946ac0
[  263.341362] R13: ffff88003f80a908 R14: ffff88003f80a908 R15: ffff88003f8adf00
[  263.348613] FS:  0000000000000000(0000) GS:ffff8800019f1000(0063)
knlGS:00000000f7d6c700
[  263.356884] CS:  0010 DS: 002b ES: 002b CR0: 000000008005003b
[  263.362747] CR2: ffff88003f806ea0 CR3: 000000003e44a000 CR4: 00000000000006a0
[  263.369995] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  263.377246] DR3: 0000000000000000 DR6: 00000000ffff4ff0 DR7: 0000000000000400
[  263.384494]  [<ffffffff81125048>] sysfs_schedule_callback_work+0x28/0x40
[  263.391378]  [<ffffffff81055510>] run_workqueue+0x70/0x130
[  263.397049]  [<ffffffff81055677>] worker_thread+0xa7/0x120
[  263.402720]  [<ffffffff810593d9>] kthread+0x49/0x90
[  263.407784]  [<ffffffff8100d45a>] child_rip+0xa/0x20
[  263.412935]  [<ffffffffffffffff>] 0xffffffffffffffff
[  281.464381] NOHZ: local_softirq_pending 01

This is how I triggered it:

# echo 1 > /sys/bus/pci/devices/0000\:00\:00.0/remove
# echo 1 > /sys/bus/pci/rescan
# echo 1 > /sys/bus/pci/rescan
# echo 1 > /sys/bus/pci/devices/0000\:00\:00.0/remove

The line numbers:

$ addr2line -e vmlinux -i ffffffff811eb501 ffffffff81125048
lib/kobject.c:589
fs/sysfs/file.c:677

586 void kobject_put(struct kobject *kobj)
587 {
588 if (kobj) {
589 if (!kobj->state_initialized)
590 WARN(1, KERN_WARNING "kobject: '%s' (%p): is not "
591 "initialized, yet kobject_put() is being "
592 "called.\n", kobject_name(kobj), kobj);
593 kref_put(&kobj->kref, kobject_release);
594 }
595 }

669 static void sysfs_schedule_callback_work(struct work_struct *work) 670 {
671 struct sysfs_schedule_callback_struct *ss = container_of(work,
672 struct sysfs_schedule_callback_struct, work);
673

674         (ss->func)(ss->data);
675         kobject_put(ss->kobj);
676         module_put(ss->owner);
677         kfree(ss);

678 }

(The short story: the ss->kobj was already freed when this function was called.)

Hope this helps :-)

Vegard

"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

* Vegard Nossum <[email protected]>:

Ok, just checking. :)

Yes, thanks a lot!

I'll work on these. Thanks for the excellent bug reports.

/ac