Re: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake

[Anchal Agarwal <anchalag@amazon.com>]

On Thu, Dec 12, 2019 at 11:10:32PM +0100, Rafael J. Wysocki wrote:
> On Monday, October 29, 2018 6:52:16 PM CET Anchal Agarwal wrote:
> > Currently we do not see sleep_enable bit set after guest resumes
> > from hibernation. Hibernation is triggered in guest on receiving
> > a sleep trigger from the hypervisor(S4 state). We see that power
> > button is enabled on wake up from S4 state however sleep button
> > isn't.
> 
> I'm not against this change in principle, although it may change behavior
> in a somewhat unexpected way on some systems.
> 
Apologies for late response, I was out on vacation. 
Can you please elaborate more on that?
> > This causes subsequent invocation of sleep state to fail
> > in the guest.
> 
> However, can you explain this in a bit more detail?
>  
I can explain this for my particular case, however, any code path going
through acpi_hw_legacy_wake path will not have sleep button enabled. Why
is only power button event enabled and not sleep here?

In my case, I am using sleep button event to generate S4 state
which causes guest VM running on xen to hibernate. I have found
in my debugging process that after first resume when kernel goes 
through this code path, sleep button event is not enabled and 
when hypervisor try to generate sci interrupt, its not received in
the guest since this path does not enable sleep event.


Thanks,
Anchal
> > Signed-off-by: Anchal Agarwal <anchalag@amazon.com>
> > Reviewed-by: Balbir Singh <sblbir@amazon.com>
> > Reviewed-by: Frank van der Linden <fllinden@amazon.com>
> > ---
> >  drivers/acpi/acpica/hwsleep.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/acpi/acpica/hwsleep.c b/drivers/acpi/acpica/hwsleep.c
> > index b62db8ec446f..a176c7802760 100644
> > --- a/drivers/acpi/acpica/hwsleep.c
> > +++ b/drivers/acpi/acpica/hwsleep.c
> > @@ -300,6 +300,17 @@ acpi_status acpi_hw_legacy_wake(u8 sleep_state)
> >  				    [ACPI_EVENT_POWER_BUTTON].
> >  				    status_register_id, ACPI_CLEAR_STATUS);
> >  
> > +	/* Enable sleep button */
> > +	(void)
> > +	      acpi_write_bit_register(acpi_gbl_fixed_event_info
> > +				      [ACPI_EVENT_SLEEP_BUTTON].
> > +				      enable_register_id, ACPI_ENABLE_EVENT);
> > +
> > +	(void)
> > +	      acpi_write_bit_register(acpi_gbl_fixed_event_info
> > +				      [ACPI_EVENT_SLEEP_BUTTON].
> > +				      status_register_id, ACPI_CLEAR_STATUS);
> > +
> >  	acpi_hw_execute_sleep_method(METHOD_PATHNAME__SST, ACPI_SST_WORKING);
> >  	return_ACPI_STATUS(status);
> >  }
> > 
> 
> 
> 
>