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.
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;
...
}
- Introduce new API to setup pci_slot->hotplug and rename. This would be
as follows:
int pci_slot_hp_register(struct pci_slot *pci_slot,
struct hotplug_slot *hotplug_slot, const char *name)
{
...
if (pci_slot->hotplug) {
Some cleanups;
return -EBUSY;
}
if ((err = rename_slot(slot, name))
Some cleanups;
return err;
}
pci_slot->hotplug = hotplug;
...
}
It is intended to be used by pci_hp_register() as follows:
int pci_hp_register(...)
{
...
pci_slot = pci_create_slot(bus, slot_nr, name);
if ((result = IS_ERR(pci_slot)))
goto out;
if ((err = pci_slot_hp_register(pci_slot, hotplug, name)))
goto out;
...
}
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
