[PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Chen Yu]

The problem is Linux registers pm_power_off = efi_power_off
only if we are in hardware reduced mode. Actually, what we also
want is to do this when ACPI S5 is simply not supported.
That should handle both the HW reduced mode, and the HW-full
mode where the DSDT fails to supply an _S5 object.

This patch fixes this issue by introducing a new flag acpi_no_s5 which
indicates the non-existence of _S5. The initial state of acpi_no_s5 is
false and probed in acpi_sleep_init, then we'll later see the updated
value in efi_poweroff_required, according to which we can set pm_power_off
to efi_power_off in efi_shutdown_init.

Suggested-by: Len Brown <len.brown@intel.com>
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 arch/x86/platform/efi/quirks.c | 3 ++-
 drivers/acpi/sleep.c           | 2 ++
 include/acpi/acpixf.h          | 6 ++++++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
index 2d66db8..8ec43bc 100644
--- a/arch/x86/platform/efi/quirks.c
+++ b/arch/x86/platform/efi/quirks.c
@@ -295,5 +295,6 @@ bool efi_reboot_required(void)
 
 bool efi_poweroff_required(void)
 {
-	return !!acpi_gbl_reduced_hardware;
+	return (!!acpi_gbl_reduced_hardware)
+		|| (!!acpi_no_s5);
 }
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9cb9752..6f75e54 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -846,6 +846,8 @@ int __init acpi_sleep_init(void)
 		sleep_states[ACPI_STATE_S5] = 1;
 		pm_power_off_prepare = acpi_power_off_prepare;
 		pm_power_off = acpi_power_off;
+	} else {
+		acpi_no_s5 = TRUE;
 	}
 
 	supported[0] = 0;
diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
index c96621e..2b4d7f7 100644
--- a/include/acpi/acpixf.h
+++ b/include/acpi/acpixf.h
@@ -252,6 +252,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_osi_data, 0);
 ACPI_INIT_GLOBAL(u8, acpi_gbl_reduced_hardware, FALSE);
 
 /*
+ * Some HW-full platforms do not have S5, so they may need
+ * to leverage efi power off for a shutdown.
+ */
+ACPI_INIT_GLOBAL(u8, acpi_no_s5, FALSE);
+
+/*
  * This mechanism is used to trace a specified AML method. The method is
  * traced each time it is executed.
  */
-- 
1.8.4.2

Re: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Rafael J. Wysocki]

On Wed, Mar 2, 2016 at 2:29 PM, Chen Yu <yu.c.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> The problem is Linux registers pm_power_off = efi_power_off
> only if we are in hardware reduced mode. Actually, what we also
> want is to do this when ACPI S5 is simply not supported.
> That should handle both the HW reduced mode, and the HW-full
> mode where the DSDT fails to supply an _S5 object.
>
> This patch fixes this issue by introducing a new flag acpi_no_s5 which
> indicates the non-existence of _S5. The initial state of acpi_no_s5 is
> false and probed in acpi_sleep_init, then we'll later see the updated
> value in efi_poweroff_required, according to which we can set pm_power_off
> to efi_power_off in efi_shutdown_init.
>
> Suggested-by: Len Brown <len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Chen Yu <yu.c.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/platform/efi/quirks.c | 3 ++-
>  drivers/acpi/sleep.c           | 2 ++
>  include/acpi/acpixf.h          | 6 ++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index 2d66db8..8ec43bc 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -295,5 +295,6 @@ bool efi_reboot_required(void)
>
>  bool efi_poweroff_required(void)
>  {
> -       return !!acpi_gbl_reduced_hardware;
> +       return (!!acpi_gbl_reduced_hardware)
> +               || (!!acpi_no_s5);

This can be written as:

    return acpi_gbl_reduced_hardware || acpi_no_s5;

>  }
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 9cb9752..6f75e54 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -846,6 +846,8 @@ int __init acpi_sleep_init(void)
>                 sleep_states[ACPI_STATE_S5] = 1;
>                 pm_power_off_prepare = acpi_power_off_prepare;
>                 pm_power_off = acpi_power_off;
> +       } else {
> +               acpi_no_s5 = TRUE;
>         }
>
>         supported[0] = 0;
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index c96621e..2b4d7f7 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -252,6 +252,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_osi_data, 0);
>  ACPI_INIT_GLOBAL(u8, acpi_gbl_reduced_hardware, FALSE);
>
>  /*
> + * Some HW-full platforms do not have S5, so they may need
> + * to leverage efi power off for a shutdown.
> + */
> +ACPI_INIT_GLOBAL(u8, acpi_no_s5, FALSE);

Do you have to add it here?

Can't you simply define it as a global bool variable in sleep.c and
add a declaration to include/linux/acpi.h?

> +
> +/*
>   * This mechanism is used to trace a specified AML method. The method is
>   * traced each time it is executed.
>   */
> --

Thanks,
Rafael

RE: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Chen, Yu C]

Hi Rafael,

> -----Original Message-----
> From: rjwysocki@gmail.com [mailto:rjwysocki@gmail.com] On Behalf Of
> Rafael J. Wysocki
> Sent: Thursday, March 03, 2016 7:45 AM
> To: Chen, Yu C
> Cc: ACPI Devel Maling List; linux-efi@vger.kernel.org; linux-
> pm@vger.kernel.org; Rafael J. Wysocki; Len Brown; matt.fleming@intel.com;
> Thomas Gleixner; x86@kernel.org; Zhang, Rui; Zheng, Lv
> Subject: Re: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full
> platforms without _S5
> 
> On Wed, Mar 2, 2016 at 2:29 PM, Chen Yu <yu.c.chen@intel.com> wrote:
> > The problem is Linux registers pm_power_off = efi_power_off only if we
> > are in hardware reduced mode. Actually, what we also want is to do
> > this when ACPI S5 is simply not supported.
> > That should handle both the HW reduced mode, and the HW-full mode
> > where the DSDT fails to supply an _S5 object.
> >
> > This patch fixes this issue by introducing a new flag acpi_no_s5 which
> > indicates the non-existence of _S5. The initial state of acpi_no_s5 is
> > false and probed in acpi_sleep_init, then we'll later see the updated
> > value in efi_poweroff_required, according to which we can set
> > pm_power_off to efi_power_off in efi_shutdown_init.
> >
> > Suggested-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  arch/x86/platform/efi/quirks.c | 3 ++-
> >  drivers/acpi/sleep.c           | 2 ++
> >  include/acpi/acpixf.h          | 6 ++++++
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/platform/efi/quirks.c
> > b/arch/x86/platform/efi/quirks.c index 2d66db8..8ec43bc 100644
> > --- a/arch/x86/platform/efi/quirks.c
> > +++ b/arch/x86/platform/efi/quirks.c
> > @@ -295,5 +295,6 @@ bool efi_reboot_required(void)
> >
> >  bool efi_poweroff_required(void)
> >  {
> > -       return !!acpi_gbl_reduced_hardware;
> > +       return (!!acpi_gbl_reduced_hardware)
> > +               || (!!acpi_no_s5);
> 
> This can be written as:
> 
>     return acpi_gbl_reduced_hardware || acpi_no_s5;
> 
OK.
> >  }
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index
> > 9cb9752..6f75e54 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -846,6 +846,8 @@ int __init acpi_sleep_init(void)
> >                 sleep_states[ACPI_STATE_S5] = 1;
> >                 pm_power_off_prepare = acpi_power_off_prepare;
> >                 pm_power_off = acpi_power_off;
> > +       } else {
> > +               acpi_no_s5 = TRUE;
> >         }
> >
> >         supported[0] = 0;
> > diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h index
> > c96621e..2b4d7f7 100644
> > --- a/include/acpi/acpixf.h
> > +++ b/include/acpi/acpixf.h
> > @@ -252,6 +252,12 @@ ACPI_INIT_GLOBAL(u8, acpi_gbl_osi_data, 0);
> > ACPI_INIT_GLOBAL(u8, acpi_gbl_reduced_hardware, FALSE);
> >
> >  /*
> > + * Some HW-full platforms do not have S5, so they may need
> > + * to leverage efi power off for a shutdown.
> > + */
> > +ACPI_INIT_GLOBAL(u8, acpi_no_s5, FALSE);
> 
> Do you have to add it here?
> 
> Can't you simply define it as a global bool variable in sleep.c and add a
> declaration to include/linux/acpi.h?
> 
OK, let me move it there. I'll send a V2 patch, thanks!

Re: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Matt Fleming]

On Wed, 02 Mar, at 09:29:17PM, Chen Yu wrote:
> The problem is Linux registers pm_power_off = efi_power_off
> only if we are in hardware reduced mode. Actually, what we also
> want is to do this when ACPI S5 is simply not supported.
> That should handle both the HW reduced mode, and the HW-full
> mode where the DSDT fails to supply an _S5 object.
> 
> This patch fixes this issue by introducing a new flag acpi_no_s5 which
> indicates the non-existence of _S5. The initial state of acpi_no_s5 is
> false and probed in acpi_sleep_init, then we'll later see the updated
> value in efi_poweroff_required, according to which we can set pm_power_off
> to efi_power_off in efi_shutdown_init.
> 
> Suggested-by: Len Brown <len.brown-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Chen Yu <yu.c.chen-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  arch/x86/platform/efi/quirks.c | 3 ++-
>  drivers/acpi/sleep.c           | 2 ++
>  include/acpi/acpixf.h          | 6 ++++++
>  3 files changed, 10 insertions(+), 1 deletion(-)

Are there legacy platforms without _S5 where we currently handle
reboot via some other means that are going to break if we attempt an
EFI reboot?

I ask because historically performing an EFI reboot has been all kinds
of buggy on x86.

Also, is this what Windows does if _S5 is absent?

RE: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Chen, Yu C]

Hi  Matt,

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Matt Fleming
> Sent: Friday, March 04, 2016 9:33 PM
> To: Chen, Yu C
> Cc: linux-acpi@vger.kernel.org; linux-efi@vger.kernel.org; linux-
> pm@vger.kernel.org; rjw@sisk.pl; lenb@kernel.org; tglx@linutronix.de;
> x86@kernel.org; Zhang, Rui; Zheng, Lv
> Subject: Re: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full
> platforms without _S5
> 
> On Wed, 02 Mar, at 09:29:17PM, Chen Yu wrote:
> > The problem is Linux registers pm_power_off = efi_power_off only if we
> > are in hardware reduced mode. Actually, what we also want is to do
> > this when ACPI S5 is simply not supported.
> > That should handle both the HW reduced mode, and the HW-full mode
> > where the DSDT fails to supply an _S5 object.
> >
> > This patch fixes this issue by introducing a new flag acpi_no_s5 which
> > indicates the non-existence of _S5. The initial state of acpi_no_s5 is
> > false and probed in acpi_sleep_init, then we'll later see the updated
> > value in efi_poweroff_required, according to which we can set
> > pm_power_off to efi_power_off in efi_shutdown_init.
> >
> > Suggested-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  arch/x86/platform/efi/quirks.c | 3 ++-
> >  drivers/acpi/sleep.c           | 2 ++
> >  include/acpi/acpixf.h          | 6 ++++++
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> Are there legacy platforms without _S5 where we currently handle reboot
> via some other means that are going to break if we attempt an EFI reboot?
> 
> I ask because historically performing an EFI reboot has been all kinds of buggy
> on x86.
I made a double check and  yes there are other components would like to
register their customized pm_power_off if _S5 is not available.
So if EFI reboot is not stable, current code may  not be robust enough .
Hum, how about this one?
(assigned to efi reboot only when there is no other components registered pm_power_off)
 return  acpi_gbl_reduced_hardware ||
	(acpi_no_s5 && ! pm_power_off);
> 
> Also, is this what Windows does if _S5 is absent?
I'm not sure, Windows might also leverage other drivers to achieve a poweroff. 

yu

RE: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full platforms without _S5

[Chen, Yu C]

Hi Matt

> -----Original Message-----
> From: linux-pm-owner@vger.kernel.org [mailto:linux-pm-
> owner@vger.kernel.org] On Behalf Of Matt Fleming
> Sent: Friday, March 04, 2016 9:33 PM
> To: Chen, Yu C
> Cc: linux-acpi@vger.kernel.org; linux-efi@vger.kernel.org; linux-
> pm@vger.kernel.org; rjw@sisk.pl; lenb@kernel.org; tglx@linutronix.de;
> x86@kernel.org; Zhang, Rui; Zheng, Lv
> Subject: Re: [PATCH][RFC] ACPI / PM: Fix poweroff issue on HW-full
> platforms without _S5
> 
> On Wed, 02 Mar, at 09:29:17PM, Chen Yu wrote:
> > The problem is Linux registers pm_power_off = efi_power_off only if we
> > are in hardware reduced mode. Actually, what we also want is to do
> > this when ACPI S5 is simply not supported.
> > That should handle both the HW reduced mode, and the HW-full mode
> > where the DSDT fails to supply an _S5 object.
> >
> > This patch fixes this issue by introducing a new flag acpi_no_s5 which
> > indicates the non-existence of _S5. The initial state of acpi_no_s5 is
> > false and probed in acpi_sleep_init, then we'll later see the updated
> > value in efi_poweroff_required, according to which we can set
> > pm_power_off to efi_power_off in efi_shutdown_init.
> >
> > Suggested-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
> > ---
> >  arch/x86/platform/efi/quirks.c | 3 ++-
> >  drivers/acpi/sleep.c           | 2 ++
> >  include/acpi/acpixf.h          | 6 ++++++
> >  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> Are there legacy platforms without _S5 where we currently handle reboot
> via some other means that are going to break if we attempt an EFI reboot?
> 
> I ask because historically performing an EFI reboot has been all kinds of buggy
> on x86.
> 
I've just sent out another version v3,  since legacy platform without _S5 will not be overwritten
to EFI poweroff because it will not pass the efi_enabled(EFI_RUNTIME_SERVICES) check
in efi_shutdown_init, this patch does not break pegacy platforms without _S5.

yu