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

login
register
mail settings
Submitter Tang Chen
Date Oct. 17, 2012, 3:25 a.m.
Message ID <1350444308-27102-1-git-send-email-tangchen@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/191949/
State Not Applicable
Headers show

Comments

Tang Chen - Oct. 17, 2012, 3:25 a.m.
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
Yinghai Lu - Oct. 17, 2012, 5:18 a.m.
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
Tang Chen - Oct. 17, 2012, 7:39 a.m.
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tang Chen - Oct. 17, 2012, 7:42 a.m.
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/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Oct. 17, 2012, 4:28 p.m.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tang Chen - Oct. 18, 2012, 1 a.m.
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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tang Chen - Oct. 19, 2012, 8:49 a.m.
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/
>

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu - Oct. 19, 2012, 8:36 p.m.
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.

Patch

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__ */