>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
