Fixups for duplicate slot names

PATCH In review Alex Chiang 2 participants 11 messages Aug 6, 2008 – Aug 11, 2008

Subsystems (2)

ACPI PCI

lore

Share & tools

lore

Alex Chiang

author

2008-08-06 05:07 UTC #0 lore raw

cover Fixups for duplicate slot names

* Kenji Kaneshige <[email protected]>:

Quoted 7 lines

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

Alex Chiang

author

2008-08-06 05:11 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/pci/hotplug/acpiphp.h      |    2 +-
 drivers/pci/hotplug/acpiphp_core.c |   17 ++++++++++-------
 2 files changed, 11 insertions(+), 8 deletions(-)

drivers/pci/hotplug/acpiphp.h +1 -1

index 5a58b07..2ac519d 100644
@@ -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;
 };
 
 /*

drivers/pci/hotplug/acpiphp_core.c +10 -7

index 0e496e8..ec164f4 100644
@@ -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:

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:11 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/pci/hotplug/fakephp.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

drivers/pci/hotplug/fakephp.c +12 -3

index 40337a0..af8a1bf 100644
@@ -60,13 +60,15 @@
 #define DRIVER_AUTHOR	"Greg Kroah-Hartman <[email protected]>"
 #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:

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:11 UTC #3 lore raw

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

In reply to 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 <[email protected]>

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

drivers/pci/hotplug/pciehp.h +2 -4

index e3a1e7e..27dda64 100644
@@ -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;

drivers/pci/hotplug/pciehp_core.c +0 -7

index 3677495..6c1df8d 100644
@@ -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 */

drivers/pci/hotplug/pciehp_hpc.c +8 ~~-10~~

index ad27e9e..4ae7d58 100644
@@ -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);
 }

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:12 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/pci/hotplug/pcihp_skeleton.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

drivers/pci/hotplug/pcihp_skeleton.c +9 -2

index e3dd6cf..3e53026 100644
@@ -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:

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:12 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/pci/hotplug/shpchp.h      |    5 ++---
 drivers/pci/hotplug/shpchp_core.c |   28 +++++++++-------------------
 2 files changed, 11 insertions(+), 22 deletions(-)

drivers/pci/hotplug/shpchp.h +2 -3

index 8a026f7..8da1044 100644
@@ -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 {

drivers/pci/hotplug/shpchp_core.c +9 ~~-19~~

index a8cbd03..c490380 100644
@@ -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:

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:13 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/acpi/pci_slot.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

drivers/acpi/pci_slot.c +12 -2

index d5b4ef8..b146b72 100644
@@ -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;
 }

Signature · 1 line

1.6.0.rc0.g95f8

Alex Chiang

author

2008-08-06 05:13 UTC #7 lore raw

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

In reply to 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 <[email protected]>

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

drivers/pci/hotplug/pci_hotplug_core.c +0 -4

index 5f85b1b..4476f0c 100644
@@ -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

drivers/pci/slot.c +58 -7

106 lines not on this page · raw message

include/linux/pci.h +1 -1

index 825be38..4b6e847 100644
@@ -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);

Signature · 1 line

1.6.0.rc0.g95f8

Kenji Kaneshige

2008-08-07 08:44 UTC #8 lore raw

In reply to Alex Chiang

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

Quoted 60 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:
Quoted 6 lines
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Alex Chiang

author

2008-08-07 17:25 UTC #9 lore raw

* Kenji Kaneshige <[email protected]>:

Quoted 7 lines

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.

Quoted 8 lines

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

Kenji Kaneshige

2008-08-11 01:20 UTC #10 lore raw

Quoted 30 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:
Quoted 7 lines
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.
Quoted 8 lines
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.

Quoted 6 lines

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