| Message ID | 20191101212319.1FFC440EB1@dev-dsk-anchalag-2a-9c2d1d96.us-west-2.amazon.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ACPICA: Enable sleep button on ACPI legacy wake | expand |
+ 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
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
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 > > > > >
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); > } >
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); > > } > > > > > >
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!
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 > > >
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?
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); }