Re: [PATCH v2 02/13] PCI: prevent duplicate slot names

Kenji Kaneshige
2008-09-11T02:49:04Z

Alex Chiang wrote:
> Prevent callers of pci_create_slot() from registering slots with
> duplicate names. This condition occurs most often when PCI hotplug
> drivers are loaded on platforms with broken firmware that assigns
> identical names to multiple slots.
>
> We now rename these duplicate slots on behalf of the user.
>
> If firmware assigns the name N to multiple slots, then:
>
> The first registered slot is assigned N
> The second registered slot is assigned N-1
> The third registered slot is assigned N-2
> The Mth registered slot becomes N-M
>
> A side effect of this patch is that the error condition for when
> multiple drivers attempt to claim the same slot becomes much more
> prominent.
>
> In other words, the previous error condition returned for
> duplicate slot names (-EEXIST) masked the case when multiple
> drivers attempted to claim the same slot. Now, the -EBUSY return
> makes the true error more obvious.
>
> This is the permanent fix mentioned in earlier commits:
>
> shpchp: Rename duplicate slot name N as N-1, N-2, N-M...
> d6a9e9b40be7da84f82eb414c2ad98c5bb69986b
>
> pciehp: Rename duplicate slot name N as N-1, N-2, N-M...
> 167e782e301188c7c7e31e486bbeea5f918324c1
>
> Cc: jbarnes@virtuousgeek.org
> Cc: kristen.c.accardi@intel.com
> Cc: matthew@wil.cx
> Cc: kaneshige.kenji@jp.fujitsu.com
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
>
> drivers/pci/hotplug/pci_hotplug_core.c | 23 ++++--
> drivers/pci/hotplug/pciehp_core.c | 14 ----
> drivers/pci/hotplug/shpchp_core.c | 15 ----
> drivers/pci/slot.c | 117 +++++++++++++++++++++++++++-----
> include/linux/pci.h | 3 +
> 5 files changed, 117 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 3e37d63..2232608 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -558,7 +558,8 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> const char *name)
> {
> int result;
> - struct pci_slot *pci_slot;
> + struct pci_dev *dev;
> + struct pci_slot *pci_slot, *tmp_slot = NULL;
>
> if (slot == NULL)
> return -ENODEV;
> @@ -570,9 +571,17 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> return -EINVAL;
> }
>
> - /* Check if we have already registered a slot with the same name. */
> - if (get_slot_from_name(name))
> - return -EEXIST;
> + /*
> + * If we find a tmp_slot here, it means that another slot
> + * driver has already created a pci_slot for this device.
> + * We care (below) if the existing slot has a different name from
> + * the new name that this particular hotplug driver is requesting.
> + */
> + dev = pci_get_slot(bus, PCI_DEVFN(slot_nr, 0));
> + if (dev && dev->slot) {
> + tmp_slot = dev->slot;
> + pci_dev_put(dev);
> + }
>

I have two comments here.

(1) I think the reference counter of the device will be leaked if
(dev == NULL) && (dev->slot != NULL). We need pci_dev_put() whenever
dev is not NULL.

(2) When the slot is empty, the 'dev' will be always NULL. Therefore,
'tmp_slot' will be always NULL on the empty slot here. Because of
this, the following code to rename the slot will not work on the
empty slot, I think.

> /*
> - * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> + * Allow pcihp drivers to override existing slot name.
> */
> - if (strcmp(kobject_name(&pci_slot->kobj), name)) {
> - result = kobject_rename(&pci_slot->kobj, name);
> + if (tmp_slot && strcmp(kobject_name(&tmp_slot->kobj), name)) {
> + result = pci_rename_slot(pci_slot, name);
> if (result) {
> pci_destroy_slot(pci_slot);
> return result;

Thanks,
Kenji Kaneshige