[PATCH 0/7] acpiphp - slots management fix

[PATCH 0/7] acpiphp - slots management fix

[MUNEDA Takahiro]

Hi,

This series of patches contains slots management fix.
Here are summary of changes:

o [PATCH 1/7] - removes unnecesary struct members

o [PATCH 2/7] - removes init_slots(), because acpiphp doesn't
                register slots when driver is loaded.

o [PATCH 3/7] - changes register_slot() to register hotplug
                slots when they are found

o [PATCH 4/7] - changes init_bridge_misc() to register hotplug
                slots when they are found

o [PATCH 5/7] - removes cleanup_slots(), because acpiphp
                doesn't unregister slots when driver is going
                to be un-loaded.

o [PATCH 6/7] - changes cleanup_brige() to unregister hotplug
                slots when acpiphp cleanup the bridge.

o [PATCH 7/7] - adds spin_lock/_unlock to protect slot_list


For more specifc information, please see the header of each patch.
These patches are against 2.6.16-rc1. I tested them on Tiger4.

Thanks,
MUNE

-- 
MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>

Re: [PATCH 0/7] acpiphp - slots management fix

[Pavel Machek]

Hi!

> This series of patches contains slots management fix.
> Here are summary of changes:

Quite a lot of development happened in -mm in this area... Won't this
clash?
							Pavel
-- 
Thanks, Sharp!

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[MUNEDA Takahiro]

Hi Pavel,

At Fri, 20 Jan 2006 12:30:42 +0100,
Pavel Machek <pavel@suse.cz> wrote:
> 
> Quite a lot of development happened in -mm in this area... Won't this
> clash?

Thanks for your comments.
I have tested these patches on pure 2.6.16-rc1.
I haven't tested these patches applied with the Kristen's.
I guess it might be conflict with.

OK, new -mm was released, so I'll rebase the patch
against for 2.6.16-rc1-mm2 and Kristen's patches in a few days

Thanks,
MUNE

-- 
MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>

Re: [PATCH 0/7] acpiphp - slots management fix

[Greg KH]

On Fri, Jan 20, 2006 at 07:08:30PM +0900, MUNEDA Takahiro wrote:
> Hi,
> 
> This series of patches contains slots management fix.
> Here are summary of changes:
> 
> o [PATCH 1/7] - removes unnecesary struct members
> 
> o [PATCH 2/7] - removes init_slots(), because acpiphp doesn't
>                 register slots when driver is loaded.
> 
> o [PATCH 3/7] - changes register_slot() to register hotplug
>                 slots when they are found
> 
> o [PATCH 4/7] - changes init_bridge_misc() to register hotplug
>                 slots when they are found
> 
> o [PATCH 5/7] - removes cleanup_slots(), because acpiphp
>                 doesn't unregister slots when driver is going
>                 to be un-loaded.
> 
> o [PATCH 6/7] - changes cleanup_brige() to unregister hotplug
>                 slots when acpiphp cleanup the bridge.
> 
> o [PATCH 7/7] - adds spin_lock/_unlock to protect slot_list
> 
> 
> For more specifc information, please see the header of each patch.
> These patches are against 2.6.16-rc1. I tested them on Tiger4.

This patch series does not even compile when applied:

  CC [M]  drivers/pci/hotplug/acpiphp_core.o
drivers/pci/hotplug/acpiphp_core.c: In function `acpiphp_register_hotplug_slot':
drivers/pci/hotplug/acpiphp_core.c:355: error: `hotplug_slot' undeclared (first use in this function)
drivers/pci/hotplug/acpiphp_core.c:355: error: (Each undeclared identifier is reported only once
drivers/pci/hotplug/acpiphp_core.c:355: error: for each function it appears in.)
drivers/pci/hotplug/acpiphp_core.c:359: error: `hotplug_slot_info' undeclared (first use in this function)
make[2]: *** [drivers/pci/hotplug/acpiphp_core.o] Error 1

Care to redo them?

thanks,

greg k-h

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[MUNEDA Takahiro]

Hi,

At Thu, 26 Jan 2006 20:14:25 -0800,
Greg KH <greg@kroah.com> wrote:
> 
> On Fri, Jan 20, 2006 at 07:08:30PM +0900, MUNEDA Takahiro wrote:
> > Hi,
> > 
> > This series of patches contains slots management fix.
> > Here are summary of changes:
> > 
> > o [PATCH 1/7] - removes unnecesary struct members
> > 
> > o [PATCH 2/7] - removes init_slots(), because acpiphp doesn't
> >                 register slots when driver is loaded.
> > 
> > o [PATCH 3/7] - changes register_slot() to register hotplug
> >                 slots when they are found
> > 
> > o [PATCH 4/7] - changes init_bridge_misc() to register hotplug
> >                 slots when they are found
> > 
> > o [PATCH 5/7] - removes cleanup_slots(), because acpiphp
> >                 doesn't unregister slots when driver is going
> >                 to be un-loaded.
> > 
> > o [PATCH 6/7] - changes cleanup_brige() to unregister hotplug
> >                 slots when acpiphp cleanup the bridge.
> > 
> > o [PATCH 7/7] - adds spin_lock/_unlock to protect slot_list
> > 
> > 
> > For more specifc information, please see the header of each patch.
> > These patches are against 2.6.16-rc1. I tested them on Tiger4.
> 
> This patch series does not even compile when applied:
> 
>   CC [M]  drivers/pci/hotplug/acpiphp_core.o
> drivers/pci/hotplug/acpiphp_core.c: In function `acpiphp_register_hotplug_slot':
> drivers/pci/hotplug/acpiphp_core.c:355: error: `hotplug_slot' undeclared (first use in this function)
> drivers/pci/hotplug/acpiphp_core.c:355: error: (Each undeclared identifier is reported only once
> drivers/pci/hotplug/acpiphp_core.c:355: error: for each function it appears in.)
> drivers/pci/hotplug/acpiphp_core.c:359: error: `hotplug_slot_info' undeclared (first use in this function)
> make[2]: *** [drivers/pci/hotplug/acpiphp_core.o] Error 1
> 
> Care to redo them?


I'm sorry for my late replying.
And thanks for your taking care of my patch.

At first, please let me introduce the base idea of this patch.
I'm trying to support p2p bridge hotplug with acpiphp.

 Current acpiphp manages hotplug slot by ID. The ID is incremented
 when acpiphp founds hotpluggable slots.
 If the bridges(with hotpluggable slots) are hotplugged many times,
 the hotpluggable slots are added many times also. The ID might be
 overflowed. So this patch removes IDs to manage slots.

 And, this patch changes the slot register/unregister timing.
 Current acpiphp registers the slots in the init_slots()
 called from acpiphp_init(), and unregisters the slots in the
 cleanup_slots() called from acpiphp_exit().
 acpiphp doesn't assume the increase and decrease of the
 hotpluggable slots.
 This patch removes the slot register/unregister processes from the
 init/exit phases. Instead, adds the these processes in the bridge
 add/cleanup phases.

Currently, this change doesn't have any meanings. But these changes
are needed to support p2p bridge(with hotplug slot) hotplug.

Here is an updated patch. Old patches were not need to be separated,
so I merge them into below patch.

This patch is against 2.6.16-rc1-mm3. I tested them on Tiger4 and
it works fine.

Thanks,
MUNE

o This patch removes IDs to manage slots.
o This patch removes the slot register/unregister processes from the
  init/exit phases. Instead, adds these processes in the bridge 
  add/cleanup phases.

Signed-off-by: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>

 drivers/pci/hotplug/acpiphp.h      |    5 -
 drivers/pci/hotplug/acpiphp_core.c |  118 ++++++++++++++++++-------------------
 drivers/pci/hotplug/acpiphp_glue.c |   48 +++++++--------
 3 files changed, 85 insertions(+), 86 deletions(-)

Index: linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-2.6.16-rc1-mm3.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp.h
@@ -60,7 +60,6 @@ struct acpiphp_slot;
  * struct slot - slot information for each *physical* slot
  */
 struct slot {
-	u8 number;
 	struct hotplug_slot	*hotplug_slot;
 	struct list_head	slot_list;
 
@@ -121,7 +120,6 @@ struct acpiphp_slot {
 					   objects (i.e. for each function) */
 	struct mutex crit_sect;
 
-	u32		id;		/* slot id (serial #) for hotplug core */
 	u8		device;		/* pci device# */
 
 	u32		sun;		/* ACPI _SUN (slot unique number) */
@@ -206,12 +204,13 @@ struct acpiphp_attention_info
 /* acpiphp_core.c */
 extern int acpiphp_register_attention(struct acpiphp_attention_info*info);
 extern int acpiphp_unregister_attention(struct acpiphp_attention_info *info);
+extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
+extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
 
 /* acpiphp_glue.c */
 extern int acpiphp_glue_init (void);
 extern void acpiphp_glue_exit (void);
 extern int acpiphp_get_num_slots (void);
-extern struct acpiphp_slot *get_slot_from_id (int id);
 typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
 
 extern int acpiphp_enable_slot (struct acpiphp_slot *slot);
Index: linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp_core.c
===================================================================
--- linux-2.6.16-rc1-mm3.orig/drivers/pci/hotplug/acpiphp_core.c
+++ linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp_core.c
@@ -45,6 +45,7 @@
 #include "acpiphp.h"
 
 static LIST_HEAD(slot_list);
+static DEFINE_SPINLOCK(list_lock);
 
 #define MY_NAME	"acpiphp"
 
@@ -341,62 +342,56 @@ static void release_slot(struct hotplug_
 	kfree(slot);
 }
 
-/**
- * init_slots - initialize 'struct slot' structures for each slot
- *
- */
-static int __init init_slots(void)
+/* callback routine to initialize 'struct slot' for each slot */
+int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct slot *slot;
+	struct hotplug_slot *hotplug_slot;
+	struct hotplug_slot_info *hotplug_slot_info;
 	int retval = -ENOMEM;
-	int i;
 
-	for (i = 0; i < num_slots; ++i) {
-		slot = kmalloc(sizeof(struct slot), GFP_KERNEL);
-		if (!slot)
-			goto error;
-		memset(slot, 0, sizeof(struct slot));
-
-		slot->hotplug_slot = kmalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
-		if (!slot->hotplug_slot)
-			goto error_slot;
-		memset(slot->hotplug_slot, 0, sizeof(struct hotplug_slot));
-
-		slot->hotplug_slot->info = kmalloc(sizeof(struct hotplug_slot_info), GFP_KERNEL);
-		if (!slot->hotplug_slot->info)
-			goto error_hpslot;
-		memset(slot->hotplug_slot->info, 0, sizeof(struct hotplug_slot_info));
-
-		slot->hotplug_slot->name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
-		if (!slot->hotplug_slot->name)
-			goto error_info;
-
-		slot->number = i;
-
-		slot->hotplug_slot->private = slot;
-		slot->hotplug_slot->release = &release_slot;
-		slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
-
-		slot->acpi_slot = get_slot_from_id(i);
-		slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
-		slot->hotplug_slot->info->attention_status = 0;
-		slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
-		slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
-		slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
-		slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
-
-		make_slot_name(slot);
-
-		retval = pci_hp_register(slot->hotplug_slot);
-		if (retval) {
-			err("pci_hp_register failed with error %d\n", retval);
-			goto error_name;
-		}
-
-		/* add slot to our internal list */
-		list_add(&slot->slot_list, &slot_list);
-		info("Slot [%s] registered\n", slot->hotplug_slot->name);
-	}
+	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
+	if (!slot)
+		goto error;
+
+	slot->hotplug_slot = kzalloc(sizeof(*hotplug_slot), GFP_KERNEL);
+	if (!slot->hotplug_slot)
+		goto error_slot;
+
+	slot->hotplug_slot->info = kzalloc(sizeof(*hotplug_slot_info),
+					   GFP_KERNEL);
+	if (!slot->hotplug_slot->info)
+		goto error_hpslot;
+
+	slot->hotplug_slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+	if (!slot->hotplug_slot->name)
+		goto error_info;
+
+	slot->hotplug_slot->private = slot;
+	slot->hotplug_slot->release = &release_slot;
+	slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
+
+	slot->acpi_slot = acpiphp_slot;
+	slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
+	slot->hotplug_slot->info->attention_status = 0;
+	slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
+	slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
+	slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
+	slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
+
+	make_slot_name(slot);
+
+	retval = pci_hp_register(slot->hotplug_slot);
+	if (retval) {
+		err("pci_hp_register failed with error %d\n", retval);
+		goto error_name;
+ 	}
+
+	/* add slot to our internal list */
+	spin_lock(&list_lock);
+	list_add(&slot->slot_list, &slot_list);
+	spin_unlock(&list_lock);
+	info("Slot [%s] registered\n", slot->hotplug_slot->name);
 
 	return 0;
 error_name:
@@ -412,16 +407,26 @@ error:
 }
 
 
-static void __exit cleanup_slots (void)
+void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct list_head *tmp, *n;
 	struct slot *slot;
+	int retval = 0;
 
 	list_for_each_safe (tmp, n, &slot_list) {
 		/* memory will be freed in release_slot callback */
 		slot = list_entry(tmp, struct slot, slot_list);
+		if (slot->acpi_slot->sun != acpiphp_slot->sun)
+			continue;
+
+		spin_lock(&list_lock);
 		list_del(&slot->slot_list);
-		pci_hp_deregister(slot->hotplug_slot);
+		spin_unlock(&list_lock);
+		info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
+
+		retval = pci_hp_deregister(slot->hotplug_slot);
+		if (retval)
+			err("pci_hp_deregister failed with error %d\n", retval);
 	}
 }
 
@@ -436,16 +441,13 @@ static int __init acpiphp_init(void)
 
 	/* read all the ACPI info from the system */
 	retval = init_acpi();
-	if (retval)
-		return retval;
 
-	return init_slots();
+	return retval;
 }
 
 
 static void __exit acpiphp_exit(void)
 {
-	cleanup_slots();
 	/* deallocate internal data structures etc. */
 	acpiphp_glue_exit();
 }
Index: linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.16-rc1-mm3.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6.16-rc1-mm3/drivers/pci/hotplug/acpiphp_glue.c
@@ -154,8 +154,7 @@ register_slot(acpi_handle handle, u32 lv
 	acpi_handle tmp;
 	acpi_status status = AE_OK;
 	unsigned long adr, sun;
-	int device, function;
-	static int num_slots = 0;	/* XXX if we support I/O node hotplug... */
+	int device, function, retval;
 
 	status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
 
@@ -210,7 +209,6 @@ register_slot(acpi_handle handle, u32 lv
 
 		memset(slot, 0, sizeof(struct acpiphp_slot));
 		slot->bridge = bridge;
-		slot->id = num_slots++;
 		slot->device = device;
 		slot->sun = sun;
 		INIT_LIST_HEAD(&slot->funcs);
@@ -224,6 +222,11 @@ register_slot(acpi_handle handle, u32 lv
 		dbg("found ACPI PCI Hotplug slot %d at PCI %04x:%02x:%02x\n",
 				slot->sun, pci_domain_nr(bridge->pci_bus),
 				bridge->pci_bus->number, slot->device);
+		retval = acpiphp_register_hotplug_slot(slot);
+		if (retval) {
+			warn("acpiphp_register_hotplug_slot failed(err code = 0x%x)\n", retval);
+			goto err_exit;
+		}
 	}
 
 	newfunc->slot = slot;
@@ -254,6 +257,14 @@ register_slot(acpi_handle handle, u32 lv
 	}
 
 	return AE_OK;
+
+ err_exit:
+	bridge->nr_slots--;
+	bridge->slots = slot->next;
+	kfree(slot);
+	kfree(newfunc);
+
+	return AE_OK;
 }
 
 
@@ -336,9 +347,16 @@ static void init_bridge_misc(struct acpi
 	/* decode ACPI 2.0 _HPP (hot plug parameters) */
 	decode_hpp(bridge);
 
+	/* must be added to the list prior to calling register_slot */
+	list_add(&bridge->list, &bridge_list);
+
 	/* register all slot objects under this bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
 				     register_slot, bridge, NULL);
+	if (ACPI_FAILURE(status)) {
+		list_del(&bridge->list);
+		return;
+	}
 
 	/* install notify handler */
 	if (bridge->type != BRIDGE_TYPE_HOST) {
@@ -351,8 +369,6 @@ static void init_bridge_misc(struct acpi
 			err("failed to register interrupt notify handler\n");
 		}
 	}
-
-	list_add(&bridge->list, &bridge_list);
 }
 
 
@@ -553,6 +569,8 @@ static void cleanup_bridge(struct acpiph
 			list_del(list);
 			kfree(func);
 		}
+		acpiphp_unregister_hotplug_slot(slot);
+		list_del(&slot->funcs);
 		kfree(slot);
 		slot = next;
 	}
@@ -1570,26 +1588,6 @@ static int acpiphp_for_each_slot(acpiphp
 }
 #endif
 
-/* search matching slot from id  */
-struct acpiphp_slot *get_slot_from_id(int id)
-{
-	struct list_head *node;
-	struct acpiphp_bridge *bridge;
-	struct acpiphp_slot *slot;
-
-	list_for_each (node, &bridge_list) {
-		bridge = (struct acpiphp_bridge *)node;
-		for (slot = bridge->slots; slot; slot = slot->next)
-			if (slot->id == id)
-				return slot;
-	}
-
-	/* should never happen! */
-	err("%s: no object for id %d\n", __FUNCTION__, id);
-	WARN_ON(1);
-	return NULL;
-}
-
 
 /**
  * acpiphp_enable_slot - power on slot

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[Greg KH]

On Sun, Jan 29, 2006 at 12:34:29PM +0900, MUNEDA Takahiro wrote:
> Hi,
> 
> At Thu, 26 Jan 2006 20:14:25 -0800,
> Greg KH <greg@kroah.com> wrote:
> > 
> > On Fri, Jan 20, 2006 at 07:08:30PM +0900, MUNEDA Takahiro wrote:
> > > Hi,
> > > 
> > > This series of patches contains slots management fix.
> > > Here are summary of changes:
> > > 
> > > o [PATCH 1/7] - removes unnecesary struct members
> > > 
> > > o [PATCH 2/7] - removes init_slots(), because acpiphp doesn't
> > >                 register slots when driver is loaded.
> > > 
> > > o [PATCH 3/7] - changes register_slot() to register hotplug
> > >                 slots when they are found
> > > 
> > > o [PATCH 4/7] - changes init_bridge_misc() to register hotplug
> > >                 slots when they are found
> > > 
> > > o [PATCH 5/7] - removes cleanup_slots(), because acpiphp
> > >                 doesn't unregister slots when driver is going
> > >                 to be un-loaded.
> > > 
> > > o [PATCH 6/7] - changes cleanup_brige() to unregister hotplug
> > >                 slots when acpiphp cleanup the bridge.
> > > 
> > > o [PATCH 7/7] - adds spin_lock/_unlock to protect slot_list
> > > 
> > > 
> > > For more specifc information, please see the header of each patch.
> > > These patches are against 2.6.16-rc1. I tested them on Tiger4.
> > 
> > This patch series does not even compile when applied:
> > 
> >   CC [M]  drivers/pci/hotplug/acpiphp_core.o
> > drivers/pci/hotplug/acpiphp_core.c: In function `acpiphp_register_hotplug_slot':
> > drivers/pci/hotplug/acpiphp_core.c:355: error: `hotplug_slot' undeclared (first use in this function)
> > drivers/pci/hotplug/acpiphp_core.c:355: error: (Each undeclared identifier is reported only once
> > drivers/pci/hotplug/acpiphp_core.c:355: error: for each function it appears in.)
> > drivers/pci/hotplug/acpiphp_core.c:359: error: `hotplug_slot_info' undeclared (first use in this function)
> > make[2]: *** [drivers/pci/hotplug/acpiphp_core.o] Error 1
> > 
> > Care to redo them?
> 
> 
> I'm sorry for my late replying.
> And thanks for your taking care of my patch.
> 
> At first, please let me introduce the base idea of this patch.
> I'm trying to support p2p bridge hotplug with acpiphp.
> 
>  Current acpiphp manages hotplug slot by ID. The ID is incremented
>  when acpiphp founds hotpluggable slots.
>  If the bridges(with hotpluggable slots) are hotplugged many times,
>  the hotpluggable slots are added many times also. The ID might be
>  overflowed. So this patch removes IDs to manage slots.
> 
>  And, this patch changes the slot register/unregister timing.
>  Current acpiphp registers the slots in the init_slots()
>  called from acpiphp_init(), and unregisters the slots in the
>  cleanup_slots() called from acpiphp_exit().
>  acpiphp doesn't assume the increase and decrease of the
>  hotpluggable slots.
>  This patch removes the slot register/unregister processes from the
>  init/exit phases. Instead, adds the these processes in the bridge
>  add/cleanup phases.
> 
> Currently, this change doesn't have any meanings. But these changes
> are needed to support p2p bridge(with hotplug slot) hotplug.
> 
> Here is an updated patch. Old patches were not need to be separated,
> so I merge them into below patch.
> 
> This patch is against 2.6.16-rc1-mm3. I tested them on Tiger4 and
> it works fine.

Ok, Kristen, have any objections to this patch?  I know it's in much the
same area that you are working in.

thanks,

greg k-h

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[Kristen Accardi]

On Sat, 2006-01-28 at 19:45 -0800, Greg KH wrote:
> On Sun, Jan 29, 2006 at 12:34:29PM +0900, MUNEDA Takahiro wrote:
> > Hi,
> > 
> > At Thu, 26 Jan 2006 20:14:25 -0800,
> > Greg KH <greg@kroah.com> wrote:
> > > 
> > > On Fri, Jan 20, 2006 at 07:08:30PM +0900, MUNEDA Takahiro wrote:
> > > > Hi,
> > > > 
> > > > This series of patches contains slots management fix.
> > > > Here are summary of changes:
> > > > 
> > > > o [PATCH 1/7] - removes unnecesary struct members
> > > > 
> > > > o [PATCH 2/7] - removes init_slots(), because acpiphp doesn't
> > > >                 register slots when driver is loaded.
> > > > 
> > > > o [PATCH 3/7] - changes register_slot() to register hotplug
> > > >                 slots when they are found
> > > > 
> > > > o [PATCH 4/7] - changes init_bridge_misc() to register hotplug
> > > >                 slots when they are found
> > > > 
> > > > o [PATCH 5/7] - removes cleanup_slots(), because acpiphp
> > > >                 doesn't unregister slots when driver is going
> > > >                 to be un-loaded.
> > > > 
> > > > o [PATCH 6/7] - changes cleanup_brige() to unregister hotplug
> > > >                 slots when acpiphp cleanup the bridge.
> > > > 
> > > > o [PATCH 7/7] - adds spin_lock/_unlock to protect slot_list
> > > > 
> > > > 
> > > > For more specifc information, please see the header of each patch.
> > > > These patches are against 2.6.16-rc1. I tested them on Tiger4.
> > > 
> > > This patch series does not even compile when applied:
> > > 
> > >   CC [M]  drivers/pci/hotplug/acpiphp_core.o
> > > drivers/pci/hotplug/acpiphp_core.c: In function `acpiphp_register_hotplug_slot':
> > > drivers/pci/hotplug/acpiphp_core.c:355: error: `hotplug_slot' undeclared (first use in this function)
> > > drivers/pci/hotplug/acpiphp_core.c:355: error: (Each undeclared identifier is reported only once
> > > drivers/pci/hotplug/acpiphp_core.c:355: error: for each function it appears in.)
> > > drivers/pci/hotplug/acpiphp_core.c:359: error: `hotplug_slot_info' undeclared (first use in this function)
> > > make[2]: *** [drivers/pci/hotplug/acpiphp_core.o] Error 1
> > > 
> > > Care to redo them?
> > 
> > 
> > I'm sorry for my late replying.
> > And thanks for your taking care of my patch.
> > 
> > At first, please let me introduce the base idea of this patch.
> > I'm trying to support p2p bridge hotplug with acpiphp.
> > 
> >  Current acpiphp manages hotplug slot by ID. The ID is incremented
> >  when acpiphp founds hotpluggable slots.
> >  If the bridges(with hotpluggable slots) are hotplugged many times,
> >  the hotpluggable slots are added many times also. The ID might be
> >  overflowed. So this patch removes IDs to manage slots.
> > 
> >  And, this patch changes the slot register/unregister timing.
> >  Current acpiphp registers the slots in the init_slots()
> >  called from acpiphp_init(), and unregisters the slots in the
> >  cleanup_slots() called from acpiphp_exit().
> >  acpiphp doesn't assume the increase and decrease of the
> >  hotpluggable slots.
> >  This patch removes the slot register/unregister processes from the
> >  init/exit phases. Instead, adds the these processes in the bridge
> >  add/cleanup phases.
> > 
> > Currently, this change doesn't have any meanings. But these changes
> > are needed to support p2p bridge(with hotplug slot) hotplug.
> > 
> > Here is an updated patch. Old patches were not need to be separated,
> > so I merge them into below patch.
> > 
> > This patch is against 2.6.16-rc1-mm3. I tested them on Tiger4 and
> > it works fine.
> 
> Ok, Kristen, have any objections to this patch?  I know it's in much the
> same area that you are working in.
> 
> thanks,
> 
> greg k-h
> -

Just reviewing them, they seem like they will be fine to me.

Thanks,
Kristen

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[Greg KH]

On Tue, Jan 31, 2006 at 10:20:18AM -0800, Kristen Accardi wrote:
> Just reviewing them, they seem like they will be fine to me.

This conflicts with your other patch you sent me.  I'll keep yours, and
drop this one, and let you merge his in with yours.

Is that ok?

thanks,

greg k-h

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[Kristen Accardi]

On Tue, 2006-02-07 at 12:13 -0800, Greg KH wrote:
> On Tue, Jan 31, 2006 at 10:20:18AM -0800, Kristen Accardi wrote:
> > Just reviewing them, they seem like they will be fine to me.
> 
> This conflicts with your other patch you sent me.  I'll keep yours, and
> drop this one, and let you merge his in with yours.
> 
> Is that ok?
> 
> thanks,
> 
> greg k-h

I tried to merge this in against 2.6.16-rc2-mm1, which contains the
patch you kept, and there are compile issues in his patch still.  I
think it would be best if Muneda merged these himself.

Kristen

Re: [Pcihpd-discuss] Re: [PATCH 0/7] acpiphp - slots management fix

[MUNEDA Takahiro]

Hi Kristen,

At Fri, 10 Feb 2006 11:28:24 -0800,
Kristen Accardi <kristen.c.accardi@intel.com> wrote:
> 
> On Tue, 2006-02-07 at 12:13 -0800, Greg KH wrote:
> > On Tue, Jan 31, 2006 at 10:20:18AM -0800, Kristen Accardi wrote:
> > > Just reviewing them, they seem like they will be fine to me.
> > 
> > This conflicts with your other patch you sent me.  I'll keep yours, and
> > drop this one, and let you merge his in with yours.
> > 
> > Is that ok?
> > 
> > thanks,
> > 
> > greg k-h
> 
> I tried to merge this in against 2.6.16-rc2-mm1, which contains the
> patch you kept, and there are compile issues in his patch still.  I
> think it would be best if Muneda merged these himself.

OK, I understood.
However, I can not insmod acpiphp on 2.6.12-rc2-mm1 because
of the dock related error(I reported to list a little while
ago), I'll merge it after the error is fixed.

Unfortunately, I don't have any laptop with _DCK method. So
I can't fix/test the patch.

Thanks,
MUNE

[PATCH] acpiphp - slot management fix - V2

[MUNEDA Takahiro]

Hi,

At Mon, 13 Feb 2006 11:27:28 +0900,
MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com> wrote:
> 
> > I tried to merge this in against 2.6.16-rc2-mm1, which contains the
> > patch you kept, and there are compile issues in his patch still.  I
> > think it would be best if Muneda merged these himself.
> 
> OK, I understood.
> However, I can not insmod acpiphp on 2.6.12-rc2-mm1 because
> of the dock related error(I reported to list a little while
> ago), I'll merge it after the error is fixed.

Kenji fixed the _DCK method problem, so I rebased my patch
against 2.6.16-rc2-mm1 + kenji's patch.
I tested them on Tiger4 and it works fine.

Thanks,
MUNE

o This patch removes IDs to manage slots.
o This patch removes the slot register/unregister processes
  from the init/exit phases. Instead, adds these processes
  in the bridge add/cleanup phases.
o Currently, this change doesn't have any meanings. But
  these changes are needed to support p2p bridge(with
  hotplug slot)

Signed-off-by: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>

 drivers/pci/hotplug/acpiphp.h      |    5 -
 drivers/pci/hotplug/acpiphp_core.c |  118 ++++++++++++++++++-------------------
 drivers/pci/hotplug/acpiphp_glue.c |   48 +++++++--------
 3 files changed, 85 insertions(+), 86 deletions(-)

Index: linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp.h
@@ -60,7 +60,6 @@ struct acpiphp_slot;
  * struct slot - slot information for each *physical* slot
  */
 struct slot {
-	u8 number;
 	struct hotplug_slot	*hotplug_slot;
 	struct list_head	slot_list;
 
@@ -121,7 +120,6 @@ struct acpiphp_slot {
 					   objects (i.e. for each function) */
 	struct mutex crit_sect;
 
-	u32		id;		/* slot id (serial #) for hotplug core */
 	u8		device;		/* pci device# */
 
 	u32		sun;		/* ACPI _SUN (slot unique number) */
@@ -229,12 +227,13 @@ struct acpiphp_dock_station {
 /* acpiphp_core.c */
 extern int acpiphp_register_attention(struct acpiphp_attention_info*info);
 extern int acpiphp_unregister_attention(struct acpiphp_attention_info *info);
+extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
+extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
 
 /* acpiphp_glue.c */
 extern int acpiphp_glue_init (void);
 extern void acpiphp_glue_exit (void);
 extern int acpiphp_get_num_slots (void);
-extern struct acpiphp_slot *get_slot_from_id (int id);
 typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
 void handle_hotplug_event_func(acpi_handle, u32, void*);
 
Index: linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_core.c
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/pci/hotplug/acpiphp_core.c
+++ linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_core.c
@@ -45,6 +45,7 @@
 #include "acpiphp.h"
 
 static LIST_HEAD(slot_list);
+static DEFINE_SPINLOCK(list_lock);
 
 #define MY_NAME	"acpiphp"
 
@@ -341,62 +342,56 @@ static void release_slot(struct hotplug_
 	kfree(slot);
 }
 
-/**
- * init_slots - initialize 'struct slot' structures for each slot
- *
- */
-static int __init init_slots(void)
+/* callback routine to initialize 'struct slot' for each slot */
+int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct slot *slot;
+	struct hotplug_slot *hotplug_slot;
+	struct hotplug_slot_info *hotplug_slot_info;
 	int retval = -ENOMEM;
-	int i;
 
-	for (i = 0; i < num_slots; ++i) {
-		slot = kmalloc(sizeof(struct slot), GFP_KERNEL);
-		if (!slot)
-			goto error;
-		memset(slot, 0, sizeof(struct slot));
-
-		slot->hotplug_slot = kmalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
-		if (!slot->hotplug_slot)
-			goto error_slot;
-		memset(slot->hotplug_slot, 0, sizeof(struct hotplug_slot));
-
-		slot->hotplug_slot->info = kmalloc(sizeof(struct hotplug_slot_info), GFP_KERNEL);
-		if (!slot->hotplug_slot->info)
-			goto error_hpslot;
-		memset(slot->hotplug_slot->info, 0, sizeof(struct hotplug_slot_info));
-
-		slot->hotplug_slot->name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
-		if (!slot->hotplug_slot->name)
-			goto error_info;
-
-		slot->number = i;
-
-		slot->hotplug_slot->private = slot;
-		slot->hotplug_slot->release = &release_slot;
-		slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
-
-		slot->acpi_slot = get_slot_from_id(i);
-		slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
-		slot->hotplug_slot->info->attention_status = 0;
-		slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
-		slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
-		slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
-		slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
-
-		make_slot_name(slot);
-
-		retval = pci_hp_register(slot->hotplug_slot);
-		if (retval) {
-			err("pci_hp_register failed with error %d\n", retval);
-			goto error_name;
-		}
-
-		/* add slot to our internal list */
-		list_add(&slot->slot_list, &slot_list);
-		info("Slot [%s] registered\n", slot->hotplug_slot->name);
-	}
+	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
+	if (!slot)
+		goto error;
+
+	slot->hotplug_slot = kzalloc(sizeof(*hotplug_slot), GFP_KERNEL);
+	if (!slot->hotplug_slot)
+		goto error_slot;
+
+	slot->hotplug_slot->info = kzalloc(sizeof(*hotplug_slot_info),
+					   GFP_KERNEL);
+	if (!slot->hotplug_slot->info)
+		goto error_hpslot;
+
+	slot->hotplug_slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+	if (!slot->hotplug_slot->name)
+		goto error_info;
+
+	slot->hotplug_slot->private = slot;
+	slot->hotplug_slot->release = &release_slot;
+	slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
+
+	slot->acpi_slot = acpiphp_slot;
+	slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
+	slot->hotplug_slot->info->attention_status = 0;
+	slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
+	slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
+	slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
+	slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
+
+	make_slot_name(slot);
+
+	retval = pci_hp_register(slot->hotplug_slot);
+	if (retval) {
+		err("pci_hp_register failed with error %d\n", retval);
+		goto error_name;
+ 	}
+
+	/* add slot to our internal list */
+	spin_lock(&list_lock);
+	list_add(&slot->slot_list, &slot_list);
+	spin_unlock(&list_lock);
+	info("Slot [%s] registered\n", slot->hotplug_slot->name);
 
 	return 0;
 error_name:
@@ -412,16 +407,26 @@ error:
 }
 
 
-static void __exit cleanup_slots (void)
+void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct list_head *tmp, *n;
 	struct slot *slot;
+	int retval = 0;
 
 	list_for_each_safe (tmp, n, &slot_list) {
 		/* memory will be freed in release_slot callback */
 		slot = list_entry(tmp, struct slot, slot_list);
+		if (slot->acpi_slot->sun != acpiphp_slot->sun)
+			continue;
+
+		spin_lock(&list_lock);
 		list_del(&slot->slot_list);
-		pci_hp_deregister(slot->hotplug_slot);
+		spin_unlock(&list_lock);
+		info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
+
+		retval = pci_hp_deregister(slot->hotplug_slot);
+		if (retval)
+			err("pci_hp_deregister failed with error %d\n", retval);
 	}
 }
 
@@ -439,16 +444,13 @@ static int __init acpiphp_init(void)
 
 	/* read all the ACPI info from the system */
 	retval = init_acpi();
-	if (retval && !(docking_station))
-		return retval;
 
-	return init_slots();
+	return retval;
 }
 
 
 static void __exit acpiphp_exit(void)
 {
-	cleanup_slots();
 	/* deallocate internal data structures etc. */
 	acpiphp_glue_exit();
 
Index: linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_glue.c
@@ -128,8 +128,7 @@ register_slot(acpi_handle handle, u32 lv
 	acpi_handle tmp;
 	acpi_status status = AE_OK;
 	unsigned long adr, sun;
-	int device, function;
-	static int num_slots = 0;	/* XXX if we support I/O node hotplug... */
+	int device, function, retval;
 
 	status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
 
@@ -198,7 +197,6 @@ register_slot(acpi_handle handle, u32 lv
 
 		memset(slot, 0, sizeof(struct acpiphp_slot));
 		slot->bridge = bridge;
-		slot->id = num_slots++;
 		slot->device = device;
 		slot->sun = sun;
 		INIT_LIST_HEAD(&slot->funcs);
@@ -212,6 +210,11 @@ register_slot(acpi_handle handle, u32 lv
 		dbg("found ACPI PCI Hotplug slot %d at PCI %04x:%02x:%02x\n",
 				slot->sun, pci_domain_nr(bridge->pci_bus),
 				bridge->pci_bus->number, slot->device);
+		retval = acpiphp_register_hotplug_slot(slot);
+		if (retval) {
+			warn("acpiphp_register_hotplug_slot failed(err code = 0x%x)\n", retval);
+			goto err_exit;
+		}
 	}
 
 	newfunc->slot = slot;
@@ -246,6 +249,14 @@ register_slot(acpi_handle handle, u32 lv
 	}
 
 	return status;
+
+ err_exit:
+	bridge->nr_slots--;
+	bridge->slots = slot->next;
+	kfree(slot);
+	kfree(newfunc);
+
+	return AE_OK;
 }
 
 
@@ -328,9 +339,16 @@ static void init_bridge_misc(struct acpi
 	/* decode ACPI 2.0 _HPP (hot plug parameters) */
 	decode_hpp(bridge);
 
+	/* must be added to the list prior to calling register_slot */
+	list_add(&bridge->list, &bridge_list);
+
 	/* register all slot objects under this bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
 				     register_slot, bridge, NULL);
+	if (ACPI_FAILURE(status)) {
+		list_del(&bridge->list);
+		return;
+	}
 
 	/* install notify handler */
 	if (bridge->type != BRIDGE_TYPE_HOST) {
@@ -343,8 +361,6 @@ static void init_bridge_misc(struct acpi
 			err("failed to register interrupt notify handler\n");
 		}
 	}
-
-	list_add(&bridge->list, &bridge_list);
 }
 
 
@@ -548,6 +564,8 @@ static void cleanup_bridge(struct acpiph
 			list_del(list);
 			kfree(func);
 		}
+		acpiphp_unregister_hotplug_slot(slot);
+		list_del(&slot->funcs);
 		kfree(slot);
 		slot = next;
 	}
@@ -1400,26 +1418,6 @@ static int acpiphp_for_each_slot(acpiphp
 }
 #endif
 
-/* search matching slot from id  */
-struct acpiphp_slot *get_slot_from_id(int id)
-{
-	struct list_head *node;
-	struct acpiphp_bridge *bridge;
-	struct acpiphp_slot *slot;
-
-	list_for_each (node, &bridge_list) {
-		bridge = (struct acpiphp_bridge *)node;
-		for (slot = bridge->slots; slot; slot = slot->next)
-			if (slot->id == id)
-				return slot;
-	}
-
-	/* should never happen! */
-	err("%s: no object for id %d\n", __FUNCTION__, id);
-	WARN_ON(1);
-	return NULL;
-}
-
 
 /**
  * acpiphp_enable_slot - power on slot

Re: [Pcihpd-discuss] [PATCH] acpiphp - slot management fix - V2

[Kenji Kaneshige]

MUNEDA Takahiro wrote:
> -static void __exit cleanup_slots (void)
> +void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
>  {
>  	struct list_head *tmp, *n;
>  	struct slot *slot;
> +	int retval = 0;
>  
>  	list_for_each_safe (tmp, n, &slot_list) {
>  		/* memory will be freed in release_slot callback */
>  		slot = list_entry(tmp, struct slot, slot_list);
> +		if (slot->acpi_slot->sun != acpiphp_slot->sun)
> +			continue;
> +
> +		spin_lock(&list_lock);
>  		list_del(&slot->slot_list);
> -		pci_hp_deregister(slot->hotplug_slot);
> +		spin_unlock(&list_lock);
> +		info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
> +
> +		retval = pci_hp_deregister(slot->hotplug_slot);
> +		if (retval)
> +			err("pci_hp_deregister failed with error %d\n", retval);
>  	}
>  }

With your patch, I think this list_for_each_safe() loop becomes
needless and it should be removed. In addition, slot_list is no
longer needed.

Here is a sample patch against your patch. Note that I have not
tested it very much.

Thanks,
Kenji Kaneshige


 drivers/pci/hotplug/acpiphp.h      |    3 +--
 drivers/pci/hotplug/acpiphp_core.c |   32 ++++++++------------------------
 2 files changed, 9 insertions(+), 26 deletions(-)

Index: linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/pci/hotplug/acpiphp.h	2006-02-14 17:05:47.000000000 +0900
+++ linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp.h	2006-02-14 18:02:38.000000000 +0900
@@ -61,8 +61,6 @@
  */
 struct slot {
 	struct hotplug_slot	*hotplug_slot;
-	struct list_head	slot_list;
-
 	struct acpiphp_slot	*acpi_slot;
 };
 
@@ -118,6 +116,7 @@
 	struct acpiphp_bridge *bridge;	/* parent */
 	struct list_head funcs;		/* one slot may have different
 					   objects (i.e. for each function) */
+	struct slot *slot;
 	struct mutex crit_sect;
 
 	u8		device;		/* pci device# */
Index: linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_core.c
===================================================================
--- linux-2.6.16-rc2-mm1.orig/drivers/pci/hotplug/acpiphp_core.c	2006-02-14 17:05:47.000000000 +0900
+++ linux-2.6.16-rc2-mm1/drivers/pci/hotplug/acpiphp_core.c	2006-02-14 18:02:50.000000000 +0900
@@ -44,9 +44,6 @@
 #include "pci_hotplug.h"
 #include "acpiphp.h"
 
-static LIST_HEAD(slot_list);
-static DEFINE_SPINLOCK(list_lock);
-
 #define MY_NAME	"acpiphp"
 
 static int debug;
@@ -379,6 +376,8 @@
 	slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
 	slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
 
+	acpiphp_slot->slot = slot;
+
 	make_slot_name(slot);
 
 	retval = pci_hp_register(slot->hotplug_slot);
@@ -387,10 +386,6 @@
 		goto error_name;
  	}
 
-	/* add slot to our internal list */
-	spin_lock(&list_lock);
-	list_add(&slot->slot_list, &slot_list);
-	spin_unlock(&list_lock);
 	info("Slot [%s] registered\n", slot->hotplug_slot->name);
 
 	return 0;
@@ -409,25 +404,14 @@
 
 void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
-	struct list_head *tmp, *n;
-	struct slot *slot;
+	struct slot *slot = acpiphp_slot->slot;
 	int retval = 0;
 
-	list_for_each_safe (tmp, n, &slot_list) {
-		/* memory will be freed in release_slot callback */
-		slot = list_entry(tmp, struct slot, slot_list);
-		if (slot->acpi_slot->sun != acpiphp_slot->sun)
-			continue;
-
-		spin_lock(&list_lock);
-		list_del(&slot->slot_list);
-		spin_unlock(&list_lock);
-		info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
-
-		retval = pci_hp_deregister(slot->hotplug_slot);
-		if (retval)
-			err("pci_hp_deregister failed with error %d\n", retval);
-	}
+	info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
+
+	retval = pci_hp_deregister(slot->hotplug_slot);
+	if (retval)
+		err("pci_hp_deregister failed with error %d\n", retval);
 }
 
 

[PATCH] acpiphp - slot management fix - V3

[MUNEDA Takahiro]

Hi,

At Tue, 14 Feb 2006 18:17:20 +0900,
Kenji Kaneshige <kaneshige.kenji@jp.fujitsu.com> wrote:
> 
> With your patch, I think this list_for_each_safe() loop becomes
> needless and it should be removed. In addition, slot_list is no
> longer needed.
> 
> Here is a sample patch against your patch. Note that I have not
> tested it very much.

Thank you for your comments. Your proposal looks good to me.
So I've merged your patch.
This patch is against for 2.6.16-rc3-mm1.
I tested it on my Tiger4 and it works well.

Thanks,
MUNE

o This patch removes IDs (for slots management).
o This patch removes the slot register/unregister processes
  from the init/exit phases. Instead, adds these processes
  in the bridge add/cleanup phases.
o Currently, this change doesn't have any meanings. But
  these changes are needed to support p2p bridge(with
  hotplug slot)

Signed-off-by: MUNEDA Takahiro <muneda.takahiro@jp.fujitsu.com>

 drivers/pci/hotplug/acpiphp.h      |    8 --
 drivers/pci/hotplug/acpiphp_core.c |  119 ++++++++++++++++---------------------
 drivers/pci/hotplug/acpiphp_glue.c |   48 +++++++-------
 3 files changed, 78 insertions(+), 97 deletions(-)

Index: linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp.h
===================================================================
--- linux-2.6.16-rc3-mm1.orig/drivers/pci/hotplug/acpiphp.h
+++ linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp.h
@@ -60,10 +60,7 @@ struct acpiphp_slot;
  * struct slot - slot information for each *physical* slot
  */
 struct slot {
-	u8 number;
 	struct hotplug_slot	*hotplug_slot;
-	struct list_head	slot_list;
-
 	struct acpiphp_slot	*acpi_slot;
 };
 
@@ -119,9 +116,9 @@ struct acpiphp_slot {
 	struct acpiphp_bridge *bridge;	/* parent */
 	struct list_head funcs;		/* one slot may have different
 					   objects (i.e. for each function) */
+	struct slot *slot;
 	struct mutex crit_sect;
 
-	u32		id;		/* slot id (serial #) for hotplug core */
 	u8		device;		/* pci device# */
 
 	u32		sun;		/* ACPI _SUN (slot unique number) */
@@ -229,12 +226,13 @@ struct acpiphp_dock_station {
 /* acpiphp_core.c */
 extern int acpiphp_register_attention(struct acpiphp_attention_info*info);
 extern int acpiphp_unregister_attention(struct acpiphp_attention_info *info);
+extern int acpiphp_register_hotplug_slot(struct acpiphp_slot *slot);
+extern void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *slot);
 
 /* acpiphp_glue.c */
 extern int acpiphp_glue_init (void);
 extern void acpiphp_glue_exit (void);
 extern int acpiphp_get_num_slots (void);
-extern struct acpiphp_slot *get_slot_from_id (int id);
 typedef int (*acpiphp_callback)(struct acpiphp_slot *slot, void *data);
 void handle_hotplug_event_func(acpi_handle, u32, void*);
 
Index: linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp_core.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/drivers/pci/hotplug/acpiphp_core.c
+++ linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp_core.c
@@ -44,8 +44,6 @@
 #include "pci_hotplug.h"
 #include "acpiphp.h"
 
-static LIST_HEAD(slot_list);
-
 #define MY_NAME	"acpiphp"
 
 static int debug;
@@ -341,62 +339,53 @@ static void release_slot(struct hotplug_
 	kfree(slot);
 }
 
-/**
- * init_slots - initialize 'struct slot' structures for each slot
- *
- */
-static int __init init_slots(void)
+/* callback routine to initialize 'struct slot' for each slot */
+int acpiphp_register_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
 	struct slot *slot;
+	struct hotplug_slot *hotplug_slot;
+	struct hotplug_slot_info *hotplug_slot_info;
 	int retval = -ENOMEM;
-	int i;
 
-	for (i = 0; i < num_slots; ++i) {
-		slot = kmalloc(sizeof(struct slot), GFP_KERNEL);
-		if (!slot)
-			goto error;
-		memset(slot, 0, sizeof(struct slot));
-
-		slot->hotplug_slot = kmalloc(sizeof(struct hotplug_slot), GFP_KERNEL);
-		if (!slot->hotplug_slot)
-			goto error_slot;
-		memset(slot->hotplug_slot, 0, sizeof(struct hotplug_slot));
-
-		slot->hotplug_slot->info = kmalloc(sizeof(struct hotplug_slot_info), GFP_KERNEL);
-		if (!slot->hotplug_slot->info)
-			goto error_hpslot;
-		memset(slot->hotplug_slot->info, 0, sizeof(struct hotplug_slot_info));
-
-		slot->hotplug_slot->name = kmalloc(SLOT_NAME_SIZE, GFP_KERNEL);
-		if (!slot->hotplug_slot->name)
-			goto error_info;
-
-		slot->number = i;
-
-		slot->hotplug_slot->private = slot;
-		slot->hotplug_slot->release = &release_slot;
-		slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
-
-		slot->acpi_slot = get_slot_from_id(i);
-		slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
-		slot->hotplug_slot->info->attention_status = 0;
-		slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
-		slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
-		slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
-		slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
-
-		make_slot_name(slot);
-
-		retval = pci_hp_register(slot->hotplug_slot);
-		if (retval) {
-			err("pci_hp_register failed with error %d\n", retval);
-			goto error_name;
-		}
-
-		/* add slot to our internal list */
-		list_add(&slot->slot_list, &slot_list);
-		info("Slot [%s] registered\n", slot->hotplug_slot->name);
-	}
+	slot = kzalloc(sizeof(*slot), GFP_KERNEL);
+	if (!slot)
+		goto error;
+
+	slot->hotplug_slot = kzalloc(sizeof(*hotplug_slot), GFP_KERNEL);
+	if (!slot->hotplug_slot)
+		goto error_slot;
+
+	slot->hotplug_slot->info = kzalloc(sizeof(*hotplug_slot_info),
+					   GFP_KERNEL);
+	if (!slot->hotplug_slot->info)
+		goto error_hpslot;
+
+	slot->hotplug_slot->name = kzalloc(SLOT_NAME_SIZE, GFP_KERNEL);
+	if (!slot->hotplug_slot->name)
+		goto error_info;
+
+	slot->hotplug_slot->private = slot;
+	slot->hotplug_slot->release = &release_slot;
+	slot->hotplug_slot->ops = &acpi_hotplug_slot_ops;
+
+	slot->acpi_slot = acpiphp_slot;
+	slot->hotplug_slot->info->power_status = acpiphp_get_power_status(slot->acpi_slot);
+	slot->hotplug_slot->info->attention_status = 0;
+	slot->hotplug_slot->info->latch_status = acpiphp_get_latch_status(slot->acpi_slot);
+	slot->hotplug_slot->info->adapter_status = acpiphp_get_adapter_status(slot->acpi_slot);
+	slot->hotplug_slot->info->max_bus_speed = PCI_SPEED_UNKNOWN;
+	slot->hotplug_slot->info->cur_bus_speed = PCI_SPEED_UNKNOWN;
+
+	acpiphp_slot->slot = slot;
+	make_slot_name(slot);
+
+	retval = pci_hp_register(slot->hotplug_slot);
+	if (retval) {
+		err("pci_hp_register failed with error %d\n", retval);
+		goto error_name;
+ 	}
+
+	info("Slot [%s] registered\n", slot->hotplug_slot->name);
 
 	return 0;
 error_name:
@@ -412,17 +401,16 @@ error:
 }
 
 
-static void __exit cleanup_slots (void)
+void acpiphp_unregister_hotplug_slot(struct acpiphp_slot *acpiphp_slot)
 {
-	struct list_head *tmp, *n;
-	struct slot *slot;
+	struct slot *slot = acpiphp_slot->slot;
+	int retval = 0;
 
-	list_for_each_safe (tmp, n, &slot_list) {
-		/* memory will be freed in release_slot callback */
-		slot = list_entry(tmp, struct slot, slot_list);
-		list_del(&slot->slot_list);
-		pci_hp_deregister(slot->hotplug_slot);
-	}
+	info ("Slot [%s] unregistered\n", slot->hotplug_slot->name);
+
+	retval = pci_hp_deregister(slot->hotplug_slot);
+	if (retval)
+		err("pci_hp_deregister failed with error %d\n", retval);
 }
 
 
@@ -439,16 +427,13 @@ static int __init acpiphp_init(void)
 
 	/* read all the ACPI info from the system */
 	retval = init_acpi();
-	if (retval && !(docking_station))
-		return retval;
 
-	return init_slots();
+	return retval;
 }
 
 
 static void __exit acpiphp_exit(void)
 {
-	cleanup_slots();
 	/* deallocate internal data structures etc. */
 	acpiphp_glue_exit();
 
Index: linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp_glue.c
===================================================================
--- linux-2.6.16-rc3-mm1.orig/drivers/pci/hotplug/acpiphp_glue.c
+++ linux-2.6.16-rc3-mm1/drivers/pci/hotplug/acpiphp_glue.c
@@ -128,8 +128,7 @@ register_slot(acpi_handle handle, u32 lv
 	acpi_handle tmp;
 	acpi_status status = AE_OK;
 	unsigned long adr, sun;
-	int device, function;
-	static int num_slots = 0;	/* XXX if we support I/O node hotplug... */
+	int device, function, retval;
 
 	status = acpi_evaluate_integer(handle, "_ADR", NULL, &adr);
 
@@ -198,7 +197,6 @@ register_slot(acpi_handle handle, u32 lv
 
 		memset(slot, 0, sizeof(struct acpiphp_slot));
 		slot->bridge = bridge;
-		slot->id = num_slots++;
 		slot->device = device;
 		slot->sun = sun;
 		INIT_LIST_HEAD(&slot->funcs);
@@ -212,6 +210,11 @@ register_slot(acpi_handle handle, u32 lv
 		dbg("found ACPI PCI Hotplug slot %d at PCI %04x:%02x:%02x\n",
 				slot->sun, pci_domain_nr(bridge->pci_bus),
 				bridge->pci_bus->number, slot->device);
+		retval = acpiphp_register_hotplug_slot(slot);
+		if (retval) {
+			warn("acpiphp_register_hotplug_slot failed(err code = 0x%x)\n", retval);
+			goto err_exit;
+		}
 	}
 
 	newfunc->slot = slot;
@@ -246,6 +249,14 @@ register_slot(acpi_handle handle, u32 lv
 	}
 
 	return status;
+
+ err_exit:
+	bridge->nr_slots--;
+	bridge->slots = slot->next;
+	kfree(slot);
+	kfree(newfunc);
+
+	return AE_OK;
 }
 
 
@@ -328,9 +339,16 @@ static void init_bridge_misc(struct acpi
 	/* decode ACPI 2.0 _HPP (hot plug parameters) */
 	decode_hpp(bridge);
 
+	/* must be added to the list prior to calling register_slot */
+	list_add(&bridge->list, &bridge_list);
+
 	/* register all slot objects under this bridge */
 	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, bridge->handle, (u32)1,
 				     register_slot, bridge, NULL);
+	if (ACPI_FAILURE(status)) {
+		list_del(&bridge->list);
+		return;
+	}
 
 	/* install notify handler */
 	if (bridge->type != BRIDGE_TYPE_HOST) {
@@ -343,8 +361,6 @@ static void init_bridge_misc(struct acpi
 			err("failed to register interrupt notify handler\n");
 		}
 	}
-
-	list_add(&bridge->list, &bridge_list);
 }
 
 
@@ -548,6 +564,8 @@ static void cleanup_bridge(struct acpiph
 			list_del(list);
 			kfree(func);
 		}
+		acpiphp_unregister_hotplug_slot(slot);
+		list_del(&slot->funcs);
 		kfree(slot);
 		slot = next;
 	}
@@ -1400,26 +1418,6 @@ static int acpiphp_for_each_slot(acpiphp
 }
 #endif
 
-/* search matching slot from id  */
-struct acpiphp_slot *get_slot_from_id(int id)
-{
-	struct list_head *node;
-	struct acpiphp_bridge *bridge;
-	struct acpiphp_slot *slot;
-
-	list_for_each (node, &bridge_list) {
-		bridge = (struct acpiphp_bridge *)node;
-		for (slot = bridge->slots; slot; slot = slot->next)
-			if (slot->id == id)
-				return slot;
-	}
-
-	/* should never happen! */
-	err("%s: no object for id %d\n", __FUNCTION__, id);
-	WARN_ON(1);
-	return NULL;
-}
-
 
 /**
  * acpiphp_enable_slot - power on slot