Re: [PATCH 02/13] PCI: prevent duplicate slot names

Kenji Kaneshige <kaneshige.kenji@xxxxxxxxxxxxxx> · Thu, 21 Aug 2008 19:24:37 +0900

Hi Alex-san,

Thank you for updated patches and I'm sorry for delayed comment.
I reviewed your patch and I found two problems. Please see below.

Alex Chiang wrote:
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
	The Mth registered slot becomes N-M

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.

Finally, since we now prevent duplicate slot names, we remove
the logic introduced by the following commits:

	pci hotplug core: add check of duplicate slot name
	a86161b3134465f072d965ca7508ec9c1e2e52c7

	pciehp: fix slot name
	3800345f723fd130d50434d4717b99d4a9f383c8

	pciehp: add message about pciehp_slot_with_bus option
	9e4f2e8d4ddb04ad16a3828cd9a369a5a5287009

	shpchp: fix slot name
	ef0ff95f136f0f2d035667af5d18b824609de320

	shpchp: add message about shpchp_slot_with_bus option
	b3bd307c628af2f0a581c42d5d7e4bcdbbf64b6a

Cc: jbarnes@xxxxxxxxxxxxxxxx
Cc: kristen.c.accardi@xxxxxxxxx
Cc: matthew@xxxxxx
Cc: kaneshige.kenji@xxxxxxxxxxxxxx
Signed-off-by: Alex Chiang <achiang@xxxxxx>
---

 drivers/pci/hotplug/pci_hotplug_core.c |    4 -
 drivers/pci/hotplug/pciehp.h           |    1 
 drivers/pci/hotplug/pciehp_core.c      |    7 --
 drivers/pci/hotplug/pciehp_hpc.c       |    6 --
 drivers/pci/hotplug/shpchp_core.c      |   14 -----
 drivers/pci/slot.c                     |   93 +++++++++++++++++++++++++-------
 6 files changed, 76 insertions(+), 49 deletions(-)

diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
index 96f274e..da5908f 100644
--- a/drivers/pci/hotplug/pci_hotplug_core.c
+++ b/drivers/pci/hotplug/pci_hotplug_core.c
@@ -570,10 +570,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(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/hotplug/pciehp.h b/drivers/pci/hotplug/pciehp.h
index e3a1e7e..9e6cec6 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...)						\
diff --git a/drivers/pci/hotplug/pciehp_core.c b/drivers/pci/hotplug/pciehp_core.c
index 5952315..bed77af 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"
 
@@ -226,10 +223,6 @@ static int init_slots(struct controller *ctrl)
 					 slot->name);
 		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..43ff979 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -1032,11 +1032,7 @@ static void pcie_shutdown_notification(struct controller *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);
+	snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
 }
 
 static int pcie_init_slot(struct controller *ctrl)
diff --git a/drivers/pci/hotplug/shpchp_core.c b/drivers/pci/hotplug/shpchp_core.c
index 7bc06c0..136d9ea 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"
 
@@ -101,12 +98,7 @@ static void release_slot(struct hotplug_slot *hotplug_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);
+	snprintf(slot->hotplug_slot->name, SLOT_NAME_SIZE, "%d", slot->number);
 }
 
 static int init_slots(struct controller *ctrl)
@@ -162,10 +154,6 @@ static int init_slots(struct controller *ctrl)
 				hotplug_slot->name);
 		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;
 		}
 
diff --git a/drivers/pci/slot.c b/drivers/pci/slot.c
index 7e5b85c..42f0e12 100644
--- a/drivers/pci/slot.c
+++ b/drivers/pci/slot.c
@@ -73,6 +73,50 @@ 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, width, dup = 1;
+	struct kobject *dup_slot;
+
+	new_name = kstrdup(name, GFP_KERNEL);
+	if (!new_name)
+		goto out;
+
+	/*
+	 * Start off allocating enough room for "name-X"
+	 */
+	len = strlen(name) + 2;
+	width = 1;
+
+try_again:
+	dup_slot = kset_find_obj(pci_slots_kset, new_name);
+	if (!dup_slot)
+		goto out;
+

The kset_find_obj() increments reference counter of specified kobject. So
we must call kobject_put() for 'dup_slot', otherwise it leaks reference
counter of 'dup_slot' kobject and the corresponding slot directory will
never be removed. Here is a sample to fix this problem.

	dup_slot = kset_find_ojb(pci_slots_kset, new_name);
	if (!dup_slot)
		goto out;
	kobject_put(dup_slot);
	^^^^^^^^^^^^^^^^^^^^^^ Added this line


I found another problem in pci_hp_register() that would be more complex
to fix than above-mentioned problem. Here is a pci_hp_register() with
all of your patch applied.

int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
                       const char *name)
{
	(snip...)

       /*
        * 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.
        */
       pci_slot = pci_create_slot(bus, slot_nr, name);
       if (IS_ERR(pci_slot))
               return PTR_ERR(pci_slot);

       if (pci_slot->hotplug) {
               dbg("%s: already claimed\n", __func__);
               pci_destroy_slot(pci_slot);
               return -EBUSY;
       }

       slot->pci_slot = pci_slot;
       pci_slot->hotplug = slot;

       /*
        * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
        */
       if (strcmp(kobject_name(&pci_slot->kobj), name)) {
               result = kobject_rename(&pci_slot->kobj, name);
               if (result) {
                       pci_destroy_slot(pci_slot);
                       return result;
               }
       }

	(snip...)
}

If name duplication was detected in pci_create_slot(), it renames the
slot name like 'N-1' and return successfully. Even though slot's kobject
name was registered as 'N-1', 'name' array still have 'N' at this point.
So the following 'if' statement becomes true unexpectedly.

	/*
        * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
        */
       if (strcmp(kobject_name(&pci_slot->kobj), name)) {

Then pci_hp_register() attempt to rename the slot name with 'N' again
by calling kobject_rename(), but it fails because there already exists
kobject with name 'N'. As a result, pci_hp_register() will fail.

Thanks,
Kenji Kaneshige


--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




