Re: post 2.6.26 requires pciehp_slot_with_bus

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

Alex Chiang wrote:
> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>> Thank you for patches, Alex-san!
>>
>> I've reviewed those patches and tested them on my ia64 machine
>> that have both shpc and pcie hotplug slots. Your patch looks
>> good.
> 
> Thank you for reviewing and testing.
> 
>> As you mentioned, we are considering the problem also from the
>> view point of slot detection. But I think your patch is needed
>> regardless of that because there might be platforms whose slots
>> are detected properly but firmware assigns the physical slot
>> number wrongly. I think Alex's patch should go to mainline.
> 
> That is a good point.
> 
>> P.S.: I found a possible improvement, though it is not a big
>> problem and we don't not need to fix it soon. I'd like to tell
>> you about it just in case. Current pci_hp_register() checks if
>> name is duplicated first, before checking if another hotplug
>> driver is already registered to the slot. So, if shpchp/pciehp
>> driver tries to register hotplug slot that is already registered
>> by the other hotplug driver (e.g. acpiphp) with the same name,
>> shpchp/pciehp driver will do as follows:
>>
>> (1) shpchp/pciehp call pci_hp_register()
>> (2) pci_hp_register() returns -EEXIST
>> (3) shpchp/pciehp call pci_hp_register() with other name ("M-1")
>> (4) pci_hp_register() returns -EBUSY
>>
>> if pci_hp_register() checked if another hotplug driver is already
>> registered first, step (2) and (3) could be removed.
> 
> Thanks, that seems pretty easy to do.
> 
> Would you mind testing this patch as well? You should probably
> apply it on top of the other two patches to see how all three
> patches interact.
> 
> Thanks!
> 
> /ac
> 
> 
> From: Alex Chiang <achiang@dl580g5.kio>
> Subject: [PATCH] PCI hotplug: check for claimed slot before duplicate named slot
> 
> Kenji Kaneshige observes that:
> 
> If shpchp/pciehp driver tries to register hotplug slot that is
> already registered by the other hotplug driver (e.g. acpiphp) with
> the same name, shpchp/pciehp driver will do as follows:
> 
> (1) shpchp/pciehp call pci_hp_register()
> (2) pci_hp_register() returns -EEXIST
> (3) shpchp/pciehp call pci_hp_register() with other name ("M-1")
> (4) pci_hp_register() returns -EBUSY
> 
> If pci_hp_register() checked if another hotplug driver is already
> registered first, step (2) and (3) could be removed.
> 
> This patch does not prevent the *same* driver from attempting
> to register multiple slots with the same name (on systems with
> broken firmware). For that situation, we still need to detect
> a name collision and return -EEXIST if so.
> 
> Signed-off-by: Alex Chiang <achiang@hp.com>
> ---
>  drivers/pci/hotplug/pci_hotplug_core.c |   11 ++++++-----
>  1 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 5f85b1b..9c379b6 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -568,10 +568,6 @@ 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(slot->name))
> -		return -EEXIST;
> -
>  	/*
>  	 * No problems if we call this interface from both ACPI_PCI_SLOT
>  	 * driver and call it here again. If we've already created the
> @@ -587,6 +583,12 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
>  		return -EBUSY;
>  	}
>  
> +	/* Check if we have already registered a slot with the same name. */
> +	if (get_slot_from_name(slot->name)) {
> +		pci_destroy_slot(pci_slot);
> +		return -EEXIST;
> +	}
> +
>  	slot->pci_slot = pci_slot;
>  	pci_slot->hotplug = slot;
>  
> @@ -609,7 +611,6 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
>  	kobject_uevent(&pci_slot->kobj, KOBJ_ADD);
>  	dbg("Added slot %s to the list\n", slot->name);
>  
> -
>  	return result;
>  }
>  

Unfortunately, we can't simply move the following check after pci_create_slot().

> -	/* Check if we have already registered a slot with the same name. */
> -	if (get_slot_from_name(slot->name))
> -		return -EEXIST;
> -

With this change, kobject_init_and_add() called in pci_create_slot() will
show stack trace if a hotplug driver attempts to register multiple slot with
the same name. That is, stack trace will be shown on the platform that wrongly
assing the physical slot number to multiple slots. I'm very sorry, but I don't
have enough time to consider how to fix it today.

Thanks,
Kenji Kaneshige