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

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

[Agarwal, Anchal]

    + Rafael, Bob
    
    Also, is there a reason why this wasn't CC-ed to linux-acpi mailing list? It would be good to get feedback from there too..
I think I did to all the email id's generated from get_maintainer.pl. Ccing again 
+ linux-acpi

    Thanks,
    Erik
    
    > -----Original Message-----
    > From: Anchal Agarwal <anchalag@amazon.com>
    > Sent: Monday, October 29, 2018 10:52 AM
    > Subject: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake
    > 
    > 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. This causes subsequent
    > invocation of sleep state to fail in the guest. Any environment  going through
    > acpi_hw_legacy_wake() won't have sleep button enabled.
    > 
    > 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);
    >  }
    > --
    > 2.15.3.AMZN
    
    

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

[Agarwal, Anchal]

Apologies for resending it. Looks like it does not show in cc list it went to all mailing list however the thread do appear on lkml. 
Thanks,
Anchal
    
        + Rafael, Bob
        
        Also, is there a reason why this wasn't CC-ed to linux-acpi mailing list? It would be good to get feedback from there too..
    I think I did to all the email id's generated from get_maintainer.pl. Ccing again 
    + linux-acpi
    
        Thanks,
        Erik
        
        > -----Original Message-----
        > From: Anchal Agarwal <anchalag@amazon.com>
        > Sent: Monday, October 29, 2018 10:52 AM
        > Subject: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake
        > 
        > 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. This causes subsequent
        > invocation of sleep state to fail in the guest. Any environment  going through
        > acpi_hw_legacy_wake() won't have sleep button enabled.
        > 
        > 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);
        >  }
        > --
        > 2.15.3.AMZN
        
        
    
    

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

[Anchal Agarwal]

Gentle ping on this one.

--
Anchal

On Mon, Nov 04, 2019 at 09:58:16AM -0800, Agarwal, Anchal wrote:
> Apologies for resending it. Looks like it does not show in cc list it went to all mailing list however the thread do appear on lkml. 
> Thanks,
> Anchal
>     
>         + Rafael, Bob
>         
>         Also, is there a reason why this wasn't CC-ed to linux-acpi mailing list? It would be good to get feedback from there too..
>     I think I did to all the email id's generated from get_maintainer.pl. Ccing again 
>     + linux-acpi
>     
>         Thanks,
>         Erik
>         
>         > -----Original Message-----
>         > From: Anchal Agarwal <anchalag@amazon.com>
>         > Sent: Monday, October 29, 2018 10:52 AM
>         > Subject: [PATCH] ACPICA: Enable sleep button on ACPI legacy wake
>         > 
>         > 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. This causes subsequent
>         > invocation of sleep state to fail in the guest. Any environment  going through
>         > acpi_hw_legacy_wake() won't have sleep button enabled.
>         > 
>         > 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);
>         >  }
>         > --
>         > 2.15.3.AMZN
>         
>         
>     
>     
> 

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

[Rafael J. Wysocki]

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.

> This causes subsequent invocation of sleep state to fail
> in the guest.

However, can you explain this in a bit more detail?
 
> 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);
>  }
> 

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

[Anchal Agarwal]

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);
> >  }
> > 
> 
> 
> 
> 

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

[Rafael J. Wysocki]

On Thursday, January 2, 2020 11:39:19 PM CET Anchal Agarwal wrote:
> 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?

See below.

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

Good question and related to the potentially unexpected behavior part.

Presumably, on some systems the sleep button could not be enabled
automatically as it triggered another suspend right away after that.

That's speculation, though.

> In my case, I am using sleep button event to generate S4 state
> which causes guest VM running on xen to hibernate.

Usually, however, the sleep button is used to trigger transitions into S3
(or suspend-to-idle on systems that support it).

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

OK

So the patch can be applied AFAICS, but if it is reported to cause problems to
happen, it will need to be reverted.

Please resend the patch with a proper CC list (including Bob, Erik, the ACPICA
devel list etc).

Thanks!

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

[Anchal Agarwal]

On Wed, Jan 15, 2020 at 11:24:47AM +0100, Rafael J. Wysocki wrote:
> On Thursday, January 2, 2020 11:39:19 PM CET Anchal Agarwal wrote:
> > 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?
> 
> See below.
> 
> > > > 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?
> 
> Good question and related to the potentially unexpected behavior part.
> 
> Presumably, on some systems the sleep button could not be enabled
> automatically as it triggered another suspend right away after that.
> 
> That's speculation, though.
> 
> > In my case, I am using sleep button event to generate S4 state
> > which causes guest VM running on xen to hibernate.
> 
> Usually, however, the sleep button is used to trigger transitions into S3
> (or suspend-to-idle on systems that support it).
> 
> > 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.
> 
> OK
> 
> So the patch can be applied AFAICS, but if it is reported to cause problems to
> happen, it will need to be reverted.
> 
> Please resend the patch with a proper CC list (including Bob, Erik, the ACPICA
> devel list etc).
> 
> Thanks!
Sure can I add your Reviewed-by or Acked-by?

Thanks,
Anchal
> 
> 
> 

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

[Rafael J. Wysocki]

On Wed, Jan 15, 2020 at 7:58 PM Anchal Agarwal <anchalag@amazon.com> wrote:
>
> On Wed, Jan 15, 2020 at 11:24:47AM +0100, Rafael J. Wysocki wrote:
> > On Thursday, January 2, 2020 11:39:19 PM CET Anchal Agarwal wrote:
> > > 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?
> >
> > See below.
> >
> > > > > 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?
> >
> > Good question and related to the potentially unexpected behavior part.
> >
> > Presumably, on some systems the sleep button could not be enabled
> > automatically as it triggered another suspend right away after that.
> >
> > That's speculation, though.
> >
> > > In my case, I am using sleep button event to generate S4 state
> > > which causes guest VM running on xen to hibernate.
> >
> > Usually, however, the sleep button is used to trigger transitions into S3
> > (or suspend-to-idle on systems that support it).
> >
> > > 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.
> >
> > OK
> >
> > So the patch can be applied AFAICS, but if it is reported to cause problems to
> > happen, it will need to be reverted.
> >
> > Please resend the patch with a proper CC list (including Bob, Erik, the ACPICA
> > devel list etc).
> >
> > Thanks!
> Sure can I add your Reviewed-by or Acked-by?

I'll be applying this eventually anyway, so why?