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

[Bjorn Helgaas <bjorn.helgaas@hp.com>]

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
> 
>