* [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
@ 2015-06-11 11:12 Yijing Wang
2015-06-12 8:20 ` Yijing Wang
2015-07-16 4:22 ` Bjorn Helgaas
0 siblings, 2 replies; 19+ messages in thread
From: Yijing Wang @ 2015-06-11 11:12 UTC (permalink / raw)
To: bhelgaas; +Cc: linux-pci, rajatjain, 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-11 11:12 [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock Yijing Wang
@ 2015-06-12 8:20 ` Yijing Wang
2015-06-12 18:13 ` Rajat Jain
[not found] ` <CAA93t1ooSY2keDigmUPpO7LzvT12YwQjpxH0b1xA508LL+VWdg@mail.gmail.com>
2015-07-16 4:22 ` Bjorn Helgaas
1 sibling, 2 replies; 19+ messages in thread
From: Yijing Wang @ 2015-06-12 8:20 UTC (permalink / raw)
To: Yijing Wang, bhelgaas; +Cc: linux-pci, Rajat Jain
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-12 8:20 ` Yijing Wang
@ 2015-06-12 18:13 ` Rajat Jain
2015-06-12 18:19 ` Rajat Jain
[not found] ` <CAA93t1ooSY2keDigmUPpO7LzvT12YwQjpxH0b1xA508LL+VWdg@mail.gmail.com>
1 sibling, 1 reply; 19+ messages in thread
From: Rajat Jain @ 2015-06-12 18:13 UTC (permalink / raw)
To: Yijing Wang; +Cc: Bjorn Helgaas, linux-pci, 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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-12 18:13 ` Rajat Jain
@ 2015-06-12 18:19 ` Rajat Jain
0 siblings, 0 replies; 19+ messages in thread
From: Rajat Jain @ 2015-06-12 18:19 UTC (permalink / raw)
To: Rajat Jain
Cc: Yijing Wang, Bjorn Helgaas, linux-pci, guenter.roeck,
Raghuraman Thirumalairajan
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
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
[not found] ` <CAA93t1ooSY2keDigmUPpO7LzvT12YwQjpxH0b1xA508LL+VWdg@mail.gmail.com>
@ 2015-06-12 18:20 ` Guenter Roeck
2015-06-15 0:40 ` Yijing Wang
2015-06-27 3:05 ` Yijing Wang
0 siblings, 2 replies; 19+ messages in thread
From: Guenter Roeck @ 2015-06-12 18:20 UTC (permalink / raw)
To: Rajat Jain; +Cc: Yijing Wang, Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-12 18:20 ` Guenter Roeck
@ 2015-06-15 0:40 ` Yijing Wang
2015-06-27 3:05 ` Yijing Wang
1 sibling, 0 replies; 19+ messages in thread
From: Yijing Wang @ 2015-06-15 0:40 UTC (permalink / raw)
To: Guenter Roeck, Rajat Jain; +Cc: Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-12 18:20 ` Guenter Roeck
2015-06-15 0:40 ` Yijing Wang
@ 2015-06-27 3:05 ` Yijing Wang
2015-06-27 3:19 ` Guenter Roeck
1 sibling, 1 reply; 19+ messages in thread
From: Yijing Wang @ 2015-06-27 3:05 UTC (permalink / raw)
To: Guenter Roeck, Rajat Jain; +Cc: Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-27 3:05 ` Yijing Wang
@ 2015-06-27 3:19 ` Guenter Roeck
2015-06-27 3:37 ` Yijing Wang
0 siblings, 1 reply; 19+ messages in thread
From: Guenter Roeck @ 2015-06-27 3:19 UTC (permalink / raw)
To: Yijing Wang; +Cc: Rajat Jain, Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-27 3:19 ` Guenter Roeck
@ 2015-06-27 3:37 ` Yijing Wang
0 siblings, 0 replies; 19+ messages in thread
From: Yijing Wang @ 2015-06-27 3:37 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Rajat Jain, Bjorn Helgaas, linux-pci
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-06-11 11:12 [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock Yijing Wang
2015-06-12 8:20 ` Yijing Wang
@ 2015-07-16 4:22 ` Bjorn Helgaas
2015-07-16 7:55 ` Yijing Wang
1 sibling, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2015-07-16 4:22 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
[+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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-16 4:22 ` Bjorn Helgaas
@ 2015-07-16 7:55 ` Yijing Wang
2015-07-16 15:25 ` Bjorn Helgaas
0 siblings, 1 reply; 19+ messages in thread
From: Yijing Wang @ 2015-07-16 7:55 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-16 7:55 ` Yijing Wang
@ 2015-07-16 15:25 ` Bjorn Helgaas
2015-07-17 1:14 ` Yijing Wang
0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2015-07-16 15:25 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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);
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-16 15:25 ` Bjorn Helgaas
@ 2015-07-17 1:14 ` Yijing Wang
2015-07-17 1:35 ` Bjorn Helgaas
0 siblings, 1 reply; 19+ messages in thread
From: Yijing Wang @ 2015-07-17 1:14 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
>>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 1:14 ` Yijing Wang
@ 2015-07-17 1:35 ` Bjorn Helgaas
2015-07-17 1:54 ` Yijing Wang
0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2015-07-17 1:35 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 1:35 ` Bjorn Helgaas
@ 2015-07-17 1:54 ` Yijing Wang
2015-07-17 2:05 ` Bjorn Helgaas
0 siblings, 1 reply; 19+ messages in thread
From: Yijing Wang @ 2015-07-17 1:54 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 1:54 ` Yijing Wang
@ 2015-07-17 2:05 ` Bjorn Helgaas
2015-07-17 2:24 ` Yijing Wang
0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2015-07-17 2:05 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 2:05 ` Bjorn Helgaas
@ 2015-07-17 2:24 ` Yijing Wang
2015-07-17 2:46 ` Bjorn Helgaas
0 siblings, 1 reply; 19+ messages in thread
From: Yijing Wang @ 2015-07-17 2:24 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
>>>>> 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
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 2:24 ` Yijing Wang
@ 2015-07-17 2:46 ` Bjorn Helgaas
2015-07-17 2:52 ` Yijing Wang
0 siblings, 1 reply; 19+ messages in thread
From: Bjorn Helgaas @ 2015-07-17 2:46 UTC (permalink / raw)
To: Yijing Wang; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
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
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock
2015-07-17 2:46 ` Bjorn Helgaas
@ 2015-07-17 2:52 ` Yijing Wang
0 siblings, 0 replies; 19+ messages in thread
From: Yijing Wang @ 2015-07-17 2:52 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Rajat Jain, Guenter Roeck, Rafael J. Wysocki
>>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-07-17 3:08 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-11 11:12 [PATCH] PCI: Use a local mutex instead of pci_bus_sem to avoid deadlock Yijing Wang
2015-06-12 8:20 ` Yijing Wang
2015-06-12 18:13 ` Rajat Jain
2015-06-12 18:19 ` Rajat Jain
[not found] ` <CAA93t1ooSY2keDigmUPpO7LzvT12YwQjpxH0b1xA508LL+VWdg@mail.gmail.com>
2015-06-12 18:20 ` Guenter Roeck
2015-06-15 0:40 ` Yijing Wang
2015-06-27 3:05 ` Yijing Wang
2015-06-27 3:19 ` Guenter Roeck
2015-06-27 3:37 ` Yijing Wang
2015-07-16 4:22 ` Bjorn Helgaas
2015-07-16 7:55 ` Yijing Wang
2015-07-16 15:25 ` Bjorn Helgaas
2015-07-17 1:14 ` Yijing Wang
2015-07-17 1:35 ` Bjorn Helgaas
2015-07-17 1:54 ` Yijing Wang
2015-07-17 2:05 ` Bjorn Helgaas
2015-07-17 2:24 ` Yijing Wang
2015-07-17 2:46 ` Bjorn Helgaas
2015-07-17 2:52 ` Yijing Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).