PCI, ACPI: Physical PCI slot objects

From: Kenji Kaneshige <[email protected]>

Export kobject_rename() to fix the following link error. This happens when pci_hotplug_core driver is compiled as a kernel module.

ERROR: "kobject_rename" [drivers/pci/hotplug/pci_hotplug.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by Kenji Kaneshige <[email protected]>

Acked-by Alex Chiang <[email protected]>

Cc Greg KH <[email protected]>

From: Kenji Kaneshige <[email protected]>

Export kobject_rename() to fix the following link error. This happens when pci_hotplug_core driver is compiled as a kernel module.

ERROR: "kobject_rename" [drivers/pci/hotplug/pci_hotplug.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by Kenji Kaneshige <[email protected]>

Acked-by Alex Chiang <[email protected]>

Cc Greg KH <[email protected]>

Cc Kristen Carlson Accardi <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 lib/kobject.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lib/kobject.c +0 -0

index 718e510..dcade05 100644

Quoted 27 lines · On Wed, 4 Jun 2008 14:16:06 -0600 Alex Chiang <[email protected]> wrote:

From: Kenji Kaneshige <[email protected]>

Export kobject_rename() to fix the following link error. This happens when pci_hotplug_core driver is compiled as a kernel module.

ERROR: "kobject_rename" [drivers/pci/hotplug/pci_hotplug.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by Kenji Kaneshige <[email protected]>

Acked-by Alex Chiang <[email protected]>

Cc Greg KH <[email protected]>

Cc Kristen Carlson Accardi <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 lib/kobject.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lib/kobject.c +0 -0

index 718e510..dcade05 100644
@@ -439,6 +439,7 @@ out:
 

lib/kobject.c out: · line 442

Quoted 3 lines

lib/kobject.c +1 -0

 	return error;
 }
+EXPORT_SYMBOL_GPL(kobject_rename);

Shouldn't this go straight into 2.6.26?

Quoted 29 lines · Andrew Morton

Quoted 27 lines · On Wed, 4 Jun 2008 14:16:06 -0600 Alex Chiang <[email protected]> wrote:

From: Kenji Kaneshige <[email protected]>

Export kobject_rename() to fix the following link error. This happens when pci_hotplug_core driver is compiled as a kernel module.

ERROR: "kobject_rename" [drivers/pci/hotplug/pci_hotplug.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by Kenji Kaneshige <[email protected]>

Acked-by Alex Chiang <[email protected]>

Cc Greg KH <[email protected]>

Cc Kristen Carlson Accardi <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 lib/kobject.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lib/kobject.c +0 -0

index 718e510..dcade05 100644
@@ -439,6 +439,7 @@ out:
 

Quoted 7 lines

lib/kobject.c out: · line 442

Quoted 3 lines

lib/kobject.c +1 -0

 	return error;
 }
+EXPORT_SYMBOL_GPL(kobject_rename);

Shouldn't this go straight into 2.6.26?

I think "No", because currently its only required by Alex's "pci slot" patches.

Quoted 31 lines

Quoted 29 lines · Andrew Morton

Quoted 27 lines · On Wed, 4 Jun 2008 14:16:06 -0600 Alex Chiang <[email protected]> wrote:

From: Kenji Kaneshige <[email protected]>

Export kobject_rename() to fix the following link error. This happens when pci_hotplug_core driver is compiled as a kernel module.

ERROR: "kobject_rename" [drivers/pci/hotplug/pci_hotplug.ko] undefined! make[1]: *** [__modpost] Error 1
make: *** [modules] Error 2
make: *** Waiting for unfinished jobs....

Signed-off-by Kenji Kaneshige <[email protected]>

Acked-by Alex Chiang <[email protected]>

Cc Greg KH <[email protected]>

Cc Kristen Carlson Accardi <[email protected]>

Cc Len Brown <[email protected]>

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---
 lib/kobject.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

lib/kobject.c +0 -0

index 718e510..dcade05 100644
@@ -439,6 +439,7 @@ out:
 

Quoted 5 lines

Quoted 5 lines

lib/kobject.c out: · line 442

Quoted 3 lines

lib/kobject.c +1 -0

 	return error;
 }
+EXPORT_SYMBOL_GPL(kobject_rename);

Shouldn't this go straight into 2.6.26?

Quoted 2 lines

I think "No", because currently its only required by Alex's "pci slot" patches.

In that case the fix should be integrated into the patch which it is fixing.

Well, it doesn't _have_ to be integrated because in this case it can precede acpi-pci-slot-detection-driver.patch. But we might as well fold it, to keep the commit noise level down.

BTW. In the meantime, your current patch set seems fine to go.

I'll add code to actually do the renumbering of the slot separately, along with some other cleanup & rework of the pseries PCI hotplug I started doing.

* Benjamin Herrenschmidt <[email protected]>:

Quoted 1 line

BTW. In the meantime, your current patch set seems fine to go.

That sounds suspiciously like an Acked-by: to me... ;)

Quoted 3 lines

I'll add code to actually do the renumbering of the slot separately, along with some other cleanup & rework of the pseries PCI hotplug I started doing.

Great, thanks!

+#define ACPI_STA_FUNCTIONING (0x00000008)

Quoted 3 lines

+#define ACPI_STA_FUNCTIONING (0x00000008)

please use the existing define in acpi/actypes.h:
#define ACPI_STA_DEVICE_FUNCTIONING 0x08

I don't notice any breakage WRT ACPI usage here,
I defer to Jesse on the PCI-ness parts.

Acked-by Len Brown <[email protected]>

Hi Jesse, Ben, Kenji-san,

This is v14 of the physical PCI slot series.

This patchset has been kicking around -mm for the past few months, and they were getting clobbered on a continual basis, so let's say I'm quite motivated to get them into mainline. ;)

This mail describes two things:

- an update for handling pSeries
- explanation of how I did not regress Kenji-san's latest changes

Review comments are much appreciated.

-----------------------
pSeries-related changes
-----------------------
The most recent outstanding issue with this series was breakage with pSeries. In a nutshell, the problem was:

- pci_hp_register() interface changed to require a devfn when registering a slot

- pSeries doesn't necessarily know the devfn of an unpopulated slot

There are more details, of course, and they are in the archives. I can dig them up if people want more context.

After working offline with BenH and Willy, we thought that the best way forward was for the new infrastructure to provide a new API, pci_update_slot_number(), which can be used by callers to modify the slot number after slot creation.

This change goes hand in hand with modifying the semantics of pci_hp_register() where callers are now allowed to pass -1 for slot_nr to create a 'placeholder' slot.

The third related change is that the infrastructure will only display an 'address' value of 'dddd:bb' (domain:bus) when the device is -1. In the normal case, the full address of dddd:bb:dd is displayed.

I did fold an earlier -mm fixup patch into these changes to improve future bisectability.

I added kerneldoc to explain the APIs.

-----------------------------
maintaining Kenji-san's fixes
-----------------------------
Finally, this patch series slightly changes the logic introduced by Kenji-san's patches:

	9e4f2e8d4ddb04ad16a3828cd9a369a5a5287009
	a86161b3134465f072d965ca7508ec9c1e2e52c7

For a86161b31344, the check against registering a slot with the same name multiple times is removed. That check prevents a scenario where multiple pcihp drivers try to claim the same slot.

The check is removed because the new code allows multiple callers of pci_create_slot(). One callsite is pci_hp_register(), the other is in the ACPI slot detection driver provided in patch 4/4. In the case of multiple legitimate callers, the correct thing to do is refcount the struct pci_slot's kobj.

In the case of two pcihp drivers attempting to claim the same slot, pci_hp_register() returns -EBUSY to indicate it has already been claimed. This logic has been part of the patch series from the beginning.

Thus, the end behavior introduced by a86161b31344 is preserved, although in a slightly different implementation.

The firmware defect that Kenji-san is trying to fix with 9e4f2e8d4d is the case where broken firmware will present the kernel with slots like bb1:dd1, name "A" and bb2:dd2, name "A".

In other words, two different busses or two different devices on the same bus, but both have the same name.

In this case, pci_create_slot() thinks they are two different physical slots (which is true), and tries to register them with the kobject core, which will then complain about registering two objects with the same name. -EEXIST will be returned back up through the pcihp core and back to pciehp, which will then printk the message added by 9e4f2e8d4d.

Thus, the condition Kenji-san is trying to warn about with 9e4f2e8d4d is preserved.

Thanks,

/ac

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

I tried to test your patch using latest linux-next, but I could not apply your patch with the following messages:

I think maybe your patch is conflicting with the following patch.

commit 372fa8fa0a77c47c7bcfae0ccd14e0c5f97d17fe

Author: Kay Sievers <[email protected]>
Date:   Fri May 2 06:02:41 2008 +0200

driver-core: prepare for removal of 20 char limit from struct device

The access of struct device->bus_id is replaced by dev_name(), to be able to use the non-limited kobject name after the conversion of the device register callers.

Hi Kenji-san,

* Kenji Kaneshige <[email protected]>:

Quoted 2 lines

I tried to test your patch using latest linux-next, but I could not apply your patch with the following messages:

Sorry, that latest patch series was against Linus' tree, not linux-next.

Quoted 11 lines

I think maybe your patch is conflicting with the following patch.

commit 372fa8fa0a77c47c7bcfae0ccd14e0c5f97d17fe

Author: Kay Sievers <[email protected]>
Date:   Fri May 2 06:02:41 2008 +0200

driver-core: prepare for removal of 20 char limit from struct device

The access of struct device->bus_id is replaced by dev_name(), to be able to use the non-limited kobject name after the conversion of the device register callers.

Yes, thanks for pointing this out. I've fixed patch 1/4.

I verified that patches 2, 3 and 4 apply cleanly against linux-next, so this should be the only fix you'll need.

Sorry for the confusion.

/ac

commit c0175afea14cdaff3c33d894eb474eed1dcfa65d

Author: Alex Chiang <[email protected]>
Date:   Wed Jun 4 21:04:46 2008 -0600

Construct one fakephp slot per PCI slot

Register one slot per slot, rather than one slot per function. Change the name of the slot to fake%d instead of the pci address.

Signed-off-by Alex Chiang <[email protected]>

Signed-off-by Matthew Wilcox <[email protected]>

Cc Greg KH <[email protected]>

Cc Kristen Carlson Accardi <[email protected]>

Cc Len Brown <[email protected]>

Cc Kenji Kaneshige <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

drivers/pci/hotplug/fakephp.c +30 -61

index d3393f4..e066959 100644
@@ -66,6 +66,7 @@ struct dummy_slot {
 	struct pci_dev *dev;
 	struct work_struct remove_work;
 	unsigned long removed;
+	char name[8];
 };
 
 static int debug;

@@ -101,6 +102,7 @@ static int add_slot(struct pci_dev *dev)
 	struct dummy_slot *dslot;
 	struct hotplug_slot *slot;
 	int retval = -ENOMEM;
+	static int count = 1;
 
 	slot = kzalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
 	if (!slot)

@@ -114,15 +116,13 @@ static int add_slot(struct pci_dev *dev)
 	slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
 	slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
 
-	slot->name = kstrdup(dev_name(&dev->dev), GFP_KERNEL);
-	if (!slot->name)
-		goto error_info;
-	dbg("slot->name = %s\n", slot->name);
-
 	dslot = kzalloc(sizeof(struct dummy_slot), GFP_KERNEL);
 	if (!dslot)
-		goto error_name;
+		goto error_info;
 
+	slot->name = dslot->name;
+	snprintf(slot->name, sizeof(dslot->name), "fake%d", count++);
+	dbg("slot->name = %s\n", slot->name);
 	slot->ops = &dummy_hotplug_slot_ops;
 	slot->release = &dummy_release;
 	slot->private = dslot;

@@ -140,8 +140,6 @@ static int add_slot(struct pci_dev *dev)
 
 error_dslot:
 	kfree(dslot);
-error_name:
-	kfree(slot->name);
 error_info:
 	kfree(slot->info);
 error_slot:

@@ -153,17 +151,17 @@ error:
 static int __init pci_scan_buses(void)
 {
 	struct pci_dev *dev = NULL;
-	int retval = 0;
+	int lastslot = 0;
 
 	while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
-		retval = add_slot(dev);
-		if (retval) {
-			pci_dev_put(dev);
-			break;
-		}
+		if (PCI_FUNC(dev->devfn) > 0 &&
+				lastslot == PCI_SLOT(dev->devfn))
+			continue;
+		lastslot = PCI_SLOT(dev->devfn);
+		add_slot(dev);
 	}
 
-	return retval;
+	return 0;
 }
 
 static void remove_slot(struct dummy_slot *dslot)

@@ -301,23 +299,9 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
 	return 0;
 }
 
-/* find the hotplug_slot for the pci_dev */
-static struct hotplug_slot *get_slot_from_dev(struct pci_dev *dev)
-{
-	struct dummy_slot *dslot;
-
-	list_for_each_entry(dslot, &slot_list, node) {
-		if (dslot->dev == dev)
-			return dslot->slot;
-	}
-	return NULL;
-}
-
-
 static int disable_slot(struct hotplug_slot *slot)
 {
 	struct dummy_slot *dslot;
-	struct hotplug_slot *hslot;
 	struct pci_dev *dev;
 	int func;
 

@@ -327,41 +311,27 @@ static int disable_slot(struct hotplug_slot *slot)
 
 	dbg("%s - physical_slot = %s\n", __func__, slot->name);
 
-	/* don't disable bridged devices just yet, we can't handle them easily... */
-	if (dslot->dev->subordinate) {
-		err("Can't remove PCI devices with other PCI devices behind it yet.\n");
-		return -ENODEV;
-	}
-	if (test_and_set_bit(0, &dslot->removed)) {
-		dbg("Slot already scheduled for removal\n");
-		return -ENODEV;
-	}
-	/* search for subfunctions and disable them first */
-	if (!(dslot->dev->devfn & 7)) {
-		for (func = 1; func < 8; func++) {
-			dev = pci_get_slot(dslot->dev->bus,
-					dslot->dev->devfn + func);
-			if (dev) {
-				hslot = get_slot_from_dev(dev);
-				if (hslot)
-					disable_slot(hslot);
-				else {
-					err("Hotplug slot not found for subfunction of PCI device\n");
-					return -ENODEV;
-				}
-				pci_dev_put(dev);
-			} else
-				dbg("No device in slot found\n");
+	for (func = 7; func >= 0; func--) {
+		dev = pci_get_slot(dslot->dev->bus, dslot->dev->devfn + func);
+		if (!dev)
+			continue;
+
+		if (test_and_set_bit(0, &dslot->removed)) {
+			dbg("Slot already scheduled for removal\n");
+			return -ENODEV;
 		}
-	}
 
-	/* remove the device from the pci core */
-	pci_remove_bus_device(dslot->dev);
+		/* queue work item to blow away this sysfs entry and other
+		 * parts.
+		 */
+		INIT_WORK(&dslot->remove_work, remove_slot_worker);
+		queue_work(dummyphp_wq, &dslot->remove_work);
 
-	/* queue work item to blow away this sysfs entry and other parts. */
-	INIT_WORK(&dslot->remove_work, remove_slot_worker);
-	queue_work(dummyphp_wq, &dslot->remove_work);
+		/* blow away this sysfs entry and other parts. */
+		remove_slot(dslot);
 
+		pci_dev_put(dev);
+	}
 	return 0;
 }
 
--

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

Hi Jesse, Ben, Kenji-san,

This is v14 of the physical PCI slot series.

This patchset has been kicking around -mm for the past few months, and they were getting clobbered on a continual basis, so let's say I'm quite motivated to get them into mainline. ;)

This mail describes two things:

- an update for handling pSeries
- explanation of how I did not regress Kenji-san's latest changes

Review comments are much appreciated.

-----------------------
pSeries-related changes
-----------------------
The most recent outstanding issue with this series was breakage with pSeries. In a nutshell, the problem was:

- pci_hp_register() interface changed to require a devfn when registering a slot

- pSeries doesn't necessarily know the devfn of an unpopulated slot

There are more details, of course, and they are in the archives. I can dig them up if people want more context.

After working offline with BenH and Willy, we thought that the best way forward was for the new infrastructure to provide a new API, pci_update_slot_number(), which can be used by callers to modify the slot number after slot creation.

This change goes hand in hand with modifying the semantics of pci_hp_register() where callers are now allowed to pass -1 for slot_nr to create a 'placeholder' slot.

The third related change is that the infrastructure will only display an 'address' value of 'dddd:bb' (domain:bus) when the device is -1. In the normal case, the full address of dddd:bb:dd is displayed.

I did fold an earlier -mm fixup patch into these changes to improve future bisectability.

I added kerneldoc to explain the APIs.

-----------------------------
maintaining Kenji-san's fixes
-----------------------------
Finally, this patch series slightly changes the logic introduced by Kenji-san's patches:

	9e4f2e8d4ddb04ad16a3828cd9a369a5a5287009
	a86161b3134465f072d965ca7508ec9c1e2e52c7

For a86161b31344, the check against registering a slot with the same name multiple times is removed. That check prevents a scenario where multiple pcihp drivers try to claim the same slot.

The check is removed because the new code allows multiple callers of pci_create_slot(). One callsite is pci_hp_register(), the other is in the ACPI slot detection driver provided in patch 4/4. In the case of multiple legitimate callers, the correct thing to do is refcount the struct pci_slot's kobj.

In the case of two pcihp drivers attempting to claim the same slot, pci_hp_register() returns -EBUSY to indicate it has already been claimed. This logic has been part of the patch series from the beginning.

Thus, the end behavior introduced by a86161b31344 is preserved, although in a slightly different implementation.

The firmware defect that Kenji-san is trying to fix with 9e4f2e8d4d is the case where broken firmware will present the kernel with slots like bb1:dd1, name "A" and bb2:dd2, name "A".

In other words, two different busses or two different devices on the same bus, but both have the same name.

In this case, pci_create_slot() thinks they are two different physical slots (which is true), and tries to register them with the kobject core, which will then complain about registering two objects with the same name. -EEXIST will be returned back up through the pcihp core and back to pciehp, which will then printk the message added by 9e4f2e8d4d.

Thus, the condition Kenji-san is trying to warn about with 9e4f2e8d4d is preserved.

I tried your patches and I have a comment (question) about the behavior.

To emulate the (broken?) platform that assigns the same name to multiple slots, I made a debug patch for pciehp driver to assign same name ('1000') to all slots (my environment has two PCIe slots). With this patch, I noticed that the behavior or pci_hp_register() (or pci_create_slot()) varies depending on whether pci_slot driver is loaded or not. See below.

(a) With pci_slot driver loaded
I got the following error message when I loaded pciehp driver.

pciehp: pci_hp_register failed with error -17
pciehp: Failed to register slot because of name collision. Try 'pciehp_slot_with_bus' module option.
pciehp: pciehp: slot initialization failed
(b) Without pci_slot driver loaded
I got the kernel stack dump and error messages when I loaded pciehp driver.

	kobject_add_internal failed for 1000 with -EEXIST, don't try to
	register things with the same name in the same directory.

	Call Trace:
	 [<a000000100015180>] show_stack+0x40/0xa0
	                                sp=e0000040a086fb80 bsp=e0000040a0861158
	 [<a000000100015210>] dump_stack+0x30/0x60
                               sp=e0000040a086fd50 bsp=e0000040a0861140
	 [<a0000001003b3910>] kobject_add_internal+0x330/0x400
	                                sp=e0000040a086fd50 bsp=e0000040a0861100
	 [<a0000001003b3bd0>] kobject_add_varg+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a08610c8
	 [<a0000001003b3c90>] kobject_init_and_add+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a0861068
	 [<a0000001003d69b0>] pci_create_slot+0x150/0x260
	                                sp=e0000040a086fd80 bsp=e0000040a0861030
	 [<a000000200b71870>] pci_hp_register+0x130/0x880 [pci_hotplug]
	                                sp=e0000040a086fd80 bsp=e0000040a0860ff0
	 [<a000000200ec1a60>] pciehp_probe+0x720/0xca0 [pciehp]

	(snip...)

	Unable to register kobject 1000
	pciehp: pci_hp_register failed with error -17
	pciehp: Failed to register slot because of name collision. Try  
	'pciehp_slot_with_bus' module option.
	pciehp: pciehp: slot initialization failed

Could you tell me why that difference happen? And my expectation is the result should be (a) above regardless of whether pci_slot driver is loaded or not.

* Kenji Kaneshige <[email protected]>:

Quoted 47 lines

I tried your patches and I have a comment (question) about the behavior.

To emulate the (broken?) platform that assigns the same name to multiple slots, I made a debug patch for pciehp driver to assign same name ('1000') to all slots (my environment has two PCIe slots). With this patch, I noticed that the behavior or pci_hp_register() (or pci_create_slot()) varies depending on whether pci_slot driver is loaded or not. See below.

(a) With pci_slot driver loaded
I got the following error message when I loaded pciehp driver.

pciehp: pci_hp_register failed with error -17
pciehp: Failed to register slot because of name collision. Try 'pciehp_slot_with_bus' module option.
pciehp: pciehp: slot initialization failed
(b) Without pci_slot driver loaded
I got the kernel stack dump and error messages when I loaded pciehp driver.

	kobject_add_internal failed for 1000 with -EEXIST, don't try to
	register things with the same name in the same directory.

	Call Trace:
	 [<a000000100015180>] show_stack+0x40/0xa0
	                                sp=e0000040a086fb80 bsp=e0000040a0861158
	 [<a000000100015210>] dump_stack+0x30/0x60
                               sp=e0000040a086fd50 bsp=e0000040a0861140
	 [<a0000001003b3910>] kobject_add_internal+0x330/0x400
	                                sp=e0000040a086fd50 bsp=e0000040a0861100
	 [<a0000001003b3bd0>] kobject_add_varg+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a08610c8
	 [<a0000001003b3c90>] kobject_init_and_add+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a0861068
	 [<a0000001003d69b0>] pci_create_slot+0x150/0x260
	                                sp=e0000040a086fd80 bsp=e0000040a0861030
	 [<a000000200b71870>] pci_hp_register+0x130/0x880 [pci_hotplug]
	                                sp=e0000040a086fd80 bsp=e0000040a0860ff0
	 [<a000000200ec1a60>] pciehp_probe+0x720/0xca0 [pciehp]

	(snip...)

	Unable to register kobject 1000
	pciehp: pci_hp_register failed with error -17
	pciehp: Failed to register slot because of name collision. Try  
	'pciehp_slot_with_bus' module option.
	pciehp: pciehp: slot initialization failed

Quoted 3 lines

Could you tell me why that difference happen? And my expectation is the result should be (a) above regardless of whether pci_slot driver is loaded or not.

The difference was because in (a), pci_slot created the slots and when pciehp tried to register them, the pci slot infrastructure simply refcounted them and returned, but did not try to re-register the kobjects with the kobj core.

In (b), the pci hotplug core allowed you to create multiple slots with the same name, and called pci_create_slot() multiple times. This time, since you were trying to create new slot objects, we did not refcount them; we actually did a kzalloc *and* tried to register them with the kobject core, which is why we got that stack trace.

I read your patch (a86161b3134465f) in closer detail and decided that it can work without problems with my patch series.

- Your patch will keep track of hotplug slots and disallow multiple hotplug slots with the same name

- the PCI slot infrastructure will still allow multiple callers of pci_create_slot() to succeed by refcounting identical slots

This is the correct behavior to allow pciehp and pci_slot to coexist because pci_slot is not trying to register a hotplug handler or do any other hotplug operations.

Quoted 1 line · Alex Chiang

* Kenji Kaneshige <[email protected]>:

Thank you for explanation. I understood.

But I have one concern about the behavior when pci slot driver is not loaded. My patch (a86161b3134465f) prevents the kernel trace dump because of duplicate kobject add that would happen in the following two cases, though (b) is not described in the header of the patch (sorry).

(a) multiple driver attempt to handle the same slot.

(b) one or more driver attempt to register multiple slots with the same name (This can happen if broken platform assigns the same slot number to multiple hotplug slots, for example).

With your patch, duplicate kobject add in case (b) is not prevented. That is my concern.

I made a below patch to prevent (b), please take a look. And could you please consider merging it to "[PATCH 2/3] Introduce pci_slot" in your latest series.

Thanks,
Kenji Kaneshige

---
 drivers/pci/hotplug/pci_hotplug_core.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: 20080610/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================

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

@@ -555,6 +555,7 @@ int pci_hp_register(struct hotplug_slot 
 {
 	int result;
 	struct pci_slot *pci_slot;
+	struct hotplug_slot *tmp;
 
 	if (slot == NULL)
 		return -ENODEV;

@@ -567,6 +568,21 @@ int pci_hp_register(struct hotplug_slot 
 	}
 
 	/*
+	 * Prevent registering multiple hotplug slots with the same name.
+	 */
+	spin_lock(&pci_hotplug_slot_list_lock);
+	list_for_each_entry(tmp, &pci_hotplug_slot_list, slot_list) {
+		pci_slot = tmp->pci_slot;
+		if (pci_slot->bus == bus && pci_slot->number == slot_nr)
+			continue;
+		if (!strcmp(tmp->name, slot->name)) {

Quoted 32 lines

 drivers/pci/hotplug/pci_hotplug_core.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: 20080610/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================

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

@@ -555,6 +555,7 @@ int pci_hp_register(struct hotplug_slot
 {
 	int result;
 	struct pci_slot *pci_slot;
+	struct hotplug_slot *tmp;
 
 	if (slot == NULL)
 		return -ENODEV;

@@ -567,6 +568,21 @@ int pci_hp_register(struct hotplug_slot
 	}
 
 	/*
+	 * Prevent registering multiple hotplug slots with the same name.
+	 */
+	spin_lock(&pci_hotplug_slot_list_lock);
+	list_for_each_entry(tmp, &pci_hotplug_slot_list, slot_list) {
+		pci_slot = tmp->pci_slot;
+		if (pci_slot->bus == bus && pci_slot->number == slot_nr)
+			continue;
+		if (!strcmp(tmp->name, slot->name)) {
+			spin_unlock(&pci_hotplug_slot_list_lock);
+			return -EEXIST;
+		}
+	}
+	spin_unlock(&pci_hotplug_slot_list_lock);

drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot · line 588

Quoted 5 lines

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

+
+	/*
 	 * 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.

Please note that I made this patch against linux-next with your latest three patches applied.

Quoted 32 lines

Quoted 32 lines

 drivers/pci/hotplug/pci_hotplug_core.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: 20080610/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================

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

@@ -555,6 +555,7 @@ int pci_hp_register(struct hotplug_slot
 {
 	int result;
 	struct pci_slot *pci_slot;
+	struct hotplug_slot *tmp;
 
 	if (slot == NULL)
 		return -ENODEV;

@@ -567,6 +568,21 @@ int pci_hp_register(struct hotplug_slot
 	}
 
 	/*
+	 * Prevent registering multiple hotplug slots with the same name.
+	 */
+	spin_lock(&pci_hotplug_slot_list_lock);
+	list_for_each_entry(tmp, &pci_hotplug_slot_list, slot_list) {
+		pci_slot = tmp->pci_slot;
+		if (pci_slot->bus == bus && pci_slot->number == slot_nr)
+			continue;
+		if (!strcmp(tmp->name, slot->name)) {
+			spin_unlock(&pci_hotplug_slot_list_lock);
+			return -EEXIST;
+		}
+	}
+	spin_unlock(&pci_hotplug_slot_list_lock);

Quoted 8 lines

drivers/pci/hotplug/pci_hotplug_core.c int pci_hp_register(struct hotplug_slot · line 588

Quoted 5 lines

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

+
+	/*
 	 * 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.

Please note that I made this patch against linux-next with your latest three patches applied.

I looked this over and it seems like it's already handled in Alex's latest patchset (v15); can you take a look and make sure, Kenji-san (see Alex's latest mail to you too).

I just need acks from you and Kristen before pushing this...

Thanks,
Jesse

Quoted 70 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:

Quoted 47 lines

I tried your patches and I have a comment (question) about the behavior.

To emulate the (broken?) platform that assigns the same name to multiple slots, I made a debug patch for pciehp driver to assign same name ('1000') to all slots (my environment has two PCIe slots). With this patch, I noticed that the behavior or pci_hp_register() (or pci_create_slot()) varies depending on whether pci_slot driver is loaded or not. See below.

(a) With pci_slot driver loaded
I got the following error message when I loaded pciehp driver.

pciehp: pci_hp_register failed with error -17
pciehp: Failed to register slot because of name collision. Try 'pciehp_slot_with_bus' module option.
pciehp: pciehp: slot initialization failed
(b) Without pci_slot driver loaded
I got the kernel stack dump and error messages when I loaded pciehp driver.

	kobject_add_internal failed for 1000 with -EEXIST, don't try to
	register things with the same name in the same directory.

	Call Trace:
	 [<a000000100015180>] show_stack+0x40/0xa0
	                                sp=e0000040a086fb80 bsp=e0000040a0861158
	 [<a000000100015210>] dump_stack+0x30/0x60
                               sp=e0000040a086fd50 bsp=e0000040a0861140
	 [<a0000001003b3910>] kobject_add_internal+0x330/0x400
	                                sp=e0000040a086fd50 bsp=e0000040a0861100
	 [<a0000001003b3bd0>] kobject_add_varg+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a08610c8
	 [<a0000001003b3c90>] kobject_init_and_add+0x90/0xc0
	                                sp=e0000040a086fd50 bsp=e0000040a0861068
	 [<a0000001003d69b0>] pci_create_slot+0x150/0x260
	                                sp=e0000040a086fd80 bsp=e0000040a0861030
	 [<a000000200b71870>] pci_hp_register+0x130/0x880 [pci_hotplug]
	                                sp=e0000040a086fd80 bsp=e0000040a0860ff0
	 [<a000000200ec1a60>] pciehp_probe+0x720/0xca0 [pciehp]

	(snip...)

	Unable to register kobject 1000
	pciehp: pci_hp_register failed with error -17
	pciehp: Failed to register slot because of name collision. Try  
	'pciehp_slot_with_bus' module option.
	pciehp: pciehp: slot initialization failed

Quoted 3 lines

Could you tell me why that difference happen? And my expectation is the result should be (a) above regardless of whether pci_slot driver is loaded or not.

The difference was because in (a), pci_slot created the slots and when pciehp tried to register them, the pci slot infrastructure simply refcounted them and returned, but did not try to re-register the kobjects with the kobj core.

In (b), the pci hotplug core allowed you to create multiple slots with the same name, and called pci_create_slot() multiple times. This time, since you were trying to create new slot objects, we did not refcount them; we actually did a kzalloc *and* tried to register them with the kobject core, which is why we got that stack trace.

I read your patch (a86161b3134465f) in closer detail and decided that it can work without problems with my patch series.

- Your patch will keep track of hotplug slots and disallow multiple hotplug slots with the same name

Quoted 8 lines

- the PCI slot infrastructure will still allow multiple callers of pci_create_slot() to succeed by refcounting identical slots

This is the correct behavior to allow pciehp and pci_slot to coexist because pci_slot is not trying to register a hotplug handler or do any other hotplug operations.

Thank you for explanation. I understood.

But I have one concern about the behavior when pci slot driver is not loaded. My patch (a86161b3134465f) prevents the kernel trace dump because of duplicate kobject add that would happen in the following two cases, though (b) is not described in the header of the patch (sorry).

(a) multiple driver attempt to handle the same slot.

(b) one or more driver attempt to register multiple slots with the same name (This can happen if broken platform assigns the same slot number to multiple hotplug slots, for example).

With your patch, duplicate kobject add in case (b) is not prevented. That is my concern.

I made a below patch to prevent (b), please take a look. And could you please consider merging it to "[PATCH 2/3] Introduce pci_slot" in your latest series.

Thanks,
Kenji Kaneshige

---
 drivers/pci/hotplug/pci_hotplug_core.c |   16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Index: 20080610/drivers/pci/hotplug/pci_hotplug_core.c
===================================================================

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

@@ -555,6 +555,7 @@ int pci_hp_register(struct hotplug_slot 
 {
 	int result;
 	struct pci_slot *pci_slot;
+	struct hotplug_slot *tmp;
 
 	if (slot == NULL)
 		return -ENODEV;

@@ -567,6 +568,21 @@ int pci_hp_register(struct hotplug_slot 
 	}
 
 	/*
+	 * Prevent registering multiple hotplug slots with the same name.
+	 */
+	spin_lock(&pci_hotplug_slot_list_lock);
+	list_for_each_entry(tmp, &pci_hotplug_slot_list, slot_list) {
+		pci_slot = tmp->pci_slot;
+		if (pci_slot->bus == bus && pci_slot->number == slot_nr)
+			continue;
+		if (!strcmp(tmp->name, slot->name)) {
+			spin_unlock(&pci_hotplug_slot_list_lock);
+			return -EEXIST;
+		}
+	}
+	spin_unlock(&pci_hotplug_slot_list_lock);
+
+	/*
 	 * 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.

Note how we're checking get_slot_from_name. That should prevent your scenario (b) that you describe above.

Maybe the diff was confusing, but I am definitely not removing your code. I'm simply adding on top of a86161b3134465f, and not removing it.

Quoted 3 lines

I made a below patch to prevent (b), please take a look. And could you please consider merging it to "[PATCH 2/3] Introduce pci_slot" in your latest series.

Ok, now this is very confusing to me. Why is this patch so different from a86161b3134465f?

Are you saying the call to get_slot_from_name() is no longer sufficient?

Alex-san, Jesse-san,

Quoted 7 lines

Note how we're checking get_slot_from_name. That should prevent your scenario (b) that you describe above.

Maybe the diff was confusing, but I am definitely not removing your code. I'm simply adding on top of a86161b3134465f, and not removing it.

I have to apologize. I was using v14 unintentionally on my test environment yesterday, while I thought I was using v15.

I think v15 will prevent senario (b), though I have not tried it yet. I'll check it again.

And I agree that Alex-san's patch go to Jesse-san's linux-next. If I found something after that, I'll report it or send a incremental patch. To tell the truth, I have several patches that are waiting for Alex-san's patch to be merged to linux-next:)

Quoted 10 lines

Quoted 3 lines

I made a below patch to prevent (b), please take a look. And could you please consider merging it to "[PATCH 2/3] Introduce pci_slot" in your latest series.

Ok, now this is very confusing to me. Why is this patch so different from a86161b3134465f?

Are you saying the call to get_slot_from_name() is no longer sufficient?

Though I might misunderstand something about your patch, I thought get_slot_from_name() approach would break what your patch is trying to do.

My understanding about your patch is as follows:

(x) If multiple hotplug drivers try to register the same slot (try to handle the same slot, in other words), pci_hp_register() returns -EBUSY.

(y) If one or more drivers try to assign the same name to multiple slots, pci_hp_register() returns -EEXIST.

I thought senario (x) will return -EEXIST instead of -EBUSY if we use get_slot_from_name() approach. So I made a different patch.

In addition, regardless of whether my understanding is correct or not, I noticed my patch I sent yesterday might be not good, because I made it under the misunderstanding that I thought pci_hp_register() is called even by ACPI pci slot driver...

Anyway, I'll look at your patch again after having several cups of coffee.

Hi Kenji-san,

* Kenji Kaneshige <[email protected]>:

Quoted 9 lines

Alex-san, Jesse-san,

Quoted 7 lines

Note how we're checking get_slot_from_name. That should prevent your scenario (b) that you describe above.

Maybe the diff was confusing, but I am definitely not removing your code. I'm simply adding on top of a86161b3134465f, and not removing it.

Quoted 5 lines

I have to apologize. I was using v14 unintentionally on my test environment yesterday, while I thought I was using v15.

I think v15 will prevent senario (b), though I have not tried it yet. I'll check it again.

You can check either v15, which applies to Stephen Rothwell's linux-next, or you can check v16, which applies to Jesse's linux-next.

The only difference is in patch 1/3, where we are touching fakephp, which is not the patch that is confusing us here. :)

Quoted 4 lines

And I agree that Alex-san's patch go to Jesse-san's linux-next. If I found something after that, I'll report it or send a incremental patch. To tell the truth, I have several patches that are waiting for Alex-san's patch to be merged to linux-next:)

Yeah, I think incremental patches from here out are good.

Quoted 19 lines

Quoted 9 lines

Quoted 3 lines

I made a below patch to prevent (b), please take a look. And could you please consider merging it to "[PATCH 2/3] Introduce pci_slot" in your latest series.

Ok, now this is very confusing to me. Why is this patch so different from a86161b3134465f?

Are you saying the call to get_slot_from_name() is no longer sufficient?

Though I might misunderstand something about your patch, I thought get_slot_from_name() approach would break what your patch is trying to do.

My understanding about your patch is as follows:

(x) If multiple hotplug drivers try to register the same slot (try to handle the same slot, in other words), pci_hp_register() returns -EBUSY.

Quoted 2 lines

(y) If one or more drivers try to assign the same name to multiple slots, pci_hp_register() returns -EEXIST.

That was the original intent, but I think that returning -EEXIST for (x) should be sufficient. If it turns out we really do want -EBUSY for (x), we can add your latest fixup patch later.