[PATCH 1/2] ACPICA: Clear power button status before enabling event

[PATCH 1/2] ACPICA: Clear power button status before enabling event

[Bjorn Helgaas]

Clear power button status before enabling event.

It's unusual to enable an event, then immediately clear it, so this
looks like a possible bug.  If it was intentional, perhaps a comment
would be in order.

This patch may be used under either the GPL v2 or the BSD-style license
used for the Intel ACPICA.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
CC: Bob Moore <robert.moore@intel.com>
---
 drivers/acpi/acpica/hwsleep.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index db307a3..3558c53 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -613,12 +613,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 	(void)
 	    acpi_write_bit_register(acpi_gbl_fixed_event_info
 			      [ACPI_EVENT_POWER_BUTTON].
-			      enable_register_id, ACPI_ENABLE_EVENT);
+			      status_register_id, ACPI_CLEAR_STATUS);
 
 	(void)
 	    acpi_write_bit_register(acpi_gbl_fixed_event_info
 			      [ACPI_EVENT_POWER_BUTTON].
-			      status_register_id, ACPI_CLEAR_STATUS);
+			      enable_register_id, ACPI_ENABLE_EVENT);
 
 	arg.integer.value = ACPI_SST_WORKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);

[PATCH 2/2] ACPICA: Use fixed event wrappers to enable/disable/clear

[Bjorn Helgaas]

Use acpi_enable_event(), acpi_disable_event(), and acpi_clear_event()
instead of writing the register directly.

This patch may be used under either the GPL v2 or the BSD-style license
used for the Intel ACPICA.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
CC: Bob Moore <robert.moore@intel.com>
---
 drivers/acpi/acpica/evevent.c |   13 +++----------
 drivers/acpi/acpica/hwsleep.c |   12 ++----------
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/acpica/evevent.c b/drivers/acpi/acpica/evevent.c
index cd55c77..5df6af7 100644
--- a/drivers/acpi/acpica/evevent.c
+++ b/drivers/acpi/acpica/evevent.c
@@ -203,10 +203,7 @@ static acpi_status acpi_ev_fixed_event_initialize(void)
 		/* Disable the fixed event */
 
 		if (acpi_gbl_fixed_event_info[i].enable_register_id != 0xFF) {
-			status =
-			    acpi_write_bit_register(acpi_gbl_fixed_event_info
-						    [i].enable_register_id,
-						    ACPI_DISABLE_EVENT);
+			status = acpi_disable_event(i, 0);
 			if (ACPI_FAILURE(status)) {
 				return (status);
 			}
@@ -288,18 +285,14 @@ static u32 acpi_ev_fixed_event_dispatch(u32 event)
 	ACPI_FUNCTION_ENTRY();
 
 	/* Clear the status bit */
-
-	(void)acpi_write_bit_register(acpi_gbl_fixed_event_info[event].
-				      status_register_id, ACPI_CLEAR_STATUS);
+	acpi_clear_event(event);
 
 	/*
 	 * Make sure we've got a handler. If not, report an error. The event is
 	 * disabled to prevent further interrupts.
 	 */
 	if (NULL == acpi_gbl_fixed_event_handlers[event].handler) {
-		(void)acpi_write_bit_register(acpi_gbl_fixed_event_info[event].
-					      enable_register_id,
-					      ACPI_DISABLE_EVENT);
+		acpi_disable_event(event, 0);
 
 		ACPI_ERROR((AE_INFO,
 			    "No installed handler for fixed event [%08X]",
diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index 3558c53..44ae73e 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -609,16 +609,8 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 	acpi_gbl_system_awake_and_running = TRUE;
 
 	/* Enable power button */
-
-	(void)
-	    acpi_write_bit_register(acpi_gbl_fixed_event_info
-			      [ACPI_EVENT_POWER_BUTTON].
-			      status_register_id, ACPI_CLEAR_STATUS);
-
-	(void)
-	    acpi_write_bit_register(acpi_gbl_fixed_event_info
-			      [ACPI_EVENT_POWER_BUTTON].
-			      enable_register_id, ACPI_ENABLE_EVENT);
+	acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
+	acpi_enable_event(ACPI_EVENT_POWER_BUTTON, 0);
 
 	arg.integer.value = ACPI_SST_WORKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[yakui_zhao]

On Tue, 2009-06-16 at 00:49 +0800, Bjorn Helgaas wrote:
> Clear power button status before enabling event.
> 
> It's unusual to enable an event, then immediately clear it, so this
> looks like a possible bug.  If it was intentional, perhaps a comment
> would be in order.
IMO this patch is unnecessary.
It seems that we will clear the power button event immediately after it
is resumed from OS. (This is done in the function of
acpi_suspend_enter).

Maybe the power event status bit is set before we re-enable the event
bit. And after we re-enable the power button event, OS can handle the
power button event (the acpi_leave_sleep_state is called with interrupts
enabled). 

If the patch is applied, the power button event will be lost.

Thanks.
> 
> This patch may be used under either the GPL v2 or the BSD-style license
> used for the Intel ACPICA.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> CC: Bob Moore <robert.moore@intel.com>
> ---
>  drivers/acpi/acpica/hwsleep.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> index db307a3..3558c53 100644
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -613,12 +613,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
>  	(void)
>  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
>  			      [ACPI_EVENT_POWER_BUTTON].
> -			      enable_register_id, ACPI_ENABLE_EVENT);
> +			      status_register_id, ACPI_CLEAR_STATUS);
>  
>  	(void)
>  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
>  			      [ACPI_EVENT_POWER_BUTTON].
> -			      status_register_id, ACPI_CLEAR_STATUS);
> +			      enable_register_id, ACPI_ENABLE_EVENT);
>  
>  	arg.integer.value = ACPI_SST_WORKING;
>  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[Bjorn Helgaas]

On Tuesday 16 June 2009 9:21:40 pm yakui_zhao wrote:
> On Tue, 2009-06-16 at 00:49 +0800, Bjorn Helgaas wrote:
> > Clear power button status before enabling event.
> > 
> > It's unusual to enable an event, then immediately clear it, so this
> > looks like a possible bug.  If it was intentional, perhaps a comment
> > would be in order.
> IMO this patch is unnecessary.

> It seems that we will clear the power button event immediately after it
> is resumed from OS. (This is done in the function of
> acpi_suspend_enter).

A comment in acpi_suspend_enter() refers to ACPI 3.0b sec. 4.7.2.2.1.1,
which says "OSPM responds [to a button press after the button press that
transitioned the system into a sleeping state] by clearing the power button
status bit and waking the system."

So *somebody* has to clear the status bit, but I'm not sure that it has
to be done in the Linux-specific code, e.g,. acpi_suspend_enter().  The
term "OSPM" seems broad enough to include both the ACPI CA and the Linux-
specific code, and it may be more robust to clear it in the CA.

> Maybe the power event status bit is set before we re-enable the event
> bit. And after we re-enable the power button event, OS can handle the
> power button event (the acpi_leave_sleep_state is called with interrupts
> enabled). 
> 
> If the patch is applied, the power button event will be lost.

I think this is the scenario you refer to:

  button press A causes wakeup
  <possible button press B>
  acpi_suspend_enter() clears event
  <possible button press C>
  acpi_leave_sleep_state() clears event
  acpi_leave_sleep_state() enables event
  <possible button press D>

Even without this patch, we would lose button event B.  With this patch,
we would also lose button event C.  This whole sequence should take very
little time, so I'm dubious that there is any value in keeping either
B or C -- it seems they'd most likely be unintentional.

Actually, it seems like it would make the most sense to apply this patch
*and* stop clearing the event in acpi_suspend_enter().  Then the code is
simpler and easier to analyze, because we only touch the button status in
one place.

But I admit I know very little about suspend/resume, so maybe I just
need more convincing :-)

Bjorn

> > This patch may be used under either the GPL v2 or the BSD-style license
> > used for the Intel ACPICA.
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > CC: Bob Moore <robert.moore@intel.com>
> > ---
> >  drivers/acpi/acpica/hwsleep.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> > index db307a3..3558c53 100644
> > --- a/drivers/acpi/acpica/hwsleep.c
> > +++ b/drivers/acpi/acpica/hwsleep.c
> > @@ -613,12 +613,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> >  	(void)
> >  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> >  			      [ACPI_EVENT_POWER_BUTTON].
> > -			      enable_register_id, ACPI_ENABLE_EVENT);
> > +			      status_register_id, ACPI_CLEAR_STATUS);
> >  
> >  	(void)
> >  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> >  			      [ACPI_EVENT_POWER_BUTTON].
> > -			      status_register_id, ACPI_CLEAR_STATUS);
> > +			      enable_register_id, ACPI_ENABLE_EVENT);
> >  
> >  	arg.integer.value = ACPI_SST_WORKING;
> >  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[yakui_zhao]

On Wed, 2009-06-17 at 12:26 +0800, Bjorn Helgaas wrote:
> On Tuesday 16 June 2009 9:21:40 pm yakui_zhao wrote:
> > On Tue, 2009-06-16 at 00:49 +0800, Bjorn Helgaas wrote:
> > > Clear power button status before enabling event.
> > > 
> > > It's unusual to enable an event, then immediately clear it, so this
> > > looks like a possible bug.  If it was intentional, perhaps a comment
> > > would be in order.
> > IMO this patch is unnecessary.
> 
> > It seems that we will clear the power button event immediately after it
> > is resumed from OS. (This is done in the function of
> > acpi_suspend_enter).
> 
> A comment in acpi_suspend_enter() refers to ACPI 3.0b sec. 4.7.2.2.1.1,
> which says "OSPM responds [to a button press after the button press that
> transitioned the system into a sleeping state] by clearing the power button
> status bit and waking the system."
If the power button event is not cleared after finishing the resume, the
acpid will receive the power button event and then power off the system.
So OSPM had better clear the power button event in course of resuming to
avoid that the box is poweroff.
> 
> So *somebody* has to clear the status bit, but I'm not sure that it has
> to be done in the Linux-specific code, e.g,. acpi_suspend_enter().  The
> term "OSPM" seems broad enough to include both the ACPI CA and the Linux-
> specific code, and it may be more robust to clear it in the CA.
> 
> > Maybe the power event status bit is set before we re-enable the event
> > bit. And after we re-enable the power button event, OS can handle the
> > power button event (the acpi_leave_sleep_state is called with interrupts
> > enabled). 
> > 
> > If the patch is applied, the power button event will be lost.
> 
> I think this is the scenario you refer to:
> 
>   button press A causes wakeup
>   <possible button press B>
>   acpi_suspend_enter() clears event
>   <possible button press C>
>   acpi_leave_sleep_state() clears event
>   acpi_leave_sleep_state() enables event
>   <possible button press D>
> 
> Even without this patch, we would lose button event B.  With this patch,
> we would also lose button event C.  This whole sequence should take very
> little time, so I'm dubious that there is any value in keeping either
> B or C -- it seems they'd most likely be unintentional.
Without this patch, the power button event C can be handled.

If the power button event bit will be set in the _WAK object, then this
event can't be handled after applying your patch.
> 
> Actually, it seems like it would make the most sense to apply this patch
> *and* stop clearing the event in acpi_suspend_enter().  Then the code is
> simpler and easier to analyze, because we only touch the button status in
> one place.
IMO it will be better to clear the power button event in the function of
acpi_suspend_enter.
If we don't do that, maybe the BIOS will set the power button event
status/enable bit. And after the interrupt is enabled, the power button
event handler will send the event. In such case the acpid will receive
the event. Maybe the box will be powered off after resuming. This is not
what we wanted.
Thanks.
> 
> But I admit I know very little about suspend/resume, so maybe I just
> need more convincing :-)
> 
> Bjorn
> 
> > > This patch may be used under either the GPL v2 or the BSD-style license
> > > used for the Intel ACPICA.
> > > 
> > > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > > CC: Bob Moore <robert.moore@intel.com>
> > > ---
> > >  drivers/acpi/acpica/hwsleep.c |    4 ++--
> > >  1 files changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> > > index db307a3..3558c53 100644
> > > --- a/drivers/acpi/acpica/hwsleep.c
> > > +++ b/drivers/acpi/acpica/hwsleep.c
> > > @@ -613,12 +613,12 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> > >  	(void)
> > >  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > >  			      [ACPI_EVENT_POWER_BUTTON].
> > > -			      enable_register_id, ACPI_ENABLE_EVENT);
> > > +			      status_register_id, ACPI_CLEAR_STATUS);
> > >  
> > >  	(void)
> > >  	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > >  			      [ACPI_EVENT_POWER_BUTTON].
> > > -			      status_register_id, ACPI_CLEAR_STATUS);
> > > +			      enable_register_id, ACPI_ENABLE_EVENT);
> > >  
> > >  	arg.integer.value = ACPI_SST_WORKING;
> > >  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[Bjorn Helgaas]

On Wednesday 17 June 2009 01:34:38 am yakui_zhao wrote:
> On Wed, 2009-06-17 at 12:26 +0800, Bjorn Helgaas wrote:
> > On Tuesday 16 June 2009 9:21:40 pm yakui_zhao wrote:
> > > On Tue, 2009-06-16 at 00:49 +0800, Bjorn Helgaas wrote:
> > > > Clear power button status before enabling event.
> > > > 
> > > > It's unusual to enable an event, then immediately clear it, so this
> > > > looks like a possible bug.  If it was intentional, perhaps a comment
> > > > would be in order.
> > > IMO this patch is unnecessary.
> > 
> > > It seems that we will clear the power button event immediately after it
> > > is resumed from OS. (This is done in the function of
> > > acpi_suspend_enter).
> > 
> > A comment in acpi_suspend_enter() refers to ACPI 3.0b sec. 4.7.2.2.1.1,
> > which says "OSPM responds [to a button press after the button press that
> > transitioned the system into a sleeping state] by clearing the power button
> > status bit and waking the system."
> If the power button event is not cleared after finishing the resume, the
> acpid will receive the power button event and then power off the system.
> So OSPM had better clear the power button event in course of resuming to
> avoid that the box is poweroff.

I agree.

> > So *somebody* has to clear the status bit, but I'm not sure that it has
> > to be done in the Linux-specific code, e.g,. acpi_suspend_enter().  The
> > term "OSPM" seems broad enough to include both the ACPI CA and the Linux-
> > specific code, and it may be more robust to clear it in the CA.
> > 
> > > Maybe the power event status bit is set before we re-enable the event
> > > bit. And after we re-enable the power button event, OS can handle the
> > > power button event (the acpi_leave_sleep_state is called with interrupts
> > > enabled). 
> > > 
> > > If the patch is applied, the power button event will be lost.
> > 
> > I think this is the scenario you refer to:
> > 
> >   button press A causes wakeup
> >   <possible button press B>
> >   acpi_suspend_enter() clears event
> >   <possible button press C>
      acpi_leave_sleep_state() runs _WAK here
> >   acpi_leave_sleep_state() clears event
> >   acpi_leave_sleep_state() enables event
> >   <possible button press D>
> > 
> > Even without this patch, we would lose button event B.  With this patch,
> > we would also lose button event C.  This whole sequence should take very
> > little time, so I'm dubious that there is any value in keeping either
> > B or C -- it seems they'd most likely be unintentional.
> Without this patch, the power button event C can be handled.
> 
> If the power button event bit will be set in the _WAK object, then this
> event can't be handled after applying your patch.

I agree (because we run _WAK before clearing the power button event).

> > Actually, it seems like it would make the most sense to apply this patch
> > *and* stop clearing the event in acpi_suspend_enter().  Then the code is
> > simpler and easier to analyze, because we only touch the button status in
> > one place.
> IMO it will be better to clear the power button event in the function of
> acpi_suspend_enter.
> If we don't do that, maybe the BIOS will set the power button event
> status/enable bit. And after the interrupt is enabled, the power button
> event handler will send the event. In such case the acpid will receive
> the event. Maybe the box will be powered off after resuming. This is not
> what we wanted.

I agree.

What's your opinion of the following patch?  This drops the clear
from acpi_suspend_enter().  In the ACPI CA acpi_leave_sleep_state(),
it clears first, then enables, and moves both before the _WAK
execution.

  button press A causes wakeup
  <possible button press (or BIOS sets event) B>
  acpi_leave_sleep_state() clears all GPEs
  acpi_leave_sleep_state() enables runtime GPEs
  acpi_leave_sleep_state() clears power button event
  <possible button press C>
  acpi_leave_sleep_state() enables power button event
  <possible button press D>
  acpi_leave_sleep_state() runs _WAK
  <possible button "press" E from _WAK>
  <possible button press F>
  
This should work the same as the current code -- we drop event B
as desired, and we handle all others.

It has the advantages that we remove a bit of code from Linux/ACPI,
we only touch the power button events in one place, and we follow
the same pattern (clear, then enable) as the GPEs.

I'm not sure why acpi_suspend_enter() only clears the event when
the acpi_target_sleep_state == ACPI_STATE_S3.  Sec. 4.7.2.2.1.1
doesn't mention that.

Bjorn


diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
index db307a3..c3252ee 100644
--- a/drivers/acpi/acpica/hwsleep.c
+++ b/drivers/acpi/acpica/hwsleep.c
@@ -592,6 +592,18 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 		return_ACPI_STATUS(status);
 	}
 
+	/* Enable power button */
+
+	(void)
+	    acpi_write_bit_register(acpi_gbl_fixed_event_info
+			      [ACPI_EVENT_POWER_BUTTON].
+			      status_register_id, ACPI_CLEAR_STATUS);
+
+	(void)
+	    acpi_write_bit_register(acpi_gbl_fixed_event_info
+			      [ACPI_EVENT_POWER_BUTTON].
+			      enable_register_id, ACPI_ENABLE_EVENT);
+
 	arg.integer.value = sleep_state;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
@@ -608,18 +620,6 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
 
 	acpi_gbl_system_awake_and_running = TRUE;
 
-	/* Enable power button */
-
-	(void)
-	    acpi_write_bit_register(acpi_gbl_fixed_event_info
-			      [ACPI_EVENT_POWER_BUTTON].
-			      enable_register_id, ACPI_ENABLE_EVENT);
-
-	(void)
-	    acpi_write_bit_register(acpi_gbl_fixed_event_info
-			      [ACPI_EVENT_POWER_BUTTON].
-			      status_register_id, ACPI_CLEAR_STATUS);
-
 	arg.integer.value = ACPI_SST_WORKING;
 	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
 	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 01574a0..b63c525 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -257,13 +257,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	/* Reprogram control registers and execute _BFS */
 	acpi_leave_sleep_state_prep(acpi_state);
 
-	/* ACPI 3.0 specs (P62) says that it's the responsibility
-	 * of the OSPM to clear the status bit [ implying that the
-	 * POWER_BUTTON event should not reach userspace ]
-	 */
-	if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3))
-		acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
-
 	/*
 	 * Disable and clear GPE status before interrupt is enabled. Some GPEs
 	 * (like wakeup GPE) haven't handler, this can avoid such GPE misfire.

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[yakui_zhao]

On Thu, 2009-06-18 at 03:08 +0800, Bjorn Helgaas wrote:
> On Wednesday 17 June 2009 01:34:38 am yakui_zhao wrote:
> > On Wed, 2009-06-17 at 12:26 +0800, Bjorn Helgaas wrote:
> > > On Tuesday 16 June 2009 9:21:40 pm yakui_zhao wrote:
> > > > On Tue, 2009-06-16 at 00:49 +0800, Bjorn Helgaas wrote:
> > > > > Clear power button status before enabling event.
> > > > > 
> > > > > It's unusual to enable an event, then immediately clear it, so this
> > > > > looks like a possible bug.  If it was intentional, perhaps a comment
> > > > > would be in order.
> > > > IMO this patch is unnecessary.
> > > 
> > > > It seems that we will clear the power button event immediately after it
> > > > is resumed from OS. (This is done in the function of
> > > > acpi_suspend_enter).
> > > 
> > > A comment in acpi_suspend_enter() refers to ACPI 3.0b sec. 4.7.2.2.1.1,
> > > which says "OSPM responds [to a button press after the button press that
> > > transitioned the system into a sleeping state] by clearing the power button
> > > status bit and waking the system."
> > If the power button event is not cleared after finishing the resume, the
> > acpid will receive the power button event and then power off the system.
> > So OSPM had better clear the power button event in course of resuming to
> > avoid that the box is poweroff.
> 
> I agree.
> 
> > > So *somebody* has to clear the status bit, but I'm not sure that it has
> > > to be done in the Linux-specific code, e.g,. acpi_suspend_enter().  The
> > > term "OSPM" seems broad enough to include both the ACPI CA and the Linux-
> > > specific code, and it may be more robust to clear it in the CA.
> > > 
> > > > Maybe the power event status bit is set before we re-enable the event
> > > > bit. And after we re-enable the power button event, OS can handle the
> > > > power button event (the acpi_leave_sleep_state is called with interrupts
> > > > enabled). 
> > > > 
> > > > If the patch is applied, the power button event will be lost.
> > > 
> > > I think this is the scenario you refer to:
> > > 
> > >   button press A causes wakeup
> > >   <possible button press B>
> > >   acpi_suspend_enter() clears event
> > >   <possible button press C>
>       acpi_leave_sleep_state() runs _WAK here
> > >   acpi_leave_sleep_state() clears event
> > >   acpi_leave_sleep_state() enables event
> > >   <possible button press D>
> > > 
> > > Even without this patch, we would lose button event B.  With this patch,
> > > we would also lose button event C.  This whole sequence should take very
> > > little time, so I'm dubious that there is any value in keeping either
> > > B or C -- it seems they'd most likely be unintentional.
> > Without this patch, the power button event C can be handled.
> > 
> > If the power button event bit will be set in the _WAK object, then this
> > event can't be handled after applying your patch.
> 
> I agree (because we run _WAK before clearing the power button event).
> 
> > > Actually, it seems like it would make the most sense to apply this patch
> > > *and* stop clearing the event in acpi_suspend_enter().  Then the code is
> > > simpler and easier to analyze, because we only touch the button status in
> > > one place.
> > IMO it will be better to clear the power button event in the function of
> > acpi_suspend_enter.
> > If we don't do that, maybe the BIOS will set the power button event
> > status/enable bit. And after the interrupt is enabled, the power button
> > event handler will send the event. In such case the acpid will receive
> > the event. Maybe the box will be powered off after resuming. This is not
> > what we wanted.
> 
> I agree.
> 
> What's your opinion of the following patch?  This drops the clear
> from acpi_suspend_enter().  In the ACPI CA acpi_leave_sleep_state(),
> it clears first, then enables, and moves both before the _WAK
> execution.
> 
>   button press A causes wakeup
>   <possible button press (or BIOS sets event) B>
>   acpi_leave_sleep_state() clears all GPEs
>   acpi_leave_sleep_state() enables runtime GPEs
>   acpi_leave_sleep_state() clears power button event
>   <possible button press C>
>   acpi_leave_sleep_state() enables power button event
>   <possible button press D>
>   acpi_leave_sleep_state() runs _WAK
>   <possible button "press" E from _WAK>
>   <possible button press F>
>   
> This should work the same as the current code -- we drop event B
> as desired, and we handle all others.
> 
> It has the advantages that we remove a bit of code from Linux/ACPI,
> we only touch the power button events in one place, and we follow
> the same pattern (clear, then enable) as the GPEs.
> 
> I'm not sure why acpi_suspend_enter() only clears the event when
> the acpi_target_sleep_state == ACPI_STATE_S3.  Sec. 4.7.2.2.1.1
> doesn't mention that.
We had better not delete the code that clears the power button event in
course of acpi_suspend_enter. 
This is to avoid that the power button wake event hits the user space.

Maybe the BIOS will set the power button event enable/status bit. And
after the interrupt is enabled, the power button event will hit the
userland. So it is appropriate to clear the power button event as early
as possible.
    Of course it can be cleared in the function of
"acpi_leave_sleep_state_prep". 
   In the following patch the power button event is cleared in the
function of acpi_leave_sleep_state, which is called with the interrupt
enabled.
   IMO this is not appropriate.

Thanks.
> 
> Bjorn
> 
> 
> diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> index db307a3..c3252ee 100644
> --- a/drivers/acpi/acpica/hwsleep.c
> +++ b/drivers/acpi/acpica/hwsleep.c
> @@ -592,6 +592,18 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
>  		return_ACPI_STATUS(status);
>  	}
>  
> +	/* Enable power button */
> +
> +	(void)
> +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> +			      [ACPI_EVENT_POWER_BUTTON].
> +			      status_register_id, ACPI_CLEAR_STATUS);
> +
> +	(void)
> +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> +			      [ACPI_EVENT_POWER_BUTTON].
> +			      enable_register_id, ACPI_ENABLE_EVENT);
> +
>  	arg.integer.value = sleep_state;
>  	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> @@ -608,18 +620,6 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
>  
>  	acpi_gbl_system_awake_and_running = TRUE;
>  
> -	/* Enable power button */
> -
> -	(void)
> -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> -			      [ACPI_EVENT_POWER_BUTTON].
> -			      enable_register_id, ACPI_ENABLE_EVENT);
> -
> -	(void)
> -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> -			      [ACPI_EVENT_POWER_BUTTON].
> -			      status_register_id, ACPI_CLEAR_STATUS);
> -
>  	arg.integer.value = ACPI_SST_WORKING;
>  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
>  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 01574a0..b63c525 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -257,13 +257,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>  	/* Reprogram control registers and execute _BFS */
>  	acpi_leave_sleep_state_prep(acpi_state);
>  
> -	/* ACPI 3.0 specs (P62) says that it's the responsibility
> -	 * of the OSPM to clear the status bit [ implying that the
> -	 * POWER_BUTTON event should not reach userspace ]
> -	 */
> -	if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3))
> -		acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
> -
>  	/*
>  	 * Disable and clear GPE status before interrupt is enabled. Some GPEs
>  	 * (like wakeup GPE) haven't handler, this can avoid such GPE misfire.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[Bjorn Helgaas]

On Wednesday 17 June 2009 8:14:26 pm yakui_zhao wrote:
> On Thu, 2009-06-18 at 03:08 +0800, Bjorn Helgaas wrote:
> > What's your opinion of the following patch?  This drops the clear
> > from acpi_suspend_enter().  In the ACPI CA acpi_leave_sleep_state(),
> > it clears first, then enables, and moves both before the _WAK
> > execution.
> > 
> >   button press A causes wakeup
> >   <possible button press (or BIOS sets event) B>
> >   acpi_leave_sleep_state() clears all GPEs
> >   acpi_leave_sleep_state() enables runtime GPEs
> >   acpi_leave_sleep_state() clears power button event
> >   <possible button press C>
> >   acpi_leave_sleep_state() enables power button event
> >   <possible button press D>
> >   acpi_leave_sleep_state() runs _WAK
> >   <possible button "press" E from _WAK>
> >   <possible button press F>
> >   
> > This should work the same as the current code -- we drop event B
> > as desired, and we handle all others.
> > 
> > It has the advantages that we remove a bit of code from Linux/ACPI,
> > we only touch the power button events in one place, and we follow
> > the same pattern (clear, then enable) as the GPEs.
> > 
> > I'm not sure why acpi_suspend_enter() only clears the event when
> > the acpi_target_sleep_state == ACPI_STATE_S3.  Sec. 4.7.2.2.1.1
> > doesn't mention that.
> We had better not delete the code that clears the power button event in
> course of acpi_suspend_enter. 
> This is to avoid that the power button wake event hits the user space.
> 
> Maybe the BIOS will set the power button event enable/status bit. And
> after the interrupt is enabled, the power button event will hit the
> userland. So it is appropriate to clear the power button event as early
> as possible.

OK, I think I see now.  I didn't think through the "should not reach
userspace" language.  I think we just want to clear the event so we
don't take another interrupt when we re-enable interrupts near the
end of acpi_suspend_enter().

>     Of course it can be cleared in the function of
> "acpi_leave_sleep_state_prep".

The interrupt path (acpi_ev_sci_xrupt_handler() ->
acpi_ev_fixed_event_detect() -> acpi_ev_fixed_event_dispatch())
is mostly in the ACPI CA, so I like your idea of clearing the
power button event in acpi_leave_sleep_state_prep() because
that's also in the CA, and it seems more robust if the CA
clears it rather than assuming that the host OS will clear it.

I would probably suggest moving the acpi_disable_all_gpes() into
acpi_leave_sleep_state_prep() for the same reason.

It's still asymmetric because we disable & clear *all* GPEs,
but only the power button fixed event.

And I still don't understand the ACPI_STATE_S3 test around the
power button event clear.  Do you think that's necessary?

Thanks for your patience in explaining all this to me.

Bjorn

> > diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> > index db307a3..c3252ee 100644
> > --- a/drivers/acpi/acpica/hwsleep.c
> > +++ b/drivers/acpi/acpica/hwsleep.c
> > @@ -592,6 +592,18 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> >  		return_ACPI_STATUS(status);
> >  	}
> >  
> > +	/* Enable power button */
> > +
> > +	(void)
> > +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > +			      [ACPI_EVENT_POWER_BUTTON].
> > +			      status_register_id, ACPI_CLEAR_STATUS);
> > +
> > +	(void)
> > +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > +			      [ACPI_EVENT_POWER_BUTTON].
> > +			      enable_register_id, ACPI_ENABLE_EVENT);
> > +
> >  	arg.integer.value = sleep_state;
> >  	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
> >  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> > @@ -608,18 +620,6 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> >  
> >  	acpi_gbl_system_awake_and_running = TRUE;
> >  
> > -	/* Enable power button */
> > -
> > -	(void)
> > -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > -			      [ACPI_EVENT_POWER_BUTTON].
> > -			      enable_register_id, ACPI_ENABLE_EVENT);
> > -
> > -	(void)
> > -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > -			      [ACPI_EVENT_POWER_BUTTON].
> > -			      status_register_id, ACPI_CLEAR_STATUS);
> > -
> >  	arg.integer.value = ACPI_SST_WORKING;
> >  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
> >  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index 01574a0..b63c525 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -257,13 +257,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
> >  	/* Reprogram control registers and execute _BFS */
> >  	acpi_leave_sleep_state_prep(acpi_state);
> >  
> > -	/* ACPI 3.0 specs (P62) says that it's the responsibility
> > -	 * of the OSPM to clear the status bit [ implying that the
> > -	 * POWER_BUTTON event should not reach userspace ]
> > -	 */
> > -	if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3))
> > -		acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
> > -
> >  	/*
> >  	 * Disable and clear GPE status before interrupt is enabled. Some GPEs
> >  	 * (like wakeup GPE) haven't handler, this can avoid such GPE misfire.
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[yakui_zhao]

On Thu, 2009-06-18 at 11:38 +0800, Bjorn Helgaas wrote:
> On Wednesday 17 June 2009 8:14:26 pm yakui_zhao wrote:
> > On Thu, 2009-06-18 at 03:08 +0800, Bjorn Helgaas wrote:
> > > What's your opinion of the following patch?  This drops the clear
> > > from acpi_suspend_enter().  In the ACPI CA acpi_leave_sleep_state(),
> > > it clears first, then enables, and moves both before the _WAK
> > > execution.
> > > 
> > >   button press A causes wakeup
> > >   <possible button press (or BIOS sets event) B>
> > >   acpi_leave_sleep_state() clears all GPEs
> > >   acpi_leave_sleep_state() enables runtime GPEs
> > >   acpi_leave_sleep_state() clears power button event
> > >   <possible button press C>
> > >   acpi_leave_sleep_state() enables power button event
> > >   <possible button press D>
> > >   acpi_leave_sleep_state() runs _WAK
> > >   <possible button "press" E from _WAK>
> > >   <possible button press F>
> > >   
> > > This should work the same as the current code -- we drop event B
> > > as desired, and we handle all others.
> > > 
> > > It has the advantages that we remove a bit of code from Linux/ACPI,
> > > we only touch the power button events in one place, and we follow
> > > the same pattern (clear, then enable) as the GPEs.
> > > 
> > > I'm not sure why acpi_suspend_enter() only clears the event when
> > > the acpi_target_sleep_state == ACPI_STATE_S3.  Sec. 4.7.2.2.1.1
> > > doesn't mention that.
> > We had better not delete the code that clears the power button event in
> > course of acpi_suspend_enter. 
> > This is to avoid that the power button wake event hits the user space.
> > 
> > Maybe the BIOS will set the power button event enable/status bit. And
> > after the interrupt is enabled, the power button event will hit the
> > userland. So it is appropriate to clear the power button event as early
> > as possible.
> 
> OK, I think I see now.  I didn't think through the "should not reach
> userspace" language.  I think we just want to clear the event so we
> don't take another interrupt when we re-enable interrupts near the
> end of acpi_suspend_enter().
What you said is partially right.
   The power button wakeup event had better not hit the userland. If the
acpid receives the power button wakeup event, then the system will be
shutdown. This is not what we wanted.
   So it will be better that the power button event is cleared as early
as possible(At least it is cleared before the interrupt is re-enabled).
> 
> >     Of course it can be cleared in the function of
> > "acpi_leave_sleep_state_prep".
> 
> The interrupt path (acpi_ev_sci_xrupt_handler() ->
> acpi_ev_fixed_event_detect() -> acpi_ev_fixed_event_dispatch())
> is mostly in the ACPI CA, so I like your idea of clearing the
> power button event in acpi_leave_sleep_state_prep() because
> that's also in the CA, and it seems more robust if the CA
> clears it rather than assuming that the host OS will clear it.
> 
> I would probably suggest moving the acpi_disable_all_gpes() into
> acpi_leave_sleep_state_prep() for the same reason.
It is ok for me. 
> 
> It's still asymmetric because we disable & clear *all* GPEs,
> but only the power button fixed event.

> 
> And I still don't understand the ACPI_STATE_S3 test around the
> power button event clear.  Do you think that's necessary?
For the S3 test it is necessary to clear the power button event before
the interrupt is re-enabled.
Now the power button event is cleared at two places in course of resume.
One is  in the acpi_suspend_enter and another is in the function of
acpi_leave_sleep_state. As the acpi_leave_sleep_state is called with
interrupt enabled, it will be too late. Maybe when the interrupt is
enabled, the power button event is fired and then hit the userland.

If the power button event is cleared in the function of
acpi_leave_sleep_state_prep, it is unnecessary to clear it explicitly in
the acpi_suspend_enter.

Thanks.
> 
> Thanks for your patience in explaining all this to me.
> 
> Bjorn
> 
> > > diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> > > index db307a3..c3252ee 100644
> > > --- a/drivers/acpi/acpica/hwsleep.c
> > > +++ b/drivers/acpi/acpica/hwsleep.c
> > > @@ -592,6 +592,18 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> > >  		return_ACPI_STATUS(status);
> > >  	}
> > >  
> > > +	/* Enable power button */
> > > +
> > > +	(void)
> > > +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > > +			      [ACPI_EVENT_POWER_BUTTON].
> > > +			      status_register_id, ACPI_CLEAR_STATUS);
> > > +
> > > +	(void)
> > > +	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > > +			      [ACPI_EVENT_POWER_BUTTON].
> > > +			      enable_register_id, ACPI_ENABLE_EVENT);
> > > +
> > >  	arg.integer.value = sleep_state;
> > >  	status = acpi_evaluate_object(NULL, METHOD_NAME__WAK, &arg_list, NULL);
> > >  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> > > @@ -608,18 +620,6 @@ acpi_status acpi_leave_sleep_state(u8 sleep_state)
> > >  
> > >  	acpi_gbl_system_awake_and_running = TRUE;
> > >  
> > > -	/* Enable power button */
> > > -
> > > -	(void)
> > > -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > > -			      [ACPI_EVENT_POWER_BUTTON].
> > > -			      enable_register_id, ACPI_ENABLE_EVENT);
> > > -
> > > -	(void)
> > > -	    acpi_write_bit_register(acpi_gbl_fixed_event_info
> > > -			      [ACPI_EVENT_POWER_BUTTON].
> > > -			      status_register_id, ACPI_CLEAR_STATUS);
> > > -
> > >  	arg.integer.value = ACPI_SST_WORKING;
> > >  	status = acpi_evaluate_object(NULL, METHOD_NAME__SST, &arg_list, NULL);
> > >  	if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
> > > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > > index 01574a0..b63c525 100644
> > > --- a/drivers/acpi/sleep.c
> > > +++ b/drivers/acpi/sleep.c
> > > @@ -257,13 +257,6 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
> > >  	/* Reprogram control registers and execute _BFS */
> > >  	acpi_leave_sleep_state_prep(acpi_state);
> > >  
> > > -	/* ACPI 3.0 specs (P62) says that it's the responsibility
> > > -	 * of the OSPM to clear the status bit [ implying that the
> > > -	 * POWER_BUTTON event should not reach userspace ]
> > > -	 */
> > > -	if (ACPI_SUCCESS(status) && (acpi_state == ACPI_STATE_S3))
> > > -		acpi_clear_event(ACPI_EVENT_POWER_BUTTON);
> > > -
> > >  	/*
> > >  	 * Disable and clear GPE status before interrupt is enabled. Some GPEs
> > >  	 * (like wakeup GPE) haven't handler, this can avoid such GPE misfire.
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> > 
> 

Re: [PATCH 1/2] ACPICA: Clear power button status before enabling event

[Bjorn Helgaas]

On Thursday 18 June 2009 01:00:21 am yakui_zhao wrote:
> On Thu, 2009-06-18 at 11:38 +0800, Bjorn Helgaas wrote:
> > On Wednesday 17 June 2009 8:14:26 pm yakui_zhao wrote:
> > > Maybe the BIOS will set the power button event enable/status bit. And
> > > after the interrupt is enabled, the power button event will hit the
> > > userland. So it is appropriate to clear the power button event as early
> > > as possible.
> > 
> > OK, I think I see now.  I didn't think through the "should not reach
> > userspace" language.  I think we just want to clear the event so we
> > don't take another interrupt when we re-enable interrupts near the
> > end of acpi_suspend_enter().

> What you said is partially right.
>    The power button wakeup event had better not hit the userland. If the
> acpid receives the power button wakeup event, then the system will be
> shutdown. This is not what we wanted.
>    So it will be better that the power button event is cleared as early
> as possible(At least it is cleared before the interrupt is re-enabled).

I don't like to think about this in terms like "as early as
possible" because that doesn't help us figure out where the
clearing needs to happen for correct system behavior.  "Before
interrupts are re-enabled" does help us analyze correctness.

It's true that there is a window of time where button presses
will be ignored, and moving the clear earlier does reduce the
size of that window.  But the window is tiny compared to the
time it takes a human to press a button, so I don't think this
is very important.

> > And I still don't understand the ACPI_STATE_S3 test around the
> > power button event clear.  Do you think that's necessary?

> For the S3 test it is necessary to clear the power button event before
> the interrupt is re-enabled.

My question is, can we clear the power button event when resuming
from any kind of suspend?  If not, what is special about ACPI_STATE_S3
that means we should clear the event for S3, but not for other states?

Maybe I should have started by simply suggesting the removal of the
clear in acpi_leave_sleep_state().  Does that serve some purpose?
It looks useless to me.  Normally we would clear the event in the
interrupt path (in acpi_ev_fixed_event_dispatch()), and I would think
that would be enough.

Bjorn