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.
Thanks,
Shaohua
> ...
> CPU1 is down
> Extended CMOS year: 2000
> BUG: sleeping function called from invalid context at kernel/mutex.c:88
> in_atomic(): 0, irqs_disabled(): 1, pid: 3872, name: bash
> Pid: 3872, comm: bash Not tainted 2.6.28-rc9-09771-gbe5a7e4 #79
> Call Trace:
> [<ffffffff8022c0e4>] __might_sleep+0xcf/0xd1
> [<ffffffff80584a34>] mutex_lock+0x1d/0x33
> [<ffffffff803b1f91>] acpi_enable_wakeup_device+0x19/0x9e
> [<ffffffff803b253b>] acpi_suspend_enter+0x37/0xd2
> [<ffffffff8025bedd>] suspend_devices_and_enter+0x120/0x1b3
> [<ffffffff8025c199>] enter_state+0x200/0x290
> [<ffffffff8025c2e0>] state_store+0xb7/0xd8
> [<ffffffff80371b9b>] kobj_attr_store+0x17/0x19
> [<ffffffff802d8c75>] sysfs_write_file+0xdf/0x114
> [<ffffffff8029518d>] vfs_write+0xae/0x137
> [<ffffffff802952da>] sys_write+0x47/0x70
> [<ffffffff8020b3db>] system_call_fastpath+0x16/0x1b
> Back to C!
>
> -- Len Brown, Intel Open Source Technology Center
>
>
> > On Tue, 9 Dec 2008, Shaohua Li wrote:
> >
> > > Change acpi_device_lock to a mutex to avoid 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 | 31 ++++++++-----------------------
> > > 4 files changed, 22 insertions(+), 39 deletions(-)
> > >
> > > Index: linux/drivers/acpi/scan.c
> > > ===================================================================
> > > --- linux.orig/drivers/acpi/scan.c 2008-12-09 10:34:00.000000000 +0800
> > > +++ linux/drivers/acpi/scan.c 2008-12-09 10:36:07.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-09 10:34:18.000000000 +0800
> > > +++ linux/drivers/acpi/sleep/proc.c 2008-12-09 10:40:10.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 10:37:22.000000000 +0800
> > > +++ linux/drivers/acpi/sleep/sleep.h 2008-12-09 10:37:50.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-09 10:34:08.000000000 +0800
> > > +++ linux/drivers/acpi/sleep/wakeup.c 2008-12-09 10:39:18.000000000 +0800
> > > @@ -14,9 +14,6 @@
> > > #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 +27,7 @@ void acpi_enable_wakeup_device_prep(u8 s
> > >
> > > ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device_prep");
> > >
> > > - 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,
> > > @@ -41,11 +38,9 @@ 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);
> > > + mutex_unlock(&acpi_device_lock);
> > > }
> > >
> > > /**
> > > @@ -62,7 +57,7 @@ void acpi_enable_wakeup_device(u8 sleep_
> > > * Refer ACPI2.0: P212
> > > */
> > > ACPI_FUNCTION_TRACE("acpi_enable_wakeup_device");
> > > - 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);
> > > @@ -76,22 +71,18 @@ 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);
> > > + mutex_unlock(&acpi_device_lock);
> > > }
> > >
> > > /**
> > > @@ -105,7 +96,7 @@ void acpi_disable_wakeup_device(u8 sleep
> > >
> > > ACPI_FUNCTION_TRACE("acpi_disable_wakeup_device");
> > >
> > > - 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);
> > > @@ -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,8 @@ 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);
> > > + mutex_unlock(&acpi_device_lock);
> > > }
> > >
> > > static int __init acpi_wakeup_device_init(void)
> > > @@ -149,7 +136,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 +144,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
> > >
> > --
> > 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
> >
> --
> 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
--
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
