2006-03-18 13:24:13

by Luming Yu

[permalink] [raw]
Subject: RE: 2.6.16-rc5: known regressions [TP 600X S3, vanilla DSDT]

>> The short-term proper way could be:
>> 1. add a global variable: acpi_in_suspend.
>> 2. in acpi_pm_prepare:
>> a.call acpi_os_wait_events_complete()
>> b.set acpi_in_suspend = YES.
>> in acpi_pm_finish :
>> set acpi_in_suspend = NO.
>> 3. in acpi_thermal_run:
>> if (acpi_in_suspend == YES)
>> do nothing.
>
>I tested the included diff to implement the above short-term fix. It
>also hung on the second sleep. BUT, it's the same reason that the
>utils.c change didn't help: because acpi_thermal_add() was loading
>THM[0267]. After the usual modification to acpi_thermal_add() to have
>it ignore THM[267], the system didn't hang (12 cycles). Which is
>progress.

Hmm, probably, you need to do :

4. in acpi_thermal_notify,
if (acpi_in_suspend == YES)
do nothing.


2006-03-18 14:38:22

by Sanjoy Mahajan

[permalink] [raw]
Subject: Re: 2.6.16-rc5: known regressions [TP 600X S3, vanilla DSDT]

> Hmm, probably, you need to do :
>
> 4. in acpi_thermal_notify,
> if (acpi_in_suspend == YES)
> do nothing.

I've just tested that. It suspended twice without problem, which made
me think the problem was solved. But it hung on the third suspend!

I placed all the source under revision control, since I was spending
more time moving versions of files back and forth (and fixing the
inevitable mistakes, forgetting which version was which) than I would by
figuring out how to use the SCM. So here is its generated diff between
the vanilla kernel (with config file that uses vanilla DSDT) and the
kernel that I just tested.

As you can see, the only change is the short-term fix including item 4
above. It doesn't do anything else, e.g. there's no code to load just
THM0 (which is probably why it hung).

Perhaps the other thermal zones have different problems, or maybe
there's yet another source of thermal method calls?

-Sanjoy


diff -r ac486e270597 -r 03c54e90f75d drivers/acpi/sleep/main.c
--- a/drivers/acpi/sleep/main.c Sat Mar 18 08:35:34 2006 -0500
+++ b/drivers/acpi/sleep/main.c Sat Mar 18 09:08:04 2006 -0500
@@ -19,6 +19,12 @@
#include <acpi/acpi_drivers.h>
#include "sleep.h"

+/* for functions putting machine to sleep to know that we're
+ suspending, so that they can careful about what AML methods they
+ invoke (to avoid trying untested BIOS code paths) */
+int acpi_in_suspend;
+EXPORT_SYMBOL(acpi_in_suspend);
+
u8 sleep_states[ACPI_S_STATE_COUNT];

static struct pm_ops acpi_pm_ops;
@@ -55,6 +61,8 @@ static int acpi_pm_prepare(suspend_state
printk("acpi_pm_prepare does not support %d \n", pm_state);
return -EPERM;
}
+ acpi_os_wait_events_complete(NULL);
+ acpi_in_suspend = TRUE;
return acpi_sleep_prepare(acpi_state);
}

@@ -131,6 +139,7 @@ static int acpi_pm_finish(suspend_state_
{
u32 acpi_state = acpi_suspend_states[pm_state];

+ acpi_in_suspend = FALSE;
acpi_leave_sleep_state(acpi_state);
acpi_disable_wakeup_device(acpi_state);

diff -r ac486e270597 -r 03c54e90f75d drivers/acpi/thermal.c
--- a/drivers/acpi/thermal.c Sat Mar 18 08:35:34 2006 -0500
+++ b/drivers/acpi/thermal.c Sat Mar 18 09:08:04 2006 -0500
@@ -79,6 +79,8 @@ static int tzp;
static int tzp;
module_param(tzp, int, 0);
MODULE_PARM_DESC(tzp, "Thermal zone polling frequency, in 1/10 seconds.\n");
+
+extern int acpi_in_suspend;

static int acpi_thermal_add(struct acpi_device *device);
static int acpi_thermal_remove(struct acpi_device *device, int type);
@@ -683,6 +685,8 @@ static void acpi_thermal_run(unsigned lo
static void acpi_thermal_run(unsigned long data)
{
struct acpi_thermal *tz = (struct acpi_thermal *)data;
+ if (acpi_in_suspend) /* thermal methods might cause a hang */
+ return; /* so don't do them */
if (!tz->zombie)
acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
acpi_thermal_check, (void *)data);
@@ -1224,6 +1228,9 @@ static void acpi_thermal_notify(acpi_han
struct acpi_device *device = NULL;

ACPI_FUNCTION_TRACE("acpi_thermal_notify");
+
+ if (acpi_in_suspend) /* thermal methods might cause a hang */
+ return_VOID; /* so don't do them */

if (!tz)
return_VOID;