ACPICA: Clear power button status before enabling event
Subsystems (2)
PATCH 1/2 ACPICA: Clear power button status before enabling event
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.
--- drivers/acpi/acpica/hwsleep.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/acpi/acpica/hwsleep.c +2 -2
@@ -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
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.
--- drivers/acpi/acpica/evevent.c | 13 +++---------- drivers/acpi/acpica/hwsleep.c | 12 ++---------- 2 files changed, 5 insertions(+), 20 deletions(-)
drivers/acpi/acpica/evevent.c +3 -10
@@ -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]",
drivers/acpi/acpica/hwsleep.c +2 -10
@@ -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 );
Quoted 5 lines · 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.
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.
Quoted 34 lines
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 <[email protected]>CC Bob Moore <[email protected]>--- drivers/acpi/acpica/hwsleep.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/acpi/acpica/hwsleep.c +2
-3@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 11 lines · yakui_zhao
Quoted 5 lines · 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.
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.
Quoted 6 lines
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_stateis 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
Quoted 35 lines
Quoted 33 lines
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 <[email protected]>CC Bob Moore <[email protected]>--- drivers/acpi/acpica/hwsleep.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/acpi/acpica/hwsleep.c +2
-3@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 9 lines · Bjorn Helgaas
Quoted 7 lines · yakui_zhao
Quoted 5 lines · 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.
IMO this patch is unnecessary.
Quoted 8 lines
Quoted 3 lines
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.
Quoted 23 lines
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.Quoted 6 lines
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_stateis 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>
Quoted 4 lines
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.
Quoted 5 lines
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.
Quoted 42 lines
But I admit I know very little about suspend/resume, so maybe I just need more convincing :-)
Bjorn
Quoted 35 lines
Quoted 33 lines
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 <[email protected]>CC Bob Moore <[email protected]>--- drivers/acpi/acpica/hwsleep.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-)drivers/acpi/acpica/hwsleep.c +2
-3@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 14 lines · yakui_zhao
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · yakui_zhao
Quoted 5 lines · 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.
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).
Quoted 8 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 10 lines
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.Quoted 5 lines
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_stateis called with interrupts enabled).
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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
Quoted 4 lines
Quoted 4 lines
acpi_leave_sleep_state()clears event
acpi_leave_sleep_state()enables event
<possible button press D>
Quoted 8 lines
Quoted 4 lines
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
_WAKobject, then this event can't be handled after applying your patch.
I agree (because we run _WAK before clearing the power button event).
Quoted 11 lines
Quoted 4 lines
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
drivers/acpi/acpica/hwsleep.c +12 -12
@@ -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 ) {
drivers/acpi/sleep.c +0 -7
@@ -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.
Quoted 94 lines · Bjorn Helgaas
Quoted 14 lines · yakui_zhao
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · yakui_zhao
Quoted 5 lines · 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.
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).Quoted 8 lines
Quoted 4 lines
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.
Quoted 10 lines
Quoted 10 lines
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.Quoted 5 lines
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_stateis called with interrupts enabled).Quoted 8 lines
Quoted 8 lines
Quoted 1 line
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_WAKhereQuoted 4 lines
Quoted 4 lines
acpi_leave_sleep_state()clears event
acpi_leave_sleep_state()enables event
<possible button press D>Quoted 8 lines
Quoted 4 lines
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
_WAKobject, then this event can't be handled after applying your patch.I agree (because we run
_WAKbefore clearing the power button event).Quoted 11 lines
Quoted 4 lines
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 CAacpi_leave_sleep_state(), it clears first, then enables, and moves both before the_WAKexecution.button press A causes wakeup
<possible button press (or BIOS sets event) B>acpi_leave_sleep_state()clears all GPEsacpi_leave_sleep_state()enables runtime GPEsacpi_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.
Quoted 7 lines
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 theacpi_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.
Quoted 68 lines
Bjorn
drivers/acpi/acpica/hwsleep.c +12
-12@@ -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 ) {drivers/acpi/sleep.c +0
-8@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 28 lines · yakui_zhao
Quoted 20 lines · Bjorn Helgaas
What's your opinion of the following patch? This drops the clear from
acpi_suspend_enter(). In the ACPI CAacpi_leave_sleep_state(), it clears first, then enables, and moves both before the_WAKexecution.button press A causes wakeup
<possible button press (or BIOS sets event) B>acpi_leave_sleep_state()clears all GPEsacpi_leave_sleep_state()enables runtime GPEsacpi_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.
Quoted 7 lines
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 theacpi_target_sleep_state==ACPI_STATE_S3. Sec. 4.7.2.2.1.1 doesn't mention that.
Quoted 8 lines
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().
Quoted 2 lines
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
Quoted 66 lines
Quoted 64 lines
drivers/acpi/acpica/hwsleep.c +12
-12@@ -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 ) {drivers/acpi/sleep.c +0
-8@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 38 lines · Bjorn Helgaas
Quoted 28 lines · yakui_zhao
Quoted 20 lines · Bjorn Helgaas
What's your opinion of the following patch? This drops the clear from
acpi_suspend_enter(). In the ACPI CAacpi_leave_sleep_state(), it clears first, then enables, and moves both before the_WAKexecution.button press A causes wakeup
<possible button press (or BIOS sets event) B>acpi_leave_sleep_state()clears all GPEsacpi_leave_sleep_state()enables runtime GPEsacpi_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.
Quoted 7 lines
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 theacpi_target_sleep_state==ACPI_STATE_S3. Sec. 4.7.2.2.1.1 doesn't mention that.Quoted 8 lines
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.
Quoted 4 lines
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).
Quoted 11 lines
Quoted 3 lines
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 inacpi_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.
Quoted 2 lines
I would probably suggest moving the
acpi_disable_all_gpes()intoacpi_leave_sleep_state_prep()for the same reason.
It is ok for me.
Quoted 7 lines
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_S3test 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.
Quoted 72 lines
Thanks for your patience in explaining all this to me.
Bjorn
Quoted 66 lines
Quoted 64 lines
drivers/acpi/acpica/hwsleep.c +12
-12@@ -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 ) {drivers/acpi/sleep.c +0
-8@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 12 lines · yakui_zhao
Quoted 10 lines · Bjorn Helgaas
Quoted 4 lines · yakui_zhao
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().
Quoted 6 lines
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.
Quoted 5 lines
Quoted 2 lines
And I still don't understand the
ACPI_STATE_S3test 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