[PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Tang Chen]

Hi Yinghai,

List acpi_root_bridge_list is only updated when kernel is booting,
or in _handle_hotplug_event_root() when handling ACPI_NOTIFY_DEVICE_CHECK
event on a pci root bridge device. But when we hotplug a container, which
contains one or more pci root bridges, container_notify_cb() will be
called but not _handle_hotplug_event_root(). As a result,
acpi_root_bridge_list won't be updated.

This patch makes the following api and struct public in pci_root_hp.h,
	struct acpi_root_bridge;
	add_acpi_root_bridge()
	remove_acpi_root_bridge()
	acpi_root_handle_to_bridge()
and call add_acpi_root_bridge() in acpi_bus_check_add() and call
remove_acpi_root_bridge() in acpi_bus_remove().

This patch is based on Lu Yinghai's git tree branch for-pci-split-pci-root-hp-2.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/pci_root_hp.c |   20 ++++++--------------
 drivers/acpi/scan.c        |   18 ++++++++++++++++--
 include/acpi/pci_root_hp.h |   13 +++++++++++++
 3 files changed, 35 insertions(+), 16 deletions(-)
 create mode 100644 include/acpi/pci_root_hp.h

diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
index 01e71f6..6381a26 100644
--- a/drivers/acpi/pci_root_hp.c
+++ b/drivers/acpi/pci_root_hp.c
@@ -31,7 +31,7 @@ static const struct acpi_device_id root_device_ids[] = {
 
 #define ACPI_STA_FUNCTIONING	(0x00000008)
 
-static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
+struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
 {
 	struct acpi_root_bridge *bridge;
 
@@ -43,7 +43,7 @@ static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
 }
 
 /* allocate and initialize host bridge data structure */
-static void add_acpi_root_bridge(acpi_handle handle)
+void add_acpi_root_bridge(acpi_handle handle)
 {
 	struct acpi_root_bridge *bridge;
 	acpi_handle dummy_handle;
@@ -79,7 +79,7 @@ static void add_acpi_root_bridge(acpi_handle handle)
 	list_add(&bridge->list, &acpi_root_bridge_list);
 }
 
-static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
+void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
 {
 	list_del(&bridge->list);
 	kfree(bridge);
@@ -172,10 +172,8 @@ static void handle_root_bridge_removal(acpi_handle handle,
 	u32 flags = 0;
 	struct acpi_device *device;
 
-	if (bridge) {
+	if (bridge)
 		flags = bridge->flags;
-		remove_acpi_root_bridge(bridge);
-	}
 
 	if (!acpi_bus_get_device(handle, &device)) {
 		int ret_val;
@@ -223,10 +221,8 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		/* bus enumerate */
 		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!bridge)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 
 		break;
 
@@ -234,10 +230,8 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		/* device check */
 		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!bridge)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST:
@@ -304,8 +298,6 @@ find_root_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 		printk(KERN_DEBUG "acpi root: %s notify handler is installed\n",
 				 objname);
 
-	add_acpi_root_bridge(handle);
-
 	return AE_OK;
 }
 
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 6ca2eaf..c258064 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -12,6 +12,7 @@
 #include <linux/dmi.h>
 
 #include <acpi/acpi_drivers.h>
+#include <acpi/pci_root_hp.h>
 
 #include "internal.h"
 
@@ -1265,8 +1266,17 @@ int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
 	dev->removal_type = ACPI_BUS_REMOVAL_EJECT;
 	device_release_driver(&dev->dev);
 
-	if (rmdevice)
+	if (rmdevice) {
+		if (acpi_is_root_bridge(dev->handle)) {
+			struct acpi_root_bridge *bridge;
+
+			bridge = acpi_root_handle_to_bridge(dev->handle);
+			if (bridge)
+				remove_acpi_root_bridge(bridge);
+		}
+
 		acpi_device_unregister(dev, ACPI_BUS_REMOVAL_EJECT);
+	}
 
 	return 0;
 }
@@ -1448,9 +1458,13 @@ static acpi_status acpi_bus_check_add(acpi_handle handle, u32 lvl,
 	 */
 	device = NULL;
 	acpi_bus_get_device(handle, &device);
-	if (ops->acpi_op_add && !device)
+	if (ops->acpi_op_add && !device) {
 		acpi_add_single_object(&device, handle, type, sta, ops);
 
+		if (acpi_is_root_bridge(handle))
+			add_acpi_root_bridge(handle);
+	}
+
 	if (!device)
 		return AE_CTRL_DEPTH;
 
diff --git a/include/acpi/pci_root_hp.h b/include/acpi/pci_root_hp.h
new file mode 100644
index 0000000..2761add
--- /dev/null
+++ b/include/acpi/pci_root_hp.h
@@ -0,0 +1,13 @@
+
+/*PCI root bridge hotplug API */
+
+#ifndef __PCI_ROOT_HP__
+#define __PCI_ROOT_HP__
+
+struct acpi_root_bridge;
+
+void add_acpi_root_bridge(acpi_handle handle);
+void remove_acpi_root_bridge(struct acpi_root_bridge *bridge);
+struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle);
+
+#endif				/* __PCI_ROOT_HP_H__ */
-- 
1.7.1

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 2068 bytes --]

On Tue, Oct 16, 2012 at 8:25 PM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> Hi Yinghai,
>
> List acpi_root_bridge_list is only updated when kernel is booting,
> or in _handle_hotplug_event_root() when handling ACPI_NOTIFY_DEVICE_CHECK
> event on a pci root bridge device. But when we hotplug a container, which
> contains one or more pci root bridges, container_notify_cb() will be
> called but not _handle_hotplug_event_root(). As a result,
> acpi_root_bridge_list won't be updated.
>
> This patch makes the following api and struct public in pci_root_hp.h,
>         struct acpi_root_bridge;
>         add_acpi_root_bridge()
>         remove_acpi_root_bridge()
>         acpi_root_handle_to_bridge()
> and call add_acpi_root_bridge() in acpi_bus_check_add() and call
> remove_acpi_root_bridge() in acpi_bus_remove().
>
> This patch is based on Lu Yinghai's git tree branch for-pci-split-pci-root-hp-2.
>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 6ca2eaf..c258064 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -12,6 +12,7 @@
>  #include <linux/dmi.h>
>
>  #include <acpi/acpi_drivers.h>
> +#include <acpi/pci_root_hp.h>
>
>  #include "internal.h"
>
> @@ -1265,8 +1266,17 @@ int acpi_bus_remove(struct acpi_device *dev, int rmdevice)
>         dev->removal_type = ACPI_BUS_REMOVAL_EJECT;
>         device_release_driver(&dev->dev);
>
> -       if (rmdevice)
> +       if (rmdevice) {
> +               if (acpi_is_root_bridge(dev->handle)) {
> +                       struct acpi_root_bridge *bridge;
> +
> +                       bridge = acpi_root_handle_to_bridge(dev->handle);
> +                       if (bridge)
> +                               remove_acpi_root_bridge(bridge);
> +               }
> +
>                 acpi_device_unregister(dev, ACPI_BUS_REMOVAL_EJECT);
> +       }

no, we don't need that.

after closely looking, it seems we can dump acpi_root_bridge.

please check if attached patch could work with container path.

Thanks

Yinghai

[-- Attachment #2: kill_remove_acpi_root_bridge.patch --]
[-- Type: application/octet-stream, Size: 5666 bytes --]

Subject: [PATCH] PCI, ACPI: remove acpi_root_bridge in pci_root_hp

Tang noticed that hotplug through container will not update acpi_root_bridge
list.

After closely checking, we don't need that for struct for tracking and
could use acpi_pci_root directly.

Reported-by: Tang Chen <tangchen@cn.fujitsu.com>
Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root_hp.c |  111 ++++++++-------------------------------------
 1 file changed, 21 insertions(+), 90 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root_hp.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root_hp.c
+++ linux-2.6/drivers/acpi/pci_root_hp.c
@@ -12,13 +12,6 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 
-static LIST_HEAD(acpi_root_bridge_list);
-struct acpi_root_bridge {
-	struct list_head list;
-	acpi_handle handle;
-	u32 flags;
-};
-
 static const struct acpi_device_id root_device_ids[] = {
 	{"PNP0A03", 0},
 	{"PNP0A08", 0},
@@ -31,60 +24,6 @@ static const struct acpi_device_id root_
 
 #define ACPI_STA_FUNCTIONING	(0x00000008)
 
-static struct acpi_root_bridge *acpi_root_handle_to_bridge(acpi_handle handle)
-{
-	struct acpi_root_bridge *bridge;
-
-	list_for_each_entry(bridge, &acpi_root_bridge_list, list)
-		if (bridge->handle == handle)
-			return bridge;
-
-	return NULL;
-}
-
-/* allocate and initialize host bridge data structure */
-static void add_acpi_root_bridge(acpi_handle handle)
-{
-	struct acpi_root_bridge *bridge;
-	acpi_handle dummy_handle;
-	acpi_status status;
-
-	/* if the bridge doesn't have _STA, we assume it is always there */
-	status = acpi_get_handle(handle, "_STA", &dummy_handle);
-	if (ACPI_SUCCESS(status)) {
-		unsigned long long tmp;
-
-		status = acpi_evaluate_integer(handle, "_STA", NULL, &tmp);
-		if (ACPI_FAILURE(status)) {
-			printk(KERN_DEBUG "%s: _STA evaluation failure\n",
-						 __func__);
-			return;
-		}
-		if ((tmp & ACPI_STA_FUNCTIONING) == 0)
-			/* don't register this object */
-			return;
-	}
-
-	bridge = kzalloc(sizeof(struct acpi_root_bridge), GFP_KERNEL);
-	if (!bridge)
-		return;
-
-	bridge->handle = handle;
-
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
-		bridge->flags |= ROOT_BRIDGE_HAS_EJ0;
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
-		bridge->flags |= ROOT_BRIDGE_HAS_PS3;
-
-	list_add(&bridge->list, &acpi_root_bridge_list);
-}
-
-static void remove_acpi_root_bridge(struct acpi_root_bridge *bridge)
-{
-	list_del(&bridge->list);
-	kfree(bridge);
-}
-
 struct acpi_root_hp_work {
 	struct work_struct work;
 	acpi_handle handle;
@@ -166,28 +105,25 @@ static int acpi_root_evaluate_object(acp
 	return 0;
 }
 
-static void handle_root_bridge_removal(acpi_handle handle,
-		 struct acpi_root_bridge *bridge)
+static void handle_root_bridge_removal(struct acpi_device *device)
 {
+	int ret_val;
 	u32 flags = 0;
-	struct acpi_device *device;
-
-	if (bridge) {
-		flags = bridge->flags;
-		remove_acpi_root_bridge(bridge);
-	}
-
-	if (!acpi_bus_get_device(handle, &device)) {
-		int ret_val;
+	acpi_handle dummy_handle;
+	acpi_handle handle = device->handle;
 
-		/* remove pci devices at first */
-		ret_val = acpi_bus_trim(device, 0);
-		printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
+		flags |= ROOT_BRIDGE_HAS_EJ0;
+	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
+		flags |= ROOT_BRIDGE_HAS_PS3;
 
-		/* remove acpi devices */
-		ret_val = acpi_bus_trim(device, 1);
-		printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
-	}
+	/* remove pci devices at first */
+	ret_val = acpi_bus_trim(device, 0);
+	printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
+
+	/* remove acpi devices */
+	ret_val = acpi_bus_trim(device, 1);
+	printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
 
 	if (flags & ROOT_BRIDGE_HAS_PS3) {
 		acpi_status status;
@@ -202,7 +138,7 @@ static void handle_root_bridge_removal(a
 
 static void _handle_hotplug_event_root(struct work_struct *work)
 {
-	struct acpi_root_bridge *bridge;
+	struct acpi_pci_root *root;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
@@ -214,7 +150,7 @@ static void _handle_hotplug_event_root(s
 	handle = hp_work->handle;
 	type = hp_work->type;
 
-	bridge = acpi_root_handle_to_bridge(handle);
+	root = acpi_pci_find_root(handle);
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
@@ -223,10 +159,8 @@ static void _handle_hotplug_event_root(s
 		/* bus enumerate */
 		printk(KERN_DEBUG "%s: Bus check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!root)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 
 		break;
 
@@ -234,17 +168,16 @@ static void _handle_hotplug_event_root(s
 		/* device check */
 		printk(KERN_DEBUG "%s: Device check notify on %s\n", __func__,
 				 objname);
-		if (!bridge) {
+		if (!root)
 			handle_root_bridge_insertion(handle);
-			add_acpi_root_bridge(handle);
-		}
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST:
 		/* request device eject */
 		printk(KERN_DEBUG "%s: Device eject notify on %s\n", __func__,
 				 objname);
-		handle_root_bridge_removal(handle, bridge);
+		if (root)
+			handle_root_bridge_removal(root->device);
 		break;
 	default:
 		printk(KERN_WARNING "notify_handler: unknown event type 0x%x for %s\n",
@@ -304,8 +237,6 @@ find_root_bridges(acpi_handle handle, u3
 		printk(KERN_DEBUG "acpi root: %s notify handler is installed\n",
 				 objname);
 
-	add_acpi_root_bridge(handle);
-
 	return AE_OK;
 }
 

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Tang Chen]

On 10/17/2012 01:18 PM, Yinghai Lu wrote:
>
> no, we don't need that.
>
> after closely looking, it seems we can dump acpi_root_bridge.
>
> please check if attached patch could work with container path.

Hi Yinghai,

Your patch seems working well.

BTW, I actually found that the two lists, acpi_root_bridge and
acpi_pci_roots in drivers/acpi/pci_root.c were similar, except
your acpi_root_bridge included PNP0A08.

In the beginning, I thought you would use it in some other situations,
and now it is clear that it can be removed.
Thanks. :)

And also, I have another 2 questions, maybe you can help me.
1) Do we need to put PNP0A08 into acpi_pci_roots ?
2) In container_notify_cb(), when it got a ACPI_NOTIFY_EJECT_REQUEST
    event, it doesn't do the hot-remove things.
    I use your sci emulator patch to test it. I did the following thing:
	echo echo "\_SB_.LSB1" > /sys/kernel/debug/acpi/sci_notify
    where \_SB_.LSB1 is a container, it just did nothing.
    Do we need to support this operation ?

Thanks. :)

>
> Thanks
>
> Yinghai

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Tang Chen]

On 10/17/2012 03:39 PM, Tang Chen wrote:
> On 10/17/2012 01:18 PM, Yinghai Lu wrote:
>>
>> no, we don't need that.
>>
>> after closely looking, it seems we can dump acpi_root_bridge.
>>
>> please check if attached patch could work with container path.
>
> Hi Yinghai,
>
> Your patch seems working well.
>
> BTW, I actually found that the two lists, acpi_root_bridge and
> acpi_pci_roots in drivers/acpi/pci_root.c were similar, except
> your acpi_root_bridge included PNP0A08.
>
> In the beginning, I thought you would use it in some other situations,
> and now it is clear that it can be removed.
> Thanks. :)
>
> And also, I have another 2 questions, maybe you can help me.
> 1) Do we need to put PNP0A08 into acpi_pci_roots ?
> 2) In container_notify_cb(), when it got a ACPI_NOTIFY_EJECT_REQUEST
> event, it doesn't do the hot-remove things.
> I use your sci emulator patch to test it. I did the following thing:
> echo echo "\_SB_.LSB1" > /sys/kernel/debug/acpi/sci_notify
	==> echo "\_SB_.LSB1 3" > /sys/kernel/debug/acpi/sci_notify

> where \_SB_.LSB1 is a container, it just did nothing.
> Do we need to support this operation ?
>
> Thanks. :)
>
>>
>> Thanks
>>
>> Yinghai
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Yinghai Lu]

On Wed, Oct 17, 2012 at 12:39 AM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> On 10/17/2012 01:18 PM, Yinghai Lu wrote:
>
> And also, I have another 2 questions, maybe you can help me.
> 1) Do we need to put PNP0A08 into acpi_pci_roots ?

looks like we need to unify those two ids.

> 2) In container_notify_cb(), when it got a ACPI_NOTIFY_EJECT_REQUEST
>    event, it doesn't do the hot-remove things.
>    I use your sci emulator patch to test it. I did the following thing:
>         echo echo "\_SB_.LSB1" > /sys/kernel/debug/acpi/sci_notify
>    where \_SB_.LSB1 is a container, it just did nothing.
>    Do we need to support this operation ?

yes, looks like need to add container_device_remove and call it under
 container_notify_cb/ACPI_NOTIFY_EJECT_REQUEST

and should look like handle_root_bridge_removal to call acpi_bus_trim two times.

Thanks

Yinghai

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Tang Chen]

On 10/18/2012 12:28 AM, Yinghai Lu wrote:
> On Wed, Oct 17, 2012 at 12:39 AM, Tang Chen<tangchen@cn.fujitsu.com>  wrote:
>> On 10/17/2012 01:18 PM, Yinghai Lu wrote:
>>
>> And also, I have another 2 questions, maybe you can help me.
>> 1) Do we need to put PNP0A08 into acpi_pci_roots ?
>
> looks like we need to unify those two ids.
>
>> 2) In container_notify_cb(), when it got a ACPI_NOTIFY_EJECT_REQUEST
>>     event, it doesn't do the hot-remove things.
>>     I use your sci emulator patch to test it. I did the following thing:
>>          echo echo "\_SB_.LSB1">  /sys/kernel/debug/acpi/sci_notify
>>     where \_SB_.LSB1 is a container, it just did nothing.
>>     Do we need to support this operation ?
>
> yes, looks like need to add container_device_remove and call it under
>   container_notify_cb/ACPI_NOTIFY_EJECT_REQUEST
>
> and should look like handle_root_bridge_removal to call acpi_bus_trim two times.

Hi Yinghai,

OK, I can do that. :)
And I'll send patches for that soon. :)

>
> Thanks
>
> Yinghai
>

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Tang Chen]

On 10/18/2012 12:28 AM, Yinghai Lu wrote:
> On Wed, Oct 17, 2012 at 12:39 AM, Tang Chen<tangchen@cn.fujitsu.com>  wrote:
>> On 10/17/2012 01:18 PM, Yinghai Lu wrote:
>>
>> And also, I have another 2 questions, maybe you can help me.
>> 1) Do we need to put PNP0A08 into acpi_pci_roots ?
>
> looks like we need to unify those two ids.
>
>> 2) In container_notify_cb(), when it got a ACPI_NOTIFY_EJECT_REQUEST
>>     event, it doesn't do the hot-remove things.
>>     I use your sci emulator patch to test it. I did the following thing:
>>          echo echo "\_SB_.LSB1">  /sys/kernel/debug/acpi/sci_notify
>>     where \_SB_.LSB1 is a container, it just did nothing.
>>     Do we need to support this operation ?
>
> yes, looks like need to add container_device_remove and call it under
>   container_notify_cb/ACPI_NOTIFY_EJECT_REQUEST
>
> and should look like handle_root_bridge_removal to call acpi_bus_trim two times.

Hi Yinghai,

You said the following in another patch:
"[PATCH 27/40] ACPI: acpi_bus_trim to support two steps."

 > For root bus hotremove support, we need to have pci device removed
 > before acpi devices.
 >
 > So try to keep all acpi devices, and only stop drivers with them.

Is it just container and pci_root_bridge hot-remove need to call
acpi_bus_trim() twice ? For normal device without sub-device, I think
it is OK to call acpi_bus_trim(device, 1).

The reason why I'm asking this question is:

I saw in acpi_bus_hot_remove_device(), it almost does the same things
you did in handle_root_bridge_removal(), except calling acpi_bus_trim()
twice. And there are more than one path could do container hot-remove.

If I add a container_device_remove() doing the similar things, it could
be duplicated. So, shall we just remove handle_root_bridge_removal(),
and only use acpi_bus_hot_remove_device() ?

Of course, we need to call acpi_bus_trim() twice in
acpi_bus_hot_remove_device().

Thanks. :)

>
> Thanks
>
> Yinghai
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 1/1] Update acpi_root_bridge_list in container hotplug path.

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

On Fri, Oct 19, 2012 at 1:49 AM, Tang Chen <tangchen@cn.fujitsu.com> wrote:

> Is it just container and pci_root_bridge hot-remove need to call
> acpi_bus_trim() twice ? For normal device without sub-device, I think
> it is OK to call acpi_bus_trim(device, 1).
>
> The reason why I'm asking this question is:
>
> I saw in acpi_bus_hot_remove_device(), it almost does the same things
> you did in handle_root_bridge_removal(), except calling acpi_bus_trim()
> twice. And there are more than one path could do container hot-remove.
>
> If I add a container_device_remove() doing the similar things, it could
> be duplicated. So, shall we just remove handle_root_bridge_removal(),
> and only use acpi_bus_hot_remove_device() ?
>
> Of course, we need to call acpi_bus_trim() twice in
> acpi_bus_hot_remove_device().

yes.  we could use acpi_bus_hot_remove_device to simply
acpi_bus_hot_remove_device()

but that eject_event should have device instead of passing handle, and
later check if that device is there or not.

like attached two patches.

[-- Attachment #2: root_bridge_remove_1.patch --]
[-- Type: application/octet-stream, Size: 2197 bytes --]

Subject: [PATCH] ACPI: update ej_event interface to take acpi_device

Should use acpi_device pointer directly instead of use handle and
get the device pointer again.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/scan.c     |   14 ++++----------
 include/acpi/acpi_bus.h |    2 +-
 2 files changed, 5 insertions(+), 11 deletions(-)

Index: linux-2.6/drivers/acpi/scan.c
===================================================================
--- linux-2.6.orig/drivers/acpi/scan.c
+++ linux-2.6/drivers/acpi/scan.c
@@ -95,19 +95,13 @@ static DEVICE_ATTR(modalias, 0444, acpi_
 void acpi_bus_hot_remove_device(void *context)
 {
 	struct acpi_eject_event *ej_event = (struct acpi_eject_event *) context;
-	struct acpi_device *device;
-	acpi_handle handle = ej_event->handle;
+	struct acpi_device *device = ej_event->device;
+	acpi_handle handle = device->handle;
 	struct acpi_object_list arg_list;
 	union acpi_object arg;
 	acpi_status status = AE_OK;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 
-	if (acpi_bus_get_device(handle, &device))
-		goto err_out;
-
-	if (!device)
-		goto err_out;
-
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 		"Hot-removing device %s...\n", dev_name(&device->dev)));
 
@@ -189,7 +183,7 @@ acpi_eject_store(struct device *d, struc
 		goto err;
 	}
 
-	ej_event->handle = acpi_device->handle;
+	ej_event->device = acpi_device;
 	if (acpi_device->flags.eject_pending) {
 		/* event originated from ACPI eject notification */
 		ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
@@ -197,7 +191,7 @@ acpi_eject_store(struct device *d, struc
 	} else {
 		/* event originated from user */
 		ej_event->event = ACPI_OST_EC_OSPM_EJECT;
-		(void) acpi_evaluate_hotplug_ost(ej_event->handle,
+		(void) acpi_evaluate_hotplug_ost(acpi_device->handle,
 			ej_event->event, ACPI_OST_SC_EJECT_IN_PROGRESS, NULL);
 	}
 
Index: linux-2.6/include/acpi/acpi_bus.h
===================================================================
--- linux-2.6.orig/include/acpi/acpi_bus.h
+++ linux-2.6/include/acpi/acpi_bus.h
@@ -304,7 +304,7 @@ struct acpi_bus_event {
 };
 
 struct acpi_eject_event {
-	acpi_handle	handle;
+	struct acpi_device	*device;
 	u32		event;
 };
 

[-- Attachment #3: root_bridge_remove_2.patch --]
[-- Type: application/octet-stream, Size: 2519 bytes --]

Subject: [PATCH] ACPI, PCI: Simplify handle_root_bridge_removal()

Tang Chen found handle_root_bridge_removal is very similiar to
acpi_bus_hot_remove_device().
Only difference is that it call trim two times.

Change to handle_root_bridge_removal to call trim one time and then 
use acpi_bus_hot_remove_device. 

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/acpi/pci_root_hp.c |   51 +++++++--------------------------------------
 1 file changed, 9 insertions(+), 42 deletions(-)

Index: linux-2.6/drivers/acpi/pci_root_hp.c
===================================================================
--- linux-2.6.orig/drivers/acpi/pci_root_hp.c
+++ linux-2.6/drivers/acpi/pci_root_hp.c
@@ -56,56 +56,23 @@ static void handle_root_bridge_insertion
 		printk(KERN_ERR "cannot start bridge\n");
 }
 
-static int acpi_root_evaluate_object(acpi_handle handle, char *cmd, int val)
-{
-	acpi_status status;
-	struct acpi_object_list arg_list;
-	union acpi_object arg;
-
-	arg_list.count = 1;
-	arg_list.pointer = &arg;
-	arg.type = ACPI_TYPE_INTEGER;
-	arg.integer.value = val;
-
-	status = acpi_evaluate_object(handle, cmd, &arg_list, NULL);
-	if (ACPI_FAILURE(status)) {
-		pr_warn("%s: %s to %d failed\n",
-				 __func__, cmd, val);
-		return -1;
-	}
-
-	return 0;
-}
-
 static void handle_root_bridge_removal(struct acpi_device *device)
 {
 	int ret_val;
-	u32 flags = 0;
-	acpi_handle dummy_handle;
-	acpi_handle handle = device->handle;
-
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_EJ0", &dummy_handle)))
-		flags |= ROOT_BRIDGE_HAS_EJ0;
-	if (ACPI_SUCCESS(acpi_get_handle(handle, "_PS3", &dummy_handle)))
-		flags |= ROOT_BRIDGE_HAS_PS3;
+	struct acpi_eject_event *ej_event;
+
+	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+	if (!ej_event)
+		return;
+
+	ej_event->device = device;
+	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
 
 	/* remove pci devices at first */
 	ret_val = acpi_bus_trim(device, 0);
 	printk(KERN_DEBUG "acpi_bus_trim stop return %x\n", ret_val);
 
-	/* remove acpi devices */
-	ret_val = acpi_bus_trim(device, 1);
-	printk(KERN_DEBUG "acpi_bus_trim remove return %x\n", ret_val);
-
-	if (flags & ROOT_BRIDGE_HAS_PS3) {
-		acpi_status status;
-
-		status = acpi_evaluate_object(handle, "_PS3", NULL, NULL);
-		if (ACPI_FAILURE(status))
-			pr_warn("%s: _PS3 failed\n", __func__);
-	}
-	if (flags & ROOT_BRIDGE_HAS_EJ0)
-		acpi_root_evaluate_object(handle, "_EJ0", 1);
+	acpi_bus_hot_remove_device(ej_event);
 }
 
 static void _handle_hotplug_event_root(struct work_struct *work)