[PATCH] acpi: Fall back to manually changing SCI_EN

[PATCH] acpi: Fall back to manually changing SCI_EN

[Matthew Garrett]

The ACPI spec tells us that the ACPI SCI_EN bit is under hardware control
and shouldn't be touched by the OS. It seems that the Leading Other OS
ignores this and some machines expect this behaviour. We have a blacklist
for these, but given that we're able to detect the failure case and the
alternative to breaking the spec is letting the machine crash and burn,
let's try falling back when we know the alternative is a mostly-dead
machine.

Signed-off-by: Matthew Garrett <mjg@redhat.com>
---
 drivers/acpi/sleep.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index f74834a..79df8d4 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -227,6 +227,7 @@ static int acpi_suspend_begin(suspend_state_t pm_state)
 static int acpi_suspend_enter(suspend_state_t pm_state)
 {
 	acpi_status status = AE_OK;
+	acpi_status enable_status = AE_OK;
 	unsigned long flags = 0;
 	u32 acpi_state = acpi_target_sleep_state;
 
@@ -254,10 +255,19 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
 	}
 
 	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
-	if (set_sci_en_on_resume)
+	if (!set_sci_en_on_resume)
+		enable_status = acpi_enable();
+
+	if (set_sci_en_on_resume || enable_status == AE_NO_HARDWARE_RESPONSE)
+		/* If we're still in legacy mode then we have a problem. The
+		 * spec tells us that this bit is under hardware control, but
+		 * there's no plausible way that the OS can transition back to
+		 * legacy mode so our choices here are to either ignore the
+		 * spec or crash and burn horribly. The latter doesn't seem
+		 * like it's ever going to be the preferable choice, so let's
+		 * live dangerously.
+		 */
 		acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
-	else
-		acpi_enable();
 
 	/* Reprogram control registers and execute _BFS */
 	acpi_leave_sleep_state_prep(acpi_state);
-- 
1.7.0.1

Re: [PATCH] acpi: Fall back to manually changing SCI_EN

[Alex Chiang]

* Matthew Garrett <mjg@redhat.com>:
> The ACPI spec tells us that the ACPI SCI_EN bit is under hardware control
> and shouldn't be touched by the OS. It seems that the Leading Other OS
> ignores this and some machines expect this behaviour. We have a blacklist
> for these, but given that we're able to detect the failure case and the
> alternative to breaking the spec is letting the machine crash and burn,
> let's try falling back when we know the alternative is a mostly-dead
> machine.

Yes, we got a hint from a Lenovo BIOS developer:

	A SCI_EN bit had not been set at S3 resume post. It
	should be set as ACPI defines. It seems that Windows OS
	sets SCI_EN bit by itself after S3 resume....

So I believe that Matthew's approach is reasonably safe and
correct.

Acked-by: Alex Chiang <achiang@canonical.com>

/ac

RE: [PATCH] acpi: Fall back to manually changing SCI_EN

[Rezwanul_Kabir]

Agree with getting Matthew's patch to fall back on force setting SCI_EN bit. I have discovered this issue recently with New Gen Dell systems where the systems were suspending right the first time around and then resorted to rebooting ( instead of resuming ) the second time. Also, the system would come with SCI interrupt disabled the first time around.
 I worked with the BIOS teams to get this fixed in the latest BIOS revs but just as Alex pointed out Matthew's approach seems to be the default bahaviour in the "Other OS". The problem may be more widespread than the limited set of blacklisted platforms..

Thanks..
   --rez
 

Rezwanul Kabir
Dell Inc.
512-725-0766
 

>-----Original Message-----
>From: linux-acpi-owner@vger.kernel.org 
>[mailto:linux-acpi-owner@vger.kernel.org] On Behalf Of Alex Chiang
>Sent: Tuesday, April 20, 2010 10:00 AM
>To: Matthew Garrett
>Cc: linux-kernel@vger.kernel.org; linux-acpi@vger.kernel.org; 
>lenb@kernel.org
>Subject: Re: [PATCH] acpi: Fall back to manually changing SCI_EN
>
>* Matthew Garrett <mjg@redhat.com>:
>> The ACPI spec tells us that the ACPI SCI_EN bit is under hardware 
>> control and shouldn't be touched by the OS. It seems that 
>the Leading 
>> Other OS ignores this and some machines expect this 
>behaviour. We have 
>> a blacklist for these, but given that we're able to detect 
>the failure 
>> case and the alternative to breaking the spec is letting the machine 
>> crash and burn, let's try falling back when we know the 
>alternative is 
>> a mostly-dead machine.
>
>Yes, we got a hint from a Lenovo BIOS developer:
>
>	A SCI_EN bit had not been set at S3 resume post. It
>	should be set as ACPI defines. It seems that Windows OS
>	sets SCI_EN bit by itself after S3 resume....
>
>So I believe that Matthew's approach is reasonably safe and correct.
>
>Acked-by: Alex Chiang <achiang@canonical.com>
>
>/ac
>--
>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
>

Re: [PATCH] acpi: Fall back to manually changing SCI_EN

[Rafael J. Wysocki]

On Monday 19 April 2010, Matthew Garrett wrote:
> The ACPI spec tells us that the ACPI SCI_EN bit is under hardware control
> and shouldn't be touched by the OS. It seems that the Leading Other OS
> ignores this and some machines expect this behaviour. We have a blacklist
> for these, but given that we're able to detect the failure case and the
> alternative to breaking the spec is letting the machine crash and burn,
> let's try falling back when we know the alternative is a mostly-dead
> machine.
> 
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

I guess we can try that, but I'd prefer it if that went into .35.

It _should_ be safe, but ...

Anyway, Acked-by: Rafael J. Wysocki <rjw@sisk.pl>

> ---
>  drivers/acpi/sleep.c |   16 +++++++++++++---
>  1 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index f74834a..79df8d4 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -227,6 +227,7 @@ static int acpi_suspend_begin(suspend_state_t pm_state)
>  static int acpi_suspend_enter(suspend_state_t pm_state)
>  {
>  	acpi_status status = AE_OK;
> +	acpi_status enable_status = AE_OK;
>  	unsigned long flags = 0;
>  	u32 acpi_state = acpi_target_sleep_state;
>  
> @@ -254,10 +255,19 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
>  	}
>  
>  	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
> -	if (set_sci_en_on_resume)
> +	if (!set_sci_en_on_resume)
> +		enable_status = acpi_enable();
> +
> +	if (set_sci_en_on_resume || enable_status == AE_NO_HARDWARE_RESPONSE)
> +		/* If we're still in legacy mode then we have a problem. The
> +		 * spec tells us that this bit is under hardware control, but
> +		 * there's no plausible way that the OS can transition back to
> +		 * legacy mode so our choices here are to either ignore the
> +		 * spec or crash and burn horribly. The latter doesn't seem
> +		 * like it's ever going to be the preferable choice, so let's
> +		 * live dangerously.
> +		 */
>  		acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> -	else
> -		acpi_enable();
>  
>  	/* Reprogram control registers and execute _BFS */
>  	acpi_leave_sleep_state_prep(acpi_state);

Re: [PATCH] acpi: Fall back to manually changing SCI_EN

[Len Brown]

On Wed, 28 Apr 2010, Rafael J. Wysocki wrote:

> On Monday 19 April 2010, Matthew Garrett wrote:
> > The ACPI spec tells us that the ACPI SCI_EN bit is under hardware control
> > and shouldn't be touched by the OS. It seems that the Leading Other OS
> > ignores this and some machines expect this behaviour. We have a blacklist
> > for these, but given that we're able to detect the failure case and the
> > alternative to breaking the spec is letting the machine crash and burn,
> > let's try falling back when we know the alternative is a mostly-dead
> > machine.
> > 
> > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> I guess we can try that, but I'd prefer it if that went into .35.
> 
> It _should_ be safe, but ...
> 
> Anyway, Acked-by: Rafael J. Wysocki <rjw@sisk.pl>


I'd like to push this patch in .34,
and in .35 remove the DMI list and the boot option.

One thing that has bothered me about the boot option
is that it writes SCI_EN instead of callin acpi_enable()
as opposed to in-addition-to acpi_enable().
If all acpi_enable() did was set SCI_EN, then it would
not make a difference.  However, acpi_enable() writes
SMI_CMD and we really have no idea what other stuff
the BIOS may do in SMM on this transition.

So if we don't end up reverting this one,
I'd really like to see the boot option gone in .35.

thanks,
Len Brown, Intel Open Source Technology Center
-
> > ---
> >  drivers/acpi/sleep.c |   16 +++++++++++++---
> >  1 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> > index f74834a..79df8d4 100644
> > --- a/drivers/acpi/sleep.c
> > +++ b/drivers/acpi/sleep.c
> > @@ -227,6 +227,7 @@ static int acpi_suspend_begin(suspend_state_t pm_state)
> >  static int acpi_suspend_enter(suspend_state_t pm_state)
> >  {
> >  	acpi_status status = AE_OK;
> > +	acpi_status enable_status = AE_OK;
> >  	unsigned long flags = 0;
> >  	u32 acpi_state = acpi_target_sleep_state;
> >  
> > @@ -254,10 +255,19 @@ static int acpi_suspend_enter(suspend_state_t pm_state)
> >  	}
> >  
> >  	/* If ACPI is not enabled by the BIOS, we need to enable it here. */
> > -	if (set_sci_en_on_resume)
> > +	if (!set_sci_en_on_resume)
> > +		enable_status = acpi_enable();
> > +
> > +	if (set_sci_en_on_resume || enable_status == AE_NO_HARDWARE_RESPONSE)
> > +		/* If we're still in legacy mode then we have a problem. The
> > +		 * spec tells us that this bit is under hardware control, but
> > +		 * there's no plausible way that the OS can transition back to
> > +		 * legacy mode so our choices here are to either ignore the
> > +		 * spec or crash and burn horribly. The latter doesn't seem
> > +		 * like it's ever going to be the preferable choice, so let's
> > +		 * live dangerously.
> > +		 */
> >  		acpi_write_bit_register(ACPI_BITREG_SCI_ENABLE, 1);
> > -	else
> > -		acpi_enable();
> >  
> >  	/* Reprogram control registers and execute _BFS */
> >  	acpi_leave_sleep_state_prep(acpi_state);
> --

Re: [PATCH] acpi: Fall back to manually changing SCI_EN

[Rafael J. Wysocki]

On Thursday 29 April 2010, Len Brown wrote:
> 
> On Wed, 28 Apr 2010, Rafael J. Wysocki wrote:
> 
> > On Monday 19 April 2010, Matthew Garrett wrote:
> > > The ACPI spec tells us that the ACPI SCI_EN bit is under hardware control
> > > and shouldn't be touched by the OS. It seems that the Leading Other OS
> > > ignores this and some machines expect this behaviour. We have a blacklist
> > > for these, but given that we're able to detect the failure case and the
> > > alternative to breaking the spec is letting the machine crash and burn,
> > > let's try falling back when we know the alternative is a mostly-dead
> > > machine.
> > > 
> > > Signed-off-by: Matthew Garrett <mjg@redhat.com>
> > 
> > I guess we can try that, but I'd prefer it if that went into .35.
> > 
> > It _should_ be safe, but ...
> > 
> > Anyway, Acked-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> 
> I'd like to push this patch in .34,
> and in .35 remove the DMI list and the boot option.
> 
> One thing that has bothered me about the boot option
> is that it writes SCI_EN instead of callin acpi_enable()
> as opposed to in-addition-to acpi_enable().
> If all acpi_enable() did was set SCI_EN, then it would
> not make a difference.  However, acpi_enable() writes
> SMI_CMD and we really have no idea what other stuff
> the BIOS may do in SMM on this transition.
> 
> So if we don't end up reverting this one,
> I'd really like to see the boot option gone in .35.

There are some suspend changes in .34 that may cause problems to happen for
people, so I'm not sure if it's actually a good idea to introduce one more of
them after -rc5.

Also removing the boot option can easily wait until .36 IMHO.

Rafael