lkml.org 
[lkml]   [2008]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 04/16] PCI: prevent duplicate slot names
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




\
 
 \ /
  Last update: 2008-10-09 14:37    [W:0.109 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site