* Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
> Alex Chiang wrote:
>> * Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx>:
>>> 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.
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
