[PATCH 1/5] Show the physical device node of backlight class device.

[PATCH 1/5] Show the physical device node of backlight class device.

[Zhang Rui]

Create symbol link from backlight class device to ACPI video device.

More and more laptops are shipped with multiple ACPI
video devices, while we export only one of them to userspace.

With this patch applied, we can know which ACPI video device
is used by "cat /sys/class/backlight/acpi_video0/device/path".

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1bdfb37..9de143a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -976,6 +976,11 @@ static void acpi_video_device_find_cap(struct acpi_video_device *device)
 		device->backlight->props.max_brightness = device->brightness->count-3;
 		kfree(name);
 
+		result = sysfs_create_link(&device->backlight->dev.kobj,
+					   &device->dev->dev.kobj, "device");
+		if (result)
+			printk(KERN_ERR PREFIX "Create sysfs link\n");
+
 		device->cdev = thermal_cooling_device_register("LCD",
 					device->dev, &video_cooling_ops);
 		if (IS_ERR(device->cdev))
@@ -1990,6 +1995,7 @@ static int acpi_video_bus_put_one_device(struct acpi_video_device *device)
 	status = acpi_remove_notify_handler(device->dev->handle,
 					    ACPI_DEVICE_NOTIFY,
 					    acpi_video_device_notify);
+	sysfs_remove_link(&device->backlight->dev.kobj, "device");
 	backlight_device_unregister(device->backlight);
 	if (device->cdev) {
 		sysfs_remove_link(&device->dev->dev.kobj,
-- 
1.5.4.4

[PATCH 2/5] fix a deadlock in hotplug case

[Zhang Rui]

we used to run the hotplug code in keventd_wq.
But when hot removing the ACPI battery device,
power_supply_unregister invokes flush_scheduled_work.
This causes a deadlock.
Introduce a new workqueue for hotplug in this patch.
http://bugzilla.kernel.org/show_bug.cgi?id=13533

Tested-by: Paul Martin <pm@debian.org>
Tested-by: Vojtech Gondzala <vojtech.gondzala@gmail.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/osl.c |   23 ++++++++++++++---------
 1 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c
index d916bea..80488db 100644
--- a/drivers/acpi/osl.c
+++ b/drivers/acpi/osl.c
@@ -79,6 +79,11 @@ static acpi_osd_handler acpi_irq_handler;
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
+/* 
+ * run the hotplug code in a seperate workqueue
+ * to avoid the deadlock issue
+ */
+static struct workqueue_struct *kacpi_hotplug_wq;
 
 struct acpi_res_list {
 	resource_size_t start;
@@ -192,8 +197,10 @@ acpi_status acpi_os_initialize1(void)
 {
 	kacpid_wq = create_singlethread_workqueue("kacpid");
 	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
+	kacpi_hotplug_wq = create_singlethread_workqueue("kacpi_hotplug");
 	BUG_ON(!kacpid_wq);
 	BUG_ON(!kacpi_notify_wq);
+	BUG_ON(!kacpi_hotplug_wq);
 	return AE_OK;
 }
 
@@ -206,6 +213,7 @@ acpi_status acpi_os_terminate(void)
 
 	destroy_workqueue(kacpid_wq);
 	destroy_workqueue(kacpi_notify_wq);
+	destroy_workqueue(kacpi_hotplug_wq);
 
 	return AE_OK;
 }
@@ -716,6 +724,7 @@ static acpi_status __acpi_os_execute(acpi_execute_type type,
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
 	struct workqueue_struct *queue;
+	work_func_t func;
 	int ret;
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.\n",
@@ -740,15 +749,11 @@ static acpi_status __acpi_os_execute(acpi_execute_type type,
 	dpc->function = function;
 	dpc->context = context;
 
-	if (!hp) {
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-		queue = (type == OSL_NOTIFY_HANDLER) ?
-			kacpi_notify_wq : kacpid_wq;
-		ret = queue_work(queue, &dpc->work);
-	} else {
-		INIT_WORK(&dpc->work, acpi_os_execute_hp_deferred);
-		ret = schedule_work(&dpc->work);
-	}
+	queue = hp ? kacpi_hotplug_wq :
+		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
+	func = hp ? acpi_os_execute_hp_deferred : acpi_os_execute_deferred;
+	INIT_WORK(&dpc->work, func);
+	ret = queue_work(queue, &dpc->work);
 
 	if (!ret) {
 		printk(KERN_ERR PREFIX
-- 
1.5.4.4

[PATCH 3/5] run ACPI device hot removal in kacpi_hotplug_wq

[Zhang Rui]

Run the ACPI device hot removal code in kacpi_hotplug_wq
rather than a new kernel thread.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/scan.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 8ff510b..9c6e42e 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -95,7 +95,7 @@ acpi_device_modalias_show(struct device *dev, struct device_attribute *attr, cha
 }
 static DEVICE_ATTR(modalias, 0444, acpi_device_modalias_show, NULL);
 
-static int acpi_bus_hot_remove_device(void *context)
+static void acpi_bus_hot_remove_device(void *context)
 {
 	struct acpi_device *device;
 	acpi_handle handle = context;
@@ -104,10 +104,10 @@ static int acpi_bus_hot_remove_device(void *context)
 	acpi_status status = AE_OK;
 
 	if (acpi_bus_get_device(handle, &device))
-		return 0;
+		return;
 
 	if (!device)
-		return 0;
+		return;
 
 	ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 		"Hot-removing device %s...\n", dev_name(&device->dev)));
@@ -115,7 +115,7 @@ static int acpi_bus_hot_remove_device(void *context)
 	if (acpi_bus_trim(device, 1)) {
 		printk(KERN_ERR PREFIX
 				"Removing device failed\n");
-		return -1;
+		return;
 	}
 
 	/* power off device */
@@ -142,9 +142,10 @@ static int acpi_bus_hot_remove_device(void *context)
 	 */
 	status = acpi_evaluate_object(handle, "_EJ0", &arg_list, NULL);
 	if (ACPI_FAILURE(status))
-		return -ENODEV;
+		printk(KERN_WARNING PREFIX
+				"Eject device failed\n");
 
-	return 0;
+	return;
 }
 
 static ssize_t
@@ -155,7 +156,6 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 	acpi_status status;
 	acpi_object_type type = 0;
 	struct acpi_device *acpi_device = to_acpi_device(d);
-	struct task_struct *task;
 
 	if ((!count) || (buf[0] != '1')) {
 		return -EINVAL;
@@ -172,11 +172,7 @@ acpi_eject_store(struct device *d, struct device_attribute *attr,
 		goto err;
 	}
 
-	/* remove the device in another thread to fix the deadlock issue */
-	task = kthread_run(acpi_bus_hot_remove_device,
-				acpi_device->handle, "acpi_hot_remove_device");
-	if (IS_ERR(task))
-		ret = PTR_ERR(task);
+	acpi_os_hotplug_execute(acpi_bus_hot_remove_device, acpi_device->handle);
 err:
 	return ret;
 }
-- 
1.5.4.4

[PATCH 4/5] disable Vista compatibility on some sony laptops

[Zhang Rui]

Linux claims Vista compatibility to the BIOS for a number of
reasons, but this brings hard lockup on some Sony laptops.

Disable Vista compatibility via DMI for these laptops unless
we can figure out what Vista is doing for this platform.
http://bugzilla.kernel.org/show_bug.cgi?id=12904

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/blacklist.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/blacklist.c b/drivers/acpi/blacklist.c
index 09c6980..f6baa77 100644
--- a/drivers/acpi/blacklist.c
+++ b/drivers/acpi/blacklist.c
@@ -192,6 +192,22 @@ static struct dmi_system_id acpi_osi_dmi_table[] __initdata = {
 		     DMI_MATCH(DMI_PRODUCT_NAME, "ESPRIMO Mobile V5505"),
 		},
 	},
+	{
+	.callback = dmi_disable_osi_vista,
+	.ident = "Sony VGN-NS10J_S",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Sony Corporation"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "VGN-NS10J_S"),
+		},
+	},
+	{
+	.callback = dmi_disable_osi_vista,
+	.ident = "Sony VGN-SR290J",
+	.matches = {
+		     DMI_MATCH(DMI_SYS_VENDOR, "Sony Corporation"),
+		     DMI_MATCH(DMI_PRODUCT_NAME, "Sony VGN-SR290J"),
+		},
+	},
 
 	/*
 	 * BIOS invocation of _OSI(Linux) is almost always a BIOS bug.
-- 
1.5.4.4

[PATCH 5/5] ACPI: video: DMI workaround broken Acer 7720 BIOS enabling display brightness

[Zhang Rui]

http://bugzilla.kernel.org/show_bug.cgi?id=13121

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/video.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 9de143a..1914de5 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -586,6 +586,14 @@ static struct dmi_system_id video_dmi_table[] __initdata = {
 		DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 5315"),
 		},
 	},
+	{
+	 .callback = video_set_bqc_offset,
+	 .ident = "Acer Aspire 7720",
+	 .matches = {
+		DMI_MATCH(DMI_BOARD_VENDOR, "Acer"),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Aspire 7720"),
+		},
+	},
 	{}
 };
 
-- 
1.5.4.4

Re: [PATCH 5/5] ACPI: video: DMI workaround broken Acer 7720 BIOS enabling display brightness

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 4/5] disable Vista compatibility on some sony laptops

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 3/5] run ACPI device hot removal in kacpi_hotplug_wq

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 2/5] fix a deadlock in hotplug case

[Bjorn Helgaas]

On Sunday 21 June 2009 09:31:15 pm Zhang Rui wrote:
> we used to run the hotplug code in keventd_wq.
> But when hot removing the ACPI battery device,
> power_supply_unregister invokes flush_scheduled_work.
> This causes a deadlock.
> Introduce a new workqueue for hotplug in this patch.
> http://bugzilla.kernel.org/show_bug.cgi?id=13533

Can you be specific about how the deadlock occurs?  That will
make it easier to make sure we don't re-introduce the deadlock
if this design is ever changed in the future.  And I will probably
try to change this design, so this is really a self-serving
request :-)

I suspect the deadlock is something like this:

  <user removes battery, platform generates system-level notify>
    acpi_bus_notify
      blocking_notifier_call_chain
        acpi_dock_notifier_call
          acpi_os_hotplug_execute(acpi_dock_deferred_cb, ...)
            __acpi_os_execute(..., acpi_dock_deferred_cb, ...)
              schedule_work(<acpi_dock_deferred_cb>)
                queue_work(keventd_wq, <acpi_dock_deferred_cb>)
                  <acpi_dock_deferred_cb queued for execution in keventd_wq>

  worker_thread(keventd_wq)
    acpi_os_execute_hp_deferred
      acpi_dock_deferred_cb
        dock_notify
          hotplug_dock_devices
            dock_remove_acpi_device
              acpi_bus_trim
                acpi_bus_remove
                  device_release_driver
                    acpi_device_remove
                      acpi_battery_remove
                        sysfs_remove_battery
                          power_supply_unregister
                            flush_scheduled_work
                              flush_workqueue(keventd_wq)

Now we're waiting for keventd_wq to be flushed, but it won't be
considered flushed until acpi_os_execute_hp_deferred() completes.

This sort of analysis is obviously way too long and too specific
for a source code comment, but it would be nice to have it in the
bugzilla, at least.

And maybe the changelog could mention that flush_scheduled_work()
applies to keventd_wq, just to close the loop there.

The source code comment:

> +/* 
> + * run the hotplug code in a seperate workqueue
> + * to avoid the deadlock issue
> + */

should be more specific about what "the deadlock issue" is.
Maybe something like:

  /*
   * We can't run hotplug code in keventd_wq because the hotplug
   * code may call driver .remove() functions, which often use
   * flush_scheduled_work() to wait for keventd_wq to be flushed.
   */

And it might make more sense to put the comment where the queue
is used, i.e., in __acpi_os_execute(), rather than at the definition.

Bjorn

> +static struct workqueue_struct *kacpi_hotplug_wq;
>  
>  struct acpi_res_list {
>  	resource_size_t start;
> @@ -192,8 +197,10 @@ acpi_status acpi_os_initialize1(void)
>  {
>  	kacpid_wq = create_singlethread_workqueue("kacpid");
>  	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
> +	kacpi_hotplug_wq = create_singlethread_workqueue("kacpi_hotplug");
>  	BUG_ON(!kacpid_wq);
>  	BUG_ON(!kacpi_notify_wq);
> +	BUG_ON(!kacpi_hotplug_wq);
>  	return AE_OK;
>  }
>  
> @@ -206,6 +213,7 @@ acpi_status acpi_os_terminate(void)
>  
>  	destroy_workqueue(kacpid_wq);
>  	destroy_workqueue(kacpi_notify_wq);
> +	destroy_workqueue(kacpi_hotplug_wq);
>  
>  	return AE_OK;
>  }
> @@ -716,6 +724,7 @@ static acpi_status __acpi_os_execute(acpi_execute_type type,
>  	acpi_status status = AE_OK;
>  	struct acpi_os_dpc *dpc;
>  	struct workqueue_struct *queue;
> +	work_func_t func;
>  	int ret;
>  	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
>  			  "Scheduling function [%p(%p)] for deferred execution.\n",
> @@ -740,15 +749,11 @@ static acpi_status __acpi_os_execute(acpi_execute_type type,
>  	dpc->function = function;
>  	dpc->context = context;
>  
> -	if (!hp) {
> -		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> -		queue = (type == OSL_NOTIFY_HANDLER) ?
> -			kacpi_notify_wq : kacpid_wq;
> -		ret = queue_work(queue, &dpc->work);
> -	} else {
> -		INIT_WORK(&dpc->work, acpi_os_execute_hp_deferred);
> -		ret = schedule_work(&dpc->work);
> -	}
> +	queue = hp ? kacpi_hotplug_wq :
> +		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> +	func = hp ? acpi_os_execute_hp_deferred : acpi_os_execute_deferred;
> +	INIT_WORK(&dpc->work, func);
> +	ret = queue_work(queue, &dpc->work);
>  
>  	if (!ret) {
>  		printk(KERN_ERR PREFIX

Re: [PATCH 2/5] fix a deadlock in hotplug case

[Zhang Rui]

On Tue, 2009-06-23 at 05:54 +0800, Bjorn Helgaas wrote:
> On Sunday 21 June 2009 09:31:15 pm Zhang Rui wrote:
> > we used to run the hotplug code in keventd_wq.
> > But when hot removing the ACPI battery device,
> > power_supply_unregister invokes flush_scheduled_work.
> > This causes a deadlock.
> > Introduce a new workqueue for hotplug in this patch.
> > http://bugzilla.kernel.org/show_bug.cgi?id=13533
> 
> Can you be specific about how the deadlock occurs?  That will
> make it easier to make sure we don't re-introduce the deadlock
> if this design is ever changed in the future.  And I will probably
> try to change this design, so this is really a self-serving
> request :-)
> 
> I suspect the deadlock is something like this:
> 
>   <user removes battery, platform generates system-level notify>
>     acpi_bus_notify
>       blocking_notifier_call_chain
>         acpi_dock_notifier_call
>           acpi_os_hotplug_execute(acpi_dock_deferred_cb, ...)
>             __acpi_os_execute(..., acpi_dock_deferred_cb, ...)
>               schedule_work(<acpi_dock_deferred_cb>)
>                 queue_work(keventd_wq, <acpi_dock_deferred_cb>)
>                   <acpi_dock_deferred_cb queued for execution in keventd_wq>
> 
>   worker_thread(keventd_wq)
>     acpi_os_execute_hp_deferred
>       acpi_dock_deferred_cb
>         dock_notify
>           hotplug_dock_devices
>             dock_remove_acpi_device
>               acpi_bus_trim
>                 acpi_bus_remove
>                   device_release_driver
>                     acpi_device_remove
>                       acpi_battery_remove
>                         sysfs_remove_battery
>                           power_supply_unregister
>                             flush_scheduled_work
>                               flush_workqueue(keventd_wq)
> 
> Now we're waiting for keventd_wq to be flushed, but it won't be
> considered flushed until acpi_os_execute_hp_deferred() completes.
> 
exactly.

> This sort of analysis is obviously way too long and too specific
> for a source code comment, but it would be nice to have it in the
> bugzilla, at least.
> 
> And maybe the changelog could mention that flush_scheduled_work()
> applies to keventd_wq, just to close the loop there.
> 
> The source code comment:
> 
> > +/* 
> > + * run the hotplug code in a seperate workqueue
> > + * to avoid the deadlock issue
> > + */
> 
> should be more specific about what "the deadlock issue" is.
> Maybe something like:
> 
>   /*
>    * We can't run hotplug code in keventd_wq because the hotplug
>    * code may call driver .remove() functions, which often use
>    * flush_scheduled_work() to wait for keventd_wq to be flushed.
>    */
> 
> And it might make more sense to put the comment where the queue
> is used, i.e., in __acpi_os_execute(), rather than at the definition.
> 
good point.
Refreshed patch attached.

we used to run the hotplug code in keventd_wq.
But when hot removing the ACPI battery device,
power_supply_unregister invokes flush_scheduled_work.
This causes a deadlock. i.e
1. When dock is unplugged, all the hotplug code is run on kevent_wq.
2. the hotplug code removes all the child devices of dock device.
3. removing the child device may invoke flush_scheduled_work
4. flush_scheduled_work waits until all the work on kevent_wq to be
   finished, while this will never be true because the hotplug code
   is running on keventd_wq...

Introduce a new workqueue for hotplug in this patch.
http://bugzilla.kernel.org/show_bug.cgi?id=13533

Tested-by: Paul Martin <pm@debian.org>
Tested-by: Vojtech Gondzala <vojtech.gondzala@gmail.com>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 drivers/acpi/osl.c |   25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Index: linux-2.6/drivers/acpi/osl.c
===================================================================
--- linux-2.6.orig/drivers/acpi/osl.c
+++ linux-2.6/drivers/acpi/osl.c
@@ -79,6 +79,7 @@ static acpi_osd_handler acpi_irq_handler
 static void *acpi_irq_context;
 static struct workqueue_struct *kacpid_wq;
 static struct workqueue_struct *kacpi_notify_wq;
+static struct workqueue_struct *kacpi_hotplug_wq;
 
 struct acpi_res_list {
 	resource_size_t start;
@@ -192,8 +193,10 @@ acpi_status acpi_os_initialize1(void)
 {
 	kacpid_wq = create_singlethread_workqueue("kacpid");
 	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
+	kacpi_hotplug_wq = create_singlethread_workqueue("kacpi_hotplug");
 	BUG_ON(!kacpid_wq);
 	BUG_ON(!kacpi_notify_wq);
+	BUG_ON(!kacpi_hotplug_wq);
 	return AE_OK;
 }
 
@@ -206,6 +209,7 @@ acpi_status acpi_os_terminate(void)
 
 	destroy_workqueue(kacpid_wq);
 	destroy_workqueue(kacpi_notify_wq);
+	destroy_workqueue(kacpi_hotplug_wq);
 
 	return AE_OK;
 }
@@ -716,6 +720,7 @@ static acpi_status __acpi_os_execute(acp
 	acpi_status status = AE_OK;
 	struct acpi_os_dpc *dpc;
 	struct workqueue_struct *queue;
+	work_func_t func;
 	int ret;
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.\n",
@@ -740,15 +745,17 @@ static acpi_status __acpi_os_execute(acp
 	dpc->function = function;
 	dpc->context = context;
 
-	if (!hp) {
-		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
-		queue = (type == OSL_NOTIFY_HANDLER) ?
-			kacpi_notify_wq : kacpid_wq;
-		ret = queue_work(queue, &dpc->work);
-	} else {
-		INIT_WORK(&dpc->work, acpi_os_execute_hp_deferred);
-		ret = schedule_work(&dpc->work);
-	}
+	/*
+	 * 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.
+	 */
+	queue = hp ? kacpi_hotplug_wq :
+		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
+	func = hp ? acpi_os_execute_hp_deferred : acpi_os_execute_deferred;
+	INIT_WORK(&dpc->work, func);
+	ret = queue_work(queue, &dpc->work);
 
 	if (!ret) {
 		printk(KERN_ERR PREFIX

Re: [PATCH 2/5] fix a deadlock in hotplug case

[Bjorn Helgaas]

On Monday 22 June 2009 8:20:29 pm Zhang Rui wrote:
> On Tue, 2009-06-23 at 05:54 +0800, Bjorn Helgaas wrote:
> > On Sunday 21 June 2009 09:31:15 pm Zhang Rui wrote:
> > > we used to run the hotplug code in keventd_wq.
> > > But when hot removing the ACPI battery device,
> > > power_supply_unregister invokes flush_scheduled_work.
> > > This causes a deadlock.
> > > Introduce a new workqueue for hotplug in this patch.
> > > http://bugzilla.kernel.org/show_bug.cgi?id=13533
> > 
> > Can you be specific about how the deadlock occurs?  That will
> > make it easier to make sure we don't re-introduce the deadlock
> > if this design is ever changed in the future.  And I will probably
> > try to change this design, so this is really a self-serving
> > request :-)
> > 
> > I suspect the deadlock is something like this:
> > 
> >   <user removes battery, platform generates system-level notify>
> >     acpi_bus_notify
> >       blocking_notifier_call_chain
> >         acpi_dock_notifier_call
> >           acpi_os_hotplug_execute(acpi_dock_deferred_cb, ...)
> >             __acpi_os_execute(..., acpi_dock_deferred_cb, ...)
> >               schedule_work(<acpi_dock_deferred_cb>)
> >                 queue_work(keventd_wq, <acpi_dock_deferred_cb>)
> >                   <acpi_dock_deferred_cb queued for execution in keventd_wq>
> > 
> >   worker_thread(keventd_wq)
> >     acpi_os_execute_hp_deferred
> >       acpi_dock_deferred_cb
> >         dock_notify
> >           hotplug_dock_devices
> >             dock_remove_acpi_device
> >               acpi_bus_trim
> >                 acpi_bus_remove
> >                   device_release_driver
> >                     acpi_device_remove
> >                       acpi_battery_remove
> >                         sysfs_remove_battery
> >                           power_supply_unregister
> >                             flush_scheduled_work
> >                               flush_workqueue(keventd_wq)
> > 
> > Now we're waiting for keventd_wq to be flushed, but it won't be
> > considered flushed until acpi_os_execute_hp_deferred() completes.
> > 
> exactly.
> 
> > This sort of analysis is obviously way too long and too specific
> > for a source code comment, but it would be nice to have it in the
> > bugzilla, at least.
> > 
> > And maybe the changelog could mention that flush_scheduled_work()
> > applies to keventd_wq, just to close the loop there.
> > 
> > The source code comment:
> > 
> > > +/* 
> > > + * run the hotplug code in a seperate workqueue
> > > + * to avoid the deadlock issue
> > > + */
> > 
> > should be more specific about what "the deadlock issue" is.
> > Maybe something like:
> > 
> >   /*
> >    * We can't run hotplug code in keventd_wq because the hotplug
> >    * code may call driver .remove() functions, which often use
> >    * flush_scheduled_work() to wait for keventd_wq to be flushed.
> >    */
> > 
> > And it might make more sense to put the comment where the queue
> > is used, i.e., in __acpi_os_execute(), rather than at the definition.
> > 
> good point.
> Refreshed patch attached.

Thanks!  Your changelog makes it a lot more clear.  I added the
full analysis from above to the bugzilla as well.

Reviewed-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

> we used to run the hotplug code in keventd_wq.
> But when hot removing the ACPI battery device,
> power_supply_unregister invokes flush_scheduled_work.
> This causes a deadlock. i.e
> 1. When dock is unplugged, all the hotplug code is run on kevent_wq.
> 2. the hotplug code removes all the child devices of dock device.
> 3. removing the child device may invoke flush_scheduled_work
> 4. flush_scheduled_work waits until all the work on kevent_wq to be
>    finished, while this will never be true because the hotplug code
>    is running on keventd_wq...
> 
> Introduce a new workqueue for hotplug in this patch.
> http://bugzilla.kernel.org/show_bug.cgi?id=13533
> 
> Tested-by: Paul Martin <pm@debian.org>
> Tested-by: Vojtech Gondzala <vojtech.gondzala@gmail.com>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---
>  drivers/acpi/osl.c |   25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> Index: linux-2.6/drivers/acpi/osl.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/osl.c
> +++ linux-2.6/drivers/acpi/osl.c
> @@ -79,6 +79,7 @@ static acpi_osd_handler acpi_irq_handler
>  static void *acpi_irq_context;
>  static struct workqueue_struct *kacpid_wq;
>  static struct workqueue_struct *kacpi_notify_wq;
> +static struct workqueue_struct *kacpi_hotplug_wq;
>  
>  struct acpi_res_list {
>  	resource_size_t start;
> @@ -192,8 +193,10 @@ acpi_status acpi_os_initialize1(void)
>  {
>  	kacpid_wq = create_singlethread_workqueue("kacpid");
>  	kacpi_notify_wq = create_singlethread_workqueue("kacpi_notify");
> +	kacpi_hotplug_wq = create_singlethread_workqueue("kacpi_hotplug");
>  	BUG_ON(!kacpid_wq);
>  	BUG_ON(!kacpi_notify_wq);
> +	BUG_ON(!kacpi_hotplug_wq);
>  	return AE_OK;
>  }
>  
> @@ -206,6 +209,7 @@ acpi_status acpi_os_terminate(void)
>  
>  	destroy_workqueue(kacpid_wq);
>  	destroy_workqueue(kacpi_notify_wq);
> +	destroy_workqueue(kacpi_hotplug_wq);
>  
>  	return AE_OK;
>  }
> @@ -716,6 +720,7 @@ static acpi_status __acpi_os_execute(acp
>  	acpi_status status = AE_OK;
>  	struct acpi_os_dpc *dpc;
>  	struct workqueue_struct *queue;
> +	work_func_t func;
>  	int ret;
>  	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
>  			  "Scheduling function [%p(%p)] for deferred execution.\n",
> @@ -740,15 +745,17 @@ static acpi_status __acpi_os_execute(acp
>  	dpc->function = function;
>  	dpc->context = context;
>  
> -	if (!hp) {
> -		INIT_WORK(&dpc->work, acpi_os_execute_deferred);
> -		queue = (type == OSL_NOTIFY_HANDLER) ?
> -			kacpi_notify_wq : kacpid_wq;
> -		ret = queue_work(queue, &dpc->work);
> -	} else {
> -		INIT_WORK(&dpc->work, acpi_os_execute_hp_deferred);
> -		ret = schedule_work(&dpc->work);
> -	}
> +	/*
> +	 * 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.
> +	 */
> +	queue = hp ? kacpi_hotplug_wq :
> +		(type == OSL_NOTIFY_HANDLER ? kacpi_notify_wq : kacpid_wq);
> +	func = hp ? acpi_os_execute_hp_deferred : acpi_os_execute_deferred;
> +	INIT_WORK(&dpc->work, func);
> +	ret = queue_work(queue, &dpc->work);
>  
>  	if (!ret) {
>  		printk(KERN_ERR PREFIX
> 
> 
> 

Re: [PATCH 2/5] fix a deadlock in hotplug case

[Len Brown]

> Reviewed-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

applied

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH 1/5] Show the physical device node of backlight class device.

[Len Brown]

applied

thanks,
Len Brown, Intel Open Source Technology Center