Re: [PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

Re: [PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

[Robert Hancock]

Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume
> 
> Some Apple boxes evidently require us to set SCI_EN on resume
> directly, because if we don't do that, they hung somewhere in the
> resume code path.  Moreover, on these boxes it is not sufficient to
> use acpi_enable() to turn ACPI on during resume.  All of this is
> against the ACPI specification which states that (1) the BIOS is
> supposed to return from the S3 sleep state with ACPI enabled
> (SCI_EN set) and (2) the SCI_EN bit is owned by the hardware and we
> are not supposed to change it.
> 
> For this reason, blacklist the affected systems so that the SCI_EN
> bit is set during resume on them.
> 
> [NOTE: Unconditional setting SCI_EN for all system on resume doesn't
>  work, because it makes some other systems crash (that's to be
>  expected).  Also, it is not entirely clear right now if all of the
>  Apple boxes require this workaround.]

This is rather gross. The usual question arises, why does Windows work 
without such a hack? Maybe there is no better solution for now, but 
somebody should really look into why the behavior is different in Linux..

Re: [PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

[Bob Copeland]

On Mon, Nov 17, 2008 at 11:45:59PM -0600, Robert Hancock wrote:
> This is rather gross. The usual question arises, why does Windows work 
> without such a hack? Maybe there is no better solution for now, but 
> somebody should really look into why the behavior is different in Linux..

Except in this case, why does OS X work without such a hack :)

This is fallout from the acpi_enable() change for MSI netbooks IIRC,
another fixup for another broken ACPI implementation.  So it goes.

-- 
Bob Copeland %% www.bobcopeland.com

Re: [PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

[Rafael J. Wysocki]

On Tuesday, 18 of November 2008, Robert Hancock wrote:
> Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > Subject: ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume
> > 
> > Some Apple boxes evidently require us to set SCI_EN on resume
> > directly, because if we don't do that, they hung somewhere in the
> > resume code path.  Moreover, on these boxes it is not sufficient to
> > use acpi_enable() to turn ACPI on during resume.  All of this is
> > against the ACPI specification which states that (1) the BIOS is
> > supposed to return from the S3 sleep state with ACPI enabled
> > (SCI_EN set) and (2) the SCI_EN bit is owned by the hardware and we
> > are not supposed to change it.
> > 
> > For this reason, blacklist the affected systems so that the SCI_EN
> > bit is set during resume on them.
> > 
> > [NOTE: Unconditional setting SCI_EN for all system on resume doesn't
> >  work, because it makes some other systems crash (that's to be
> >  expected).  Also, it is not entirely clear right now if all of the
> >  Apple boxes require this workaround.]
> 
> This is rather gross. The usual question arises, why does Windows work 
> without such a hack? Maybe there is no better solution for now, but 
> somebody should really look into why the behavior is different in Linux..

I discussed this with Len and we agreed that the best thing we can do is to
blacklist those boxes.

In fact, the SCI_EN flag is already forcibly set in irqrouter_resume() due to
the very same Apple BIOS borkage and what this patch actually does is to
move that as early in the resume code path as possible.  Also it generally
is inappropriate to set SCI_EN for all systems unconditionally (I'm planning
to remove this from irqrouter_resume() in .29, because it's a noop after the
$subject patch).

Apparently, acpi_enable() in acpi_suspend_enter() doesn't work on the Apple
boxes and that's why the $subject patch is necessary.  It would be nice to know
_why_ it doesn't work on them, but I'm not sure if we can find that out.

Thanks,
Rafael

Re: [PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

[Pavel Machek]

Hi!

> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume
> 
> Some Apple boxes evidently require us to set SCI_EN on resume
> directly, because if we don't do that, they hung somewhere in the
> resume code path.  Moreover, on these boxes it is not sufficient to
> use acpi_enable() to turn ACPI on during resume.  All of this is
> against the ACPI specification which states that (1) the BIOS is
> supposed to return from the S3 sleep state with ACPI enabled
> (SCI_EN set) and (2) the SCI_EN bit is owned by the hardware and we
> are not supposed to change it.
> 
> For this reason, blacklist the affected systems so that the SCI_EN
> bit is set during resume on them.
> 
> [NOTE: Unconditional setting SCI_EN for all system on resume doesn't
>  work, because it makes some other systems crash (that's to be
>  expected).  Also, it is not entirely clear right now if all of the
>  Apple boxes require this workaround.]


Silent bug workarounds are evil. In this case, MSI insisted their BIOS
is correct because it works with opensuse11, and forced us to backport
workaround to sled instead of fixing their bios :-(.


> This patch fixes the recent regression tracked as
> http://bugzilla.kernel.org/show_bug.cgi?id=12038
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> Tested-by: Tino Keitel <tino.keitel@gmx.de>
> Tested-by: Bob Copeland <me@bobcopeland.com>

ACK.


> ---
>  drivers/acpi/sleep/main.c |   40 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 39 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/drivers/acpi/sleep/main.c
> ===================================================================
> --- linux-2.6.orig/drivers/acpi/sleep/main.c
> +++ linux-2.6/drivers/acpi/sleep/main.c
> @@ -104,6 +104,18 @@ void __init acpi_s4_no_nvs(void)
>  	s4_no_nvs = true;
>  }
>  
> +/*
> + * 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;
> +
>  /**
>   *	acpi_pm_disable_gpes - Disable the GPEs.
>   */
> @@ -249,7 +261,11 @@ static int acpi_suspend_enter(suspend_st
>  	}
>  
>  	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
> -	acpi_enable();
> +	if (set_sci_en_on_resume)
> +		acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1);
> +	else
> +		acpi_enable();
> +
>  	/* Reprogram control registers and execute _BFS */
>  	acpi_leave_sleep_state_prep(acpi_state);
>  
> @@ -337,6 +353,12 @@ static int __init init_old_suspend_order
>  	return 0;
>  }
>  
> +static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d)
> +{
> +	set_sci_en_on_resume = true;
> +	return 0;
> +}
> +
>  static struct dmi_system_id __initdata acpisleep_dmi_table[] = {
>  	{
>  	.callback = init_old_suspend_ordering,
> @@ -354,6 +376,22 @@ static struct dmi_system_id __initdata a
>  		DMI_MATCH(DMI_PRODUCT_NAME, "HP xw4600 Workstation"),
>  		},
>  	},
> +	{
> +	.callback = init_set_sci_en_on_resume,
> +	.ident = "Apple MacBook 1,1",
> +	.matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"),
> +		},
> +	},
> +	{
> +	.callback = init_set_sci_en_on_resume,
> +	.ident = "Apple MacMini 1,1",
> +	.matches = {
> +		DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."),
> +		DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"),
> +		},
> +	},
>  	{},
>  };
>  #endif /* CONFIG_SUSPEND */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH] ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: ACPI suspend: Blacklist boxes that require us to set SCI_EN directly on resume

Some Apple boxes evidently require us to set SCI_EN on resume
directly, because if we don't do that, they hung somewhere in the
resume code path.  Moreover, on these boxes it is not sufficient to
use acpi_enable() to turn ACPI on during resume.  All of this is
against the ACPI specification which states that (1) the BIOS is
supposed to return from the S3 sleep state with ACPI enabled
(SCI_EN set) and (2) the SCI_EN bit is owned by the hardware and we
are not supposed to change it.

For this reason, blacklist the affected systems so that the SCI_EN
bit is set during resume on them.

[NOTE: Unconditional setting SCI_EN for all system on resume doesn't
 work, because it makes some other systems crash (that's to be
 expected).  Also, it is not entirely clear right now if all of the
 Apple boxes require this workaround.]

This patch fixes the recent regression tracked as
http://bugzilla.kernel.org/show_bug.cgi?id=12038

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
Tested-by: Tino Keitel <tino.keitel@gmx.de>
Tested-by: Bob Copeland <me@bobcopeland.com>
---
 drivers/acpi/sleep/main.c |   40 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 39 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/acpi/sleep/main.c
===================================================================
--- linux-2.6.orig/drivers/acpi/sleep/main.c
+++ linux-2.6/drivers/acpi/sleep/main.c
@@ -104,6 +104,18 @@ void __init acpi_s4_no_nvs(void)
 	s4_no_nvs = true;
 }
 
+/*
+ * 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;
+
 /**
  *	acpi_pm_disable_gpes - Disable the GPEs.
  */
@@ -249,7 +261,11 @@ static int acpi_suspend_enter(suspend_st
 	}
 
 	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
-	acpi_enable();
+	if (set_sci_en_on_resume)
+		acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1);
+	else
+		acpi_enable();
+
 	/* Reprogram control registers and execute _BFS */
 	acpi_leave_sleep_state_prep(acpi_state);
 
@@ -337,6 +353,12 @@ static int __init init_old_suspend_order
 	return 0;
 }
 
+static int __init init_set_sci_en_on_resume(const struct dmi_system_id *d)
+{
+	set_sci_en_on_resume = true;
+	return 0;
+}
+
 static struct dmi_system_id __initdata acpisleep_dmi_table[] = {
 	{
 	.callback = init_old_suspend_ordering,
@@ -354,6 +376,22 @@ static struct dmi_system_id __initdata a
 		DMI_MATCH(DMI_PRODUCT_NAME, "HP xw4600 Workstation"),
 		},
 	},
+	{
+	.callback = init_set_sci_en_on_resume,
+	.ident = "Apple MacBook 1,1",
+	.matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "MacBook1,1"),
+		},
+	},
+	{
+	.callback = init_set_sci_en_on_resume,
+	.ident = "Apple MacMini 1,1",
+	.matches = {
+		DMI_MATCH(DMI_SYS_VENDOR, "Apple Computer, Inc."),
+		DMI_MATCH(DMI_PRODUCT_NAME, "Macmini1,1"),
+		},
+	},
 	{},
 };
 #endif /* CONFIG_SUSPEND */