login
Search
Hello Kenji-san, You are obviously correct about my prior bad patch that simply removed the check in pci_hp_register() for duplicate names. I also talked with Willy, and he thought that the proper way to fix all these issues was not in the individual drivers, but in the PCI core, which I agree with. Here is a patch series that attempts to implement Willy's suggestion. Patches 1--6 convert the callers of pci_hp_register/pci_create_slot from a static char name[] to a dynamically kmalloc'ed char *name. There are also cleanups in the individual drivers to remove the _slot_with_bus parameters. Patch 7/7 implements the collision rename logic. It's not the prettiest thing in the world, so your comments are welcome. :) It's getting late here and I haven't had time to really test it, other than a quick compile test, but I wanted to try and send it so that you could take a look and possibly try it. ;) These patches apply on top of Linus's latest tree. Thanks! /ac drivers/acpi/pci_slot.c | 15 ++++++- drivers/pci/hotplug/acpiphp.h | 2 - drivers/pci/hotplug/acpiphp_core.c | 18 +++++---- drivers/pci/hotplug/fakephp.c | 16 ++++++-- drivers/pci/hotplug/pci_hotplug_core.c | 4 -- drivers/pci/hotplug/pciehp.h | 6 +-- drivers/pci/hotplug/pciehp_core.c | 7 --- drivers/pci/hotplug/pciehp_hpc.c | 19 ++++----- drivers/pci/hotplug/pcihp_skeleton.c | 12 ++++-- drivers/pci/hotplug/shpchp.h | 5 +- drivers/pci/hotplug/shpchp_core.c | 29 ++++---------- drivers/pci/slot.c | 65 +++++++++++++++++++++++++++++---- include/linux/pci.h | 3 - 13 files changed, 124 insertions(+), 77 deletions(-) --
Callers of pci_hp_register() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/acpiphp.h | 2 +-
drivers/pci/hotplug/acpiphp_core.c | 17 ++++++++++-------
2 files changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/hotplug/acpiphp.h b/drivers/pci/hotplug/acpiphp.h
index 5a58b07..2ac519d 100644
--- a/drivers/pci/hotplug/acpiphp.h
+++ b/drivers/pci/hotplug/acpiphp.h
@@ -63,7 +63,7 @@ struct slot {
struct hotplug_slot *hotplug_slot;
struct acpiphp_slot *acpi_slot;
struct hotplug_slot_info info;
- char name[SLOT_NAME_SIZE];
+ char *name;
};
/*
diff --git a/drivers/pci/hotplug/acpiphp_core.c b/drivers/pci/hotplug/acpiphp_core.c
index 0e496e8..ec164f4 100644
--- a/drivers/pci/hotplug/acpiphp_core.c
+++ b/drivers/pci/hotplug/acpiphp_core.c
@@ -301,6 +301,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+ kfree(slot->name);
kfree(slot->hotplug_slot);
kfree(slot);
}
@@ -315,9 +316,13 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
if (!slot)
goto error;
+ slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+ if (!slot->name)
+ goto error_slot;
+
slot->hotplug_slot = kzalloc(sizeof(*slot->hotplug_slot), GFP_KERNEL);
if (!slot->hotplug_slot)
- goto error_slot;
+ goto error_name;
slot->hotplug_slot->info = &slot->info;
@@ -336,23 +341,21 @@ 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(slot->name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
retval = pci_hp_register(slot->hotplug_slot,
...Callers of pci_hp_register() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/fakephp.c | 15 ++++++++++++---
1 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/hotplug/fakephp.c b/drivers/pci/hotplug/fakephp.c
index 40337a0..af8a1bf 100644
--- a/drivers/pci/hotplug/fakephp.c
+++ b/drivers/pci/hotplug/fakephp.c
@@ -60,13 +60,15 @@
#define DRIVER_AUTHOR "Greg Kroah-Hartman <greg@kroah.com>"
#define DRIVER_DESC "Fake PCI Hot Plug Controller Driver"
+#define SLOT_NAME_SIZE 8
+
struct dummy_slot {
struct list_head node;
struct hotplug_slot *slot;
struct pci_dev *dev;
struct work_struct remove_work;
unsigned long removed;
- char name[8];
+ char *name;
};
static int debug;
@@ -91,6 +93,7 @@ static void dummy_release(struct hotplug_slot *slot)
list_del(&dslot->node);
kfree(dslot->slot->info);
+ kfree(dslot->name);
kfree(dslot->slot);
pci_dev_put(dslot->dev);
kfree(dslot);
@@ -119,8 +122,12 @@ static int add_slot(struct pci_dev *dev)
if (!dslot)
goto error_info;
+ dslot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+ if (!dslot->name)
+ goto error_dslot;
+
slot->name = dslot->name;
- snprintf(slot->name, sizeof(dslot->name), "fake%d", count++);
+ snprintf(slot->name, SLOT_NAME_SIZE, "fake%d", count++);
dbg("slot->name = %s\n", slot->name);
slot->ops = &dummy_hotplug_slot_ops;
slot->release = &dummy_release;
@@ -129,7 +136,7 @@ static int add_slot(struct pci_dev *dev)
retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn));
if (retval) {
err("pci_hp_register failed with error %d\n", retval);
- goto error_dslot;
+ goto error_name;
}
dslot->slot = slot;
@@ -137,6 +144,8 @@ static int add_slot(struct pci_dev *dev)
list_add (&dslot->node, &slot_list);
return ...Callers of pci_hp_register() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
This change also cleans up pciehp's struct slot, removing the unused
struct task_list and moving the number member so that it is naturally
aligned, reducing the size from 180 bytes down to 116 bytes.
We also remove the pciehp_slot_with_bus module parameter which
essentially reverts commits:
pciehp: fix slot name
3800345f723fd130d50434d4717b99d4a9f383c8
pciehp: add message about pciehp_slot_with_bus option
9e4f2e8d4ddb04ad16a3828cd9a369a5a5287009
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/pciehp.h | 6 ++----
drivers/pci/hotplug/pciehp_core.c | 7 -------
drivers/pci/hotplug/pciehp_hpc.c | 18 ++++++++----------
3 files changed, 10 insertions(+), 21 deletions(-)
diff --git a/drivers/pci/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index e3a1e7e..27dda64 100644
--- a/drivers/pci/hotplug/pciehp.h
+++ b/drivers/pci/hotplug/pciehp.h
@@ -43,7 +43,6 @@ extern int pciehp_poll_mode;
extern int pciehp_poll_time;
extern int pciehp_debug;
extern int pciehp_force;
-extern int pciehp_slot_with_bus;
extern struct workqueue_struct *pciehp_wq;
#define dbg(format, arg...) \
@@ -62,15 +61,14 @@ 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];
+ char *name;
unsigned long last_emi_toggle;
struct delayed_work work; /* work for button event */
struct mutex lock;
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 3677495..6c1df8d 100644
--- a/drivers/pci/hotplug/pciehp_core.c
+++ ...Callers of pci_hp_register() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
Let's give future implementors a good example to copy from.
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/pcihp_skeleton.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/hotplug/pcihp_skeleton.c b/drivers/pci/hotplug/pcihp_skeleton.c
index e3dd6cf..3e53026 100644
--- a/drivers/pci/hotplug/pcihp_skeleton.c
+++ b/drivers/pci/hotplug/pcihp_skeleton.c
@@ -41,7 +41,7 @@ struct slot {
u8 number;
struct hotplug_slot *hotplug_slot;
struct list_head slot_list;
- char name[SLOT_NAME_SIZE];
+ char *name;
};
static LIST_HEAD(slot_list);
@@ -232,6 +232,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
kfree(slot->hotplug_slot->info);
kfree(slot->hotplug_slot);
+ kfree(slot->name);
kfree(slot);
}
@@ -265,9 +266,13 @@ static int __init init_slots(void)
if (!slot)
goto error;
+ slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+ if (!slot->name)
+ goto error_slot;
+
hotplug_slot = kzalloc(sizeof(*hotplug_slot), GFP_KERNEL);
if (!hotplug_slot)
- goto error_slot;
+ goto error_name;
slot->hotplug_slot = hotplug_slot;
info = kzalloc(sizeof(*info), GFP_KERNEL);
@@ -308,6 +313,8 @@ error_info:
kfree(info);
error_hpslot:
kfree(hotplug_slot);
+error_name:
+ kfree(slot->name);
error_slot:
kfree(slot);
error:
--
1.6.0.rc0.g95f8
--
Callers of pci_hp_register() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
This change also removes the unused struct task_event from the
slot structure.
We also remove the shpchp_slot_with_bus module parameter, which
essentially reverts commits:
shpchp: fix slot name
ef0ff95f136f0f2d035667af5d18b824609de320
shpchp: add message about shpchp_slot_with_bus option
b3bd307c628af2f0a581c42d5d7e4bcdbbf64b6a
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/shpchp.h | 5 ++---
drivers/pci/hotplug/shpchp_core.c | 28 +++++++++-------------------
2 files changed, 11 insertions(+), 22 deletions(-)
diff --git a/drivers/pci/hotplug/shpchp.h b/drivers/pci/hotplug/shpchp.h
index 8a026f7..8da1044 100644
--- a/drivers/pci/hotplug/shpchp.h
+++ b/drivers/pci/hotplug/shpchp.h
@@ -69,15 +69,14 @@ struct slot {
u8 state;
u8 presence_save;
u8 pwr_save;
- struct timer_list task_event;
- u8 hp_slot;
+ char *name;
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 {
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index a8cbd03..c490380 100644
--- a/drivers/pci/hotplug/shpchp_core.c
+++ b/drivers/pci/hotplug/shpchp_core.c
@@ -39,7 +39,6 @@
int shpchp_debug;
int shpchp_poll_mode;
int shpchp_poll_time;
-static int shpchp_slot_with_bus;
struct workqueue_struct *shpchp_wq;
#define DRIVER_VERSION "0.4"
@@ -53,11 +52,9 @@ MODULE_LICENSE("GPL");
module_param(shpchp_debug, bool, 0644);
module_param(shpchp_poll_mode, bool, 0644);
module_param(shpchp_poll_time, int, 0644);
-module_param(shpchp_slot_with_bus, bool, 0644);
MODULE_PARM_DESC(shpchp_debug, ...Callers of pci_create_slot() need to be converted from a
char name[] to a char *name.
This change allows pci_create_slot() to dynamically rename the
sysfs name of the slot in the event of a name collision.
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/acpi/pci_slot.c | 14 ++++++++++++--
1 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/acpi/pci_slot.c b/drivers/acpi/pci_slot.c
index d5b4ef8..b146b72 100644
--- a/drivers/acpi/pci_slot.c
+++ b/drivers/acpi/pci_slot.c
@@ -133,7 +133,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
{
int device;
unsigned long sun;
- char name[SLOT_NAME_SIZE];
+ char *name;
struct acpi_pci_slot *slot;
struct pci_slot *pci_slot;
struct callback_args *parent_context = context;
@@ -149,10 +149,18 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
return AE_OK;
}
- snprintf(name, sizeof(name), "%u", (u32)sun);
+ name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+ if (!name) {
+ err("%s: cannot allocate memory for name\n", __func__);
+ kfree(slot);
+ return AE_OK;
+ }
+
+ snprintf(name, SLOT_NAME_SIZE, "%u", (u32)sun);
pci_slot = pci_create_slot(pci_bus, device, name);
if (IS_ERR(pci_slot)) {
err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
+ kfree(name);
kfree(slot);
return AE_OK;
}
@@ -167,6 +175,8 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
dbg("pci_slot: %p, pci_bus: %x, device: %d, name: %s\n",
pci_slot, pci_bus->number, device, name);
+ kfree(name);
+
return AE_OK;
}
--
1.6.0.rc0.g95f8
--
Commit a86161b3134465f072d965ca7508ec9c1e2e52c7 added a check
to detect platforms with broken firmware that attempt to assign
identical slot names to multiple slots.
This approach is suboptimal because it prints a scary message
and forces the user to reload a hotplug driver with a module
parameter. We can do better here by attempting to 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
The Mth registered slot becomes N-M
This patch also has the effect of making attempts by multiple
drivers claiming the same slot become a more prominent error,
returning -EBUSY in those cases.
Signed-off-by: Alex Chiang <achiang@hp.com>
---
drivers/pci/hotplug/pci_hotplug_core.c | 4 --
drivers/pci/slot.c | 65 ++++++++++++++++++++++++++++---
include/linux/pci.h | 2 +-
3 files changed, 59 insertions(+), 12 deletions(-)
diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 5f85b1b..4476f0c 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -568,10 +568,6 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
return -EINVAL;
}
- /* Check if we have already registered a slot with the same name. */
- if (get_slot_from_name(slot->name))
- return -EEXIST;
-
/*
* No problems if we call this interface from both ACPI_PCI_SLOT
* driver and call it here again. If we've already created the
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7e5b85c..e35dcae 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -84,7 +84,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.
*
- * ...Hi Alex-san, Thank you for patches! I looked at your patches and it looks good. But I have only one concern about how to allocate/free memory for slot name. With your change, memory region for slot name will be allocated by hotplug *controller* driver and it can be freed using kfree() by hotplug *core* driver (not hotplug controller driver). So all hotplug controller drivers including drivers implemented in the future need to take it into account. I think it will be more robust if we can allocate and free memory in the same component (maybe hotplug core driver in this case). Thanks, Kenji Kaneshige --
Hm, I didn't think this would be a problem. The sequence is: - controller allocates memory for slot name - if core detects a collision: - it frees the name - it allocates new memory for name - it assigns that memory to the name parameter - controller->release will eventually free the name So it shouldn't matter if the core frees the original name pointer and allocates new memory, because the core will change the pointer for the controller. This is why I had to change the interface of pci_create_slot() from taking a const char *name to a char *name. Is there something I'm missing? /ac --
I'm worrying about the following cases for example: - hotplug controller driver allocates memory for slot name using other than kmalloc(). - hotplug controller driver allocates memory for slot name as a part of another data structure. Thanks, Kenji Kaneshige --
