Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

Alex Chiang wrote:
> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>> Alex Chiang wrote:
>>> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>>>> Thank your new patches. Very quick!!!
>>> I'm trying to get into 2.6.28. ;)
>>>
>>>> Though I have not reviewed/tested your patches yet (of course), I have
>>>> one concern as I said in the e-mail soon before. Does the new one
>>>> consider the following senario?
>>>>
>>>> 	Scenario C:
>>>> 	hotplug driver(A)               	hotplug_driver(B)
>>>> 	--------------                  	----------------
>>>>        pci_create_slot(name=A, rename=1)
>>>> 						pci_create_slot(name=B, rename=1)
>>>>
>>>> The hotplug driver (A) creates the slot with name "A". The the hotplug
>>>> driver (B) tries to create the same slot, but wants the name "B" instead.
>>>> In this case, hotplug driver fails to create the slot and the slot name
>>>> should not be changed to "B" from "A".
>>> Hm... I don't think this is a common scenario but...
>>>
>> It was a common scenario until recently because acpiphp and
>> native hotplug drivers(pciehp, shpchp) had different naming
>> rules. That is, acpiphp used _SUN number, while pciehp/shpchp
>> used bus number and physical slot number pair. Although the
>> pciehp/shpchp driver has been changed not to use bus_number
>> for slot names and acpiphp and pciehp/shpchp has the same
>> names on my system now, but I think the scenario is still
>> possible because naming rule of each hotplug driver could be
>> changed in the future again.
>>
>> By the way, acpiphp was changed to handle 64bit _SUN number
>> recently for new ia64 HP servers, IIRC. Are hotplug slots
>> on that server can also be handled through PCIe controller?
>> If it is yes, I think _SUN doesn't match physical slot number
>> because physical slot number (in Slot Capabilities Register)
>> has only 13bit. In this case, the above scenario will happen.
> 
> Hm, ok, I agree.
> 
>>> int pci_hp_register(...)
>>> {
>>> 	...
>>>
>>>         pci_slot = pci_create_slot(bus, slot_nr, name, 1);
>>>         if (IS_ERR(pci_slot))                    return 
>>> PTR_ERR(pci_slot);
>>>
>>>         if (pci_slot->hotplug) {
>>>                 dbg("%s: already claimed\n", __func__);
>>>                 pci_destroy_slot(pci_slot);
>>>                 return -EBUSY;
>>>         }
>>> 	...
>>> }
>>>
>>> I could maybe move that check into pci_create_slot() instead.
>>>
>>> struct pci_slot *pci_create_slot(...)
>>> {
>>> 	...
>>>
>>>         /*
>>>          * Get existing slot and rename if desired
>>>          */
>>>         slot = get_slot(parent, slot_nr);
>>>         if (slot && rename) {
>>> 		if ((err = slot->hotplug ? -EBUSY : 0)
>>> 		     || (err = rename_slot(slot, name))) {
>>>                         kobject_put(&slot->kobj);
>>>                         slot = NULL;
>>>                         goto err;
>>>                 } else
>>>                         goto out;
>>>         } else if (slot)
>>>                 goto out;
>>> 	...
>>> }
>>>
>>> Seems a little ugly to me, but maybe it's necessary?
>>>
>> I don't like this, and I think it's wrong because callers
>> might get -EBUSY even though they are not related to hotplug.
>>
>> I thought of the following alternative ideas, when I was making
>> sample patches. What do you think about those? My was concerned
>> that both need to add hotplug related code into generic pci slot
>> management code/API.
>>
>> - Add 'hotplug' arg to pci_create_slot(), instead of 'rename'
>>  flag. The pci_create_slot() would be as follows:
>>
>> 	struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
>> 	{
>> 		...
>> 		/*
>> 		 * Get existing slot and rename if desired
>> 		 */
>> 		slot = get_slot(parent, slot_nr);
>> 		if (slot) {
>> 			if (hotplug) {
>> 				if ((err = slot->hotplug ? -EBUSY : 0)
>> 				     || err = rename_slot(slot, name))) {
>> 					Some cleanups;
>> 					return err;
>> 				}
>> 			}
>> 			goto out;
>> 		}
>> 		...
>> 	out:
>> 		if (hotplug)
>> 			slot->hotplug = hotplug;
>> 		...
>> 	}
> 
> I like this approach a little better, since the flow of execution
> is easier to understand (vs. pci_create_slot + pci_slot_hp_register).
> 
> I prototyped it, but didn't get a chance to test it (I did
> compile it though).
> 
> I'll send 2 test patches shortly that should replace the earlier
> 03/16 and 04/16 patches.
> 

I'm sorry, but I forgot to tell you one important thing. Now we are
trying to change pci slot management API to setup pci_slot->hotplug.
We must consider how to implement the counterpart to clean up
pci_slot->hotplug at the same time. My current idea is adding hotplug
arg to pci_destroy_slot(), but it seems a little ugly...

Thanks,
Kenji Kaneshige