PCI core learns hotplug
Subsystems (3)
v5 cover PCI core learns hotplug
Here is v5 of this series that addresses the review comments from v4.
I've pushed out a new test tree that contains the updated series for testing. The tree consists of:
Linus's v2.6.28
+ Jesse's linux-next queue (as of 19 March 2009)
+ the 13 patches in this series
It does not contain the sysfs callback patch that is required for interface torture tests. That can be had here:
http://thread.gmane.org/gmane.linux.kernel/806648
You can pull my tree here:
git://git.kernel.org/pub/scm/linux/kernel/git/achiang/pci-hotplug.git test-20090319
This is also a nice chance to test the shiny new patchwork instance that we have running on kernel.org that we hope to use for linux-pci in the future:
http://patchwork.kernel.org/project/linux-pci/list/
I still haven't figured out the locking issue that Kenji-san pointed out from last time, because I haven't been able to reproduce it.
Any further testing/review is welcomed and appreciated.
Thanks!
/ac
v4 -> v5:
- acquire pci_bus_sem when walking device list in pci_rescan_bus
- keep CONFIG_HOTPLUG (instead of CONFIG_HOTPLUG_PCI)
- comment why we need device_schedule_callback()
- remove CAP_SYS_ADMIN check
- do not remove primary bus in remove_callback()
- do not enable bridges multiple times
- checkpatch cleanups
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 (9):
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 enable bridges more than once
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 | 33 ++ Documentation/filesystems/sysfs-pci.txt | 10 + drivers/pci/bus.c | 6 drivers/pci/hotplug/fakephp.c | 443 +++++++--------------------- drivers/pci/pci-driver.c | 1 drivers/pci/pci-sysfs.c | 81 +++++ drivers/pci/pci.h | 6 drivers/pci/probe.c | 112 +++++-- drivers/pci/setup-bus.c | 3 include/linux/pci.h | 12 + 11 files changed, 356 insertions(+), 378 deletions(-)
PATCH v5 1/13 PCI: pci_is_root_bus helper
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(-)
include/linux/pci.h +9 -0
@@ -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) {
Jesse Barnes
AppliedRe: [PATCH v5 01/13] PCI: pci_is_root_bus helper
Quoted 9 lines · On Fri, 20 Mar 2009 14:55:55 -0600 Alex Chiang <[email protected]> wrote:
From: Kenji Kaneshige <[email protected]>
Introduce
pci_is_root_bushelper function. This will help make code more consistent, as well as prevent incorrect assumptions (such aspci_bus->self== NULL on a root bus, which is not always true).Signed-off-by Kenji Kaneshige <[email protected]>Signed-off-by Alex Chiang <[email protected]>---
Applied this series, thanks everyone!
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH v5 2/13 PCI: don't scan existing devices
In reply to Alex ChiangFrom: 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(-)
drivers/pci/probe.c +6 -0
@@ -1013,6 +1013,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 ;
PATCH v5 3/13 PCI: pci_scan_slot() returns newly found devices
In reply to Alex ChiangFrom: 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 | 40 ++++++++++++++++------------------------ 1 files changed, 16 insertions(+), 24 deletions(-)
drivers/pci/probe.c +16 -24
@@ -1037,35 +1037,27 @@ 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; - - dev =pci_scan_single_device (bus, devfn); -if (dev) { - nr++; +int fn, nr =0 ; +struct pci_dev *dev; -/* -* 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 ; - } + dev =pci_scan_single_device (bus, devfn); +if (dev && !dev->is_added)/* new device? */ + nr++; + +if ((dev && dev->multifunction) || + (!dev &&pcibios_scan_all_fns (bus, devfn))) { +for (fn =1 ; fn <8 ; fn++) { + dev =pci_scan_single_device (bus, devfn + fn); +if (dev) { +if (!dev->is_added) + nr++; + dev->multifunction =1 ; } - }else { -if (func ==0 && !scan_all_fns) -break ; } }
PATCH v5 4/13 PCI: always scan child buses
In reply to Alex ChiangWhile 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 | 34 ++++++++++++++++++++-------------- 1 files changed, 20 insertions(+), 14 deletions(-)
drivers/pci/probe.c +20 -14
@@ -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;
@@ -1083,8 +1083,14 @@ 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 ||
PATCH v5 5/13 PCI: do not initialize bridges more than once
In reply to Alex ChiangIn preparation for PCI core hotplug, we need to ensure that we do not attempt to re-initialize bridges that have already been initialized.
We only need to worry about non-root buses, since we will not allow root bus removal.
---
drivers/pci/setup-bus.c | 3 +++ 1 files changed, 3 insertions(+), 0 deletions(-)
drivers/pci/setup-bus.c +3 -0
@@ -144,6 +144,9 @@ static void pci_setup_bridge(struct pci_bus *bus)struct pci_bus_region region;u32 l, bu, lu, io_upper16; +if (!pci_is_root_bus (bus) && bus->is_added) +return ; +dev_info (&bridge->dev,"PCI bridge, secondary bus %04x:%02x\n" ,pci_domain_nr (bus), bus->number);
PATCH v5 6/13 PCI: do not enable bridges more than once
In reply to Alex ChiangIn preparation for PCI core hotplug, we need to ensure that we do not attempt to re-enable bridges that have already been enabled.
---
drivers/pci/bus.c | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-)
drivers/pci/bus.c +4 -2
@@ -184,8 +184,10 @@ void pci_enable_bridges(struct pci_bus *bus)list_for_each_entry (dev, &bus->devices, bus_list) {if (dev->subordinate) { - retval =pci_enable_device (dev); -pci_set_master (dev); +if (atomic_read (&dev->enable_cnt) ==0 ) { + retval =pci_enable_device (dev); +pci_set_master (dev); + }pci_enable_bridges (dev->subordinate); } }
PATCH v5 7/13 PCI: Introduce pci_rescan_bus()
In reply to Alex ChiangThis 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/hotplug/fakephp.c | 6 +++--- drivers/pci/probe.c | 32 ++++++++++++++++++++++++++++++++ include/linux/pci.h | 3 +++ 3 files changed, 38 insertions(+), 3 deletions(-)
drivers/pci/hotplug/fakephp.c +3 -3
@@ -245,12 +245,12 @@ static int pci_rescan_slot(struct pci_dev *temp)/** -* pci_rescan_bus - Rescan PCI bus +* pci_rescan_bus_local - fakephp version of rescan PCI bus * @bus: the PCI bus to rescan * * Call pci_rescan_slot for each possible function of the bus. */ -static void pci_rescan_bus (const struct pci_bus *bus) +static void pci_rescan_bus_local (const struct pci_bus *bus) {unsigned int devfn;struct pci_dev *dev;
@@ -291,7 +291,7 @@ static void pci_rescan_buses(const struct list_head *list)const struct list_head *l;list_for_each (l,list) {const struct pci_bus *b =pci_bus_b (l); -pci_rescan_bus (b); +pci_rescan_bus_local (b);pci_rescan_buses (&b->children); } }
drivers/pci/probe.c +32 -0
@@ -1203,6 +1203,38 @@ 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); + +up_read (&pci_bus_sem); +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); +down_read (&pci_bus_sem); + +pci_bus_assign_resources (bus); +pci_enable_bridges (bus); +pci_bus_add_devices (bus); + +return max; +} +EXPORT_SYMBOL_GPL (pci_rescan_bus); +EXPORT_SYMBOL (pci_add_new_bus);EXPORT_SYMBOL (pci_scan_slot);EXPORT_SYMBOL (pci_scan_bridge);
include/linux/pci.h +3 -0
@@ -710,6 +710,9 @@ int pci_back_from_sleep(struct pci_dev *dev);/* Functions for PCI Hotplug drivers to use */ int pci_bus_find_capability (struct pci_bus *bus,unsigned int devfn,int cap); +#ifdef CONFIG_HOTPLUG +unsigned int pci_rescan_bus (struct pci_bus *bus); +#endif /* Vital product data routines */ ssize_t pci_read_vpd (struct pci_dev *dev,loff_t pos,size_t count,void *buf);
PATCH v5 8/13 PCI: Introduce /sys/bus/pci/rescan
In reply to Alex ChiangThis interface allows the user to force a rescan of all PCI buses in system, and rediscover devices that have been removed earlier.
pci_bus_attrs implementation from Trent Piepho.
Thanks to Vegard Nossum for discovering locking issues with the sysfs interface.
---
Documentation/ABI/testing/sysfs-bus-pci | 9 +++++++++ drivers/pci/pci-driver.c | 1 + drivers/pci/pci-sysfs.c | 26 ++++++++++++++++++++++++++ drivers/pci/pci.h | 6 ++++++ drivers/pci/probe.c | 4 ++-- 5 files changed, 44 insertions(+), 2 deletions(-)
Documentation/ABI/testing/sysfs-bus-pci +9 -0
@@ -57,6 +57,15 @@ Description: match the driver to the device. For example: # echo "8086 10f5" > /sys/bus/pci/drivers/foo/remove_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]>
drivers/pci/pci-driver.c +1 -0
@@ -1049,6 +1049,7 @@ struct bus_type pci_bus_type = { .remove = pci_device_remove, .shutdown = pci_device_shutdown, .dev_attrs = pci_dev_attrs, + .bus_attrs = pci_bus_attrs, .pm = PCI_PM_OPS_PTR, };
drivers/pci/pci-sysfs.c +26 -0
@@ -219,6 +219,32 @@ msi_bus_store(struct device *dev, struct device_attribute *attr,return count; } +#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 (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; +} + +struct bus_attribute pci_bus_attrs[] = { +__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), + __ATTR_NULL +}; +#endif +struct device_attribute pci_dev_attrs[] = {__ATTR_RO (resource),__ATTR_RO (vendor),
drivers/pci/pci.h +6 -0
@@ -136,6 +136,12 @@ extern int pcie_mch_quirk;extern struct device_attribute pci_dev_attrs[];extern struct device_attribute dev_attr_cpuaffinity;extern struct device_attribute dev_attr_cpulistaffinity; +#ifdef CONFIG_HOTPLUG +extern struct bus_attribute pci_bus_attrs[]; +#else +#define pci_bus_attrs NULL +#endif +/** * pci_match_one_device - Tell if a PCI device structure has a matching
drivers/pci/probe.c +2 -2
@@ -1219,13 +1219,13 @@ unsigned int __devinit pci_rescan_bus(struct pci_bus *bus) max =pci_scan_child_bus (bus); -up_read (&pci_bus_sem); +down_read (&pci_bus_sem);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); -down_read (&pci_bus_sem); +up_read (&pci_bus_sem);pci_bus_assign_resources (bus);pci_enable_bridges (bus);
PATCH v5 9/13 PCI: Introduce /sys/bus/pci/devices/.../remove
In reply to Alex ChiangThis 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.
---
Documentation/ABI/testing/sysfs-bus-pci | 8 +++++++ Documentation/filesystems/sysfs-pci.txt | 10 +++++++++ drivers/pci/pci-sysfs.c | 36 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 0 deletions(-)
Documentation/ABI/testing/sysfs-bus-pci +8 -0
@@ -66,6 +66,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
@@ -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 +36 -0
@@ -243,6 +243,39 @@ struct bus_attribute pci_bus_attrs[] = {__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), __ATTR_NULL }; + +static void remove_callback (struct device *dev) +{ +struct pci_dev *pdev =to_pci_dev (dev); + +mutex_lock (&pci_remove_rescan_mutex); +pci_remove_bus_device (pdev); +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 (pci_is_root_bus (pdev->bus)) +return -EBUSY ; + +/* An attribute cannot be unregistered by one of its own methods, +* so we have to use this roundabout approach. +*/ +if (val) + ret =device_schedule_callback (dev, remove_callback); +if (ret) + count = ret; +return count; +}#endif struct device_attribute pci_dev_attrs[] = {
@@ -263,6 +296,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 |S_IWGRP ),NULL , remove_store), +#endif __ATTR_NULL, };
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 112 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 | 36 +++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 0 deletions(-)Documentation/ABI/testing/sysfs-bus-pci +8
-0@@ -66,6 +66,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@@ -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 +25
-0@@ -243,6 +243,39 @@ struct bus_attribute pci_bus_attrs[] = {__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), __ATTR_NULL }; + +static void remove_callback (struct device *dev) +{ +struct pci_dev *pdev =to_pci_dev (dev); + +mutex_lock (&pci_remove_rescan_mutex); +pci_remove_bus_device (pdev); +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 (pci_is_root_bus (pdev->bus)) +return -EBUSY ; + +/* An attribute cannot be unregistered by one of its own methods,
drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 280
Quoted 10 lines
drivers/pci/pci-sysfs.c +8
-0+* so we have to use this roundabout approach. +*/ +if (val) + ret =device_schedule_callback (dev, remove_callback); +if (ret) + count = ret; + return count; +}#endif
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 > /sys/bus/pci/devices/ .../remove".
[ 537.379995] =============================================
[ 537.380124] [ INFO: possible recursive locking detected ]
[ 537.380128] 2.6.29-rc8-kk #1
[ 537.380128] ---------------------------------------------
[ 537.380128] events/4/56 is trying to acquire lock:
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
[ 537.380128]
[ 537.380128] but task is already holding lock:
[ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128]
[ 537.380128] other info that might help us debug this:
[ 537.380128] 3 locks held by events/4/56:
[ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40
[ 537.380128]
[ 537.380128] stack backtrace:
[ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1
[ 537.380128] Call Trace:
[ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260
[ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40
[ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80
[ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0
[ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0
[ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20
[ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e]
[ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50
[ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70
[ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90
[ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40
[ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120
[ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190
[ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70
[ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60
[ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0
[ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40
[ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50
[ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230
[ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230
[ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100
[ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40
[ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100
[ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80
[ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20
[ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30
[ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80
[ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20I think the cause of this error message is flush_workqueue() from the
work of keventd. When removing device using "/sys/bus/pci/devices/.../
remove", pci_remove_bus_device() is executed by the keventd's work
through device_schedule_callback(), and it invokes e1000e's remove
callback. And then, e1000e's remove callback invokes flush_workqueue().
Actually, the kernel error messages are not displayed when I changed
e1000e driver to not call flush_workqueue(). In my understanding, flush_workqueue() from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver.
Drivers can use flush_workqueue(), of course.
BTW, I also have another worry about executing pci_remove_bus_device() by the work of keventd. The pci_remove_bus_device() will take a long time especially when the bridge device near the root bus is specified. The long delay of keventd's work will have bad effects to other works on the workqueue.
Thanks,
Kenji Kaneshige
Quoted 18 lines
drivers/pci/pci-sysfs.c +3
-1struct device_attribute pci_dev_attrs[] = {@@ -263,6 +296,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 |S_IWGRP ),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
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Hi Ingo,
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
Quoted 4 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.
Quoted 7 lines
Quoted 7 lines
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]>
[snip part of patch]
Quoted 35 lines
Quoted 35 lines
diff --git
a/drivers/pci/pci-sysfs.cb/drivers/pci/pci-sysfs.c
index be7468a..e16990e 100644drivers/pci/pci-sysfs.c +27
-0@@ -243,6 +243,39 @@ struct bus_attribute pci_bus_attrs[] = {__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), __ATTR_NULL }; + +static void remove_callback (struct device *dev) +{ +struct pci_dev *pdev =to_pci_dev (dev); + +mutex_lock (&pci_remove_rescan_mutex); +pci_remove_bus_device (pdev); +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 (pci_is_root_bus (pdev->bus)) +return -EBUSY ; + +/* An attribute cannot be unregistered by one of its own methods, +* so we have to use this roundabout approach. +*/
Quoted 8 lines
drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 280
Quoted 8 lines
drivers/pci/pci-sysfs.c +6
-0+if (val) + ret =device_schedule_callback (dev, remove_callback); +if (ret) + count = ret; + return count; +}#endif
Kenji Kaneshige reported the below lockdep problem when testing my patch on one of his machines.
Quoted 54 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 [ 537.380128] [ 537.380128] stack backtrace: [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 [ 537.380128] Call Trace: [ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 [ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 [ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 [ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] [ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 [ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 [ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90 [ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40 [ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 [ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190 [ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70 [ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 [ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 [ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40 [ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 [ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230 [ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230 [ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100 [ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 [ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100 [ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80 [ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20 [ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30 [ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80 [ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20
Quoted 12 lines
I think the cause of this error message is
flush_workqueue()from the work of keventd. When removing device using "/sys/bus/pci/devices/.../remove",pci_remove_bus_device()is executed by the keventd's work throughdevice_schedule_callback(), and it invokes e1000e's remove callback. And then, e1000e's remove callback invokesflush_workqueue(). Actually, the kernel error messages are not displayed when I changed e1000e driver to not callflush_workqueue(). In my understanding,flush_workqueue()from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver. Drivers can useflush_workqueue(), of course.
I agree with this analysis; the reason we're seeing this lockdep warning is because the sysfs attributed scheduled a removal for itself using device_schedule_callback(). This is necessary because sysfs attributes can't remove themselves due to other locking issues.
My question is -- is it a bug to call flush_workqueue during run_workqueue?
Conceptually, I don't think it should be a bug; it should be a nop, since run_workqueue _is_ flushing the work queue.
Thoughts?
Quoted 5 lines
BTW, I also have another worry about executing
pci_remove_bus_device()by the work of keventd. Thepci_remove_bus_device()will take a long time especially when the bridge device near the root bus is specified. The long delay of keventd's work will have bad effects to other works on the workqueue.
The real fix is to fix sysfs so that attributes can remove themselves directly. I will work with Tejun Heo on getting this working sooner rather than later. That will avoid the locking issue you discovered above as well as the concern you point out about putting long running tasks in the keventd work queue.
Thanks.
/ac
Quoted 26 lines
Thanks,
Kenji KaneshigeQuoted 18 lines
drivers/pci/pci-sysfs.c +3
-1struct device_attribute pci_dev_attrs[] = {@@ -263,6 +296,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 |S_IWGRP ),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
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
( Cc:-ed a few more interested parties - the thread is about
workqueue dependency lockdep coverage. )
Quoted 132 lines · Alex Chiang
Hi Ingo,
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
Quoted 4 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.
Quoted 7 lines
Quoted 7 lines
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]>[snip part of patch]
Quoted 35 lines
Quoted 35 lines
diff --git
a/drivers/pci/pci-sysfs.cb/drivers/pci/pci-sysfs.c
index be7468a..e16990e 100644drivers/pci/pci-sysfs.c +27
-0@@ -243,6 +243,39 @@ struct bus_attribute pci_bus_attrs[] = {__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), __ATTR_NULL }; + +static void remove_callback (struct device *dev) +{ +struct pci_dev *pdev =to_pci_dev (dev); + +mutex_lock (&pci_remove_rescan_mutex); +pci_remove_bus_device (pdev); +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 (pci_is_root_bus (pdev->bus)) +return -EBUSY ; + +/* An attribute cannot be unregistered by one of its own methods, +* so we have to use this roundabout approach. +*/ Quoted 8 lines
drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 280
Quoted 8 lines
drivers/pci/pci-sysfs.c +6
-0+if (val) + ret =device_schedule_callback (dev, remove_callback); +if (ret) + count = ret; + return count; +}#endif Kenji Kaneshige reported the below lockdep problem when testing my patch on one of his machines.
Quoted 54 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 [ 537.380128] [ 537.380128] stack backtrace: [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 [ 537.380128] Call Trace: [ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 [ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 [ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 [ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] [ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 [ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 [ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90 [ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40 [ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 [ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190 [ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70 [ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 [ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 [ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40 [ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 [ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230 [ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230 [ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100 [ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 [ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100 [ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80 [ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20 [ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30 [ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80 [ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20Quoted 12 lines
I think the cause of this error message is
flush_workqueue()from the work of keventd. When removing device using "/sys/bus/pci/devices/.../remove",pci_remove_bus_device()is executed by the keventd's work throughdevice_schedule_callback(), and it invokes e1000e's remove callback. And then, e1000e's remove callback invokesflush_workqueue(). Actually, the kernel error messages are not displayed when I changed e1000e driver to not callflush_workqueue(). In my understanding,flush_workqueue()from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver. Drivers can useflush_workqueue(), of course.
Quoted 8 lines
I agree with this analysis; the reason we're seeing this lockdep warning is because the sysfs attributed scheduled a removal for itself using
device_schedule_callback(). This is necessary because sysfs attributes can't remove themselves due to other locking issues.My question is -- is it a bug to call
flush_workqueueduringrun_workqueue?
Yes, it generally is.
Quoted 4 lines
Conceptually, I don't think it should be a bug; it should be a nop, since
run_workqueue_is_ flushing the work queue.Thoughts?
well ... but running a work item holds up further processing of the queue - and there lies the deadlock potential. (but ... i have not looked deeply, there's always the possibility of a false positive.)
Ingo
Quoted 12 lines
Quoted 6 lines
BTW, I also have another worry about executing
pci_remove_bus_device()by the work of keventd. Thepci_remove_bus_device()will take a long time especially when the bridge device near the root bus is specified. The long delay of keventd's work will have bad effects to other works on the workqueue.The real fix is to fix sysfs so that attributes can remove themselves directly. I will work with Tejun Heo on getting this working sooner rather than later. That will avoid the locking issue you discovered above as well as the concern you point out about putting long running tasks in the keventd work queue.
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 149 lines · On Tue, 24 Mar 2009 10:25:25 +0100 Ingo Molnar <[email protected]> wrote:
( Cc:-ed a few more interested parties - the thread is about
workqueue dependency lockdep coverage. )Quoted 132 lines · Alex Chiang
Hi Ingo,
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
Quoted 4 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.
Quoted 7 lines
Quoted 7 lines
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]>[snip part of patch]
Quoted 35 lines
Quoted 35 lines
diff --git
a/drivers/pci/pci-sysfs.cb/drivers/pci/pci-sysfs.c
index be7468a..e16990e 100644drivers/pci/pci-sysfs.c +27
-0@@ -243,6 +243,39 @@ struct bus_attribute pci_bus_attrs[] = {__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , bus_rescan_store), __ATTR_NULL }; + +static void remove_callback (struct device *dev) +{ +struct pci_dev *pdev =to_pci_dev (dev); + +mutex_lock (&pci_remove_rescan_mutex); +pci_remove_bus_device (pdev); +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 (pci_is_root_bus (pdev->bus)) +return -EBUSY ; + +/* An attribute cannot be unregistered by one of its own methods, +* so we have to use this roundabout approach. +*/ Quoted 8 lines
drivers/pci/pci-sysfs.c struct bus_attribute pci_bus_attrs[] = { · line 280
Quoted 8 lines
drivers/pci/pci-sysfs.c +6
-0+if (val) + ret =device_schedule_callback (dev, remove_callback); +if (ret) + count = ret; + return count; +}#endif Kenji Kaneshige reported the below lockdep problem when testing my patch on one of his machines.
Quoted 54 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 [ 537.380128] [ 537.380128] stack backtrace: [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 [ 537.380128] Call Trace: [ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 [ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 [ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 [ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] [ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 [ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 [ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90 [ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40 [ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 [ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190 [ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70 [ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 [ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 [ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40 [ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 [ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230 [ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230 [ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100 [ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 [ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100 [ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80 [ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20 [ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30 [ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80 [ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20Quoted 12 lines
I think the cause of this error message is
flush_workqueue()from the work of keventd. When removing device using "/sys/bus/pci/devices/.../remove",pci_remove_bus_device()is executed by the keventd's work throughdevice_schedule_callback(), and it invokes e1000e's remove callback. And then, e1000e's remove callback invokesflush_workqueue(). Actually, the kernel error messages are not displayed when I changed e1000e driver to not callflush_workqueue(). In my understanding,flush_workqueue()from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver. Drivers can useflush_workqueue(), of course.Quoted 8 lines
I agree with this analysis; the reason we're seeing this lockdep warning is because the sysfs attributed scheduled a removal for itself using
device_schedule_callback(). This is necessary because sysfs attributes can't remove themselves due to other locking issues.My question is -- is it a bug to call
flush_workqueueduringrun_workqueue?Yes, it generally is.
Quoted 9 lines
Quoted 4 lines
Conceptually, I don't think it should be a bug; it should be a nop, since
run_workqueue_is_ flushing the work queue.Thoughts?
well ... but running a work item holds up further processing of the queue - and there lies the deadlock potential. (but ... i have not looked deeply, there's always the possibility of a false positive.)
Thing is, we've always supported kevetnd-calls-flush_work(). That's what "morton gets to eat his hat" in run_workqueue() is all about.
Now, -mm's workqueue-avoid-recursion-in-run_workqueue.patch changes all of that. And that patch recently triggered a warning due to some games which USB was playing. I was told this is because USB is being bad.
But I don't think we've seen a coherent description of what's actually _wrong_ with the current code. flush_cpu_workqueue() has been handling this case for many years with no problems reported as far as I know.
So what has caused this sudden flurry of reports? Did something change in lockdep? What is this
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
[ 537.380128]
[ 537.380128] but task is already holding lock:
[ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230supposed to mean? "events" isn't a lock - it's the name of a kernel thread, isn't it? If this is supposed to be deadlockable then how?
Because I don't immediately see what's wrong with e1000_remove() calling flush_work(). It's undesirable, and we can perhaps improve it via some means, but where is the bug?
Quoted 13 lines
Quoted 12 lines
Quoted 6 lines
BTW, I also have another worry about executing
pci_remove_bus_device()by the work of keventd. Thepci_remove_bus_device()will take a long time especially when the bridge device near the root bus is specified. The long delay of keventd's work will have bad effects to other works on the workqueue.The real fix is to fix sysfs so that attributes can remove themselves directly. I will work with Tejun Heo on getting this working sooner rather than later. That will avoid the locking issue you discovered above as well as the concern you point out about putting long running tasks in the keventd work queue.
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 3 lines · Andrew Morton
Thing is, we've always supported kevetnd-calls-flush_work(). That's what "morton gets to eat his hat" in
run_workqueue()is all about.
Supported as in not complained about it, but its always presented a deadlock scenario.
Quoted 2 lines
Now, -mm's workqueue-avoid-recursion-in-run_workqueue.patch changes all of that.
See the discussions around that patch, Lai Jiangshan discovered that it had more deadlock potential than we even suspected.
To quote:
---
Quoted 22 lines · Lai Jiangshan
Quoted 14 lines · Oleg Nesterov
Quoted 13 lines · Lai Jiangshan
DEADLOCK EXAMPLE for explain my above option:
(
work_func0()andwork_func1()are work callback, and they callsflush_workqueue())CPU#0 CPU#1
run_workqueue()run_workqueue()
work_func0()work_func1()
flush_workqueue()flush_workqueue()
flush_cpu_workqueue(0).
flush_cpu_workqueue(cpu#1)flush_cpu_workqueue(cpu#0)
waitingwork_func1()in cpu#1 waitingwork_func0in cpu#0Quoted 5 lines
Quoted 1 line
DEADLOCK!
I am not sure. Note that when
work_func0()callsrun_workqueue(), it will clearcwq->current_work, so anotherflush_on CPU#1 will not wait forwork_func0, no?
Quoted 5 lines
cwq->current_workis changed only when
!list_empty(&cwq->worklist)
inrun_workqueue().so
cwq->current_workmay not be changed.
Ah, indeed.
Thanks for correcting me!
---
Quoted 6 lines
And that patch recently triggered a warning due to some games which
USB was playing. I was told this is because USB is being bad.But I don't think we've seen a coherent description of what's actually _wrong_ with the current code.
flush_cpu_workqueue()has been handling this case for many years with no problems reported as far as I know.
Might be sheer luck, but afaik we did have some actual deadlocks due to workqueue flushing -- a particular one I can remember was cpu-hotplug vs cpufreq.
Quoted 10 lines
So what has caused this sudden flurry of reports? Did something change in lockdep? What is this
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230supposed to mean? "events" isn't a lock - it's the name of a kernel thread, isn't it?
No workqueue lockdep support has been in there for a while now. /me pokes at git for a bit..
4e6045f134784f4b158b3c0f7a282b04bd816887 -- Oct 2007, ca. 2.6.24-rc1
What it does it gives the workqueue a lock-object and each worklet. It then validates that you only get:
workqueue
worklet
nestings, eg. calling flush_workqueue() from a worklet will generate
workqueue <-.
worklet |
workqueue -'
recursion, IOW the above splat.
Another thing it does is connect the lockchains of workqueue callers with those of the worklet. eg.
---
code path 1:
my_function() -> lock(L1); ...; flush_workqueue(); ...
code path 2:
run_workqueue() -> my_work() -> ...; lock(L1); ...
you can get a deadlock when my_work() is queued or running but my_function() has acquired L1 already.
---
Quoted 5 lines
If this is supposed to be deadlockable then how?
Because I don't immediately see what's wrong with
e1000_remove()callingflush_work(). It's undesirable, and we can perhaps improve it via some means, but where is the bug?
I hope the above answers why flushing a workqueue from within that same workqueue is a very bad thing.
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 7 lines · Peter Zijlstra
Quoted 3 lines
But I don't think we've seen a coherent description of what's actually _wrong_ with the current code.
flush_cpu_workqueue()has been handling this case for many years with no problems reported as far as I know.Might be sheer luck, but afaik we did have some actual deadlocks due to workqueue flushing -- a particular one I can remember was cpu-hotplug vs cpufreq.
Two cases are relevant here actually -- the recursion which hasn't ever shown up before, and a number of possible deadlocks of e.g. some people doing, effectively:
rtnl_lock(); flush_scheduled_work(); rtnl_unlock();
vs. the linkwatch work that can, at this point in time, still be queued, and needs the rtnl as well.
A little digging through git logs finds more references, e.g. commits f90d4118bacef87894621a3e8aba853fa0c89abc and fd781fa25c9e9c6fd1599df060b05e7c4ad724e5.
Some others were fixed that I remember, but apparently without putting the lockdep report into the commit log.
johannes
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
In reply to Andrew MortonQuoted 7 lines · Andrew Morton
But I don't think we've seen a coherent description of what's actually _wrong_ with the current code.
flush_cpu_workqueue()has been handling this case for many years with no problems reported as far as I know.So what has caused this sudden flurry of reports? Did something change in lockdep? What is this
Quoted 7 lines
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230supposed to mean? "events" isn't a lock - it's the name of a kernel thread, isn't it? If this is supposed to be deadlockable then how?
events is indeed the schedule_work workqueue thread name -- I just used that for lack of a better name.
Quoted 3 lines
Because I don't immediately see what's wrong with
e1000_remove()callingflush_work(). It's undesirable, and we can perhaps improve it via some means, but where is the bug?
There is no bug -- it's a false positive in a way. I've pointed this out in the original thread, see
http://thread.gmane.org/gmane.linux.kernel/550877/focus=550932
johannes
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
* Johannes Berg <[email protected]>:
Quoted 20 lines · Andrew Morton
Quoted 8 lines
But I don't think we've seen a coherent description of what's actually _wrong_ with the current code.
flush_cpu_workqueue()has been handling this case for many years with no problems reported as far as I know.So what has caused this sudden flurry of reports? Did something change in lockdep? What is this
Quoted 7 lines
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230supposed to mean? "events" isn't a lock - it's the name of a kernel thread, isn't it? If this is supposed to be deadlockable then how?
events is indeed the
schedule_workworkqueue thread name -- I just used that for lack of a better name.
Quoted 7 lines
Quoted 3 lines
Because I don't immediately see what's wrong with
e1000_remove()callingflush_work(). It's undesirable, and we can perhaps improve it via some means, but where is the bug?There is no bug -- it's a false positive in a way. I've pointed this out in the original thread, see
http://thread.gmane.org/gmane.linux.kernel/550877/focus=550932
I'm actually a bit confused now.
Peter explained why flushing a workqueue from the same queue is bad, and in general I agree, but what do you mean by "false positive"?
By the way, this scenario:
code path 1: my_function() -> lock(L1); ...; flush_workqueue(); ... code path 2: run_workqueue() -> my_work() -> ...; lock(L1); ...
is _not_ what is happening here.
sysfs_schedule_callback() is an ugly piece of code that exists because a sysfs attribute cannot remove itself without deadlocking. So the callback mechanism was created to allow a different kernel thread to remove the sysfs attribute and avoid deadlock.
So what you really have going on is:
sysfs callback -> add remove callback to global workqueue remove callback fires off (pci_remove_bus_device) and we do... device_unregister driver's ->remove method called driver's ->remove method calls flush_scheduled_work
Yes, after read the thread I agree that generically calling flush_workqueue in the middle of run_workqueue is bad, but the lockdep warning that Kenji showed us really won't deadlock.
This is because pci_remove_bus_device() will not acquire any lock L1 that an individual device driver will attempt to acquire in the remove path. If that were the case, we would deadlock every time you rmmod'ed a device driver's module or every time you shut your machine down.
I think from my end, there are 2 things I need to do:
a) make sysfs_schedule_callback() use its own work queue instead of global work queue, because too many drivers call flush_scheduled_work in their remove path
b) give sysfs attributes the ability to commit suicide
(a) is short term work, 2.6.30 timeframe, since it doesn't involve any large conceptual changes.
(b) is picking up Tejun Heo's existing work, but that was a bit controversial last time, and I'm not sure it will make it during this merge window.
Question for the lockdep folks though -- given what I described, do you agree that the warning we saw was a false positive? Or am I off in left field?
Thanks.
/ac
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 5 lines · Alex Chiang
Quoted 3 lines
There is no bug -- it's a false positive in a way. I've pointed this out in the original thread, see
http://thread.gmane.org/gmane.linux.kernel/550877/focus=550932I'm actually a bit confused now.
Sorry.
Quoted 3 lines
Peter explained why flushing a workqueue from the same queue is bad, and in general I agree, but what do you mean by "false positive"?
Well, even though generally flushing it from within is bad, the actual thing lockdep reports is bogus -- it's reporting a nested locking.
Quoted 9 lines
By the way, this scenario:
code path 1: my_function() -> lock(L1); ...; flush_workqueue(); ... code path 2: run_workqueue() -> my_work() -> ...; lock(L1); ...is _not_ what is happening here.
Indeed.
Quoted 11 lines
So what you really have going on is:
sysfs callback -> add remove callback to global workqueue remove callback fires off (pci_remove_bus_device) and we do... device_unregister driver's ->remove method called driver's ->remove method calls flush_scheduled_workYes, after read the thread I agree that generically calling
flush_workqueuein the middle ofrun_workqueueis bad, but the lockdep warning that Kenji showed us really won't deadlock.
Exactly that is what I meant by "false positive".
Quoted 17 lines
This is because
pci_remove_bus_device()will not acquire any lock L1 that an individual device driver will attempt to acquire in the remove path. If that were the case, we would deadlock every time you rmmod'ed a device driver's module or every time you shut your machine down.I think from my end, there are 2 things I need to do:
a) make
sysfs_schedule_callback()use its own work queue instead of global work queue, because too many drivers callflush_scheduled_workin their remove pathb) give sysfs attributes the ability to commit suicide
(a) is short term work, 2.6.30 timeframe, since it doesn't involve any large conceptual changes.
Quoted 7 lines
(b) is picking up Tejun Heo's existing work, but that was a bit controversial last time, and I'm not sure it will make it during this merge window.
Question for the lockdep folks though -- given what I described, do you agree that the warning we saw was a false positive? Or am I off in left field?
I think we're not sure yet -- it seems Lai Jiangshan described a scenario in which flushing from within the work actually _can_ deadlock.
johannes
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
In reply to Ingo MolnarQuoted 79 lines · Ingo Molnar
Quoted 79 lines
* Kenji Kaneshige <[email protected]>:
Kenji Kaneshige reported the below lockdep problem when testing my patch on one of his machines.
Quoted 54 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 [ 537.380128] [ 537.380128] stack backtrace: [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 [ 537.380128] Call Trace: [ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 [ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 [ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 [ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] [ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 [ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 [ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90 [ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40 [ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 [ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190 [ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70 [ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 [ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 [ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40 [ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 [ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230 [ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230 [ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100 [ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 [ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100 [ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80 [ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20 [ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30 [ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80 [ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20Quoted 12 lines
I think the cause of this error message is
flush_workqueue()from the work of keventd. When removing device using "/sys/bus/pci/devices/.../remove",pci_remove_bus_device()is executed by the keventd's work throughdevice_schedule_callback(), and it invokes e1000e's remove callback. And then, e1000e's remove callback invokesflush_workqueue(). Actually, the kernel error messages are not displayed when I changed e1000e driver to not callflush_workqueue(). In my understanding,flush_workqueue()from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver. Drivers can useflush_workqueue(), of course.I agree with this analysis; the reason we're seeing this lockdep warning is because the sysfs attributed scheduled a removal for itself using
device_schedule_callback(). This is necessary because sysfs attributes can't remove themselves due to other locking issues.
Quoted 7 lines
Quoted 2 lines
My question is -- is it a bug to call
flush_workqueueduringrun_workqueue?Yes, it generally is.
Quoted 2 lines
Conceptually, I don't think it should be a bug; it should be a nop, since
run_workqueue_is_ flushing the work queue.
As it was already said, we can deadlock.
Can't e1000_remove() avoid flush_scheduled_work() ? (and it should be always avoided when possible).
Of course, I don't understand this code. But afaics e1000_remove() can just cancel its own works (in struct e1000_adapter), no?
cancel_work_sync(work) from run_workqueue() should be OK even if this work is queued on the same wq. If it is queued on the same CPU cancel_work_sync() won't block because we are ->current_work.
Btw. Again, I don't understand the code, but this looks suspicious:
e1000_remove: set_bit(__E1000_DOWN, &adapter->state); del_timer_sync(&adapter->watchdog_timer); flush_scheduled_work();
What if e1000_watchdog_task() is running, has already checked !test_bit(__E1000_DOWN, &adapter->state), but didn't call mod_timer(&adapter->phy_info_timer) yet?
Oleg.
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
(Adding some e1000e folks to answer questions about their driver)
* Oleg Nesterov <[email protected]>:
Quoted 83 lines · Ingo Molnar
Quoted 82 lines
Quoted 81 lines
* Kenji Kaneshige <[email protected]>:
Kenji Kaneshige reported the below lockdep problem when testing my patch on one of his machines.
Quoted 54 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40 [ 537.380128] [ 537.380128] stack backtrace: [ 537.380128] Pid: 56, comm: events/4 Not tainted 2.6.29-rc8-kk #1 [ 537.380128] Call Trace: [ 537.380128] [<ffffffff8026dfcd>] validate_chain+0xb7d/0x1260 [ 537.380128] [<ffffffff8026eade>] __lock_acquire+0x42e/0xa40 [ 537.380128] [<ffffffff8026f148>] lock_acquire+0x58/0x80 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.380128] [<ffffffff8025800d>] flush_workqueue+0x4d/0xa0 [ 537.380128] [<ffffffff80257fc0>] ? flush_workqueue+0x0/0xa0 [ 537.383380] [<ffffffff80258070>] flush_scheduled_work+0x10/0x20 [ 537.383380] [<ffffffffa0144065>] e1000_remove+0x55/0xfe [e1000e] [ 537.383380] [<ffffffff8033ee30>] ? sysfs_schedule_callback_work+0x0/0x50 [ 537.383380] [<ffffffff803bfeb2>] pci_device_remove+0x32/0x70 [ 537.383380] [<ffffffff80441da9>] __device_release_driver+0x59/0x90 [ 537.383380] [<ffffffff80441edb>] device_release_driver+0x2b/0x40 [ 537.383380] [<ffffffff804419d6>] bus_remove_device+0xa6/0x120 [ 537.384382] [<ffffffff8043e46b>] device_del+0x12b/0x190 [ 537.384382] [<ffffffff8043e4f6>] device_unregister+0x26/0x70 [ 537.384382] [<ffffffff803ba969>] pci_stop_dev+0x49/0x60 [ 537.384382] [<ffffffff803baab0>] pci_remove_bus_device+0x40/0xc0 [ 537.384382] [<ffffffff803c10d9>] remove_callback+0x29/0x40 [ 537.384382] [<ffffffff8033ee4f>] sysfs_schedule_callback_work+0x1f/0x50 [ 537.384382] [<ffffffff8025769a>] run_workqueue+0x15a/0x230 [ 537.384382] [<ffffffff80257648>] ? run_workqueue+0x108/0x230 [ 537.384382] [<ffffffff8025846f>] worker_thread+0x9f/0x100 [ 537.384382] [<ffffffff8025bce0>] ? autoremove_wake_function+0x0/0x40 [ 537.384382] [<ffffffff802583d0>] ? worker_thread+0x0/0x100 [ 537.384382] [<ffffffff8025b89d>] kthread+0x4d/0x80 [ 537.384382] [<ffffffff8020d4ba>] child_rip+0xa/0x20 [ 537.386380] [<ffffffff8020cebc>] ? restore_args+0x0/0x30 [ 537.386380] [<ffffffff8025b850>] ? kthread+0x0/0x80 [ 537.386380] [<ffffffff8020d4b0>] ? child_rip+0x0/0x20Quoted 12 lines
I think the cause of this error message is
flush_workqueue()from the work of keventd. When removing device using "/sys/bus/pci/devices/.../remove",pci_remove_bus_device()is executed by the keventd's work throughdevice_schedule_callback(), and it invokes e1000e's remove callback. And then, e1000e's remove callback invokesflush_workqueue(). Actually, the kernel error messages are not displayed when I changed e1000e driver to not callflush_workqueue(). In my understanding,flush_workqueue()from the work must be avoided because it can cause a deadlock. Please note that this is not a problem of e1000e driver. Drivers can useflush_workqueue(), of course.I agree with this analysis; the reason we're seeing this lockdep warning is because the sysfs attributed scheduled a removal for itself using
device_schedule_callback(). This is necessary because sysfs attributes can't remove themselves due to other locking issues.My question is -- is it a bug to call
flush_workqueueduringrun_workqueue?
Quoted 6 lines
Quoted 4 lines
Yes, it generally is.
Quoted 2 lines
Conceptually, I don't think it should be a bug; it should be a nop, since
run_workqueue_is_ flushing the work queue.As it was already said, we can deadlock.
As answered in another email, I agree that generically, it can deadlock. I think this particular warning is a false positive though, because pci_remove_bus_device() definitely cannot acquire any locks that lower level device drivers want to acquire.
If that were true, then hotplug would never work, and you would also see hangs every time you shut your machine off.
I can't answer the e1000e questions below but...
Quoted 19 lines
Can't
e1000_remove()avoidflush_scheduled_work()? (and it should be always avoided when possible).Of course, I don't understand this code. But afaics
e1000_remove()can just cancel its own works (in structe1000_adapter), no?
cancel_work_sync(work)fromrun_workqueue()should be OK even if this work is queued on the same wq. If it is queued on the same CPUcancel_work_sync()won't block because we are ->current_work.Btw. Again, I don't understand the code, but this looks suspicious:
e1000_remove: set_bit(__E1000_DOWN, &adapter->state); del_timer_sync(&adapter->watchdog_timer); flush_scheduled_work();
Quoted 3 lines
What if
e1000_watchdog_task()is running, has already checked !test_bit(__E1000_DOWN,&adapter->state), but didn't callmod_timer(&adapter->phy_info_timer)yet?
Even if e1000e is doing something funny, the conclusion I'm coming to is that the sysfs_schedule_callback implementation needs to change, because there are simply too many drivers that call flush_scheduled_work.
Thanks.
/ac
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
In reply to Kenji Kaneshige* Kenji Kaneshige <[email protected]>:
Quoted 6 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".
Quoted 15 lines
[ 537.379995] =============================================
[ 537.380124] [ INFO: possible recursive locking detected ]
[ 537.380128] 2.6.29-rc8-kk #1
[ 537.380128] ---------------------------------------------
[ 537.380128] events/4/56 is trying to acquire lock:
[ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0
[ 537.380128]
[ 537.380128] but task is already holding lock:
[ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128]
[ 537.380128] other info that might help us debug this:
[ 537.380128] 3 locks held by events/4/56:
[ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230
[ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40I still cannot reproduce this lockdep issue, even using your .config with an e1000e device on an x86_64 kernel. :(
I tried removing the endpoint, an intermediate bridge device, and the parent bus. I don't know what I'm doing wrong...
Can you please try this patch though, and see if it fixes the warning? It applies on top of my other sysfs patch that introduces a mutex in sysfs_schedule_callback.
Thanks.
/ac
fs/sysfs/file.c +8 -1
@@ -667,6 +667,7 @@ struct sysfs_schedule_callback_struct {struct work_struct work; }; +static struct workqueue_struct *sysfs_workqueue;static DEFINE_MUTEX (sysfs_workq_mutex);static LIST_HEAD (sysfs_workq);static void sysfs_schedule_callback_work (struct work_struct *work)
@@ -720,6 +721,12 @@ int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *), }mutex_unlock (&sysfs_workq_mutex); +if (sysfs_workqueue ==NULL ) { + sysfs_workqueue =create_workqueue ("sysfsqueue" ); +if (sysfs_workqueue ==NULL ) +return -ENOMEM ; + } + ss =kmalloc (sizeof (*ss),GFP_KERNEL );if (!ss) {module_put (owner);
@@ -735,7 +742,7 @@ int sysfs_schedule_callback(struct kobject *kobj, void (*func)(void *),mutex_lock (&sysfs_workq_mutex);list_add_tail (&ss->workq_list, &sysfs_workq);mutex_unlock (&sysfs_workq_mutex); -schedule_work (&ss->work); +queue_work (sysfs_workqueue, &ss->work);return 0 ; }EXPORT_SYMBOL_GPL (sysfs_schedule_callback);
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 22 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".Quoted 15 lines
[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40
Quoted 6 lines
I still cannot reproduce this lockdep issue, even using your .config with an e1000e device on an
x86_64kernel. :(I tried removing the endpoint, an intermediate bridge device, and the parent bus. I don't know what I'm doing wrong...
I don't know either...
The reproducibility is 100% on my environment. The steps are just boot the system and remove the device.
Quoted 3 lines
Can you please try this patch though, and see if it fixes the warning? It applies on top of my other sysfs patch that introduces a mutex in
sysfs_schedule_callback.
Anyway, I confirmed the kernel error messages were gone with the patch against sysfs. Note that I used the following patch I made for testing instead since your patch could not be applied to Jesse's linux-next.
Thanks,
Kenji Kaneshige
fs/sysfs/file.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
fs/sysfs/file.c +13 -1
@@ -677,6 +677,7 @@kfree (ss); } +static struct workqueue_struct *sysfsd_wq;/** * sysfs_schedule_callback - helper to schedule a callback for a kobject * @kobj: object we're acting for.
@@ -704,6 +705,17 @@if (!try_module_get (owner))return -ENODEV ; + +if (!sysfsd_wq) { + sysfsd_wq =create_workqueue ("sysfsd" ); +if (!sysfsd_wq) { +printk (KERN_ERR +"%s: Could not create workqueue\n" , __func__); +WARN_ON (1 ); +return -ENOMEM ; + } + } + ss =kmalloc (sizeof (*ss),GFP_KERNEL );if (!ss) {module_put (owner);
@@ -715,7 +727,7 @@ ss->data = data; ss->owner = owner;INIT_WORK (&ss->work, sysfs_schedule_callback_work); -schedule_work (&ss->work); +queue_work (sysfsd_wq, &ss->work);return 0 ; }EXPORT_SYMBOL_GPL (sysfs_schedule_callback);
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
* Kenji Kaneshige <[email protected]>:
Quoted 34 lines
Quoted 22 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".Quoted 15 lines
[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40Quoted 6 lines
I still cannot reproduce this lockdep issue, even using your .config with an e1000e device on an
x86_64kernel. :(I tried removing the endpoint, an intermediate bridge device, and the parent bus. I don't know what I'm doing wrong...
I don't know either...
The reproducibility is 100% on my environment. The steps are just boot the system and remove the device.
Quoted 8 lines
Quoted 3 lines
Can you please try this patch though, and see if it fixes the warning? It applies on top of my other sysfs patch that introduces a mutex in
sysfs_schedule_callback.Anyway, I confirmed the kernel error messages were gone with the patch against sysfs. Note that I used the following patch I made for testing instead since your patch could not be applied to Jesse's linux-next.
Great, thank you for testing Kenji-san.
/ac
Re: [PATCH v5 09/13] PCI: Introduce /sys/bus/pci/devices/.../remove
Quoted 36 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 35 lines
Quoted 21 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 5 lines
I still have the following kernel error messages in testing with your latest set of patches (Jesse's linux-next). The test case is removing e1000e device or its parent bridge by "echo 1 >
/sys/bus/pci/devices/.../remove".Quoted 15 lines
[ 537.379995] ============================================= [ 537.380124] [ INFO: possible recursive locking detected ] [ 537.380128] 2.6.29-rc8-kk #1 [ 537.380128] --------------------------------------------- [ 537.380128] events/4/56 is trying to acquire lock: [ 537.380128] (events){--..}, at: [<ffffffff80257fc0>] flush_workqueue+0x0/0xa0 [ 537.380128] [ 537.380128] but task is already holding lock: [ 537.380128] (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] [ 537.380128] other info that might help us debug this: [ 537.380128] 3 locks held by events/4/56: [ 537.380128] #0: (events){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #1: (&ss->work){--..}, at: [<ffffffff80257648>] run_workqueue+0x108/0x230 [ 537.380128] #2: (pci_remove_rescan_mutex){--..}, at: [<ffffffff803c10d1>] remove_callback+0x21/0x40Quoted 6 lines
I still cannot reproduce this lockdep issue, even using your .config with an e1000e device on an
x86_64kernel. :(I tried removing the endpoint, an intermediate bridge device, and the parent bus. I don't know what I'm doing wrong...
I don't know either...
The reproducibility is 100% on my environment. The steps are just boot the system and remove the device.Quoted 3 lines
Can you please try this patch though, and see if it fixes the warning? It applies on top of my other sysfs patch that introduces a mutex in
sysfs_schedule_callback.
Quoted 7 lines
Quoted 4 lines
Anyway, I confirmed the kernel error messages were gone with the patch against sysfs. Note that I used the following patch I made for testing instead since your patch could not be applied to Jesse's linux-next.
Great, thank you for testing Kenji-san.
You're welcome.
Just in case, my patch is just for testing, and it is very buggy (no destroy operation, lack of module_put() in error code path, and so on). Please consider it as just for testing.
Thanks,
Kenji Kaneshige
PATCH v5 10/13 PCI: Introduce /sys/bus/pci/devices/.../rescan
In reply to Alex ChiangThis 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 | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 0 deletions(-)
Documentation/ABI/testing/sysfs-bus-pci +10 -0
@@ -74,6 +74,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]>
drivers/pci/pci-sysfs.c +19 -0
@@ -244,6 +244,24 @@ struct bus_attribute pci_bus_attrs[] = { __ATTR_NULL }; +static ssize_t +dev_rescan_store (struct device *dev,struct device_attribute *attr, +const char *buf,size_t count) +{ +unsigned long val; +struct pci_dev *pdev =to_pci_dev (dev); + +if (strict_strtoul (buf,0 , &val) <0 ) +return -EINVAL ; + +if (val) { +mutex_lock (&pci_remove_rescan_mutex); +pci_rescan_bus (pdev->bus); +mutex_unlock (&pci_remove_rescan_mutex); + } +return count; +} +static void remove_callback (struct device *dev) {struct pci_dev *pdev =to_pci_dev (dev);
@@ -298,6 +316,7 @@ struct device_attribute pci_dev_attrs[] = { __ATTR(msi_bus, 0644, msi_bus_show, msi_bus_store),#ifdef CONFIG_HOTPLUG __ATTR (remove, (S_IWUSR |S_IWGRP ),NULL , remove_store), +__ATTR (rescan, (S_IWUSR |S_IWGRP ),NULL , dev_rescan_store), #endif __ATTR_NULL, };
PATCH v5 11/13 PCI Hotplug: restore fakephp interface with complete reimplementation
In reply to Alex ChiangFrom: 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
drivers/pci/hotplug/Makefile +1 -1
@@ -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
drivers/pci/hotplug/fakephp.c deleted +0 -395
395 lines not on this page · raw message
drivers/pci/hotplug/legacy_fakephp.c new file +162 -0
162 lines not on this page · raw message
PATCH v5 12/13 PCI Hotplug: rename legacy_fakephp to fakephp
In reply to Alex ChiangWe 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
drivers/pci/hotplug/Makefile +1 -1
@@ -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
drivers/pci/hotplug/fakephp.c new file +162 -0
162 lines not on this page · raw message
drivers/pci/hotplug/legacy_fakephp.c deleted +0 -162
162 lines not on this page · raw message
PATCH v5 13/13 PCI Hotplug: schedule fakephp for feature removal
In reply to Alex ChiangNow 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 | 33 ++++++++++++++++++++++++++++ 1 files changed, 33 insertions(+), 0 deletions(-)
Documentation/feature-removal-schedule.txt +33 -0
@@ -335,6 +335,7 @@ 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: sysfs ui for changing p4-clockmod parameters
@@ -344,3 +345,35 @@ Why: See commits 129f8ae9b1b5be94517da76009ea956e89104ce8 and Removal is subject to fixing any remaining bugs in ACPI which may cause the thermal throttling not to happen at the right time. Who: Dave Jones <[email protected]>, Matthew Garrett <[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]>
Re: [PATCH v5 13/13] PCI Hotplug: schedule fakephp for feature removal
Quoted 6 lines · Alex Chiang
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 Why: See commits 129f8ae9b1b5be94517da76009ea956e89104ce8 and · line 352
Quoted 3 lines
Documentation/feature-removal-schedule.txt +3
-0+ +What: fakephp and associated sysfs files in /sys/bus/pci/slots/ +When: 2011
Hi, Alex,
So it passed three years!
Can you please submit patch to remove fakephp ?
Thanks
Yinghai Lu