| Message ID | 1456925357-2899-1-git-send-email-yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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; > } > 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 -- 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
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!
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. Also, is this what Windows does if _S5 is absent? -- 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
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 -- 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
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 -- 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
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. */
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(-)