| Message ID | 1335539820-11232-3-git-send-email-jiang.liu@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, Apr 27, 2012 at 11:16:43PM +0800, Jiang Liu wrote: > From: Jiang Liu <jiang.liu@huawei.com> > > There are multiple ways to trigger PCI hotplug requests concurrently, > such as: > 1. Sysfs interfaces exported by the PCI core subsystem > 2. Sysfs interfaces exported by the PCI hotplug subsystem > 3. PCI hotplug events triggered by PCI Hotplug Controllers > 4. ACPI hotplug events for PCI host bridges > 5. Driver binding/unbinding events > > The PCI core subsystem doesn't support concurrent hotplug operations yet, > so all PCI hotplug requests should be globally serialized. This patch > introduces several new interfaces to serialize PCI hotplug operations. > > pci_hotplug_try_enter(): try to acquire write lock Ick, no, why would you ever want to do that? > pci_hotplug_enter(): acquire write lock > pci_hotplug_exit(): release write lock > pci_hotplug_disable(): acquire read lock > pci_hotplug_enable(): release read lock No, the pci hotplug core should not need a rwsem, just a simple lock, if that: pci_hotplug_lock() pci_hotplug_unlock() and that's it. Really you should not need these functions, the pci hotplug core should handle it for you, and the drivers should not care at all. That's the "proper" way to fix this up, serialize stuff within the pci core, not the individual drivers. > Today we have reproduced the issue on a real platform by using > acpiphp driver. It's an IA64 platform running Suse 11SP1 (official > 2.6.32.12 kernel). The test script is: > > This issue could be reproduced on an IA64 platform with Suse 11SP1 > (official 2.6.32.12 kernel) and acpiphp driver. You do realize just how old that kernel is right? Please don't assume that this kernel version is relevant to us all these years later. > +/* > + * trylock for writing -- returns 1 if successful, 0 if contention > + */ > +int pci_hotplug_try_enter(void) > +{ > + if (current != pci_hotplug_mutex_owner) { > + if (down_write_trylock(&pci_hotplug_rwsem) == 0) > + return 0; > + pci_hotplug_mutex_owner = current; > + } > + pci_hotplug_mutex_recursive++; > + > + return 1; > +} > +EXPORT_SYMBOL(pci_hotplug_try_enter); Don't try to invent new lock types like this, you are bound to get it wrong. And don't create recursive locks, fix the code up to never need it. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-5-2 13:00, Greg KH wrote: > On Fri, Apr 27, 2012 at 11:16:43PM +0800, Jiang Liu wrote: >> From: Jiang Liu<jiang.liu@huawei.com> >> >> There are multiple ways to trigger PCI hotplug requests concurrently, >> such as: >> 1. Sysfs interfaces exported by the PCI core subsystem >> 2. Sysfs interfaces exported by the PCI hotplug subsystem >> 3. PCI hotplug events triggered by PCI Hotplug Controllers >> 4. ACPI hotplug events for PCI host bridges >> 5. Driver binding/unbinding events >> >> The PCI core subsystem doesn't support concurrent hotplug operations yet, >> so all PCI hotplug requests should be globally serialized. This patch >> introduces several new interfaces to serialize PCI hotplug operations. >> >> pci_hotplug_try_enter(): try to acquire write lock > > Ick, no, why would you ever want to do that? > >> pci_hotplug_enter(): acquire write lock >> pci_hotplug_exit(): release write lock >> pci_hotplug_disable(): acquire read lock >> pci_hotplug_enable(): release read lock > > No, the pci hotplug core should not need a rwsem, just a simple lock, if > that: > pci_hotplug_lock() > pci_hotplug_unlock() > and that's it. > > Really you should not need these functions, the pci hotplug core should > handle it for you, and the drivers should not care at all. That's the > "proper" way to fix this up, serialize stuff within the pci core, not > the individual drivers. > >> Today we have reproduced the issue on a real platform by using >> acpiphp driver. It's an IA64 platform running Suse 11SP1 (official >> 2.6.32.12 kernel). The test script is: >> >> This issue could be reproduced on an IA64 platform with Suse 11SP1 >> (official 2.6.32.12 kernel) and acpiphp driver. > > You do realize just how old that kernel is right? Please don't assume > that this kernel version is relevant to us all these years later. This could be reproduced with latest kernel too, such as 3.3. We use 2.6.32.12 because the all 3.x series kernels can't boot on our IA64 platform due to an issue in scheduler. We have reproduced similar issue on x86 platforms with 3.x kernels. > >> +/* >> + * trylock for writing -- returns 1 if successful, 0 if contention >> + */ >> +int pci_hotplug_try_enter(void) >> +{ >> + if (current != pci_hotplug_mutex_owner) { >> + if (down_write_trylock(&pci_hotplug_rwsem) == 0) >> + return 0; >> + pci_hotplug_mutex_owner = current; >> + } >> + pci_hotplug_mutex_recursive++; >> + >> + return 1; >> +} >> +EXPORT_SYMBOL(pci_hotplug_try_enter); > > Don't try to invent new lock types like this, you are bound to get it > wrong. And don't create recursive locks, fix the code up to never need > it. > > thanks, > > greg k-h > > . > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 02, 2012 at 03:25:21PM +0800, Jiang Liu wrote: > On 2012-5-2 13:00, Greg KH wrote: > >On Fri, Apr 27, 2012 at 11:16:43PM +0800, Jiang Liu wrote: > >>From: Jiang Liu<jiang.liu@huawei.com> > >> > >>There are multiple ways to trigger PCI hotplug requests concurrently, > >>such as: > >>1. Sysfs interfaces exported by the PCI core subsystem > >>2. Sysfs interfaces exported by the PCI hotplug subsystem > >>3. PCI hotplug events triggered by PCI Hotplug Controllers > >>4. ACPI hotplug events for PCI host bridges > >>5. Driver binding/unbinding events > >> > >>The PCI core subsystem doesn't support concurrent hotplug operations yet, > >>so all PCI hotplug requests should be globally serialized. This patch > >>introduces several new interfaces to serialize PCI hotplug operations. > >> > >>pci_hotplug_try_enter(): try to acquire write lock > > > >Ick, no, why would you ever want to do that? > > > >>pci_hotplug_enter(): acquire write lock > >>pci_hotplug_exit(): release write lock > >>pci_hotplug_disable(): acquire read lock > >>pci_hotplug_enable(): release read lock > > > >No, the pci hotplug core should not need a rwsem, just a simple lock, if > >that: > > pci_hotplug_lock() > > pci_hotplug_unlock() > >and that's it. > > > >Really you should not need these functions, the pci hotplug core should > >handle it for you, and the drivers should not care at all. That's the > >"proper" way to fix this up, serialize stuff within the pci core, not > >the individual drivers. > > > >>Today we have reproduced the issue on a real platform by using > >>acpiphp driver. It's an IA64 platform running Suse 11SP1 (official > >>2.6.32.12 kernel). The test script is: > >> > >>This issue could be reproduced on an IA64 platform with Suse 11SP1 > >>(official 2.6.32.12 kernel) and acpiphp driver. > > > >You do realize just how old that kernel is right? Please don't assume > >that this kernel version is relevant to us all these years later. > This could be reproduced with latest kernel too, such as 3.3. We > use 2.6.32.12 because the all 3.x series kernels can't boot on > our IA64 platform due to an issue in scheduler. We have reproduced > similar issue on x86 platforms with 3.x kernels. That's wonderful, and is relevant, unlike what you originally wrote. Please always write relevant changelog comments, otherwise they are... greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/hotplug.c b/drivers/pci/hotplug.c index 2b5352a..975bd3d 100644 --- a/drivers/pci/hotplug.c +++ b/drivers/pci/hotplug.c @@ -1,8 +1,63 @@ #include <linux/kernel.h> #include <linux/pci.h> #include <linux/module.h> +#include <linux/rwsem.h> #include "pci.h" +/* Recursive mutex for PCI hotplug operations. */ +static DECLARE_RWSEM(pci_hotplug_rwsem); +static struct task_struct *pci_hotplug_mutex_owner; +static int pci_hotplug_mutex_recursive; + +/* + * trylock for writing -- returns 1 if successful, 0 if contention + */ +int pci_hotplug_try_enter(void) +{ + if (current != pci_hotplug_mutex_owner) { + if (down_write_trylock(&pci_hotplug_rwsem) == 0) + return 0; + pci_hotplug_mutex_owner = current; + } + pci_hotplug_mutex_recursive++; + + return 1; +} +EXPORT_SYMBOL(pci_hotplug_try_enter); + +void pci_hotplug_enter(void) +{ + if (current != pci_hotplug_mutex_owner) { + down_write(&pci_hotplug_rwsem); + pci_hotplug_mutex_owner = current; + } + pci_hotplug_mutex_recursive++; + +} +EXPORT_SYMBOL(pci_hotplug_enter); + +void pci_hotplug_exit(void) +{ + BUG_ON(pci_hotplug_mutex_owner != current); + if (--pci_hotplug_mutex_recursive == 0) { + pci_hotplug_mutex_owner = NULL; + up_write(&pci_hotplug_rwsem); + } +} +EXPORT_SYMBOL(pci_hotplug_exit); + +void pci_hotplug_enable(void) +{ + up_read(&pci_hotplug_rwsem); +} +EXPORT_SYMBOL(pci_hotplug_enable); + +void pci_hotplug_disable(void) +{ + down_read(&pci_hotplug_rwsem); +} +EXPORT_SYMBOL(pci_hotplug_disable); + int pci_uevent(struct device *dev, struct kobj_uevent_env *env) { struct pci_dev *pdev; diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c index 202f4a9..1572665 100644 --- a/drivers/pci/hotplug/pci_hotplug_core.c +++ b/drivers/pci/hotplug/pci_hotplug_core.c @@ -537,7 +537,7 @@ int __must_check pci_hp_change_slot_info(struct hotplug_slot *hotplug, return 0; } -static int __init pci_hotplug_init (void) +static int __init pci_hp_init(void) { int result; @@ -553,13 +553,13 @@ err_cpci: return result; } -static void __exit pci_hotplug_exit (void) +static void __exit pci_hp_exit(void) { cpci_hotplug_exit(); } -module_init(pci_hotplug_init); -module_exit(pci_hotplug_exit); +module_init(pci_hp_init); +module_exit(pci_hp_exit); MODULE_AUTHOR(DRIVER_AUTHOR); MODULE_DESCRIPTION(DRIVER_DESC); diff --git a/include/linux/pci.h b/include/linux/pci.h index 0603a60..1c5f153 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -884,6 +884,20 @@ unsigned int pci_rescan_bus_bridge_resize(struct pci_dev *bridge); unsigned int pci_rescan_bus(struct pci_bus *bus); #endif +#ifdef CONFIG_HOTPLUG +extern int pci_hotplug_try_enter(void); +extern void pci_hotplug_enter(void); +extern void pci_hotplug_exit(void); +extern void pci_hotplug_disable(void); +extern void pci_hotplug_enable(void); +#else +static inline int pci_hotplug_try_enter(void) { return 1; } +static inline void pci_hotplug_enter(void) {} +static inline void pci_hotplug_exit(void) {} +static inline void pci_hotplug_enable(void) {} +static inline void pci_hotplug_disable(void) {} +#endif + /* Vital product data routines */ ssize_t pci_read_vpd(struct pci_dev *dev, loff_t pos, size_t count, void *buf); ssize_t pci_write_vpd(struct pci_dev *dev, loff_t pos, size_t count, const void *buf);