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