* Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> Alex Chiang wrote:
>
>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
>> index e00266b..b196ea1 100644
>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>> @@ -572,41 +572,32 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>> return -EINVAL;
>> }
>> - /* Check if we have already registered a slot with the same name. */
>> - if (get_slot_from_name(name))
>> - return -EEXIST;
>> -
>> /*
>> - * No problems if we call this interface from both ACPI_PCI_SLOT
>> - * driver and call it here again. If we've already created the
>> - * pci_slot, the interface will simply bump the refcount.
>> + * Look for existing slot. If we find it, and it was created by a
>> + * slot detection driver (ie, doesn't have a ->hotplug()) then we
>> + * allow the hotplug driver calling us to rename the slot if desired.
>> + *
>> + * Otherwise, create the slot and carry on with life.
>> */
>> - pci_slot = pci_create_slot(bus, slot_nr, name);
>> - if (IS_ERR(pci_slot))
>> - return PTR_ERR(pci_slot);
>> -
>> mutex_lock(&pci_hp_mutex);
>> - if (pci_slot->hotplug) {
>> - mutex_unlock(&pci_hp_mutex);
>> - dbg("%s: already claimed\n", __func__);
>> - pci_destroy_slot(pci_slot);
>> - return -EBUSY;
>> + pci_slot = pci_get_pci_slot(bus, slot_nr);
>> + if (pci_slot) {
>> + if (pci_slot->hotplug) {
>> + result = -EBUSY;
>> + goto err;
>> + }
>> +
>> + if (strcmp(kobject_name(&pci_slot->kobj), name))
>> + if ((result = pci_rename_slot(pci_slot, name)))
>> + goto err;
>> + } else {
>> + pci_slot = pci_create_slot(bus, slot_nr, name);
>> + if ((result = IS_ERR(pci_slot)))
>> + goto out;
>> }
>>
>
> I think there still remain race condition. The situation I imagine
> is as follows, though I might be misunderstanding something.
>
> If pci_slot driver created pci_slot between pci_get_pci_slot() and
> pci_create_slot() in pci_hp_register() in the hotplug driver's thread,
> pci_rename_slot() would never be called and name specified by pci_slot
> driver would be used instead of the one specified by hotplug driver.
Ah, now I finally understand.
And I think the solution is to avoid all the crap code I've been
introducing into pci_hp_register.
A better way to do it would be to make slot creation and name
override into one atomic operation, inside of pci_create_slot.
We are already serializing pci_create_slot by acquiring
pci_bus_sem, so that should prevent any race condition.
I wrote a long changelog entry for my patches that should explain
it all.
Please review. :)
>> -void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
>> +void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
>> {
>> - int name_count = 0;
>> struct pci_slot *tmp;
>> down_write(&pci_bus_sem);
>> list_for_each_entry(tmp, &slot->bus->slots, list) {
>> WARN_ON(tmp->number == slot_nr);
>> - if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
>> - name_count++;
>> + goto out;
>> }
>> - if (name_count > 1)
>> - printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
>> -
>> slot->number = slot_nr;
>> +out:
>> up_write(&pci_bus_sem);
>> }
>> -EXPORT_SYMBOL_GPL(pci_update_slot_number);
>> +EXPORT_SYMBOL_GPL(pci_renumber_slot);
>>
>
> You changed exported symbol name. I think it should be made as
> an another patch.
Ok.
Thanks.
/ac
--
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
