PCI: let the core manage slot names

PATCH Superseded Alex Chiang 2 participants 27 messages Oct 9, 2008 – Oct 10, 2008

Subsystems (5)

PowerPC ACPI PCI IBM Power PCI Hotplug Driver for RPA-compliant PPC64 platform +1 more subsystems

lore

Share & tools

lore

Alex Chiang

author

2008-10-09 04:46 UTC #0 lore raw

v5 cover PCI: let the core manage slot names

Hi all,

This is v5 of the series that implements a series of changes that allows the PCI core to manage slot names, rather than individual hotplug drivers.

Thanks again to Kenji-san for continuing to point out that my previous attempts were racy and had horrible, complicated code. :) And thanks to Willy for reviewing my poor attempt at introducing locking.

I've decided to do them both a favor and take a new approach that does not change any locking semantics in any of the existing interfaces today (pci_hp_register, pci_hp_deregister, pci_create_slot).

Keep in mind that the problems we're trying to solve are:

- possible duplicate slot names from firmware
- allow hotplug drivers to override detection driver slot names

The race that Kenji-san points out occurs during slot creation, where both a detection driver and hotplug driver try to create a slot at the same time, with different names.

The solution is to combine slot creation _and_ slot rename into one atomic operation via an updated pci_create_slot() API. pci_create_slot is already serialized, so that's a good start.

We add a 'rename' parameter to pci_create_slot. The hotplug drivers set it to 1. The detection drivers set it to 0. Now, when trying to create a new slot:

- check to see if it's already been created
- if yes, and we want a rename, then go ahead and rename it
- if yes, and we don't care about the name, then just return
- if no, then create the slot normally

This logic is fully described in the changelog in patch 04/16. I believe it is a much simpler approach than the earlier crap I was producing.

I tested this series with the pci_slot driver and fakephp dup_slots=1 with multiple loads and unloads in different order, and it all seemed to work.

I _hope_ this is the last round. ;)

Thanks!

/ac

v4 -> v5:
- add 'rename' param to pci_create_slot
- make use of 'rename' param in pci_create_slot
- remove v4 serialization
- remove crap false name collsion code from v2 and v3
- rpaphp uses kstrdup (Thanks to Pekka Enberg for suggestion)

v3 -> v4:
- Do not access hotplug_slot_name() before name initialization
- Serialize pci_hp_register/deregister

v2 -> v3:
- incorporate Willy's code review comments
- fix possible memory leak, pointed out by Rolf Eike Beer
- make false name collision detection work for empty slots
- add 'dup_slots' module_param to fakephp to help debug all this ;)

v1 -> v2:
- fix possible false name collisions

---

Alex Chiang (16):
PCI Hotplug: fakephp: add duplicate slot name debugging
PCI: Hotplug core: remove 'name'
PCI: shcphp: remove 'name' parameter
PCI: SGI Hotplug: stop managing bss_hotplug_slot->name
PCI: rpaphp: kmalloc/kfree slot->name directly
PCI: pciehp: remove 'name' parameter
PCI: ibmphp: stop managing hotplug_slot->name
PCI: fakephp: remove 'name' parameter
PCI: cpqphp: stop managing hotplug_slot->name
PCI: cpci_hotplug: stop managing hotplug_slot->name
PCI: acpiphp: remove 'name' parameter
PCI, PCI Hotplug: introduce slot_name helpers
PCI: prevent duplicate slot names
PCI: update pci_create_slot() to take a 'rename' param
PCI: rename pci_update_slot_number to pci_renumber_slot PCI Hotplug core: add 'name' param pci_hp_register interface

 drivers/acpi/pci_slot.c                 |    2 
 drivers/pci/hotplug/acpiphp.h           |    9 +-
 drivers/pci/hotplug/acpiphp_core.c      |   32 ++++---
 drivers/pci/hotplug/cpci_hotplug.h      |    6 +
 drivers/pci/hotplug/cpci_hotplug_core.c |   75 ++++++----------
 drivers/pci/hotplug/cpci_hotplug_pci.c  |    4 -
 drivers/pci/hotplug/cpqphp.h            |   13 +--
 drivers/pci/hotplug/cpqphp_core.c       |   43 ++++-----
 drivers/pci/hotplug/fakephp.c           |   26 ++++-
 drivers/pci/hotplug/ibmphp.h            |    5 -
 drivers/pci/hotplug/ibmphp_ebda.c       |   19 +---
 drivers/pci/hotplug/pci_hotplug_core.c  |   30 ++----
 drivers/pci/hotplug/pciehp.h            |    9 +-
 drivers/pci/hotplug/pciehp_core.c       |   49 ++++------
 drivers/pci/hotplug/pciehp_ctrl.c       |   53 ++++++-----
 drivers/pci/hotplug/pciehp_hpc.c        |    1 
 drivers/pci/hotplug/rpaphp_slot.c       |   10 +-
 drivers/pci/hotplug/sgi_hotplug.c       |   18 +---
 drivers/pci/hotplug/shpchp.h            |    9 +-
 drivers/pci/hotplug/shpchp_core.c       |   52 ++++-------
 drivers/pci/hotplug/shpchp_ctrl.c       |   48 +++++-----
 drivers/pci/slot.c                      |  150 ++++++++++++++++++++++++-------
 include/linux/pci.h                     |    9 +-
 include/linux/pci_hotplug.h             |   11 +-
 24 files changed, 359 insertions(+), 324 deletions(-)

Alex Chiang

author

2008-10-09 04:49 UTC #1 lore raw

PATCH v5 1/16 PCI Hotplug core: add 'name' param pci_hp_register interface

Update pci_hp_register() to take a const char *name parameter.

The motivation for this is to clean up the individual hotplug drivers so that each one does not have to manage its own name. The PCI core should be the place where we manage the name.

We update the interface and all callsites first, in a "no functional change" manner, and clean up the drivers later.

Cc [email protected]

Reviewed-by Matthew Wilcox <[email protected]>

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

---

 drivers/pci/hotplug/acpiphp_core.c      |    3 ++-
 drivers/pci/hotplug/cpci_hotplug_core.c |    3 ++-
 drivers/pci/hotplug/cpqphp_core.c       |    3 ++-
 drivers/pci/hotplug/fakephp.c           |    3 ++-
 drivers/pci/hotplug/ibmphp_ebda.c       |    3 ++-
 drivers/pci/hotplug/pci_hotplug_core.c  |   15 ++++++++-------
 drivers/pci/hotplug/pciehp_core.c       |    3 ++-
 drivers/pci/hotplug/rpaphp_slot.c       |    2 +-
 drivers/pci/hotplug/sgi_hotplug.c       |    3 ++-
 drivers/pci/hotplug/shpchp_core.c       |    3 ++-
 include/linux/pci_hotplug.h             |    3 ++-
 11 files changed, 27 insertions(+), 17 deletions(-)

drivers/pci/hotplug/acpiphp_core.c +2 -1

index 0e496e8..e984176 100644
@@ -340,7 +340,8 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 
 	retval = pci_hp_register(slot->hotplug_slot,
 					acpiphp_slot->bridge->pci_bus,
-					acpiphp_slot->device);
+					acpiphp_slot->device,
+					slot->name);
 	if (retval == -EBUSY)
 		goto error_hpslot;
 	if (retval) {

drivers/pci/hotplug/cpci_hotplug_core.c +2 -1

index 9359479..5e5dee8 100644
@@ -285,7 +285,8 @@ cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last)
 		info->attention_status = cpci_get_attention_status(slot);
 
 		dbg("registering slot %s", slot->hotplug_slot->name);
-		status = pci_hp_register(slot->hotplug_slot, bus, i);
+		status = pci_hp_register(slot->hotplug_slot, bus, i,
+					 slot->hotplug_slot->name);
 		if (status) {
 			err("pci_hp_register failed with error %d", status);
 			goto error_name;

drivers/pci/hotplug/cpqphp_core.c +2 -1

index 54defec..a7fe458 100644
@@ -436,7 +436,8 @@ static int ctrl_slot_setup(struct controller *ctrl,
 				slot_number);
 		result = pci_hp_register(hotplug_slot,
 					 ctrl->pci_dev->subordinate,
-					 slot->device);
+					 slot->device,
+					 hotplug_slot->name);
 		if (result) {
 			err("pci_hp_register failed with error %d\n", result);
 			goto error_name;

drivers/pci/hotplug/fakephp.c +2 -1

index 146ca9c..3069f21 100644
@@ -126,7 +126,8 @@ static int add_slot(struct pci_dev *dev)
 	slot->release = &dummy_release;
 	slot->private = dslot;
 
-	retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn));
+	retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn),
+				 slot->name);
 	if (retval) {
 		err("pci_hp_register failed with error %d\n", retval);
 		goto error_dslot;

drivers/pci/hotplug/ibmphp_ebda.c +2 -1

index 8f286ac..85528d6 100644
@@ -966,7 +966,8 @@ static int __init ebda_rsrc_controller (void)
 	list_for_each_entry(tmp_slot, &ibmphp_slot_head, ibm_slot_list) {
 		snprintf (tmp_slot->hotplug_slot->name, 30, "%s", create_file_name (tmp_slot));
 		pci_hp_register(tmp_slot->hotplug_slot,
-			pci_find_bus(0, tmp_slot->bus), tmp_slot->device);
+			pci_find_bus(0, tmp_slot->bus), tmp_slot->device,
+			tmp_slot->hotplug_slot->name);
 	}
 
 	print_ebda_hpc ();

drivers/pci/hotplug/pci_hotplug_core.c +8 -7

index 27d2b6f..0e7a511 100644
@@ -547,13 +547,15 @@ out:
  * @bus: bus this slot is on
  * @slot: pointer to the &struct hotplug_slot to register
  * @slot_nr: slot number
+ * @name: name registered with kobject core
  *
  * Registers a hotplug slot with the pci hotplug subsystem, which will allow
  * userspace interaction to the slot.
  *
  * Returns 0 if successful, anything else for an error.
  */
-int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
+int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
+			const char *name)
 {
 	int result;
 	struct pci_slot *pci_slot;

@@ -569,7 +571,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
 	}
 
 	/* Check if we have already registered a slot with the same name. */
-	if (get_slot_from_name(slot->name))
+	if (get_slot_from_name(name))
 		return -EEXIST;
 
 	/*

@@ -577,7 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
 	 * 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, slot->name);
+	pci_slot = pci_create_slot(bus, slot_nr, name);
 	if (IS_ERR(pci_slot))
 		return PTR_ERR(pci_slot);

@@ -593,8 +595,8 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
 	/*
 	 * Allow pcihp drivers to override the ACPI_PCI_SLOT name.
 	 */
-	if (strcmp(kobject_name(&pci_slot->kobj), slot->name)) {
-		result = kobject_rename(&pci_slot->kobj, 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;

@@ -607,8 +609,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr)
 
 	result = fs_add_slot(pci_slot);
 	kobject_uevent(&pci_slot->kobj, KOBJ_ADD);
-	dbg("Added slot %s to the list\n", slot->name);
-
+	dbg("Added slot %s to the list\n", name);
 
 	return result;
 }

drivers/pci/hotplug/pciehp_core.c +2 -1

index c748a19..3ace5e0 100644
@@ -226,7 +226,8 @@ static int init_slots(struct controller *ctrl)
 duplicate_name:
 		retval = pci_hp_register(hotplug_slot,
 					 ctrl->pci_dev->subordinate,
-					 slot->device);
+					 slot->device,
+					 slot->name);
 		if (retval) {
 			/*
 			 * If slot N already exists, we'll try to create

drivers/pci/hotplug/rpaphp_slot.c +1 -1

index 5088450..736d3b4 100644
@@ -137,7 +137,7 @@ int rpaphp_register_slot(struct slot *slot)
 		slotno = PCI_SLOT(PCI_DN(slot->dn->child)->devfn);
 	else
 		slotno = -1;
-	retval = pci_hp_register(php_slot, slot->bus, slotno);
+	retval = pci_hp_register(php_slot, slot->bus, slotno, slot->name);
 	if (retval) {
 		err("pci_hp_register failed with error %d\n", retval);
 		return retval;

drivers/pci/hotplug/sgi_hotplug.c +2 -1

index 410fe03..6d20bbd 100644
@@ -653,7 +653,8 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
 		bss_hotplug_slot->ops = &sn_hotplug_slot_ops;
 		bss_hotplug_slot->release = &sn_release_slot;
 
-		rc = pci_hp_register(bss_hotplug_slot, pci_bus, device);
+		rc = pci_hp_register(bss_hotplug_slot, pci_bus, device,
+				     bss_hotplug_slot->name);
 		if (rc)
 			goto register_err;

drivers/pci/hotplug/shpchp_core.c +2 -1

index cc38615..bf50966 100644
@@ -146,7 +146,8 @@ static int init_slots(struct controller *ctrl)
 		    slot->hp_slot, slot->number, ctrl->slot_device_offset);
 duplicate_name:
 		retval = pci_hp_register(slot->hotplug_slot,
-				ctrl->pci_dev->subordinate, slot->device);
+				ctrl->pci_dev->subordinate, slot->device,
+				hotplug_slot->name);
 		if (retval) {
 			/*
 			 * If slot N already exists, we'll try to create

include/linux/pci_hotplug.h +2 -1

index a08cd06..5efba66 100644
@@ -165,7 +165,8 @@ struct hotplug_slot {
 };
 #define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj)
 
-extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr);
+extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr,
+			   const char *name);
 extern int pci_hp_deregister(struct hotplug_slot *slot);
 extern int __must_check pci_hp_change_slot_info	(struct hotplug_slot *slot,
 						 struct hotplug_slot_info *info);

Alex Chiang

author

2008-10-09 04:46 UTC #2 lore raw

PATCH v5 2/16 PCI: rename pci_update_slot_number to pci_renumber_slot

In reply to Alex Chiang

The GPL exported symbol pci_update_slot_number has been renamed to pci_renumber_slot. Some of the safety checks were unnecessary and were removed.

Cc [email protected]

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

---

 drivers/pci/slot.c  |   15 +++++----------
 include/linux/pci.h |    2 +-
 2 files changed, 6 insertions(+), 11 deletions(-)

drivers/pci/slot.c +5 ~~-10~~

index 0c6db03..b9b90ab 100644
@@ -175,7 +175,7 @@ placeholder:
 EXPORT_SYMBOL_GPL(pci_create_slot);
 
 /**
- * pci_update_slot_number - update %struct pci_slot -> number
+ * pci_renumber_slot - update %struct pci_slot -> number
  * @slot - %struct pci_slot to update
  * @slot_nr - new number for slot
  *

@@ -183,27 +183,22 @@ EXPORT_SYMBOL_GPL(pci_create_slot);
  * 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)
+void pci_renumber_slot(struct pci_slot *slot, int slot_nr)
 {
-	int name_count = 0;
 	struct pci_slot *tmp;
 
 	down_write(&pci_bus_sem);
 
 	list_for_each_entry(tmp, &slot->bus->slots, list) {
 		WARN_ON(tmp->number == slot_nr);
-		if (!strcmp(kobject_name(&tmp->kobj), kobject_name(&slot->kobj)))
-			name_count++;
+		goto out;
 	}
 
-	if (name_count > 1)
-		printk(KERN_WARNING "pci_update_slot_number found %d slots with the same name: %s\n", name_count, kobject_name(&slot->kobj));
-
 	slot->number = slot_nr;
+out:
 	up_write(&pci_bus_sem);
 }
-EXPORT_SYMBOL_GPL(pci_update_slot_number);
+EXPORT_SYMBOL_GPL(pci_renumber_slot);
 
 /**
  * pci_destroy_slot - decrement refcount for physical PCI slot

include/linux/pci.h +1 -1

index 79dc7ce..5d96b7a 100644
@@ -510,7 +510,7 @@ struct pci_bus *pci_add_new_bus(struct pci_bus *parent, struct pci_dev *dev,
 struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
 				 const char *name);
 void pci_destroy_slot(struct pci_slot *slot);
-void pci_update_slot_number(struct pci_slot *slot, int slot_nr);
+void pci_renumber_slot(struct pci_slot *slot, int slot_nr);
 int pci_scan_slot(struct pci_bus *bus, int devfn);
 struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn);
 void pci_device_add(struct pci_dev *dev, struct pci_bus *bus);

Alex Chiang

author

2008-10-09 04:47 UTC #3 lore raw

PATCH v5 3/16 PCI: update pci_create_slot() to take a 'rename' param

In reply to Alex Chiang

Slot detection drivers can co-exist with hotplug drivers. The names of the detected/claimed slots may be different depending on module load order.

For legacy reasons, we need to allow hotplug drivers to override the slot name if a detection driver is loaded first (and they find the same slots).

Creating and overriding slot names should be an atomic operation, otherwise you get a locking nightmare as various drivers race to call pci_create_slot().

pci_create_slot() is already serialized by grabbing the pci_bus_sem.

We update the API and add a 'rename' param, which is:

set if the caller wants to rename an already existing slot unset if the caller doesn't care

The hotplug drivers care about renaming; the detection drivers do not.

pci_create_slot() does not actually use the 'rename' parameter in this patch. A later patch will add the logic that uses it.

Cc [email protected]

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

---

 drivers/acpi/pci_slot.c                |    2 +-
 drivers/pci/hotplug/pci_hotplug_core.c |    2 +-
 drivers/pci/slot.c                     |    3 ++-
 include/linux/pci.h                    |    2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

drivers/acpi/pci_slot.c +1 -1

index d5b4ef8..0946890 100644
@@ -150,7 +150,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 
 	snprintf(name, sizeof(name), "%u", (u32)sun);
-	pci_slot = pci_create_slot(pci_bus, device, name);
+	pci_slot = pci_create_slot(pci_bus, device, name, 0);
 	if (IS_ERR(pci_slot)) {
 		err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
 		kfree(slot);

drivers/pci/hotplug/pci_hotplug_core.c +1 -1

index 0e7a511..a5e01bc 100644
@@ -579,7 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
 	 * 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);
+	pci_slot = pci_create_slot(bus, slot_nr, name, 1);
 	if (IS_ERR(pci_slot))
 		return PTR_ERR(pci_slot);

drivers/pci/slot.c +2 -1

index b9b90ab..8379c50 100644
@@ -83,6 +83,7 @@ static struct kobj_type pci_slot_ktype = {
  * @parent: struct pci_bus of parent bridge
  * @slot_nr: PCI_SLOT(pci_dev->devfn) or -1 for placeholder
  * @name: user visible string presented in /sys/bus/pci/slots/<name>
+ * @rename: 1 if driver wants to rename an existing slot, 0 otherwise
  *
  * PCI slots have first class attributes such as address, speed, width,
  * and a &struct pci_slot is used to manage them. This interface will

@@ -111,7 +112,7 @@ static struct kobj_type pci_slot_ktype = {
  */
 
 struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
-				 const char *name)
+				 const char *name, int rename)
 {
 	struct pci_dev *dev;
 	struct pci_slot *slot;

include/linux/pci.h +1 -1

index 5d96b7a..aa49fb9 100644
@@ -508,7 +508,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);
+				 const char *name, int rename);
 void pci_destroy_slot(struct pci_slot *slot);
 void pci_renumber_slot(struct pci_slot *slot, int slot_nr);
 int pci_scan_slot(struct pci_bus *bus, int devfn);

Alex Chiang

author

2008-10-09 04:47 UTC #4 lore raw

PATCH v5 4/16 PCI: prevent duplicate slot names

In reply to Alex Chiang

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

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. The -EBUSY was getting masked previously by -EEXIST in the event of duplicate slot names; this is no longer so.

This is the permanent fix mentioned in earlier commits d6a9e9b4 and 167e782e (shpchp/pciehp: Rename duplicate slot name...).

Finally, we take advantage of the new 'rename' parameter in pci_create_slot() to prevent a slot create/rename race between hotplug drivers and detection drivers.

	Scenario A:
	hotplug driver                  detection_driver
	--------------                  ----------------
	pci_create_slot(rename=1)
					pci_create_slot(rename=0)

The hotplug driver creates the slot with its desired name, and then releases the semaphore. Now, the detection driver tries to create the same slot, but it already exists. We don't care about renaming, so return the existing slot.

	Scenario B:
	hotplug driver                  detection_driver
	--------------                  ----------------
					pci_create_slot(rename=0)
	pci_create_slot(rename=1)

The detection driver creates the slot with name "X". Then the hotplug driver tries to create the same slot, but wants the name "Y" instead. We detect that we're trying to create the same slot and that we also want a rename, so rename the slot to "Y" and return.

Cc [email protected]

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

---

 drivers/pci/hotplug/pci_hotplug_core.c |   15 ----
 drivers/pci/hotplug/pciehp_core.c      |   15 ----
 drivers/pci/hotplug/shpchp_core.c      |   15 ----
 drivers/pci/slot.c                     |  132 ++++++++++++++++++++++++++------
 4 files changed, 108 insertions(+), 69 deletions(-)

drivers/pci/hotplug/pci_hotplug_core.c +0 ~~-15~~

index a5e01bc..ed87585 100644
@@ -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

@@ -592,17 +588,6 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
 	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;
-		}
-	}
-
 	spin_lock(&pci_hotplug_slot_list_lock);
 	list_add(&slot->slot_list, &pci_hotplug_slot_list);
 	spin_unlock(&pci_hotplug_slot_list_lock);

drivers/pci/hotplug/pciehp_core.c +0 ~~-15~~

index 3ace5e0..af89d7b 100644
@@ -196,7 +196,6 @@ static int init_slots(struct controller *ctrl)
 	struct slot *slot;
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
-	int len, dup = 1;
 	int retval = -ENOMEM;
 
 	list_for_each_entry(slot, &ctrl->slot_list, slot_list) {

@@ -223,25 +222,11 @@ static int init_slots(struct controller *ctrl)
 		ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x "
 			 "slot_device_offset=%x\n", slot->bus, slot->device,
 			 slot->hp_slot, slot->number, ctrl->slot_device_offset);
-duplicate_name:
 		retval = pci_hp_register(hotplug_slot,
 					 ctrl->pci_dev->subordinate,
 					 slot->device,
 					 slot->name);
 		if (retval) {
-			/*
-			 * If slot N already exists, we'll try to create
-			 * slot N-1, N-2 ... N-M, until we overflow.
-			 */
-			if (retval == -EEXIST) {
-				len = snprintf(slot->name, SLOT_NAME_SIZE,
-					       "%d-%d", slot->number, dup++);
-				if (len < SLOT_NAME_SIZE)
-					goto duplicate_name;
-				else
-					ctrl_err(ctrl, "duplicate slot name "
-						 "overflow\n");
-			}
 			ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
 				 retval);
 			goto error_info;

drivers/pci/hotplug/shpchp_core.c +1 ~~-14~~

index bf50966..cfdd079 100644
@@ -102,7 +102,7 @@ static int init_slots(struct controller *ctrl)
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
 	int retval = -ENOMEM;
-	int i, len, dup = 1;
+	int i;
 
 	for (i = 0; i < ctrl->num_slots; i++) {
 		slot = kzalloc(sizeof(*slot), GFP_KERNEL);

@@ -144,23 +144,10 @@ static int init_slots(struct controller *ctrl)
 		dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
 		    "slot_device_offset=%x\n", slot->bus, slot->device,
 		    slot->hp_slot, slot->number, ctrl->slot_device_offset);
-duplicate_name:
 		retval = pci_hp_register(slot->hotplug_slot,
 				ctrl->pci_dev->subordinate, slot->device,
 				hotplug_slot->name);
 		if (retval) {
-			/*
-			 * If slot N already exists, we'll try to create
-			 * slot N-1, N-2 ... N-M, until we overflow.
-			 */
-			if (retval == -EEXIST) {
-				len = snprintf(slot->name, SLOT_NAME_SIZE,
-					       "%d-%d", slot->number, dup++);
-				if (len < SLOT_NAME_SIZE)
-					goto duplicate_name;
-				else
-					err("duplicate slot name overflow\n");
-			}
 			err("pci_hp_register failed with error %d\n", retval);
 			goto error_info;
 		}

drivers/pci/slot.c +107 ~~-25~~

189 lines not on this page · raw message

Kenji Kaneshige

2008-10-09 05:32 UTC #5 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

Quoted 43 lines · Alex Chiang

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 etc.
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. The -EBUSY was getting masked previously by -EEXIST in the event of duplicate slot names; this is no longer so.
This is the permanent fix mentioned in earlier commits d6a9e9b4 and 167e782e (shpchp/pciehp: Rename duplicate slot name...).
Finally, we take advantage of the new 'rename' parameter in pci_create_slot() to prevent a slot create/rename race between hotplug drivers and detection drivers.
	Scenario A:
	hotplug driver                  detection_driver
	--------------                  ----------------
	pci_create_slot(rename=1)
					pci_create_slot(rename=0)
The hotplug driver creates the slot with its desired name, and then releases the semaphore. Now, the detection driver tries to create the same slot, but it already exists. We don't care about renaming, so return the existing slot.
	Scenario B:
	hotplug driver                  detection_driver
	--------------                  ----------------
					pci_create_slot(rename=0)
	pci_create_slot(rename=1)

Quoted 5 lines

The detection driver creates the slot with name "X". Then the hotplug driver tries to create the same slot, but wants the name "Y" instead. We detect that we're trying to create the same slot and that we also want a rename, so rename the slot to "Y" and return.

Thank your new patches. Very quick!!!

Though I have not reviewed/tested your patches yet (of course), I have one concern as I said in the e-mail soon before. Does the new one consider the following senario?

	Scenario C:
	hotplug driver(A)               	hotplug_driver(B)
	--------------                  	----------------
       pci_create_slot(name=A, rename=1)
						pci_create_slot(name=B, rename=1)

The hotplug driver (A) creates the slot with name "A". The the hotplug driver (B) tries to create the same slot, but wants the name "B" instead. In this case, hotplug driver fails to create the slot and the slot name should not be changed to "B" from "A".

Thanks,
Kenji Kaneshige

Alex Chiang

author

2008-10-09 05:57 UTC #6 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

* Kenji Kaneshige <[email protected]>:

Quoted 2 lines

Thank your new patches. Very quick!!!

I'm trying to get into 2.6.28. ;)

Quoted 10 lines

Though I have not reviewed/tested your patches yet (of course), I have one concern as I said in the e-mail soon before. Does the new one consider the following senario?
	Scenario C:
	hotplug driver(A)               	hotplug_driver(B)
	--------------                  	----------------
       pci_create_slot(name=A, rename=1)
						pci_create_slot(name=B, rename=1)

Quoted 4 lines

The hotplug driver (A) creates the slot with name "A". The the hotplug driver (B) tries to create the same slot, but wants the name "B" instead. In this case, hotplug driver fails to create the slot and the slot name should not be changed to "B" from "A".

Hm... I don't think this is a common scenario but...

int pci_hp_register(...)

{
	...

        pci_slot = pci_create_slot(bus, slot_nr, name, 1);
        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;
        }
	...
}

I could maybe move that check into pci_create_slot() instead.

struct pci_slot *pci_create_slot(...)

{
	...

        /*
         * Get existing slot and rename if desired
         */
        slot = get_slot(parent, slot_nr);
        if (slot && rename) {
		if ((err = slot->hotplug ? -EBUSY : 0)
		     || (err = rename_slot(slot, name))) {
                        kobject_put(&slot->kobj);
                        slot = NULL;
                        goto err;
                } else
                        goto out;
        } else if (slot)
                goto out;
	...
}

Seems a little ugly to me, but maybe it's necessary?

/ac

Kenji Kaneshige

2008-10-09 12:34 UTC #7 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

Quoted 15 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:
Quoted 1 line
Thank your new patches. Very quick!!!
I'm trying to get into 2.6.28. ;)
Quoted 10 lines
Though I have not reviewed/tested your patches yet (of course), I have one concern as I said in the e-mail soon before. Does the new one consider the following senario?
	Scenario C:
	hotplug driver(A)               	hotplug_driver(B)
	--------------                  	----------------
       pci_create_slot(name=A, rename=1)
						pci_create_slot(name=B, rename=1)

Quoted 7 lines

Quoted 4 lines
The hotplug driver (A) creates the slot with name "A". The the hotplug driver (B) tries to create the same slot, but wants the name "B" instead. In this case, hotplug driver fails to create the slot and the slot name should not be changed to "B" from "A".
Hm... I don't think this is a common scenario but...

It was a common scenario until recently because acpiphp and native hotplug drivers(pciehp, shpchp) had different naming rules. That is, acpiphp used _SUN number, while pciehp/shpchp used bus number and physical slot number pair. Although the pciehp/shpchp driver has been changed not to use bus_number for slot names and acpiphp and pciehp/shpchp has the same names on my system now, but I think the scenario is still possible because naming rule of each hotplug driver could be changed in the future again.

By the way, acpiphp was changed to handle 64bit _SUN number recently for new ia64 HP servers, IIRC. Are hotplug slots on that server can also be handled through PCIe controller? If it is yes, I think _SUN doesn't match physical slot number because physical slot number (in Slot Capabilities Register) has only 13bit. In this case, the above scenario will happen.

Quoted 39 lines

int pci_hp_register(...)

{
	...

        pci_slot = pci_create_slot(bus, slot_nr, name, 1);
        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;
        }
	...
}

I could maybe move that check into pci_create_slot() instead.

struct pci_slot *pci_create_slot(...)

{
	...

        /*
         * Get existing slot and rename if desired
         */
        slot = get_slot(parent, slot_nr);
        if (slot && rename) {
		if ((err = slot->hotplug ? -EBUSY : 0)
		     || (err = rename_slot(slot, name))) {
                        kobject_put(&slot->kobj);
                        slot = NULL;
                        goto err;
                } else
                        goto out;
        } else if (slot)
                goto out;
	...
}

Quoted 2 lines

Seems a little ugly to me, but maybe it's necessary?

I don't like this, and I think it's wrong because callers might get -EBUSY even though they are not related to hotplug.

I thought of the following alternative ideas, when I was making sample patches. What do you think about those? My was concerned that both need to add hotplug related code into generic pci slot management code/API.

- Add 'hotplug' arg to pci_create_slot(), instead of 'rename' flag. The pci_create_slot() would be as follows:

	struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
	{
		...
		/*
		 * Get existing slot and rename if desired
		 */
		slot = get_slot(parent, slot_nr);
		if (slot) {
			if (hotplug) {
				if ((err = slot->hotplug ? -EBUSY : 0)
				     || err = rename_slot(slot, name))) {
					Some cleanups;
					return err;
				}
			}
			goto out;
		}
		...
	out:
		if (hotplug)
			slot->hotplug = hotplug;
		...
	}

- Introduce new API to setup pci_slot->hotplug and rename. This would be as follows:

	int pci_slot_hp_register(struct pci_slot *pci_slot,
				 struct hotplug_slot *hotplug_slot, const char *name)
	{
		...

		if (pci_slot->hotplug) {
			Some cleanups;
			return -EBUSY;
		}
		if ((err = rename_slot(slot, name))
			Some cleanups;
			return err;
		}
		pci_slot->hotplug = hotplug;
		...
	}

It is intended to be used by pci_hp_register() as follows:

	int pci_hp_register(...)
	{
		...
	        pci_slot = pci_create_slot(bus, slot_nr, name);
       	if ((result = IS_ERR(pci_slot)))
               	goto out;
		if ((err = pci_slot_hp_register(pci_slot, hotplug, name)))
			goto out;
		...
	}

Thanks,
Kenji Kaneshige

Alex Chiang

author

2008-10-10 02:10 UTC #8 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

* Kenji Kaneshige <[email protected]>:

Quoted 34 lines

Quoted 15 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 1 line
Thank your new patches. Very quick!!!
I'm trying to get into 2.6.28. ;)
Quoted 10 lines
Though I have not reviewed/tested your patches yet (of course), I have one concern as I said in the e-mail soon before. Does the new one consider the following senario?
	Scenario C:
	hotplug driver(A)               	hotplug_driver(B)
	--------------                  	----------------
       pci_create_slot(name=A, rename=1)
						pci_create_slot(name=B, rename=1)
Quoted 7 lines
Quoted 4 lines
The hotplug driver (A) creates the slot with name "A". The the hotplug driver (B) tries to create the same slot, but wants the name "B" instead. In this case, hotplug driver fails to create the slot and the slot name should not be changed to "B" from "A".
Hm... I don't think this is a common scenario but...
It was a common scenario until recently because acpiphp and native hotplug drivers(pciehp, shpchp) had different naming rules. That is, acpiphp used _SUN number, while pciehp/shpchp used bus number and physical slot number pair. Although the pciehp/shpchp driver has been changed not to use bus_number for slot names and acpiphp and pciehp/shpchp has the same names on my system now, but I think the scenario is still possible because naming rule of each hotplug driver could be changed in the future again.

Quoted 6 lines

By the way, acpiphp was changed to handle 64bit _SUN number recently for new ia64 HP servers, IIRC. Are hotplug slots on that server can also be handled through PCIe controller? If it is yes, I think _SUN doesn't match physical slot number because physical slot number (in Slot Capabilities Register) has only 13bit. In this case, the above scenario will happen.

Hm, ok, I agree.

Quoted 68 lines

Quoted 39 lines
int pci_hp_register(...)
{
	...

        pci_slot = pci_create_slot(bus, slot_nr, name, 1);
        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;
        }
	...
}
I could maybe move that check into pci_create_slot() instead.
struct pci_slot *pci_create_slot(...)
{
	...

        /*
         * Get existing slot and rename if desired
         */
        slot = get_slot(parent, slot_nr);
        if (slot && rename) {
		if ((err = slot->hotplug ? -EBUSY : 0)
		     || (err = rename_slot(slot, name))) {
                        kobject_put(&slot->kobj);
                        slot = NULL;
                        goto err;
                } else
                        goto out;
        } else if (slot)
                goto out;
	...
}
Quoted 2 lines
Seems a little ugly to me, but maybe it's necessary?
I don't like this, and I think it's wrong because callers might get -EBUSY even though they are not related to hotplug.
I thought of the following alternative ideas, when I was making sample patches. What do you think about those? My was concerned that both need to add hotplug related code into generic pci slot management code/API.
- Add 'hotplug' arg to pci_create_slot(), instead of 'rename' flag. The pci_create_slot() would be as follows:
	struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
	{
		...
		/*
		 * Get existing slot and rename if desired
		 */
		slot = get_slot(parent, slot_nr);
		if (slot) {
			if (hotplug) {
				if ((err = slot->hotplug ? -EBUSY : 0)
				     || err = rename_slot(slot, name))) {
					Some cleanups;
					return err;
				}
			}

Quoted 8 lines

			goto out;
		}
		...
	out:
		if (hotplug)
			slot->hotplug = hotplug;
		...
	}

I like this approach a little better, since the flow of execution is easier to understand (vs. pci_create_slot + pci_slot_hp_register).

I prototyped it, but didn't get a chance to test it (I did compile it though).

I'll send 2 test patches shortly that should replace the earlier 03/16 and 04/16 patches.

Thanks.

/ac

Alex Chiang

author

2008-10-10 02:11 UTC #9 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

commit 2338f56b789bb030434be828c4788679d72901b5

Author: Alex Chiang <[email protected]>
Date:   Thu Oct 9 19:56:58 2008 -0600

PCI: update pci_create_slot() to take a 'hotplug' param

Slot detection drivers can co-exist with hotplug drivers. The names of the detected/claimed slots may be different depending on module load order.

For legacy reasons, we need to allow hotplug drivers to override the slot name if a detection driver is loaded first (and they find the same slots).

Creating and overriding slot names should be an atomic operation, otherwise you get a locking nightmare as various drivers race to call pci_create_slot().

pci_create_slot() is already serialized by grabbing the pci_bus_sem.

We update the API and add a 'hotplug' param, which is:

set if the caller is a hotplug driver
unset if the caller is a detection driver

pci_create_slot() does not actually use the 'hotplug' parameter in this patch. A later patch will add the logic that uses it.

Cc [email protected]

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

drivers/acpi/pci_slot.c +1 -1

index d5b4ef8..8d4a568 100644
@@ -150,7 +150,7 @@ register_slot(acpi_handle handle, u32 lvl, void *context, void **rv)
 	}
 
 	snprintf(name, sizeof(name), "%u", (u32)sun);
-	pci_slot = pci_create_slot(pci_bus, device, name);
+	pci_slot = pci_create_slot(pci_bus, device, name, NULL);
 	if (IS_ERR(pci_slot)) {
 		err("pci_create_slot returned %ld\n", PTR_ERR(pci_slot));
 		kfree(slot);

drivers/pci/hotplug/pci_hotplug_core.c +1 -1

index 0e7a511..191b58e 100644
@@ -579,7 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
 	 * 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);
+	pci_slot = pci_create_slot(bus, slot_nr, name, slot);
 	if (IS_ERR(pci_slot))
 		return PTR_ERR(pci_slot);

drivers/pci/slot.c +3 -1

index b9b90ab..0e009c3 100644
@@ -83,6 +83,7 @@ static struct kobj_type pci_slot_ktype = {
  * @parent: struct pci_bus of parent bridge
  * @slot_nr: PCI_SLOT(pci_dev->devfn) or -1 for placeholder
  * @name: user visible string presented in /sys/bus/pci/slots/<name>
+ * @hotplug: set if caller is hotplug driver, NULL otherwise
  *
  * PCI slots have first class attributes such as address, speed, width,
  * and a &struct pci_slot is used to manage them. This interface will

@@ -111,7 +112,8 @@ static struct kobj_type pci_slot_ktype = {
  */
 
 struct pci_slot *pci_create_slot(struct pci_bus *parent, int slot_nr,
-				 const char *name)
+				 const char *name,
+				 struct hotplug_slot *hotplug)
 {
 	struct pci_dev *dev;
 	struct pci_slot *slot;

include/linux/pci.h +2 -1

index 5d96b7a..8bbbbbc 100644
@@ -508,7 +508,8 @@ 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);
+				 const char *name,
+				 struct hotplug_slot *hotplug);
 void pci_destroy_slot(struct pci_slot *slot);
 void pci_renumber_slot(struct pci_slot *slot, int slot_nr);
 int pci_scan_slot(struct pci_bus *bus, int devfn);

Alex Chiang

author

2008-10-10 02:13 UTC #10 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

In reply to Alex Chiang

Note, the comments might be a bit stale...

Also note the change in rename_slot() so we don't try and rename something with the same name.

/ac

commit eb00679613b5e5b7d7b4ebd82d4a229fff1ecefa

Author: Alex Chiang <[email protected]>
Date:   Thu Oct 9 20:06:49 2008 -0600

PCI: prevent duplicate slot names

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

This is the permanent fix mentioned in earlier commits d6a9e9b4 and 167e782e (shpchp/pciehp: Rename duplicate slot name...).

Finally, we take advantage of the new 'rename' parameter in pci_create_slot() to prevent a slot create/rename race between hotplug drivers and detection drivers.

Scenario A:
hotplug driver detection_driver
-------------- ----------------
pci_create_slot(rename=1)
pci_create_slot(rename=0)

Scenario B:
hotplug driver detection_driver
-------------- ----------------
pci_create_slot(rename=0)
pci_create_slot(rename=1)

Cc [email protected]

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

drivers/pci/hotplug/pci_hotplug_core.c +0 ~~-22~~

index 191b58e..b4e3a43 100644
@@ -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

@@ -583,25 +579,7 @@ int pci_hp_register(struct hotplug_slot *slot, struct pci_bus *bus, int slot_nr,
 	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;
-		}
-	}
 
 	spin_lock(&pci_hotplug_slot_list_lock);
 	list_add(&slot->slot_list, &pci_hotplug_slot_list);

drivers/pci/hotplug/pciehp_core.c +0 ~~-15~~

index 3ace5e0..af89d7b 100644
@@ -196,7 +196,6 @@ static int init_slots(struct controller *ctrl)
 	struct slot *slot;
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
-	int len, dup = 1;
 	int retval = -ENOMEM;
 
 	list_for_each_entry(slot, &ctrl->slot_list, slot_list) {

@@ -223,25 +222,11 @@ static int init_slots(struct controller *ctrl)
 		ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x "
 			 "slot_device_offset=%x\n", slot->bus, slot->device,
 			 slot->hp_slot, slot->number, ctrl->slot_device_offset);
-duplicate_name:
 		retval = pci_hp_register(hotplug_slot,
 					 ctrl->pci_dev->subordinate,
 					 slot->device,
 					 slot->name);
 		if (retval) {
-			/*
-			 * If slot N already exists, we'll try to create
-			 * slot N-1, N-2 ... N-M, until we overflow.
-			 */
-			if (retval == -EEXIST) {
-				len = snprintf(slot->name, SLOT_NAME_SIZE,
-					       "%d-%d", slot->number, dup++);
-				if (len < SLOT_NAME_SIZE)
-					goto duplicate_name;
-				else
-					ctrl_err(ctrl, "duplicate slot name "
-						 "overflow\n");
-			}
 			ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
 				 retval);
 			goto error_info;

drivers/pci/hotplug/shpchp_core.c +1 ~~-14~~

index bf50966..cfdd079 100644
@@ -102,7 +102,7 @@ static int init_slots(struct controller *ctrl)
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
 	int retval = -ENOMEM;
-	int i, len, dup = 1;
+	int i;
 
 	for (i = 0; i < ctrl->num_slots; i++) {
 		slot = kzalloc(sizeof(*slot), GFP_KERNEL);

@@ -144,23 +144,10 @@ static int init_slots(struct controller *ctrl)
 		dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
 		    "slot_device_offset=%x\n", slot->bus, slot->device,
 		    slot->hp_slot, slot->number, ctrl->slot_device_offset);
-duplicate_name:
 		retval = pci_hp_register(slot->hotplug_slot,
 				ctrl->pci_dev->subordinate, slot->device,
 				hotplug_slot->name);
 		if (retval) {
-			/*
-			 * If slot N already exists, we'll try to create
-			 * slot N-1, N-2 ... N-M, until we overflow.
-			 */
-			if (retval == -EEXIST) {
-				len = snprintf(slot->name, SLOT_NAME_SIZE,
-					       "%d-%d", slot->number, dup++);
-				if (len < SLOT_NAME_SIZE)
-					goto duplicate_name;
-				else
-					err("duplicate slot name overflow\n");
-			}
 			err("pci_hp_register failed with error %d\n", retval);
 			goto error_info;
 		}

drivers/pci/slot.c +111 ~~-24~~

194 lines not on this page · raw message

Kenji Kaneshige

2008-10-10 04:45 UTC #11 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

In reply to Alex Chiang

Quoted 117 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:
Quoted 31 lines
Quoted 14 lines · Alex Chiang
* Kenji Kaneshige <[email protected]>:
Quoted 1 line
Thank your new patches. Very quick!!!
I'm trying to get into 2.6.28. ;)
Quoted 10 lines
Though I have not reviewed/tested your patches yet (of course), I have one concern as I said in the e-mail soon before. Does the new one consider the following senario?
	Scenario C:
	hotplug driver(A)               	hotplug_driver(B)
	--------------                  	----------------
       pci_create_slot(name=A, rename=1)
						pci_create_slot(name=B, rename=1)
Quoted 6 lines
Quoted 4 lines
The hotplug driver (A) creates the slot with name "A". The the hotplug driver (B) tries to create the same slot, but wants the name "B" instead. In this case, hotplug driver fails to create the slot and the slot name should not be changed to "B" from "A".
Hm... I don't think this is a common scenario but...
It was a common scenario until recently because acpiphp and native hotplug drivers(pciehp, shpchp) had different naming rules. That is, acpiphp used _SUN number, while pciehp/shpchp used bus number and physical slot number pair. Although the pciehp/shpchp driver has been changed not to use bus_number for slot names and acpiphp and pciehp/shpchp has the same names on my system now, but I think the scenario is still possible because naming rule of each hotplug driver could be changed in the future again.
Quoted 6 lines
By the way, acpiphp was changed to handle 64bit _SUN number recently for new ia64 HP servers, IIRC. Are hotplug slots on that server can also be handled through PCIe controller? If it is yes, I think _SUN doesn't match physical slot number because physical slot number (in Slot Capabilities Register) has only 13bit. In this case, the above scenario will happen.
Hm, ok, I agree.
Quoted 67 lines
Quoted 39 lines
int pci_hp_register(...)
{
	...

        pci_slot = pci_create_slot(bus, slot_nr, name, 1);
        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;
        }
	...
}
I could maybe move that check into pci_create_slot() instead.
struct pci_slot *pci_create_slot(...)
{
	...

        /*
         * Get existing slot and rename if desired
         */
        slot = get_slot(parent, slot_nr);
        if (slot && rename) {
		if ((err = slot->hotplug ? -EBUSY : 0)
		     || (err = rename_slot(slot, name))) {
                        kobject_put(&slot->kobj);
                        slot = NULL;
                        goto err;
                } else
                        goto out;
        } else if (slot)
                goto out;
	...
}
Quoted 2 lines
Seems a little ugly to me, but maybe it's necessary?
I don't like this, and I think it's wrong because callers might get -EBUSY even though they are not related to hotplug.
I thought of the following alternative ideas, when I was making sample patches. What do you think about those? My was concerned that both need to add hotplug related code into generic pci slot management code/API.
- Add 'hotplug' arg to pci_create_slot(), instead of 'rename' flag. The pci_create_slot() would be as follows:
	struct pci_slot *pci_create_slot(..., struct hotplug_slot *hotplug)
	{
		...
		/*
		 * Get existing slot and rename if desired
		 */
		slot = get_slot(parent, slot_nr);
		if (slot) {
			if (hotplug) {
				if ((err = slot->hotplug ? -EBUSY : 0)
				     || err = rename_slot(slot, name))) {
					Some cleanups;
					return err;
				}
			}
Quoted 8 lines
			goto out;
		}
		...
	out:
		if (hotplug)
			slot->hotplug = hotplug;
		...
	}

Quoted 9 lines

I like this approach a little better, since the flow of execution is easier to understand (vs. pci_create_slot + pci_slot_hp_register).
I prototyped it, but didn't get a chance to test it (I did compile it though).
I'll send 2 test patches shortly that should replace the earlier 03/16 and 04/16 patches.

I'm sorry, but I forgot to tell you one important thing. Now we are trying to change pci slot management API to setup pci_slot->hotplug. We must consider how to implement the counterpart to clean up pci_slot->hotplug at the same time. My current idea is adding hotplug arg to pci_destroy_slot(), but it seems a little ugly...

Thanks,
Kenji Kaneshige

Alex Chiang

author

2008-10-10 05:28 UTC #12 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

* Kenji Kaneshige <[email protected]>:

Quoted 7 lines

I'm sorry, but I forgot to tell you one important thing. Now we are trying to change pci slot management API to setup pci_slot->hotplug. We must consider how to implement the counterpart to clean up pci_slot->hotplug at the same time. My current idea is adding hotplug arg to pci_destroy_slot(), but it seems a little ugly...

Ugh, I'm not sure which is worse, an unbalanced API vs passing a hotplug_slot to pci_destroy_slot.

pci_destroy_slot should never touch the pci_slot->hotplug argument, I think, because it is possible for non-hotplug callers to call pci_create_slot.

I think the rule should just be:

- all hotplug drivers must use pci_hp_register/deregister
- hotplug drivers must pass a valid hotplug_slot argument

- all detection drivers must use pci_create/destroy_slot
- detection drivers must pass NULL for hotplug_slot

If you prefer, we could wrap pci_create_slot for detection drivers:

struct pci_slot *pci_slot_register(struct pci_bus *parent, int slot_nr,
const char *name)

{
	return pci_create_slot(parent, slot_nr, name, NULL);
}

EXPORT_SYMBOL_GPL(pci_slot_register);

And then do not export pci_create_slot().

Hm?

/ac

Kenji Kaneshige

2008-10-10 08:12 UTC #13 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

Quoted 15 lines · Alex Chiang

* Kenji Kaneshige <[email protected]>:
Quoted 6 lines
I'm sorry, but I forgot to tell you one important thing. Now we are trying to change pci slot management API to setup pci_slot->hotplug. We must consider how to implement the counterpart to clean up pci_slot->hotplug at the same time. My current idea is adding hotplug arg to pci_destroy_slot(), but it seems a little ugly...
Ugh, I'm not sure which is worse, an unbalanced API vs passing a hotplug_slot to pci_destroy_slot.
pci_destroy_slot should never touch the pci_slot->hotplug argument, I think, because it is possible for non-hotplug callers to call pci_create_slot.

Quoted 8 lines

I think the rule should just be:
- all hotplug drivers must use pci_hp_register/deregister
- hotplug drivers must pass a valid hotplug_slot argument
- all detection drivers must use pci_create/destroy_slot
- detection drivers must pass NULL for hotplug_slot

I'm still concerned about one thing. Now that we added hotplug arg to pci_create_slot(), pci_bus_sem could be used for changing pci_slot->hotplug. But if there is no generic API to clean up pci_slot->hotplug, we can't use pci_bus_sem because pci hotplug core cannot hold pci_bus_sem. As a result, we need to add the following rule in addition to the above rules.

- hotplug core driver must hold some mutex/semaphore when calling pci_create_slot()
- hotplug core driver must hold the same mutex/semaphore when cleaning up pci_slot->hotplug.

That is, even though pci_create_slot provides the interface to setup pci_slot->hotplug, caller needs to pay attention to race condition about pci_slot->hotplug. I think it's a little strange.

But I might be overcareful...

Thanks,
Kenji Kaneshige

Quoted 19 lines

If you prefer, we could wrap pci_create_slot for detection drivers:
struct pci_slot *pci_slot_register(struct pci_bus *parent, int slot_nr,
const char *name)
{
	return pci_create_slot(parent, slot_nr, name, NULL);
}
EXPORT_SYMBOL_GPL(pci_slot_register);
And then do not export pci_create_slot().
Hm?
/ac

Alex Chiang

author

2008-10-10 21:29 UTC #14 lore raw

Re: [PATCH v5 04/16] PCI: prevent duplicate slot names

* Kenji Kaneshige <[email protected]>:

Quoted 24 lines

Quoted 7 lines · Alex Chiang
Ugh, I'm not sure which is worse, an unbalanced API vs passing a hotplug_slot to pci_destroy_slot.
pci_destroy_slot should never touch the pci_slot->hotplug argument, I think, because it is possible for non-hotplug callers to call pci_create_slot.
Quoted 8 lines
I think the rule should just be:
- all hotplug drivers must use pci_hp_register/deregister
- hotplug drivers must pass a valid hotplug_slot argument
- all detection drivers must use pci_create/destroy_slot
- detection drivers must pass NULL for hotplug_slot
I'm still concerned about one thing. Now that we added hotplug arg to pci_create_slot(), pci_bus_sem could be used for changing pci_slot->hotplug. But if there is no generic API to clean up pci_slot->hotplug, we can't use pci_bus_sem because pci hotplug core cannot hold pci_bus_sem. As a result, we need to add the following rule in addition to the above rules.

Quoted 8 lines

- hotplug core driver must hold some mutex/semaphore when calling pci_create_slot()
- hotplug core driver must hold the same mutex/semaphore when cleaning up pci_slot->hotplug.
That is, even though pci_create_slot provides the interface to setup pci_slot->hotplug, caller needs to pay attention to race condition about pci_slot->hotplug. I think it's a little strange.

I think we can use your earlier patch that replaced the hotplug list spinlock with a pci_hp_mutex, and that should implement the two new rules you mention, right?

Let me try and insert your patch into the rest of my series and see how it looks.

/ac

Alex Chiang

author

2008-10-09 04:48 UTC #15 lore raw

PATCH v5 5/16 PCI, PCI Hotplug: introduce slot_name helpers

In reply to Alex Chiang

In preparation for cleaning up the various hotplug drivers such that they don't have to manage their own 'name' parameters anymore, we provide the following convenience functions:

	pci_slot_name()
	hotplug_slot_name()

These helpers will be used by individual hotplug drivers.

Cc [email protected]

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

---

 include/linux/pci.h         |    5 +++++
 include/linux/pci_hotplug.h |    5 +++++
 2 files changed, 10 insertions(+), 0 deletions(-)

include/linux/pci.h +5 -0

index aa49fb9..4e07c3f 100644
@@ -64,6 +64,11 @@ struct pci_slot {
 	struct kobject kobj;
 };
 
+static inline const char *pci_slot_name(const struct pci_slot *slot)
+{
+	return kobject_name(&slot->kobj);
+}
+
 /* File state for mmap()s on /proc/bus/pci/X/Y */
 enum pci_mmap_state {
 	pci_mmap_io,

include/linux/pci_hotplug.h +5 -0

index 5efba66..a3a3245 100644
@@ -165,6 +165,11 @@ struct hotplug_slot {
 };
 #define to_hotplug_slot(n) container_of(n, struct hotplug_slot, kobj)
 
+static inline const char *hotplug_slot_name(const struct hotplug_slot *slot)
+{
+	return pci_slot_name(slot->pci_slot);
+}
+
 extern int pci_hp_register(struct hotplug_slot *, struct pci_bus *, int nr,
 			   const char *name);
 extern int pci_hp_deregister(struct hotplug_slot *slot);

Alex Chiang

author

2008-10-09 04:48 UTC #16 lore raw

PATCH v5 6/16 PCI: acpiphp: remove 'name' parameter

In reply to Alex Chiang

We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.

Remove 'name' from acpiphp's version of struct slot.

Cc [email protected]

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

---

 drivers/pci/hotplug/acpiphp.h      |    9 +++++----
 drivers/pci/hotplug/acpiphp_core.c |   31 ++++++++++++++++---------------
 2 files changed, 21 insertions(+), 19 deletions(-)

drivers/pci/hotplug/acpiphp.h +5 -4

index 5a58b07..f9e244d 100644
@@ -50,9 +50,6 @@
 #define info(format, arg...) printk(KERN_INFO "%s: " format, MY_NAME , ## arg)
 #define warn(format, arg...) printk(KERN_WARNING "%s: " format, MY_NAME , ## arg)
 
-/* name size which is used for entries in pcihpfs */
-#define SLOT_NAME_SIZE	20		/* {_SUN} */
-
 struct acpiphp_bridge;
 struct acpiphp_slot;

@@ -63,9 +60,13 @@ struct slot {
 	struct hotplug_slot	*hotplug_slot;
 	struct acpiphp_slot	*acpi_slot;
 	struct hotplug_slot_info info;
-	char name[SLOT_NAME_SIZE];
 };
 
+static inline const char *slot_name(struct slot *slot)
+{
+	return hotplug_slot_name(slot->hotplug_slot);
+}
+
 /*
  * struct acpiphp_bridge - PCI bridge information
  *

drivers/pci/hotplug/acpiphp_core.c +16 ~~-15~~

index e984176..95b536a 100644
@@ -44,6 +44,9 @@
 
 #define MY_NAME	"acpiphp"
 
+/* name size which is used for entries in pcihpfs */
+#define SLOT_NAME_SIZE  21              /* {_SUN} */
+
 static int debug;
 int acpiphp_debug;

@@ -84,7 +87,6 @@ static struct hotplug_slot_ops acpi_hotplug_slot_ops = {
 	.get_adapter_status	= get_adapter_status,
 };
 
-
 /**
  * acpiphp_register_attention - set attention LED callback
  * @info: must be completely filled with LED callbacks

@@ -136,7 +138,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	/* enable the specified slot */
 	return acpiphp_enable_slot(slot->acpi_slot);

@@ -154,7 +156,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)
 	struct slot *slot = hotplug_slot->private;
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	/* disable the specified slot */
 	retval = acpiphp_disable_slot(slot->acpi_slot);

@@ -177,7 +179,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)
  {
 	int retval = -ENODEV;
 
- 	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot_name(hotplug_slot));
  
 	if (attention_info && try_module_get(attention_info->owner)) {
 		retval = attention_info->set_attn(hotplug_slot, status);

@@ -200,7 +202,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	*value = acpiphp_get_power_status(slot->acpi_slot);

@@ -222,7 +224,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
 {
 	int retval = -EINVAL;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot_name(hotplug_slot));
 
 	if (attention_info && try_module_get(attention_info->owner)) {
 		retval = attention_info->get_attn(hotplug_slot, value);

@@ -245,7 +247,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	*value = acpiphp_get_latch_status(slot->acpi_slot);

@@ -265,7 +267,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	*value = acpiphp_get_adapter_status(slot->acpi_slot);

@@ -299,7 +301,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	kfree(slot->hotplug_slot);
 	kfree(slot);

@@ -310,6 +312,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct slot *slot;
 	int retval = -ENOMEM;
+	char name[SLOT_NAME_SIZE];
 
 	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
 	if (!slot)

@@ -321,8 +324,6 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 
 	slot->hotplug_slot->info = &slot->info;
 
-	slot->hotplug_slot->name = slot->name;
-
 	slot->hotplug_slot->private = slot;
 	slot->hotplug_slot->release = &release_slot;
 	slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;

@@ -336,12 +337,12 @@ 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(name, SLOT_NAME_SIZE, "%u", slot->acpi_slot->sun);
 
 	retval = pci_hp_register(slot->hotplug_slot,
 					acpiphp_slot->bridge->pci_bus,
 					acpiphp_slot->device,
-					slot->name);
+					name);
 	if (retval == -EBUSY)
 		goto error_hpslot;
 	if (retval) {

@@ -349,7 +350,7 @@ int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 		goto error_hpslot;
  	}
 
-	info("Slot [%s] registered\n", slot->hotplug_slot->name);
+	info("Slot [%s] registered\n", slot_name(slot));
 
 	return 0;
 error_hpslot:

@@ -366,7 +367,7 @@ void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 	struct slot *slot = acpiphp_slot->slot;
 	int retval = 0;
 
-	info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
+	info("Slot [%s] unregistered\n", slot_name(slot));
 
 	retval = pci_hp_deregister(slot->hotplug_slot);
 	if (retval)

Alex Chiang

author

2008-10-09 04:48 UTC #17 lore raw

PATCH v5 7/16 PCI: cpci_hotplug: stop managing hotplug_slot->name

In reply to Alex Chiang

We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.

Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.

Cc [email protected]

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

---

 drivers/pci/hotplug/cpci_hotplug.h      |    6 ++
 drivers/pci/hotplug/cpci_hotplug_core.c |   76 ++++++++++++-------------------
 drivers/pci/hotplug/cpci_hotplug_pci.c  |    4 +-
 3 files changed, 37 insertions(+), 49 deletions(-)

drivers/pci/hotplug/cpci_hotplug.h +6 -0

index d9769b3..9fff878 100644
@@ -30,6 +30,7 @@
 
 #include <linux/types.h>
 #include <linux/pci.h>
+#include <linux/pci_hotplug.h>
 
 /* PICMG 2.1 R2.0 HS CSR bits: */
 #define HS_CSR_INS	0x0080

@@ -69,6 +70,11 @@ struct cpci_hp_controller {
 	struct cpci_hp_controller_ops *ops;
 };
 
+static inline const char *slot_name(struct slot *slot)
+{
+	return hotplug_slot_name(slot->hotplug_slot);
+}
+
 extern int cpci_hp_register_controller(struct cpci_hp_controller *controller);
 extern int cpci_hp_unregister_controller(struct cpci_hp_controller *controller);
 extern int cpci_hp_register_bus(struct pci_bus *bus, u8 first, u8 last);

drivers/pci/hotplug/cpci_hotplug_core.c +29 ~~-47~~

216 lines not on this page · raw message

drivers/pci/hotplug/cpci_hotplug_pci.c +2 -2

index df82b95..829c327 100644
@@ -209,7 +209,7 @@ int cpci_led_on(struct slot* slot)
 					      hs_cap + 2,
 					      hs_csr)) {
 			err("Could not set LOO for slot %s",
-			    slot->hotplug_slot->name);
+			    hotplug_slot_name(slot->hotplug_slot));
 			return -ENODEV;
 		}
 	}

@@ -238,7 +238,7 @@ int cpci_led_off(struct slot* slot)
 					      hs_cap + 2,
 					      hs_csr)) {
 			err("Could not clear LOO for slot %s",
-			    slot->hotplug_slot->name);
+			    hotplug_slot_name(slot->hotplug_slot));
 			return -ENODEV;
 		}
 	}

Alex Chiang

author

2008-10-09 04:49 UTC #18 lore raw

PATCH v5 8/16 PCI: cpqphp: stop managing hotplug_slot->name

In reply to Alex Chiang

We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.

Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.

Cc [email protected]

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

---

 drivers/pci/hotplug/cpqphp.h      |   13 ++++-------
 drivers/pci/hotplug/cpqphp_core.c |   42 +++++++++++++++++--------------------
 2 files changed, 24 insertions(+), 31 deletions(-)

drivers/pci/hotplug/cpqphp.h +5 -8

index b1decfa..afaf8f6 100644
@@ -449,6 +449,11 @@ extern u8 cpqhp_disk_irq;
 
 /* inline functions */
 
+static inline char *slot_name(struct slot *slot)
+{
+	return hotplug_slot_name(slot->hotplug_slot);
+}
+
 /*
  * return_resource
  *

@@ -696,14 +701,6 @@ static inline int get_presence_status(struct controller *ctrl, struct slot *slot
 	return presence_save;
 }
 
-#define SLOT_NAME_SIZE 10
-
-static inline void make_slot_name(char *buffer, int buffer_size, struct slot *slot)
-{
-	snprintf(buffer, buffer_size, "%d", slot->number);
-}
-
-
 static inline int wait_for_ctrl_irq(struct controller *ctrl)
 {
         DECLARE_WAITQUEUE(wait, current);

drivers/pci/hotplug/cpqphp_core.c +19 ~~-23~~

152 lines not on this page · raw message

Alex Chiang

author

2008-10-09 04:49 UTC #19 lore raw

PATCH v5 9/16 PCI: fakephp: remove 'name' parameter

In reply to Alex Chiang

Remove 'name' from fakephp's struct dummy_slot, as the PCI core will now manage our slot name for us.

Cc [email protected]

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

---

 drivers/pci/hotplug/fakephp.c |   19 ++++++++++---------
 1 files changed, 10 insertions(+), 9 deletions(-)

drivers/pci/hotplug/fakephp.c +10 -9

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

@@ -96,10 +95,13 @@ static void dummy_release(struct hotplug_slot *slot)
 	kfree(dslot);
 }
 
+#define SLOT_NAME_SIZE	8
+
 static int add_slot(struct pci_dev *dev)
 {
 	struct dummy_slot *dslot;
 	struct hotplug_slot *slot;
+	char name[SLOT_NAME_SIZE];
 	int retval = -ENOMEM;
 	static int count = 1;

@@ -119,20 +121,18 @@ static int add_slot(struct pci_dev *dev)
 	if (!dslot)
 		goto error_info;
 
-	slot->name = dslot->name;
-	snprintf(slot->name, sizeof(dslot->name), "fake%d", count++);
-	dbg("slot->name = %s\n", slot->name);
+	snprintf(name, SLOT_NAME_SIZE, "fake%d", count++);
 	slot->ops = &dummy_hotplug_slot_ops;
 	slot->release = &dummy_release;
 	slot->private = dslot;
 
-	retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn),
-				 slot->name);
+	retval = pci_hp_register(slot, dev->bus, PCI_SLOT(dev->devfn), name);
 	if (retval) {
 		err("pci_hp_register failed with error %d\n", retval);
 		goto error_dslot;
 	}
 
+	dbg("slot->name = %s\n", hotplug_slot_name(slot));
 	dslot->slot = slot;
 	dslot->dev = pci_dev_get(dev);
 	list_add (&dslot->node, &slot_list);

@@ -168,10 +168,11 @@ static void remove_slot(struct dummy_slot *dslot)
 {
 	int retval;
 
-	dbg("removing slot %s\n", dslot->slot->name);
+	dbg("removing slot %s\n", hotplug_slot_name(dslot->slot));
 	retval = pci_hp_deregister(dslot->slot);
 	if (retval)
-		err("Problem unregistering a slot %s\n", dslot->slot->name);
+		err("Problem unregistering a slot %s\n",
+			hotplug_slot_name(dslot->slot));
 }
 
 /* called from the single-threaded workqueue handler to remove a slot */

@@ -309,7 +310,7 @@ static int disable_slot(struct hotplug_slot *slot)
 		return -ENODEV;
 	dslot = slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot_name(slot));
 
 	for (func = 7; func >= 0; func--) {
 		dev = pci_get_slot(dslot->dev->bus, dslot->dev->devfn + func);

Alex Chiang

author

2008-10-09 04:50 UTC #20 lore raw

PATCH v5 10/16 PCI: ibmphp: stop managing hotplug_slot->name

In reply to Alex Chiang

We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.

Now, we simply advise the PCI core of the name that we would like, and let the core take care of the rest.

Additionally, slightly rearrange the members of struct slot so they are naturally aligned to eliminate holes.

Cc [email protected]

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

---

 drivers/pci/hotplug/ibmphp.h      |    5 ++---
 drivers/pci/hotplug/ibmphp_ebda.c |   20 +++++++-------------
 2 files changed, 9 insertions(+), 16 deletions(-)

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

index 612d963..a8d391a 100644
@@ -707,17 +707,16 @@ struct slot {
 	u8 device;
 	u8 number;
 	u8 real_physical_slot_num;
-	char name[100];
 	u32 capabilities;
 	u8 supported_speed;
 	u8 supported_bus_mode;
+	u8 flag;		/* this is for disable slot and polling */
+	u8 ctlr_index;
 	struct hotplug_slot *hotplug_slot;
 	struct controller *ctrl;
 	struct pci_func *func;
 	u8 irq[4];
-	u8 flag;		/* this is for disable slot and polling */
 	int bit_mode;		/* 0 = 32, 1 = 64 */
-	u8 ctlr_index;
 	struct bus_info *bus_on;
 	struct list_head ibm_slot_list;
 	u8 status;

drivers/pci/hotplug/ibmphp_ebda.c +7 ~~-13~~

index 85528d6..402dc10 100644
@@ -587,11 +587,14 @@ static u8 calculate_first_slot (u8 slot_num)
 	return first_slot + 1;
 
 }
+
+#define SLOT_NAME_SIZE 30
+
 static char *create_file_name (struct slot * slot_cur)
 {
 	struct opt_rio *opt_vg_ptr = NULL;
 	struct opt_rio_lo *opt_lo_ptr = NULL;
-	static char str[30];
+	static char str[SLOT_NAME_SIZE];
 	int which = 0; /* rxe = 1, chassis = 0 */
 	u8 number = 1; /* either chassis or rxe # */
 	u8 first_slot = 1;

@@ -703,7 +706,6 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
 
 	slot = hotplug_slot->private;
 	kfree(slot->hotplug_slot->info);
-	kfree(slot->hotplug_slot->name);
 	kfree(slot->hotplug_slot);
 	slot->ctrl = NULL;
 	slot->bus_on = NULL;

@@ -734,6 +736,7 @@ static int __init ebda_rsrc_controller (void)
 	struct bus_info *bus_info_ptr1, *bus_info_ptr2;
 	int rc;
 	struct slot *tmp_slot;
+	char name[SLOT_NAME_SIZE];
 
 	addr = hpc_list_ptr->phys_addr;
 	for (ctlr = 0; ctlr < hpc_list_ptr->num_ctlrs; ctlr++) {

@@ -897,12 +900,6 @@ static int __init ebda_rsrc_controller (void)
 				goto error_no_hp_info;
 			}
 
-			hp_slot_ptr->name = kmalloc(30, GFP_KERNEL);
-			if (!hp_slot_ptr->name) {
-				rc = -ENOMEM;
-				goto error_no_hp_name;
-			}
-
 			tmp_slot = kzalloc(sizeof(*tmp_slot), GFP_KERNEL);
 			if (!tmp_slot) {
 				rc = -ENOMEM;

@@ -964,10 +961,9 @@ static int __init ebda_rsrc_controller (void)
 	}			/* each hpc  */
 
 	list_for_each_entry(tmp_slot, &ibmphp_slot_head, ibm_slot_list) {
-		snprintf (tmp_slot->hotplug_slot->name, 30, "%s", create_file_name (tmp_slot));
+		snprintf(name, SLOT_NAME_SIZE, "%s", create_file_name(tmp_slot));
 		pci_hp_register(tmp_slot->hotplug_slot,
-			pci_find_bus(0, tmp_slot->bus), tmp_slot->device,
-			tmp_slot->hotplug_slot->name);
+			pci_find_bus(0, tmp_slot->bus), tmp_slot->device, name);
 	}
 
 	print_ebda_hpc ();

@@ -977,8 +973,6 @@ static int __init ebda_rsrc_controller (void)
 error:
 	kfree (hp_slot_ptr->private);
 error_no_slot:
-	kfree (hp_slot_ptr->name);
-error_no_hp_name:
 	kfree (hp_slot_ptr->info);
 error_no_hp_info:
 	kfree (hp_slot_ptr);

Alex Chiang

author

2008-10-09 04:50 UTC #21 lore raw

PATCH v5 11/16 PCI: pciehp: remove 'name' parameter

In reply to Alex Chiang

We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.

Remove 'name' from pciehp's version of struct slot, and remove unused 'task_list' as well.

Cc [email protected]

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

---

 drivers/pci/hotplug/pciehp.h      |    9 ++++--
 drivers/pci/hotplug/pciehp_core.c |   33 ++++++++++++-----------
 drivers/pci/hotplug/pciehp_ctrl.c |   53 ++++++++++++++++++++-----------------
 drivers/pci/hotplug/pciehp_hpc.c  |    1 -
 4 files changed, 52 insertions(+), 44 deletions(-)

drivers/pci/hotplug/pciehp.h +6 -3

index c367978..394f998 100644
@@ -74,15 +74,13 @@ 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];
 	unsigned long last_emi_toggle;
 	struct delayed_work work;	/* work for button event */
 	struct mutex lock;

@@ -175,6 +173,11 @@ int pciehp_enable_slot(struct slot *p_slot);
 int pciehp_disable_slot(struct slot *p_slot);
 int pcie_enable_notification(struct controller *ctrl);
 
+static inline const char *slot_name(struct slot *slot)
+{
+	return hotplug_slot_name(slot->hotplug_slot);
+}
+
 static inline struct slot *pciehp_find_slot(struct controller *ctrl, u8 device)
 {
 	struct slot *slot;

drivers/pci/hotplug/pciehp_core.c +17 ~~-16~~

index af89d7b..62be1b5 100644
@@ -185,7 +185,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
 	struct slot *slot = hotplug_slot->private;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, hotplug_slot_name(hotplug_slot));
 
 	kfree(hotplug_slot->info);
 	kfree(hotplug_slot);

@@ -196,6 +196,7 @@ static int init_slots(struct controller *ctrl)
 	struct slot *slot;
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
+	char name[SLOT_NAME_SIZE];
 	int retval = -ENOMEM;
 
 	list_for_each_entry(slot, &ctrl->slot_list, slot_list) {

@@ -209,15 +210,11 @@ static int init_slots(struct controller *ctrl)
 
 		/* register this slot with the hotplug pci core */
 		hotplug_slot->info = info;
-		hotplug_slot->name = slot->name;
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
 		hotplug_slot->ops = &pciehp_hotplug_slot_ops;
-		get_power_status(hotplug_slot, &info->power_status);
-		get_attention_status(hotplug_slot, &info->attention_status);
-		get_latch_status(hotplug_slot, &info->latch_status);
-		get_adapter_status(hotplug_slot, &info->adapter_status);
 		slot->hotplug_slot = hotplug_slot;
+		snprintf(name, SLOT_NAME_SIZE, "%u", slot->number);
 
 		ctrl_dbg(ctrl, "Registering bus=%x dev=%x hp_slot=%x sun=%x "
 			 "slot_device_offset=%x\n", slot->bus, slot->device,

@@ -225,12 +222,16 @@ static int init_slots(struct controller *ctrl)
 		retval = pci_hp_register(hotplug_slot,
 					 ctrl->pci_dev->subordinate,
 					 slot->device,
-					 slot->name);
+					 name);
 		if (retval) {
 			ctrl_err(ctrl, "pci_hp_register failed with error %d\n",
 				 retval);
 			goto error_info;
 		}
+		get_power_status(hotplug_slot, &info->power_status);
+		get_attention_status(hotplug_slot, &info->attention_status);
+		get_latch_status(hotplug_slot, &info->latch_status);
+		get_adapter_status(hotplug_slot, &info->adapter_status);
 		/* create additional sysfs entries */
 		if (EMI(ctrl)) {
 			retval = sysfs_create_file(&hotplug_slot->pci_slot->kobj,

@@ -273,7 +274,7 @@ static int set_attention_status(struct hotplug_slot *hotplug_slot, u8 status)
 	struct slot *slot = hotplug_slot->private;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		  __func__, hotplug_slot->name);
+		  __func__, slot_name(slot));
 
 	hotplug_slot->info->attention_status = status;

@@ -289,7 +290,7 @@ static int enable_slot(struct hotplug_slot *hotplug_slot)
 	struct slot *slot = hotplug_slot->private;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, slot_name(slot));
 
 	return pciehp_sysfs_enable_slot(slot);
 }

@@ -300,7 +301,7 @@ static int disable_slot(struct hotplug_slot *hotplug_slot)
 	struct slot *slot = hotplug_slot->private;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		  __func__, hotplug_slot->name);
+		  __func__, slot_name(slot));
 
 	return pciehp_sysfs_disable_slot(slot);
 }

@@ -311,7 +312,7 @@ static int get_power_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		  __func__, hotplug_slot->name);
+		  __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_power_status(slot, value);
 	if (retval < 0)

@@ -326,7 +327,7 @@ static int get_attention_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		  __func__, hotplug_slot->name);
+		  __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_attention_status(slot, value);
 	if (retval < 0)

@@ -341,7 +342,7 @@ static int get_latch_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_latch_status(slot, value);
 	if (retval < 0)

@@ -356,7 +357,7 @@ static int get_adapter_status(struct hotplug_slot *hotplug_slot, u8 *value)
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_adapter_status(slot, value);
 	if (retval < 0)

@@ -372,7 +373,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_max_bus_speed(slot, value);
 	if (retval < 0)

@@ -387,7 +388,7 @@ static int get_cur_bus_speed(struct hotplug_slot *hotplug_slot, enum pci_bus_spe
 	int retval;
 
 	ctrl_dbg(slot->ctrl, "%s - physical_slot = %s\n",
-		 __func__, hotplug_slot->name);
+		 __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_cur_bus_speed(slot, value);
 	if (retval < 0)

drivers/pci/hotplug/pciehp_ctrl.c +29 ~~-24~~

178 lines not on this page · raw message

drivers/pci/hotplug/pciehp_hpc.c +0 -1

index 8e9530c..4c74d53 100644
@@ -1061,7 +1061,6 @@ static int pcie_init_slot(struct controller *ctrl)
 	slot->device = ctrl->slot_device_offset + slot->hp_slot;
 	slot->hpc_ops = ctrl->hpc_ops;
 	slot->number = ctrl->first_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);

Alex Chiang

author

2008-10-09 04:50 UTC #22 lore raw

PATCH v5 12/16 PCI: rpaphp: kmalloc/kfree slot->name directly

In reply to Alex Chiang

rpaphp tends to use slot->name directly everywhere, and doesn't ever need slot->hotplug_slot->name.

struct hotplug_slot->name is going away, so convert rpaphp directly manipulate its own slot->name everywhere, and don't bother touching slot->hotplug_slot->name.

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

---

 drivers/pci/hotplug/rpaphp_slot.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

drivers/pci/hotplug/rpaphp_slot.c +3 -5

index 736d3b4..2ea9cf1 100644
@@ -43,7 +43,7 @@ static void rpaphp_release_slot(struct hotplug_slot *hotplug_slot)
 void dealloc_slot_struct(struct slot *slot)
 {
 	kfree(slot->hotplug_slot->info);
-	kfree(slot->hotplug_slot->name);
+	kfree(slot->name);
 	kfree(slot->hotplug_slot);
 	kfree(slot);
 }

@@ -63,11 +63,9 @@ struct slot *alloc_slot_struct(struct device_node *dn,
 					   GFP_KERNEL);
 	if (!slot->hotplug_slot->info)
 		goto error_hpslot;
-	slot->hotplug_slot->name = kmalloc(strlen(drc_name) + 1, GFP_KERNEL);
-	if (!slot->hotplug_slot->name)
+	slot->name = kstrdup(drc_name, GFP_KERNEL);
+	if (!slot->name)
 		goto error_info;	
-	slot->name = slot->hotplug_slot->name;
-	strcpy(slot->name, drc_name);
 	slot->dn = dn;
 	slot->index = drc_index;
 	slot->power_domain = power_domain;

Alex Chiang

author

2008-10-09 04:51 UTC #23 lore raw

PATCH v5 13/16 PCI: SGI Hotplug: stop managing bss_hotplug_slot->name

In reply to Alex Chiang

We no longer need to manage our version of hotplug_slot->name since the PCI and hotplug core manage it on our behalf.

Update the sn_hp_slot_private_alloc() interface to fill in the correct name for us, as that function already has all the parameters needed to determine the name.

Cc [email protected]

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

---

 drivers/pci/hotplug/sgi_hotplug.c |   19 ++++++-------------
 1 files changed, 6 insertions(+), 13 deletions(-)

drivers/pci/hotplug/sgi_hotplug.c +6 ~~-13~~

index 6d20bbd..d748698 100644
@@ -161,7 +161,8 @@ static int sn_pci_bus_valid(struct pci_bus *pci_bus)
 }
 
 static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot,
-				    struct pci_bus *pci_bus, int device)
+				    struct pci_bus *pci_bus, int device,
+				    char *name)
 {
 	struct pcibus_info *pcibus_info;
 	struct slot *slot;

@@ -173,15 +174,9 @@ static int sn_hp_slot_private_alloc(struct hotplug_slot *bss_hotplug_slot,
 		return -ENOMEM;
 	bss_hotplug_slot->private = slot;
 
-	bss_hotplug_slot->name = kmalloc(SN_SLOT_NAME_SIZE, GFP_KERNEL);
-	if (!bss_hotplug_slot->name) {
-		kfree(bss_hotplug_slot->private);
-		return -ENOMEM;
-	}
-
 	slot->device_num = device;
 	slot->pci_bus = pci_bus;
-	sprintf(bss_hotplug_slot->name, "%04x:%02x:%02x",
+	sprintf(name, "%04x:%02x:%02x",
 		pci_domain_nr(pci_bus),
 		((u16)pcibus_info->pbi_buscommon.bs_persist_busnum),
 		device + 1);

@@ -608,7 +603,6 @@ static inline int get_power_status(struct hotplug_slot *bss_hotplug_slot,
 static void sn_release_slot(struct hotplug_slot *bss_hotplug_slot)
 {
 	kfree(bss_hotplug_slot->info);
-	kfree(bss_hotplug_slot->name);
 	kfree(bss_hotplug_slot->private);
 	kfree(bss_hotplug_slot);
 }

@@ -618,6 +612,7 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
 	int device;
 	struct pci_slot *pci_slot;
 	struct hotplug_slot *bss_hotplug_slot;
+	char name[SN_SLOT_NAME_SIZE];
 	int rc = 0;
 
 	/*

@@ -645,16 +640,14 @@ static int sn_hotplug_slot_register(struct pci_bus *pci_bus)
 		}
 
 		if (sn_hp_slot_private_alloc(bss_hotplug_slot,
-					     pci_bus, device)) {
+					     pci_bus, device, name)) {
 			rc = -ENOMEM;
 			goto alloc_err;
 		}
-
 		bss_hotplug_slot->ops = &sn_hotplug_slot_ops;
 		bss_hotplug_slot->release = &sn_release_slot;
 
-		rc = pci_hp_register(bss_hotplug_slot, pci_bus, device,
-				     bss_hotplug_slot->name);
+		rc = pci_hp_register(bss_hotplug_slot, pci_bus, device, name);
 		if (rc)
 			goto register_err;

Alex Chiang

author

2008-10-09 04:52 UTC #24 lore raw

PATCH v5 14/16 PCI: shcphp: remove 'name' parameter

In reply to Alex Chiang

We do not need to manage our own name parameter, especially since the PCI core can change it on our behalf, in the case of duplicate slot names.

Remove 'name' from shpchp's version of struct slot.

This change also removes the unused struct task_event from the slot structure.

Cc [email protected]

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

---

 drivers/pci/hotplug/shpchp.h      |    9 +++++--
 drivers/pci/hotplug/shpchp_core.c |   38 ++++++++++++++---------------
 drivers/pci/hotplug/shpchp_ctrl.c |   48 +++++++++++++++++++------------------
 3 files changed, 48 insertions(+), 47 deletions(-)

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

index 8a026f7..4d9fed0 100644
@@ -69,15 +69,13 @@ struct slot {
 	u8 state;
 	u8 presence_save;
 	u8 pwr_save;
-	struct timer_list task_event;
-	u8 hp_slot;
 	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 {

@@ -169,6 +167,11 @@ extern void cleanup_slots(struct controller *ctrl);
 extern void shpchp_queue_pushbutton_work(struct work_struct *work);
 extern int shpc_init( struct controller *ctrl, struct pci_dev *pdev);
 
+static inline const char *slot_name(struct slot *slot)
+{
+	return hotplug_slot_name(slot->hotplug_slot);
+}
+
 #ifdef CONFIG_ACPI
 #include <linux/pci-acpi.h>
 static inline int get_hp_params_from_firmware(struct pci_dev *dev,

drivers/pci/hotplug/shpchp_core.c +18 ~~-20~~

index cfdd079..7af9191 100644
@@ -89,7 +89,7 @@ static void release_slot(struct hotplug_slot *hotplug_slot)
 {
 	struct slot *slot = hotplug_slot->private;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	kfree(slot->hotplug_slot->info);
 	kfree(slot->hotplug_slot);

@@ -101,6 +101,7 @@ static int init_slots(struct controller *ctrl)
 	struct slot *slot;
 	struct hotplug_slot *hotplug_slot;
 	struct hotplug_slot_info *info;
+	char name[SLOT_NAME_SIZE];
 	int retval = -ENOMEM;
 	int i;

@@ -119,8 +120,6 @@ static int init_slots(struct controller *ctrl)
 			goto error_hpslot;
 		hotplug_slot->info = info;
 
-		hotplug_slot->name = slot->name;
-
 		slot->hp_slot = i;
 		slot->ctrl = ctrl;
 		slot->bus = ctrl->pci_dev->subordinate->number;

@@ -133,25 +132,24 @@ static int init_slots(struct controller *ctrl)
 		/* register this slot with the hotplug pci core */
 		hotplug_slot->private = slot;
 		hotplug_slot->release = &release_slot;
-		snprintf(slot->name, SLOT_NAME_SIZE, "%d", slot->number);
+		snprintf(name, SLOT_NAME_SIZE, "%d", slot->number);
 		hotplug_slot->ops = &shpchp_hotplug_slot_ops;
 
-		get_power_status(hotplug_slot, &info->power_status);
-		get_attention_status(hotplug_slot, &info->attention_status);
-		get_latch_status(hotplug_slot, &info->latch_status);
-		get_adapter_status(hotplug_slot, &info->adapter_status);
-
 		dbg("Registering bus=%x dev=%x hp_slot=%x sun=%x "
 		    "slot_device_offset=%x\n", slot->bus, slot->device,
 		    slot->hp_slot, slot->number, ctrl->slot_device_offset);
 		retval = pci_hp_register(slot->hotplug_slot,
-				ctrl->pci_dev->subordinate, slot->device,
-				hotplug_slot->name);
+				ctrl->pci_dev->subordinate, slot->device, name);
 		if (retval) {
 			err("pci_hp_register failed with error %d\n", retval);
 			goto error_info;
 		}
 
+		get_power_status(hotplug_slot, &info->power_status);
+		get_attention_status(hotplug_slot, &info->attention_status);
+		get_latch_status(hotplug_slot, &info->latch_status);
+		get_adapter_status(hotplug_slot, &info->adapter_status);
+
 		list_add(&slot->slot_list, &ctrl->slot_list);
 	}

@@ -189,7 +187,7 @@ static int set_attention_status (struct hotplug_slot *hotplug_slot, u8 status)
 {
 	struct slot *slot = get_slot(hotplug_slot);
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	hotplug_slot->info->attention_status = status;
 	slot->hpc_ops->set_attention_status(slot, status);

@@ -201,7 +199,7 @@ static int enable_slot (struct hotplug_slot *hotplug_slot)
 {
 	struct slot *slot = get_slot(hotplug_slot);
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	return shpchp_sysfs_enable_slot(slot);
 }

@@ -210,7 +208,7 @@ static int disable_slot (struct hotplug_slot *hotplug_slot)
 {
 	struct slot *slot = get_slot(hotplug_slot);
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	return shpchp_sysfs_disable_slot(slot);
 }

@@ -220,7 +218,7 @@ static int get_power_status (struct hotplug_slot *hotplug_slot, u8 *value)
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_power_status(slot, value);
 	if (retval < 0)

@@ -234,7 +232,7 @@ static int get_attention_status (struct hotplug_slot *hotplug_slot, u8 *value)
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_attention_status(slot, value);
 	if (retval < 0)

@@ -248,7 +246,7 @@ static int get_latch_status (struct hotplug_slot *hotplug_slot, u8 *value)
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_latch_status(slot, value);
 	if (retval < 0)

@@ -262,7 +260,7 @@ static int get_adapter_status (struct hotplug_slot *hotplug_slot, u8 *value)
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_adapter_status(slot, value);
 	if (retval < 0)

@@ -277,7 +275,7 @@ static int get_max_bus_speed(struct hotplug_slot *hotplug_slot,
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_max_bus_speed(slot, value);
 	if (retval < 0)

@@ -291,7 +289,7 @@ static int get_cur_bus_speed (struct hotplug_slot *hotplug_slot, enum pci_bus_sp
 	struct slot *slot = get_slot(hotplug_slot);
 	int retval;
 
-	dbg("%s - physical_slot = %s\n", __func__, hotplug_slot->name);
+	dbg("%s - physical_slot = %s\n", __func__, slot_name(slot));
 
 	retval = slot->hpc_ops->get_cur_bus_speed(slot, value);
 	if (retval < 0)

drivers/pci/hotplug/shpchp_ctrl.c +24 ~~-24~~

166 lines not on this page · raw message

Alex Chiang

author

2008-10-09 04:51 UTC #25 lore raw

PATCH v5 15/16 PCI: Hotplug core: remove 'name'

In reply to Alex Chiang

Now that the PCI core manages the 'name' for each individual hotplug driver, and all drivers (except rpaphp) have been converted to use hotplug_slot_name(), there is no need for the PCI hotplug core to drag around its own copy of name either.

Cc [email protected]

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

---

 drivers/pci/hotplug/pci_hotplug_core.c |    6 +++---
 include/linux/pci_hotplug.h            |    3 ---
 2 files changed, 3 insertions(+), 6 deletions(-)

drivers/pci/hotplug/pci_hotplug_core.c +3 -3

index ed87585..4d8a8cd 100644
@@ -533,7 +533,7 @@ static struct hotplug_slot *get_slot_from_name (const char *name)
 	spin_lock(&pci_hotplug_slot_list_lock);
 	list_for_each (tmp, &pci_hotplug_slot_list) {
 		slot = list_entry (tmp, struct hotplug_slot, slot_list);
-		if (strcmp(slot->name, name) == 0)
+		if (strcmp(hotplug_slot_name(slot), name) == 0)
 			goto out;
 	}
 	slot = NULL;

@@ -616,7 +616,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
 	if (!hotplug)
 		return -ENODEV;
 
-	temp = get_slot_from_name(hotplug->name);
+	temp = get_slot_from_name(hotplug_slot_name(hotplug));
 	if (temp != hotplug)
 		return -ENODEV;

@@ -626,7 +626,7 @@ int pci_hp_deregister(struct hotplug_slot *hotplug)
 
 	slot = hotplug->pci_slot;
 	fs_remove_slot(slot);
-	dbg("Removed slot %s from the list\n", hotplug->name);
+	dbg("Removed slot %s from the list\n", hotplug_slot_name(hotplug));
 
 	hotplug->release(hotplug);
 	slot->hotplug = NULL;

include/linux/pci_hotplug.h +0 -3

index a3a3245..a00bd1a 100644
@@ -142,8 +142,6 @@ struct hotplug_slot_info {
 
 /**
  * struct hotplug_slot - used to register a physical slot with the hotplug pci core
- * @name: the name of the slot being registered.  This string must
- * be unique amoung slots registered on this system.
  * @ops: pointer to the &struct hotplug_slot_ops to be used for this slot
  * @info: pointer to the &struct hotplug_slot_info for the initial values for
  * this slot.

@@ -153,7 +151,6 @@ struct hotplug_slot_info {
  * needs.
  */
 struct hotplug_slot {
-	char				*name;
 	struct hotplug_slot_ops		*ops;
 	struct hotplug_slot_info	*info;
 	void (*release) (struct hotplug_slot *slot);

Alex Chiang

author

2008-10-09 04:52 UTC #26 lore raw

PATCH v5 16/16 PCI Hotplug: fakephp: add duplicate slot name debugging

In reply to Alex Chiang

The PCI core now manages slot names on behalf of slot detection and slot hotplug drivers, including the handling of duplicate slot names.

We can use the fakephp driver to help test the new functionality. Add a 'dup_slots' module param to force fakephp to create multiple slots with the same name. We can then verify that the PCI core correctly renamed the slots.

sapphire:/sys/bus/pci/slots # modprobe fakephp dup_slots sapphire:/sys/bus/pci/slots # ls
fake fake-10 fake-3 fake-5 fake-7 fake-9
fake-1 fake-2 fake-4 fake-6 fake-8

Cc [email protected]

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

---

 drivers/pci/hotplug/fakephp.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

drivers/pci/hotplug/fakephp.c +8 -2

index 24dcbf1..3a2637a 100644
@@ -69,6 +69,7 @@ struct dummy_slot {
 };
 
 static int debug;
+static int dup_slots;
 static LIST_HEAD(slot_list);
 static struct workqueue_struct *dummyphp_wq;

@@ -121,7 +122,11 @@ static int add_slot(struct pci_dev *dev)
 	if (!dslot)
 		goto error_info;
 
-	snprintf(name, SLOT_NAME_SIZE, "fake%d", count++);
+	if (dup_slots)
+		snprintf(name, SLOT_NAME_SIZE, "fake");
+	else
+		snprintf(name, SLOT_NAME_SIZE, "fake%d", count++);
+	dbg("slot->name = %s\n", name);
 	slot->ops = &dummy_hotplug_slot_ops;
 	slot->release = &dummy_release;
 	slot->private = dslot;

@@ -375,4 +380,5 @@ MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("GPL");
 module_param(debug, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(debug, "Debugging mode enabled or not");
-
+module_param(dup_slots, bool, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(dup_slots, "Force duplicate slot names for debugging");