[PATCH 0/7] Fixups for duplicate slot names

[PATCH 0/7] Fixups for duplicate slot names

[Alex Chiang]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>
> With this change, kobject_init_and_add() called in
> pci_create_slot() will show stack trace if a hotplug driver
> attempts to register multiple slot with the same name. That is,
> stack trace will be shown on the platform that wrongly assing
> the physical slot number to multiple slots. I'm very sorry, but
> I don't have enough time to consider how to fix it today.

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(-)

[PATCH 1/7] acpiphp: convert to a kmalloc'ed slot name

[Alex Chiang]

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,
 					acpiphp_slot->bridge->pci_bus,
 					acpiphp_slot->device);
-	if (retval == -EBUSY)
-		goto error_hpslot;
-	if (retval) {
-		err("pci_hp_register failed with error %d\n", retval);
+	if (retval)
 		goto error_hpslot;
- 	}
 
 	info("Slot [%s] registered\n", slot->hotplug_slot->name);
 
 	return 0;
 error_hpslot:
 	kfree(slot->hotplug_slot);
+error_name:
+	kfree(slot->name);
 error_slot:
 	kfree(slot);
 error:
-- 
1.6.0.rc0.g95f8

[PATCH 2/7] fakephp: convert to a kmalloc'ed slot name

[Alex Chiang]

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 retval;
 
+error_name:
+	kfree(dslot->name);
 error_dslot:
 	kfree(dslot);
 error_info:
-- 
1.6.0.rc0.g95f8

[PATCH 3/7] pciehp: convert to a kmalloc'ed slot name

[Alex Chiang]

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
+++ b/drivers/pci/hotplug/pciehp_core.c
@@ -41,7 +41,6 @@ int pciehp_debug;
 int pciehp_poll_mode;
 int pciehp_poll_time;
 int pciehp_force;
-int pciehp_slot_with_bus;
 struct workqueue_struct *pciehp_wq;
 
 #define DRIVER_VERSION	"0.4"
@@ -56,12 +55,10 @@ module_param(pciehp_debug, bool, 0644);
 module_param(pciehp_poll_mode, bool, 0644);
 module_param(pciehp_poll_time, int, 0644);
 module_param(pciehp_force, bool, 0644);
-module_param(pciehp_slot_with_bus, bool, 0644);
 MODULE_PARM_DESC(pciehp_debug, "Debugging mode enabled or not");
 MODULE_PARM_DESC(pciehp_poll_mode, "Using polling mechanism for hot-plug events or not");
 MODULE_PARM_DESC(pciehp_poll_time, "Polling mechanism frequency, in seconds");
 MODULE_PARM_DESC(pciehp_force, "Force pciehp, even if _OSC and OSHP are missing");
-MODULE_PARM_DESC(pciehp_slot_with_bus, "Use bus number in the slot name");
 
 #define PCIE_MODULE_NAME "pciehp"
 
@@ -225,10 +222,6 @@ static int init_slots(struct controller *ctrl)
 					 slot->device);
 		if (retval) {
 			err("pci_hp_register failed with error %d\n", retval);
-			if (retval == -EEXIST)
-				err("Failed to register slot because of name "
-				    "collision. Try \'pciehp_slot_with_bus\' "
-				    "module option.\n");
 			goto error_info;
 		}
 		/* create additional sysfs entries */
diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index ad27e9e..4ae7d58 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -1030,15 +1030,6 @@ static void pcie_shutdown_notification(struct controller *ctrl)
 	pciehp_free_irq(ctrl);
 }
 
-static void make_slot_name(struct slot *slot)
-{
-	if (pciehp_slot_with_bus)
-		snprintf(slot->name, SLOT_NAME_SIZE, "%04d_%04d",
-			 slot->bus, slot->number);
-	else
-		snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
-}
-
 static int pcie_init_slot(struct controller *ctrl)
 {
 	struct slot *slot;
@@ -1047,13 +1038,19 @@ static int pcie_init_slot(struct controller *ctrl)
 	if (!slot)
 		return -ENOMEM;
 
+	slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+	if (!slot->name) {
+		kfree(slot);
+		return -ENOMEM;
+	}
+
 	slot->hp_slot = 0;
 	slot->ctrl = ctrl;
 	slot->bus = ctrl->pci_dev->subordinate->number;
 	slot->device = ctrl->slot_device_offset + slot->hp_slot;
 	slot->hpc_ops = ctrl->hpc_ops;
 	slot->number = ctrl->first_slot;
-	make_slot_name(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);
@@ -1068,6 +1065,7 @@ static void pcie_cleanup_slot(struct controller *ctrl)
 	cancel_delayed_work(&slot->work);
 	flush_scheduled_work();
 	flush_workqueue(pciehp_wq);
+	kfree(slot->name);
 	kfree(slot);
 }
 
-- 
1.6.0.rc0.g95f8

[PATCH 4/7] PCI hotplug: convert skeleton code to a kmalloc'ed slot name

[Alex Chiang]

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

[PATCH 5/7] shpchp: convert to a kmalloc'ed slot name

[Alex Chiang]

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, "Debugging mode enabled or not");
 MODULE_PARM_DESC(shpchp_poll_mode, "Using polling mechanism for hot-plug events or not");
 MODULE_PARM_DESC(shpchp_poll_time, "Polling mechanism frequency, in seconds");
-MODULE_PARM_DESC(shpchp_slot_with_bus, "Use bus number in the slot name");
 
 #define SHPC_MODULE_NAME "shpchp"
 
@@ -96,19 +93,10 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
 
 	kfree(slot->hotplug_slot->info);
 	kfree(slot->hotplug_slot);
+	kfree(slot->name);
 	kfree(slot);
 }
 
-static void make_slot_name(struct slot *slot)
-{
-	if (shpchp_slot_with_bus)
-		snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%04d_%04d",
-			 slot->bus, slot->number);
-	else
-		snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%d",
-			 slot->number);
-}
-
 static int init_slots(struct controller *ctrl)
 {
 	struct slot *slot;
@@ -122,9 +110,13 @@ static int init_slots(struct controller *ctrl)
 		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);
@@ -146,7 +138,7 @@ static int init_slots(struct controller *ctrl)
 		/* register this slot with the hotplug pci core */
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
-		make_slot_name(slot);
+		snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
 		hotplug_slot->ops = &shpchp_hotplug_slot_ops;
 
 		get_power_status(hotplug_slot, &info->power_status);
@@ -161,10 +153,6 @@ static int init_slots(struct controller *ctrl)
 				ctrl->pci_dev->subordinate, slot->device);
 		if (retval) {
 			err("pci_hp_register failed with error %d\n", retval);
-			if (retval == -EEXIST)
-				err("Failed to register slot because of name "
-                                    "collision. Try \'shpchp_slot_with_bus\' "
-				    "module option.\n");
 			goto error_info;
 		}
 
@@ -176,6 +164,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

[PATCH 6/7] pci_slot: convert to a kmalloc'ed slot name

[Alex Chiang]

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

[PATCH 7/7] PCI Hotplug core: fixups for duplicate slot names

[Alex Chiang]

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.
  *
- * 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
+ * The Mth slot is assigned N-M
  *
  * Placeholder slots:
  * In most cases, @pci_bus, @slot_nr will be sufficient to uniquely identify
@@ -103,13 +115,15 @@ static struct kobj_type pci_slot_ktype = {
  * 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)
+				 char *name)
 {
+	struct kobject *dup_slot;
 	struct pci_slot *slot;
-	int err;
+	char *old_name;
+	int err, len, width, dup = 1;
 
 	down_write(&pci_bus_sem);
 
@@ -138,8 +152,43 @@ placeholder:
 
 	slot->bus = parent;
 	slot->number = slot_nr;
-
 	slot->kobj.kset = pci_slots_kset;
+
+	old_name = kstrdup(name, GFP_KERNEL);
+	if (!old_name) {
+		err = -ENOMEM;
+		goto err_oldname;
+	}
+
+	/*
+	 * Start off allocating enough room for "name-X"
+	 */
+	len = strlen(name) + 2;
+	width = 1;
+duplicate_name:
+	dup_slot = kset_find_obj(pci_slots_kset, name);
+	if (dup_slot) {
+		/*
+		 * We hit this the first time through, which gives us
+		 * space for terminating NULL, and then every power of 10
+		 * afterwards, which gives us space to add another digit
+		 * to "name-XX..."
+		 */
+		if (dup % width == 0) {
+			len++;
+			width *= 10;
+		}
+		kfree(name);
+		name = kzalloc(len, GFP_KERNEL);
+		if (!name) {
+			err = -ENOMEM;
+			goto err;
+		}
+
+		snprintf(name, len, "%s-%d", old_name, dup++);
+		goto duplicate_name;
+	}
+
 	err = kobject_init_and_add(&slot->kobj, &pci_slot_ktype, NULL,
 				   "%s", name);
 	if (err) {
@@ -158,6 +207,8 @@ placeholder:
 	up_write(&pci_bus_sem);
 	return slot;
  err:
+	kfree(old_name);
+ err_oldname:
 	kfree(slot);
 	slot = ERR_PTR(err);
 	goto out;
@@ -172,7 +223,7 @@ EXPORT_SYMBOL_GPL(pci_create_slot);
  * The primary purpose of this interface is to allow callers who earlier
  * 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)
 {
@@ -202,7 +253,7 @@ EXPORT_SYMBOL_GPL(pci_update_slot_number);
  * %struct pci_slot is refcounted, so destroying them is really easy; we
  * just call kobject_put on its kobj and let our release methods do the
  * rest.
- */
+ **/
 
 void pci_destroy_slot(struct pci_slot *slot)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 825be38..4b6e847 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -509,7 +509,7 @@ struct pci_bus *pci_create_bus(struct device *parent, int bus,
 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);
+				 char *name);
 void pci_destroy_slot(struct pci_slot *slot);
 void pci_update_slot_number(struct pci_slot *slot, int slot_nr);
 int pci_scan_slot(struct pci_bus *bus, int devfn);
-- 
1.6.0.rc0.g95f8

Re: [PATCH 0/7] Fixups for duplicate slot names

[Kenji Kaneshige]

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


Alex Chiang wrote:
> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>> With this change, kobject_init_and_add() called in
>> pci_create_slot() will show stack trace if a hotplug driver
>> attempts to register multiple slot with the same name. That is,
>> stack trace will be shown on the platform that wrongly assing
>> the physical slot number to multiple slots. I'm very sorry, but
>> I don't have enough time to consider how to fix it today.
> 
> 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(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 0/7] Fixups for duplicate slot names

[Alex Chiang]

* Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
> 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).

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

Re: [PATCH 0/7] Fixups for duplicate slot names

[Kenji Kaneshige]

Alex Chiang wrote:
> * Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com>:
>> 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).
> 
> 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