PCI: let the core manage slot names
Subsystems (5)
v3 cover PCI: let the core manage slot names
Hi Jesse, Kenji, et al,
This is v3 of the series that implements a series of changes that allows the PCI core to manage slot names, rather than individual hotplug drivers.
v2 -> v3:
- incorporate Willy's code review comments
- fix possible memory leak, pointed out by Rolf Eike Beer
- make false name collision detection work for empty slots
- add 'dup_slots' module_param to fakephp to help debug all this ;)
v1 -> v2:
- fix possible false name collisions
This series applies against linux-next. I've tested this by loading pci_slot and various hotplug drivers to ensure that the hotplug drivers can successfully override the pci_slot names.
I'm including the previous explanation/justification to remind folks of the raison d'etre for this patchset.
Thanks.
/ac
There are several benefits to this approach:
1) The core can prevent duplicate slot names on systems with broken firmware.
2) Since the kobject core keeps a copy of the slot name, there is no need for each driver to manage a separate copy, especially since the core can rename slots from underneath drivers. We save runtime memory by only referencing the kobject name.
2.a) The PCI hotplug core doesn't need its own reference for slot name either.
3) Individual hotplug drivers become just a little bit simpler by pushing as many kzalloc() calls for 'name' down into the PCI core as possible.
---
Alex Chiang (14):
PCI Hotplug: fakephp: add duplicate slot name debugging
PCI: Hotplug core: remove 'name'
PCI: shcphp: remove 'name' parameter
PCI: SGI Hotplug: stop managing bss_hotplug_slot->name
PCI: rpaphp: stop managing hotplug_slot->name
PCI: pciehp: remove 'name' parameter
PCI: ibmphp: stop managing hotplug_slot->name
PCI: fakephp: remove 'name' parameter
PCI: cpqphp: stop managing hotplug_slot->name
PCI: cpci_hotplug: stop managing hotplug_slot->name
PCI: acpiphp: remove 'name' parameter
PCI, PCI Hotplug: introduce slot_name helpers
PCI: prevent duplicate slot names
PCI Hotplug core: add 'name' param pci_hp_register interface
drivers/pci/hotplug/acpiphp.h | 9 +- drivers/pci/hotplug/acpiphp_core.c | 32 +++--- drivers/pci/hotplug/cpci_hotplug.h | 6 + drivers/pci/hotplug/cpci_hotplug_core.c | 74 +++++--------- drivers/pci/hotplug/cpci_hotplug_pci.c | 4 - drivers/pci/hotplug/cpqphp.h | 13 +- drivers/pci/hotplug/cpqphp_core.c | 42 ++++---- drivers/pci/hotplug/fakephp.c | 25 +++-- drivers/pci/hotplug/ibmphp.h | 5 - drivers/pci/hotplug/ibmphp_ebda.c | 19 +--- drivers/pci/hotplug/pci_hotplug_core.c | 62 ++++++------ drivers/pci/hotplug/pciehp.h | 9 +- drivers/pci/hotplug/pciehp_core.c | 41 +++----- drivers/pci/hotplug/pciehp_ctrl.c | 53 +++++----- drivers/pci/hotplug/pciehp_hpc.c | 1 drivers/pci/hotplug/rpaphp.h | 10 +- drivers/pci/hotplug/rpaphp_core.c | 6 + drivers/pci/hotplug/rpaphp_pci.c | 4 - drivers/pci/hotplug/rpaphp_slot.c | 26 ++--- drivers/pci/hotplug/sgi_hotplug.c | 18 +-- drivers/pci/hotplug/shpchp.h | 9 +- drivers/pci/hotplug/shpchp_core.c | 42 +++----- drivers/pci/hotplug/shpchp_ctrl.c | 48 ++++----- drivers/pci/slot.c | 167 +++++++++++++++++++++++-------- include/linux/pci.h | 9 +- include/linux/pci_hotplug.h | 13 ++ 26 files changed, 401 insertions(+), 346 deletions(-)
PATCH v3 1/14 PCI Hotplug core: add 'name' param pci_hp_register interface
Update pci_hp_register() to take a const char *name parameter.
The motivation for this is to clean up the individual hotplug drivers so that each one does not have to manage its own name. The PCI core should be the place where we manage the name.
We update the interface and all callsites first, in a "no functional change" manner, and clean up the drivers later.
---
drivers/pci/hotplug/acpiphp_core.c | 3 ++- drivers/pci/hotplug/cpci_hotplug_core.c | 3 ++- drivers/pci/hotplug/cpqphp_core.c | 3 ++- drivers/pci/hotplug/fakephp.c | 3 ++- drivers/pci/hotplug/ibmphp_ebda.c | 3 ++- drivers/pci/hotplug/pci_hotplug_core.c | 15 ++++++++------- drivers/pci/hotplug/pciehp_core.c | 3 ++- drivers/pci/hotplug/rpaphp_slot.c | 2 +- drivers/pci/hotplug/sgi_hotplug.c | 3 ++- drivers/pci/hotplug/shpchp_core.c | 3 ++- include/linux/pci_hotplug.h | 5 ++++- 11 files changed, 29 insertions(+), 17 deletions(-)
drivers/pci/hotplug/acpiphp_core.c +2 -1
@@ -340,7 +340,8 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) retval =pci_hp_register (slot->hotplug_slot, acpiphp_slot->bridge->pci_bus, - acpiphp_slot->device); + acpiphp_slot->device, + slot->name);if (retval == -EBUSY ) goto error_hpslot;if (retval) {
drivers/pci/hotplug/cpci_hotplug_core.c +2 -1
@@ -285,7 +285,8 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last) info->attention_status =cpci_get_attention_status (slot);dbg ("registering slot %s" , slot->hotplug_slot->name); - status =pci_hp_register (slot->hotplug_slot, bus, i); + status =pci_hp_register (slot->hotplug_slot, bus, i, + slot->hotplug_slot->name);if (status) {err ("pci_hp_register failed with error %d" , status); goto error_name;
drivers/pci/hotplug/cpqphp_core.c +2 -1
@@ -436,7 +436,8 @@ static int ctrl_slot_setup(struct controller *ctrl, slot_number); result =pci_hp_register (hotplug_slot, ctrl->pci_dev->subordinate, - slot->device); + slot->device, + hotplug_slot->name);if (result) {err ("pci_hp_register failed with error %d\n" , result); goto error_name;
drivers/pci/hotplug/fakephp.c +2 -1
@@ -126,7 +126,8 @@ static int add_slot(struct pci_dev *dev) slot->release = &dummy_release; slot->private = dslot; - retval =pci_hp_register (slot, dev->bus,PCI_SLOT (dev->devfn)); + retval =pci_hp_register (slot, dev->bus,PCI_SLOT (dev->devfn), + slot->name);if (retval) {err ("pci_hp_register failed with error %d\n" , retval); goto error_dslot;
drivers/pci/hotplug/ibmphp_ebda.c +2 -1
@@ -966,7 +966,8 @@ static int __init ebda_rsrc_controller (void)list_for_each_entry (tmp_slot, &ibmphp_slot_head, ibm_slot_list) {snprintf (tmp_slot->hotplug_slot->name,30 ,"%s" ,create_file_name (tmp_slot));pci_hp_register (tmp_slot->hotplug_slot, -pci_find_bus (0 , tmp_slot->bus), tmp_slot->device); +pci_find_bus (0 , tmp_slot->bus), tmp_slot->device, + tmp_slot->hotplug_slot->name); }print_ebda_hpc ();
drivers/pci/hotplug/pci_hotplug_core.c +8 -7
@@ -547,13 +547,15 @@ out:* @bus: bus this slot is on * @slot: pointer to the &struct hotplug_slot to register * @slot_nr: slot number +* @name: name registered with kobject core * * Registers a hotplug slot with the pci hotplug subsystem, which will allow * userspace interaction to the slot. * * Returns 0 if successful, anything else for an error. */ -int pci_hp_register (struct hotplug_slot *slot,struct pci_bus *bus,int slot_nr) +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;
@@ -569,7 +571,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr) }/* Check if we have already registered a slot with the same name. */ -if (get_slot_from_name (slot->name)) +if (get_slot_from_name (name))return -EEXIST ;/*
@@ -577,7 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)* driver and call it here again. If we've already created the * pci_slot, the interface will simply bump the refcount. */ - pci_slot =pci_create_slot (bus, slot_nr, slot->name); + pci_slot =pci_create_slot (bus, slot_nr, name);if (IS_ERR (pci_slot))return PTR_ERR (pci_slot);
@@ -593,8 +595,8 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)/* * Allow pcihp drivers to override the ACPI_PCI_SLOT name. */ -if (strcmp (kobject_name (&pci_slot->kobj), slot->name)) { - result =kobject_rename (&pci_slot->kobj, slot->name); +if (strcmp (kobject_name (&pci_slot->kobj), name)) { + result =kobject_rename (&pci_slot->kobj, name);if (result) {pci_destroy_slot (pci_slot);return result;
@@ -607,8 +609,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr) result =fs_add_slot (pci_slot);kobject_uevent (&pci_slot->kobj,KOBJ_ADD ); -dbg ("Added slot %s to the list\n" , slot->name); - +dbg ("Added slot %s to the list\n" , name);return result; }
drivers/pci/hotplug/pciehp_core.c +2 -1
@@ -226,7 +226,8 @@ static int init_slots(struct controller *ctrl) duplicate_name: retval =pci_hp_register (hotplug_slot, ctrl->pci_dev->subordinate, - slot->device); + slot->device, + slot->name);if (retval) {/* * If slot N already exists, we'll try to create
drivers/pci/hotplug/rpaphp_slot.c +1 -1
@@ -137,7 +137,7 @@ int rpaphp_register_slot(struct slot *slot) slotno =PCI_SLOT (PCI_DN (slot->dn->child)->devfn); else slotno =-1 ; - retval =pci_hp_register (php_slot, slot->bus, slotno); + retval =pci_hp_register (php_slot, slot->bus, slotno, slot->name);if (retval) {err ("pci_hp_register failed with error %d\n" , retval);return retval;
drivers/pci/hotplug/sgi_hotplug.c +2 -1
@@ -653,7 +653,8 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) bss_hotplug_slot->ops = &sn_hotplug_slot_ops; bss_hotplug_slot->release = &sn_release_slot; - rc =pci_hp_register (bss_hotplug_slot, pci_bus, device); + rc =pci_hp_register (bss_hotplug_slot, pci_bus, device, + bss_hotplug_slot->name);if (rc) goto register_err;
drivers/pci/hotplug/shpchp_core.c +2 -1
@@ -146,7 +146,8 @@ static int init_slots(struct controller *ctrl) slot->hp_slot, slot->number, ctrl->slot_device_offset); duplicate_name: retval =pci_hp_register (slot->hotplug_slot, - ctrl->pci_dev->subordinate, slot->device); + ctrl->pci_dev->subordinate, slot->device, + hotplug_slot->name);if (retval) {/* * If slot N already exists, we'll try to create
include/linux/pci_hotplug.h +4 -1
@@ -165,7 +165,10 @@ struct hotplug_slot { };#define to_hotplug_slot (n) container_of(n, struct hotplug_slot, kobj) -extern int pci_hp_register (struct hotplug_slot *,struct pci_bus *,int nr); +extern int pci_hp_register (struct hotplug_slot *, +struct pci_bus *, +int nr, +const char *name);extern int pci_hp_deregister (struct hotplug_slot *slot);extern int __must_check pci_hp_change_slot_info (struct hotplug_slot *slot,struct hotplug_slot_info *info);
PATCH v3 2/14 PCI: prevent duplicate slot names
In reply to Alex ChiangPrevent 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 etc.
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
This commit also introduces two new PCI core interfaces:
struct pci_slot *pci_get_pci_slot(struct pci_bus *parent, int slot_nr); void pci_renumber_slot(struct pci_slot *slot, int slot_nr);
And removes the old, unused pci_update_slot_number() API.
---
drivers/pci/hotplug/pci_hotplug_core.c | 49 ++++----- drivers/pci/hotplug/pciehp_core.c | 15 --- drivers/pci/hotplug/shpchp_core.c | 15 --- drivers/pci/slot.c | 167 ++++++++++++++++++++++++-------- include/linux/pci.h | 4 + 5 files changed, 153 insertions(+), 97 deletions(-)
drivers/pci/hotplug/pci_hotplug_core.c +23 -26
@@ -570,39 +570,32 @@ 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 ; -/* -* No problems if we call this interface from both ACPI_PCI_SLOT -* driver and call it here again. If we've already created the -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired. +* +* Otherwise, create the slot and carry on with life. */ - pci_slot =pci_create_slot (bus, slot_nr, name); -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 ; + pci_slot =pci_get_pci_slot (bus, slot_nr); +if (pci_slot) { +if (pci_slot->hotplug) { + result = -EBUSY ; + goto err; + } + +if (strcmp (kobject_name (&pci_slot->kobj), name)) +if ((result =pci_rename_slot (pci_slot, name))) + goto err; + }else { + pci_slot =pci_create_slot (bus, slot_nr, name); +if ((result =IS_ERR (pci_slot))) + goto out; } slot->pci_slot = pci_slot; pci_slot->hotplug = slot; -/* -* Allow pcihp drivers to override the ACPI_PCI_SLOT name. -*/ -if (strcmp (kobject_name (&pci_slot->kobj), name)) { - result =kobject_rename (&pci_slot->kobj, name); -if (result) { -pci_destroy_slot (pci_slot); -return result; - } - } -spin_lock (&pci_hotplug_slot_list_lock);list_add (&slot->slot_list, &pci_hotplug_slot_list);spin_unlock (&pci_hotplug_slot_list_lock);
@@ -611,7 +604,11 @@ 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" , name); +out:return result; +err: +pci_destroy_slot (pci_slot); + goto out; }/**
drivers/pci/hotplug/pciehp_core.c +0 -15
@@ -196,7 +196,6 @@ static int init_slots(struct controller *ctrl)struct slot *slot;struct hotplug_slot *hotplug_slot;struct hotplug_slot_info *info; -int len, dup =1 ;int retval = -ENOMEM ;list_for_each_entry (slot, &ctrl->slot_list, slot_list) {
@@ -223,25 +222,11 @@ static int init_slots(struct controller *ctrl)ctrl_dbg (ctrl,"Registering bus=%x dev=%x hp_slot=%x sun=%x " "slot_device_offset=%x\n" , slot->bus, slot->device, slot->hp_slot, slot->number, ctrl->slot_device_offset); -duplicate_name: retval =pci_hp_register (hotplug_slot, ctrl->pci_dev->subordinate, slot->device, slot->name);if (retval) { -/* -* If slot N already exists, we'll try to create -* slot N-1, N-2 ... N-M, until we overflow. -*/ -if (retval == -EEXIST ) { - len =snprintf (slot->name,SLOT_NAME_SIZE , -"%d-%d" , slot->number, dup++); -if (len <SLOT_NAME_SIZE ) - goto duplicate_name; -else -ctrl_err (ctrl,"duplicate slot name " -"overflow\n" ); - }ctrl_err (ctrl,"pci_hp_register failed with error %d\n" , retval); goto error_info;
drivers/pci/hotplug/shpchp_core.c +1 -14
@@ -102,7 +102,7 @@ static int init_slots(struct controller *ctrl)struct hotplug_slot *hotplug_slot;struct hotplug_slot_info *info;int retval = -ENOMEM ; -int i, len, dup =1 ; +int i;for (i =0 ; i < ctrl->num_slots; i++) { slot =kzalloc (sizeof (*slot),GFP_KERNEL );
@@ -144,23 +144,10 @@ static int init_slots(struct controller *ctrl)dbg ("Registering bus=%x dev=%x hp_slot=%x sun=%x " "slot_device_offset=%x\n" , slot->bus, slot->device, slot->hp_slot, slot->number, ctrl->slot_device_offset); -duplicate_name: retval =pci_hp_register (slot->hotplug_slot, ctrl->pci_dev->subordinate, slot->device, hotplug_slot->name);if (retval) { -/* -* If slot N already exists, we'll try to create -* slot N-1, N-2 ... N-M, until we overflow. -*/ -if (retval == -EEXIST ) { - len =snprintf (slot->name,SLOT_NAME_SIZE , -"%d-%d" , slot->number, dup++); -if (len <SLOT_NAME_SIZE ) - goto duplicate_name; -else -err ("duplicate slot name overflow\n" ); - }err ("pci_hp_register failed with error %d\n" , retval); goto error_info; }
drivers/pci/slot.c +126 -41
241 lines not on this page · raw message
include/linux/pci.h +3 -1
@@ -509,8 +509,10 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev, int busnr);struct pci_slot *pci_create_slot (struct pci_bus *parent,int slot_nr,const char *name); +struct pci_slot *pci_get_pci_slot (struct pci_bus *parent,int slot_nr);void pci_destroy_slot (struct pci_slot *slot); -void pci_update_slot_number (struct pci_slot *slot,int slot_nr); +int pci_rename_slot (struct pci_slot *slot,const char *name); +void pci_renumber_slot (struct pci_slot *slot,int slot_nr);int pci_scan_slot (struct pci_bus *bus,int devfn);struct pci_dev *pci_scan_single_device (struct pci_bus *bus,int devfn);void pci_device_add (struct pci_dev *dev,struct pci_bus *bus);
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v3 02/14] PCI: prevent duplicate slot names
It seems my previous e-mail was not sent properly. So resending it.
Thanks,
Kenji Kaneshige
Quoted 51 lines · Kenji Kaneshige
Hi Alex-san,
Here is one comment, though I have not finished reviewing/testing your patches yet (sorry for the delay).
Alex Chiang wrote:
(snip.)
Quoted 23 lines
drivers/pci/hotplug/pci_hotplug_core.c +3
-7@@ -570,39 +570,32 @@ 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 ; -/* -* No problems if we call this interface from both ACPI_PCI_SLOT -* driver and call it here again. If we've already created the -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired.
drivers/pci/hotplug/pci_hotplug_core.c +2
-0+* +* Otherwise, create the slot and carry on with life. drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, · line 580
Quoted 10 lines
drivers/pci/hotplug/pci_hotplug_core.c +1
-8*/ - pci_slot =pci_create_slot (bus, slot_nr, name); -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 ; + pci_slot =pci_get_pci_slot (bus, slot_nr);The
pci_get_pci_slot()function referspci_bus->slotslist, so it should be called withpci_bus_semsemaphore held aspci_create_slot()does, orpci_bus_semsemaphore should be held bypci_get_pci_slot()itself.Thanks,
Kenji Kaneshige
Re: [PATCH v3 02/14] PCI: prevent duplicate slot names
* Kenji Kaneshige <[email protected]>:
Quoted 41 lines
Quoted 40 lines · Kenji Kaneshige
Hi Alex-san,
Here is one comment, though I have not finished reviewing/testing your patches yet (sorry for the delay).
Alex Chiang wrote:
(snip.)
Quoted 31 lines
drivers/pci/hotplug/pci_hotplug_core.c +0
-0@@ -570,39 +570,32 @@ 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 samename. */
drivers/pci/hotplug/pci_hotplug_core.c +3
-6-if (get_slot_from_name (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 -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired.
drivers/pci/hotplug/pci_hotplug_core.c +2
-6+* +* Otherwise, create the slot and carry on with life. */ - pci_slot =pci_create_slot (bus, slot_nr, name); -if (IS_ERR (pci_slot)) -return PTR_ERR (pci_slot); - -if (pci_slot->hotplug) { -dbg ("%s: already claimed\n" , __func__);
Quoted 8 lines
Quoted 8 lines
drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, · line 580
Quoted 3 lines
drivers/pci/hotplug/pci_hotplug_core.c +1
-2-pci_destroy_slot (pci_slot); -return -EBUSY ; + pci_slot =pci_get_pci_slot (bus, slot_nr);The
pci_get_pci_slot()function referspci_bus->slotslist, so it should be called withpci_bus_semsemaphore held aspci_create_slot()does, orpci_bus_semsemaphore should be held bypci_get_pci_slot()itself.
Yes, I've changed pci_get_pci_slot() to acquire the pci_bus_sem semaphore.
Thank you for pointing this out.
It will be fixed in v4 of this patch series, which I will send out after I receive the rest of your review comments.
Thanks!
/ac
Re: [PATCH v3 02/14] PCI: prevent duplicate slot names
Alex-san,
Quoted 49 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 39 lines
Quoted 38 lines · Kenji Kaneshige
Hi Alex-san,
Here is one comment, though I have not finished reviewing/testing your patches yet (sorry for the delay).
Alex Chiang wrote:
(snip.)
Quoted 29 lines
drivers/pci/hotplug/pci_hotplug_core.c +0
-0@@ -570,39 +570,32 @@ 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 samename. */
drivers/pci/hotplug/pci_hotplug_core.c +3
-6-if (get_slot_from_name (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 -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired.
drivers/pci/hotplug/pci_hotplug_core.c +2
-5+* +* Otherwise, create the slot and carry on with life. */ - pci_slot =pci_create_slot (bus, slot_nr, name); -if (IS_ERR (pci_slot)) -return PTR_ERR (pci_slot); - -if (pci_slot->hotplug) {Quoted 8 lines
Quoted 8 lines
drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, · line 580
Quoted 4 lines
drivers/pci/hotplug/pci_hotplug_core.c +1
-3-dbg ("%s: already claimed\n" , __func__); -pci_destroy_slot (pci_slot); -return -EBUSY ; + pci_slot =pci_get_pci_slot (bus, slot_nr);The
pci_get_pci_slot()function referspci_bus->slotslist, so it should be called withpci_bus_semsemaphore held aspci_create_slot()does, orpci_bus_semsemaphore should be held bypci_get_pci_slot()itself.
Quoted 8 lines
Yes, I've changed
pci_get_pci_slot()to acquire thepci_bus_semsemaphore.Thank you for pointing this out.
It will be fixed in v4 of this patch series, which I will send out after I receive the rest of your review comments.
I noticed that changing pci_get_pci_slot() to acquire the pci_bus_sem might be not enough. If slot was created between pci_get_pci_slot() and pci_create_slot() by another thread in the following code, something wrong would happen I think.
pci_slot = pci_get_pci_slot(bus, slot_nr);
if (pci_slot) {
if (pci_slot->hotplug) {
result = -EBUSY;
goto err;
}
if (strcmp(kobject_name(&pci_slot->kobj), name))
if ((result = pci_rename_slot(pci_slot, name)))
goto err;
} else {
pci_slot = pci_create_slot(bus, slot_nr, name);
if ((result = IS_ERR(pci_slot)))
goto out;
}I've finished reviewing and testing your patches. The rest of your patch looks good to me. Of course, we must not forget the comment from Taku Izumi.
Thanks,
Kenji Kaneshige
Re: [PATCH v3 02/14] PCI: prevent duplicate slot names
Hi Kenji-san,
* Kenji Kaneshige <[email protected]>:
Quoted 65 lines
Quoted 50 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 40 lines
Quoted 39 lines · Kenji Kaneshige
Hi Alex-san,
Here is one comment, though I have not finished reviewing/testing your patches yet (sorry for the delay).
Alex Chiang wrote:
(snip.)
Quoted 30 lines
drivers/pci/hotplug/pci_hotplug_core.c +0
-0@@ -570,39 +570,32 @@ 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 samename. */
drivers/pci/hotplug/pci_hotplug_core.c +5
-11-if (get_slot_from_name (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 -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired. +* +* Otherwise, create the slot and carry on with life. */ - pci_slot =pci_create_slot (bus, slot_nr, name); -if (IS_ERR (pci_slot)) -return PTR_ERR (pci_slot); - -if (pci_slot->hotplug) {Quoted 8 lines
Quoted 8 lines
drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, · line 580
Quoted 4 lines
drivers/pci/hotplug/pci_hotplug_core.c +1
-3-dbg ("%s: already claimed\n" , __func__); -pci_destroy_slot (pci_slot); -return -EBUSY ; + pci_slot =pci_get_pci_slot (bus, slot_nr);The
pci_get_pci_slot()function referspci_bus->slotslist, so it should be called withpci_bus_semsemaphore held aspci_create_slot()does, orpci_bus_semsemaphore should be held bypci_get_pci_slot()itself.Quoted 8 lines
Yes, I've changed
pci_get_pci_slot()to acquire thepci_bus_semsemaphore.Thank you for pointing this out.
It will be fixed in v4 of this patch series, which I will send out after I receive the rest of your review comments.
I noticed that changing
pci_get_pci_slot()to acquire thepci_bus_semmight be not enough. If slot was created betweenpci_get_pci_slot()andpci_create_slot()by another thread in the following code, something wrong would happen I think.
Quoted 15 lines
pci_slot = pci_get_pci_slot(bus, slot_nr);
if (pci_slot) {
if (pci_slot->hotplug) {
result = -EBUSY;
goto err;
}
if (strcmp(kobject_name(&pci_slot->kobj), name))
if ((result = pci_rename_slot(pci_slot, name)))
goto err;
} else {
pci_slot = pci_create_slot(bus, slot_nr, name);
if ((result = IS_ERR(pci_slot)))
goto out;
}I'm sorry, I don't think I see the problem you are pointing out.
If pci_get_pci_slot() finds a pci_slot, we do not modify pci_bus->slots any further, so even if a new slot is created, it shouldn't affect the pci_slot that we already found.
I must be missing something, but I don't know what. Would you mind explaining what you had in mind with your comment?
Quoted 3 lines
I've finished reviewing and testing your patches. The rest of your patch looks good to me. Of course, we must not forget the comment from Taku Izumi.
Yes, I've modified my patch series to take into account the bugs that Taku-san found.
Thank you both for your reviews.
/ac
Re: [PATCH v3 02/14] PCI: prevent duplicate slot names
Quoted 81 lines · Alex Chiang
Hi Kenji-san,
* Kenji Kaneshige <[email protected]>:
Quoted 62 lines
Quoted 48 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 39 lines
Quoted 38 lines · Kenji Kaneshige
Hi Alex-san,
Here is one comment, though I have not finished reviewing/testing your patches yet (sorry for the delay).
Alex Chiang wrote:
(snip.)
Quoted 29 lines
drivers/pci/hotplug/pci_hotplug_core.c +0
-0@@ -570,39 +570,32 @@ 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 samename. */
drivers/pci/hotplug/pci_hotplug_core.c +5
-11-if (get_slot_from_name (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 -* pci_slot, the interface will simply bump the refcount. +* Look for existing slot. If we find it, and it was created by a +* slot detection driver (ie, doesn't have a ->hotplug()) then we +* allow the hotplug driver calling us to rename the slot if desired. +* +* Otherwise, create the slot and carry on with life. */ - pci_slot =pci_create_slot (bus, slot_nr, name); -if (IS_ERR (pci_slot)) -return PTR_ERR (pci_slot); - -if (pci_slot->hotplug) {Quoted 8 lines
Quoted 8 lines
drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr, · line 580
Quoted 4 lines
drivers/pci/hotplug/pci_hotplug_core.c +1
-3-dbg ("%s: already claimed\n" , __func__); -pci_destroy_slot (pci_slot); -return -EBUSY ; + pci_slot =pci_get_pci_slot (bus, slot_nr);The
pci_get_pci_slot()function referspci_bus->slotslist, so it should be called withpci_bus_semsemaphore held aspci_create_slot()does, orpci_bus_semsemaphore should be held bypci_get_pci_slot()itself.Quoted 8 lines
Yes, I've changed
pci_get_pci_slot()to acquire thepci_bus_semsemaphore.Thank you for pointing this out.
It will be fixed in v4 of this patch series, which I will send out after I receive the rest of your review comments.
I noticed that changing
pci_get_pci_slot()to acquire thepci_bus_semmight be not enough. If slot was created betweenpci_get_pci_slot()andpci_create_slot()by another thread in the following code, something wrong would happen I think.Quoted 15 lines
pci_slot = pci_get_pci_slot(bus, slot_nr); if (pci_slot) { if (pci_slot->hotplug) { result = -EBUSY; goto err; } if (strcmp(kobject_name(&pci_slot->kobj), name)) if ((result = pci_rename_slot(pci_slot, name))) goto err; } else { pci_slot = pci_create_slot(bus, slot_nr, name); if ((result = IS_ERR(pci_slot))) goto out; }
Quoted 9 lines
I'm sorry, I don't think I see the problem you are pointing out.
If
pci_get_pci_slot()finds apci_slot, we do not modifypci_bus->slotsany further, so even if a new slot is created, it shouldn't affect thepci_slotthat we already found.I must be missing something, but I don't know what. Would you mind explaining what you had in mind with your comment?
Here is one of the example of the problem I imagine.
When pci_get_pci_slot() does NOT find a pci_slot, pci_hp_register() calls pci_create_slot(). If another hotplug driver creates slot between pci_get_pci_slot() and pci_create_slot(), hotplug member of pci_slot returned from pci_create_slot() is not NULL in the following code.
Quoted 3 lines
Quoted 3 lines
pci_slot = pci_create_slot(bus, slot_nr, name); if ((result = IS_ERR(pci_slot))) goto out;
In this case, we should return with -EBUSY. But because there is no check to see if pci->slot is NULL, pci_slot->hotplug will be overwritten with the new value.
Thanks,
Kenji Kaneshige
PATCH v3 3/14 PCI, PCI Hotplug: introduce slot_name helpers
In reply to Alex ChiangIn preparation for cleaning up the various hotplug drivers such that they don't have to manage their own 'name' parameters anymore, we provide the following convenience functions:
pci_slot_name() hotplug_slot_name()
These helpers will be used by individual hotplug drivers.
---
include/linux/pci.h | 5 +++++ include/linux/pci_hotplug.h | 5 +++++ 2 files changed, 10 insertions(+), 0 deletions(-)
include/linux/pci.h +5 -0
@@ -64,6 +64,11 @@ struct pci_slot {struct kobject kobj; }; +static inline const char *pci_slot_name (const struct pci_slot *slot) +{ +return kobject_name (&slot->kobj); +} +/* File state for mmap()s on /proc/bus/pci/X/Y */ enum pci_mmap_state { pci_mmap_io,
include/linux/pci_hotplug.h +5 -0
@@ -165,6 +165,11 @@ struct hotplug_slot { };#define to_hotplug_slot (n) container_of(n, struct hotplug_slot, kobj) +static inline const char *hotplug_slot_name (const struct hotplug_slot *slot) +{ +return pci_slot_name (slot->pci_slot); +} + extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr,
PATCH v3 4/14 PCI: acpiphp: remove 'name' parameter
In reply to Alex ChiangWe do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.
Remove 'name' from acpiphp's version of struct slot.
---
drivers/pci/hotplug/acpiphp.h | 9 +++++---- drivers/pci/hotplug/acpiphp_core.c | 31 ++++++++++++++++--------------- 2 files changed, 21 insertions(+), 19 deletions(-)
drivers/pci/hotplug/acpiphp.h +5 -4
@@ -50,9 +50,6 @@ #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg) #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg) -/* name size which is used for entries in pcihpfs */ -#define SLOT_NAME_SIZE 20/* {_SUN} */ -struct acpiphp_bridge ;struct acpiphp_slot ;
@@ -63,9 +60,13 @@ struct slot {struct hotplug_slot *hotplug_slot;struct acpiphp_slot *acpi_slot;struct hotplug_slot_info info; -char name[SLOT_NAME_SIZE ]; }; +static inline const char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +/* * struct acpiphp_bridge - PCI bridge information *
drivers/pci/hotplug/acpiphp_core.c +16 -15
@@ -44,6 +44,9 @@#define MY_NAME "acpiphp" +/* name size which is used for entries in pcihpfs */ +#define SLOT_NAME_SIZE 21/* {_SUN} */ +static int debug;int acpiphp_debug;
@@ -84,7 +87,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops = { .get_adapter_status = get_adapter_status, }; -/** * acpiphp_register_attention - set attention LED callback * @info: must be completely filled with LED callbacks
@@ -136,7 +138,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));/* enable the specified slot */ return acpiphp_enable_slot (slot->acpi_slot);
@@ -154,7 +156,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)struct slot *slot = hotplug_slot->private;int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));/* disable the specified slot */ retval =acpiphp_disable_slot (slot->acpi_slot);
@@ -177,7 +179,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot) {int retval = -ENODEV ; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,hotplug_slot_name (hotplug_slot));if (attention_info &&try_module_get (attention_info->owner)) { retval = attention_info->set_attn (hotplug_slot, status);
@@ -200,7 +202,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); *value =acpiphp_get_power_status (slot->acpi_slot);
@@ -222,7 +224,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value) {int retval = -EINVAL ; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,hotplug_slot_name (hotplug_slot));if (attention_info &&try_module_get (attention_info->owner)) { retval = attention_info->get_attn (hotplug_slot, value);
@@ -245,7 +247,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); *value =acpiphp_get_latch_status (slot->acpi_slot);
@@ -265,7 +267,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); *value =acpiphp_get_adapter_status (slot->acpi_slot);
@@ -299,7 +301,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));kfree (slot->hotplug_slot);kfree (slot);
@@ -310,6 +312,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) {struct slot *slot;int retval = -ENOMEM ; +char name[SLOT_NAME_SIZE ]; slot =kzalloc (sizeof (*slot),GFP_KERNEL );if (!slot)
@@ -321,8 +324,6 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) slot->hotplug_slot->info = &slot->info; - slot->hotplug_slot->name = slot->name; - slot->hotplug_slot->private = slot; slot->hotplug_slot->release = &release_slot; slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
@@ -336,12 +337,12 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) slot->hotplug_slot->info->cur_bus_speed =PCI_SPEED_UNKNOWN ; acpiphp_slot->slot = slot; -snprintf (slot->name,sizeof (slot->name),"%u" , slot->acpi_slot->sun); +scnprintf (name,SLOT_NAME_SIZE ,"%u" , slot->acpi_slot->sun); retval =pci_hp_register (slot->hotplug_slot, acpiphp_slot->bridge->pci_bus, acpiphp_slot->device, - slot->name); + name);if (retval == -EBUSY ) goto error_hpslot;if (retval) {
@@ -349,7 +350,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot) goto error_hpslot; } -info ("Slot [%s] registered\n" , slot->hotplug_slot->name); +info ("Slot [%s] registered\n" ,slot_name (slot));return 0 ; error_hpslot:
@@ -366,7 +367,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)struct slot *slot = acpiphp_slot->slot;int retval =0 ; -info ("Slot [%s] unregistered\n" , slot->hotplug_slot->name); +info ("Slot [%s] unregistered\n" ,slot_name (slot)); retval =pci_hp_deregister (slot->hotplug_slot);if (retval)
PATCH v3 5/14 PCI: cpci_hotplug: stop managing hotplug_slot->name
In reply to Alex ChiangWe no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.
Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.
---
drivers/pci/hotplug/cpci_hotplug.h | 6 ++ drivers/pci/hotplug/cpci_hotplug_core.c | 75 ++++++++++++------------------- drivers/pci/hotplug/cpci_hotplug_pci.c | 4 +- 3 files changed, 36 insertions(+), 49 deletions(-)
drivers/pci/hotplug/cpci_hotplug.h +6 -0
@@ -30,6 +30,7 @@#include <linux/types.h> #include <linux/pci.h> +#include <linux/pci_hotplug.h> /* PICMG 2.1 R2.0 HS CSR bits: */ #define HS_CSR_INS 0x0080
@@ -69,6 +70,11 @@ struct cpci_hp_controller {struct cpci_hp_controller_ops *ops; }; +static inline const char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +extern int cpci_hp_register_controller (struct cpci_hp_controller *controller);extern int cpci_hp_unregister_controller (struct cpci_hp_controller *controller);extern int cpci_hp_register_bus (struct pci_bus *bus,u8 first,u8 last);
drivers/pci/hotplug/cpci_hotplug_core.c +28 -47
214 lines not on this page · raw message
drivers/pci/hotplug/cpci_hotplug_pci.c +2 -2
@@ -209,7 +209,7 @@ int cpci_led_on(struct slot* slot) hs_cap + 2, hs_csr)) {err ("Could not set LOO for slot %s" , - slot->hotplug_slot->name); +hotplug_slot_name (slot->hotplug_slot));return -ENODEV ; } }
@@ -238,7 +238,7 @@ int cpci_led_off(struct slot* slot) hs_cap + 2, hs_csr)) {err ("Could not clear LOO for slot %s" , - slot->hotplug_slot->name); +hotplug_slot_name (slot->hotplug_slot));return -ENODEV ; } }
PATCH v3 6/14 PCI: cpqphp: stop managing hotplug_slot->name
In reply to Alex ChiangWe no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.
Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.
---
drivers/pci/hotplug/cpqphp.h | 13 +++++------- drivers/pci/hotplug/cpqphp_core.c | 41 +++++++++++++++++-------------------- 2 files changed, 24 insertions(+), 30 deletions(-)
drivers/pci/hotplug/cpqphp.h +5 -8
@@ -449,6 +449,11 @@ extern u8 cpqhp_disk_irq;/* inline functions */ +static inline char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +/* * return_resource *
@@ -696,14 +701,6 @@ static inline int get_presence_status(struct controller *ctrl, struct slot *slotreturn presence_save; } -#define SLOT_NAME_SIZE 10 - -static inline void make_slot_name (char *buffer,int buffer_size,struct slot *slot) -{ -snprintf (buffer, buffer_size,"%d" , slot->number); -} - -static inline int wait_for_ctrl_irq (struct controller *ctrl) {DECLARE_WAITQUEUE (wait, current);
drivers/pci/hotplug/cpqphp_core.c +19 -22
151 lines not on this page · raw message
PATCH v3 7/14 PCI: fakephp: remove 'name' parameter
In reply to Alex ChiangRemove 'name' from fakephp's struct dummy_slot, as the PCI core will now manage our slot name for us.
---
drivers/pci/hotplug/fakephp.c | 19 ++++++++++--------- 1 files changed, 10 insertions(+), 9 deletions(-)
drivers/pci/hotplug/fakephp.c +10 -9
@@ -66,7 +66,6 @@ struct dummy_slot {struct pci_dev *dev;struct work_struct remove_work;unsigned long removed; -char name[8 ]; };static int debug;
@@ -96,10 +95,13 @@ static void dummy_release(struct hotplug_slot *slot)kfree (dslot); } +#define SLOT_NAME_SIZE 8 +static int add_slot (struct pci_dev *dev) {struct dummy_slot *dslot;struct hotplug_slot *slot; +char name[SLOT_NAME_SIZE ];int retval = -ENOMEM ;static int count =1 ;
@@ -119,15 +121,13 @@ static int add_slot(struct pci_dev *dev)if (!dslot) goto error_info; - slot->name = dslot->name; -snprintf (slot->name,sizeof (dslot->name),"fake%d" , count++); -dbg ("slot->name = %s\n" , slot->name); +snprintf (name,SLOT_NAME_SIZE ,"fake%d" , count++); +dbg ("slot->name = %s\n" , name); slot->ops = &dummy_hotplug_slot_ops; slot->release = &dummy_release; slot->private = dslot; - retval =pci_hp_register (slot, dev->bus,PCI_SLOT (dev->devfn), - slot->name); + retval =pci_hp_register (slot, dev->bus,PCI_SLOT (dev->devfn), name);if (retval) {err ("pci_hp_register failed with error %d\n" , retval); goto error_dslot;
@@ -168,10 +168,11 @@ static void remove_slot(struct dummy_slot *dslot) {int retval; -dbg ("removing slot %s\n" , dslot->slot->name); +dbg ("removing slot %s\n" ,hotplug_slot_name (dslot->slot)); retval =pci_hp_deregister (dslot->slot);if (retval) -err ("Problem unregistering a slot %s\n" , dslot->slot->name); +err ("Problem unregistering a slot %s\n" , +hotplug_slot_name (dslot->slot)); }/* called from the single-threaded workqueue handler to remove a slot */
@@ -309,7 +310,7 @@ static int disable_slot(struct hotplug_slot *slot)return -ENODEV ; dslot = slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,hotplug_slot_name (slot));for (func =7 ; func >=0 ; func--) { dev =pci_get_slot (dslot->dev->bus, dslot->dev->devfn + func);
PATCH v3 8/14 PCI: ibmphp: stop managing hotplug_slot->name
In reply to Alex ChiangWe no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.
Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.
Additionally, slightly rearrange the members of struct slot so they are naturally aligned to eliminate holes.
---
drivers/pci/hotplug/ibmphp.h | 5 ++--- drivers/pci/hotplug/ibmphp_ebda.c | 20 +++++++------------- 2 files changed, 9 insertions(+), 16 deletions(-)
drivers/pci/hotplug/ibmphp.h +2 -3
@@ -707,17 +707,16 @@ struct slot {u8 device;u8 number;u8 real_physical_slot_num; -char name[100 ];u32 capabilities;u8 supported_speed;u8 supported_bus_mode; +u8 flag;/* this is for disable slot and polling */ +u8 ctlr_index;struct hotplug_slot *hotplug_slot;struct controller *ctrl;struct pci_func *func;u8 irq[4 ]; -u8 flag;/* this is for disable slot and polling */ int bit_mode;/* 0 = 32, 1 = 64 */ -u8 ctlr_index;struct bus_info *bus_on;struct list_head ibm_slot_list;u8 status;
drivers/pci/hotplug/ibmphp_ebda.c +7 -13
@@ -587,11 +587,14 @@ static u8 calculate_first_slot (u8 slot_num)return first_slot +1 ; } + +#define SLOT_NAME_SIZE 30 +static char *create_file_name (struct slot * slot_cur) {struct opt_rio *opt_vg_ptr =NULL ;struct opt_rio_lo *opt_lo_ptr =NULL ; -static char str[30 ]; +static char str[SLOT_NAME_SIZE ];int which =0 ;/* rxe = 1, chassis = 0 */ u8 number =1 ;/* either chassis or rxe # */ u8 first_slot =1 ;
@@ -703,7 +706,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot) slot = hotplug_slot->private;kfree (slot->hotplug_slot->info); -kfree (slot->hotplug_slot->name);kfree (slot->hotplug_slot); slot->ctrl =NULL ; slot->bus_on =NULL ;
@@ -734,6 +736,7 @@ static int __init ebda_rsrc_controller (void)struct bus_info *bus_info_ptr1, *bus_info_ptr2;int rc;struct slot *tmp_slot; +char name[SLOT_NAME_SIZE ]; addr = hpc_list_ptr->phys_addr;for (ctlr =0 ; ctlr < hpc_list_ptr->num_ctlrs; ctlr++) {
@@ -897,12 +900,6 @@ static int __init ebda_rsrc_controller (void) goto error_no_hp_info; } - hp_slot_ptr->name =kmalloc (30 ,GFP_KERNEL ); -if (!hp_slot_ptr->name) { - rc = -ENOMEM ; - goto error_no_hp_name; - } - tmp_slot =kzalloc (sizeof (*tmp_slot),GFP_KERNEL );if (!tmp_slot) { rc = -ENOMEM ;
@@ -964,10 +961,9 @@ static int __init ebda_rsrc_controller (void) }/* each hpc */ list_for_each_entry (tmp_slot, &ibmphp_slot_head, ibm_slot_list) { -snprintf (tmp_slot->hotplug_slot->name,30 ,"%s" ,create_file_name (tmp_slot)); +snprintf (name,SLOT_NAME_SIZE ,"%s" ,create_file_name (tmp_slot));pci_hp_register (tmp_slot->hotplug_slot, -pci_find_bus (0 , tmp_slot->bus), tmp_slot->device, - tmp_slot->hotplug_slot->name); +pci_find_bus (0 , tmp_slot->bus), tmp_slot->device, name); }print_ebda_hpc ();
@@ -977,8 +973,6 @@ static int __init ebda_rsrc_controller (void) error:kfree (hp_slot_ptr->private); error_no_slot: -kfree (hp_slot_ptr->name); -error_no_hp_name:kfree (hp_slot_ptr->info); error_no_hp_info:kfree (hp_slot_ptr);
PATCH v3 9/14 PCI: pciehp: remove 'name' parameter
In reply to Alex ChiangWe do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.
Remove 'name' from pciehp's version of struct slot, and remove unused 'task_list' as well.
---
drivers/pci/hotplug/pciehp.h | 9 ++++-- drivers/pci/hotplug/pciehp_core.c | 25 +++++++++-------- drivers/pci/hotplug/pciehp_ctrl.c | 53 ++++++++++++++++++++----------------- drivers/pci/hotplug/pciehp_hpc.c | 1 - 4 files changed, 48 insertions(+), 40 deletions(-)
drivers/pci/hotplug/pciehp.h +6 -3
@@ -74,15 +74,13 @@ extern struct workqueue_struct *pciehp_wq;struct slot {u8 bus;u8 device; -u32 number;u8 state; -struct timer_list task_event;u8 hp_slot; +u32 number;struct controller *ctrl;struct hpc_ops *hpc_ops;struct hotplug_slot *hotplug_slot;struct list_head slot_list; -char name[SLOT_NAME_SIZE ];unsigned long last_emi_toggle;struct delayed_work work;/* work for button event */ struct mutex lock;
@@ -175,6 +173,11 @@ int pciehp_enable_slot(struct slot *p_slot);int pciehp_disable_slot (struct slot *p_slot);int pcie_enable_notification (struct controller *ctrl); +static inline const char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +static inline struct slot *pciehp_find_slot (struct controller *ctrl,u8 device) {struct slot *slot;
drivers/pci/hotplug/pciehp_core.c +13 -12
@@ -185,7 +185,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)struct slot *slot = hotplug_slot->private;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,hotplug_slot_name (hotplug_slot));kfree (hotplug_slot->info);kfree (hotplug_slot);
@@ -196,6 +196,7 @@ static int init_slots(struct controller *ctrl)struct slot *slot;struct hotplug_slot *hotplug_slot;struct hotplug_slot_info *info; +char name[SLOT_NAME_SIZE ];int retval = -ENOMEM ;list_for_each_entry (slot, &ctrl->slot_list, slot_list) {
@@ -209,7 +210,6 @@ static int init_slots(struct controller *ctrl)/* register this slot with the hotplug pci core */ hotplug_slot->info = info; - hotplug_slot->name = slot->name; hotplug_slot->private = slot; hotplug_slot->release = &release_slot; hotplug_slot->ops = &pciehp_hotplug_slot_ops;
@@ -218,6 +218,7 @@ static int init_slots(struct controller *ctrl)get_latch_status (hotplug_slot, &info->latch_status);get_adapter_status (hotplug_slot, &info->adapter_status); slot->hotplug_slot = hotplug_slot; +snprintf (name,SLOT_NAME_SIZE ,"%u" , slot->number); ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x " "slot_device_offset=%x\n", slot->bus, slot->device,
@@ -225,7 +226,7 @@ static int init_slots(struct controller *ctrl) retval =pci_hp_register (hotplug_slot, ctrl->pci_dev->subordinate, slot->device, - slot->name); + name);if (retval) {ctrl_err (ctrl,"pci_hp_register failed with error %d\n" , retval);
@@ -273,7 +274,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)struct slot *slot = hotplug_slot->private;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); hotplug_slot->info->attention_status = status;
@@ -289,7 +290,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)struct slot *slot = hotplug_slot->private;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot));return pciehp_sysfs_enable_slot (slot); }
@@ -300,7 +301,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)struct slot *slot = hotplug_slot->private;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot));return pciehp_sysfs_disable_slot (slot); }
@@ -311,7 +312,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)int retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_power_status (slot, value);if (retval <0 )
@@ -326,7 +327,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)int retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_attention_status (slot, value);if (retval <0 )
@@ -341,7 +342,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)int retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_latch_status (slot, value);if (retval <0 )
@@ -356,7 +357,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)int retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_adapter_status (slot, value);if (retval <0 )
@@ -372,7 +373,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,int retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_max_bus_speed (slot, value);if (retval <0 )
@@ -387,7 +388,7 @@ static int get_cur_bus_speed(struct hotplug_slot *hotplug_slot, enum pci_bus_speint retval;ctrl_dbg (slot->ctrl,"%s - physical_slot = %s\n" , - __func__, hotplug_slot->name); + __func__,slot_name (slot)); retval = slot->hpc_ops->get_cur_bus_speed (slot, value);if (retval <0 )
drivers/pci/hotplug/pciehp_ctrl.c +29 -24
178 lines not on this page · raw message
drivers/pci/hotplug/pciehp_hpc.c +0 -1
@@ -1061,7 +1061,6 @@ static int pcie_init_slot(struct controller *ctrl) slot->device = ctrl->slot_device_offset + slot->hp_slot; slot->hpc_ops = ctrl->hpc_ops; slot->number = ctrl->first_slot; -snprintf (slot->name,SLOT_NAME_SIZE ,"%d" , slot->number);mutex_init (&slot->lock);INIT_DELAYED_WORK (&slot->work, pciehp_queue_pushbutton_work);list_add (&slot->slot_list, &ctrl->slot_list);
PATCH v3 10/14 PCI: rpaphp: stop managing hotplug_slot->name
In reply to Alex ChiangWe no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.
This means that alloc_slot_struct() no longer needs to take a 'name' param. On the other hand, we give rpaphp_register_slot() a 'name' param now to simplify the hotplug registration process. rpaphp_register_slot() can directly pass the drc_name to the PCI hotplug core.
---
drivers/pci/hotplug/rpaphp.h | 10 +++++++--- drivers/pci/hotplug/rpaphp_core.c | 6 +++--- drivers/pci/hotplug/rpaphp_pci.c | 4 ++-- drivers/pci/hotplug/rpaphp_slot.c | 26 +++++++++----------------- 4 files changed, 21 insertions(+), 25 deletions(-)
drivers/pci/hotplug/rpaphp.h +7 -3
@@ -73,13 +73,17 @@ struct slot {u32 index;u32 type;u32 power_domain; -char *name;struct device_node *dn;struct pci_bus *bus;struct list_head *pci_devs;struct hotplug_slot *hotplug_slot; }; +static inline const char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +extern struct hotplug_slot_ops rpaphp_hotplug_slot_ops;extern struct list_head rpaphp_slot_head;
@@ -96,8 +100,8 @@ extern int rpaphp_get_drc_props(struct device_node *dn, int *drc_index,/* rpaphp_slot.c */ extern void dealloc_slot_struct (struct slot *slot); -extern struct slot *alloc_slot_struct (struct device_node *dn,int drc_index,char *drc_name,int power_domain); -extern int rpaphp_register_slot (struct slot *slot); +extern struct slot *alloc_slot_struct (struct device_node *dn,int drc_index,int power_domain); +extern int rpaphp_register_slot (struct slot *slot,const char *name);extern int rpaphp_deregister_slot (struct slot *slot);#endif /* _PPC64PHP_H */
drivers/pci/hotplug/rpaphp_core.c +3 -3
@@ -324,7 +324,7 @@ int rpaphp_add_slot(struct device_node *dn) type = (char *) &types[1 ];for (i =0 ; i < indexes[0 ]; i++) { - slot =alloc_slot_struct (dn, indexes[i +1 ], name, power_domains[i +1 ]); + slot =alloc_slot_struct (dn, indexes[i +1 ], power_domains[i +1 ]);if (!slot)return -ENOMEM ;
@@ -335,7 +335,7 @@ int rpaphp_add_slot(struct device_node *dn) retval =rpaphp_enable_slot (slot);if (!retval) - retval =rpaphp_register_slot (slot); + retval =rpaphp_register_slot (slot, name);if (retval)dealloc_slot_struct (slot);
@@ -404,7 +404,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot) }else if (state ==EMPTY ) { slot->state =EMPTY ; }else { -err ("%s: slot[%s] is in invalid state\n" , __func__, slot->name); +err ("%s: slot[%s] is in invalid state\n" , __func__,slot_name (slot)); slot->state =NOT_VALID ;return -EINVAL ; }
drivers/pci/hotplug/rpaphp_pci.c +2 -2
@@ -51,7 +51,7 @@ int rpaphp_get_sensor_state(struct slot *slot, int *state) &setlevel);if (rc <0 ) {dbg ("%s: power on slot[%s] failed rc=%d.\n" , - __func__, slot->name, rc); + __func__,slot_name (slot), rc); }else { rc =rtas_get_sensor (DR_ENTITY_SENSE , slot->index, state);
@@ -111,7 +111,7 @@ int rpaphp_enable_slot(struct slot *slot)/* non-empty slot has to have child */ if (!slot->dn->child) {err ("%s: slot[%s]'s device_node doesn't have child for adapter\n" , - __func__, slot->name); + __func__,slot_name (slot));return -EINVAL ; }
drivers/pci/hotplug/rpaphp_slot.c +9 -17
@@ -43,13 +43,12 @@ static void rpaphp_release_slot(struct hotplug_slot *hotplug_slot)void dealloc_slot_struct (struct slot *slot) {kfree (slot->hotplug_slot->info); -kfree (slot->hotplug_slot->name);kfree (slot->hotplug_slot);kfree (slot); }struct slot *alloc_slot_struct (struct device_node *dn, -int drc_index,char *drc_name,int power_domain) +int drc_index,int power_domain) {struct slot *slot;
@@ -63,11 +62,6 @@ struct slot *alloc_slot_struct(struct device_node *dn, GFP_KERNEL);if (!slot->hotplug_slot->info) goto error_hpslot; - slot->hotplug_slot->name =kmalloc (strlen (drc_name) +1 ,GFP_KERNEL ); -if (!slot->hotplug_slot->name) - goto error_info; - slot->name = slot->hotplug_slot->name; -strcpy (slot->name, drc_name); slot->dn = dn; slot->index = drc_index; slot->power_domain = power_domain;
@@ -77,8 +71,6 @@ struct slot *alloc_slot_struct(struct device_node *dn,return (slot); -error_info: -kfree (slot->hotplug_slot->info); error_hpslot:kfree (slot->hotplug_slot); error_slot:
@@ -92,7 +84,7 @@ static int is_registered(struct slot *slot)struct slot *tmp_slot;list_for_each_entry (tmp_slot, &rpaphp_slot_head, rpaphp_slot_list) { -if (!strcmp (tmp_slot->name, slot->name)) +if (!strcmp (slot_name (tmp_slot),slot_name (slot)))return 1 ; }return 0 ;
@@ -104,32 +96,32 @@ int rpaphp_deregister_slot(struct slot *slot)struct hotplug_slot *php_slot = slot->hotplug_slot;dbg ("%s - Entry: deregistering slot=%s\n" , - __func__, slot->name); + __func__,slot_name (slot));list_del (&slot->rpaphp_slot_list); retval =pci_hp_deregister (php_slot);if (retval) -err ("Problem unregistering a slot %s\n" , slot->name); +err ("Problem unregistering a slot %s\n" ,slot_name (slot));dbg ("%s - Exit: rc[%d]\n" , __func__, retval);return retval; }EXPORT_SYMBOL_GPL (rpaphp_deregister_slot); -int rpaphp_register_slot (struct slot *slot) +int rpaphp_register_slot (struct slot *slot,const char *name) {struct hotplug_slot *php_slot = slot->hotplug_slot;int retval;int slotno;dbg ("%s registering slot:path[%s] index[%x], name[%s] pdomain[%x] type[%d]\n" , - __func__, slot->dn->full_name, slot->index, slot->name, + __func__, slot->dn->full_name, slot->index,slot_name (slot), slot->power_domain, slot->type);/* should not try to register the same slot twice */ if (is_registered (slot)) { -err ("rpaphp_register_slot: slot[%s] is already registered\n" , slot->name); +err ("rpaphp_register_slot: slot[%s] is already registered\n" ,slot_name (slot));return -EAGAIN ; }
@@ -137,7 +129,7 @@ int rpaphp_register_slot(struct slot *slot) slotno =PCI_SLOT (PCI_DN (slot->dn->child)->devfn); else slotno =-1 ; - retval =pci_hp_register (php_slot, slot->bus, slotno, slot->name); + retval =pci_hp_register (php_slot, slot->bus, slotno, name);if (retval) {err ("pci_hp_register failed with error %d\n" , retval);return retval;
@@ -145,7 +137,7 @@ int rpaphp_register_slot(struct slot *slot)/* add slot to our internal list */ list_add (&slot->rpaphp_slot_list, &rpaphp_slot_head); -info ("Slot [%s] registered\n" , slot->name); +info ("Slot [%s] registered\n" , name);return 0 ; }
PATCH v3 11/14 PCI: SGI Hotplug: stop managing bss_hotplug_slot->name
In reply to Alex ChiangWe no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.
Update the sn_hp_slot_private_alloc() interface to fill in the correct name for us, as that function already has all the parameters needed to determine the name.
---
drivers/pci/hotplug/sgi_hotplug.c | 19 ++++++------------- 1 files changed, 6 insertions(+), 13 deletions(-)
drivers/pci/hotplug/sgi_hotplug.c +6 -13
@@ -161,7 +161,8 @@ static int sn_pci_bus_valid(struct pci_bus *pci_bus) }static int sn_hp_slot_private_alloc (struct hotplug_slot *bss_hotplug_slot, -struct pci_bus *pci_bus,int device) +struct pci_bus *pci_bus,int device, +char *name) {struct pcibus_info *pcibus_info;struct slot *slot;
@@ -173,15 +174,9 @@ static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot,return -ENOMEM ; bss_hotplug_slot->private = slot; - bss_hotplug_slot->name =kmalloc (SN_SLOT_NAME_SIZE ,GFP_KERNEL ); -if (!bss_hotplug_slot->name) { -kfree (bss_hotplug_slot->private); -return -ENOMEM ; - } - slot->device_num = device; slot->pci_bus = pci_bus; -sprintf (bss_hotplug_slot->name,"%04x:%02x:%02x" , +sprintf (name,"%04x:%02x:%02x" ,pci_domain_nr (pci_bus), ((u16 )pcibus_info->pbi_buscommon.bs_persist_busnum), device +1 );
@@ -608,7 +603,6 @@ static inline int get_power_status(struct hotplug_slot *bss_hotplug_slot,static void sn_release_slot (struct hotplug_slot *bss_hotplug_slot) {kfree (bss_hotplug_slot->info); -kfree (bss_hotplug_slot->name);kfree (bss_hotplug_slot->private);kfree (bss_hotplug_slot); }
@@ -618,6 +612,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)int device;struct pci_slot *pci_slot;struct hotplug_slot *bss_hotplug_slot; +char name[SN_SLOT_NAME_SIZE ];int rc =0 ;/*
@@ -645,16 +640,14 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus) }if (sn_hp_slot_private_alloc (bss_hotplug_slot, - pci_bus, device)) { + pci_bus, device, name)) { rc = -ENOMEM ; goto alloc_err; } - bss_hotplug_slot->ops = &sn_hotplug_slot_ops; bss_hotplug_slot->release = &sn_release_slot; - rc =pci_hp_register (bss_hotplug_slot, pci_bus, device, - bss_hotplug_slot->name); + rc =pci_hp_register (bss_hotplug_slot, pci_bus, device, name);if (rc) goto register_err;
PATCH v3 12/14 PCI: shcphp: remove 'name' parameter
In reply to Alex ChiangWe do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.
Remove 'name' from shpchp's version of struct slot.
This change also removes the unused struct task_event from the slot structure.
---
drivers/pci/hotplug/shpchp.h | 9 +++++-- drivers/pci/hotplug/shpchp_core.c | 28 ++++++++++------------ drivers/pci/hotplug/shpchp_ctrl.c | 48 +++++++++++++++++++------------------ 3 files changed, 43 insertions(+), 42 deletions(-)
drivers/pci/hotplug/shpchp.h +6 -3
@@ -69,15 +69,13 @@ struct slot {u8 state;u8 presence_save;u8 pwr_save; -struct timer_list task_event; -u8 hp_slot;struct controller *ctrl;struct hpc_ops *hpc_ops;struct hotplug_slot *hotplug_slot;struct list_head slot_list; -char name[SLOT_NAME_SIZE ];struct delayed_work work;/* work for button event */ struct mutex lock; +u8 hp_slot; };struct event_info {
@@ -169,6 +167,11 @@ extern void cleanup_slots(struct controller *ctrl);extern void shpchp_queue_pushbutton_work (struct work_struct *work);extern int shpc_init (struct controller *ctrl,struct pci_dev *pdev); +static inline const char *slot_name (struct slot *slot) +{ +return hotplug_slot_name (slot->hotplug_slot); +} +#ifdef CONFIG_ACPI #include <linux/pci-acpi.h> static inline int get_hp_params_from_firmware(struct pci_dev *dev,
drivers/pci/hotplug/shpchp_core.c +13 -15
@@ -89,7 +89,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot) {struct slot *slot = hotplug_slot->private; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));kfree (slot->hotplug_slot->info);kfree (slot->hotplug_slot);
@@ -101,6 +101,7 @@ static int init_slots(struct controller *ctrl)struct slot *slot;struct hotplug_slot *hotplug_slot;struct hotplug_slot_info *info; +char name[SLOT_NAME_SIZE ];int retval = -ENOMEM ;int i;
@@ -119,8 +120,6 @@ static int init_slots(struct controller *ctrl) goto error_hpslot; hotplug_slot->info = info; - hotplug_slot->name = slot->name; - slot->hp_slot = i; slot->ctrl = ctrl; slot->bus = ctrl->pci_dev->subordinate->number;
@@ -133,7 +132,7 @@ static int init_slots(struct controller *ctrl)/* register this slot with the hotplug pci core */ hotplug_slot->private = slot; hotplug_slot->release = &release_slot; -snprintf (slot->name,SLOT_NAME_SIZE ,"%d" , slot->number); +snprintf (name,SLOT_NAME_SIZE ,"%d" , slot->number); hotplug_slot->ops = &shpchp_hotplug_slot_ops;get_power_status (hotplug_slot, &info->power_status);
@@ -145,8 +144,7 @@ static int init_slots(struct controller *ctrl)"slot_device_offset=%x\n" , slot->bus, slot->device, slot->hp_slot, slot->number, ctrl->slot_device_offset); retval =pci_hp_register (slot->hotplug_slot, - ctrl->pci_dev->subordinate, slot->device, - hotplug_slot->name); + ctrl->pci_dev->subordinate, slot->device, name);if (retval) {err ("pci_hp_register failed with error %d\n" , retval); goto error_info;
@@ -189,7 +187,7 @@ static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status) {struct slot *slot =get_slot (hotplug_slot); -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); hotplug_slot->info->attention_status = status; slot->hpc_ops->set_attention_status (slot, status);
@@ -201,7 +199,7 @@ static int enable_slot (struct hotplug_slot *hotplug_slot) {struct slot *slot =get_slot (hotplug_slot); -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));return shpchp_sysfs_enable_slot (slot); }
@@ -210,7 +208,7 @@ static int disable_slot (struct hotplug_slot *hotplug_slot) {struct slot *slot =get_slot (hotplug_slot); -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot));return shpchp_sysfs_disable_slot (slot); }
@@ -220,7 +218,7 @@ static int get_power_status (struct hotplug_slot *hotplug_slot, u8 *value)struct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_power_status (slot, value);if (retval <0 )
@@ -234,7 +232,7 @@ static int get_attention_status (struct hotplug_slot *hotplug_slot, u8 *value)struct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_attention_status (slot, value);if (retval <0 )
@@ -248,7 +246,7 @@ static int get_latch_status (struct hotplug_slot *hotplug_slot, u8 *value)struct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_latch_status (slot, value);if (retval <0 )
@@ -262,7 +260,7 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)struct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_adapter_status (slot, value);if (retval <0 )
@@ -277,7 +275,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,struct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_max_bus_speed (slot, value);if (retval <0 )
@@ -291,7 +289,7 @@ static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_spstruct slot *slot =get_slot (hotplug_slot);int retval; -dbg ("%s - physical_slot = %s\n" , __func__, hotplug_slot->name); +dbg ("%s - physical_slot = %s\n" , __func__,slot_name (slot)); retval = slot->hpc_ops->get_cur_bus_speed (slot, value);if (retval <0 )
drivers/pci/hotplug/shpchp_ctrl.c +24 -24
166 lines not on this page · raw message
PATCH v3 13/14 PCI: Hotplug core: remove 'name'
In reply to Alex ChiangNow that the PCI core manages the 'name' for each individual hotplug driver, and all drivers have been converted to use hotplug_slot_name(), there is no need for the PCI hotplug core to drag around its own copy of name either.
---
drivers/pci/hotplug/pci_hotplug_core.c | 6 +++--- include/linux/pci_hotplug.h | 3 --- 2 files changed, 3 insertions(+), 6 deletions(-)
drivers/pci/hotplug/pci_hotplug_core.c +3 -3
@@ -533,7 +533,7 @@ static struct hotplug_slot *get_slot_from_name (const char *name)spin_lock (&pci_hotplug_slot_list_lock); list_for_each (tmp, &pci_hotplug_slot_list) { slot = list_entry (tmp, struct hotplug_slot, slot_list); -if (strcmp (slot->name, name) ==0 ) +if (strcmp (hotplug_slot_name (slot), name) ==0 ) goto out; } slot =NULL ;
@@ -628,7 +628,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)if (!hotplug)return -ENODEV ; - temp =get_slot_from_name (hotplug->name); + temp =get_slot_from_name (hotplug_slot_name (hotplug));if (temp != hotplug)return -ENODEV ;
@@ -638,7 +638,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug) slot = hotplug->pci_slot;fs_remove_slot (slot); -dbg ("Removed slot %s from the list\n" , hotplug->name); +dbg ("Removed slot %s from the list\n" ,hotplug_slot_name (hotplug)); hotplug->release (hotplug); slot->hotplug =NULL ;
include/linux/pci_hotplug.h +0 -3
@@ -142,8 +142,6 @@ struct hotplug_slot_info {/** * struct hotplug_slot - used to register a physical slot with the hotplug pci core -* @name: the name of the slot being registered. This string must -* be unique amoung slots registered on this system. * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot * @info: pointer to the &struct hotplug_slot_info for the initial values for * this slot.
@@ -153,7 +151,6 @@ struct hotplug_slot_info {* needs. */ struct hotplug_slot { -char *name;struct hotplug_slot_ops *ops;struct hotplug_slot_info *info;void (*release) (struct hotplug_slot *slot);
PATCH v3 14/14 PCI Hotplug: fakephp: add duplicate slot name debugging
In reply to Alex ChiangThe PCI core now manages slot names on behalf of slot detection and slot hotplug drivers, including the handling of duplicate slot names.
We can use the fakephp driver to help test the new functionality. Add a 'dup_slots' module param to force fakephp to create multiple slots with the same name. We can then verify that the PCI core correctly renamed the slots.
sapphire:/sys/bus/pci/slots # modprobe fakephp dup_slots sapphire:/sys/bus/pci/slots # ls
fake fake-10 fake-3 fake-5 fake-7 fake-9
fake-1 fake-2 fake-4 fake-6 fake-8
---
drivers/pci/hotplug/fakephp.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
drivers/pci/hotplug/fakephp.c +7 -2
@@ -69,6 +69,7 @@ struct dummy_slot { };static int debug; +static int dup_slots;static LIST_HEAD (slot_list);static struct workqueue_struct *dummyphp_wq;
@@ -121,7 +122,10 @@ static int add_slot(struct pci_dev *dev)if (!dslot) goto error_info; -snprintf (name,SLOT_NAME_SIZE ,"fake%d" , count++); +if (dup_slots) +snprintf (name,SLOT_NAME_SIZE ,"fake" ); +else +snprintf (name,SLOT_NAME_SIZE ,"fake%d" , count++);dbg ("slot->name = %s\n" , name); slot->ops = &dummy_hotplug_slot_ops; slot->release = &dummy_release;
@@ -375,4 +379,5 @@ MODULE_DESCRIPTION(DRIVER_DESC);MODULE_LICENSE ("GPL" );module_param (debug, bool,S_IRUGO |S_IWUSR );MODULE_PARM_DESC (debug,"Debugging mode enabled or not" ); - +module_param (dup_slots, bool,S_IRUGO |S_IWUSR ); +MODULE_PARM_DESC (dup_slots,"Force duplicate slot names for debugging" );
Hi Alex-san,
Thank you for the updated patches.
I'll review and test them. Please give me several days.
Thanks,
Kenji Kaneshige
Quoted 97 lines · Alex Chiang
Hi Jesse, Kenji, et al,
This is v3 of the series that implements a series of changes that allows the PCI core to manage slot names, rather than individual hotplug drivers.
v2 -> v3:
- incorporate Willy's code review comments
- fix possible memory leak, pointed out by Rolf Eike Beer
- make false name collision detection work for empty slots
- add 'dup_slots'module_paramto fakephp to help debug all this ;)v1 -> v2:
- fix possible false name collisionsThis series applies against linux-next. I've tested this by loading
pci_slotand various hotplug drivers to ensure that the hotplug drivers can successfully override thepci_slotnames.I'm including the previous explanation/justification to remind folks of the raison d'etre for this patchset.
Thanks.
/ac
There are several benefits to this approach:
1) The core can prevent duplicate slot names on systems with broken firmware.
2) Since the kobject core keeps a copy of the slot name, there is no need for each driver to manage a separate copy, especially since the core can rename slots from underneath drivers. We save runtime memory by only referencing the kobject name.
2.a) The PCI hotplug core doesn't need its own reference for slot name either.
3) Individual hotplug drivers become just a little bit simpler by pushing as many
kzalloc()calls for 'name' down into the PCI core as possible.---
Alex Chiang (14):
PCI Hotplug: fakephp: add duplicate slot name debugging
PCI: Hotplug core: remove 'name'
PCI: shcphp: remove 'name' parameter
PCI: SGI Hotplug: stop managingbss_hotplug_slot->name
PCI: rpaphp: stop managinghotplug_slot->name
PCI: pciehp: remove 'name' parameter
PCI: ibmphp: stop managinghotplug_slot->name
PCI: fakephp: remove 'name' parameter
PCI: cpqphp: stop managinghotplug_slot->name
PCI:cpci_hotplug: stop managinghotplug_slot->name
PCI: acpiphp: remove 'name' parameter
PCI, PCI Hotplug: introduceslot_namehelpers
PCI: prevent duplicate slot names
PCI Hotplug core: add 'name' parampci_hp_registerinterfacedrivers/pci/hotplug/acpiphp.h | 9 +- drivers/pci/hotplug/acpiphp_core.c | 32 +++--- drivers/pci/hotplug/cpci_hotplug.h | 6 + drivers/pci/hotplug/cpci_hotplug_core.c | 74 +++++--------- drivers/pci/hotplug/cpci_hotplug_pci.c | 4 - drivers/pci/hotplug/cpqphp.h | 13 +- drivers/pci/hotplug/cpqphp_core.c | 42 ++++---- drivers/pci/hotplug/fakephp.c | 25 +++-- drivers/pci/hotplug/ibmphp.h | 5 - drivers/pci/hotplug/ibmphp_ebda.c | 19 +--- drivers/pci/hotplug/pci_hotplug_core.c | 62 ++++++------ drivers/pci/hotplug/pciehp.h | 9 +- drivers/pci/hotplug/pciehp_core.c | 41 +++----- drivers/pci/hotplug/pciehp_ctrl.c | 53 +++++----- drivers/pci/hotplug/pciehp_hpc.c | 1 drivers/pci/hotplug/rpaphp.h | 10 +- drivers/pci/hotplug/rpaphp_core.c | 6 + drivers/pci/hotplug/rpaphp_pci.c | 4 - drivers/pci/hotplug/rpaphp_slot.c | 26 ++--- drivers/pci/hotplug/sgi_hotplug.c | 18 +-- drivers/pci/hotplug/shpchp.h | 9 +- drivers/pci/hotplug/shpchp_core.c | 42 +++----- drivers/pci/hotplug/shpchp_ctrl.c | 48 ++++----- drivers/pci/slot.c | 167 +++++++++++++++++++++++-------- include/linux/pci.h | 9 +- include/linux/pci_hotplug.h | 13 ++ 26 files changed, 401 insertions(+), 346 deletions(-)--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Alex,
Quoted 3 lines
This series applies against linux-next. I've tested this by loading
pci_slotand various hotplug drivers to ensure that the hotplug drivers can successfully override thepci_slotnames.
It seems that there are some bugs. Pciehp and shpchp modules can not be loaded when the debug option is specified. Segmentation fault occurs with stack dump.
pciehp_probe() or shpchp_probe()
-> init_slots()
-> get_power_status()
-> slot_name() : This is called when the debug option is specified.
-> hotplug_slot_name()
-> pci_slot_name(slot->pci_slot)
~~~~~~~~~~~~~~
Not initialized yet.Signature · 3 lines
Best regards, Taku Izumi <[email protected]>
Hi Taku-san,
* Taku Izumi <[email protected]>:
Quoted 8 lines
Quoted 3 lines
This series applies against linux-next. I've tested this by loading
pci_slotand various hotplug drivers to ensure that the hotplug drivers can successfully override thepci_slotnames.It seems that there are some bugs. Pciehp and shpchp modules can not be loaded when the debug option is specified. Segmentation fault occurs with stack dump.
Quoted 8 lines
pciehp_probe()orshpchp_probe()
->init_slots()-> get_power_status() -> slot_name() : This is called when the debug option is specified. -> hotplug_slot_name() -> pci_slot_name(slot->pci_slot) ~~~~~~~~~~~~~~ Not initialized yet.
Thank you for pointing this out. I've fixed this (and the other drivers) by make sure that the slot names are initialized before we attempt to use them.
Trying to fix rpaphp was too much work for very little gain, so I deleted that patch from my patch series. But the changes to the hotplug core still make sense, so it's ok to have just one oddball left.
I'll wait until Kenji-san sends out his final review comments before sending out v4.
Thanks.
/ac