Hi all,
This is v4 of the series that implements a series of changes
that allows the PCI core to manage slot names, rather than
individual hotplug drivers.
Thank you to Kenji-san and Taku-san for testing and finding my bugs.
Thanks to Willy for helping me out with the locking issues in
pci_hp_register.
Again, I have tested by loading pci_slot, acpiphp, and fakephp in various
combinations, and using the new dup_slots=1 param to fakephp. I also had:
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_SPINLOCK_SLEEP=y
and didn't see any complaints.
This series applies against Jesse's linux-next branch. I just fetched his
branch and peeked at it, and based on the log, my series _should_ apply
with minimal fuzz (but I didn't do a rebase because I didn't think to
check until writing this mail ;).
It would be great if folks could give one last (hopefully!) review.
Thanks.
/ac
v3 -> v4:
- Do not access hotplug_slot_name() before name initialization
- Serialize pci_hp_register/deregister
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
---
Alex Chiang (15):
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: kmalloc/kfree slot->name directly
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: serialize pci_hp_register/deregister
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 | 75 +++++--------
drivers/pci/hotplug/cpci_hotplug_pci.c | 4 -
drivers/pci/hotplug/cpqphp.h | 13 +-
drivers/pci/hotplug/cpqphp_core.c | 43 +++----
drivers/pci/hotplug/fakephp.c | 26 +++-
drivers/pci/hotplug/ibmphp.h | 5 -
drivers/pci/hotplug/ibmphp_ebda.c | 19 +--
drivers/pci/hotplug/pci_hotplug_core.c | 68 ++++++------
drivers/pci/hotplug/pciehp.h | 9 +-
drivers/pci/hotplug/pciehp_core.c | 49 +++-----
drivers/pci/hotplug/pciehp_ctrl.c | 53 +++++----
drivers/pci/hotplug/pciehp_hpc.c | 1
drivers/pci/hotplug/rpaphp_slot.c | 9 +-
drivers/pci/hotplug/sgi_hotplug.c | 18 +--
drivers/pci/hotplug/shpchp.h | 9 +-
drivers/pci/hotplug/shpchp_core.c | 52 +++------
drivers/pci/hotplug/shpchp_ctrl.c | 48 ++++----
drivers/pci/slot.c | 181 +++++++++++++++++++++++--------
include/linux/pci.h | 9 +-
include/linux/pci_hotplug.h | 11 +-
23 files changed, 407 insertions(+), 342 deletions(-)
Add explicit locking to pci_hp_register/deregister. For a given pci_slot,
we want to make sure that the pci_slot->hotplug pointer isn't being
changed from underneath us.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/pci_hotplug_core.c | 7 +++++++
drivers/pci/slot.c | 4 ++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 0e7a511..e00266b 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -37,6 +37,7 @@
#include <linux/init.h>
#include <linux/mount.h>
#include <linux/namei.h>
+#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
#include <asm/uaccess.h>
@@ -554,6 +555,7 @@ out:
*
* Returns 0 if successful, anything else for an error.
*/
+static DEFINE_MUTEX(pci_hp_mutex);
int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
const char *name)
{
@@ -583,7 +585,9 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
if (IS_ERR(pci_slot))
return PTR_ERR(pci_slot);
+ mutex_lock(&pci_hp_mutex);
if (pci_slot->hotplug) {
+ mutex_unlock(&pci_hp_mutex);
dbg("%s: already claimed\n", __func__);
pci_destroy_slot(pci_slot);
return -EBUSY;
@@ -591,6 +595,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
slot->pci_slot = pci_slot;
pci_slot->hotplug = slot;
+ mutex_unlock(&pci_hp_mutex);
/*
* Allow pcihp drivers to override the ACPI_PCI_SLOT name.
@@ -635,6 +640,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
if (temp != hotplug)
return -ENODEV;
+ mutex_lock(&pci_hp_mutex);
spin_lock(&pci_hotplug_slot_list_lock);
list_del(&hotplug->slot_list);
spin_unlock(&pci_hotplug_slot_list_lock);
@@ -645,6 +651,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
hotplug->release(hotplug);
slot->hotplug = NULL;
+ mutex_unlock(&pci_hp_mutex);
pci_destroy_slot(slot);
return 0;
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 0c6db03..1fffb27 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -164,10 +164,10 @@ placeholder:
pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
__func__, pci_domain_nr(parent), parent->number, slot_nr);
- out:
+out:
up_write(&pci_bus_sem);
return slot;
- err:
+err:
kfree(slot);
slot = ERR_PTR(err);
goto out;
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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
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 | 3 ++-
11 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 0e496e8..e984176 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -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) {
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index 9359479..5e5dee8 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -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;
diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index 54defec..a7fe458 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -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;
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 146ca9c..3069f21 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -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;
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index 8f286ac..85528d6 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -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 ();
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 27d2b6f..0e7a511 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -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;
}
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index c748a19..3ace5e0 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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
diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 5088450..736d3b4 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -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;
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 410fe03..6d20bbd 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -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;
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index cc38615..bf50966 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -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
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index a08cd06..5efba66 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -165,7 +165,8 @@ 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);
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
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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/pci_hotplug_core.c | 52 ++++-----
drivers/pci/hotplug/pciehp_core.c | 15 ---
drivers/pci/hotplug/shpchp_core.c | 15 ---
drivers/pci/slot.c | 177 ++++++++++++++++++++++++--------
include/linux/pci.h | 4 +
5 files changed, 160 insertions(+), 103 deletions(-)
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index e00266b..b196ea1 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -572,41 +572,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);
-
mutex_lock(&pci_hp_mutex);
- if (pci_slot->hotplug) {
- mutex_unlock(&pci_hp_mutex);
- 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;
- mutex_unlock(&pci_hp_mutex);
-
- /*
- * 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);
@@ -616,7 +607,12 @@ 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:
+ mutex_unlock(&pci_hp_mutex);
return result;
+err:
+ pci_destroy_slot(pci_slot);
+ goto out;
}
/**
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 3ace5e0..af89d7b 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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;
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index bf50966..cfdd079 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -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;
}
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 1fffb27..e6af4f9 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -78,6 +78,73 @@ static struct kobj_type pci_slot_ktype = {
.default_attrs = pci_slot_default_attrs,
};
+static char *make_slot_name(const char *name)
+{
+ char *new_name;
+ int len, max, dup;
+
+ new_name = kstrdup(name, GFP_KERNEL);
+ if (!new_name)
+ return NULL;
+
+ /*
+ * Make sure we hit the realloc case the first time through the
+ * loop. 'len' will be strlen(name) + 3 at that point which is
+ * enough space for "name-X" and the trailing NUL.
+ */
+ len = strlen(name) + 2;
+ max = 1;
+ dup = 1;
+
+ for (;;) {
+ struct kobject *dup_slot;
+ dup_slot = kset_find_obj(pci_slots_kset, new_name);
+ if (!dup_slot)
+ break;
+ kobject_put(dup_slot);
+ if (dup == max) {
+ len++;
+ max *= 10;
+ kfree(new_name);
+ new_name = kmalloc(len, GFP_KERNEL);
+ if (!new_name)
+ break;
+ }
+ sprintf(new_name, "%s-%d", name, dup++);
+ }
+
+ return new_name;
+}
+
+/**
+ * pci_get_pci_slot - search for and get a %struct pci_slot
+ * @parent: %struct pci_bus of parent bridge
+ * @slot_nr: PCI_SLOT(pci_dev->devfn) we're looking for
+ *
+ * Search a %struct pci_bus for a given @slot_nr. If we find one,
+ * increment its refcount and return it. Callers are responsible for
+ * calling pci_destroy_slot() if we find one.
+ *
+ * If we don't find it, return NULL.
+ */
+struct pci_slot *pci_get_pci_slot(struct pci_bus *parent, int slot_nr)
+{
+ struct pci_slot *slot;
+
+ down_read(&pci_bus_sem);
+ list_for_each_entry(slot, &parent->slots, list)
+ if (slot->number == slot_nr)
+ goto out;
+
+ slot = NULL;
+out:
+ if (slot)
+ kobject_get(&slot->kobj);
+ up_read(&pci_bus_sem);
+ return slot;
+}
+EXPORT_SYMBOL_GPL(pci_get_pci_slot);
+
/**
* pci_create_slot - create or increment refcount for physical PCI slot
* @parent: struct pci_bus of parent bridge
@@ -89,7 +156,19 @@ static struct kobj_type pci_slot_ktype = {
* either return a new &struct pci_slot to the caller, or if the pci_slot
* already exists, its refcount will be incremented.
*
- * Slots are uniquely identified by a @pci_bus, @slot_nr, @name tuple.
+ * Slots are uniquely identified by a @pci_bus, @slot_nr tuple.
+ *
+ * The kobject API imposes a restriction on us, and does not allow sysfs
+ * entries with duplicate names. There are known platforms with broken
+ * firmware that assign the same name to multiple slots.
+ *
+ * We workaround these broken platforms by renaming the slots on behalf
+ * of the caller. If firmware assigns name N to multiple slots:
+ *
+ * The first slot is assigned N
+ * The second slot is assigned N-1
+ * The third slot is assigned N-2
+ * etc.
*
* Placeholder slots:
* In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify
@@ -98,44 +177,27 @@ static struct kobj_type pci_slot_ktype = {
* the slot. In this scenario, the caller may pass -1 for @slot_nr.
*
* The following semantics are imposed when the caller passes @slot_nr ==
- * -1. First, the check for existing %struct pci_slot is skipped, as the
- * caller may know about several unpopulated slots on a given %struct
- * pci_bus, and each slot would have a @slot_nr of -1. Uniqueness for
- * these slots is then determined by the @name parameter. We expect
- * kobject_init_and_add() to warn us if the caller attempts to create
- * multiple slots with the same name. The other change in semantics is
- * user-visible, which is the 'address' parameter presented in sysfs will
- * consist solely of a dddd:bb tuple, where dddd is the PCI domain of the
- * %struct pci_bus and bb is the bus number. In other words, the devfn of
- * the 'placeholder' slot will not be displayed.
+ * -1. First, we no longer check for an existing %struct pci_slot, as there
+ * may be many slots with @slot_nr of -1. The other change in semantics is
+ * user-visible, which is the 'address' parameter presented in sysfs will
+ * consist solely of a dddd:bb tuple, where dddd is the PCI domain of the
+ * %struct pci_bus and bb is the bus number. In other words, the devfn of
+ * the 'placeholder' slot will not be displayed.
*/
-
struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
const char *name)
{
struct pci_dev *dev;
struct pci_slot *slot;
- int err;
+ int err = 0;
+ char *slot_name = NULL;
- down_write(&pci_bus_sem);
-
- if (slot_nr == -1)
- goto placeholder;
-
- /* If we've already created this slot, bump refcount and return. */
- list_for_each_entry(slot, &parent->slots, list) {
- if (slot->number == slot_nr) {
- kobject_get(&slot->kobj);
- pr_debug("%s: inc refcount to %d on %04x:%02x:%02x\n",
- __func__,
- atomic_read(&slot->kobj.kref.refcount),
- pci_domain_nr(parent), parent->number,
- slot_nr);
+ if (slot_nr != -1) {
+ slot = pci_get_pci_slot(parent, slot_nr);
+ if (slot)
goto out;
- }
}
-placeholder:
slot = kzalloc(sizeof(*slot), GFP_KERNEL);
if (!slot) {
slot = ERR_PTR(-ENOMEM);
@@ -144,28 +206,35 @@ placeholder:
slot->bus = parent;
slot->number = slot_nr;
-
slot->kobj.kset = pci_slots_kset;
+
+ slot_name = make_slot_name(name);
+ if (!slot_name) {
+ err = -ENOMEM;
+ goto err;
+ }
+
err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
- "%s", name);
+ "%s", slot_name);
if (err) {
- printk(KERN_ERR "Unable to register kobject %s\n", name);
+ printk(KERN_ERR "Unable to register kobject %s\n", slot_name);
goto err;
}
+ down_write(&pci_bus_sem);
INIT_LIST_HEAD(&slot->list);
list_add(&slot->list, &parent->slots);
-
list_for_each_entry(dev, &parent->devices, bus_list)
if (PCI_SLOT(dev->devfn) == slot_nr)
dev->slot = slot;
+ up_write(&pci_bus_sem);
/* Don't care if debug printk has a -1 for slot_nr */
pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
__func__, pci_domain_nr(parent), parent->number, slot_nr);
out:
- up_write(&pci_bus_sem);
+ kfree(slot_name);
return slot;
err:
kfree(slot);
@@ -175,7 +244,31 @@ err:
EXPORT_SYMBOL_GPL(pci_create_slot);
/**
- * pci_update_slot_number - update %struct pci_slot -> number
+ * pci_rename_slot - update %struct pci_slot -> name
+ * @slot - %struct pci_slot to update
+ * @name - new requested name for slot
+ *
+ * Allow caller to safely rename a %struct pci_slot without making
+ * the kobject core complain about duplicate names.
+ */
+int pci_rename_slot(struct pci_slot *slot, const char *name)
+{
+ int result;
+ char *slot_name;
+
+ slot_name = make_slot_name(name);
+ if (!slot_name)
+ return -ENOMEM;
+
+ result = kobject_rename(&slot->kobj, slot_name);
+ kfree(slot_name);
+
+ return result;
+}
+EXPORT_SYMBOL_GPL(pci_rename_slot);
+
+/**
+ * pci_renumber_slot - update %struct pci_slot -> number
* @slot - %struct pci_slot to update
* @slot_nr - new number for slot
*
@@ -183,27 +276,22 @@ EXPORT_SYMBOL_GPL(pci_create_slot);
* created a placeholder slot in pci_create_slot() by passing a -1 as
* slot_nr, to update their %struct pci_slot with the correct @slot_nr.
*/
-
-void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
+void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
{
- int name_count = 0;
struct pci_slot *tmp;
down_write(&pci_bus_sem);
list_for_each_entry(tmp, &slot->bus->slots, list) {
WARN_ON(tmp->number == slot_nr);
- if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
- name_count++;
+ goto out;
}
- if (name_count > 1)
- printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
-
slot->number = slot_nr;
+out:
up_write(&pci_bus_sem);
}
-EXPORT_SYMBOL_GPL(pci_update_slot_number);
+EXPORT_SYMBOL_GPL(pci_renumber_slot);
/**
* pci_destroy_slot - decrement refcount for physical PCI slot
@@ -213,7 +301,6 @@ EXPORT_SYMBOL_GPL(pci_update_slot_number);
* just call kobject_put on its kobj and let our release methods do the
* rest.
*/
-
void pci_destroy_slot(struct pci_slot *slot)
{
pr_debug("%s: dec refcount to %d on %04x:%02x:%02x\n", __func__,
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 79dc7ce..5617a4d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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);
In 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
include/linux/pci.h | 5 +++++
include/linux/pci_hotplug.h | 5 +++++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 5617a4d..8674257 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -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,
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index 5efba66..a3a3245 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -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,
const char *name);
extern int pci_hp_deregister(struct hotplug_slot *slot);
We 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/acpiphp.h | 9 +++++----
drivers/pci/hotplug/acpiphp_core.c | 31 ++++++++++++++++---------------
2 files changed, 21 insertions(+), 19 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 5a58b07..f9e244d 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -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
*
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index e984176..95b536a 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -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);
+ snprintf(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)
We 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.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/cpqphp.h | 13 ++++-------
drivers/pci/hotplug/cpqphp_core.c | 42 +++++++++++++++++--------------------
2 files changed, 24 insertions(+), 31 deletions(-)
diff --git a/drivers/pci/hotplug/cpqphp.h b/drivers/pci/hotplug/cpqphp.h
index b1decfa..afaf8f6 100644
--- a/drivers/pci/hotplug/cpqphp.h
+++ b/drivers/pci/hotplug/cpqphp.h
@@ -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 *slot
return 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);
diff --git a/drivers/pci/hotplug/cpqphp_core.c b/drivers/pci/hotplug/cpqphp_core.c
index a7fe458..724d42c 100644
--- a/drivers/pci/hotplug/cpqphp_core.c
+++ b/drivers/pci/hotplug/cpqphp_core.c
@@ -315,14 +315,15 @@ 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->name);
kfree(slot->hotplug_slot);
kfree(slot);
}
+#define SLOT_NAME_SIZE 10
+
static int ctrl_slot_setup(struct controller *ctrl,
void __iomem *smbios_start,
void __iomem *smbios_table)
@@ -335,6 +336,7 @@ static int ctrl_slot_setup(struct controller *ctrl,
u8 slot_number;
u8 ctrl_slot;
u32 tempdword;
+ char name[SLOT_NAME_SIZE];
void __iomem *slot_entry= NULL;
int result = -ENOMEM;
@@ -363,16 +365,12 @@ static int ctrl_slot_setup(struct controller *ctrl,
if (!hotplug_slot->info)
goto error_hpslot;
hotplug_slot_info = hotplug_slot->info;
- hotplug_slot->name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
-
- if (!hotplug_slot->name)
- goto error_info;
slot->ctrl = ctrl;
slot->bus = ctrl->bus;
slot->device = slot_device;
slot->number = slot_number;
- dbg("slot->number = %d\n", slot->number);
+ dbg("slot->number = %u\n", slot->number);
slot_entry = get_SMBIOS_entry(smbios_start, smbios_table, 9,
slot_entry);
@@ -418,9 +416,9 @@ static int ctrl_slot_setup(struct controller *ctrl,
/* register this slot with the hotplug pci core */
hotplug_slot->release = &release_slot;
hotplug_slot->private = slot;
- make_slot_name(hotplug_slot->name, SLOT_NAME_SIZE, slot);
+ snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
hotplug_slot->ops = &cpqphp_hotplug_slot_ops;
-
+
hotplug_slot_info->power_status = get_slot_enabled(ctrl, slot);
hotplug_slot_info->attention_status =
cpq_get_attention_status(ctrl, slot);
@@ -437,10 +435,10 @@ static int ctrl_slot_setup(struct controller *ctrl,
result = pci_hp_register(hotplug_slot,
ctrl->pci_dev->subordinate,
slot->device,
- hotplug_slot->name);
+ name);
if (result) {
err("pci_hp_register failed with error %d\n", result);
- goto error_name;
+ goto error_info;
}
slot->next = ctrl->slot;
@@ -452,8 +450,6 @@ static int ctrl_slot_setup(struct controller *ctrl,
}
return 0;
-error_name:
- kfree(hotplug_slot->name);
error_info:
kfree(hotplug_slot_info);
error_hpslot:
@@ -639,7 +635,7 @@ static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status)
u8 device;
u8 function;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
return -ENODEV;
@@ -666,7 +662,7 @@ static int process_SI(struct hotplug_slot *hotplug_slot)
u8 device;
u8 function;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
return -ENODEV;
@@ -698,7 +694,7 @@ static int process_SS(struct hotplug_slot *hotplug_slot)
u8 device;
u8 function;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
if (cpqhp_get_bus_dev(ctrl, &bus, &devfn, slot->number) == -1)
return -ENODEV;
@@ -721,7 +717,7 @@ static int hardware_test(struct hotplug_slot *hotplug_slot, u32 value)
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
return cpqhp_hardware_test(ctrl, value);
}
@@ -732,7 +728,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = get_slot_enabled(ctrl, slot);
return 0;
@@ -743,7 +739,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = cpq_get_attention_status(ctrl, slot);
return 0;
@@ -754,7 +750,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = cpq_get_latch_status(ctrl, slot);
@@ -766,7 +762,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = get_presence_status(ctrl, slot);
@@ -778,7 +774,7 @@ static int get_max_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = ctrl->speed_capability;
@@ -790,7 +786,7 @@ static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
struct slot *slot = hotplug_slot->private;
struct controller *ctrl = slot->ctrl;
- dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
*value = ctrl->speed;
We 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/cpci_hotplug.h | 6 ++
drivers/pci/hotplug/cpci_hotplug_core.c | 76 ++++++++++++-------------------
drivers/pci/hotplug/cpci_hotplug_pci.c | 4 +-
3 files changed, 37 insertions(+), 49 deletions(-)
diff --git a/drivers/pci/hotplug/cpci_hotplug.h b/drivers/pci/hotplug/cpci_hotplug.h
index d9769b3..9fff878 100644
--- a/drivers/pci/hotplug/cpci_hotplug.h
+++ b/drivers/pci/hotplug/cpci_hotplug.h
@@ -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);
diff --git a/drivers/pci/hotplug/cpci_hotplug_core.c b/drivers/pci/hotplug/cpci_hotplug_core.c
index 5e5dee8..de94f4f 100644
--- a/drivers/pci/hotplug/cpci_hotplug_core.c
+++ b/drivers/pci/hotplug/cpci_hotplug_core.c
@@ -108,7 +108,7 @@ enable_slot(struct hotplug_slot *hotplug_slot)
struct slot *slot = hotplug_slot->private;
int retval = 0;
- dbg("%s - physical_slot = %s", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s", __func__, slot_name(slot));
if (controller->ops->set_power)
retval = controller->ops->set_power(slot, 1);
@@ -121,25 +121,23 @@ disable_slot(struct hotplug_slot *hotplug_slot)
struct slot *slot = hotplug_slot->private;
int retval = 0;
- dbg("%s - physical_slot = %s", __func__, hotplug_slot->name);
+ dbg("%s - physical_slot = %s", __func__, slot_name(slot));
down_write(&list_rwsem);
/* Unconfigure device */
- dbg("%s - unconfiguring slot %s",
- __func__, slot->hotplug_slot->name);
+ dbg("%s - unconfiguring slot %s", __func__, slot_name(slot));
if ((retval = cpci_unconfigure_slot(slot))) {
err("%s - could not unconfigure slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
goto disable_error;
}
- dbg("%s - finished unconfiguring slot %s",
- __func__, slot->hotplug_slot->name);
+ dbg("%s - finished unconfiguring slot %s", __func__, slot_name(slot));
/* Clear EXT (by setting it) */
if (cpci_clear_ext(slot)) {
err("%s - could not clear EXT for slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
retval = -ENODEV;
goto disable_error;
}
@@ -214,7 +212,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
struct slot *slot = hotplug_slot->private;
kfree(slot->hotplug_slot->info);
- kfree(slot->hotplug_slot->name);
kfree(slot->hotplug_slot);
if (slot->dev)
pci_dev_put(slot->dev);
@@ -222,12 +219,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
}
#define SLOT_NAME_SIZE 6
-static void
-make_slot_name(struct slot *slot)
-{
- snprintf(slot->hotplug_slot->name,
- SLOT_NAME_SIZE, "%02x:%02x", slot->bus->number, slot->number);
-}
int
cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
@@ -235,7 +226,7 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
struct slot *slot;
struct hotplug_slot *hotplug_slot;
struct hotplug_slot_info *info;
- char *name;
+ char name[SLOT_NAME_SIZE];
int status = -ENOMEM;
int i;
@@ -262,35 +253,31 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
goto error_hpslot;
hotplug_slot->info = info;
- name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
- if (!name)
- goto error_info;
- hotplug_slot->name = name;
-
slot->bus = bus;
slot->number = i;
slot->devfn = PCI_DEVFN(i, 0);
+ snprintf(name, SLOT_NAME_SIZE, "%02x:%02x", bus->number, i);
+
hotplug_slot->private = slot;
hotplug_slot->release = &release_slot;
- make_slot_name(slot);
hotplug_slot->ops = &cpci_hotplug_slot_ops;
/*
* Initialize the slot info structure with some known
* good values.
*/
- dbg("initializing slot %s", slot->hotplug_slot->name);
+ dbg("initializing slot %s", name);
info->power_status = cpci_get_power_status(slot);
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,
- slot->hotplug_slot->name);
+ dbg("registering slot %s", name);
+ status = pci_hp_register(slot->hotplug_slot, bus, i, name);
if (status) {
err("pci_hp_register failed with error %d", status);
- goto error_name;
+ goto error_info;
}
+ dbg("slot registered with name: %s", slot_name(slot));
/* Add slot to our internal list */
down_write(&list_rwsem);
@@ -299,8 +286,6 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
up_write(&list_rwsem);
}
return 0;
-error_name:
- kfree(name);
error_info:
kfree(info);
error_hpslot:
@@ -328,7 +313,7 @@ cpci_hp_unregister_bus(struct pci_bus *bus)
list_del(&slot->slot_list);
slots--;
- dbg("deregistering slot %s", slot->hotplug_slot->name);
+ dbg("deregistering slot %s", slot_name(slot));
status = pci_hp_deregister(slot->hotplug_slot);
if (status) {
err("pci_hp_deregister failed with error %d",
@@ -380,11 +365,10 @@ init_slots(int clear_ins)
return -1;
}
list_for_each_entry(slot, &slot_list, slot_list) {
- dbg("%s - looking at slot %s",
- __func__, slot->hotplug_slot->name);
+ dbg("%s - looking at slot %s", __func__, slot_name(slot));
if (clear_ins && cpci_check_and_clear_ins(slot))
dbg("%s - cleared INS for slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
dev = pci_get_slot(slot->bus, PCI_DEVFN(slot->number, 0));
if (dev) {
if (update_adapter_status(slot->hotplug_slot, 1))
@@ -415,8 +399,7 @@ check_slots(void)
}
extracted = inserted = 0;
list_for_each_entry(slot, &slot_list, slot_list) {
- dbg("%s - looking at slot %s",
- __func__, slot->hotplug_slot->name);
+ dbg("%s - looking at slot %s", __func__, slot_name(slot));
if (cpci_check_and_clear_ins(slot)) {
/*
* Some broken hardware (e.g. PLX 9054AB) asserts
@@ -424,35 +407,34 @@ check_slots(void)
*/
if (slot->dev) {
warn("slot %s already inserted",
- slot->hotplug_slot->name);
+ slot_name(slot));
inserted++;
continue;
}
/* Process insertion */
- dbg("%s - slot %s inserted",
- __func__, slot->hotplug_slot->name);
+ dbg("%s - slot %s inserted", __func__, slot_name(slot));
/* GSM, debug */
hs_csr = cpci_get_hs_csr(slot);
dbg("%s - slot %s HS_CSR (1) = %04x",
- __func__, slot->hotplug_slot->name, hs_csr);
+ __func__, slot_name(slot), hs_csr);
/* Configure device */
dbg("%s - configuring slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
if (cpci_configure_slot(slot)) {
err("%s - could not configure slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
continue;
}
dbg("%s - finished configuring slot %s",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
/* GSM, debug */
hs_csr = cpci_get_hs_csr(slot);
dbg("%s - slot %s HS_CSR (2) = %04x",
- __func__, slot->hotplug_slot->name, hs_csr);
+ __func__, slot_name(slot), hs_csr);
if (update_latch_status(slot->hotplug_slot, 1))
warn("failure to update latch file");
@@ -465,18 +447,18 @@ check_slots(void)
/* GSM, debug */
hs_csr = cpci_get_hs_csr(slot);
dbg("%s - slot %s HS_CSR (3) = %04x",
- __func__, slot->hotplug_slot->name, hs_csr);
+ __func__, slot_name(slot), hs_csr);
inserted++;
} else if (cpci_check_ext(slot)) {
/* Process extraction request */
dbg("%s - slot %s extracted",
- __func__, slot->hotplug_slot->name);
+ __func__, slot_name(slot));
/* GSM, debug */
hs_csr = cpci_get_hs_csr(slot);
dbg("%s - slot %s HS_CSR = %04x",
- __func__, slot->hotplug_slot->name, hs_csr);
+ __func__, slot_name(slot), hs_csr);
if (!slot->extracting) {
if (update_latch_status(slot->hotplug_slot, 0)) {
@@ -494,7 +476,7 @@ check_slots(void)
* bother trying to tell the driver or not?
*/
err("card in slot %s was improperly removed",
- slot->hotplug_slot->name);
+ slot_name(slot));
if (update_adapter_status(slot->hotplug_slot, 0))
warn("failure to update adapter file");
slot->extracting = 0;
diff --git a/drivers/pci/hotplug/cpci_hotplug_pci.c b/drivers/pci/hotplug/cpci_hotplug_pci.c
index df82b95..829c327 100644
--- a/drivers/pci/hotplug/cpci_hotplug_pci.c
+++ b/drivers/pci/hotplug/cpci_hotplug_pci.c
@@ -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;
}
}
Remove 'name' from fakephp's struct dummy_slot, as the PCI core
will now manage our slot name for us.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/fakephp.c | 19 ++++++++++---------
1 files changed, 10 insertions(+), 9 deletions(-)
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 3069f21..24dcbf1 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -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,20 +121,18 @@ 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++);
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;
}
+ dbg("slot->name = %s\n", hotplug_slot_name(slot));
dslot->slot = slot;
dslot->dev = pci_dev_get(dev);
list_add (&dslot->node, &slot_list);
@@ -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);
We 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.
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/ibmphp.h | 5 ++---
drivers/pci/hotplug/ibmphp_ebda.c | 20 +++++++-------------
2 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/pci/hotplug/ibmphp.h b/drivers/pci/hotplug/ibmphp.h
index 612d963..a8d391a 100644
--- a/drivers/pci/hotplug/ibmphp.h
+++ b/drivers/pci/hotplug/ibmphp.h
@@ -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;
diff --git a/drivers/pci/hotplug/ibmphp_ebda.c b/drivers/pci/hotplug/ibmphp_ebda.c
index 85528d6..402dc10 100644
--- a/drivers/pci/hotplug/ibmphp_ebda.c
+++ b/drivers/pci/hotplug/ibmphp_ebda.c
@@ -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);
We 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/pciehp.h | 9 ++++--
drivers/pci/hotplug/pciehp_core.c | 33 ++++++++++++-----------
drivers/pci/hotplug/pciehp_ctrl.c | 53 ++++++++++++++++++++-----------------
drivers/pci/hotplug/pciehp_hpc.c | 1 -
4 files changed, 52 insertions(+), 44 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index c367978..394f998 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -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;
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index af89d7b..62be1b5 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -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,15 +210,11 @@ 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;
- get_power_status(hotplug_slot, &info->power_status);
- get_attention_status(hotplug_slot, &info->attention_status);
- 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,12 +222,16 @@ 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);
goto error_info;
}
+ get_power_status(hotplug_slot, &info->power_status);
+ get_attention_status(hotplug_slot, &info->attention_status);
+ get_latch_status(hotplug_slot, &info->latch_status);
+ get_adapter_status(hotplug_slot, &info->adapter_status);
/* create additional sysfs entries */
if (EMI(ctrl)) {
retval = sysfs_create_file(&hotplug_slot->pci_slot->kobj,
@@ -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_spe
int 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)
diff --git a/drivers/pci/hotplug/pciehp_ctrl.c b/drivers/pci/hotplug/pciehp_ctrl.c
index acb7f9e..cce6e5f 100644
--- a/drivers/pci/hotplug/pciehp_ctrl.c
+++ b/drivers/pci/hotplug/pciehp_ctrl.c
@@ -66,7 +66,7 @@ u8 pciehp_handle_attention_button(struct slot *p_slot)
/*
* Button pressed - See if need to TAKE ACTION!!!
*/
- ctrl_info(ctrl, "Button pressed on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Button pressed on Slot(%s)\n", slot_name(p_slot));
event_type = INT_BUTTON_PRESS;
queue_interrupt_event(p_slot, event_type);
@@ -88,13 +88,13 @@ u8 pciehp_handle_switch_change(struct slot *p_slot)
/*
* Switch opened
*/
- ctrl_info(ctrl, "Latch open on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Latch open on Slot(%s)\n", slot_name(p_slot));
event_type = INT_SWITCH_OPEN;
} else {
/*
* Switch closed
*/
- ctrl_info(ctrl, "Latch close on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Latch close on Slot(%s)\n", slot_name(p_slot));
event_type = INT_SWITCH_CLOSE;
}
@@ -120,13 +120,14 @@ u8 pciehp_handle_presence_change(struct slot *p_slot)
/*
* Card Present
*/
- ctrl_info(ctrl, "Card present on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Card present on Slot(%s)\n", slot_name(p_slot));
event_type = INT_PRESENCE_ON;
} else {
/*
* Not Present
*/
- ctrl_info(ctrl, "Card not present on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Card not present on Slot(%s)\n",
+ slot_name(p_slot));
event_type = INT_PRESENCE_OFF;
}
@@ -148,13 +149,13 @@ u8 pciehp_handle_power_fault(struct slot *p_slot)
* power fault Cleared
*/
ctrl_info(ctrl, "Power fault cleared on Slot(%s)\n",
- p_slot->name);
+ slot_name(p_slot));
event_type = INT_POWER_FAULT_CLEAR;
} else {
/*
* power fault
*/
- ctrl_info(ctrl, "Power fault on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Power fault on Slot(%s)\n", slot_name(p_slot));
event_type = INT_POWER_FAULT;
ctrl_info(ctrl, "power fault bit %x set\n", 0);
}
@@ -412,12 +413,12 @@ static void handle_button_press_event(struct slot *p_slot)
p_slot->state = BLINKINGOFF_STATE;
ctrl_info(ctrl,
"PCI slot #%s - powering off due to button "
- "press.\n", p_slot->name);
+ "press.\n", slot_name(p_slot));
} else {
p_slot->state = BLINKINGON_STATE;
ctrl_info(ctrl,
"PCI slot #%s - powering on due to button "
- "press.\n", p_slot->name);
+ "press.\n", slot_name(p_slot));
}
/* blink green LED and turn off amber */
if (PWR_LED(ctrl))
@@ -434,7 +435,7 @@ static void handle_button_press_event(struct slot *p_slot)
* press the attention again before the 5 sec. limit
* expires to cancel hot-add or hot-remove
*/
- ctrl_info(ctrl, "Button cancel on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Button cancel on Slot(%s)\n", slot_name(p_slot));
ctrl_dbg(ctrl, "%s: button cancel\n", __func__);
cancel_delayed_work(&p_slot->work);
if (p_slot->state == BLINKINGOFF_STATE) {
@@ -447,7 +448,7 @@ static void handle_button_press_event(struct slot *p_slot)
if (ATTN_LED(ctrl))
p_slot->hpc_ops->set_attention_status(p_slot, 0);
ctrl_info(ctrl, "PCI slot #%s - action canceled "
- "due to button press\n", p_slot->name);
+ "due to button press\n", slot_name(p_slot));
p_slot->state = STATIC_STATE;
break;
case POWEROFF_STATE:
@@ -457,7 +458,7 @@ static void handle_button_press_event(struct slot *p_slot)
* this means that the previous attention button action
* to hot-add or hot-remove is undergoing
*/
- ctrl_info(ctrl, "Button ignore on Slot(%s)\n", p_slot->name);
+ ctrl_info(ctrl, "Button ignore on Slot(%s)\n", slot_name(p_slot));
update_slot_info(p_slot);
break;
default:
@@ -540,7 +541,7 @@ int pciehp_enable_slot(struct slot *p_slot)
rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
if (rc || !getstatus) {
ctrl_info(ctrl, "%s: no adapter on slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -ENODEV;
}
@@ -548,7 +549,7 @@ int pciehp_enable_slot(struct slot *p_slot)
rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
if (rc || getstatus) {
ctrl_info(ctrl, "%s: latch open on slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -ENODEV;
}
@@ -558,7 +559,7 @@ int pciehp_enable_slot(struct slot *p_slot)
rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
if (rc || getstatus) {
ctrl_info(ctrl, "%s: already enabled on slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -EINVAL;
}
@@ -594,7 +595,7 @@ int pciehp_disable_slot(struct slot *p_slot)
ret = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
if (ret || !getstatus) {
ctrl_info(ctrl, "%s: no adapter on slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -ENODEV;
}
@@ -604,7 +605,7 @@ int pciehp_disable_slot(struct slot *p_slot)
ret = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
if (ret || getstatus) {
ctrl_info(ctrl, "%s: latch open on slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -ENODEV;
}
@@ -614,7 +615,7 @@ int pciehp_disable_slot(struct slot *p_slot)
ret = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
if (ret || !getstatus) {
ctrl_info(ctrl, "%s: already disabled slot(%s)\n",
- __func__, p_slot->name);
+ __func__, slot_name(p_slot));
mutex_unlock(&p_slot->ctrl->crit_sect);
return -EINVAL;
}
@@ -645,14 +646,16 @@ int pciehp_sysfs_enable_slot(struct slot *p_slot)
break;
case POWERON_STATE:
ctrl_info(ctrl, "Slot %s is already in powering on state\n",
- p_slot->name);
+ slot_name(p_slot));
break;
case BLINKINGOFF_STATE:
case POWEROFF_STATE:
- ctrl_info(ctrl, "Already enabled on slot %s\n", p_slot->name);
+ ctrl_info(ctrl, "Already enabled on slot %s\n",
+ slot_name(p_slot));
break;
default:
- ctrl_err(ctrl, "Not a valid state on slot %s\n", p_slot->name);
+ ctrl_err(ctrl, "Not a valid state on slot %s\n",
+ slot_name(p_slot));
break;
}
mutex_unlock(&p_slot->lock);
@@ -678,14 +681,16 @@ int pciehp_sysfs_disable_slot(struct slot *p_slot)
break;
case POWEROFF_STATE:
ctrl_info(ctrl, "Slot %s is already in powering off state\n",
- p_slot->name);
+ slot_name(p_slot));
break;
case BLINKINGON_STATE:
case POWERON_STATE:
- ctrl_info(ctrl, "Already disabled on slot %s\n", p_slot->name);
+ ctrl_info(ctrl, "Already disabled on slot %s\n",
+ slot_name(p_slot));
break;
default:
- ctrl_err(ctrl, "Not a valid state on slot %s\n", p_slot->name);
+ ctrl_err(ctrl, "Not a valid state on slot %s\n",
+ slot_name(p_slot));
break;
}
mutex_unlock(&p_slot->lock);
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 8e9530c..4c74d53 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -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);
rpaphp tends to use slot->name directly everywhere, and doesn't
ever need slot->hotplug_slot->name.
struct hotplug_slot->name is going away, so convert rpaphp directly
manipulate its own slot->name everywhere, and don't bother touching
slot->hotplug_slot->name.
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/rpaphp_slot.c | 7 +++----
1 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/hotplug/rpaphp_slot.c b/drivers/pci/hotplug/rpaphp_slot.c
index 736d3b4..7297e85 100644
--- a/drivers/pci/hotplug/rpaphp_slot.c
+++ b/drivers/pci/hotplug/rpaphp_slot.c
@@ -43,7 +43,7 @@ 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->name);
kfree(slot->hotplug_slot);
kfree(slot);
}
@@ -63,10 +63,9 @@ 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)
+ slot->name = kmalloc(strlen(drc_name) + 1, GFP_KERNEL);
+ if (!slot->name)
goto error_info;
- slot->name = slot->hotplug_slot->name;
strcpy(slot->name, drc_name);
slot->dn = dn;
slot->index = drc_index;
We 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/sgi_hotplug.c | 19 ++++++-------------
1 files changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/hotplug/sgi_hotplug.c b/drivers/pci/hotplug/sgi_hotplug.c
index 6d20bbd..d748698 100644
--- a/drivers/pci/hotplug/sgi_hotplug.c
+++ b/drivers/pci/hotplug/sgi_hotplug.c
@@ -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;
We 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/shpchp.h | 9 +++++--
drivers/pci/hotplug/shpchp_core.c | 38 ++++++++++++++---------------
drivers/pci/hotplug/shpchp_ctrl.c | 48 +++++++++++++++++++------------------
3 files changed, 48 insertions(+), 47 deletions(-)
diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index 8a026f7..4d9fed0 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -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,
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index cfdd079..7af9191 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -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,25 +132,24 @@ 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);
- get_attention_status(hotplug_slot, &info->attention_status);
- get_latch_status(hotplug_slot, &info->latch_status);
- get_adapter_status(hotplug_slot, &info->adapter_status);
-
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);
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;
}
+ get_power_status(hotplug_slot, &info->power_status);
+ get_attention_status(hotplug_slot, &info->attention_status);
+ get_latch_status(hotplug_slot, &info->latch_status);
+ get_adapter_status(hotplug_slot, &info->adapter_status);
+
list_add(&slot->slot_list, &ctrl->slot_list);
}
@@ -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_sp
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_cur_bus_speed(slot, value);
if (retval < 0)
diff --git a/drivers/pci/hotplug/shpchp_ctrl.c b/drivers/pci/hotplug/shpchp_ctrl.c
index dfb5393..919b1ee 100644
--- a/drivers/pci/hotplug/shpchp_ctrl.c
+++ b/drivers/pci/hotplug/shpchp_ctrl.c
@@ -70,7 +70,7 @@ u8 shpchp_handle_attention_button(u8 hp_slot, struct controller *ctrl)
/*
* Button pressed - See if need to TAKE ACTION!!!
*/
- info("Button pressed on Slot(%s)\n", p_slot->name);
+ info("Button pressed on Slot(%s)\n", slot_name(p_slot));
event_type = INT_BUTTON_PRESS;
queue_interrupt_event(p_slot, event_type);
@@ -98,7 +98,7 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
/*
* Switch opened
*/
- info("Latch open on Slot(%s)\n", p_slot->name);
+ info("Latch open on Slot(%s)\n", slot_name(p_slot));
event_type = INT_SWITCH_OPEN;
if (p_slot->pwr_save && p_slot->presence_save) {
event_type = INT_POWER_FAULT;
@@ -108,7 +108,7 @@ u8 shpchp_handle_switch_change(u8 hp_slot, struct controller *ctrl)
/*
* Switch closed
*/
- info("Latch close on Slot(%s)\n", p_slot->name);
+ info("Latch close on Slot(%s)\n", slot_name(p_slot));
event_type = INT_SWITCH_CLOSE;
}
@@ -135,13 +135,13 @@ u8 shpchp_handle_presence_change(u8 hp_slot, struct controller *ctrl)
/*
* Card Present
*/
- info("Card present on Slot(%s)\n", p_slot->name);
+ info("Card present on Slot(%s)\n", slot_name(p_slot));
event_type = INT_PRESENCE_ON;
} else {
/*
* Not Present
*/
- info("Card not present on Slot(%s)\n", p_slot->name);
+ info("Card not present on Slot(%s)\n", slot_name(p_slot));
event_type = INT_PRESENCE_OFF;
}
@@ -164,14 +164,14 @@ u8 shpchp_handle_power_fault(u8 hp_slot, struct controller *ctrl)
/*
* Power fault Cleared
*/
- info("Power fault cleared on Slot(%s)\n", p_slot->name);
+ info("Power fault cleared on Slot(%s)\n", slot_name(p_slot));
p_slot->status = 0x00;
event_type = INT_POWER_FAULT_CLEAR;
} else {
/*
* Power fault
*/
- info("Power fault on Slot(%s)\n", p_slot->name);
+ info("Power fault on Slot(%s)\n", slot_name(p_slot));
event_type = INT_POWER_FAULT;
/* set power fault status for this board */
p_slot->status = 0xFF;
@@ -493,11 +493,11 @@ static void handle_button_press_event(struct slot *p_slot)
if (getstatus) {
p_slot->state = BLINKINGOFF_STATE;
info("PCI slot #%s - powering off due to button "
- "press.\n", p_slot->name);
+ "press.\n", slot_name(p_slot));
} else {
p_slot->state = BLINKINGON_STATE;
info("PCI slot #%s - powering on due to button "
- "press.\n", p_slot->name);
+ "press.\n", slot_name(p_slot));
}
/* blink green LED and turn off amber */
p_slot->hpc_ops->green_led_blink(p_slot);
@@ -512,7 +512,7 @@ static void handle_button_press_event(struct slot *p_slot)
* press the attention again before the 5 sec. limit
* expires to cancel hot-add or hot-remove
*/
- info("Button cancel on Slot(%s)\n", p_slot->name);
+ info("Button cancel on Slot(%s)\n", slot_name(p_slot));
dbg("%s: button cancel\n", __func__);
cancel_delayed_work(&p_slot->work);
if (p_slot->state == BLINKINGOFF_STATE)
@@ -521,7 +521,7 @@ static void handle_button_press_event(struct slot *p_slot)
p_slot->hpc_ops->green_led_off(p_slot);
p_slot->hpc_ops->set_attention_status(p_slot, 0);
info("PCI slot #%s - action canceled due to button press\n",
- p_slot->name);
+ slot_name(p_slot));
p_slot->state = STATIC_STATE;
break;
case POWEROFF_STATE:
@@ -531,7 +531,7 @@ static void handle_button_press_event(struct slot *p_slot)
* this means that the previous attention button action
* to hot-add or hot-remove is undergoing
*/
- info("Button ignore on Slot(%s)\n", p_slot->name);
+ info("Button ignore on Slot(%s)\n", slot_name(p_slot));
update_slot_info(p_slot);
break;
default:
@@ -574,17 +574,17 @@ static int shpchp_enable_slot (struct slot *p_slot)
mutex_lock(&p_slot->ctrl->crit_sect);
rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
if (rc || !getstatus) {
- info("No adapter on slot(%s)\n", p_slot->name);
+ info("No adapter on slot(%s)\n", slot_name(p_slot));
goto out;
}
rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
if (rc || getstatus) {
- info("Latch open on slot(%s)\n", p_slot->name);
+ info("Latch open on slot(%s)\n", slot_name(p_slot));
goto out;
}
rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
if (rc || getstatus) {
- info("Already enabled on slot(%s)\n", p_slot->name);
+ info("Already enabled on slot(%s)\n", slot_name(p_slot));
goto out;
}
@@ -633,17 +633,17 @@ static int shpchp_disable_slot (struct slot *p_slot)
rc = p_slot->hpc_ops->get_adapter_status(p_slot, &getstatus);
if (rc || !getstatus) {
- info("No adapter on slot(%s)\n", p_slot->name);
+ info("No adapter on slot(%s)\n", slot_name(p_slot));
goto out;
}
rc = p_slot->hpc_ops->get_latch_status(p_slot, &getstatus);
if (rc || getstatus) {
- info("Latch open on slot(%s)\n", p_slot->name);
+ info("Latch open on slot(%s)\n", slot_name(p_slot));
goto out;
}
rc = p_slot->hpc_ops->get_power_status(p_slot, &getstatus);
if (rc || !getstatus) {
- info("Already disabled slot(%s)\n", p_slot->name);
+ info("Already disabled slot(%s)\n", slot_name(p_slot));
goto out;
}
@@ -671,14 +671,14 @@ int shpchp_sysfs_enable_slot(struct slot *p_slot)
break;
case POWERON_STATE:
info("Slot %s is already in powering on state\n",
- p_slot->name);
+ slot_name(p_slot));
break;
case BLINKINGOFF_STATE:
case POWEROFF_STATE:
- info("Already enabled on slot %s\n", p_slot->name);
+ info("Already enabled on slot %s\n", slot_name(p_slot));
break;
default:
- err("Not a valid state on slot %s\n", p_slot->name);
+ err("Not a valid state on slot %s\n", slot_name(p_slot));
break;
}
mutex_unlock(&p_slot->lock);
@@ -703,14 +703,14 @@ int shpchp_sysfs_disable_slot(struct slot *p_slot)
break;
case POWEROFF_STATE:
info("Slot %s is already in powering off state\n",
- p_slot->name);
+ slot_name(p_slot));
break;
case BLINKINGON_STATE:
case POWERON_STATE:
- info("Already disabled on slot %s\n", p_slot->name);
+ info("Already disabled on slot %s\n", slot_name(p_slot));
break;
default:
- err("Not a valid state on slot %s\n", p_slot->name);
+ err("Not a valid state on slot %s\n", slot_name(p_slot));
break;
}
mutex_unlock(&p_slot->lock);
Now that the PCI core manages the 'name' for each individual
hotplug driver, and all drivers (except rpaphp) 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.
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/pci_hotplug_core.c | 6 +++---
include/linux/pci_hotplug.h | 3 ---
2 files changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index b196ea1..49dd28f 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -534,7 +534,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;
@@ -632,7 +632,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;
@@ -643,7 +643,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;
diff --git a/include/linux/pci_hotplug.h b/include/linux/pci_hotplug.h
index a3a3245..a00bd1a 100644
--- a/include/linux/pci_hotplug.h
+++ b/include/linux/pci_hotplug.h
@@ -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);
The 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
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Alex Chiang <[email protected]>
---
drivers/pci/hotplug/fakephp.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 24dcbf1..3a2637a 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -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,11 @@ 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;
slot->private = dslot;
@@ -375,4 +380,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");
On Sat, Oct 4, 2008 at 2:18 AM, Alex Chiang <[email protected]> wrote:
> @@ -63,10 +63,9 @@ 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)
> + slot->name = kmalloc(strlen(drc_name) + 1, GFP_KERNEL);
> + if (!slot->name)
> goto error_info;
> - slot->name = slot->hotplug_slot->name;
> strcpy(slot->name, drc_name);
Minor nit: you could use kstrdup() here.
On Fri, Oct 03, 2008 at 05:17:32PM -0600, Alex Chiang wrote:
> 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.
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Alex Chiang <[email protected]>
> ---
>
Simple, clean, obvious.
Reviewed-by: Matthew Wilcox <[email protected]>
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Fri, Oct 03, 2008 at 05:17:37PM -0600, Alex Chiang wrote:
> Add explicit locking to pci_hp_register/deregister. For a given pci_slot,
> we want to make sure that the pci_slot->hotplug pointer isn't being
> changed from underneath us.
I'm torn between saying this goes too far and this doesn't go far
enough. If pci_hp_register() and pci_hp_deregister() can race against
each other, then I don't like the way we grab, release, grab, release
the pci_hotplug_slot_list_lock in pci_hp_deregister()/get_slot_from_name().
I think it would be smarter to grab the pci_hotplug_slot_list_lock
before calling get_slot_from_name() and release it after the list_del().
Mind you, I can't see a race there, but I'm also not at my best today
and it's possible there's one there I can't see.
As it currently stands, I don't think it needs to be a mutex -- do we do
anything sleeping under it?
Another part of me says "Just slap a mutex around both functions and
have done with it".
So, in summary, more eyes on this please. I don't think I can do a good
job today at spotting races.
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 0e7a511..e00266b 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -37,6 +37,7 @@
> #include <linux/init.h>
> #include <linux/mount.h>
> #include <linux/namei.h>
> +#include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/pci_hotplug.h>
> #include <asm/uaccess.h>
> @@ -554,6 +555,7 @@ out:
> *
> * Returns 0 if successful, anything else for an error.
> */
> +static DEFINE_MUTEX(pci_hp_mutex);
> int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> const char *name)
> {
> @@ -583,7 +585,9 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> if (IS_ERR(pci_slot))
> return PTR_ERR(pci_slot);
>
> + mutex_lock(&pci_hp_mutex);
> if (pci_slot->hotplug) {
> + mutex_unlock(&pci_hp_mutex);
> dbg("%s: already claimed\n", __func__);
> pci_destroy_slot(pci_slot);
> return -EBUSY;
> @@ -591,6 +595,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>
> slot->pci_slot = pci_slot;
> pci_slot->hotplug = slot;
> + mutex_unlock(&pci_hp_mutex);
>
> /*
> * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> @@ -635,6 +640,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
> if (temp != hotplug)
> return -ENODEV;
>
> + mutex_lock(&pci_hp_mutex);
> spin_lock(&pci_hotplug_slot_list_lock);
> list_del(&hotplug->slot_list);
> spin_unlock(&pci_hotplug_slot_list_lock);
> @@ -645,6 +651,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
>
> hotplug->release(hotplug);
> slot->hotplug = NULL;
> + mutex_unlock(&pci_hp_mutex);
> pci_destroy_slot(slot);
>
> return 0;
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 0c6db03..1fffb27 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -164,10 +164,10 @@ placeholder:
> pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
> __func__, pci_domain_nr(parent), parent->number, slot_nr);
>
> - out:
> +out:
> up_write(&pci_bus_sem);
> return slot;
> - err:
> +err:
> kfree(slot);
> slot = ERR_PTR(err);
> goto out;
Gratuitous whitespace change?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
I think we no longer need pci_hotplug_slot_list_lock with your patch.
Thanks,
Kenji Kaneshige
Alex Chiang wrote:
> Add explicit locking to pci_hp_register/deregister. For a given pci_slot,
> we want to make sure that the pci_slot->hotplug pointer isn't being
> changed from underneath us.
>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Alex Chiang <[email protected]>
> ---
>
> drivers/pci/hotplug/pci_hotplug_core.c | 7 +++++++
> drivers/pci/slot.c | 4 ++--
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index 0e7a511..e00266b 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -37,6 +37,7 @@
> #include <linux/init.h>
> #include <linux/mount.h>
> #include <linux/namei.h>
> +#include <linux/mutex.h>
> #include <linux/pci.h>
> #include <linux/pci_hotplug.h>
> #include <asm/uaccess.h>
> @@ -554,6 +555,7 @@ out:
> *
> * Returns 0 if successful, anything else for an error.
> */
> +static DEFINE_MUTEX(pci_hp_mutex);
> int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> const char *name)
> {
> @@ -583,7 +585,9 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
> if (IS_ERR(pci_slot))
> return PTR_ERR(pci_slot);
>
> + mutex_lock(&pci_hp_mutex);
> if (pci_slot->hotplug) {
> + mutex_unlock(&pci_hp_mutex);
> dbg("%s: already claimed\n", __func__);
> pci_destroy_slot(pci_slot);
> return -EBUSY;
> @@ -591,6 +595,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
>
> slot->pci_slot = pci_slot;
> pci_slot->hotplug = slot;
> + mutex_unlock(&pci_hp_mutex);
>
> /*
> * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
> @@ -635,6 +640,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
> if (temp != hotplug)
> return -ENODEV;
>
> + mutex_lock(&pci_hp_mutex);
> spin_lock(&pci_hotplug_slot_list_lock);
> list_del(&hotplug->slot_list);
> spin_unlock(&pci_hotplug_slot_list_lock);
> @@ -645,6 +651,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
>
> hotplug->release(hotplug);
> slot->hotplug = NULL;
> + mutex_unlock(&pci_hp_mutex);
> pci_destroy_slot(slot);
>
> return 0;
> diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> index 0c6db03..1fffb27 100644
> --- a/drivers/pci/slot.c
> +++ b/drivers/pci/slot.c
> @@ -164,10 +164,10 @@ placeholder:
> pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
> __func__, pci_domain_nr(parent), parent->number, slot_nr);
>
> - out:
> +out:
> up_write(&pci_bus_sem);
> return slot;
> - err:
> +err:
> kfree(slot);
> slot = ERR_PTR(err);
> goto out;
>
> --
> 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
>
>
Alex Chiang wrote:
(snip.)
> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index e00266b..b196ea1 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
> @@ -572,41 +572,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);
> -
> mutex_lock(&pci_hp_mutex);
> - if (pci_slot->hotplug) {
> - mutex_unlock(&pci_hp_mutex);
> - 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;
> }
>
I think there still remain race condition. The situation I imagine
is as follows, though I might be misunderstanding something.
If pci_slot driver created pci_slot between pci_get_pci_slot() and
pci_create_slot() in pci_hp_register() in the hotplug driver's thread,
pci_rename_slot() would never be called and name specified by pci_slot
driver would be used instead of the one specified by hotplug driver.
> -void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
> +void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
> {
> - int name_count = 0;
> struct pci_slot *tmp;
>
> down_write(&pci_bus_sem);
>
> list_for_each_entry(tmp, &slot->bus->slots, list) {
> WARN_ON(tmp->number == slot_nr);
> - if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
> - name_count++;
> + goto out;
> }
>
> - if (name_count > 1)
> - printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
> -
> slot->number = slot_nr;
> +out:
> up_write(&pci_bus_sem);
> }
> -EXPORT_SYMBOL_GPL(pci_update_slot_number);
> +EXPORT_SYMBOL_GPL(pci_renumber_slot);
>
You changed exported symbol name. I think it should be made as
an another patch.
Thanks,
Kenji Kaneshige
Hi Alex-san,
I have some ideas and made some patches for comments I sent about
your [PATCH v4 02/15] and [PATCH v4 03/15]. Please take a look.
There are three patches.
- [01/03] Sample patch for [PATCH v4 02/15]
- [02/03] Sample patch for [PATCH v4 03/15]
NOTE:This doesn't target the comment about changing exported
symbol name.
- [03/03] Sample patch for [PATCH v4 14/15]
This is needed because above two patches make your [PATCH v4
14/15] can not be applied.
Note: I made those patches as replacement of your corresponding
ones. So those patches are NOT for applying on top your original
patches.
Thanks,
Kenji Kaneshige
Alex Chiang wrote:
> Hi all,
>
> This is v4 of the series that implements a series of changes
> that allows the PCI core to manage slot names, rather than
> individual hotplug drivers.
>
> Thank you to Kenji-san and Taku-san for testing and finding my bugs.
> Thanks to Willy for helping me out with the locking issues in
> pci_hp_register.
>
> Again, I have tested by loading pci_slot, acpiphp, and fakephp in various
> combinations, and using the new dup_slots=1 param to fakephp. I also had:
>
> CONFIG_DEBUG_SPINLOCK=y
> CONFIG_DEBUG_MUTEXES=y
> CONFIG_DEBUG_SPINLOCK_SLEEP=y
>
> and didn't see any complaints.
>
> This series applies against Jesse's linux-next branch. I just fetched his
> branch and peeked at it, and based on the log, my series _should_ apply
> with minimal fuzz (but I didn't do a rebase because I didn't think to
> check until writing this mail ;).
>
> It would be great if folks could give one last (hopefully!) review.
>
> Thanks.
>
> /ac
>
> v3 -> v4:
> - Do not access hotplug_slot_name() before name initialization
> - Serialize pci_hp_register/deregister
>
> 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
>
> ---
>
> Alex Chiang (15):
> 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: kmalloc/kfree slot->name directly
> 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: serialize pci_hp_register/deregister
> 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 | 75 +++++--------
> drivers/pci/hotplug/cpci_hotplug_pci.c | 4 -
> drivers/pci/hotplug/cpqphp.h | 13 +-
> drivers/pci/hotplug/cpqphp_core.c | 43 +++----
> drivers/pci/hotplug/fakephp.c | 26 +++-
> drivers/pci/hotplug/ibmphp.h | 5 -
> drivers/pci/hotplug/ibmphp_ebda.c | 19 +--
> drivers/pci/hotplug/pci_hotplug_core.c | 68 ++++++------
> drivers/pci/hotplug/pciehp.h | 9 +-
> drivers/pci/hotplug/pciehp_core.c | 49 +++-----
> drivers/pci/hotplug/pciehp_ctrl.c | 53 +++++----
> drivers/pci/hotplug/pciehp_hpc.c | 1
> drivers/pci/hotplug/rpaphp_slot.c | 9 +-
> drivers/pci/hotplug/sgi_hotplug.c | 18 +--
> drivers/pci/hotplug/shpchp.h | 9 +-
> drivers/pci/hotplug/shpchp_core.c | 52 +++------
> drivers/pci/hotplug/shpchp_ctrl.c | 48 ++++----
> drivers/pci/slot.c | 181 +++++++++++++++++++++++--------
> include/linux/pci.h | 9 +-
> include/linux/pci_hotplug.h | 11 +-
> 23 files changed, 407 insertions(+), 342 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
>
>
---
drivers/pci/hotplug/pci_hotplug_core.c | 49 +++++++++++++++++----------------
1 file changed, 26 insertions(+), 23 deletions(-)
Index: hotplug/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
--- hotplug.orig/drivers/pci/hotplug/pci_hotplug_core.c
+++ hotplug/drivers/pci/hotplug/pci_hotplug_core.c
@@ -37,6 +37,7 @@
#include <linux/init.h>
#include <linux/mount.h>
#include <linux/namei.h>
+#include <linux/mutex.h>
#include <linux/pci.h>
#include <linux/pci_hotplug.h>
#include <asm/uaccess.h>
@@ -61,7 +62,7 @@ static int debug;
//////////////////////////////////////////////////////////////////
static LIST_HEAD(pci_hotplug_slot_list);
-static DEFINE_SPINLOCK(pci_hotplug_slot_list_lock);
+static DEFINE_MUTEX(pci_hp_mutex);
/* these strings match up with the values in pci_bus_speed */
static char *pci_bus_speed_strings[] = {
@@ -530,16 +531,12 @@ static struct hotplug_slot *get_slot_fro
struct hotplug_slot *slot;
struct list_head *tmp;
- 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)
- goto out;
+ return slot;
}
- slot = NULL;
-out:
- spin_unlock(&pci_hotplug_slot_list_lock);
- return slot;
+ return NULL;
}
/**
@@ -570,9 +567,13 @@ int pci_hp_register(struct hotplug_slot
return -EINVAL;
}
+ mutex_lock(&pci_hp_mutex);
+
/* Check if we have already registered a slot with the same name. */
- if (get_slot_from_name(name))
- return -EEXIST;
+ if (get_slot_from_name(name)) {
+ result = -EEXIST;
+ goto out;
+ }
/*
* No problems if we call this interface from both ACPI_PCI_SLOT
@@ -580,13 +581,13 @@ int pci_hp_register(struct hotplug_slot
* pci_slot, the interface will simply bump the refcount.
*/
pci_slot = pci_create_slot(bus, slot_nr, name);
- if (IS_ERR(pci_slot))
- return PTR_ERR(pci_slot);
+ if ((result = IS_ERR(pci_slot)))
+ goto out;
if (pci_slot->hotplug) {
dbg("%s: already claimed\n", __func__);
- pci_destroy_slot(pci_slot);
- return -EBUSY;
+ result = -EBUSY;
+ goto cleanup;
}
slot->pci_slot = pci_slot;
@@ -597,21 +598,21 @@ int pci_hp_register(struct hotplug_slot
*/
if (strcmp(kobject_name(&pci_slot->kobj), name)) {
result = kobject_rename(&pci_slot->kobj, name);
- if (result) {
- pci_destroy_slot(pci_slot);
- return result;
- }
+ if (result)
+ goto cleanup;
}
- spin_lock(&pci_hotplug_slot_list_lock);
list_add(&slot->slot_list, &pci_hotplug_slot_list);
- spin_unlock(&pci_hotplug_slot_list_lock);
result = fs_add_slot(pci_slot);
kobject_uevent(&pci_slot->kobj, KOBJ_ADD);
dbg("Added slot %s to the list\n", name);
-
+out:
+ mutex_unlock(&pci_hp_mutex);
return result;
+cleanup:
+ pci_destroy_slot(pci_slot);
+ goto out;
}
/**
@@ -631,13 +632,14 @@ int pci_hp_deregister(struct hotplug_slo
if (!hotplug)
return -ENODEV;
+ mutex_lock(&pci_hp_mutex);
temp = get_slot_from_name(hotplug->name);
- if (temp != hotplug)
+ if (temp != hotplug) {
+ mutex_unlock(&pci_hp_mutex);
return -ENODEV;
+ }
- spin_lock(&pci_hotplug_slot_list_lock);
list_del(&hotplug->slot_list);
- spin_unlock(&pci_hotplug_slot_list_lock);
slot = hotplug->pci_slot;
fs_remove_slot(slot);
@@ -646,6 +648,7 @@ int pci_hp_deregister(struct hotplug_slo
hotplug->release(hotplug);
slot->hotplug = NULL;
pci_destroy_slot(slot);
+ mutex_unlock(&pci_hp_mutex);
return 0;
}
NOTE:This doesn't target the comment about changing exported
symbol name.
---
drivers/pci/hotplug/pci_hotplug_core.c | 18 +---
drivers/pci/hotplug/pciehp_core.c | 15 ----
drivers/pci/hotplug/shpchp_core.c | 15 ----
drivers/pci/slot.c | 123 ++++++++++++++++++++++++++-------
include/linux/pci.h | 4 -
5 files changed, 107 insertions(+), 68 deletions(-)
Index: hotplug/drivers/pci/hotplug/pciehp_core.c
===================================================================
--- hotplug.orig/drivers/pci/hotplug/pciehp_core.c
+++ hotplug/drivers/pci/hotplug/pciehp_core.c
@@ -196,7 +196,6 @@ static int init_slots(struct controller
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_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;
Index: hotplug/drivers/pci/hotplug/shpchp_core.c
===================================================================
--- hotplug.orig/drivers/pci/hotplug/shpchp_core.c
+++ hotplug/drivers/pci/hotplug/shpchp_core.c
@@ -102,7 +102,7 @@ static int init_slots(struct controller
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
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;
}
Index: hotplug/include/linux/pci.h
===================================================================
--- hotplug.orig/include/linux/pci.h
+++ hotplug/include/linux/pci.h
@@ -509,8 +509,10 @@ struct pci_bus *pci_add_new_bus(struct p
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);
Index: hotplug/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
--- hotplug.orig/drivers/pci/hotplug/pci_hotplug_core.c
+++ hotplug/drivers/pci/hotplug/pci_hotplug_core.c
@@ -569,12 +569,6 @@ int pci_hp_register(struct hotplug_slot
mutex_lock(&pci_hp_mutex);
- /* Check if we have already registered a slot with the same name. */
- if (get_slot_from_name(name)) {
- result = -EEXIST;
- goto out;
- }
-
/*
* No problems if we call this interface from both ACPI_PCI_SLOT
* driver and call it here again. If we've already created the
@@ -590,18 +584,14 @@ int pci_hp_register(struct hotplug_slot
goto cleanup;
}
- 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)
- goto cleanup;
- }
+ if ((result = pci_rename_slot(pci_slot, name)))
+ goto cleanup;
+ slot->pci_slot = pci_slot;
+ pci_slot->hotplug = slot;
list_add(&slot->slot_list, &pci_hotplug_slot_list);
result = fs_add_slot(pci_slot);
Index: hotplug/drivers/pci/slot.c
===================================================================
--- hotplug.orig/drivers/pci/slot.c
+++ hotplug/drivers/pci/slot.c
@@ -78,6 +78,46 @@ static struct kobj_type pci_slot_ktype =
.default_attrs = pci_slot_default_attrs,
};
+static char *make_slot_name(struct pci_slot *slot, const char *name)
+{
+ char *new_name;
+ int len, max, dup;
+
+ new_name = kstrdup(name, GFP_KERNEL);
+ if (!new_name)
+ return NULL;
+
+ /*
+ * Make sure we hit the realloc case the first time through the
+ * loop. 'len' will be strlen(name) + 3 at that point which is
+ * enough space for "name-X" and the trailing NUL.
+ */
+ len = strlen(name) + 2;
+ max = 1;
+ dup = 1;
+
+ for (;;) {
+ struct kobject *dup_slot;
+ dup_slot = kset_find_obj(pci_slots_kset, new_name);
+ if (!dup_slot)
+ break;
+ kobject_put(dup_slot);
+ if (dup_slot == &slot->kobj)
+ break;
+ if (dup == max) {
+ len++;
+ max *= 10;
+ kfree(new_name);
+ new_name = kmalloc(len, GFP_KERNEL);
+ if (!new_name)
+ break;
+ }
+ sprintf(new_name, "%s-%d", name, dup++);
+ }
+
+ return new_name;
+}
+
/**
* pci_create_slot - create or increment refcount for physical PCI slot
* @parent: struct pci_bus of parent bridge
@@ -89,7 +129,19 @@ static struct kobj_type pci_slot_ktype =
* either return a new &struct pci_slot to the caller, or if the pci_slot
* already exists, its refcount will be incremented.
*
- * Slots are uniquely identified by a @pci_bus, @slot_nr, @name tuple.
+ * Slots are uniquely identified by a @pci_bus, @slot_nr tuple.
+ *
+ * The kobject API imposes a restriction on us, and does not allow sysfs
+ * entries with duplicate names. There are known platforms with broken
+ * firmware that assign the same name to multiple slots.
+ *
+ * We workaround these broken platforms by renaming the slots on behalf
+ * of the caller. If firmware assigns name N to multiple slots:
+ *
+ * The first slot is assigned N
+ * The second slot is assigned N-1
+ * The third slot is assigned N-2
+ * etc.
*
* Placeholder slots:
* In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify
@@ -98,24 +150,20 @@ static struct kobj_type pci_slot_ktype =
* the slot. In this scenario, the caller may pass -1 for @slot_nr.
*
* The following semantics are imposed when the caller passes @slot_nr ==
- * -1. First, the check for existing %struct pci_slot is skipped, as the
- * caller may know about several unpopulated slots on a given %struct
- * pci_bus, and each slot would have a @slot_nr of -1. Uniqueness for
- * these slots is then determined by the @name parameter. We expect
- * kobject_init_and_add() to warn us if the caller attempts to create
- * multiple slots with the same name. The other change in semantics is
+ * -1. First, we no longer check for an existing %struct pci_slot, as there
+ * may be many slots with @slot_nr of -1. The other change in semantics is
* user-visible, which is the 'address' parameter presented in sysfs will
* consist solely of a dddd:bb tuple, where dddd is the PCI domain of the
* %struct pci_bus and bb is the bus number. In other words, the devfn of
* the 'placeholder' slot will not be displayed.
*/
-
struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
const char *name)
{
struct pci_dev *dev;
struct pci_slot *slot;
int err;
+ char *slot_name = NULL;
down_write(&pci_bus_sem);
@@ -144,18 +192,23 @@ placeholder:
slot->bus = parent;
slot->number = slot_nr;
-
slot->kobj.kset = pci_slots_kset;
+
+ slot_name = make_slot_name(slot, name);
+ if (!slot_name) {
+ err = -ENOMEM;
+ goto err;
+ }
+
err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
- "%s", name);
+ "%s", slot_name);
if (err) {
- printk(KERN_ERR "Unable to register kobject %s\n", name);
+ printk(KERN_ERR "Unable to register kobject %s\n", slot_name);
goto err;
}
INIT_LIST_HEAD(&slot->list);
list_add(&slot->list, &parent->slots);
-
list_for_each_entry(dev, &parent->devices, bus_list)
if (PCI_SLOT(dev->devfn) == slot_nr)
dev->slot = slot;
@@ -166,6 +219,7 @@ placeholder:
out:
up_write(&pci_bus_sem);
+ kfree(slot_name);
return slot;
err:
kfree(slot);
@@ -175,7 +229,35 @@ placeholder:
EXPORT_SYMBOL_GPL(pci_create_slot);
/**
- * pci_update_slot_number - update %struct pci_slot -> number
+ * pci_rename_slot - update %struct pci_slot -> name
+ * @slot - %struct pci_slot to update
+ * @name - new requested name for slot
+ *
+ * Allow caller to safely rename a %struct pci_slot without making
+ * the kobject core complain about duplicate names.
+ */
+int pci_rename_slot(struct pci_slot *slot, const char *name)
+{
+ int result = 0;
+ char *slot_name;
+
+ down_write(&pci_bus_sem);
+ slot_name = make_slot_name(slot, name);
+ if (!slot_name) {
+ result = -ENOMEM;
+ goto out;
+ }
+ if (strcmp(kobject_name(&slot->kobj), slot_name))
+ result = kobject_rename(&slot->kobj, slot_name);
+ kfree(slot_name);
+out:
+ up_write(&pci_bus_sem);
+ return result;
+}
+EXPORT_SYMBOL_GPL(pci_rename_slot);
+
+/**
+ * pci_renumber_slot - update %struct pci_slot -> number
* @slot - %struct pci_slot to update
* @slot_nr - new number for slot
*
@@ -183,27 +265,21 @@ EXPORT_SYMBOL_GPL(pci_create_slot);
* created a placeholder slot in pci_create_slot() by passing a -1 as
* slot_nr, to update their %struct pci_slot with the correct @slot_nr.
*/
-
-void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
+void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
{
- int name_count = 0;
struct pci_slot *tmp;
down_write(&pci_bus_sem);
list_for_each_entry(tmp, &slot->bus->slots, list) {
WARN_ON(tmp->number == slot_nr);
- if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
- name_count++;
+ goto out;
}
-
- if (name_count > 1)
- printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
-
slot->number = slot_nr;
+out:
up_write(&pci_bus_sem);
}
-EXPORT_SYMBOL_GPL(pci_update_slot_number);
+EXPORT_SYMBOL_GPL(pci_renumber_slot);
/**
* pci_destroy_slot - decrement refcount for physical PCI slot
@@ -213,7 +289,6 @@ EXPORT_SYMBOL_GPL(pci_update_slot_number
* just call kobject_put on its kobj and let our release methods do the
* rest.
*/
-
void pci_destroy_slot(struct pci_slot *slot)
{
pr_debug("%s: dec refcount to %d on %04x:%02x:%02x\n", __func__,
This is needed because previous two patches make your [PATCH v4
14/15] can not be applied.
---
drivers/pci/hotplug/pci_hotplug_core.c | 6 +++---
include/linux/pci_hotplug.h | 3 ---
2 files changed, 3 insertions(+), 6 deletions(-)
Index: hotplug/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================
--- hotplug.orig/drivers/pci/hotplug/pci_hotplug_core.c
+++ hotplug/drivers/pci/hotplug/pci_hotplug_core.c
@@ -533,7 +533,7 @@ static struct hotplug_slot *get_slot_fro
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)
return slot;
}
return NULL;
@@ -623,7 +623,7 @@ int pci_hp_deregister(struct hotplug_slo
return -ENODEV;
mutex_lock(&pci_hp_mutex);
- temp = get_slot_from_name(hotplug->name);
+ temp = get_slot_from_name(hotplug_slot_name(hotplug));
if (temp != hotplug) {
mutex_unlock(&pci_hp_mutex);
return -ENODEV;
@@ -633,7 +633,7 @@ int pci_hp_deregister(struct hotplug_slo
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;
Index: hotplug/include/linux/pci_hotplug.h
===================================================================
--- hotplug.orig/include/linux/pci_hotplug.h
+++ hotplug/include/linux/pci_hotplug.h
@@ -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);
* Pekka Enberg <[email protected]>:
> On Sat, Oct 4, 2008 at 2:18 AM, Alex Chiang <[email protected]> wrote:
> > @@ -63,10 +63,9 @@ 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)
> > + slot->name = kmalloc(strlen(drc_name) + 1, GFP_KERNEL);
> > + if (!slot->name)
> > goto error_info;
> > - slot->name = slot->hotplug_slot->name;
> > strcpy(slot->name, drc_name);
>
> Minor nit: you could use kstrdup() here.
Good call, I've made a change, thanks.
/ac
* Matthew Wilcox <[email protected]>:
> On Fri, Oct 03, 2008 at 05:17:37PM -0600, Alex Chiang wrote:
> > Add explicit locking to pci_hp_register/deregister. For a given pci_slot,
> > we want to make sure that the pci_slot->hotplug pointer isn't being
> > changed from underneath us.
>
> I'm torn between saying this goes too far and this doesn't go far
> enough. If pci_hp_register() and pci_hp_deregister() can race against
> each other,
I think Kenji-san was more worried about the race for multiple
callers of pci_create_slot() and all the horrible crap I was
introducing to account for slot renaming.
I've figured out a different approach which I'll be sending out
shortly. I didn't change any of the locking semantics in
pci_hp_register/deregister, so I disclaim responsibility for any
existing races that exist today. ;) (note, I don't think there
are any).
> > diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
> > index 0c6db03..1fffb27 100644
> > --- a/drivers/pci/slot.c
> > +++ b/drivers/pci/slot.c
> > @@ -164,10 +164,10 @@ placeholder:
> > pr_debug("%s: created pci_slot on %04x:%02x:%02x\n",
> > __func__, pci_domain_nr(parent), parent->number, slot_nr);
> >
> > - out:
> > +out:
> > up_write(&pci_bus_sem);
> > return slot;
> > - err:
> > +err:
> > kfree(slot);
> > slot = ERR_PTR(err);
> > goto out;
>
> Gratuitous whitespace change?
Maybe... I'm gonna keep this same hunk, but fold it into a bigger
patch so it won't be as noticable. ;)
/ac
* Kenji Kaneshige <[email protected]>:
> Alex Chiang wrote:
>
>> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
>> index e00266b..b196ea1 100644
>> --- a/drivers/pci/hotplug/pci_hotplug_core.c
>> +++ b/drivers/pci/hotplug/pci_hotplug_core.c
>> @@ -572,41 +572,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);
>> -
>> mutex_lock(&pci_hp_mutex);
>> - if (pci_slot->hotplug) {
>> - mutex_unlock(&pci_hp_mutex);
>> - 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;
>> }
>>
>
> I think there still remain race condition. The situation I imagine
> is as follows, though I might be misunderstanding something.
>
> If pci_slot driver created pci_slot between pci_get_pci_slot() and
> pci_create_slot() in pci_hp_register() in the hotplug driver's thread,
> pci_rename_slot() would never be called and name specified by pci_slot
> driver would be used instead of the one specified by hotplug driver.
Ah, now I finally understand.
And I think the solution is to avoid all the crap code I've been
introducing into pci_hp_register.
A better way to do it would be to make slot creation and name
override into one atomic operation, inside of pci_create_slot.
We are already serializing pci_create_slot by acquiring
pci_bus_sem, so that should prevent any race condition.
I wrote a long changelog entry for my patches that should explain
it all.
Please review. :)
>> -void pci_update_slot_number(struct pci_slot *slot, int slot_nr)
>> +void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
>> {
>> - int name_count = 0;
>> struct pci_slot *tmp;
>> down_write(&pci_bus_sem);
>> list_for_each_entry(tmp, &slot->bus->slots, list) {
>> WARN_ON(tmp->number == slot_nr);
>> - if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
>> - name_count++;
>> + goto out;
>> }
>> - if (name_count > 1)
>> - printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
>> -
>> slot->number = slot_nr;
>> +out:
>> up_write(&pci_bus_sem);
>> }
>> -EXPORT_SYMBOL_GPL(pci_update_slot_number);
>> +EXPORT_SYMBOL_GPL(pci_renumber_slot);
>>
>
> You changed exported symbol name. I think it should be made as
> an another patch.
Ok.
Thanks.
/ac
* Kenji Kaneshige <[email protected]>:
> Hi Alex-san,
>
> I have some ideas and made some patches for comments I sent about
> your [PATCH v4 02/15] and [PATCH v4 03/15]. Please take a look.
> There are three patches.
Hi Kenji-san,
Thanks for doing this work.
I tested your patches, but found a problem with refcounting in
your 02/03, and the slot directories in sysfs remained, even
after rmmod of all drivers.
I decided that things are getting too complicated with all these
new interfaces, so I got rid of them and updated the
pci_create_slot API instead, to take a 'rename' parameter. That
way, creating a slot and overriding its name can become an atomic
operation.
That should make the race conditions go away, and the code is
much easier to understand as well.
> - [01/03] Sample patch for [PATCH v4 02/15]
This is a good patch by itself. I think you should submit it to
Jesse. I did not need it after reworking to my new design.
> - [02/03] Sample patch for [PATCH v4 03/15]
> NOTE:This doesn't target the comment about changing exported
> symbol name.
I had to stare at this patch for a long time to understand it,
and finally saw that you were changing the rename logic to detect
if we were trying to rename an existing slot.
Unfortunately, it had some problem with the refcounting, and in
this scenario:
- pci_slot loaded
- fakephp dup_slots=1 loaded
- pci_slot unloaded
The slots claimed by pci_slot (but _not_ by fakephp) were never
released.
By the time I got this far, I was already thinking about a
redesign, so I did not try and debug further...
> - [03/03] Sample patch for [PATCH v4 14/15]
> This is needed because above two patches make your [PATCH v4
> 14/15] can not be applied.
Not needed, since I re-designed the approach.
> Note: I made those patches as replacement of your corresponding
> ones. So those patches are NOT for applying on top your original
> patches.
Again, thank you very much for all the review and hard work, and
sorry for causing so much churn. :-/
I'll be sending out the new patch series shortly.
/ac
Alex Chiang wrote:
> * Kenji Kaneshige <[email protected]>:
>> Hi Alex-san,
>>
>> I have some ideas and made some patches for comments I sent about
>> your [PATCH v4 02/15] and [PATCH v4 03/15]. Please take a look.
>> There are three patches.
>
> Hi Kenji-san,
>
> Thanks for doing this work.
>
> I tested your patches, but found a problem with refcounting in
> your 02/03, and the slot directories in sysfs remained, even
> after rmmod of all drivers.
>
> I decided that things are getting too complicated with all these
> new interfaces, so I got rid of them and updated the
> pci_create_slot API instead, to take a 'rename' parameter. That
> way, creating a slot and overriding its name can become an atomic
> operation.
>
> That should make the race conditions go away, and the code is
> much easier to understand as well.
>
OK.
I guess one of the thing that will bother us is how to handle
the case pci_slot created by pci_create_slot() already has its
hotplug callbacks in pci_hp_register(). Current code calls
pci_destroy_slot() and return -EBUSY in this case. With the
new API which takes 'rename' parameterIn addtion to that, we
need to rename the slot as it used to be, in addition.
>> - [01/03] Sample patch for [PATCH v4 02/15]
>
> This is a good patch by itself. I think you should submit it to
> Jesse. I did not need it after reworking to my new design.
>
>> - [02/03] Sample patch for [PATCH v4 03/15]
>> NOTE:This doesn't target the comment about changing exported
>> symbol name.
>
> I had to stare at this patch for a long time to understand it,
> and finally saw that you were changing the rename logic to detect
> if we were trying to rename an existing slot.
>
> Unfortunately, it had some problem with the refcounting, and in
> this scenario:
>
> - pci_slot loaded
> - fakephp dup_slots=1 loaded
> - pci_slot unloaded
>
> The slots claimed by pci_slot (but _not_ by fakephp) were never
> released.
>
> By the time I got this far, I was already thinking about a
> redesign, so I did not try and debug further...
>
>> - [03/03] Sample patch for [PATCH v4 14/15]
>> This is needed because above two patches make your [PATCH v4
>> 14/15] can not be applied.
>
> Not needed, since I re-designed the approach.
>
>> Note: I made those patches as replacement of your corresponding
>> ones. So those patches are NOT for applying on top your original
>> patches.
>
> Again, thank you very much for all the review and hard work, and
> sorry for causing so much churn. :-/
>
> I'll be sending out the new patch series shortly.
>
No problem.
I'm looking forward to looking at new patches.
Thanks,
Kenji Kaneshige