Re: [PATCH v4 03/15] PCI: prevent duplicate slot names

[Alex Chiang <achiang@hp.com>]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> 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