[PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock

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

Now we use pci_bus_sem to protect pci_slot creation and destroy,
it's unnecessary. We could introduce a new local mutex instead of
pci_bus_sem to avoid the deadlock.

Signed-off-by: Yijing Wang <wangyijing@huawei.com>
---
 drivers/pci/slot.c |   11 ++++++-----
 1 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 396c200..feb08de 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)
@@ -195,7 +196,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 +254,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;
@@ -310,7 +311,7 @@ placeholder:
 
 out:
 	kfree(slot_name);
-	up_write(&pci_bus_sem);
+	mutex_unlock(&pci_slot_mutex);
 	return slot;
 err:
 	kfree(slot);
@@ -332,9 +333,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);
 
-- 
1.7.1

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

[Yijing Wang]

rajatjain@juniper.net is not reachable now.

So add CC: rajatxjain@gmail.com

On 2015/6/11 19:12, 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
> 
> Now we use pci_bus_sem to protect pci_slot creation and destroy,
> it's unnecessary. We could introduce a new local mutex instead of
> pci_bus_sem to avoid the deadlock.
> 
> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/slot.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 396c200..feb08de 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)
> @@ -195,7 +196,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 +254,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;
> @@ -310,7 +311,7 @@ placeholder:
>  
>  out:
>  	kfree(slot_name);
> -	up_write(&pci_bus_sem);
> +	mutex_unlock(&pci_slot_mutex);
>  	return slot;
>  err:
>  	kfree(slot);
> @@ -332,9 +333,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);
>  
> 


-- 
Thanks!
Yijing

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

[Rajat Jain]

[+Guenter]

Looks good to me. Unfortunately, I do not have the hardware anymore to
test it :-(

Guenter: Do you have it, or know some body who has and wants to test this?

Thanks,

Rajat


On Fri, Jun 12, 2015 at 1:20 AM, Yijing Wang <wangyijing@huawei.com> wrote:
> rajatjain@juniper.net is not reachable now.
>
> So add CC: rajatxjain@gmail.com
>
> On 2015/6/11 19:12, 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
>>
>> Now we use pci_bus_sem to protect pci_slot creation and destroy,
>> it's unnecessary. We could introduce a new local mutex instead of
>> pci_bus_sem to avoid the deadlock.
>>
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/slot.c |   11 ++++++-----
>>  1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index 396c200..feb08de 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)
>> @@ -195,7 +196,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 +254,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;
>> @@ -310,7 +311,7 @@ placeholder:
>>
>>  out:
>>       kfree(slot_name);
>> -     up_write(&pci_bus_sem);
>> +     mutex_unlock(&pci_slot_mutex);
>>       return slot;
>>  err:
>>       kfree(slot);
>> @@ -332,9 +333,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);
>>
>>
>
>
> --
> Thanks!
> Yijing
>

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

[Rajat Jain]

Actually forgot to add Guenter. Also +rthirumal.

On Fri, Jun 12, 2015 at 11:13 AM, Rajat Jain <rajatxjain@gmail.com> wrote:
> [+Guenter]
>
> Looks good to me. Unfortunately, I do not have the hardware anymore to
> test it :-(
>
> Guenter: Do you have it, or know some body who has and wants to test/use this?
>
> Thanks,
>
> Rajat
>
>
> On Fri, Jun 12, 2015 at 1:20 AM, Yijing Wang <wangyijing@huawei.com> wrote:
>> rajatjain@juniper.net is not reachable now.
>>
>> So add CC: rajatxjain@gmail.com
>>
>> On 2015/6/11 19:12, 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
>>>
>>> Now we use pci_bus_sem to protect pci_slot creation and destroy,
>>> it's unnecessary. We could introduce a new local mutex instead of
>>> pci_bus_sem to avoid the deadlock.
>>>
>>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>>> ---
>>>  drivers/pci/slot.c |   11 ++++++-----
>>>  1 files changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>>> index 396c200..feb08de 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)
>>> @@ -195,7 +196,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 +254,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;
>>> @@ -310,7 +311,7 @@ placeholder:
>>>
>>>  out:
>>>       kfree(slot_name);
>>> -     up_write(&pci_bus_sem);
>>> +     mutex_unlock(&pci_slot_mutex);
>>>       return slot;
>>>  err:
>>>       kfree(slot);
>>> @@ -332,9 +333,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);
>>>
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>

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

[Guenter Roeck]

On Fri, Jun 12, 2015 at 11:12:35AM -0700, Rajat Jain wrote:
> [+Guenter]
> 
> Looks good to me. Unfortunately, I do not have the hardware anymore to test
> it :-(
> 
> Guenter: Do you have it, or know some body who has and wants to test this?
> 
I'll dig up the patch and test it.

Guenter

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

[Yijing Wang]

On 2015/6/13 2:20, Guenter Roeck wrote:
> On Fri, Jun 12, 2015 at 11:12:35AM -0700, Rajat Jain wrote:
>> [+Guenter]
>>
>> Looks good to me. Unfortunately, I do not have the hardware anymore to test
>> it :-(
>>
>> Guenter: Do you have it, or know some body who has and wants to test this?
>>
> I'll dig up the patch and test it.

Rajat, Guenter, thanks for your help.

Thanks!
Yijing.

> 
> Guenter
> 
> 


-- 
Thanks!
Yijing

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

[Yijing Wang]

On 2015/6/13 2:20, Guenter Roeck wrote:
> On Fri, Jun 12, 2015 at 11:12:35AM -0700, Rajat Jain wrote:
>> [+Guenter]
>>
>> Looks good to me. Unfortunately, I do not have the hardware anymore to test
>> it :-(
>>
>> Guenter: Do you have it, or know some body who has and wants to test this?
>>
> I'll dig up the patch and test it.

Hi Guenter, any update ?

> 
> Guenter
> 
> 


-- 
Thanks!
Yijing

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

[Guenter Roeck]

On Sat, Jun 27, 2015 at 11:05:36AM +0800, Yijing Wang wrote:
> On 2015/6/13 2:20, Guenter Roeck wrote:
> > On Fri, Jun 12, 2015 at 11:12:35AM -0700, Rajat Jain wrote:
> >> [+Guenter]
> >>
> >> Looks good to me. Unfortunately, I do not have the hardware anymore to test
> >> it :-(
> >>
> >> Guenter: Do you have it, or know some body who has and wants to test this?
> >>
> > I'll dig up the patch and test it.
> 
> Hi Guenter, any update ?
> 
I merged your patch into our images. I have not seen a single failure since
then.

At the same time, we tried to reproduce the problem Rajat had reported
originally. Unfortunately we have not been able to reproduce it. I don't know
if that is bad luck, if we did something wrong (most likely), or if something
else changed in the infrastructure since Rajat did his tests.

Either case, I would appreciate if this patch could find its way upstream.
I don't feel comfortable to enable AER without it. Feel free to add

Tested-by: Guenter Roeck <groeck@juniper.net>

Thanks,
Guenter

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

[Yijing Wang]

On 2015/6/27 11:19, Guenter Roeck wrote:
> On Sat, Jun 27, 2015 at 11:05:36AM +0800, Yijing Wang wrote:
>> On 2015/6/13 2:20, Guenter Roeck wrote:
>>> On Fri, Jun 12, 2015 at 11:12:35AM -0700, Rajat Jain wrote:
>>>> [+Guenter]
>>>>
>>>> Looks good to me. Unfortunately, I do not have the hardware anymore to test
>>>> it :-(
>>>>
>>>> Guenter: Do you have it, or know some body who has and wants to test this?
>>>>
>>> I'll dig up the patch and test it.
>>
>> Hi Guenter, any update ?
>>
> I merged your patch into our images. I have not seen a single failure since
> then.
> 
> At the same time, we tried to reproduce the problem Rajat had reported
> originally. Unfortunately we have not been able to reproduce it. I don't know
> if that is bad luck, if we did something wrong (most likely), or if something
> else changed in the infrastructure since Rajat did his tests.
> 
> Either case, I would appreciate if this patch could find its way upstream.
> I don't feel comfortable to enable AER without it. Feel free to add
> 
> Tested-by: Guenter Roeck <groeck@juniper.net>

OK, thanks for your help test.

Thanks!
Yijing.

> 
> Thanks,
> Guenter
> 
> 


-- 
Thanks!
Yijing

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

[Bjorn Helgaas]

[+cc Guenter, Rafael]

On Thu, Jun 11, 2015 at 07:12:14PM +0800, 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
> 
> Now we use pci_bus_sem to protect pci_slot creation and destroy,
> it's unnecessary. We could introduce a new local mutex instead of
> pci_bus_sem to avoid the deadlock.

I see there's definitely a problem here, and using a new mutex instead of
pci_bus_sem certainly avoids the deadlock.

I'm trying to convince myself that it is safe.  I think we need to protect:

  - search of bus->slots list in get_slot()
  - addition to bus->slots list in pci_create_slot()
  - search of bus->devices list in pci_create_slot()
  - search of bus->devices list in pci_slot_release()
  - deletion from bus->slots list in pci_slot_release()

Most other maintenance of these lists is protected by pci_bus_sem, so using
a different mutex here seems like a problem.

If I'm mistaken, please correct me and explain why this patch is safe.

> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
> ---
>  drivers/pci/slot.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 396c200..feb08de 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)
> @@ -195,7 +196,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 +254,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;
> @@ -310,7 +311,7 @@ placeholder:
>  
>  out:
>  	kfree(slot_name);
> -	up_write(&pci_bus_sem);
> +	mutex_unlock(&pci_slot_mutex);
>  	return slot;
>  err:
>  	kfree(slot);
> @@ -332,9 +333,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);
>  
> -- 
> 1.7.1
> 

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

[Yijing Wang]

On 2015/7/16 12:22, Bjorn Helgaas wrote:
> [+cc Guenter, Rafael]
> 
> On Thu, Jun 11, 2015 at 07:12:14PM +0800, 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
>>
>> Now we use pci_bus_sem to protect pci_slot creation and destroy,
>> it's unnecessary. We could introduce a new local mutex instead of
>> pci_bus_sem to avoid the deadlock.
> 
> I see there's definitely a problem here, and using a new mutex instead of
> pci_bus_sem certainly avoids the deadlock.
> 
> I'm trying to convince myself that it is safe.  I think we need to protect:
> 
>   - search of bus->slots list in get_slot()
>   - addition to bus->slots list in pci_create_slot()
>   - search of bus->devices list in pci_create_slot()
>   - search of bus->devices list in pci_slot_release()
>   - deletion from bus->slots list in pci_slot_release()
> 
> Most other maintenance of these lists is protected by pci_bus_sem, so using
> a different mutex here seems like a problem.
> 
> If I'm mistaken, please correct me and explain why this patch is safe.

Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
When I posted this patch, I thought we should protect the bus when we start to register a slot,
something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.

But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?


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


Thanks!
Yijing.




> 
>> Signed-off-by: Yijing Wang <wangyijing@huawei.com>
>> ---
>>  drivers/pci/slot.c |   11 ++++++-----
>>  1 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
>> index 396c200..feb08de 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)
>> @@ -195,7 +196,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 +254,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;
>> @@ -310,7 +311,7 @@ placeholder:
>>  
>>  out:
>>  	kfree(slot_name);
>> -	up_write(&pci_bus_sem);
>> +	mutex_unlock(&pci_slot_mutex);
>>  	return slot;
>>  err:
>>  	kfree(slot);
>> @@ -332,9 +333,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);
>>  
>> -- 
>> 1.7.1
>>
> 
> .
> 


-- 
Thanks!
Yijing

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

[Bjorn Helgaas]

On Thu, Jul 16, 2015 at 03:55:13PM +0800, Yijing Wang wrote:
> On 2015/7/16 12:22, Bjorn Helgaas wrote:
> > [+cc Guenter, Rafael]
> > 
> > On Thu, Jun 11, 2015 at 07:12:14PM +0800, 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
> >>
> >> Now we use pci_bus_sem to protect pci_slot creation and destroy,
> >> it's unnecessary. We could introduce a new local mutex instead of
> >> pci_bus_sem to avoid the deadlock.
> > 
> > I see there's definitely a problem here, and using a new mutex instead of
> > pci_bus_sem certainly avoids the deadlock.
> > 
> > I'm trying to convince myself that it is safe.  I think we need to protect:
> > 
> >   - search of bus->slots list in get_slot()
> >   - addition to bus->slots list in pci_create_slot()
> >   - search of bus->devices list in pci_create_slot()
> >   - search of bus->devices list in pci_slot_release()
> >   - deletion from bus->slots list in pci_slot_release()
> > 
> > Most other maintenance of these lists is protected by pci_bus_sem, so using
> > a different mutex here seems like a problem.
> > 
> > If I'm mistaken, please correct me and explain why this patch is safe.
> 
> Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
> use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
> When I posted this patch, I thought we should protect the bus when we start to register a slot,
> something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
> cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.
> 
> But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?
> 
> 
> 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);

This list_del() updates the bus->slots 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);

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

[Yijing Wang]

>>> If I'm mistaken, please correct me and explain why this patch is safe.
>>
>> Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
>> use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
>> When I posted this patch, I thought we should protect the bus when we start to register a slot,
>> something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
>> cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.
>>
>> But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?
>>
>>
>> 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);
> 
> This list_del() updates the bus->slots list.

It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().

Thanks!
Yijing.

> 
>> @@ -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);
> 
> .
> 


-- 
Thanks!
Yijing

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

[Bjorn Helgaas]

On Thu, Jul 16, 2015 at 8:14 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>> If I'm mistaken, please correct me and explain why this patch is safe.
>>>
>>> Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
>>> use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
>>> When I posted this patch, I thought we should protect the bus when we start to register a slot,
>>> something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
>>> cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.
>>>
>>> But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?
>>>
>>>
>>> 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);
>>
>> This list_del() updates the bus->slots list.
>
> It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().

That doesn't protect anybody else who might be traversing the
bus->slots list while we're deleting this entry.

>>> @@ -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);
>>
>> .
>>
>
>
> --
> Thanks!
> Yijing
>

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

[Yijing Wang]

On 2015/7/17 9:35, Bjorn Helgaas wrote:
> On Thu, Jul 16, 2015 at 8:14 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>>> If I'm mistaken, please correct me and explain why this patch is safe.
>>>>
>>>> Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
>>>> use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
>>>> When I posted this patch, I thought we should protect the bus when we start to register a slot,
>>>> something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
>>>> cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.
>>>>
>>>> But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?
>>>>
>>>>
>>>> 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);
>>>
>>> This list_del() updates the bus->slots list.
>>
>> It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().
> 
> That doesn't protect anybody else who might be traversing the
> bus->slots list while we're deleting this entry.

Hi Bjorn, sorry, I don't understand your point, before this patch, we use pci_bus_sem to protect the whole pci_slot_release(),
in which, we would traverse the bus->devices list and update the bus->slots list. And now what we did is introduce a new pci_slot_mutex
to protect the bus->slots here, and use down_read(pci_bus_sem) instead of down_write(pci_bus_sem).
Could you explain it a little more ?

Thanks!
Yijing.


> 
>>>> @@ -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);
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
> 
> .
> 


-- 
Thanks!
Yijing

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

[Bjorn Helgaas]

On Thu, Jul 16, 2015 at 8:54 PM, Yijing Wang <wangyijing@huawei.com> wrote:
> On 2015/7/17 9:35, Bjorn Helgaas wrote:
>> On Thu, Jul 16, 2015 at 8:14 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>>>> If I'm mistaken, please correct me and explain why this patch is safe.
>>>>>
>>>>> Hi Bjorn, I think pci_bus_sem here was introduced to protect the bus->slots list, because it
>>>>> use down_write() here, for bus->devices list, we only traverse it, won't add/remove it, for the latter, down_read() is enough.
>>>>> When I posted this patch, I thought we should protect the bus when we start to register a slot,
>>>>> something like a big lock at outermost routine to tell others not to touch its children devices, use pci_bus_sem to protect hotplug
>>>>> cases is not a good idea, and actually in PCI code, we have found several deadlock caused by the pci_bus_sem.
>>>>>
>>>>> But for this patch, I know what you worried, what about add a down_read(&pci_bus_sem) to avoid to introduce a regression ?
>>>>>
>>>>>
>>>>> 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);
>>>>
>>>> This list_del() updates the bus->slots list.
>>>
>>> It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().
>>
>> That doesn't protect anybody else who might be traversing the
>> bus->slots list while we're deleting this entry.
>
> Hi Bjorn, sorry, I don't understand your point, before this patch, we use pci_bus_sem to protect the whole pci_slot_release(),
> in which, we would traverse the bus->devices list and update the bus->slots list. And now what we did is introduce a new pci_slot_mutex
> to protect the bus->slots here, and use down_read(pci_bus_sem) instead of down_write(pci_bus_sem).

pci_setup_device() does this:

        list_for_each_entry(slot, &dev->bus->slots, list)
                if (PCI_SLOT(dev->devfn) == slot->number)
                        dev->slot = slot;

What keeps that code from running at the same time pci_slot_release()
is removing something from the bus->slots list?

It looks to me like the loop in pci_setup_device() is unsafe to begin
with.  But the obvious thing to do would be to add
down_read(&pci_bus_sem) there, and then you'd need a down_write() in
pci_slot_release(), so you're back where we started.

>>>>> @@ -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);
>>>>
>>>> .
>>>>
>>>
>>>
>>> --
>>> Thanks!
>>> Yijing
>>>
>>
>> .
>>
>
>
> --
> Thanks!
> Yijing
>

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

[Yijing Wang]

>>>>> This list_del() updates the bus->slots list.
>>>>
>>>> It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().
>>>
>>> That doesn't protect anybody else who might be traversing the
>>> bus->slots list while we're deleting this entry.
>>
>> Hi Bjorn, sorry, I don't understand your point, before this patch, we use pci_bus_sem to protect the whole pci_slot_release(),
>> in which, we would traverse the bus->devices list and update the bus->slots list. And now what we did is introduce a new pci_slot_mutex
>> to protect the bus->slots here, and use down_read(pci_bus_sem) instead of down_write(pci_bus_sem).
> 
> pci_setup_device() does this:
> 
>         list_for_each_entry(slot, &dev->bus->slots, list)
>                 if (PCI_SLOT(dev->devfn) == slot->number)
>                         dev->slot = slot;
> 
> What keeps that code from running at the same time pci_slot_release()
> is removing something from the bus->slots list?
> 
> It looks to me like the loop in pci_setup_device() is unsafe to begin
> with.  But the obvious thing to do would be to add
> down_read(&pci_bus_sem) there, and then you'd need a down_write() in
> pci_slot_release(), so you're back where we started.

I got it, I missed the bus->slots list traverse in pci scan code,
What about moving the bus->slots loop code from pci_setup_device() to drivers/pci/slot.c, and add a pci_slot_mutex to protect it ?
I think we should avoid to use pci_bus_sem to protect bus->slots list.

Something like this:
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index cefd636..6f00273 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1149,10 +1149,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..cf259e7 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -99,6 +99,15 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
        return bus_speed_read(slot->bus->cur_bus_speed, buf);
 }

+void pci_dev_assign_slot(struct pci_dev *dev)
+{
+       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 void pci_slot_release(struct kobject *kobj)
 {
        struct pci_dev *dev;


> 
>>>>>> @@ -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);
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks!
>>>> Yijing
>>>>
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
> 
> .
> 


-- 
Thanks!
Yijing

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

[Bjorn Helgaas]

On Thu, Jul 16, 2015 at 9:24 PM, Yijing Wang <wangyijing@huawei.com> wrote:
>>>>>> This list_del() updates the bus->slots list.
>>>>>
>>>>> It's safe here, because we have locked the pci_slot_mutex in pci_destroy_slot(), which is the only caller of pci_slot_release().
>>>>
>>>> That doesn't protect anybody else who might be traversing the
>>>> bus->slots list while we're deleting this entry.
>>>
>>> Hi Bjorn, sorry, I don't understand your point, before this patch, we use pci_bus_sem to protect the whole pci_slot_release(),
>>> in which, we would traverse the bus->devices list and update the bus->slots list. And now what we did is introduce a new pci_slot_mutex
>>> to protect the bus->slots here, and use down_read(pci_bus_sem) instead of down_write(pci_bus_sem).
>>
>> pci_setup_device() does this:
>>
>>         list_for_each_entry(slot, &dev->bus->slots, list)
>>                 if (PCI_SLOT(dev->devfn) == slot->number)
>>                         dev->slot = slot;
>>
>> What keeps that code from running at the same time pci_slot_release()
>> is removing something from the bus->slots list?
>>
>> It looks to me like the loop in pci_setup_device() is unsafe to begin
>> with.  But the obvious thing to do would be to add
>> down_read(&pci_bus_sem) there, and then you'd need a down_write() in
>> pci_slot_release(), so you're back where we started.
>
> I got it, I missed the bus->slots list traverse in pci scan code,
> What about moving the bus->slots loop code from pci_setup_device() to drivers/pci/slot.c, and add a pci_slot_mutex to protect it ?
> I think we should avoid to use pci_bus_sem to protect bus->slots list.

Seems better, although it's starting to feel a bit ad hoc.  We would
need to write down this locking rule somewhere, e.g., near the struct
pci_bus declaration.

> Something like this:
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index cefd636..6f00273 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1149,10 +1149,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..cf259e7 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -99,6 +99,15 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
>         return bus_speed_read(slot->bus->cur_bus_speed, buf);
>  }
>
> +void pci_dev_assign_slot(struct pci_dev *dev)
> +{
> +       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 void pci_slot_release(struct kobject *kobj)
>  {
>         struct pci_dev *dev;
>
>
>>
>>>>>>> @@ -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);
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks!
>>>>> Yijing
>>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>> --
>>> Thanks!
>>> Yijing
>>>
>>
>> .
>>
>
>
> --
> Thanks!
> Yijing
>

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

[Yijing Wang]

>>> It looks to me like the loop in pci_setup_device() is unsafe to begin
>>> with.  But the obvious thing to do would be to add
>>> down_read(&pci_bus_sem) there, and then you'd need a down_write() in
>>> pci_slot_release(), so you're back where we started.
>>
>> I got it, I missed the bus->slots list traverse in pci scan code,
>> What about moving the bus->slots loop code from pci_setup_device() to drivers/pci/slot.c, and add a pci_slot_mutex to protect it ?
>> I think we should avoid to use pci_bus_sem to protect bus->slots list.
> 
> Seems better, although it's starting to feel a bit ad hoc.  We would
> need to write down this locking rule somewhere, e.g., near the struct
> pci_bus declaration.

OK, I will post a updated patch.

Thanks!
Yijing.

> 
>> Something like this:
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index cefd636..6f00273 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -1149,10 +1149,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..cf259e7 100644
>> --- a/drivers/pci/slot.c
>> +++ b/drivers/pci/slot.c
>> @@ -99,6 +99,15 @@ static ssize_t cur_speed_read_file(struct pci_slot *slot, char *buf)
>>         return bus_speed_read(slot->bus->cur_bus_speed, buf);
>>  }
>>
>> +void pci_dev_assign_slot(struct pci_dev *dev)
>> +{
>> +       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 void pci_slot_release(struct kobject *kobj)
>>  {
>>         struct pci_dev *dev;
>>
>>
>>>
>>>>>>>> @@ -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);
>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> Thanks!
>>>>>> Yijing
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>> --
>>>> Thanks!
>>>> Yijing
>>>>
>>>
>>> .
>>>
>>
>>
>> --
>> Thanks!
>> Yijing
>>
> 
> .
> 


-- 
Thanks!
Yijing