PCI core learns hotplug

This API is used by the PCI core to rescan a bus and rediscover newly added devices.

Over time, it is expected that the various PCI hotplug drivers will migrate to this interface and away from the old pci_do_scan_bus() interface.

...

drivers/pci/probe.c +23 -0

@@ -1195,10 +1195,39 @@ struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent,
 EXPORT_SYMBOL(pci_scan_bus_parented);
 
 #ifdef CONFIG_HOTPLUG
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __devinit pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+	struct pci_dev *dev;
+
+	max = pci_scan_child_bus(bus);
+	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 (dev->subordinate)
+				pci_bus_size_bridges(dev->subordinate);
+
+	pci_bus_assign_resources(bus);
+	pci_enable_bridges(bus);

Quoted 42 lines

This API is used by the PCI core to rescan a bus and rediscover newly added devices.

Over time, it is expected that the various PCI hotplug drivers will migrate to this interface and away from the old pci_do_scan_bus() interface.

...

drivers/pci/probe.c +28 -0

@@ -1195,10 +1195,39 @@ struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent,
 EXPORT_SYMBOL(pci_scan_bus_parented);
 
 #ifdef CONFIG_HOTPLUG
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __devinit pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+	struct pci_dev *dev;
+
+	max = pci_scan_child_bus(bus);
+	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 (dev->subordinate)
+				pci_bus_size_bridges(dev->subordinate);
+
+	pci_bus_assign_resources(bus);
+	pci_enable_bridges(bus);
+	pci_bus_add_devices(bus);
+
+	return max;
+}
+

drivers/pci/probe.c struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent, · line 1230

Quoted 5 lines

drivers/pci/probe.c +1 -0

 EXPORT_SYMBOL(pci_add_new_bus);
 EXPORT_SYMBOL(pci_scan_slot);
 EXPORT_SYMBOL(pci_scan_bridge);
 EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+EXPORT_SYMBOL_GPL(pci_rescan_bus);

uhm, is there any rationale for the seemingly-random mixup of export types in this interface?

* Andrew Morton <[email protected]>:

Quoted 43 lines · On Wed, 18 Mar 2009 16:39:56 -0600 Alex Chiang <[email protected]> wrote:

Quoted 42 lines

This API is used by the PCI core to rescan a bus and rediscover newly added devices.

Over time, it is expected that the various PCI hotplug drivers will migrate to this interface and away from the old pci_do_scan_bus() interface.

...

drivers/pci/probe.c +28 -0

@@ -1195,10 +1195,39 @@ struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent,
 EXPORT_SYMBOL(pci_scan_bus_parented);
 
 #ifdef CONFIG_HOTPLUG
+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __devinit pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+	struct pci_dev *dev;
+
+	max = pci_scan_child_bus(bus);
+	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 (dev->subordinate)
+				pci_bus_size_bridges(dev->subordinate);
+
+	pci_bus_assign_resources(bus);
+	pci_enable_bridges(bus);
+	pci_bus_add_devices(bus);
+
+	return max;
+}
+

Quoted 8 lines

drivers/pci/probe.c struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent, · line 1230

Quoted 5 lines

drivers/pci/probe.c +1 -0

 EXPORT_SYMBOL(pci_add_new_bus);
 EXPORT_SYMBOL(pci_scan_slot);
 EXPORT_SYMBOL(pci_scan_bridge);
 EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+EXPORT_SYMBOL_GPL(pci_rescan_bus);

uhm, is there any rationale for the seemingly-random mixup of export types in this interface?

History?

I've no clue why we're mixing EXPORT_SYMBOL and EXPORT_SYMBOL_GPL.

git-blame says that the mishmash existed pre-git.

For my new interface, I thought that EXPORT_SYMBOL_GPL would be most appropriate, but maybe not?

What would you like me to do? Make them all the same?

Quoted 9 lines

* Andrew Morton <[email protected]>:

Quoted 8 lines · On Wed, 18 Mar 2009 16:39:56 -0600 Alex Chiang <[email protected]> wrote:

Quoted 6 lines

EXPORT_SYMBOL(pci_add_new_bus);
EXPORT_SYMBOL(pci_scan_slot);
EXPORT_SYMBOL(pci_scan_bridge);
EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+EXPORT_SYMBOL_GPL(pci_rescan_bus);

Quoted 6 lines

Quoted 2 lines

uhm, is there any rationale for the seemingly-random mixup of export types in this interface?

History?

I've no clue why we're mixing EXPORT_SYMBOL and EXPORT_SYMBOL_GPL.

I do :)

New PCI core exports were added with _GPL, it's the older ones that were left at the "normal" level.

Quoted 4 lines

git-blame says that the mishmash existed pre-git.

For my new interface, I thought that EXPORT_SYMBOL_GPL would be most appropriate, but maybe not?

Yes it is, pci hotplug exports has always been EXPORT_SYMBOL_GPL(), in fact I think it was the first in-tree user of this marking.

Quoted 1 line

What would you like me to do? Make them all the same?

New ones should be _GPL() please.

But please put the export in the proper place, checkpatch.pl will complain if you do not. Which means you didn't run it on your patches :)

I went over and ran it on all my patches, and that was indeed one of the complaints.

But when it comes to file consistency vs "letter of the law" I like to keep things consistent (even if broken) in the file.

I could go back and move all those EXPORT_SYMBOL declarations to the "proper" place. Is that too much noise though?

* Greg KH <[email protected]>:

Quoted 30 lines · Alex Chiang

Quoted 9 lines

* Andrew Morton <[email protected]>:

Quoted 8 lines · On Wed, 18 Mar 2009 16:39:56 -0600 Alex Chiang <[email protected]> wrote:

Quoted 6 lines

EXPORT_SYMBOL(pci_add_new_bus);
EXPORT_SYMBOL(pci_scan_slot);
EXPORT_SYMBOL(pci_scan_bridge);
EXPORT_SYMBOL_GPL(pci_scan_child_bus);
+EXPORT_SYMBOL_GPL(pci_rescan_bus);

Quoted 6 lines

Quoted 2 lines

uhm, is there any rationale for the seemingly-random mixup of export types in this interface?

History?

I've no clue why we're mixing EXPORT_SYMBOL and EXPORT_SYMBOL_GPL.

I do :)

New PCI core exports were added with _GPL, it's the older ones that were left at the "normal" level.

Quoted 4 lines

git-blame says that the mishmash existed pre-git.

For my new interface, I thought that EXPORT_SYMBOL_GPL would be most appropriate, but maybe not?

Yes it is, pci hotplug exports has always been EXPORT_SYMBOL_GPL(), in fact I think it was the first in-tree user of this marking.

Quoted 7 lines

Quoted 1 line

What would you like me to do? Make them all the same?

New ones should be _GPL() please.

But please put the export in the proper place, checkpatch.pl will complain if you do not. Which means you didn't run it on your patches :)

Andrew already yelled at me privately about checkpatch.

I went over and ran it on all my patches, and that was indeed one of the complaints.

But when it comes to file consistency vs "letter of the law" I like to keep things consistent (even if broken) in the file.

I could go back and move all those EXPORT_SYMBOL declarations to the "proper" place. Is that too much noise though?

drivers/pci/probe.c +17 -0

+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __devinit pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+	struct pci_dev *dev;
+
+	max = pci_scan_child_bus(bus);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||

Quoted 18 lines

drivers/pci/probe.c +17 -0

+/**
+ * pci_rescan_bus - scan a PCI bus for devices.
+ * @bus: PCI bus to scan
+ *
+ * Scan a PCI bus and child buses for new devices, adds them,
+ * and enables them.
+ *
+ * Returns the max number of subordinate bus discovered.
+ */
+unsigned int __devinit pci_rescan_bus(struct pci_bus *bus)
+{
+	unsigned int max;
+	struct pci_dev *dev;
+
+	max = pci_scan_child_bus(bus);
+	list_for_each_entry(dev, &bus->devices, bus_list)
+		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||

drivers/pci/probe.c struct pci_bus * __devinit pci_scan_bus_parented(struct device *parent, · line 1224

Quoted 10 lines

drivers/pci/probe.c +10 -0

+		    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
+			if (dev->subordinate)
+				pci_bus_size_bridges(dev->subordinate);
+
+	pci_bus_assign_resources(bus);
+	pci_enable_bridges(bus);
+	pci_bus_add_devices(bus);
+
+	return max;
+}

The conspicuous lack of locking for the list walk requires either

a) documenting or
b) fixing

drivers/pci/pci-sysfs.c +18 -0

+#ifdef CONFIG_HOTPLUG
+static DEFINE_MUTEX(pci_remove_rescan_mutex);
+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) {
+		mutex_lock(&pci_remove_rescan_mutex);
+		while ((b = pci_find_next_bus(b)) != NULL)
+			pci_rescan_bus(b);

Quoted 23 lines

drivers/pci/pci-sysfs.c +22 -0

+#ifdef CONFIG_HOTPLUG
+static DEFINE_MUTEX(pci_remove_rescan_mutex);
+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) {
+		mutex_lock(&pci_remove_rescan_mutex);
+		while ((b = pci_find_next_bus(b)) != NULL)
+			pci_rescan_bus(b);
+		mutex_unlock(&pci_remove_rescan_mutex);
+	}
+	return count;
+}

drivers/pci/pci-sysfs.c msi_bus_store(struct device *dev, struct device_attribute *attr, · line 249

Quoted 6 lines

drivers/pci/pci-sysfs.c +6 -0

+
+struct bus_attribute pci_bus_attrs[] = {
+	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
+	__ATTR_NULL
+};
+#endif

Why CONFIG_HOTPLUG rather than CONFIG_HOTPLUG_PCI (or similar)?

* Andrew Morton <[email protected]>:

Quoted 8 lines

drivers/pci/pci-sysfs.c msi_bus_store(struct device *dev, struct device_attribute *attr, · line 249

Quoted 6 lines

drivers/pci/pci-sysfs.c +6 -0

+
+struct bus_attribute pci_bus_attrs[] = {
+	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
+	__ATTR_NULL
+};
+#endif

Why CONFIG_HOTPLUG rather than CONFIG_HOTPLUG_PCI (or similar)?

I first started out with CONFIG_HOTPLUG_PCI but then all that stuff got compiled out, and I couldn't figure out why at the time.

Quoted 11 lines

* Andrew Morton <[email protected]>:

Quoted 6 lines

Quoted 3 lines

+ +struct bus_attribute pci_bus_attrs[] = { + __ATTR(rescan, S_IWUSR, NULL, bus_rescan_store), + __ATTR_NULL +}; +#endif

Why CONFIG_HOTPLUG rather than CONFIG_HOTPLUG_PCI (or similar)?

I first started out with CONFIG_HOTPLUG_PCI but then all that stuff got compiled out, and I couldn't figure out why at the time.

HOTPLUG_PCI is a subset of HOTPLUG. It gives you support for actual hotplug hardware. The rescan/remove functions don't do anything to hotplug hardware and don't use any code from the pci hotplug core (which is designed to support hotplug hardware). Rescan and remove just need the ability to unbind a driver from device, delete a device and create a new device. Which HOTPLUG gives us.

So, we use HOTPLUG because that's all we need. The idea is to make the sysfs interface for PCI have the most features it can support.

If we wanted to make the interface not have remove/rescan unless needed, then using HOTPLUG_PCI wouldn't make much sense. It would be better to have a new option "PCI remove/rescan sysfs interface" or something that turned it on. But it isn't much code, so I don't think it's necessary to do that.

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

...

drivers/pci/pci-sysfs.c +32 -0

@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+
+	if (pdev->subordinate)
+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);
+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+

Quoted 49 lines · On Wed, 18 Mar 2009 16:40:06 -0600 Alex Chiang <[email protected]> wrote:

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

...

drivers/pci/pci-sysfs.c +32 -0

@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+
+	if (pdev->subordinate)
+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);
+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 290

Quoted 10 lines

drivers/pci/pci-sysfs.c +9 -0

+	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
+		return -EBUSY;
+
+	if (val)
+		ret = device_schedule_callback(dev, remove_callback);
+	if (ret)
+		count = ret;
+	return count;
+}
 #endif

It is very hard for the reader (this one at least) to work out why device_schedule_callback() is used here, instead of simply doing the work directly.

The way to solve that problem is to add a code comment.

Given that we're in a sysfs write() handler where no relevant locks at all are held, it seems rather weird that we cannot perform this operation synchronously, but no doubt the comment will explain all of this.

Do we need the CAP_SYS_ADMIN check if the sysfs file permissions are correct? (I keep on asking this then forgetting the answer).

Quoted 50 lines

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

...

drivers/pci/pci-sysfs.c +32 -0

@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+
+	if (pdev->subordinate)
+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);
+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 290

Quoted 10 lines

drivers/pci/pci-sysfs.c +9 -0

+	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
+		return -EBUSY;
+
+	if (val)
+		ret = device_schedule_callback(dev, remove_callback);
+	if (ret)
+		count = ret;
+	return count;
+}
 #endif

It is very hard for the reader (this one at least) to work out why device_schedule_callback() is used here, instead of simply doing the work directly.

The way to solve that problem is to add a code comment.

Given that we're in a sysfs write() handler where no relevant locks at all are held, it seems rather weird that we cannot perform this operation synchronously, but no doubt the comment will explain all of this.

Do we need the CAP_SYS_ADMIN check if the sysfs file permissions are correct? (I keep on asking this then forgetting the answer).

The device_schedule_callback() thing exposes us to (I assume) a pile of races, the most obvious of which is "what locking or refcounting keeps *dev alive?". It would be nice to see an analysis/description of the lifetime issues here. Perhaps in the changelog, preferably in code comments.

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

Cc Trent Piepho <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

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

Documentation/ABI/testing/sysfs-bus-pci +8 -0

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

Documentation/filesystems/sysfs-pci.txt +10 -0

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

drivers/pci/pci-sysfs.c +4 -0

index 22dbc65..6e2b1fd 100644
@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;

drivers/pci/pci-sysfs.c +30 -1

+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
+		return -EBUSY;
+
+	if (val)
+		ret = device_schedule_callback(dev, remove_callback);
+	if (ret)
+		count = ret;
+	return count;
+}
 #endif
 
 struct device_attribute pci_dev_attrs[] = {

@@ -266,6 +307,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,
 };
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 93 lines · Alex Chiang

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

Cc Trent Piepho <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

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

Documentation/ABI/testing/sysfs-bus-pci +7 -0

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

Documentation/filesystems/sysfs-pci.txt +3 -0

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

Documentation/filesystems/sysfs-pci.txt +6 -0

+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
 ----------------------------------------
 diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c

index 22dbc65..6e2b1fd 100644

drivers/pci/pci-sysfs.c +9 -0

@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+
+	if (pdev->subordinate)

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 262

Quoted 5 lines

drivers/pci/pci-sysfs.c +5 -0

+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);

I cannot understand the above two lines. Could you explain what it intend?

Thanks,
Kenji Kaneshige

Quoted 46 lines

drivers/pci/pci-sysfs.c +30 -1

+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
+		return -EBUSY;
+
+	if (val)
+		ret = device_schedule_callback(dev, remove_callback);
+	if (ret)
+		count = ret;
+	return count;
+}
 #endif
  struct device_attribute pci_dev_attrs[] = {

@@ -266,6 +307,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,
 };
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

* Kenji Kaneshige <[email protected]>:

Quoted 6 lines

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 262

Quoted 3 lines

drivers/pci/pci-sysfs.c +3 -0

+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);

I cannot understand the above two lines. Could you explain what it intend?

If the user says:

	echo 1 > /sys/bus/pci/devices/.../remove

And that device is a bridge, then we need to specifically call pci_remove_bus as well, to actually remove the bus itself. Without it, pci_bus_remove_device() will remove all of its children (and subordinate buses) in a depth-first manner, but we will never actually remove the bus that the user specified.

Quoted 12 lines

* Kenji Kaneshige <[email protected]>:

Quoted 6 lines

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 262

Quoted 3 lines

drivers/pci/pci-sysfs.c +3 -0

+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);

I cannot understand the above two lines. Could you explain what it intend?

If the user says:

	echo 1 > /sys/bus/pci/devices/.../remove

Quoted 5 lines

And that device is a bridge, then we need to specifically call pci_remove_bus as well, to actually remove the bus itself. Without it, pci_bus_remove_device() will remove all of its children (and subordinate buses) in a depth-first manner, but we will never actually remove the bus that the user specified.

Did this end up being the source of the pci resource assignment warnings that were produced when re-adding a bridge that was removed?

* Kenji Kaneshige <[email protected]>:

Quoted 93 lines

Quoted 92 lines · Alex Chiang

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.

Thanks to Vegard Nossum for testing under kmemcheck and finding locking issues with the sysfs interface.

Cc Trent Piepho <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

---

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

Documentation/ABI/testing/sysfs-bus-pci +7 -0

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

Documentation/filesystems/sysfs-pci.txt +3 -0

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

Documentation/filesystems/sysfs-pci.txt +6 -0

+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
 ----------------------------------------
 diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c

index 22dbc65..6e2b1fd 100644

drivers/pci/pci-sysfs.c +8 -0

@@ -246,6 +246,47 @@ struct bus_attribute pci_bus_attrs[] = {
 	__ATTR(rescan, S_IWUSR, NULL, bus_rescan_store),
 	__ATTR_NULL
 };
+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+

Quoted 8 lines

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 262

Quoted 6 lines

drivers/pci/pci-sysfs.c +6 -0

+	if (pdev->subordinate)
+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);

I cannot understand the above two lines. Could you explain what it intend?

If the user says:

	echo 1 > /sys/bus/pci/devices/.../remove

And that device is a bridge, then we need to specifically call pci_remove_bus as well, to actually remove the bus itself. Without it, pci_bus_remove_device() will remove all of its children (and subordinate buses) in a depth-first manner, but we will never actually remove the bus that the user specified.

In other words, without it, we will still see the bus in:

	/sys/class/pci_bus/...

We only want to remove the bus if it has no children left. I think the check for list_empty(&pdev->bus->devices) might be overkill... I can try taking that bit out and testing again.

Thanks for the review.

/ac

Quoted 44 lines

Thanks,
Kenji Kaneshige

Quoted 40 lines

drivers/pci/pci-sysfs.c +30 -0

+
+	mutex_unlock(&pci_remove_rescan_mutex);
+}
+
+static ssize_t
+remove_store(struct device *dev, struct device_attribute *dummy,
+	     const char *buf, size_t count)
+{
+	int ret = 0;
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	if (strict_strtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (pdev->subordinate && pci_is_root_bus(pdev->bus))
+		return -EBUSY;
+
+	if (val)
+		ret = device_schedule_callback(dev, remove_callback);
+	if (ret)
+		count = ret;
+	return count;
+}
 #endif
  struct device_attribute pci_dev_attrs[] = {

@@ -266,6 +307,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,
 };
 

Quoted 7 lines

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 21 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:

Quoted 17 lines

Quoted 4 lines · Alex Chiang

drivers/pci/pci-sysfs.c +4 -0

+
+static void remove_callback(struct device *dev)
+{
+	int bridge = 0;

drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 262

Quoted 10 lines

drivers/pci/pci-sysfs.c +10 -0

+	struct pci_dev *pdev = to_pci_dev(dev);
+
+	mutex_lock(&pci_remove_rescan_mutex);
+
+	if (pdev->subordinate)
+		bridge = 1;
+
+	pci_remove_bus_device(pdev);
+	if (bridge && list_empty(&pdev->bus->devices))
+		pci_remove_bus(pdev->bus);

I cannot understand the above two lines. Could you explain what it intend?

If the user says:

Quoted 8 lines

	echo 1 > /sys/bus/pci/devices/.../remove

And that device is a bridge, then we need to specifically call pci_remove_bus as well, to actually remove the bus itself. Without it, pci_bus_remove_device() will remove all of its children (and subordinate buses) in a depth-first manner, but we will never actually remove the bus that the user specified.

Do you mean user removes bridge device to remove its *primary* bus? It is very strange. I think the bus should be removed when its parent bridge is removed.

Quoted 4 lines

In other words, without it, we will still see the bus in:

	/sys/class/pci_bus/...

What is the problem?

Quoted 4 lines

We only want to remove the bus if it has no children left. I think the check for list_empty(&pdev->bus->devices) might be overkill... I can try taking that bit out and testing again.

I think we don't need the two lines. But if you do that, you need list_empty(&pdev->bus->devices), doesn't it? On the other hand, we must not check 'bridge' in the if statement. Or bus will never be removed when non-bridge device is removed last on the bus.

Again, I think we don't need the two lines. But am I misunderstanding something?

A complete re-implementation of fakephp is necessary if it is to present its former interface (pre-2.6.27, when it broke).

Quoted 3 lines

A complete re-implementation of fakephp is necessary if it is to present its former interface (pre-2.6.27, when it broke).

We broke an existing interface? wtf?

If it's been broken for this long then do we actually need to resurrect it?

If so, do we need to resurrect it in its old form? This would appear to be an opportunity to improve that interface, unless the old one was perfect?

Quoted 2 lines · On Wed, 18 Mar 2009 16:40:16 -0600 Alex Chiang <[email protected]> wrote:

A complete re-implementation of fakephp is necessary if it is to present its former interface (pre-2.6.27, when it broke).

We broke an existing interface? wtf?

If it's been broken for this long then do we actually need to resurrect it?

If so, do we need to resurrect it in its old form? This would appear to be an opportunity to improve that interface, unless the old one was perfect?

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

Note, this test branch does contain the small assorted patches in AER and the PCIe portdriver that I fixed along the way, but does not contain the sysfs callback mutex that I introduced to protect myself from Vegard Nossum. ;) If you want to hammer away at the sysfs interface, please apply this patch on top:

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

What I'm continuing to do:

- investigate converting existing hotplug drivers to use new pci_rescan_bus() interface

Thanks.

/ac

v3 -> v4:
- protect sysfs interfaces with mutex
- undo changes in pci_do_scan_bus
- introduce pci_rescan_bus instead
- do not initialize bridges more than once

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 (8):
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: Introduce pci_rescan_bus()
PCI: do not initialize bridges more than once
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    |   10 +
 drivers/pci/hotplug/fakephp.c              |  443 +++++++---------------------
 drivers/pci/pci-driver.c                   |    1 
 drivers/pci/pci-sysfs.c                    |   95 ++++++
 drivers/pci/pci.h                          |    6 
 drivers/pci/probe.c                        |  103 ++++---
 drivers/pci/setup-bus.c                    |    3 
 include/linux/pci.h                        |   12 +
 10 files changed, 358 insertions(+), 374 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Alex Chiang

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Quoted 6 lines

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

I got the following oops when I did

# echo 1 > remove

on the bridge device.

[ 6639.239164] =============================================
[ 6639.239191] [ INFO: possible recursive locking detected ]
[ 6639.239212] 2.6.29-rc8-kk #1
[ 6639.239227] ---------------------------------------------
[ 6639.239243] events/8/60 is trying to acquire lock:
[ 6639.239252]  (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
[ 6639.239252]
[ 6639.239252] but task is already holding lock:
[ 6639.239252]  (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]
[ 6639.239252] other info that might help us debug this:
[ 6639.239252] 3 locks held by events/8/60:
[ 6639.239252]  #0:  (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]  #1:  (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]  #2:  (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c05b9>] remove_callback+0x29/0x80
[ 6639.239252]
[ 6639.239252] stack backtrace:
[ 6639.239252] Pid: 60, comm: events/8 Not tainted 2.6.29-rc8-kk #1
[ 6639.239252] Call Trace:
[ 6639.239252]  [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260
[ 6639.239252]  [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40
[ 6639.239252]  [<ffffffff8026f148>] lock_acquire+0x58/0x80
[ 6639.239252]  [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 6639.239252]  [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0
[ 6639.239252]  [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 6639.239252]  [<ffffffff80258070>] flush_scheduled_work+0x10/0x20
[ 6639.239252]  [<ffffffffa0158065>] e1000_remove+0x55/0xfe [e1000e]
[ 6639.239252]  [<ffffffff8033ee60>] ? sysfs_schedule_callback_work+0x0/0x50
[ 6639.239252]  [<ffffffff803bf7a2>] pci_device_remove+0x32/0x70
[ 6639.239252]  [<ffffffff80441409>] __device_release_driver+0x59/0x90
[ 6639.241006]  [<ffffffff8044153b>] device_release_driver+0x2b/0x40
[ 6639.241006]  [<ffffffff80441036>] bus_remove_device+0xa6/0x120
[ 6639.241006]  [<ffffffff8043dacb>] device_del+0x12b/0x190
[ 6639.241006]  [<ffffffff8043db56>] device_unregister+0x26/0x70
[ 6639.241006]  [<ffffffff803ba529>] pci_stop_dev+0x49/0x60
[ 6639.241006]  [<ffffffff803ba670>] pci_remove_bus_device+0x40/0xc0
[ 6639.241006]  [<ffffffff803ba71d>] pci_remove_behind_bridge+0x2d/0x50
[ 6639.241006]  [<ffffffff803ba64e>] pci_remove_bus_device+0x1e/0xc0
[ 6639.241006]  [<ffffffff803c05c8>] remove_callback+0x38/0x80
[ 6639.241006]  [<ffffffff8033ee7f>] sysfs_schedule_callback_work+0x1f/0x50
[ 6639.241006]  [<ffffffff8025769a>] run_workqueue+0x15a/0x230
[ 6639.241006]  [<ffffffff80257648>] ? run_workqueue+0x108/0x230
[ 6639.241006]  [<ffffffff8025846f>] worker_thread+0x9f/0x100
[ 6639.241006]  [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40
[ 6639.241006]  [<ffffffff802583d0>] ? worker_thread+0x0/0x100
[ 6639.241006]  [<ffffffff8025b89d>] kthread+0x4d/0x80
[ 6639.241006]  [<ffffffff8020d4ba>] child_rip+0xa/0x20
[ 6639.241006]  [<ffffffff8020cebc>] ? restore_args+0x0/0x30
[ 6639.241006]  [<ffffffff8025b850>] ? kthread+0x0/0x80
[ 6639.241006]  [<ffffffff8020d4b0>] ? child_rip+0x0/0x20
[ 6639.283330] e1000e 0000:40:00.0: PCI INT A disabled
[ 6639.324332] e1000e 0000:40:00.1: PCI INT B disabled
[ 6639.325031] aer 0000:2f:04.0:pcie22: unloading service driver aer

Thanks,
Kenji Kaneshige

Quoted 72 lines

Note, this test branch does contain the small assorted patches in AER and the PCIe portdriver that I fixed along the way, but does not contain the sysfs callback mutex that I introduced to protect myself from Vegard Nossum. ;) If you want to hammer away at the sysfs interface, please apply this patch on top:

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

What I'm continuing to do:

- investigate converting existing hotplug drivers to use new pci_rescan_bus() interface

Thanks.

/ac

v3 -> v4:
- protect sysfs interfaces with mutex
- undo changes in pci_do_scan_bus
- introduce pci_rescan_bus instead
- do not initialize bridges more than once

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 (8):
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: Introduce pci_rescan_bus()
PCI: do not initialize bridges more than once
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    |   10 +
 drivers/pci/hotplug/fakephp.c              |  443 +++++++---------------------
 drivers/pci/pci-driver.c                   |    1  
drivers/pci/pci-sysfs.c                    |   95 ++++++
 drivers/pci/pci.h                          |    6  drivers/pci/probe.c 
                       |  103 ++++---
 drivers/pci/setup-bus.c                    |    3  include/linux/pci.h 
                       |   12 +
 10 files changed, 358 insertions(+), 374 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Alex Chiang

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Quoted 6 lines

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

I got the following oops when I did

# echo 1 > remove

on the bridge device.

[ 6639.239164] =============================================
[ 6639.239191] [ INFO: possible recursive locking detected ]
[ 6639.239212] 2.6.29-rc8-kk #1
[ 6639.239227] ---------------------------------------------
[ 6639.239243] events/8/60 is trying to acquire lock:
[ 6639.239252]  (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
[ 6639.239252]
[ 6639.239252] but task is already holding lock:
[ 6639.239252]  (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]
[ 6639.239252] other info that might help us debug this:
[ 6639.239252] 3 locks held by events/8/60:
[ 6639.239252]  #0:  (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]  #1:  (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 6639.239252]  #2:  (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c05b9>] remove_callback+0x29/0x80
[ 6639.239252]
[ 6639.239252] stack backtrace:
[ 6639.239252] Pid: 60, comm: events/8 Not tainted 2.6.29-rc8-kk #1
[ 6639.239252] Call Trace:
[ 6639.239252]  [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260
[ 6639.239252]  [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40
[ 6639.239252]  [<ffffffff8026f148>] lock_acquire+0x58/0x80
[ 6639.239252]  [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 6639.239252]  [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0
[ 6639.239252]  [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 6639.239252]  [<ffffffff80258070>] flush_scheduled_work+0x10/0x20
[ 6639.239252]  [<ffffffffa0158065>] e1000_remove+0x55/0xfe [e1000e]
[ 6639.239252]  [<ffffffff8033ee60>] ? sysfs_schedule_callback_work+0x0/0x50
[ 6639.239252]  [<ffffffff803bf7a2>] pci_device_remove+0x32/0x70
[ 6639.239252]  [<ffffffff80441409>] __device_release_driver+0x59/0x90
[ 6639.241006]  [<ffffffff8044153b>] device_release_driver+0x2b/0x40
[ 6639.241006]  [<ffffffff80441036>] bus_remove_device+0xa6/0x120
[ 6639.241006]  [<ffffffff8043dacb>] device_del+0x12b/0x190
[ 6639.241006]  [<ffffffff8043db56>] device_unregister+0x26/0x70
[ 6639.241006]  [<ffffffff803ba529>] pci_stop_dev+0x49/0x60
[ 6639.241006]  [<ffffffff803ba670>] pci_remove_bus_device+0x40/0xc0
[ 6639.241006]  [<ffffffff803ba71d>] pci_remove_behind_bridge+0x2d/0x50
[ 6639.241006]  [<ffffffff803ba64e>] pci_remove_bus_device+0x1e/0xc0
[ 6639.241006]  [<ffffffff803c05c8>] remove_callback+0x38/0x80
[ 6639.241006]  [<ffffffff8033ee7f>] sysfs_schedule_callback_work+0x1f/0x50
[ 6639.241006]  [<ffffffff8025769a>] run_workqueue+0x15a/0x230
[ 6639.241006]  [<ffffffff80257648>] ? run_workqueue+0x108/0x230
[ 6639.241006]  [<ffffffff8025846f>] worker_thread+0x9f/0x100
[ 6639.241006]  [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40
[ 6639.241006]  [<ffffffff802583d0>] ? worker_thread+0x0/0x100

[ 6639.241006]  [<ffffffff8025b89d>] kthread+0x4d/0x80
[ 6639.241006]  [<ffffffff8020d4ba>] child_rip+0xa/0x20
[ 6639.241006]  [<ffffffff8020cebc>] ? restore_args+0x0/0x30
[ 6639.241006]  [<ffffffff8025b850>] ? kthread+0x0/0x80
[ 6639.241006]  [<ffffffff8020d4b0>] ? child_rip+0x0/0x20
[ 6639.283330] e1000e 0000:40:00.0: PCI INT A disabled
[ 6639.324332] e1000e 0000:40:00.1: PCI INT B disabled
[ 6639.325031] aer 0000:2f:04.0:pcie22: unloading service driver aer

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

Quoted 10 lines · Alex Chiang

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Quoted 6 lines

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

When I rescan the bus even without remove anything, I got the following messages. And enable count of the bridge is incremented every time.

[17240.309096] pci 0000:2d:00.0: BAR 6: bogus alignment [0x0-0x0] flags 0x2
[17240.309135] pcieport-driver 0000:00:01.0: setting latency timer to 64
[17240.309143] pcieport-driver 0000:00:02.0: setting latency timer to 64
[17240.309150] pcieport-driver 0000:14:00.0: setting latency timer to 64
[17240.309159] pcieport-driver 0000:15:00.0: setting latency timer to 64
[17240.309166] pcieport-driver 0000:15:01.0: setting latency timer to 64
[17240.309174] pcieport-driver 0000:15:02.0: setting latency timer to 64
[17240.309182] pci 0000:14:00.3: setting latency timer to 64
[17240.309189] pcieport-driver 0000:00:03.0: setting latency timer to 64
[17240.309198] pcieport-driver 0000:2a:00.0: setting latency timer to 64
[17240.309207] pcieport-driver 0000:2b:02.0: setting latency timer to 64
[17240.309215] pcieport-driver 0000:2b:04.0: setting latency timer to 64
[17240.309223] pcieport-driver 0000:00:04.0: setting latency timer to 64
[17240.309231] pcieport-driver 0000:2e:00.0: setting latency timer to 64
[17240.309240] pcieport-driver 0000:2f:02.0: setting latency timer to 64

[17240.309248] pcieport-driver 0000:2f:04.0: setting latency timer to 64
[17240.309256] pcieport-driver 0000:00:06.0: setting latency timer to 64
[17240.309264] pcieport-driver 0000:51:00.0: setting latency timer to 64
[17240.309273] pcieport-driver 0000:52:02.0: setting latency timer to 64
[17240.309281] pcieport-driver 0000:52:04.0: setting latency timer to 64
[17240.309291] pci 0000:64:00.0: setting latency timer to 64
[17240.309300] pcieport-driver 0000:00:1c.0: setting latency timer to 64
[17240.309308] pci 0000:00:1e.0: setting latency timer to 64

[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 6
[root@localhost pci]# echo 1 > rescan

[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 7
[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 8
[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 9

Quoted 10 lines · Alex Chiang

We're getting close to the new merge window, and I _think_ this patch series is ready for consideration. Notably:

- multiple rescans/removes of devices with and without bridges
- verified that resource allocation after multiple remove/rescan cycles is the same as what we had during initial boot
- fixes the complete suckage of fakephp (that I created)
- doesn't affect existing hotplug drivers
- tested on x86 and ia64 platforms

Quoted 6 lines

Please review, and consider testing. For testing ease, you can pull from my git branch:

	git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git
	branch 'test-20090318' is what you want

When I rescan the bus even without remove anything, I got the following messages. And enable count of the bridge is incremented every time.

[17240.309096] pci 0000:2d:00.0: BAR 6: bogus alignment [0x0-0x0] flags 0x2

[17240.309135] pcieport-driver 0000:00:01.0: setting latency timer to 64
[17240.309143] pcieport-driver 0000:00:02.0: setting latency timer to 64
[17240.309150] pcieport-driver 0000:14:00.0: setting latency timer to 64
[17240.309159] pcieport-driver 0000:15:00.0: setting latency timer to 64
[17240.309166] pcieport-driver 0000:15:01.0: setting latency timer to 64
[17240.309174] pcieport-driver 0000:15:02.0: setting latency timer to 64
[17240.309182] pci 0000:14:00.3: setting latency timer to 64
[17240.309189] pcieport-driver 0000:00:03.0: setting latency timer to 64
[17240.309198] pcieport-driver 0000:2a:00.0: setting latency timer to 64
[17240.309207] pcieport-driver 0000:2b:02.0: setting latency timer to 64
[17240.309215] pcieport-driver 0000:2b:04.0: setting latency timer to 64
[17240.309223] pcieport-driver 0000:00:04.0: setting latency timer to 64
[17240.309231] pcieport-driver 0000:2e:00.0: setting latency timer to 64
[17240.309240] pcieport-driver 0000:2f:02.0: setting latency timer to 64
[17240.309248] pcieport-driver 0000:2f:04.0: setting latency timer to 64
[17240.309256] pcieport-driver 0000:00:06.0: setting latency timer to 64
[17240.309264] pcieport-driver 0000:51:00.0: setting latency timer to 64
[17240.309273] pcieport-driver 0000:52:02.0: setting latency timer to 64
[17240.309281] pcieport-driver 0000:52:04.0: setting latency timer to 64
[17240.309291] pci 0000:64:00.0: setting latency timer to 64
[17240.309300] pcieport-driver 0000:00:1c.0: setting latency timer to 64
[17240.309308] pci 0000:00:1e.0: setting latency timer to 64

[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 6
[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 7
[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 8
[root@localhost pci]# echo 1 > rescan
[root@localhost pci]# cat /sys/bus/pci/devices/0000\:2f\:04.0/enable 9