[PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Yijing Wang]

Yijing Wang (2):
  PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
  PCI: Lock pci_slot_mutex when traverse bus->slots

 arch/powerpc/kernel/pci_of_scan.c |    6 +-----
 arch/sparc/kernel/pci.c           |    6 +-----
 drivers/pci/probe.c               |    6 +-----
 drivers/pci/slot.c                |   26 +++++++++++++++++++++-----
 include/linux/pci.h               |    4 +++-
 5 files changed, 27 insertions(+), 21 deletions(-)

[PATCH v2 1/2] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock

[Yijing Wang]

Rajat Jain reported a deadlock when a hierarchical hot plug
thread and aer recovery thread both run.
https://lkml.org/lkml/2015/3/11/861

thread 1:
pciehp_enable_slot()
	pciehp_configure_device()
		pci_bus_add_devices()
			device_attach(dev)
				device_lock(dev) //acquire device mutex successfully
			...
			pciehp_probe(dev)
				__pci_hp_register()
					pci_create_slot()
						down_write(pci_bus_sem) //deadlock here

thread 2:
aer_isr_one_error()
	aer_process_err_device()
		do_recovery()
			broadcast_error_message()
				pci_walk_bus()
					down_read(&pci_bus_sem) //acquire pci_bus_sem successfully
						report_error_detected(dev)
							device_lock(dev) // deadlock here

We use down_write(&pci_bus_sem) to protect the bus->slots list, because the
bus->slots list is only accessed in drivers/pci/slot.c, we could introduce
a new local mutex to protect bus->slots, and use down_read(&pci_bus_sem)
instead of down_write(&pci_bus_sem) to protect the bus->devices list.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/slot.c  |   15 ++++++++++-----
 include/linux/pci.h |    3 ++-
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 396c200..a9079d9 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -14,6 +14,7 @@
 
 struct kset *pci_slots_kset;
 EXPORT_SYMBOL_GPL(pci_slots_kset);
+static DEFINE_MUTEX(pci_slot_mutex);
 
 static ssize_t pci_slot_attr_show(struct kobject *kobj,
 					struct attribute *attr, char *buf)
@@ -106,9 +107,11 @@ static void pci_slot_release(struct kobject *kobj)
 	dev_dbg(&slot->bus->dev, "dev %02x, released physical slot %s\n",
 		slot->number, pci_slot_name(slot));
 
+	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &slot->bus->devices, bus_list)
 		if (PCI_SLOT(dev->devfn) == slot->number)
 			dev->slot = NULL;
+	up_read(&pci_bus_sem);
 
 	list_del(&slot->list);
 
@@ -195,7 +198,7 @@ static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)
 {
 	struct pci_slot *slot;
 	/*
-	 * We already hold pci_bus_sem so don't worry
+	 * We already hold pci_slot_mutex so don't worry
 	 */
 	list_for_each_entry(slot, &parent->slots, list)
 		if (slot->number == slot_nr) {
@@ -253,7 +256,7 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 	int err = 0;
 	char *slot_name = NULL;
 
-	down_write(&pci_bus_sem);
+	mutex_lock(&pci_slot_mutex);
 
 	if (slot_nr == -1)
 		goto placeholder;
@@ -301,16 +304,18 @@ placeholder:
 	INIT_LIST_HEAD(&slot->list);
 	list_add(&slot->list, &parent->slots);
 
+	down_read(&pci_bus_sem);
 	list_for_each_entry(dev, &parent->devices, bus_list)
 		if (PCI_SLOT(dev->devfn) == slot_nr)
 			dev->slot = slot;
+	up_read(&pci_bus_sem);
 
 	dev_dbg(&parent->dev, "dev %02x, created physical slot %s\n",
 		slot_nr, pci_slot_name(slot));
 
 out:
 	kfree(slot_name);
-	up_write(&pci_bus_sem);
+	mutex_unlock(&pci_slot_mutex);
 	return slot;
 err:
 	kfree(slot);
@@ -332,9 +337,9 @@ void pci_destroy_slot(struct pci_slot *slot)
 	dev_dbg(&slot->bus->dev, "dev %02x, dec refcount to %d\n",
 		slot->number, atomic_read(&slot->kobj.kref.refcount) - 1);
 
-	down_write(&pci_bus_sem);
+	mutex_lock(&pci_slot_mutex);
 	kobject_put(&slot->kobj);
-	up_write(&pci_bus_sem);
+	mutex_unlock(&pci_slot_mutex);
 }
 EXPORT_SYMBOL_GPL(pci_destroy_slot);
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8a0321a..34cc95d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -446,7 +446,8 @@ struct pci_bus {
 	struct list_head children;	/* list of child buses */
 	struct list_head devices;	/* list of devices on this bus */
 	struct pci_dev	*self;		/* bridge device as seen by parent */
-	struct list_head slots;		/* list of slots on this bus */
+	struct list_head slots;		/* list of slots on this bus, we use a local pci_slot_mutex instead of
+		                           pci_bus_sem to protect this list to fix a potential ABBA deadlock */
 	struct resource *resource[PCI_BRIDGE_RESOURCE_NUM];
 	struct list_head resources;	/* address space routed to this bus */
 	struct resource busn_res;	/* bus numbers routed to this bus */
-- 
1.7.1

Re: [PATCH v2 1/2] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock

[Guenter Roeck]

On 07/17/2015 02:16 AM, Yijing Wang wrote:
> Rajat Jain reported a deadlock when a hierarchical hot plug
> thread and aer recovery thread both run.
> https://lkml.org/lkml/2015/3/11/861
>
> thread 1:
> pciehp_enable_slot()
> 	pciehp_configure_device()
> 		pci_bus_add_devices()
> 			device_attach(dev)
> 				device_lock(dev) //acquire device mutex successfully
> 			...
> 			pciehp_probe(dev)
> 				__pci_hp_register()
> 					pci_create_slot()
> 						down_write(pci_bus_sem) //deadlock here
>
> thread 2:
> aer_isr_one_error()
> 	aer_process_err_device()
> 		do_recovery()
> 			broadcast_error_message()
> 				pci_walk_bus()
> 					down_read(&pci_bus_sem) //acquire pci_bus_sem successfully
> 						report_error_detected(dev)
> 							device_lock(dev) // deadlock here
>
> We use down_write(&pci_bus_sem) to protect the bus->slots list, because the
> bus->slots list is only accessed in drivers/pci/slot.c, we could introduce
> a new local mutex to protect bus->slots, and use down_read(&pci_bus_sem)
> instead of down_write(&pci_bus_sem) to protect the bus->devices list.
>
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>

I applied both patches to our system and ran a number of tests.
Works fine as far as I can see.

Tested-by: Guenter Roeck <linux@roeck-us.net>

Re: [PATCH v2 1/2] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock

[wangyijing]

在 2015/7/24 11:07, Guenter Roeck 写道:
> On 07/17/2015 02:16 AM, Yijing Wang wrote:
>> Rajat Jain reported a deadlock when a hierarchical hot plug
>> thread and aer recovery thread both run.
>> https://lkml.org/lkml/2015/3/11/861
>>
>> thread 1:
>> pciehp_enable_slot()
>>     pciehp_configure_device()
>>         pci_bus_add_devices()
>>             device_attach(dev)
>>                 device_lock(dev) //acquire device mutex successfully
>>             ...
>>             pciehp_probe(dev)
>>                 __pci_hp_register()
>>                     pci_create_slot()
>>                         down_write(pci_bus_sem) //deadlock here
>>
>> thread 2:
>> aer_isr_one_error()
>>     aer_process_err_device()
>>         do_recovery()
>>             broadcast_error_message()
>>                 pci_walk_bus()
>>                     down_read(&pci_bus_sem) //acquire pci_bus_sem successfully
>>                         report_error_detected(dev)
>>                             device_lock(dev) // deadlock here
>>
>> We use down_write(&pci_bus_sem) to protect the bus->slots list, because the
>> bus->slots list is only accessed in drivers/pci/slot.c, we could introduce
>> a new local mutex to protect bus->slots, and use down_read(&pci_bus_sem)
>> instead of down_write(&pci_bus_sem) to protect the bus->devices list.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> 
> I applied both patches to our system and ran a number of tests.
> Works fine as far as I can see.
> 
> Tested-by: Guenter Roeck <linux@roeck-us.net>

Guenter, thanks very much!

Thanks!
Yijing.

> 
> 
> 

[PATCH v2 2/2] PCI: Lock pci_slot_mutex when traverse bus->slots

[Yijing Wang]

Now we traverse bus->slots in pci_setup_device()
and other places without a lock protect. It's not
safe, if we traverse a bus->slots and free the slots
at the same time, system may crash. Now we have
pci_slot_mutex, use it to protect the bus->slots list.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 arch/powerpc/kernel/pci_of_scan.c |    6 +-----
 arch/sparc/kernel/pci.c           |    6 +-----
 drivers/pci/probe.c               |    6 +-----
 drivers/pci/slot.c                |   11 +++++++++++
 include/linux/pci.h               |    1 +
 5 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
index 42e02a2..5e2debf 100644
--- a/arch/powerpc/kernel/pci_of_scan.c
+++ b/arch/powerpc/kernel/pci_of_scan.c
@@ -126,7 +126,6 @@ struct pci_dev *of_create_pci_dev(struct device_node *node,
 {
 	struct pci_dev *dev;
 	const char *type;
-	struct pci_slot *slot;
 
 	dev = pci_alloc_dev(bus);
 	if (!dev)
@@ -145,10 +144,7 @@ struct pci_dev *of_create_pci_dev(struct device_node *node,
 	dev->needs_freset = 0;		/* pcie fundamental reset required */
 	set_pcie_port_type(dev);
 
-	list_for_each_entry(slot, &dev->bus->slots, list)
-		if (PCI_SLOT(dev->devfn) == slot->number)
-			dev->slot = slot;
-
+	pci_dev_assign_slot(dev);
 	dev->vendor = get_int_prop(node, "vendor-id", 0xffff);
 	dev->device = get_int_prop(node, "device-id", 0xffff);
 	dev->subsystem_vendor = get_int_prop(node, "subsystem-vendor-id", 0);
diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
index c928bc6..3a0e1a9 100644
--- a/arch/sparc/kernel/pci.c
+++ b/arch/sparc/kernel/pci.c
@@ -249,7 +249,6 @@ static struct pci_dev *of_create_pci_dev(struct pci_pbm_info *pbm,
 					 struct pci_bus *bus, int devfn)
 {
 	struct dev_archdata *sd;
-	struct pci_slot *slot;
 	struct platform_device *op;
 	struct pci_dev *dev;
 	const char *type;
@@ -290,10 +289,7 @@ static struct pci_dev *of_create_pci_dev(struct pci_pbm_info *pbm,
 	dev->multifunction = 0;		/* maybe a lie? */
 	set_pcie_port_type(dev);
 
-	list_for_each_entry(slot, &dev->bus->slots, list)
-		if (PCI_SLOT(dev->devfn) == slot->number)
-			dev->slot = slot;
-
+	pci_dev_assign_slot(dev);
 	dev->vendor = of_getintprop_default(node, "vendor-id", 0xffff);
 	dev->device = of_getintprop_default(node, "device-id", 0xffff);
 	dev->subsystem_vendor =
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..2a9ce16 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1133,7 +1133,6 @@ int pci_setup_device(struct pci_dev *dev)
 {
 	u32 class;
 	u8 hdr_type;
-	struct pci_slot *slot;
 	int pos = 0;
 	struct pci_bus_region region;
 	struct resource *res;
@@ -1149,10 +1148,7 @@ int pci_setup_device(struct pci_dev *dev)
 	dev->error_state = pci_channel_io_normal;
 	set_pcie_port_type(dev);
 
-	list_for_each_entry(slot, &dev->bus->slots, list)
-		if (PCI_SLOT(dev->devfn) == slot->number)
-			dev->slot = slot;
-
+	pci_dev_assign_slot(dev);
 	/* Assume 32-bit PCI; let 64-bit PCI cards (which are far rarer)
 	   set this higher, assuming the system even supports it.  */
 	dev->dma_mask = 0xffffffff;
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index a9079d9..b91d249 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -194,6 +194,17 @@ static int rename_slot(struct pci_slot *slot, const char *name)
 	return result;
 }
 
+void pci_dev_assign_slot(struct pci_dev *dev)
+{
+	struct pci_slot *slot;
+
+	mutex_lock(&pci_slot_mutex);
+	list_for_each_entry(slot, &dev->bus->slots, list)
+		if (PCI_SLOT(dev->devfn) == slot->number)
+			dev->slot = slot;
+	mutex_unlock(&pci_slot_mutex);
+}
+
 static struct pci_slot *get_slot(struct pci_bus *parent, int slot_nr)
 {
 	struct pci_slot *slot;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 34cc95d..0e81a6a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -798,6 +798,7 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 				 const char *name,
 				 struct hotplug_slot *hotplug);
 void pci_destroy_slot(struct pci_slot *slot);
+void pci_dev_assign_slot(struct pci_dev *dev);
 int pci_scan_slot(struct pci_bus *bus, int devfn);
 struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
-- 
1.7.1

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Guenter Roeck]

On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
> Yijing Wang (2):
>   PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>   PCI: Lock pci_slot_mutex when traverse bus->slots
> 
>  arch/powerpc/kernel/pci_of_scan.c |    6 +-----
>  arch/sparc/kernel/pci.c           |    6 +-----
>  drivers/pci/probe.c               |    6 +-----
>  drivers/pci/slot.c                |   26 +++++++++++++++++++++-----
>  include/linux/pci.h               |    4 +++-
>  5 files changed, 27 insertions(+), 21 deletions(-)
> 
I'll give the series a try next week.

Guenter

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Bjorn Helgaas]

On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
> Yijing Wang (2):
>   PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>   PCI: Lock pci_slot_mutex when traverse bus->slots

Applied to pci/hotplug for v4.3 with changelogs as follows, thanks!

commit b37795cdcc42286f78711311943af38a615ea17c
Author: Yijing Wang <wangyijing@huawei.com>
Date:   Fri Jul 17 17:16:31 2015 +0800

    PCI: Protect pci_bus->slots with pci_slot_mutex, not pci_bus_sem
    
    Rajat Jain reported a deadlock when PCIe hot-add and AER recovery happen at
    the same time:
    
    thread 1:
    
      pciehp_enable_slot
        pciehp_configure_device
          pci_bus_add_devices
            pci_bus_add_device
              device_attach
                device_lock(dev)                       # acquire device lock
                ...
                pciehp_probe
                  init_slot
                    pci_hp_register
                      pci_create_slot
                        down_write(pci_bus_sem)        # deadlock here
    
    thread 2:
    
      aer_isr_one_error
        aer_process_err_device
          do_recovery
            broadcast_error_message(..., report_error_detected)
              pci_walk_bus(..., cb=report_error_detected, ...)
                down_read(&pci_bus_sem)                # acquire pci_bus_sem
                report_error_detected(dev)             # cb()
                  device_lock(dev)                     # deadlock here
    
    Previously, the bus->devices and bus->slots list were protected by
    pci_bus_sem.  In pci_create_slot(), we held it for writing so we could
    add to the bus->slots list.
    
    Add a new local pci_slot_mutex to protect bus->slots.  Hold pci_bus_sem for
    reading while searching the bus->devices list.
    
    [bhelgaas: changelog]
    Link: http://lkml.kernel.org/r/CAA93t1qpPqbih+UB0McA_d_+2rVaNkXsinAUxYzK9+JXSS+L-g@mail.gmail.com
    Reported-by: Rajat Jain <rajatja@google.com>
    Tested-by: Guenter Roeck <linux@roeck-us.net>
    Signed-off-by: Yijing Wang <wangyijing@huawei.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

commit 4ea786b8f6734a706bc4c2f84c2a2bbc77845487
Author: Yijing Wang <wangyijing@huawei.com>
Date:   Fri Jul 17 17:16:32 2015 +0800

    PCI: Hold pci_slot_mutex while searching bus->slots list
    
    Previously, pci_setup_device() and similar functions searched the
    pci_bus->slots list without any locking.  It was possible for another
    thread to update the list while we searched it.
    
    Add pci_dev_assign_slot() to search the list while holding pci_slot_mutex.
    
    [bhelgaas: changelog]
    Tested-by: Guenter Roeck <linux@roeck-us.net>
    Signed-off-by: Yijing Wang <wangyijing@huawei.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Guenter Roeck]

On 07/30/2015 02:20 PM, Bjorn Helgaas wrote:
> On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
>> Yijing Wang (2):
>>    PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>>    PCI: Lock pci_slot_mutex when traverse bus->slots
>
> Applied to pci/hotplug for v4.3 with changelogs as follows, thanks!
>

Hi Bjorn,

I don't see pci/hotplug in
https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/.

Is that a different repository ?

Thanks,
Guenter

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Bjorn Helgaas]

On Thu, Jul 30, 2015 at 8:37 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 07/30/2015 02:20 PM, Bjorn Helgaas wrote:
>>
>> On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
>>>
>>> Yijing Wang (2):
>>>    PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>>>    PCI: Lock pci_slot_mutex when traverse bus->slots
>>
>>
>> Applied to pci/hotplug for v4.3 with changelogs as follows, thanks!
>>
>
> Hi Bjorn,
>
> I don't see pci/hotplug in
> https://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/.
>
> Is that a different repository ?

Sorry, I just hadn't pushed it because I have some more hotplug
patches and thought I'd push them all at once.  I pushed it now (and
will probably push some more updates soon).

Bjorn

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[Bjorn Helgaas]

On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
> Yijing Wang (2):
>   PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>   PCI: Lock pci_slot_mutex when traverse bus->slots
> 
>  arch/powerpc/kernel/pci_of_scan.c |    6 +-----
>  arch/sparc/kernel/pci.c           |    6 +-----
>  drivers/pci/probe.c               |    6 +-----
>  drivers/pci/slot.c                |   26 +++++++++++++++++++++-----
>  include/linux/pci.h               |    4 +++-
>  5 files changed, 27 insertions(+), 21 deletions(-)
> 

I squashed the following fix from Yijing into the second patch to fix a
build issue:

diff --git a/include/linux/pci.h b/include/linux/pci.h
index 214bf3f..b3ba7fe 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -798,7 +798,11 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 				 const char *name,
 				 struct hotplug_slot *hotplug);
 void pci_destroy_slot(struct pci_slot *slot);
+#ifdef CONFIG_SYSFS
 void pci_dev_assign_slot(struct pci_dev *dev);
+#else
+static inline void pci_dev_assign_slot(struct pci_dev *dev) { }
+#endif
 int pci_scan_slot(struct pci_bus *bus, int devfn);
 struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);

Re: [PATCH v2 0/2] Fix a deadlock for aer and pciehp driver

[wangyijing]

Thanks!

在 2015/8/11 3:29, Bjorn Helgaas 写道:
> On Fri, Jul 17, 2015 at 05:16:30PM +0800, Yijing Wang wrote:
>> Yijing Wang (2):
>>   PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
>>   PCI: Lock pci_slot_mutex when traverse bus->slots
>>
>>  arch/powerpc/kernel/pci_of_scan.c |    6 +-----
>>  arch/sparc/kernel/pci.c           |    6 +-----
>>  drivers/pci/probe.c               |    6 +-----
>>  drivers/pci/slot.c                |   26 +++++++++++++++++++++-----
>>  include/linux/pci.h               |    4 +++-
>>  5 files changed, 27 insertions(+), 21 deletions(-)
>>
> 
> I squashed the following fix from Yijing into the second patch to fix a
> build issue:
> 
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 214bf3f..b3ba7fe 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -798,7 +798,11 @@ struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
>  				 const char *name,
>  				 struct hotplug_slot *hotplug);
>  void pci_destroy_slot(struct pci_slot *slot);
> +#ifdef CONFIG_SYSFS
>  void pci_dev_assign_slot(struct pci_dev *dev);
> +#else
> +static inline void pci_dev_assign_slot(struct pci_dev *dev) { }
> +#endif
>  int pci_scan_slot(struct pci_bus *bus, int devfn);
>  struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
>  void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);
> 
>