| Message ID | 1457687133-3730-1-git-send-email-yu.c.chen@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, 11 Mar, at 05:05:33PM, 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 on > non-legacy platforms. That should handle both the HW reduced mode, > and the HW-full mode where the DSDT fails to supply an _S5 object. > > This patch introduces pm_power_off_default which would be used by > pm_power_off if nothing else is available. And in this case we > leverage efi power off to be this role. However since efi power off > may not be stable enough thus in order not to interfere with other > poweroff path, we only make a minimum enhancement for x86 in > native_machine_power_off. > > Suggested-by: Len Brown <len.brown@intel.com> > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > --- > v4: > - Since in v3 efi_poweroff_required() is not guaranteed to run > after all of the other code that may register alternative > power off handling, add the pm_power_off_default that would > be used by pm_power_off if nothing else is available. > v3: > - Only assign pm_power_off to efi_power_off when there are no > other pm_power_off registered at that time, in case other > commponents would like to customize their own implementation. > --- > v2: > - Convert the acpi_no_s5 to a global bool variable in sleep.c and > add a declaration to include/linux/acpi.h. > --- > arch/x86/kernel/reboot.c | 3 +++ > arch/x86/platform/efi/quirks.c | 5 +++++ > drivers/acpi/sleep.c | 7 +++++++ > drivers/firmware/efi/reboot.c | 8 ++++++++ > include/linux/acpi.h | 1 + > include/linux/efi.h | 1 + > include/linux/pm.h | 1 + > kernel/reboot.c | 1 + > 8 files changed, 27 insertions(+) Couple of things, 1) I'm still waiting for an answer to my question on whether platforms without _S5 that need EFI reset actually exist. You said they "might" exist, which makes this all sound very speculative. It is not obvious to me that this approach makes sense. 2) In v4 you're modifying the generic EFI reboot code and should Cc other developers who might care, e.g. the arm64 folks. I've Cc'd them now. -- 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: Matt Fleming [mailto:matt@codeblueprint.co.uk] > Sent: Friday, March 11, 2016 11:56 PM > To: Chen, Yu C > Cc: linux-acpi@vger.kernel.org; linux-pm@vger.kernel.org; Rafael J. Wysocki; > Len Brown; Thomas Gleixner; Ingo Molnar; H. Peter Anvin; Zhang, Rui; linux- > efi@vger.kernel.org; x86@kernel.org; linux-kernel@vger.kernel.org; Ard > Biesheuvel; Mark Salter > Subject: Re: [PATCH][RFC,v4] ACPI / PM: Introduce efi poweroff for HW-full > platforms without _S5 > > On Fri, 11 Mar, at 05:05:33PM, 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 on non-legacy platforms. > > That should handle both the HW reduced mode, and the HW-full mode > > where the DSDT fails to supply an _S5 object. > > > > This patch introduces pm_power_off_default which would be used by > > pm_power_off if nothing else is available. And in this case we > > leverage efi power off to be this role. However since efi power off > > may not be stable enough thus in order not to interfere with other > > poweroff path, we only make a minimum enhancement for x86 in > > native_machine_power_off. > > > > Suggested-by: Len Brown <len.brown@intel.com> > > Signed-off-by: Chen Yu <yu.c.chen@intel.com> > > --- > > v4: > > - Since in v3 efi_poweroff_required() is not guaranteed to run > > after all of the other code that may register alternative > > power off handling, add the pm_power_off_default that would > > be used by pm_power_off if nothing else is available. > > v3: > > - Only assign pm_power_off to efi_power_off when there are no > > other pm_power_off registered at that time, in case other > > commponents would like to customize their own implementation. > > --- > > v2: > > - Convert the acpi_no_s5 to a global bool variable in sleep.c and > > add a declaration to include/linux/acpi.h. > > --- > > arch/x86/kernel/reboot.c | 3 +++ > > arch/x86/platform/efi/quirks.c | 5 +++++ > > drivers/acpi/sleep.c | 7 +++++++ > > drivers/firmware/efi/reboot.c | 8 ++++++++ > > include/linux/acpi.h | 1 + > > include/linux/efi.h | 1 + > > include/linux/pm.h | 1 + > > kernel/reboot.c | 1 + > > 8 files changed, 27 insertions(+) > > Couple of things, > > 1) I'm still waiting for an answer to my question on whether > platforms without _S5 that need EFI reset actually exist. You > said they "might" exist, which makes this all sound very > speculative. It is not obvious to me that this approach makes > sense. There is a future Base-IA platform, we are planning to skip implementing the SLP_TYP register and the S5 object. (already there will be no S3 and no S4) > > 2) In v4 you're modifying the generic EFI reboot code and should Cc > other developers who might care, e.g. the arm64 folks. I've Cc'd > them now. OK, thanks. 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
On Fri, 11 Mar, at 04:33:46PM, Chen, Yu C wrote: > > There is a future Base-IA platform, we are planning to skip > implementing the SLP_TYP register and the S5 object. (already there > will be no S3 and no S4) Cool. This is really valuable information that should go into the commit message. Because if this is the rationale for the change, I don't see why we'd need to provide the default stuff. Instead we should just enforce EFI reboot, and only add the pm_poweroff_default hook if there is an explicit user in the future, IMO. -- 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: Matt Fleming [mailto:matt@codeblueprint.co.uk] > Sent: Tuesday, March 15, 2016 4:01 AM > To: Chen, Yu C > Cc: linux-acpi@vger.kernel.org; linux-pm@vger.kernel.org; Rafael J. Wysocki; > Len Brown; Thomas Gleixner; Ingo Molnar; H. Peter Anvin; Zhang, Rui; linux- > efi@vger.kernel.org; x86@kernel.org; linux-kernel@vger.kernel.org; Ard > Biesheuvel; Mark Salter > Subject: Re: [PATCH][RFC,v4] ACPI / PM: Introduce efi poweroff for HW-full > platforms without _S5 > > On Fri, 11 Mar, at 04:33:46PM, Chen, Yu C wrote: > > > > There is a future Base-IA platform, we are planning to skip > > implementing the SLP_TYP register and the S5 object. (already there > > will be no S3 and no S4) > > Cool. This is really valuable information that should go into the commit > message. > > Because if this is the rationale for the change, I don't see why we'd need to > provide the default stuff. Instead we should just enforce EFI reboot, and > only add the pm_poweroff_default hook if there is an explicit user in the > future, IMO. Do you mean the patch v3 make sense https://patchwork.kernel.org/patch/8514751/ and we should use efi power off as our first choice, if there is no _S5 available(no acpi_power_off), even there is a customized poweroff(driver provided, eg)? Meanwhile, the legacy platforms will not be affected because there is no path to overwrite pm_power_off to efi power off. thanks, 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
On Wed, 16 Mar, at 05:59:29AM, Chen, Yu C wrote: > Hi Matt, > > > -----Original Message----- > > From: Matt Fleming [mailto:matt@codeblueprint.co.uk] > > Sent: Tuesday, March 15, 2016 4:01 AM > > To: Chen, Yu C > > Cc: linux-acpi@vger.kernel.org; linux-pm@vger.kernel.org; Rafael J. Wysocki; > > Len Brown; Thomas Gleixner; Ingo Molnar; H. Peter Anvin; Zhang, Rui; linux- > > efi@vger.kernel.org; x86@kernel.org; linux-kernel@vger.kernel.org; Ard > > Biesheuvel; Mark Salter > > Subject: Re: [PATCH][RFC,v4] ACPI / PM: Introduce efi poweroff for HW-full > > platforms without _S5 > > > > On Fri, 11 Mar, at 04:33:46PM, Chen, Yu C wrote: > > > > > > There is a future Base-IA platform, we are planning to skip > > > implementing the SLP_TYP register and the S5 object. (already there > > > will be no S3 and no S4) > > > > Cool. This is really valuable information that should go into the commit > > message. > > > > Because if this is the rationale for the change, I don't see why we'd need to > > provide the default stuff. Instead we should just enforce EFI reboot, and > > only add the pm_poweroff_default hook if there is an explicit user in the > > future, IMO. > > Do you mean the patch v3 make sense > https://patchwork.kernel.org/patch/8514751/ > and we should use efi power off as our first choice, if there is no _S5 available(no acpi_power_off), > even there is a customized poweroff(driver provided, eg)? Unless someone can point to a platform driver that is in the upstream kernel where this is actually a problem, the answer is: yes. For that matter, unless someone can do the same for pm_power_off overriding efi_reboot() (which on x86 would only happen for ACPI HW-reduced platforms), I would be much prefer the original patch, where you had, bool efi_poweroff_reqired(voi) { return acpi_gbl_reduced_hardware || acpi_no_s5; } since you've already explained that this change won't break legacy platforms that are missing _S5 (if any even exist in the wild). -- 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/kernel/reboot.c b/arch/x86/kernel/reboot.c index ab0adc0..bec3047 100644 --- a/arch/x86/kernel/reboot.c +++ b/arch/x86/kernel/reboot.c @@ -666,6 +666,9 @@ static void native_machine_halt(void) static void native_machine_power_off(void) { + if (!pm_power_off) + pm_power_off = pm_power_off_default; + if (pm_power_off) { if (!reboot_force) machine_shutdown(); diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c index 2d66db8..8c8e3c9 100644 --- a/arch/x86/platform/efi/quirks.c +++ b/arch/x86/platform/efi/quirks.c @@ -297,3 +297,8 @@ bool efi_poweroff_required(void) { return !!acpi_gbl_reduced_hardware; } + +bool efi_poweroff_default(void) +{ + return (!pm_power_off_default && acpi_no_s5); +} diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index 9cb9752..a244f15 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -25,6 +25,11 @@ #include "internal.h" #include "sleep.h" +/* + * Some HW-full platforms do not have _S5, they may have + * to leverage other methods rather than acpi for a shutdown. + */ +bool acpi_no_s5; static u8 sleep_states[ACPI_S_STATE_COUNT]; static void acpi_sleep_tts_switch(u32 acpi_state) @@ -846,6 +851,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/drivers/firmware/efi/reboot.c b/drivers/firmware/efi/reboot.c index 9c59d1c..b3345e2 100644 --- a/drivers/firmware/efi/reboot.c +++ b/drivers/firmware/efi/reboot.c @@ -38,6 +38,11 @@ bool __weak efi_poweroff_required(void) return false; } +bool __weak efi_poweroff_default(void) +{ + return false; +} + static void efi_power_off(void) { efi.reset_system(EFI_RESET_SHUTDOWN, EFI_SUCCESS, 0, NULL); @@ -51,6 +56,9 @@ static int __init efi_shutdown_init(void) if (efi_poweroff_required()) pm_power_off = efi_power_off; + if (efi_poweroff_default()) + pm_power_off_default = efi_power_off; + return 0; } late_initcall(efi_shutdown_init); diff --git a/include/linux/acpi.h b/include/linux/acpi.h index 06ed7e5..4d2e67f 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -278,6 +278,7 @@ void acpi_irq_stats_init(void); extern u32 acpi_irq_handled; extern u32 acpi_irq_not_handled; extern unsigned int acpi_sci_irq; +extern bool acpi_no_s5; #define INVALID_ACPI_IRQ ((unsigned)-1) static inline bool acpi_sci_irq_valid(void) { diff --git a/include/linux/efi.h b/include/linux/efi.h index 47be3ad..5aad9be 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -921,6 +921,7 @@ extern struct kobject *efi_kobj; extern int efi_reboot_quirk_mode; extern bool efi_poweroff_required(void); +extern bool efi_poweroff_default(void); #ifdef CONFIG_EFI_FAKE_MEMMAP extern void __init efi_fake_memmap(void); diff --git a/include/linux/pm.h b/include/linux/pm.h index 6a5d654..b58719f 100644 --- a/include/linux/pm.h +++ b/include/linux/pm.h @@ -33,6 +33,7 @@ */ extern void (*pm_power_off)(void); extern void (*pm_power_off_prepare)(void); +extern void (*pm_power_off_default)(void); struct device; /* we have a circular dep with device.h */ #ifdef CONFIG_VT_CONSOLE_SLEEP diff --git a/kernel/reboot.c b/kernel/reboot.c index bd30a97..b9a0f36 100644 --- a/kernel/reboot.c +++ b/kernel/reboot.c @@ -49,6 +49,7 @@ int reboot_force; */ void (*pm_power_off_prepare)(void); +void (*pm_power_off_default)(void); /** * emergency_restart - reboot the system
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 on non-legacy platforms. That should handle both the HW reduced mode, and the HW-full mode where the DSDT fails to supply an _S5 object. This patch introduces pm_power_off_default which would be used by pm_power_off if nothing else is available. And in this case we leverage efi power off to be this role. However since efi power off may not be stable enough thus in order not to interfere with other poweroff path, we only make a minimum enhancement for x86 in native_machine_power_off. Suggested-by: Len Brown <len.brown@intel.com> Signed-off-by: Chen Yu <yu.c.chen@intel.com> --- v4: - Since in v3 efi_poweroff_required() is not guaranteed to run after all of the other code that may register alternative power off handling, add the pm_power_off_default that would be used by pm_power_off if nothing else is available. v3: - Only assign pm_power_off to efi_power_off when there are no other pm_power_off registered at that time, in case other commponents would like to customize their own implementation. --- v2: - Convert the acpi_no_s5 to a global bool variable in sleep.c and add a declaration to include/linux/acpi.h. --- arch/x86/kernel/reboot.c | 3 +++ arch/x86/platform/efi/quirks.c | 5 +++++ drivers/acpi/sleep.c | 7 +++++++ drivers/firmware/efi/reboot.c | 8 ++++++++ include/linux/acpi.h | 1 + include/linux/efi.h | 1 + include/linux/pm.h | 1 + kernel/reboot.c | 1 + 8 files changed, 27 insertions(+)