Re: [PATCH] ACPI: introduce kernel parameter acpi_sleep=s3_set_sci_en

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday 27 October 2009, Zhang Rui wrote:
> On Tue, 2009-10-27 at 16:21 +0800, Rafael J. Wysocki wrote:
> > On Tuesday 27 October 2009, Zhang Rui wrote:
> > > Introduce kernel parameter acpi_sleep=s3_set_sci_en
> > > 
> > > 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 lattops 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>
> > 
> > I'm generally fine with this, but ...
> > 
> > > ---
> > >  Documentation/kernel-parameters.txt |    5 ++++-
> > >  arch/x86/kernel/acpi/sleep.c        |    4 ++++
> > >  drivers/acpi/sleep.c                |    6 ++++++
> > >  include/linux/acpi.h                |    3 +++
> > >  4 files changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6/drivers/acpi/sleep.c
> > > ===================================================================
> > > --- linux-2.6.orig/drivers/acpi/sleep.c
> > > +++ linux-2.6/drivers/acpi/sleep.c
> > > @@ -182,6 +182,12 @@ static void acpi_pm_end(void)
> > >   */
> > >  static bool set_sci_en_on_resume;
> > >  
> > > +void __init acpi_set_sci_en_on_resume(void)
> > > +{
> > > +	set_sci_en_on_resume = true;
> > > +	return 0;
> > > +}
> > > +
> > >  extern void do_suspend_lowlevel(void);
> > >  
> > >  static u32 acpi_suspend_states[] = {
> > > Index: linux-2.6/arch/x86/kernel/acpi/sleep.c
> > > ===================================================================
> > > --- linux-2.6.orig/arch/x86/kernel/acpi/sleep.c
> > > +++ linux-2.6/arch/x86/kernel/acpi/sleep.c
> > > @@ -162,6 +162,10 @@ static int __init acpi_sleep_setup(char 
> > >  #endif
> > >  		if (strncmp(str, "old_ordering", 12) == 0)
> > >  			acpi_old_suspend_ordering();
> > > +#ifdef CONFIG_SUSPEND
> > > +		if (strncmp(str, "s3_set_sci_en", 13) == 0)
> > > +			acpi_set_sci_en_on_resume();
> > > +#endif /* CONFIG_SUSPEND */
> > 
> > ... do we really need these #ifdefs?
> > 
> 
> #ifdef CONFIG_SUSPEND
> ...
> static bool set_sci_en_on_resume;
> ...
> #endif /* CONFIG_SUSPEND */
> 
> Or else I have to move set_sci_en_on_resume out of this #ifdef.

Please do.

Best,
Rafael