RE: Power-button event after resume from S3

RE: Power-button event after resume from S3

[Yu, Luming]

>Before working on a patch I'd like to be sure that there is a general
>consensus that this is indeed The Right Way (TM) to fix the problem.
>Whose blessing would a patch need before being accepted?

I think this is the simplest and right way to fix this problem.
And, it should be accepted. :-)

>> Probably, you need to hack acpi_button_notify.
>> like:
>> 
>> if (acpi_in_suspend)
>> 	don't generate event.

More precisely,  it should be:
acpi_button_notify:
	if(acpi_during_suspend_resume)
		don't generate power button event to confuse user space
daemon,

Re: Power-button event after resume from S3

[Sanjoy Mahajan]

>  if(acpi_during_suspend_resume)
>	don't generate power button event to confuse user space daemon

This patch might be useful useful in setting and unsetting
acpi_during_suspend_resume (and also using it, but you should ignore
those as they are hacks for a different problem).  The equivalent
variable in the patch below is acpi_in_suspend.

diff -r ac486e270597 -r abd89292c539 drivers/acpi/osl.c
--- a/drivers/acpi/osl.c	Sat Mar 18 08:35:34 2006 -0500
+++ b/drivers/acpi/osl.c	Thu Mar 30 10:59:57 2006 -0500
@@ -634,6 +634,8 @@ static void acpi_os_execute_deferred(voi
 	return_VOID;
 }
 
+extern int acpi_in_suspend;
+
 acpi_status
 acpi_os_queue_for_execution(u32 priority,
 			    acpi_osd_exec_callback function, void *context)
@@ -643,6 +645,8 @@ acpi_os_queue_for_execution(u32 priority
 	struct work_struct *task;
 
 	ACPI_FUNCTION_TRACE("os_queue_for_execution");
+	if (acpi_in_suspend)	/* in case kacpid is causing the queue */
+		return_ACPI_STATUS(AE_OK);
 
 	ACPI_DEBUG_PRINT((ACPI_DB_EXEC,
 			  "Scheduling function [%p(%p)] for deferred execution.
\n",
diff -r ac486e270597 -r abd89292c539 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	Thu Mar 30 10:59:57 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);
 }
 
@@ -132,6 +140,7 @@ static int acpi_pm_finish(suspend_state_
 	u32 acpi_state = acpi_suspend_states[pm_state];
 
 	acpi_leave_sleep_state(acpi_state);
+	acpi_in_suspend = FALSE;
 	acpi_disable_wakeup_device(acpi_state);
 
 	/* reset firmware waking vector */
diff -r ac486e270597 -r abd89292c539 drivers/acpi/thermal.c
--- a/drivers/acpi/thermal.c	Sat Mar 18 08:35:34 2006 -0500
+++ b/drivers/acpi/thermal.c	Thu Mar 30 10:59:57 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_VOID;	/* so don't do them */
 	if (!tz->zombie)
 		acpi_os_queue_for_execution(OSD_PRIORITY_GPE,
 					    acpi_thermal_check, (void *)data);
@@ -705,6 +709,8 @@ static void acpi_thermal_check(void *dat
 
 	state = tz->state;
 
+	if (acpi_in_suspend)
+		return_VOID;
 	result = acpi_thermal_get_temperature(tz);
 	if (result)
 		return_VOID;
@@ -1224,6 +1230,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;

Re: Power-button event after resume from S3

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 543 bytes --]

Hi.

On Tuesday 11 April 2006 12:35, Sanjoy Mahajan wrote:
> >  if(acpi_during_suspend_resume)
> >	don't generate power button event to confuse user space daemon
>
> This patch might be useful useful in setting and unsetting
> acpi_during_suspend_resume (and also using it, but you should ignore
> those as they are hacks for a different problem).  The equivalent
> variable in the patch below is acpi_in_suspend.

If you want this to get wider testing, I'll be happy to include it in the 
Suspend2 patchset.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Power-button event after resume from S3

[Felix Kuehling]

On Tue, 2006-11-04 at 14:14 +1000, Nigel Cunningham wrote:
> Hi.
> 
> On Tuesday 11 April 2006 12:35, Sanjoy Mahajan wrote:
> > >  if(acpi_during_suspend_resume)
> > >	don't generate power button event to confuse user space daemon
> >
> > This patch might be useful useful in setting and unsetting
> > acpi_during_suspend_resume (and also using it, but you should ignore
> > those as they are hacks for a different problem).  The equivalent
> > variable in the patch below is acpi_in_suspend.
> 
> If you want this to get wider testing, I'll be happy to include it in the 
> Suspend2 patchset.

Just to clarify, software suspend is not affected by this problem. Only
S3, possibly S1 and maybe S4BIOS are affected, though I've never seen
the latter on any real system.

Regards,
  Felix

> 
> Regards,
> 
> Nigel

Re: Power-button event after resume from S3

[Nigel Cunningham]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

Hi.

On Friday 14 April 2006 00:28, Felix Kuehling wrote:
> On Tue, 2006-11-04 at 14:14 +1000, Nigel Cunningham wrote:
> > Hi.
> >
> > On Tuesday 11 April 2006 12:35, Sanjoy Mahajan wrote:
> > > >  if(acpi_during_suspend_resume)
> > > >	don't generate power button event to confuse user space daemon
> > >
> > > This patch might be useful useful in setting and unsetting
> > > acpi_during_suspend_resume (and also using it, but you should ignore
> > > those as they are hacks for a different problem).  The equivalent
> > > variable in the patch below is acpi_in_suspend.
> >
> > If you want this to get wider testing, I'll be happy to include it in the
> > Suspend2 patchset.
>
> Just to clarify, software suspend is not affected by this problem. Only
> S3, possibly S1 and maybe S4BIOS are affected, though I've never seen
> the latter on any real system.

Thanks. I still think it would be worth including, because Suspend2 (and now 
uswsusp, I believe) lets you choose what to do after writing the image. One 
of the options is suspending to ram, so that you effectively get disk-backed 
suspend to ram. It might be useful in this situation.

Regards,

Nigel

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: Power-button event after resume from S3

[Rafael J. Wysocki]

Hi,

On Friday 14 April 2006 07:33, Nigel Cunningham wrote:
> On Friday 14 April 2006 00:28, Felix Kuehling wrote:
> > On Tue, 2006-11-04 at 14:14 +1000, Nigel Cunningham wrote:
> > > Hi.
> > >
> > > On Tuesday 11 April 2006 12:35, Sanjoy Mahajan wrote:
> > > > >  if(acpi_during_suspend_resume)
> > > > >	don't generate power button event to confuse user space daemon
> > > >
> > > > This patch might be useful useful in setting and unsetting
> > > > acpi_during_suspend_resume (and also using it, but you should ignore
> > > > those as they are hacks for a different problem).  The equivalent
> > > > variable in the patch below is acpi_in_suspend.
> > >
> > > If you want this to get wider testing, I'll be happy to include it in the
> > > Suspend2 patchset.
> >
> > Just to clarify, software suspend is not affected by this problem. Only
> > S3, possibly S1 and maybe S4BIOS are affected, though I've never seen
> > the latter on any real system.
> 
> Thanks. I still think it would be worth including, because Suspend2 (and now 
> uswsusp, I believe) lets you choose what to do after writing the image. One 
> of the options is suspending to ram, so that you effectively get disk-backed 
> suspend to ram. It might be useful in this situation.

Yes we support suspend-to-disk-and-RAM now, so it may be useful to us too.

Greetings,
Rafael

RE: Power-button event after resume from S3

[Yu, Luming]

>Are there plans for a real fix for this problem, either in the 
>kernel or
>in acpid?
>

We need a  patch to handle the wakeup power-button event.
Do you want to hack a patch for this?

Probably, you need to hack acpi_button_notify.
like:

if (acpi_in_suspend)
	don't generate event.

--Luming

RE: Power-button event after resume from S3

[Felix Kuehling]

On Fri, 2006-07-04 at 10:49 +0800, Yu, Luming wrote:
> >Are there plans for a real fix for this problem, either in the 
> >kernel or
> >in acpid?
> >
> 
> We need a  patch to handle the wakeup power-button event.
> Do you want to hack a patch for this?

Before working on a patch I'd like to be sure that there is a general
consensus that this is indeed The Right Way (TM) to fix the problem.
Whose blessing would a patch need before being accepted?

Thanks,
  Felix

> 
> Probably, you need to hack acpi_button_notify.
> like:
> 
> if (acpi_in_suspend)
> 	don't generate event.
> 
> --Luming
> 

Power-button event after resume from S3

[Felix Kuehling]

Hi,

I am investigating a problem where acpid receives a power-button event
immediately after resuming from S3. I looked at the mailing list
archives and found that this problem is not entirely uncommon. The
advice users got so far all looked like workarounds for the real
problem. The same goes for
http://bugzilla.kernel.org/show_bug.cgi?id=3525, which further declares
it as a bug in acpid. However, I'm not sure it is really a bug in acpid.

This is my understanding:

The only ways for acpid to distinguish power-up and power-down are hacks
that somehow remember that the system is going into suspend and to
ignore the next power button event. However, there is no guarantee that
there will be such an event after resuming, so you need some kind of a
time-out. All that doesn't feel very robust to me and that's why I'm
calling it a hack.

The kernel on the other hand has more information. It could reliably
distinguish power-up and power-down events. The first SCI interrupt
after resume always seems to be the power-up event, no time-out
required. Thus it could generate different events for power-up and
power-down in /proc/acpi/event. That would make it much easier and
cleaner for acpid to distinguish them and take the appropriate action.

Finally my assumption is that it is correct behaviour for the ACPI
hardware to report the power-button to the OS on resuming from S3. I
came to that conclusion after reading parts of the ACPI 2.0 spec. Please
confirm.

Are there plans for a real fix for this problem, either in the kernel or
in acpid?

Thanks,
  Felix

P.S.: Please include the CC-list on replies.

Re: Power-button event after resume from S3

[Sanjoy Mahajan]

> I am investigating a problem where acpid receives a power-button
> event immediately after resuming from S3. I looked at the mailing
> list archives and found that this problem is not entirely uncommon.

Yes, I noticed this problem once I turned on acpi_serialize and told
ACPI to pretend I'm running "Microsoft Windows".  Then the Fn key no
longer resumed from S3, and I had to slide the power switch, which
generated wakeup and powerbutton events -- so it woke up and then shut
down.  [this is with a TP 600X]

-Sanjoy

`Never underestimate the evil of which men of power are capable.'
         --Bertrand Russell, _War Crimes in Vietnam_, chapter 1.