Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

[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.

> 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