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

[Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>]

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