Messages in this thread | | | Date | Thu, 09 Oct 2008 21:32:53 +0900 | From | Kenji Kaneshige <> | Subject | Re: [PATCH v5 04/16] PCI: prevent duplicate slot names |
| |
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.
> 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
| |