[PATCH v3 0/3] ACPI: container hot remove support.

[PATCH v3 0/3] ACPI: container hot remove support.

[Tang Chen]

Hi,

The container hotplug handler container_notify_cb() didn't implement
the hot-remove functionality. So, these 3 patches implement it like
the following way:

patch 1. Do not use kacpid_wq/kacpid_notify_wq to handle container hotplug event,
         use kacpi_hotplug_wq instead to avoid deadlock.
         Doing this is to reuse acpi_bus_hot_remove_device() in container
         hot-remove handling.

patch 2. Introduce a new function container_device_remove() to handle
         ACPI_NOTIFY_EJECT_REQUEST event for container.


change log v2 -> v3:

1. Add 1 patch(patch1). As Toshi Kan mentioned, acpi_os_hotplug_execute() is already
   kernel. So use it instead of alloc_acpi_hp_work() to add hotplug job onto kacpi_hotplug_wq.

2. In patch3: Print caller's function name when container_device_remove() fails to help to debug.

3. In patch3: Add commit message to describ why we need to call acpi_bus_trim() twice when
   removing devices.

change log v1 -> v2:

1. In patch1: Based on the lastest for-pci-split-pci-root-hp-2 branch from Lu Yinghai, 
   use alloc_acpi_hp_work() to add container hotplug work into kacpi_hotplug_wq.

2. In patch2: Allocate ej_event after container is stopped, so that we don't need to
   kfree the ej_event if stopping container failed.


This is based on Lu Yinghai's job.
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2


Tang Chen (3):
  Use acpi_os_hotplug_execute() instead of alloc_acpi_hp_work().
  Use kacpi_hotplug_wq to handle container hotplug event.
  Improve container_notify_cb() to support container hot-remove.

 drivers/acpi/container.c           |   95 +++++++++++++++++++++++++++++++-----
 drivers/acpi/osl.c                 |   28 +++++-----
 drivers/acpi/pci_root_hp.c         |   25 ++++++---
 drivers/pci/hotplug/acpiphp_glue.c |   39 ++++++++-------
 include/acpi/acpiosxf.h            |    7 +--
 5 files changed, 137 insertions(+), 57 deletions(-)

[PATCH v3 1/3] Use acpi_os_hotplug_execute() instead of alloc_acpi_hp_work().

[Tang Chen]

Hi Yinghai,

alloc_acpi_hp_work() just puts the hutplug work onto kacpi_hotplug_wq.
As mentioned by Toshi Kani, this job has been done in acpi_os_hotplug_execute().
So we should use it instead of alloc_acpi_hp_work().

This patch adds a acpi_hp_cb_data struct, which encapsulates the hotplug
event notifier's parameters:
	struct acpi_hp_cb_data {
		acpi_handle handle;
		u32 type;
		void *context;
	};

And also a function alloc_acpi_hp_work(), which calls acpi_os_hotplug_execute()
to put the hotplug job onto kacpi_hotplug_wq.

This patch is based on Lu Yinghai's tree:
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/osl.c                 |   28 ++++++++++++------------
 drivers/acpi/pci_root_hp.c         |   25 +++++++++++++++-------
 drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++----------------
 include/acpi/acpiosxf.h            |    7 ++---
 4 files changed, 55 insertions(+), 44 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 311a921..d441b16 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -52,6 +52,7 @@
 #include <acpi/acpi.h>
 #include <acpi/acpi_bus.h>
 #include <acpi/processor.h>
+#include <acpi/acpiosxf.h>
 
 #define _COMPONENT		ACPI_OS_SERVICES
 ACPI_MODULE_NAME("osl");
@@ -1592,23 +1593,22 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
 	__acpi_os_prepare_sleep = func;
 }
 
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work))
+void acpi_hp_cb_execute(acpi_handle handle, u32 type, void *context,
+			acpi_osd_exec_callback function)
 {
-	struct acpi_hp_work *hp_work;
-	int ret;
+	acpi_status status;
+	struct acpi_hp_cb_data *cb_data;
 
-	hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
-	if (!hp_work)
+	cb_data = kmalloc(sizeof(struct acpi_hp_cb_data), GFP_KERNEL);
+	if (!cb_data)
 		return;
 
-	hp_work->handle = handle;
-	hp_work->type = type;
-	hp_work->context = context;
+	cb_data->handle = handle;
+	cb_data->type = type;
+	cb_data->context = context;
 
-	INIT_WORK(&hp_work->work, func);
-	ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
-	if (!ret)
-		kfree(hp_work);
+	status = acpi_os_hotplug_execute(function, cb_data);
+	if (ACPI_FAILURE(status))
+		kfree(cb_data);
 }
-EXPORT_SYMBOL(alloc_acpi_hp_work);
+EXPORT_SYMBOL(acpi_hp_cb_execute);
diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
index 7d427e6..2ff83f4 100644
--- a/drivers/acpi/pci_root_hp.c
+++ b/drivers/acpi/pci_root_hp.c
@@ -75,19 +75,20 @@ static void handle_root_bridge_removal(struct acpi_device *device)
 	acpi_bus_hot_remove_device(ej_event);
 }
 
-static void _handle_hotplug_event_root(struct work_struct *work)
+/* This function is of type acpi_osd_exec_callback */
+static void _handle_hotplug_event_root(void *context)
 {
 	struct acpi_pci_root *root;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpi_hp_work *hp_work;
+	struct acpi_hp_cb_data *cb_data;
 	acpi_handle handle;
 	u32 type;
 
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
+	cb_data = (struct acpi_hp_cb_data *)context;
+	handle = cb_data->handle;
+	type = cb_data->type;
 
 	root = acpi_pci_find_root(handle);
 
@@ -124,14 +125,22 @@ static void _handle_hotplug_event_root(struct work_struct *work)
 		break;
 	}
 
-	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+	kfree(context); /* allocated in handle_hotplug_event_bridge */
 }
 
 static void handle_hotplug_event_root(acpi_handle handle, u32 type,
 					void *context)
 {
-	alloc_acpi_hp_work(handle, type, context,
-				_handle_hotplug_event_root);
+	/*
+	 * Currently the code adds all hotplug events to the kacpid_wq
+	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
+	 * The proper way to fix this is to reorganize the code so that
+	 * drivers (dock, etc.) do not call acpi_os_execute(), etc.
+	 * For now just re-add this work to the kacpi_hotplug_wq so we
+	 * don't deadlock on hotplug actions.
+	 */
+	acpi_hp_cb_execute(handle, type, context,
+			_handle_hotplug_event_root);
 }
 
 static bool acpi_is_root_bridge_object(acpi_handle handle)
diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
index 0833d2e..b30fc37 100644
--- a/drivers/pci/hotplug/acpiphp_glue.c
+++ b/drivers/pci/hotplug/acpiphp_glue.c
@@ -50,6 +50,8 @@
 #include <linux/slab.h>
 #include <linux/acpi.h>
 
+#include <acpi/acpiosxf.h>
+
 #include "../pci.h"
 #include "acpiphp.h"
 
@@ -1209,7 +1211,8 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
 	return AE_OK ;
 }
 
-static void _handle_hotplug_event_bridge(struct work_struct *work)
+/* This function is of type acpi_osd_exec_callback */
+static void _handle_hotplug_event_bridge(void *context)
 {
 	struct acpiphp_bridge *bridge;
 	char objname[64];
@@ -1217,13 +1220,13 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 				      .pointer = objname };
 	struct acpi_device *device;
 	int num_sub_bridges = 0;
-	struct acpi_hp_work *hp_work;
+	struct acpi_hp_cb_data *cb_data;
 	acpi_handle handle;
 	u32 type;
 
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
+	cb_data = (struct acpi_hp_cb_data *)context;
+	handle = cb_data->handle;
+	type = cb_data->type;
 
 	if (acpi_bus_get_device(handle, &device)) {
 		/* This bridge must have just been physically inserted */
@@ -1302,7 +1305,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
 	}
 
 out:
-	kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
+	kfree(context); /* allocated in handle_hotplug_event_bridge */
 }
 
 /**
@@ -1324,29 +1327,28 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
+	acpi_hp_cb_execute(handle, type, context,
+				_handle_hotplug_event_bridge);
 }
 
-static void _handle_hotplug_event_func(struct work_struct *work)
+/* This function is of type acpi_osd_exec_callback */
+static void _handle_hotplug_event_func(void *context)
 {
 	struct acpiphp_func *func;
 	char objname[64];
 	struct acpi_buffer buffer = { .length = sizeof(objname),
 				      .pointer = objname };
-	struct acpi_hp_work *hp_work;
+	struct acpi_hp_cb_data *cb_data;
 	acpi_handle handle;
 	u32 type;
-	void *context;
 
-	hp_work = container_of(work, struct acpi_hp_work, work);
-	handle = hp_work->handle;
-	type = hp_work->type;
-	context = hp_work->context;
+	cb_data = (struct acpi_hp_cb_data *)context;
+	handle = cb_data->handle;
+	type = cb_data->type;
+	func = (struct acpiphp_func *)cb_data->context;
 
 	acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
 
-	func = (struct acpiphp_func *)context;
-
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
 		/* bus re-enumerate */
@@ -1377,7 +1379,7 @@ static void _handle_hotplug_event_func(struct work_struct *work)
 		break;
 	}
 
-	kfree(hp_work); /* allocated in handle_hotplug_event_func */
+	kfree(context); /* allocated in handle_hotplug_event_func */
 }
 
 /**
@@ -1399,7 +1401,8 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
 	 * For now just re-add this work to the kacpi_hotplug_wq so we
 	 * don't deadlock on hotplug actions.
 	 */
-	alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
+	acpi_hp_cb_execute(handle, type, context,
+			_handle_hotplug_event_func);
 }
 
 static struct acpi_pci_driver acpi_pci_hp_driver = {
diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
index 9f68f69..8825891 100644
--- a/include/acpi/acpiosxf.h
+++ b/include/acpi/acpiosxf.h
@@ -194,14 +194,13 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);
 /*
  * Threads and Scheduling
  */
-struct acpi_hp_work {
-	struct work_struct work;
+struct acpi_hp_cb_data {
 	acpi_handle handle;
 	u32 type;
 	void *context;
 };
-void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
-			void (*func)(struct work_struct *work));
+void acpi_hp_cb_execute(acpi_handle handle, u32 type, void *context,
+			acpi_osd_exec_callback function);
 
 acpi_thread_id acpi_os_get_thread_id(void);
 
-- 
1.7.1

Re: [PATCH v3 1/3] Use acpi_os_hotplug_execute() instead of alloc_acpi_hp_work().

[Yinghai Lu]

On Wed, Oct 31, 2012 at 12:27 AM, Tang Chen <tangchen@cn.fujitsu.com> wrote:
> Hi Yinghai,
>
> alloc_acpi_hp_work() just puts the hutplug work onto kacpi_hotplug_wq.
> As mentioned by Toshi Kani, this job has been done in acpi_os_hotplug_execute().
> So we should use it instead of alloc_acpi_hp_work().
>
> This patch adds a acpi_hp_cb_data struct, which encapsulates the hotplug
> event notifier's parameters:
>         struct acpi_hp_cb_data {
>                 acpi_handle handle;
>                 u32 type;
>                 void *context;
>         };
>
> And also a function alloc_acpi_hp_work(), which calls acpi_os_hotplug_execute()
> to put the hotplug job onto kacpi_hotplug_wq.
>
> This patch is based on Lu Yinghai's tree:
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2
>
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/acpi/osl.c                 |   28 ++++++++++++------------
>  drivers/acpi/pci_root_hp.c         |   25 +++++++++++++++-------
>  drivers/pci/hotplug/acpiphp_glue.c |   39 +++++++++++++++++++----------------
>  include/acpi/acpiosxf.h            |    7 ++---
>  4 files changed, 55 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 311a921..d441b16 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -52,6 +52,7 @@
>  #include <acpi/acpi.h>
>  #include <acpi/acpi_bus.h>
>  #include <acpi/processor.h>
> +#include <acpi/acpiosxf.h>

not needed.

>
>  #define _COMPONENT             ACPI_OS_SERVICES
>  ACPI_MODULE_NAME("osl");
> @@ -1592,23 +1593,22 @@ void acpi_os_set_prepare_sleep(int (*func)(u8 sleep_state,
>         __acpi_os_prepare_sleep = func;
>  }
>
> -void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> -                       void (*func)(struct work_struct *work))
> +void acpi_hp_cb_execute(acpi_handle handle, u32 type, void *context,
> +                       acpi_osd_exec_callback function)
>  {
> -       struct acpi_hp_work *hp_work;
> -       int ret;
> +       acpi_status status;
> +       struct acpi_hp_cb_data *cb_data;
>
> -       hp_work = kmalloc(sizeof(*hp_work), GFP_KERNEL);
> -       if (!hp_work)
> +       cb_data = kmalloc(sizeof(struct acpi_hp_cb_data), GFP_KERNEL);
> +       if (!cb_data)
>                 return;
>
> -       hp_work->handle = handle;
> -       hp_work->type = type;
> -       hp_work->context = context;
> +       cb_data->handle = handle;
> +       cb_data->type = type;
> +       cb_data->context = context;
>
> -       INIT_WORK(&hp_work->work, func);
> -       ret = queue_work(kacpi_hotplug_wq, &hp_work->work);
> -       if (!ret)
> -               kfree(hp_work);
> +       status = acpi_os_hotplug_execute(function, cb_data);
> +       if (ACPI_FAILURE(status))
> +               kfree(cb_data);
>  }
> -EXPORT_SYMBOL(alloc_acpi_hp_work);
> +EXPORT_SYMBOL(acpi_hp_cb_execute);
> diff --git a/drivers/acpi/pci_root_hp.c b/drivers/acpi/pci_root_hp.c
> index 7d427e6..2ff83f4 100644
> --- a/drivers/acpi/pci_root_hp.c
> +++ b/drivers/acpi/pci_root_hp.c
> @@ -75,19 +75,20 @@ static void handle_root_bridge_removal(struct acpi_device *device)
>         acpi_bus_hot_remove_device(ej_event);
>  }
>
> -static void _handle_hotplug_event_root(struct work_struct *work)
> +/* This function is of type acpi_osd_exec_callback */
> +static void _handle_hotplug_event_root(void *context)
>  {
>         struct acpi_pci_root *root;
>         char objname[64];
>         struct acpi_buffer buffer = { .length = sizeof(objname),
>                                       .pointer = objname };
> -       struct acpi_hp_work *hp_work;
> +       struct acpi_hp_cb_data *cb_data;
>         acpi_handle handle;
>         u32 type;
>
> -       hp_work = container_of(work, struct acpi_hp_work, work);
> -       handle = hp_work->handle;
> -       type = hp_work->type;
> +       cb_data = (struct acpi_hp_cb_data *)context;
> +       handle = cb_data->handle;
> +       type = cb_data->type;
>
>         root = acpi_pci_find_root(handle);
>
> @@ -124,14 +125,22 @@ static void _handle_hotplug_event_root(struct work_struct *work)
>                 break;
>         }
>
> -       kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
> +       kfree(context); /* allocated in handle_hotplug_event_bridge */
>  }
>
>  static void handle_hotplug_event_root(acpi_handle handle, u32 type,
>                                         void *context)
>  {
> -       alloc_acpi_hp_work(handle, type, context,
> -                               _handle_hotplug_event_root);
> +       /*
> +        * Currently the code adds all hotplug events to the kacpid_wq
> +        * queue when it should add hotplug events to the kacpi_hotplug_wq.
> +        * The proper way to fix this is to reorganize the code so that
> +        * drivers (dock, etc.) do not call acpi_os_execute(), etc.
> +        * For now just re-add this work to the kacpi_hotplug_wq so we
> +        * don't deadlock on hotplug actions.
> +        */
> +       acpi_hp_cb_execute(handle, type, context,
> +                       _handle_hotplug_event_root);
>  }
>
>  static bool acpi_is_root_bridge_object(acpi_handle handle)
> diff --git a/drivers/pci/hotplug/acpiphp_glue.c b/drivers/pci/hotplug/acpiphp_glue.c
> index 0833d2e..b30fc37 100644
> --- a/drivers/pci/hotplug/acpiphp_glue.c
> +++ b/drivers/pci/hotplug/acpiphp_glue.c
> @@ -50,6 +50,8 @@
>  #include <linux/slab.h>
>  #include <linux/acpi.h>
>
> +#include <acpi/acpiosxf.h>
> +

not needed.

>  #include "../pci.h"
>  #include "acpiphp.h"
>
> @@ -1209,7 +1211,8 @@ check_sub_bridges(acpi_handle handle, u32 lvl, void *context, void **rv)
>         return AE_OK ;
>  }
>
> -static void _handle_hotplug_event_bridge(struct work_struct *work)
> +/* This function is of type acpi_osd_exec_callback */
> +static void _handle_hotplug_event_bridge(void *context)
>  {
>         struct acpiphp_bridge *bridge;
>         char objname[64];
> @@ -1217,13 +1220,13 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
>                                       .pointer = objname };
>         struct acpi_device *device;
>         int num_sub_bridges = 0;
> -       struct acpi_hp_work *hp_work;
> +       struct acpi_hp_cb_data *cb_data;
>         acpi_handle handle;
>         u32 type;
>
> -       hp_work = container_of(work, struct acpi_hp_work, work);
> -       handle = hp_work->handle;
> -       type = hp_work->type;
> +       cb_data = (struct acpi_hp_cb_data *)context;
> +       handle = cb_data->handle;
> +       type = cb_data->type;
>
>         if (acpi_bus_get_device(handle, &device)) {
>                 /* This bridge must have just been physically inserted */
> @@ -1302,7 +1305,7 @@ static void _handle_hotplug_event_bridge(struct work_struct *work)
>         }
>
>  out:
> -       kfree(hp_work); /* allocated in handle_hotplug_event_bridge */
> +       kfree(context); /* allocated in handle_hotplug_event_bridge */
>  }
>
>  /**
> @@ -1324,29 +1327,28 @@ static void handle_hotplug_event_bridge(acpi_handle handle, u32 type,
>          * For now just re-add this work to the kacpi_hotplug_wq so we
>          * don't deadlock on hotplug actions.
>          */
> -       alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_bridge);
> +       acpi_hp_cb_execute(handle, type, context,
> +                               _handle_hotplug_event_bridge);
>  }
>
> -static void _handle_hotplug_event_func(struct work_struct *work)
> +/* This function is of type acpi_osd_exec_callback */
> +static void _handle_hotplug_event_func(void *context)
>  {
>         struct acpiphp_func *func;
>         char objname[64];
>         struct acpi_buffer buffer = { .length = sizeof(objname),
>                                       .pointer = objname };
> -       struct acpi_hp_work *hp_work;
> +       struct acpi_hp_cb_data *cb_data;
>         acpi_handle handle;
>         u32 type;
> -       void *context;
>
> -       hp_work = container_of(work, struct acpi_hp_work, work);
> -       handle = hp_work->handle;
> -       type = hp_work->type;
> -       context = hp_work->context;
> +       cb_data = (struct acpi_hp_cb_data *)context;
> +       handle = cb_data->handle;
> +       type = cb_data->type;
> +       func = (struct acpiphp_func *)cb_data->context;

too many context looks confusing.

>
>         acpi_get_name(handle, ACPI_FULL_PATHNAME, &buffer);
>
> -       func = (struct acpiphp_func *)context;
> -
>         switch (type) {
>         case ACPI_NOTIFY_BUS_CHECK:
>                 /* bus re-enumerate */
> @@ -1377,7 +1379,7 @@ static void _handle_hotplug_event_func(struct work_struct *work)
>                 break;
>         }
>
> -       kfree(hp_work); /* allocated in handle_hotplug_event_func */
> +       kfree(context); /* allocated in handle_hotplug_event_func */
>  }
>
>  /**
> @@ -1399,7 +1401,8 @@ static void handle_hotplug_event_func(acpi_handle handle, u32 type,
>          * For now just re-add this work to the kacpi_hotplug_wq so we
>          * don't deadlock on hotplug actions.
>          */
> -       alloc_acpi_hp_work(handle, type, context, _handle_hotplug_event_func);
> +       acpi_hp_cb_execute(handle, type, context,
> +                       _handle_hotplug_event_func);
>  }
>
>  static struct acpi_pci_driver acpi_pci_hp_driver = {
> diff --git a/include/acpi/acpiosxf.h b/include/acpi/acpiosxf.h
> index 9f68f69..8825891 100644
> --- a/include/acpi/acpiosxf.h
> +++ b/include/acpi/acpiosxf.h
> @@ -194,14 +194,13 @@ void acpi_os_fixed_event_count(u32 fixed_event_number);
>  /*
>   * Threads and Scheduling
>   */
> -struct acpi_hp_work {
> -       struct work_struct work;
> +struct acpi_hp_cb_data {
>         acpi_handle handle;
>         u32 type;
>         void *context;
>  };
> -void alloc_acpi_hp_work(acpi_handle handle, u32 type, void *context,
> -                       void (*func)(struct work_struct *work));
> +void acpi_hp_cb_execute(acpi_handle handle, u32 type, void *context,
> +                       acpi_osd_exec_callback function);
>
>  acpi_thread_id acpi_os_get_thread_id(void);

Please check if you can just fold
   acpi_hp_cb_execute
callers, and use acpi_os_hotplug_execute directly.

and have two local conext struct too.

>
> --
> 1.7.1
>

Re: [PATCH v3 1/3] Use acpi_os_hotplug_execute() instead of alloc_acpi_hp_work().

[Tang Chen]

On 11/01/2012 11:52 AM, Yinghai Lu wrote:
> On Wed, Oct 31, 2012 at 12:27 AM, Tang Chen<tangchen@cn.fujitsu.com>  wrote:
> Please check if you can just fold
>     acpi_hp_cb_execute
> callers, and use acpi_os_hotplug_execute directly.
>
> and have two local conext struct too.
>
I think this could bring some duplicated work. We need to do the same
work every time we call acpi_os_hotplug_execute(), what has been done
in acpi_hp_cb_execute().

I can try to modify it and resend a new patch to see if it is better.

Thanks. :)

[PATCH v3 2/3] Use kacpi_hotplug_wq to handle container hotplug event.

[Tang Chen]

As the comments in __acpi_os_execute() said:

	We can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
	because the hotplug code may call driver .remove() functions,
	which invoke flush_scheduled_work/acpi_os_wait_events_complete
	to flush these workqueues.

we should keep the hotplug code in kacpi_hotplug_wq.

But we have the following call series in kernel now:
        acpi_ev_queue_notify_request()
        |--> acpi_os_execute()
             |--> __acpi_os_execute(type, function, context, 0)

The last parameter 0 makes the container_notify_cb() executed in
kacpi_notify_wq or kacpid_wq. So, we need to put the real hotplug code
into kacpi_hotplug_wq.

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/container.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index 69e2d6b..a10eee6 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -35,6 +35,7 @@
 #include <acpi/acpi_bus.h>
 #include <acpi/acpi_drivers.h>
 #include <acpi/container.h>
+#include <acpi/acpiosxf.h>
 
 #define PREFIX "ACPI: "
 
@@ -165,14 +166,22 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle)
 	return result;
 }
 
-static void container_notify_cb(acpi_handle handle, u32 type, void *context)
+/* This function is of type acpi_osd_exec_callback */
+static void _container_notify_cb(void *context)
 {
 	struct acpi_device *device = NULL;
 	int result;
 	int present;
 	acpi_status status;
+	struct acpi_hp_cb_data *cb_data;
+	acpi_handle handle;
+	u32 type;
 	u32 ost_code = ACPI_OST_SC_NON_SPECIFIC_FAILURE; /* default */
 
+	cb_data = (struct acpi_hp_cb_data *)context;
+	handle = cb_data->handle;
+	type = cb_data->type;
+
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
 		/* Fall through */
@@ -188,7 +197,7 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
 				/* device exist and this is a remove request */
 				device->flags.eject_pending = 1;
 				kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
-				return;
+				goto out;
 			}
 			break;
 		}
@@ -210,20 +219,37 @@ static void container_notify_cb(acpi_handle handle, u32 type, void *context)
 		if (!acpi_bus_get_device(handle, &device) && device) {
 			device->flags.eject_pending = 1;
 			kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
-			return;
+			goto out;
 		}
 		break;
 
 	default:
 		/* non-hotplug event; possibly handled by other handler */
-		return;
+		goto out;
 	}
 
 	/* Inform firmware that the hotplug operation has completed */
 	(void) acpi_evaluate_hotplug_ost(handle, type, ost_code, NULL);
+
+out:
+	kfree(context);		/* allocated in container_notify_cb */
 	return;
 }
 
+static void container_notify_cb(acpi_handle handle, u32 type,
+				void *context)
+{
+	/*
+	 * Currently the code adds all hotplug events to the kacpid_wq
+	 * queue when it should add hotplug events to the kacpi_hotplug_wq.
+	 * The proper way to fix this is to reorganize the code so that
+	 * drivers (dock, etc.) do not call acpi_os_execute(), etc.
+	 * For now just re-add this work to the kacpi_hotplug_wq so we
+	 * don't deadlock on hotplug actions.
+	 */
+	acpi_hp_cb_execute(handle, type, context, _container_notify_cb);
+}
+
 static acpi_status
 container_walk_namespace_cb(acpi_handle handle,
 			    u32 lvl, void *context, void **rv)
-- 
1.7.1

[PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Tang Chen]

This patch introduces a new function container_device_remove() to do
the container hot-remove job. It works like the following:

1. call acpi_bus_trim(device, 0) to stop the container device,
   which means to unbind ACPI drivers first before remove devices.
   (This feature is introduced by Lu Yinghai:
    http://www.spinics.net/lists/linux-pci/msg17667.html)
2. generate the KOBJ_OFFLINE uevent. (Did I do this correct ?)
3. call acpi_bus_hot_remove_device(), which will call acpi_bus_trim(device, 1)
   to remove the container.

This patch is based on Lu Yinghai's work.
git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2

Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
---
 drivers/acpi/container.c |   63 ++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
index a10eee6..4abec98 100644
--- a/drivers/acpi/container.c
+++ b/drivers/acpi/container.c
@@ -166,6 +166,32 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle)
 	return result;
 }
 
+static int container_device_remove(struct acpi_device *device)
+{
+	int ret;
+	struct acpi_eject_event *ej_event;
+
+	/* stop container device at first */
+	ret = acpi_bus_trim(device, 0);
+	pr_debug("acpi_bus_trim stop return %x\n", ret);
+	if (ret)
+		return ret;
+
+	/* send the uevent before remove the device */
+	kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
+
+	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
+	if (!ej_event)
+		return -ENOMEM;
+
+	ej_event->device = device;
+	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
+
+	acpi_bus_hot_remove_device(ej_event);
+
+	return 0;
+}
+
 /* This function is of type acpi_osd_exec_callback */
 static void _container_notify_cb(void *context)
 {
@@ -182,6 +208,9 @@ static void _container_notify_cb(void *context)
 	handle = cb_data->handle;
 	type = cb_data->type;
 
+	present = is_device_present(handle);
+	status = acpi_bus_get_device(handle, &device);
+
 	switch (type) {
 	case ACPI_NOTIFY_BUS_CHECK:
 		/* Fall through */
@@ -190,13 +219,16 @@ static void _container_notify_cb(void *context)
 		       (type == ACPI_NOTIFY_BUS_CHECK) ?
 		       "ACPI_NOTIFY_BUS_CHECK" : "ACPI_NOTIFY_DEVICE_CHECK");
 
-		present = is_device_present(handle);
-		status = acpi_bus_get_device(handle, &device);
 		if (!present) {
 			if (ACPI_SUCCESS(status)) {
 				/* device exist and this is a remove request */
-				device->flags.eject_pending = 1;
-				kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
+				result = container_device_remove(device);
+				if (result) {
+					dev_warn(&device->dev, "%s: Failed to "
+						"remove container\n",
+						__func__);
+					break;
+				}
 				goto out;
 			}
 			break;
@@ -207,7 +239,9 @@ static void _container_notify_cb(void *context)
 
 		result = container_device_add(&device, handle);
 		if (result) {
-			printk(KERN_WARNING "Failed to add container\n");
+			dev_warn(&device->dev,
+				"%s: Failed to add container\n",
+				__func__);
 			break;
 		}
 
@@ -216,12 +250,21 @@ static void _container_notify_cb(void *context)
 		break;
 
 	case ACPI_NOTIFY_EJECT_REQUEST:
-		if (!acpi_bus_get_device(handle, &device) && device) {
-			device->flags.eject_pending = 1;
-			kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
-			goto out;
+		printk(KERN_WARNING "Container driver received %s event\n",
+			"ACPI_NOTIFY_EJECT_REQUEST");
+
+		if (!present || ACPI_FAILURE(status) || !device)
+			break;
+
+		result = container_device_remove(device);
+		if (result) {
+			dev_warn(&device->dev,
+				"%s: Failed to remove container\n",
+				__func__);
+			break;
 		}
-		break;
+
+		goto out;
 
 	default:
 		/* non-hotplug event; possibly handled by other handler */
-- 
1.7.1

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Toshi Kani]

On Wed, 2012-10-31 at 15:27 +0800, Tang Chen wrote:
> This patch introduces a new function container_device_remove() to do
> the container hot-remove job. It works like the following:
> 
> 1. call acpi_bus_trim(device, 0) to stop the container device,
>    which means to unbind ACPI drivers first before remove devices.
>    (This feature is introduced by Lu Yinghai:
>     http://www.spinics.net/lists/linux-pci/msg17667.html)
> 2. generate the KOBJ_OFFLINE uevent. (Did I do this correct ?)
> 3. call acpi_bus_hot_remove_device(), which will call acpi_bus_trim(device, 1)
>    to remove the container.
> 
> This patch is based on Lu Yinghai's work.
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2
> 
> Signed-off-by: Tang Chen <tangchen@cn.fujitsu.com>
> ---
>  drivers/acpi/container.c |   63 ++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/acpi/container.c b/drivers/acpi/container.c
> index a10eee6..4abec98 100644
> --- a/drivers/acpi/container.c
> +++ b/drivers/acpi/container.c
> @@ -166,6 +166,32 @@ static int container_device_add(struct acpi_device **device, acpi_handle handle)
>  	return result;
>  }
>  
> +static int container_device_remove(struct acpi_device *device)
> +{
> +	int ret;
> +	struct acpi_eject_event *ej_event;
> +
> +	/* stop container device at first */
> +	ret = acpi_bus_trim(device, 0);
> +	pr_debug("acpi_bus_trim stop return %x\n", ret);
> +	if (ret)
> +		return ret;
> +
> +	/* send the uevent before remove the device */
> +	kobject_uevent(&device->dev.kobj, KOBJ_OFFLINE);
> +
> +	ej_event = kmalloc(sizeof(*ej_event), GFP_KERNEL);
> +	if (!ej_event)
> +		return -ENOMEM;
> +
> +	ej_event->device = device;
> +	ej_event->event = ACPI_NOTIFY_EJECT_REQUEST;
> +
> +	acpi_bus_hot_remove_device(ej_event);

Hi Tang,

Rafael pointed out in my CPU hot-remove patch that
acpi_bus_hot_remove_device() was not exported for modules.  Looks like
you have the same problem here.  FYI, I just sent the following patch
that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().

https://lkml.org/lkml/2012/11/1/225

Thanks,
-Toshi

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Yinghai Lu]

On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
>
> Rafael pointed out in my CPU hot-remove patch that
> acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> you have the same problem here.  FYI, I just sent the following patch
> that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
>
> https://lkml.org/lkml/2012/11/1/225

acpi_os_hotplug_execute() does not like having good quality yet.

c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
because the hotplug code may call driver .remove() functions,
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
which invoke flush_scheduled_work/acpi_os_wait_events_complete
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
flush these workqueues.
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
= hp ? kacpi_hotplug_wq :
c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
dpc->wait = hp ? 1 : 0;
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
(queue == kacpi_hotplug_wq)
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
if (queue == kacpi_notify_wq)
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)

really don't know why checking queue and call same code in every branch.

from comm:

commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
Author: Zhang Rui <rui.zhang@intel.com>
Date:   Mon Mar 22 15:48:54 2010 +0800

    ACPI: fixes a false alarm from lockdep

    fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
    workqueues.
    http://bugzilla.kernel.org/show_bug.cgi?id=14553
    https://bugzilla.kernel.org/show_bug.cgi?id=15521

    Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Shaohua Li <shaohua.li@intel.com>
    Signed-off-by: Zhang Rui <rui.zhang@intel.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index 8e6d866..900da68 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -758,7 +758,14 @@ static acpi_status
__acpi_os_execute(acpi_execute_type type,
        queue = hp ? kacpi_hotplug_wq :
                (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
        dpc->wait = hp ? 1 : 0;
-       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+
+       if (queue == kacpi_hotplug_wq)
+               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+       else if (queue == kacpi_notify_wq)
+               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+       else
+               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+
        ret = queue_work(queue, &dpc->work);

        if (!ret) {


Len or Rafael,
can you just revert that silly patch?

Yinghai

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Toshi Kani]

On Thu, 2012-11-01 at 11:28 -0700, Yinghai Lu wrote:
> On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> >
> > Rafael pointed out in my CPU hot-remove patch that
> > acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> > you have the same problem here.  FYI, I just sent the following patch
> > that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
> >
> > https://lkml.org/lkml/2012/11/1/225
> 
> acpi_os_hotplug_execute() does not like having good quality yet.
> 
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
> can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
> because the hotplug code may call driver .remove() functions,
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
> which invoke flush_scheduled_work/acpi_os_wait_events_complete
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
> flush these workqueues.
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
> = hp ? kacpi_hotplug_wq :
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
>  (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> 9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
> dpc->wait = hp ? 1 : 0;
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
> (queue == kacpi_hotplug_wq)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
> if (queue == kacpi_notify_wq)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)
> 
> really don't know why checking queue and call same code in every branch.
> 
> from comm:
> 
> commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
> Author: Zhang Rui <rui.zhang@intel.com>
> Date:   Mon Mar 22 15:48:54 2010 +0800
> 
>     ACPI: fixes a false alarm from lockdep
> 
>     fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
>     workqueues.
>     http://bugzilla.kernel.org/show_bug.cgi?id=14553
>     https://bugzilla.kernel.org/show_bug.cgi?id=15521
> 
>     Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 8e6d866..900da68 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -758,7 +758,14 @@ static acpi_status
> __acpi_os_execute(acpi_execute_type type,
>         queue = hp ? kacpi_hotplug_wq :
>                 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
>         dpc->wait = hp ? 1 : 0;
> -       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +
> +       if (queue == kacpi_hotplug_wq)
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +       else if (queue == kacpi_notify_wq)
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +       else
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +
>         ret = queue_work(queue, &dpc->work);
> 
>         if (!ret) {
> 
> 
> Len or Rafael,
> can you just revert that silly patch?

Hi Yinghai,

Per the following thread, the code seems to be written in this way to
allocate a separate lock_class_key for each work queue.  It should have
had some comment to explain this, though.

https://lkml.org/lkml/2009/12/13/304

Thanks,
-Toshi



> 
> Yinghai

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Rafael J. Wysocki]

On Thursday, November 01, 2012 01:17:58 PM Toshi Kani wrote:
> On Thu, 2012-11-01 at 11:28 -0700, Yinghai Lu wrote:
> > On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> > >
> > > Rafael pointed out in my CPU hot-remove patch that
> > > acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> > > you have the same problem here.  FYI, I just sent the following patch
> > > that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
> > >
> > > https://lkml.org/lkml/2012/11/1/225
> > 
> > acpi_os_hotplug_execute() does not like having good quality yet.
> > 
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
> > can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
> > because the hotplug code may call driver .remove() functions,
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
> > which invoke flush_scheduled_work/acpi_os_wait_events_complete
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
> > flush these workqueues.
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
> > = hp ? kacpi_hotplug_wq :
> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
> >  (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> > 9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
> > dpc->wait = hp ? 1 : 0;
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
> > (queue == kacpi_hotplug_wq)
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
> > if (queue == kacpi_notify_wq)
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)
> > 
> > really don't know why checking queue and call same code in every branch.
> > 
> > from comm:
> > 
> > commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
> > Author: Zhang Rui <rui.zhang@intel.com>
> > Date:   Mon Mar 22 15:48:54 2010 +0800
> > 
> >     ACPI: fixes a false alarm from lockdep
> > 
> >     fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
> >     workqueues.
> >     http://bugzilla.kernel.org/show_bug.cgi?id=14553
> >     https://bugzilla.kernel.org/show_bug.cgi?id=15521
> > 
> >     Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
> >     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> >     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >     Signed-off-by: Len Brown <len.brown@intel.com>
> > 
> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> > index 8e6d866..900da68 100644
> > --- a/drivers/acpi/osl.c
> > +++ b/drivers/acpi/osl.c
> > @@ -758,7 +758,14 @@ static acpi_status
> > __acpi_os_execute(acpi_execute_type type,
> >         queue = hp ? kacpi_hotplug_wq :
> >                 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> >         dpc->wait = hp ? 1 : 0;
> > -       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > +
> > +       if (queue == kacpi_hotplug_wq)
> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > +       else if (queue == kacpi_notify_wq)
> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > +       else
> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > +
> >         ret = queue_work(queue, &dpc->work);
> > 
> >         if (!ret) {
> > 
> > 
> > Len or Rafael,
> > can you just revert that silly patch?
> 
> Hi Yinghai,
> 
> Per the following thread, the code seems to be written in this way to
> allocate a separate lock_class_key for each work queue.  It should have
> had some comment to explain this, though.
> 
> https://lkml.org/lkml/2009/12/13/304

The code has evolved since then, however, so that it doesn't make sense
any more.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Yinghai Lu]

On Thu, Nov 1, 2012 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Thursday, November 01, 2012 01:17:58 PM Toshi Kani wrote:
>> On Thu, 2012-11-01 at 11:28 -0700, Yinghai Lu wrote:
>> > On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
>> > >
>> > > Rafael pointed out in my CPU hot-remove patch that
>> > > acpi_bus_hot_remove_device() was not exported for modules.  Looks like
>> > > you have the same problem here.  FYI, I just sent the following patch
>> > > that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
>> > >
>> > > https://lkml.org/lkml/2012/11/1/225
>> >
>> > acpi_os_hotplug_execute() does not like having good quality yet.
>> >
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
>> > can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
>> > because the hotplug code may call driver .remove() functions,
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
>> > which invoke flush_scheduled_work/acpi_os_wait_events_complete
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
>> > flush these workqueues.
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
>> > = hp ? kacpi_hotplug_wq :
>> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
>> >  (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
>> > 9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
>> > dpc->wait = hp ? 1 : 0;
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
>> > (queue == kacpi_hotplug_wq)
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
>> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
>> > if (queue == kacpi_notify_wq)
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
>> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
>> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)
>> >
>> > really don't know why checking queue and call same code in every branch.
>> >
>> > from comm:
>> >
>> > commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
>> > Author: Zhang Rui <rui.zhang@intel.com>
>> > Date:   Mon Mar 22 15:48:54 2010 +0800
>> >
>> >     ACPI: fixes a false alarm from lockdep
>> >
>> >     fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
>> >     workqueues.
>> >     http://bugzilla.kernel.org/show_bug.cgi?id=14553
>> >     https://bugzilla.kernel.org/show_bug.cgi?id=15521
>> >
>> >     Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
>> >     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>> >     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> >     Signed-off-by: Len Brown <len.brown@intel.com>
>> >
>> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
>> > index 8e6d866..900da68 100644
>> > --- a/drivers/acpi/osl.c
>> > +++ b/drivers/acpi/osl.c
>> > @@ -758,7 +758,14 @@ static acpi_status
>> > __acpi_os_execute(acpi_execute_type type,
>> >         queue = hp ? kacpi_hotplug_wq :
>> >                 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
>> >         dpc->wait = hp ? 1 : 0;
>> > -       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > +
>> > +       if (queue == kacpi_hotplug_wq)
>> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > +       else if (queue == kacpi_notify_wq)
>> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > +       else
>> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
>> > +
>> >         ret = queue_work(queue, &dpc->work);
>> >
>> >         if (!ret) {
>> >
>> >
>> > Len or Rafael,
>> > can you just revert that silly patch?
>>
>> Hi Yinghai,
>>
>> Per the following thread, the code seems to be written in this way to
>> allocate a separate lock_class_key for each work queue.  It should have
>> had some comment to explain this, though.
>>
>> https://lkml.org/lkml/2009/12/13/304
>
> The code has evolved since then, however, so that it doesn't make sense
> any more.
>

oh, no, that commit should not be reverted. instead we should add some
comment for it...

that mean : three path, will have three separated static lock dep key
from every INIT_WORK.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Rafael J. Wysocki]

On Thursday, November 01, 2012 01:16:22 PM Yinghai Lu wrote:
> On Thu, Nov 1, 2012 at 1:17 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Thursday, November 01, 2012 01:17:58 PM Toshi Kani wrote:
> >> On Thu, 2012-11-01 at 11:28 -0700, Yinghai Lu wrote:
> >> > On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> >> > >
> >> > > Rafael pointed out in my CPU hot-remove patch that
> >> > > acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> >> > > you have the same problem here.  FYI, I just sent the following patch
> >> > > that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
> >> > >
> >> > > https://lkml.org/lkml/2012/11/1/225
> >> >
> >> > acpi_os_hotplug_execute() does not like having good quality yet.
> >> >
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
> >> > can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
> >> > because the hotplug code may call driver .remove() functions,
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
> >> > which invoke flush_scheduled_work/acpi_os_wait_events_complete
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
> >> > flush these workqueues.
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
> >> > = hp ? kacpi_hotplug_wq :
> >> > c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
> >> >  (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> >> > 9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
> >> > dpc->wait = hp ? 1 : 0;
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
> >> > (queue == kacpi_hotplug_wq)
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
> >> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
> >> > if (queue == kacpi_notify_wq)
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
> >> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
> >> >  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)
> >> >
> >> > really don't know why checking queue and call same code in every branch.
> >> >
> >> > from comm:
> >> >
> >> > commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
> >> > Author: Zhang Rui <rui.zhang@intel.com>
> >> > Date:   Mon Mar 22 15:48:54 2010 +0800
> >> >
> >> >     ACPI: fixes a false alarm from lockdep
> >> >
> >> >     fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
> >> >     workqueues.
> >> >     http://bugzilla.kernel.org/show_bug.cgi?id=14553
> >> >     https://bugzilla.kernel.org/show_bug.cgi?id=15521
> >> >
> >> >     Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
> >> >     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> >> >     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> >> >     Signed-off-by: Len Brown <len.brown@intel.com>
> >> >
> >> > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> >> > index 8e6d866..900da68 100644
> >> > --- a/drivers/acpi/osl.c
> >> > +++ b/drivers/acpi/osl.c
> >> > @@ -758,7 +758,14 @@ static acpi_status
> >> > __acpi_os_execute(acpi_execute_type type,
> >> >         queue = hp ? kacpi_hotplug_wq :
> >> >                 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> >> >         dpc->wait = hp ? 1 : 0;
> >> > -       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > +
> >> > +       if (queue == kacpi_hotplug_wq)
> >> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > +       else if (queue == kacpi_notify_wq)
> >> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > +       else
> >> > +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> >> > +
> >> >         ret = queue_work(queue, &dpc->work);
> >> >
> >> >         if (!ret) {
> >> >
> >> >
> >> > Len or Rafael,
> >> > can you just revert that silly patch?
> >>
> >> Hi Yinghai,
> >>
> >> Per the following thread, the code seems to be written in this way to
> >> allocate a separate lock_class_key for each work queue.  It should have
> >> had some comment to explain this, though.
> >>
> >> https://lkml.org/lkml/2009/12/13/304
> >
> > The code has evolved since then, however, so that it doesn't make sense
> > any more.
> >
> 
> oh, no, that commit should not be reverted. instead we should add some
> comment for it...
> 
> that mean : three path, will have three separated static lock dep key
> from every INIT_WORK.

I see.

OK, I'll drop the patch removing it.

What about the following comment:

"To prevent lockdep from complaining unnecessarily, make sure that there
 is a different static lockdep key created for each workqueue by using
 INIT_WORK for each of them separately."

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Toshi Kani]

> > >> Hi Yinghai,
> > >>
> > >> Per the following thread, the code seems to be written in this way to
> > >> allocate a separate lock_class_key for each work queue.  It should have
> > >> had some comment to explain this, though.
> > >>
> > >> https://lkml.org/lkml/2009/12/13/304
> > >
> > > The code has evolved since then, however, so that it doesn't make sense
> > > any more.
> > >
> > 
> > oh, no, that commit should not be reverted. instead we should add some
> > comment for it...
> > 
> > that mean : three path, will have three separated static lock dep key
> > from every INIT_WORK.
> 
> I see.
> 
> OK, I'll drop the patch removing it.
> 
> What about the following comment:
> 
> "To prevent lockdep from complaining unnecessarily, make sure that there
>  is a different static lockdep key created for each workqueue by using
>  INIT_WORK for each of them separately."

Looks good to me.

Thanks,
-Toshi


> 
> Thanks,
> Rafael
> 
> 

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Yinghai Lu]

On Thu, Nov 1, 2012 at 2:31 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> oh, no, that commit should not be reverted. instead we should add some
>> comment for it...
>>
>> that mean : three path, will have three separated static lock dep key
>> from every INIT_WORK.
>
> I see.
>
> OK, I'll drop the patch removing it.
>
> What about the following comment:
>
> "To prevent lockdep from complaining unnecessarily, make sure that there
>  is a different static lockdep key created for each workqueue by using
>  INIT_WORK for each of them separately."
>
created ?

how about "defined" ?

or just remove  "created"

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Rafael J. Wysocki]

On Thursday, November 01, 2012 03:15:31 PM Yinghai Lu wrote:
> On Thu, Nov 1, 2012 at 2:31 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> oh, no, that commit should not be reverted. instead we should add some
> >> comment for it...
> >>
> >> that mean : three path, will have three separated static lock dep key
> >> from every INIT_WORK.
> >
> > I see.
> >
> > OK, I'll drop the patch removing it.
> >
> > What about the following comment:
> >
> > "To prevent lockdep from complaining unnecessarily, make sure that there
> >  is a different static lockdep key created for each workqueue by using
> >  INIT_WORK for each of them separately."
> >
> created ?
> 
> how about "defined" ?
> 
> or just remove  "created"

Yes, that's better.

I suppose that the appended patch may be better still, though.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI: Make seemingly useless check in osl.c more understandable

There is a seemingly useless check in drivers/acpi/osl.c added by
commit bc73675 (ACPI: fixes a false alarm from lockdep), which really
is necessary to avoid false positive lockdep complaints.  Document
this and rearrange the code related to it so that it makes fewer
checks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/acpi/osl.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

Index: linux/drivers/acpi/osl.c
===================================================================
--- linux.orig/drivers/acpi/osl.c
+++ linux/drivers/acpi/osl.c
@@ -944,17 +944,24 @@ static acpi_status __acpi_os_execute(acp
 	 * because the hotplug code may call driver .remove() functions,
 	 * which invoke flush_scheduled_work/acpi_os_wait_events_complete
 	 * to flush these workqueues.
+	 *
+	 * To prevent lockdep from complaining unnecessarily, make sure that
+	 * there is a different static lockdep key for each workqueue by using
+	 * INIT_WORK() for each of them separately.
 	 */
-	queue = hp ? kacpi_hotplug_wq :
-		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
-	dpc->wait = hp ? 1 : 0;
-
-	if (queue == kacpi_hotplug_wq)
+	if (hp) {
+		queue = kacpi_hotplug_wq;
+		dpc->wait = 1;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	else if (queue == kacpi_notify_wq)
+	} else if (type == OSL_NOTIFY_HANDLER) {
+		queue = kacpi_notify_wq;
+		dpc->wait = 0;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	else
+	} else {
+		queue = kacpid_wq;
+		dpc->wait = 0;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+	}
 
 	/*
 	 * On some machines, a software-initiated SMI causes corruption unless

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Yinghai Lu]

On Thu, Nov 1, 2012 at 4:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: ACPI: Make seemingly useless check in osl.c more understandable
>
> There is a seemingly useless check in drivers/acpi/osl.c added by
> commit bc73675 (ACPI: fixes a false alarm from lockdep), which really
> is necessary to avoid false positive lockdep complaints.  Document
> this and rearrange the code related to it so that it makes fewer
> checks.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/acpi/osl.c |   21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> Index: linux/drivers/acpi/osl.c
> ===================================================================
> --- linux.orig/drivers/acpi/osl.c
> +++ linux/drivers/acpi/osl.c
> @@ -944,17 +944,24 @@ static acpi_status __acpi_os_execute(acp
>          * because the hotplug code may call driver .remove() functions,
>          * which invoke flush_scheduled_work/acpi_os_wait_events_complete
>          * to flush these workqueues.
> +        *
> +        * To prevent lockdep from complaining unnecessarily, make sure that
> +        * there is a different static lockdep key for each workqueue by using
> +        * INIT_WORK() for each of them separately.
>          */
> -       queue = hp ? kacpi_hotplug_wq :
> -               (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> -       dpc->wait = hp ? 1 : 0;
> -
> -       if (queue == kacpi_hotplug_wq)
> +       if (hp) {
> +               queue = kacpi_hotplug_wq;
> +               dpc->wait = 1;
>                 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> -       else if (queue == kacpi_notify_wq)
> +       } else if (type == OSL_NOTIFY_HANDLER) {
> +               queue = kacpi_notify_wq;
> +               dpc->wait = 0;

yes, much clear.

at the same can you changne
dpc allocation from kmalloc with kzalloc instead.

then we save two lines for dpc->wait = 0

After that

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

>                 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> -       else
> +       } else {
> +               queue = kacpid_wq;
> +               dpc->wait = 0;
>                 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +       }
>
>         /*
>          * On some machines, a software-initiated SMI causes corruption unless
>
> --

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Rafael J. Wysocki]

On Thursday, November 01, 2012 04:39:50 PM Yinghai Lu wrote:
> On Thu, Nov 1, 2012 at 4:15 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Subject: ACPI: Make seemingly useless check in osl.c more understandable
> >
> > There is a seemingly useless check in drivers/acpi/osl.c added by
> > commit bc73675 (ACPI: fixes a false alarm from lockdep), which really
> > is necessary to avoid false positive lockdep complaints.  Document
> > this and rearrange the code related to it so that it makes fewer
> > checks.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> >  drivers/acpi/osl.c |   21 ++++++++++++++-------
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > Index: linux/drivers/acpi/osl.c
> > ===================================================================
> > --- linux.orig/drivers/acpi/osl.c
> > +++ linux/drivers/acpi/osl.c
> > @@ -944,17 +944,24 @@ static acpi_status __acpi_os_execute(acp
> >          * because the hotplug code may call driver .remove() functions,
> >          * which invoke flush_scheduled_work/acpi_os_wait_events_complete
> >          * to flush these workqueues.
> > +        *
> > +        * To prevent lockdep from complaining unnecessarily, make sure that
> > +        * there is a different static lockdep key for each workqueue by using
> > +        * INIT_WORK() for each of them separately.
> >          */
> > -       queue = hp ? kacpi_hotplug_wq :
> > -               (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> > -       dpc->wait = hp ? 1 : 0;
> > -
> > -       if (queue == kacpi_hotplug_wq)
> > +       if (hp) {
> > +               queue = kacpi_hotplug_wq;
> > +               dpc->wait = 1;
> >                 INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> > -       else if (queue == kacpi_notify_wq)
> > +       } else if (type == OSL_NOTIFY_HANDLER) {
> > +               queue = kacpi_notify_wq;
> > +               dpc->wait = 0;
> 
> yes, much clear.
> 
> at the same can you changne
> dpc allocation from kmalloc with kzalloc instead.
> 
> then we save two lines for dpc->wait = 0

Good idea. :-)

> After that
> 
> Acked-by: Yinghai Lu <yinghai@kernel.org>

For completeness:

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: ACPI: Make seemingly useless check in osl.c more understandable

There is a seemingly useless check in drivers/acpi/osl.c added by
commit bc73675 (ACPI: fixes a false alarm from lockdep), which really
is necessary to avoid false positive lockdep complaints.  Document
this and rearrange the code related to it so that it makes fewer
checks.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Yinghai Lu <yinghai@kernel.org>
---
 drivers/acpi/osl.c |   21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

Index: linux/drivers/acpi/osl.c
===================================================================
--- linux.orig/drivers/acpi/osl.c
+++ linux/drivers/acpi/osl.c
@@ -932,7 +932,7 @@ static acpi_status __acpi_os_execute(acp
 	 * having a static work_struct.
 	 */
 
-	dpc = kmalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
+	dpc = kzalloc(sizeof(struct acpi_os_dpc), GFP_ATOMIC);
 	if (!dpc)
 		return AE_NO_MEMORY;
 
@@ -944,17 +944,22 @@ static acpi_status __acpi_os_execute(acp
 	 * because the hotplug code may call driver .remove() functions,
 	 * which invoke flush_scheduled_work/acpi_os_wait_events_complete
 	 * to flush these workqueues.
+	 *
+	 * To prevent lockdep from complaining unnecessarily, make sure that
+	 * there is a different static lockdep key for each workqueue by using
+	 * INIT_WORK() for each of them separately.
 	 */
-	queue = hp ? kacpi_hotplug_wq :
-		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
-	dpc->wait = hp ? 1 : 0;
-
-	if (queue == kacpi_hotplug_wq)
+	if (hp) {
+		queue = kacpi_hotplug_wq;
+		dpc->wait = 1;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	else if (queue == kacpi_notify_wq)
+	} else if (type == OSL_NOTIFY_HANDLER) {
+		queue = kacpi_notify_wq;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-	else
+	} else {
+		queue = kacpid_wq;
 		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
+	}
 
 	/*
 	 * On some machines, a software-initiated SMI causes corruption unless


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Rafael J. Wysocki]

On Thursday, November 01, 2012 11:28:25 AM Yinghai Lu wrote:
> On Thu, Nov 1, 2012 at 9:43 AM, Toshi Kani <toshi.kani@hp.com> wrote:
> >
> > Rafael pointed out in my CPU hot-remove patch that
> > acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> > you have the same problem here.  FYI, I just sent the following patch
> > that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
> >
> > https://lkml.org/lkml/2012/11/1/225
> 
> acpi_os_hotplug_execute() does not like having good quality yet.
> 
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  941)   /*
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  942)    * We
> can't run hotplug code in keventd_wq/kacpid_wq/kacpid_notify_wq
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  943)    *
> because the hotplug code may call driver .remove() functions,
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  944)    *
> which invoke flush_scheduled_work/acpi_os_wait_events_complete
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  945)    * to
> flush these workqueues.
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  946)    */
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  947)   queue
> = hp ? kacpi_hotplug_wq :
> c02256be (Zhang Rui           2009-06-23 10:20:29 +0800  948)
>  (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> 9ac61856 (Bjorn Helgaas       2009-08-31 22:32:10 +0000  949)
> dpc->wait = hp ? 1 : 0;
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  950)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  951)   if
> (queue == kacpi_hotplug_wq)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  952)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  953)   else
> if (queue == kacpi_notify_wq)
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  954)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  955)   else
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  956)
>  INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> bc73675b (Zhang Rui           2010-03-22 15:48:54 +0800  957)
> 
> really don't know why checking queue and call same code in every branch.
> 
> from comm:
> 
> commit bc73675b99fd9850dd914be01d71af99c5d2a1ae
> Author: Zhang Rui <rui.zhang@intel.com>
> Date:   Mon Mar 22 15:48:54 2010 +0800
> 
>     ACPI: fixes a false alarm from lockdep
> 
>     fixes a false alarm from lockdep, as acpi hotplug workqueue waits other
>     workqueues.
>     http://bugzilla.kernel.org/show_bug.cgi?id=14553
>     https://bugzilla.kernel.org/show_bug.cgi?id=15521
> 
>     Original-patch-from: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>     Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>     Signed-off-by: Len Brown <len.brown@intel.com>
> 
> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
> index 8e6d866..900da68 100644
> --- a/drivers/acpi/osl.c
> +++ b/drivers/acpi/osl.c
> @@ -758,7 +758,14 @@ static acpi_status
> __acpi_os_execute(acpi_execute_type type,
>         queue = hp ? kacpi_hotplug_wq :
>                 (type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
>         dpc->wait = hp ? 1 : 0;
> -       INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +
> +       if (queue == kacpi_hotplug_wq)
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +       else if (queue == kacpi_notify_wq)
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +       else
> +               INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> +
>         ret = queue_work(queue, &dpc->work);
> 
>         if (!ret) {
> 
> 
> Len or Rafael,
> can you just revert that silly patch?

We're removing this as per:

http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=commit;h=77f1966ec9763e85e5f1a9202802e90c297b4c21

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3 3/3] Improve container_notify_cb() to support container hot-remove.

[Tang Chen]

On 11/02/2012 12:43 AM, Toshi Kani wrote:
>
> Hi Tang,
>
> Rafael pointed out in my CPU hot-remove patch that
> acpi_bus_hot_remove_device() was not exported for modules.  Looks like
> you have the same problem here.  FYI, I just sent the following patch
> that exports acpi_bus_hot_remove_device() and acpi_os_hotplug_execute().
>
> https://lkml.org/lkml/2012/11/1/225
>
Hi Toshi,

I see. Thanks for the info. :)

Thanks.

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Yasuaki Ishimatsu]

Hi Tang,

2012/10/31 16:27, Tang Chen wrote:
> Hi,
> 
> The container hotplug handler container_notify_cb() didn't implement
> the hot-remove functionality. So, these 3 patches implement it like
> the following way:
> 
> patch 1. Do not use kacpid_wq/kacpid_notify_wq to handle container hotplug event,
>           use kacpi_hotplug_wq instead to avoid deadlock.
>           Doing this is to reuse acpi_bus_hot_remove_device() in container
>           hot-remove handling.
> 
> patch 2. Introduce a new function container_device_remove() to handle
>           ACPI_NOTIFY_EJECT_REQUEST event for container.

If container device contains memory device, the function is 
very danger. As you know, we are developing a memory hotplug.
If memory has kernel memory, memory hot remove operations fails.
But container_device_remove() cannot realize it. So even if
the memory hot remove operation fails, container_device_remove()
keeps hot remove operation. Finally, the function sends _EJ0
to firmware. In this case, if the memory is accessed, kernel
panic occurs.
The example is as follows:

 https://lkml.org/lkml/2012/9/26/318

Thanks,
Yasuaki Ishimatsu

> 
> 
> change log v2 -> v3:
> 
> 1. Add 1 patch(patch1). As Toshi Kan mentioned, acpi_os_hotplug_execute() is already
>     kernel. So use it instead of alloc_acpi_hp_work() to add hotplug job onto kacpi_hotplug_wq.
> 
> 2. In patch3: Print caller's function name when container_device_remove() fails to help to debug.
> 
> 3. In patch3: Add commit message to describ why we need to call acpi_bus_trim() twice when
>     removing devices.
> 
> change log v1 -> v2:
> 
> 1. In patch1: Based on the lastest for-pci-split-pci-root-hp-2 branch from Lu Yinghai,
>     use alloc_acpi_hp_work() to add container hotplug work into kacpi_hotplug_wq.
> 
> 2. In patch2: Allocate ej_event after container is stopped, so that we don't need to
>     kfree the ej_event if stopping container failed.
> 
> 
> This is based on Lu Yinghai's job.
> git://git.kernel.org/pub/scm/linux/kernel/git/yinghai/linux-yinghai.git for-pci-split-pci-root-hp-2
> 
> 
> Tang Chen (3):
>    Use acpi_os_hotplug_execute() instead of alloc_acpi_hp_work().
>    Use kacpi_hotplug_wq to handle container hotplug event.
>    Improve container_notify_cb() to support container hot-remove.
> 
>   drivers/acpi/container.c           |   95 +++++++++++++++++++++++++++++++-----
>   drivers/acpi/osl.c                 |   28 +++++-----
>   drivers/acpi/pci_root_hp.c         |   25 ++++++---
>   drivers/pci/hotplug/acpiphp_glue.c |   39 ++++++++-------
>   include/acpi/acpiosxf.h            |    7 +--
>   5 files changed, 137 insertions(+), 57 deletions(-)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Yinghai Lu]

On Wed, Oct 31, 2012 at 4:09 AM, Yasuaki Ishimatsu
<isimatu.yasuaki@jp.fujitsu.com> wrote:
>> patch 2. Introduce a new function container_device_remove() to handle
>>           ACPI_NOTIFY_EJECT_REQUEST event for container.
>
> If container device contains memory device, the function is
> very danger. As you know, we are developing a memory hotplug.
> If memory has kernel memory, memory hot remove operations fails.
> But container_device_remove() cannot realize it. So even if
> the memory hot remove operation fails, container_device_remove()
> keeps hot remove operation. Finally, the function sends _EJ0
> to firmware. In this case, if the memory is accessed, kernel
> panic occurs.
> The example is as follows:
>
>  https://lkml.org/lkml/2012/9/26/318

so what is the overall status memory hot-remove?
how are following memory get processed ?
1. memory for kernel text, module
2. page table
3. vmemmap
4. memory for kmalloc, for dma

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Tang Chen]

Hi Yinghai,

How do you think the 1st patch ? Is the idea OK with you ?

And about the memory hotplug thing, so far as I know, we are trying to
limit kernel memory in some nodes, and only support to hot-remove the
nodes with out kernel memory. This functionality is called
online_movable. And some of the patches are already in next-tree, most
of the patches are under review. :)

Thanks. :)

On 11/01/2012 12:48 AM, Yinghai Lu wrote:
> On Wed, Oct 31, 2012 at 4:09 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com>  wrote:
>>> patch 2. Introduce a new function container_device_remove() to handle
>>>            ACPI_NOTIFY_EJECT_REQUEST event for container.
>>
>> If container device contains memory device, the function is
>> very danger. As you know, we are developing a memory hotplug.
>> If memory has kernel memory, memory hot remove operations fails.
>> But container_device_remove() cannot realize it. So even if
>> the memory hot remove operation fails, container_device_remove()
>> keeps hot remove operation. Finally, the function sends _EJ0
>> to firmware. In this case, if the memory is accessed, kernel
>> panic occurs.
>> The example is as follows:
>>
>>   https://lkml.org/lkml/2012/9/26/318
>
> so what is the overall status memory hot-remove?
> how are following memory get processed ?
> 1. memory for kernel text, module
> 2. page table
> 3. vmemmap
> 4. memory for kmalloc, for dma
>

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Jiang Liu]

On 11/01/2012 12:48 AM, Yinghai Lu wrote:
> On Wed, Oct 31, 2012 at 4:09 AM, Yasuaki Ishimatsu
> <isimatu.yasuaki@jp.fujitsu.com> wrote:
>>> patch 2. Introduce a new function container_device_remove() to handle
>>>           ACPI_NOTIFY_EJECT_REQUEST event for container.
>>
>> If container device contains memory device, the function is
>> very danger. As you know, we are developing a memory hotplug.
>> If memory has kernel memory, memory hot remove operations fails.
>> But container_device_remove() cannot realize it. So even if
>> the memory hot remove operation fails, container_device_remove()
>> keeps hot remove operation. Finally, the function sends _EJ0
>> to firmware. In this case, if the memory is accessed, kernel
>> panic occurs.
>> The example is as follows:
>>
>>  https://lkml.org/lkml/2012/9/26/318
> 
> so what is the overall status memory hot-remove?
> how are following memory get processed ?
> 1. memory for kernel text, module
> 2. page table
> 3. vmemmap
> 4. memory for kmalloc, for dma
Hi Yinghai,
	I have given a talk about the CPU/memory/PCI host bridge hotplug
current status and next step plan on China Linux Kernel Developer Conference,
please refer to
https://github.com/downloads/jiangliu/linux/ACPI%20Based%20System%20Device%20Dynamic%20Reconfiguration.pdf
	Thanks!
	Gerry

> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Tang Chen]

On 10/31/2012 07:09 PM, Yasuaki Ishimatsu wrote:
> Hi Tang,
> 
> If container device contains memory device, the function is
> very danger. As you know, we are developing a memory hotplug.
> If memory has kernel memory, memory hot remove operations fails.
> But container_device_remove() cannot realize it. So even if
> the memory hot remove operation fails, container_device_remove()
> keeps hot remove operation. Finally, the function sends _EJ0
> to firmware. In this case, if the memory is accessed, kernel
> panic occurs.
> The example is as follows:
> 
>   https://lkml.org/lkml/2012/9/26/318
> 
Hi Ishimatsu,

I see, thanks for the info. So we need to do some roll back thing.

Is anyone doing this now ?
If yes, would you please give me some links to refer to ? And I think I
should push these patches later.
If not, I think I can try to do it.

Thanks. :)

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Hanjun Guo]

On 2012/10/31 19:09, Yasuaki Ishimatsu wrote:
> Hi Tang,
> 
> 2012/10/31 16:27, Tang Chen wrote:
>> Hi,
>>
>> The container hotplug handler container_notify_cb() didn't implement
>> the hot-remove functionality. So, these 3 patches implement it like
>> the following way:
>>
>> patch 1. Do not use kacpid_wq/kacpid_notify_wq to handle container hotplug event,
>>           use kacpi_hotplug_wq instead to avoid deadlock.
>>           Doing this is to reuse acpi_bus_hot_remove_device() in container
>>           hot-remove handling.
>>
>> patch 2. Introduce a new function container_device_remove() to handle
>>           ACPI_NOTIFY_EJECT_REQUEST event for container.
> 
> If container device contains memory device, the function is 
> very danger. As you know, we are developing a memory hotplug.
> If memory has kernel memory, memory hot remove operations fails.
> But container_device_remove() cannot realize it. So even if
> the memory hot remove operation fails, container_device_remove()
> keeps hot remove operation. Finally, the function sends _EJ0
> to firmware. In this case, if the memory is accessed, kernel
> panic occurs.

Hi all,
I think Yasuaki mentioned the key point for the container device remove,
that is dependency.

Currently, container, processor, and memory hotpulg are managed by different ACPI
hotplug drivers, the driver works when handle device hotplug individually, but they
have no idea for each other.

This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
should remove its child before remove the device itself, and hot add its parent before
the device itself.

According to the ACPI namespace, we can resolve most of dependency issues. On a typical
two processor sockets system, the namespace is like this:

/_SB                   ---container device, with HID ACPI0004
    |_SCK0             ---container device, with HID ACPI0004
	  |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
          |_...
          |_CPUx
          |_MEM0       ---Memory device, with HID PNP0C80
    |_SCK1
	  |_CPU0
          |_...
          |_CPUx
          |_MEM1
    |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08

If hot remove the container device, such as SCK0, we can easily know the dependency list
is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.

And there is another corner case for hotplug devices in the namespace above, that is:
1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;

2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
   or the system will crash down. yes, dynamic dependency analysis is needed here.
   and the ACPI hotplug driver totally have no idea of this.

so, should we do something to settle this down ?

Thanks
Hanjun Guo


> The example is as follows:
> 
>  https://lkml.org/lkml/2012/9/26/318
> 
> Thanks,
> Yasuaki Ishimatsu
> 
>>

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Tang Chen]

On 11/26/2012 01:42 PM, Hanjun Guo wrote:
>
> Hi all,
> I think Yasuaki mentioned the key point for the container device remove,
> that is dependency.
>
> Currently, container, processor, and memory hotpulg are managed by different ACPI
> hotplug drivers, the driver works when handle device hotplug individually, but they
> have no idea for each other.
>
> This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
> should remove its child before remove the device itself, and hot add its parent before
> the device itself.
>
> According to the ACPI namespace, we can resolve most of dependency issues. On a typical
> two processor sockets system, the namespace is like this:
>
> /_SB                   ---container device, with HID ACPI0004
>      |_SCK0             ---container device, with HID ACPI0004
> 	  |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
>            |_...
>            |_CPUx
>            |_MEM0       ---Memory device, with HID PNP0C80
>      |_SCK1
> 	  |_CPU0
>            |_...
>            |_CPUx
>            |_MEM1
>      |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08
>
> If hot remove the container device, such as SCK0, we can easily know the dependency list
> is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.
>
> And there is another corner case for hotplug devices in the namespace above, that is:
> 1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;
>
> 2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
>     or the system will crash down. yes, dynamic dependency analysis is needed here.
>     and the ACPI hotplug driver totally have no idea of this.
>
> so, should we do something to settle this down ?

Hi Guo,

I am trying to do this too. :)

But so far as I know, Vasilis Liaskovitis has provided an approach.
Please refer to the following url.
https://lkml.org/lkml/2012/11/15/159

I think we can review his patches first. :)

And by the way, I think the ACPI based hotplug framework provided by
Liu Jiang may also settle this problem. But I'm not quit sure yet. :)

Thanks. :)

>
> Thanks
> Hanjun Guo
>
>
>> The example is as follows:
>>
>>   https://lkml.org/lkml/2012/9/26/318
>>
>> Thanks,
>> Yasuaki Ishimatsu
>>
>>>
>
>
>
>

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Hanjun Guo]

On 2012/11/26 14:06, Tang Chen wrote:
> On 11/26/2012 01:42 PM, Hanjun Guo wrote:
>>
>> Hi all,
>> I think Yasuaki mentioned the key point for the container device remove,
>> that is dependency.
>>
>> Currently, container, processor, and memory hotpulg are managed by different ACPI
>> hotplug drivers, the driver works when handle device hotplug individually, but they
>> have no idea for each other.
>>
>> This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
>> should remove its child before remove the device itself, and hot add its parent before
>> the device itself.
>>
>> According to the ACPI namespace, we can resolve most of dependency issues. On a typical
>> two processor sockets system, the namespace is like this:
>>
>> /_SB                   ---container device, with HID ACPI0004
>>      |_SCK0             ---container device, with HID ACPI0004
>>       |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
>>            |_...
>>            |_CPUx
>>            |_MEM0       ---Memory device, with HID PNP0C80
>>      |_SCK1
>>       |_CPU0
>>            |_...
>>            |_CPUx
>>            |_MEM1
>>      |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08
>>
>> If hot remove the container device, such as SCK0, we can easily know the dependency list
>> is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.
>>
>> And there is another corner case for hotplug devices in the namespace above, that is:
>> 1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;
>>
>> 2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
>>     or the system will crash down. yes, dynamic dependency analysis is needed here.
>>     and the ACPI hotplug driver totally have no idea of this.
>>
>> so, should we do something to settle this down ?
> 
> Hi Guo,
> 
> I am trying to do this too. :)
> 
> But so far as I know, Vasilis Liaskovitis has provided an approach.
> Please refer to the following url.
> https://lkml.org/lkml/2012/11/15/159
> 
> I think we can review his patches first. :)

Yes, I noticed Vasilis Liaskovitis's patch and the discussion from
Greg and Toshi, Greg suggested that no driver core changes to resolve
this problem, so we should find another way.

> 
> And by the way, I think the ACPI based hotplug framework provided by
> Liu Jiang may also settle this problem. But I'm not quit sure yet. :)

The ACPI based hotplug framework provided by Jiang Liu has settle this
problem down, you may refer to:
https://lkml.org/lkml/2012/11/4/64

Thanks
Hanjun Guo

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Hanjun Guo]

On 2012/11/26 14:06, Tang Chen wrote:
> On 11/26/2012 01:42 PM, Hanjun Guo wrote:
>>
>> Hi all,
>> I think Yasuaki mentioned the key point for the container device remove,
>> that is dependency.
>>
>> Currently, container, processor, and memory hotpulg are managed by different ACPI
>> hotplug drivers, the driver works when handle device hotplug individually, but they
>> have no idea for each other.
>>
>> This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
>> should remove its child before remove the device itself, and hot add its parent before
>> the device itself.
>>
>> According to the ACPI namespace, we can resolve most of dependency issues. On a typical
>> two processor sockets system, the namespace is like this:
>>
>> /_SB                   ---container device, with HID ACPI0004
>>      |_SCK0             ---container device, with HID ACPI0004
>>       |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
>>            |_...
>>            |_CPUx
>>            |_MEM0       ---Memory device, with HID PNP0C80
>>      |_SCK1
>>       |_CPU0
>>            |_...
>>            |_CPUx
>>            |_MEM1
>>      |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08
>>
>> If hot remove the container device, such as SCK0, we can easily know the dependency list
>> is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.
>>
>> And there is another corner case for hotplug devices in the namespace above, that is:
>> 1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;
>>
>> 2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
>>     or the system will crash down. yes, dynamic dependency analysis is needed here.
>>     and the ACPI hotplug driver totally have no idea of this.
>>
>> so, should we do something to settle this down ?
> 
> Hi Guo,
> 
> I am trying to do this too. :)

Great， what's your idea of this?


--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Tang Chen]

On 11/27/2012 09:08 AM, Hanjun Guo wrote:
> On 2012/11/26 14:06, Tang Chen wrote:
>> On 11/26/2012 01:42 PM, Hanjun Guo wrote:
>>>
>>> Hi all,
>>> I think Yasuaki mentioned the key point for the container device remove,
>>> that is dependency.
>>>
>>> Currently, container, processor, and memory hotpulg are managed by different ACPI
>>> hotplug drivers, the driver works when handle device hotplug individually, but they
>>> have no idea for each other.
>>>
>>> This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
>>> should remove its child before remove the device itself, and hot add its parent before
>>> the device itself.
>>>
>>> According to the ACPI namespace, we can resolve most of dependency issues. On a typical
>>> two processor sockets system, the namespace is like this:
>>>
>>> /_SB                   ---container device, with HID ACPI0004
>>>       |_SCK0             ---container device, with HID ACPI0004
>>>        |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
>>>             |_...
>>>             |_CPUx
>>>             |_MEM0       ---Memory device, with HID PNP0C80
>>>       |_SCK1
>>>        |_CPU0
>>>             |_...
>>>             |_CPUx
>>>             |_MEM1
>>>       |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08
>>>
>>> If hot remove the container device, such as SCK0, we can easily know the dependency list
>>> is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.
>>>
>>> And there is another corner case for hotplug devices in the namespace above, that is:
>>> 1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;
>>>
>>> 2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
>>>      or the system will crash down. yes, dynamic dependency analysis is needed here.
>>>      and the ACPI hotplug driver totally have no idea of this.
>>>
>>> so, should we do something to settle this down ?
>>
>> Hi Guo,
>>
>> I am trying to do this too. :)
>
> Great， what's your idea of this?
>
Hi Guo,

I noticed they had a lot of discussion on this topic.
https://lkml.org/lkml/2012/11/15/159

And Vasilis's latest patches are here:
https://lkml.org/lkml/2012/11/23/335

I think we can have a review of this patchset first. :)

And also, as you said, the new ACPI hotplug framework from Liu Jiang
will settle this problem more properly.
So I think any solution now could be temporary. :)

Re: [PATCH v3 0/3] ACPI: container hot remove support.

[Hanjun Guo]

On 2012/11/27 10:38, Tang Chen wrote:
> On 11/27/2012 09:08 AM, Hanjun Guo wrote:
>> On 2012/11/26 14:06, Tang Chen wrote:
>>> On 11/26/2012 01:42 PM, Hanjun Guo wrote:
>>>>
>>>> Hi all,
>>>> I think Yasuaki mentioned the key point for the container device remove,
>>>> that is dependency.
>>>>
>>>> Currently, container, processor, and memory hotpulg are managed by different ACPI
>>>> hotplug drivers, the driver works when handle device hotplug individually, but they
>>>> have no idea for each other.
>>>>
>>>> This may introduce some issues, such as Yasuaki mentioned above, that is to say, we
>>>> should remove its child before remove the device itself, and hot add its parent before
>>>> the device itself.
>>>>
>>>> According to the ACPI namespace, we can resolve most of dependency issues. On a typical
>>>> two processor sockets system, the namespace is like this:
>>>>
>>>> /_SB                   ---container device, with HID ACPI0004
>>>>       |_SCK0             ---container device, with HID ACPI0004
>>>>        |_CPU0       ---processor device, with HID ACPI0009 or LNXCPU
>>>>             |_...
>>>>             |_CPUx
>>>>             |_MEM0       ---Memory device, with HID PNP0C80
>>>>       |_SCK1
>>>>        |_CPU0
>>>>             |_...
>>>>             |_CPUx
>>>>             |_MEM1
>>>>       |_PCI0            ---Host bridge, with HID PNP0A03 or PNP0A08
>>>>
>>>> If hot remove the container device, such as SCK0, we can easily know the dependency list
>>>> is CPU0~CPUx and MEM0, but I think the ACPI hotplug driver haven't resolve this now.
>>>>
>>>> And there is another corner case for hotplug devices in the namespace above, that is:
>>>> 1) Remove SCK0. yes, we can remove it with no dependency to the host bridge PCI0;
>>>>
>>>> 2) Remove SCK1 after SCK0. we should remove the host bridge PCI0 first,
>>>>      or the system will crash down. yes, dynamic dependency analysis is needed here.
>>>>      and the ACPI hotplug driver totally have no idea of this.
>>>>
>>>> so, should we do something to settle this down ?
>>>
>>> Hi Guo,
>>>
>>> I am trying to do this too. :)
>>
>> Great， what's your idea of this?
>>
> Hi Guo,
> 
> I noticed they had a lot of discussion on this topic.
> https://lkml.org/lkml/2012/11/15/159
> 
> And Vasilis's latest patches are here:
> https://lkml.org/lkml/2012/11/23/335
> 
> I think we can have a review of this patchset first. :)

Hi Tang,
Thanks for your remind, I will review Vasilis's latest patches,
and reply Vasilis's patch if I have any comments.

> 
> And also, as you said, the new ACPI hotplug framework from Liu Jiang
> will settle this problem more properly.
> So I think any solution now could be temporary. :)
> 
> .
>