| Message ID | 1271957505-23138-2-git-send-email-apw@canonical.com |
|---|---|
| State | Accepted |
| Delegated to: | Andy Whitcroft |
| Headers | show |
On Thu, Apr 22, 2010 at 1:31 PM, Andy Whitcroft <apw@canonical.com> wrote: > From: Zhang Rui <rui.zhang@intel.com> > > Introduce kernel parameter acpi_sleep=sci_force_enable > > some laptop requires SCI_EN being set directly on resume, > or else they hung somewhere in the resume code path. > > We already have a blacklist for these laptops but we still need > this option, especially when debugging some suspend/resume problems, > in case there are systems that need this workaround and are not yet > in the blacklist. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > Signed-off-by: Len Brown <len.brown@intel.com> > > (cherry picked from commit d7f0eea9e431e1b8b0742a74db1a9490730b2a25 upstream) > BugLink: http://bugs.launchpad.net/bugs/553498 > > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > Documentation/kernel-parameters.txt | 5 ++++- > arch/x86/kernel/acpi/sleep.c | 2 ++ > drivers/acpi/sleep.c | 29 +++++++++++++++++------------ > include/linux/acpi.h | 1 + > 4 files changed, 24 insertions(+), 13 deletions(-) > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index 345c399..5f6aa11 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -241,7 +241,7 @@ and is between 256 and 4096 characters. It is defined in the file > > acpi_sleep= [HW,ACPI] Sleep options > Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig, > - old_ordering, s4_nonvs } > + old_ordering, s4_nonvs, sci_force_enable } > See Documentation/power/video.txt for information on > s3_bios and s3_mode. > s3_beep is for debugging; it makes the PC's speaker beep > @@ -254,6 +254,9 @@ and is between 256 and 4096 characters. It is defined in the file > of _PTS is used by default). > s4_nonvs prevents the kernel from saving/restoring the > ACPI NVS memory during hibernation. > + sci_force_enable causes the kernel to set SCI_EN directly > + on resume from S1/S3 (which is against the ACPI spec, > + but some broken systems don't work without it). > > acpi_use_timer_override [HW,ACPI] > Use timer override. For some broken Nvidia NF5 boards > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > index ca93638..8b85734 100644 > --- a/arch/x86/kernel/acpi/sleep.c > +++ b/arch/x86/kernel/acpi/sleep.c > @@ -162,6 +162,8 @@ static int __init acpi_sleep_setup(char *str) > #endif > if (strncmp(str, "old_ordering", 12) == 0) > acpi_old_suspend_ordering(); > + if (strncmp(str, "sci_force_enable", 16) == 0) > + acpi_set_sci_en_on_resume(); > str = strchr(str, ','); > if (str != NULL) > str += strspn(str, ", \t"); > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index cf3101f..22f4831 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -81,6 +81,23 @@ static int acpi_sleep_prepare(u32 acpi_state) > #ifdef CONFIG_ACPI_SLEEP > static u32 acpi_target_sleep_state = ACPI_STATE_S0; > /* > + * According to the ACPI specification the BIOS should make sure that ACPI is > + * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, > + * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI > + * on such systems during resume. Unfortunately that doesn't help in > + * particularly pathological cases in which SCI_EN has to be set directly on > + * resume, although the specification states very clearly that this flag is > + * owned by the hardware. The set_sci_en_on_resume variable will be set in such > + * cases. > + */ > +static bool set_sci_en_on_resume; > + > +void __init acpi_set_sci_en_on_resume(void) > +{ > + set_sci_en_on_resume = true; > +} > + > +/* > * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the > * user to request that behavior by using the 'acpi_old_suspend_ordering' > * kernel command line option that causes the following variable to be set. > @@ -170,18 +187,6 @@ static void acpi_pm_end(void) > #endif /* CONFIG_ACPI_SLEEP */ > > #ifdef CONFIG_SUSPEND > -/* > - * According to the ACPI specification the BIOS should make sure that ACPI is > - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, > - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI > - * on such systems during resume. Unfortunately that doesn't help in > - * particularly pathological cases in which SCI_EN has to be set directly on > - * resume, although the specification states very clearly that this flag is > - * owned by the hardware. The set_sci_en_on_resume variable will be set in such > - * cases. > - */ > -static bool set_sci_en_on_resume; > - > extern void do_suspend_lowlevel(void); > > static u32 acpi_suspend_states[] = { > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c010b94..07432a1 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -251,6 +251,7 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, > void __init acpi_no_s4_hw_signature(void); > void __init acpi_old_suspend_ordering(void); > void __init acpi_s4_no_nvs(void); > +void __init acpi_set_sci_en_on_resume(void); > #endif /* CONFIG_PM_SLEEP */ > > struct acpi_osc_context { > -- > 1.7.0.4 This all looks reasonable to me, and gives us an easier way to debug this issue when it crops up again. Acked-by: Chase Douglas <chase.douglas@canonical.com>
On Thu, 2010-04-22 at 18:31 +0100, Andy Whitcroft wrote: > From: Zhang Rui <rui.zhang@intel.com> > > Introduce kernel parameter acpi_sleep=sci_force_enable > I think this is a great idea, but how about making the kernel parameter able to force the SCI_EN behavior on /or/ force it off? So that if the parameter is specified at all then it overrides the dmi-based blacklist altogether. I.e. something like acpi_sleep_force_sci=on and acpi_sleep_force_sci=off. -Kamal > some laptop requires SCI_EN being set directly on resume, > or else they hung somewhere in the resume code path. > > We already have a blacklist for these laptops but we still need > this option, especially when debugging some suspend/resume problems, > in case there are systems that need this workaround and are not yet > in the blacklist. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > Acked-by: Rafael J. Wysocki <rjw@sisk.pl> > Signed-off-by: Len Brown <len.brown@intel.com> > > (cherry picked from commit d7f0eea9e431e1b8b0742a74db1a9490730b2a25 upstream) > BugLink: http://bugs.launchpad.net/bugs/553498 > > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > Documentation/kernel-parameters.txt | 5 ++++- > arch/x86/kernel/acpi/sleep.c | 2 ++ > drivers/acpi/sleep.c | 29 +++++++++++++++++------------ > include/linux/acpi.h | 1 + > 4 files changed, 24 insertions(+), 13 deletions(-) > > diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt > index 345c399..5f6aa11 100644 > --- a/Documentation/kernel-parameters.txt > +++ b/Documentation/kernel-parameters.txt > @@ -241,7 +241,7 @@ and is between 256 and 4096 characters. It is defined in the file > > acpi_sleep= [HW,ACPI] Sleep options > Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig, > - old_ordering, s4_nonvs } > + old_ordering, s4_nonvs, sci_force_enable } > See Documentation/power/video.txt for information on > s3_bios and s3_mode. > s3_beep is for debugging; it makes the PC's speaker beep > @@ -254,6 +254,9 @@ and is between 256 and 4096 characters. It is defined in the file > of _PTS is used by default). > s4_nonvs prevents the kernel from saving/restoring the > ACPI NVS memory during hibernation. > + sci_force_enable causes the kernel to set SCI_EN directly > + on resume from S1/S3 (which is against the ACPI spec, > + but some broken systems don't work without it). > > acpi_use_timer_override [HW,ACPI] > Use timer override. For some broken Nvidia NF5 boards > diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c > index ca93638..8b85734 100644 > --- a/arch/x86/kernel/acpi/sleep.c > +++ b/arch/x86/kernel/acpi/sleep.c > @@ -162,6 +162,8 @@ static int __init acpi_sleep_setup(char *str) > #endif > if (strncmp(str, "old_ordering", 12) == 0) > acpi_old_suspend_ordering(); > + if (strncmp(str, "sci_force_enable", 16) == 0) > + acpi_set_sci_en_on_resume(); > str = strchr(str, ','); > if (str != NULL) > str += strspn(str, ", \t"); > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index cf3101f..22f4831 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -81,6 +81,23 @@ static int acpi_sleep_prepare(u32 acpi_state) > #ifdef CONFIG_ACPI_SLEEP > static u32 acpi_target_sleep_state = ACPI_STATE_S0; > /* > + * According to the ACPI specification the BIOS should make sure that ACPI is > + * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, > + * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI > + * on such systems during resume. Unfortunately that doesn't help in > + * particularly pathological cases in which SCI_EN has to be set directly on > + * resume, although the specification states very clearly that this flag is > + * owned by the hardware. The set_sci_en_on_resume variable will be set in such > + * cases. > + */ > +static bool set_sci_en_on_resume; > + > +void __init acpi_set_sci_en_on_resume(void) > +{ > + set_sci_en_on_resume = true; > +} > + > +/* > * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the > * user to request that behavior by using the 'acpi_old_suspend_ordering' > * kernel command line option that causes the following variable to be set. > @@ -170,18 +187,6 @@ static void acpi_pm_end(void) > #endif /* CONFIG_ACPI_SLEEP */ > > #ifdef CONFIG_SUSPEND > -/* > - * According to the ACPI specification the BIOS should make sure that ACPI is > - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, > - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI > - * on such systems during resume. Unfortunately that doesn't help in > - * particularly pathological cases in which SCI_EN has to be set directly on > - * resume, although the specification states very clearly that this flag is > - * owned by the hardware. The set_sci_en_on_resume variable will be set in such > - * cases. > - */ > -static bool set_sci_en_on_resume; > - > extern void do_suspend_lowlevel(void); > > static u32 acpi_suspend_states[] = { > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > index c010b94..07432a1 100644 > --- a/include/linux/acpi.h > +++ b/include/linux/acpi.h > @@ -251,6 +251,7 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, > void __init acpi_no_s4_hw_signature(void); > void __init acpi_old_suspend_ordering(void); > void __init acpi_s4_no_nvs(void); > +void __init acpi_set_sci_en_on_resume(void); > #endif /* CONFIG_PM_SLEEP */ > > struct acpi_osc_context { > -- > 1.7.0.4 > >
On Thu, Apr 22, 2010 at 10:56:48AM -0700, Kamal Mostafa wrote: > On Thu, 2010-04-22 at 18:31 +0100, Andy Whitcroft wrote: > > From: Zhang Rui <rui.zhang@intel.com> > > > > Introduce kernel parameter acpi_sleep=sci_force_enable > > > > I think this is a great idea, but how about making the kernel parameter > able to force the SCI_EN behavior on /or/ force it off? So that if the > parameter is specified at all then it overrides the dmi-based blacklist > altogether. I.e. something like acpi_sleep_force_sci=on and > acpi_sleep_force_sci=off. As far as I can tell allowing you to force SCI_EN off makes no sense, the docs define it not being on on return from suspend as 'wrong'. Therefore I do not see any value in that. As an aside this commit is an unchanged cherry-pick of the upstream patch. -apw
diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt index 345c399..5f6aa11 100644 --- a/Documentation/kernel-parameters.txt +++ b/Documentation/kernel-parameters.txt @@ -241,7 +241,7 @@ and is between 256 and 4096 characters. It is defined in the file acpi_sleep= [HW,ACPI] Sleep options Format: { s3_bios, s3_mode, s3_beep, s4_nohwsig, - old_ordering, s4_nonvs } + old_ordering, s4_nonvs, sci_force_enable } See Documentation/power/video.txt for information on s3_bios and s3_mode. s3_beep is for debugging; it makes the PC's speaker beep @@ -254,6 +254,9 @@ and is between 256 and 4096 characters. It is defined in the file of _PTS is used by default). s4_nonvs prevents the kernel from saving/restoring the ACPI NVS memory during hibernation. + sci_force_enable causes the kernel to set SCI_EN directly + on resume from S1/S3 (which is against the ACPI spec, + but some broken systems don't work without it). acpi_use_timer_override [HW,ACPI] Use timer override. For some broken Nvidia NF5 boards diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c index ca93638..8b85734 100644 --- a/arch/x86/kernel/acpi/sleep.c +++ b/arch/x86/kernel/acpi/sleep.c @@ -162,6 +162,8 @@ static int __init acpi_sleep_setup(char *str) #endif if (strncmp(str, "old_ordering", 12) == 0) acpi_old_suspend_ordering(); + if (strncmp(str, "sci_force_enable", 16) == 0) + acpi_set_sci_en_on_resume(); str = strchr(str, ','); if (str != NULL) str += strspn(str, ", \t"); diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c index cf3101f..22f4831 100644 --- a/drivers/acpi/sleep.c +++ b/drivers/acpi/sleep.c @@ -81,6 +81,23 @@ static int acpi_sleep_prepare(u32 acpi_state) #ifdef CONFIG_ACPI_SLEEP static u32 acpi_target_sleep_state = ACPI_STATE_S0; /* + * According to the ACPI specification the BIOS should make sure that ACPI is + * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, + * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI + * on such systems during resume. Unfortunately that doesn't help in + * particularly pathological cases in which SCI_EN has to be set directly on + * resume, although the specification states very clearly that this flag is + * owned by the hardware. The set_sci_en_on_resume variable will be set in such + * cases. + */ +static bool set_sci_en_on_resume; + +void __init acpi_set_sci_en_on_resume(void) +{ + set_sci_en_on_resume = true; +} + +/* * ACPI 1.0 wants us to execute _PTS before suspending devices, so we allow the * user to request that behavior by using the 'acpi_old_suspend_ordering' * kernel command line option that causes the following variable to be set. @@ -170,18 +187,6 @@ static void acpi_pm_end(void) #endif /* CONFIG_ACPI_SLEEP */ #ifdef CONFIG_SUSPEND -/* - * According to the ACPI specification the BIOS should make sure that ACPI is - * enabled and SCI_EN bit is set on wake-up from S1 - S3 sleep states. Still, - * some BIOSes don't do that and therefore we use acpi_enable() to enable ACPI - * on such systems during resume. Unfortunately that doesn't help in - * particularly pathological cases in which SCI_EN has to be set directly on - * resume, although the specification states very clearly that this flag is - * owned by the hardware. The set_sci_en_on_resume variable will be set in such - * cases. - */ -static bool set_sci_en_on_resume; - extern void do_suspend_lowlevel(void); static u32 acpi_suspend_states[] = { diff --git a/include/linux/acpi.h b/include/linux/acpi.h index c010b94..07432a1 100644 --- a/include/linux/acpi.h +++ b/include/linux/acpi.h @@ -251,6 +251,7 @@ int acpi_check_mem_region(resource_size_t start, resource_size_t n, void __init acpi_no_s4_hw_signature(void); void __init acpi_old_suspend_ordering(void); void __init acpi_s4_no_nvs(void); +void __init acpi_set_sci_en_on_resume(void); #endif /* CONFIG_PM_SLEEP */ struct acpi_osc_context {