Messages in this thread | | | Date | Fri, 10 Oct 2008 13:43:42 +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>: >> 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
| |