On Mon, 2008-12-22 at 09:35 +0800, Shaohua Li wrote:
> On Fri, Dec 19, 2008 at 06:03:10PM +0800, Len Brown wrote:
> > Shaohua,
> >
> > This one failed b/c it put a mutex into the irqs_disabled
> > part of suspend:
> Maybe just delete the lock in wakeup.c. It's not correct in theory, but
> in theory it should be ok.
Convert acpi_device_lock to a mutex to avoid potential race in wakeup
device read/write. Found by Linus.
Signed-off-by: Shaohua Li <shaohua.li@xxxxxxxxx>
---
drivers/acpi/scan.c | 14 +++++++-------
drivers/acpi/sleep/proc.c | 13 ++++---------
drivers/acpi/sleep/sleep.h | 3 +++
drivers/acpi/sleep/wakeup.c | 30 +++++++-----------------------
4 files changed, 21 insertions(+), 39 deletions(-)
Index: linux/drivers/acpi/scan.c
===================================================================
--- linux.orig/drivers/acpi/scan.c 2008-12-19 11:06:56.000000000 +0800
+++ linux/drivers/acpi/scan.c 2009-01-06 11:31:23.000000000 +0800
@@ -23,7 +23,7 @@ extern struct acpi_device *acpi_root;
static LIST_HEAD(acpi_device_list);
static LIST_HEAD(acpi_bus_id_list);
-DEFINE_SPINLOCK(acpi_device_lock);
+DEFINE_MUTEX(acpi_device_lock);
LIST_HEAD(acpi_wakeup_device_list);
struct acpi_device_bus_id{
@@ -440,7 +440,7 @@ static int acpi_device_register(struct a
return -ENOMEM;
}
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
/*
* Find suitable bus_id and instance number in acpi_bus_id_list
* If failed, create one and link it into acpi_bus_id_list
@@ -468,7 +468,7 @@ static int acpi_device_register(struct a
list_add_tail(&device->g_list, &acpi_device_list);
if (device->wakeup.flags.valid)
list_add_tail(&device->wakeup_list, &acpi_wakeup_device_list);
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
if (device->parent)
device->dev.parent = &parent->dev;
@@ -489,20 +489,20 @@ static int acpi_device_register(struct a
device->removal_type = ACPI_BUS_REMOVAL_NORMAL;
return 0;
end:
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
if (device->parent) {
list_del(&device->node);
list_del(&device->g_list);
} else
list_del(&device->g_list);
list_del(&device->wakeup_list);
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
return result;
}
static void acpi_device_unregister(struct acpi_device *device, int type)
{
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
if (device->parent) {
list_del(&device->node);
list_del(&device->g_list);
@@ -510,7 +510,7 @@ static void acpi_device_unregister(struc
list_del(&device->g_list);
list_del(&device->wakeup_list);
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
acpi_detach_data(device->handle, acpi_bus_data_handler);
Index: linux/drivers/acpi/sleep/proc.c
===================================================================
--- linux.orig/drivers/acpi/sleep/proc.c 2008-12-19 11:06:56.000000000 +0800
+++ linux/drivers/acpi/sleep/proc.c 2009-01-06 11:31:23.000000000 +0800
@@ -338,9 +338,6 @@ acpi_system_write_alarm(struct file *fil
}
#endif /* HAVE_ACPI_LEGACY_ALARM */
-extern struct list_head acpi_wakeup_device_list;
-extern spinlock_t acpi_device_lock;
-
static int
acpi_system_wakeup_device_seq_show(struct seq_file *seq, void *offset)
{
@@ -348,7 +345,7 @@ acpi_system_wakeup_device_seq_show(struc
seq_printf(seq, "Device\tS-state\t Status Sysfs node\n");
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev =
container_of(node, struct acpi_device, wakeup_list);
@@ -356,7 +353,6 @@ acpi_system_wakeup_device_seq_show(struc
if (!dev->wakeup.flags.valid)
continue;
- spin_unlock(&acpi_device_lock);
ldev = acpi_get_physical_device(dev->handle);
seq_printf(seq, "%s\t S%d\t%c%-8s ",
@@ -371,9 +367,8 @@ acpi_system_wakeup_device_seq_show(struc
seq_printf(seq, "\n");
put_device(ldev);
- spin_lock(&acpi_device_lock);
}
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
return 0;
}
@@ -404,7 +399,7 @@ acpi_system_write_wakeup_device(struct f
strbuf[len] = '\0';
sscanf(strbuf, "%s", str);
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev =
container_of(node, struct acpi_device, wakeup_list);
@@ -441,7 +436,7 @@ acpi_system_write_wakeup_device(struct f
}
}
}
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
return count;
}
Index: linux/drivers/acpi/sleep/sleep.h
===================================================================
--- linux.orig/drivers/acpi/sleep/sleep.h 2008-12-09 16:13:38.000000000 +0800
+++ linux/drivers/acpi/sleep/sleep.h 2009-01-06 11:31:23.000000000 +0800
@@ -5,3 +5,6 @@ extern int acpi_suspend (u32 state);
extern void acpi_enable_wakeup_device_prep(u8 sleep_state);
extern void acpi_enable_wakeup_device(u8 sleep_state);
extern void acpi_disable_wakeup_device(u8 sleep_state);
+
+extern struct list_head acpi_wakeup_device_list;
+extern struct mutex acpi_device_lock;
Index: linux/drivers/acpi/sleep/wakeup.c
===================================================================
--- linux.orig/drivers/acpi/sleep/wakeup.c 2008-12-19 11:06:56.000000000 +0800
+++ linux/drivers/acpi/sleep/wakeup.c 2009-01-06 11:43:25.000000000 +0800
@@ -11,12 +11,14 @@
#include <acpi/acevents.h>
#include "sleep.h"
+/*
+ * We didn't lock acpi_device_lock in the file, because it invokes oops in
+ * suspend/resume and isn't really required as this is called in S-state. At
+ * that time, there is no device hotplug
+ **/
#define _COMPONENT ACPI_SYSTEM_COMPONENT
ACPI_MODULE_NAME("wakeup_devices")
-extern struct list_head acpi_wakeup_device_list;
-extern spinlock_t acpi_device_lock;
-
/**
* acpi_enable_wakeup_device_prep - prepare wakeup devices
* @sleep_state: ACPI state
@@ -30,7 +32,6 @@ void acpi_enable_wakeup_device_prep(u8 s
ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device_prep");
- spin_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev = container_of(node,
struct acpi_device,
@@ -41,11 +42,8 @@ void acpi_enable_wakeup_device_prep(u8 s
(sleep_state > (u32) dev->wakeup.sleep_state))
continue;
- spin_unlock(&acpi_device_lock);
acpi_enable_wakeup_device_power(dev, sleep_state);
- spin_lock(&acpi_device_lock);
}
- spin_unlock(&acpi_device_lock);
}
/**
@@ -62,7 +60,6 @@ void acpi_enable_wakeup_device(u8 sleep_
* Refer ACPI2.0: P212
*/
ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device");
- spin_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev =
container_of(node, struct acpi_device, wakeup_list);
@@ -76,22 +73,17 @@ void acpi_enable_wakeup_device(u8 sleep_
if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared)
|| sleep_state > (u32) dev->wakeup.sleep_state) {
if (dev->wakeup.flags.run_wake) {
- spin_unlock(&acpi_device_lock);
/* set_gpe_type will disable GPE, leave it like that */
acpi_set_gpe_type(dev->wakeup.gpe_device,
dev->wakeup.gpe_number,
ACPI_GPE_TYPE_RUNTIME);
- spin_lock(&acpi_device_lock);
}
continue;
}
- spin_unlock(&acpi_device_lock);
if (!dev->wakeup.flags.run_wake)
acpi_enable_gpe(dev->wakeup.gpe_device,
dev->wakeup.gpe_number);
- spin_lock(&acpi_device_lock);
}
- spin_unlock(&acpi_device_lock);
}
/**
@@ -105,7 +97,6 @@ void acpi_disable_wakeup_device(u8 sleep
ACPI_FUNCTION_TRACE("acpi_disable_wakeup_device");
- spin_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev =
container_of(node, struct acpi_device, wakeup_list);
@@ -116,19 +107,16 @@ void acpi_disable_wakeup_device(u8 sleep
if ((!dev->wakeup.state.enabled && !dev->wakeup.flags.prepared)
|| sleep_state > (u32) dev->wakeup.sleep_state) {
if (dev->wakeup.flags.run_wake) {
- spin_unlock(&acpi_device_lock);
acpi_set_gpe_type(dev->wakeup.gpe_device,
dev->wakeup.gpe_number,
ACPI_GPE_TYPE_WAKE_RUN);
/* Re-enable it, since set_gpe_type will disable it */
acpi_enable_gpe(dev->wakeup.gpe_device,
dev->wakeup.gpe_number);
- spin_lock(&acpi_device_lock);
}
continue;
}
- spin_unlock(&acpi_device_lock);
acpi_disable_wakeup_device_power(dev);
/* Never disable run-wake GPE */
if (!dev->wakeup.flags.run_wake) {
@@ -137,9 +125,7 @@ void acpi_disable_wakeup_device(u8 sleep
acpi_clear_gpe(dev->wakeup.gpe_device,
dev->wakeup.gpe_number, ACPI_NOT_ISR);
}
- spin_lock(&acpi_device_lock);
}
- spin_unlock(&acpi_device_lock);
}
static int __init acpi_wakeup_device_init(void)
@@ -149,7 +135,7 @@ static int __init acpi_wakeup_device_ini
if (acpi_disabled)
return 0;
- spin_lock(&acpi_device_lock);
+ mutex_lock(&acpi_device_lock);
list_for_each_safe(node, next, &acpi_wakeup_device_list) {
struct acpi_device *dev = container_of(node,
struct acpi_device,
@@ -157,16 +143,14 @@ static int __init acpi_wakeup_device_ini
/* In case user doesn't load button driver */
if (!dev->wakeup.flags.run_wake || dev->wakeup.state.enabled)
continue;
- spin_unlock(&acpi_device_lock);
acpi_set_gpe_type(dev->wakeup.gpe_device,
dev->wakeup.gpe_number,
ACPI_GPE_TYPE_WAKE_RUN);
acpi_enable_gpe(dev->wakeup.gpe_device,
dev->wakeup.gpe_number);
dev->wakeup.state.enabled = 1;
- spin_lock(&acpi_device_lock);
}
- spin_unlock(&acpi_device_lock);
+ mutex_unlock(&acpi_device_lock);
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
