Alex Chiang wrote:
* 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.
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
--
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
