2.6.18 suspend regression on Intel Macs

2.6.18 suspend regression on Intel Macs

[Frédéric Riss]

Well, I'm not sure it qualifies as a regression, because AFAIK no
official kernels can s2ram/resume Intel Macs correctly out of the box.

There has already been some discussion about the SCI_EN ACPI control bit
not being set when the Mactel boxes come out of suspend to ram. 
http://marc.theaimsgroup.com/?l=linux-acpi&m=114957637501557&w=2
http://marc.theaimsgroup.com/?l=linux-kernel&m=115005083610700&w=2
http://bugme.osdl.org/show_bug.cgi?id=6670

The symptom is:
    irq 9: nobody cared (try booting with the "irqpoll" option)
    Disabling IRQ #9
when the system comes out of sleep, making ACPI non-functional.

Two days after having released 2.6.17, Linus commited a fix for this
issue in his tree (commit 5603509137940f4cbc577281cee62110d4097b1b):

@@ -812,6 +812,9 @@ static int irqrouter_resume(struct sys_d

ACPI_FUNCTION_TRACE("irqrouter_resume");

+ /* Make sure SCI is enabled again (Apple firmware bug?) */
+ acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1, ACPI_MTX_DO_NOT_LOCK);
+

I since then used lightly patched 2.6.17 kernels on my MacMini without a
problem. With the release of 2.6.18, I decided to switch to a vanilla
kernel but I realized that the above issue reappeared. 

I tracked it down to the ACPI merge that took place on July 1st. More
precisely the commit 967440e3be1af06ad4dc7bb18d2e3c16130fe067 (ACPI:
ACPICA 20060623) contains the following hunk:

@@ -635,6 +663,25 @@ acpi_status acpi_hw_register_write(u8 us
 
 	case ACPI_REGISTER_PM1_CONTROL:	/* 16-bit access */
 
+		/*
+		 * Perform a read first to preserve certain bits (per ACPI spec)
+		 *
+		 * Note: This includes SCI_EN, we never want to change this bit
+		 */
+		status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,
+					       ACPI_REGISTER_PM1_CONTROL,
+					       &read_value);
+		if (ACPI_FAILURE(status)) {
+			goto unlock_and_exit;
+		}
+
+		/* Insert the bits to be preserved */
+
+		ACPI_INSERT_BITS(value, ACPI_PM1_CONTROL_PRESERVED_BITS,
+				 read_value);
+
+		/* Now we can write the data */
+
 		status =
 		    acpi_hw_low_level_write(16, value,
 					    &acpi_gbl_FADT->xpm1a_cnt_blk);


which makes Linus' fix a no-op, because it disallows setting the SCI_EN
bit.

Fred.

Re: 2.6.18 suspend regression on Intel Macs

[Pavel Machek]

Hi!

> Well, I'm not sure it qualifies as a regression, because AFAIK no
> official kernels can s2ram/resume Intel Macs correctly out of the box.
...

> There has already been some discussion about the SCI_EN ACPI control bit
> not being set when the Mactel boxes come out of suspend to ram. 
> http://marc.theaimsgroup.com/?l=linux-acpi&m=114957637501557&w=2
> http://marc.theaimsgroup.com/?l=linux-kernel&m=115005083610700&w=2
> http://bugme.osdl.org/show_bug.cgi?id=6670
> 
> The symptom is:
>     irq 9: nobody cared (try booting with the "irqpoll" option)
>     Disabling IRQ #9
> when the system comes out of sleep, making ACPI non-functional.
> 
> Two days after having released 2.6.17, Linus commited a fix for this
> issue in his tree (commit 5603509137940f4cbc577281cee62110d4097b1b):

If fix was in 2.6.18-gitX, yes, that probably counts as a regression.

> I since then used lightly patched 2.6.17 kernels on my MacMini without a
> problem. With the release of 2.6.18, I decided to switch to a vanilla
> kernel but I realized that the above issue reappeared. 
> 
> I tracked it down to the ACPI merge that took place on July 1st. More
> precisely the commit 967440e3be1af06ad4dc7bb18d2e3c16130fe067 (ACPI:
> ACPICA 20060623) contains the following hunk:
> 
> @@ -635,6 +663,25 @@ acpi_status acpi_hw_register_write(u8 us
>  
>  	case ACPI_REGISTER_PM1_CONTROL:	/* 16-bit access */
>  
> +		/*
> +		 * Perform a read first to preserve certain bits (per ACPI spec)
> +		 *
> +		 * Note: This includes SCI_EN, we never want to change this bit
> +		 */
> +		status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,
> +					       ACPI_REGISTER_PM1_CONTROL,
> +					       &read_value);
> +		if (ACPI_FAILURE(status)) {
> +			goto unlock_and_exit;
> +		}
> +
> +		/* Insert the bits to be preserved */
> +
> +		ACPI_INSERT_BITS(value, ACPI_PM1_CONTROL_PRESERVED_BITS,
> +				 read_value);
> +
> +		/* Now we can write the data */
> +
>  		status =
>  		    acpi_hw_low_level_write(16, value,
>  					    &acpi_gbl_FADT->xpm1a_cnt_blk);
> 
> 
> which makes Linus' fix a no-op, because it disallows setting the SCI_EN
> bit.

Okay, just submit a (tested) patch. (And add massive comments why it
is neccessary on mac mini).
									Pavel	

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

Re: 2.6.18 suspend regression on Intel Macs

[Arjan van de Ven]

On Tue, 2006-10-10 at 12:39 +0200, Pavel Machek wrote:
> Hi!
> 
> > Well, I'm not sure it qualifies as a regression, because AFAIK no
> > official kernels can s2ram/resume Intel Macs correctly out of the box.
> ...
> 
> > There has already been some discussion about the SCI_EN ACPI control bit
> > not being set when the Mactel boxes come out of suspend to ram. 
> > http://marc.theaimsgroup.com/?l=linux-acpi&m=114957637501557&w=2
> > http://marc.theaimsgroup.com/?l=linux-kernel&m=115005083610700&w=2
> > http://bugme.osdl.org/show_bug.cgi?id=6670
> > 
> > The symptom is:
> >     irq 9: nobody cared (try booting with the "irqpoll" option)
> >     Disabling IRQ #9
> > when the system comes out of sleep, making ACPI non-functional.
> > 
> > Two days after having released 2.6.17, Linus commited a fix for this
> > issue in his tree (commit 5603509137940f4cbc577281cee62110d4097b1b):
> 
> If fix was in 2.6.18-gitX, yes, that probably counts as a regression

"fix" for some value of the word.
The problem is that this is very much against the spec, and also quite
likely breaks a bunch of machines...

If we do this we probably should at least key this of some DMI
identification for the mac mini..

Re: 2.6.18 suspend regression on Intel Macs

[Pavel Machek]

Hi!

> > > http://marc.theaimsgroup.com/?l=linux-kernel&m=115005083610700&w=2
> > > http://bugme.osdl.org/show_bug.cgi?id=6670
> > > 
> > > The symptom is:
> > >     irq 9: nobody cared (try booting with the "irqpoll" option)
> > >     Disabling IRQ #9
> > > when the system comes out of sleep, making ACPI non-functional.
> > > 
> > > Two days after having released 2.6.17, Linus commited a fix for this
> > > issue in his tree (commit 5603509137940f4cbc577281cee62110d4097b1b):
> > 
> > If fix was in 2.6.18-gitX, yes, that probably counts as a regression
> 
> "fix" for some value of the word.
> The problem is that this is very much against the spec, and also quite
> likely breaks a bunch of machines...
> 
> If we do this we probably should at least key this of some DMI
> identification for the mac mini..

Do we have reports of machines breaking?

But okay, basing it on DMI works for me. (Do those crappy Apple
machines have a DMI?)
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: 2.6.18 suspend regression on Intel Macs

[Linus Torvalds]

On Tue, 10 Oct 2006, Arjan van de Ven wrote:
> 
> "fix" for some value of the word.

The value would be every _meaningful_ value.

> The problem is that this is very much against the spec, and also quite
> likely breaks a bunch of machines...

No, it was not shown to break a single machine, and since the SCI bit was 
supposed to be on, it's a no-op on any non-broken hardware.

> If we do this we probably should at least key this of some DMI
> identification for the mac mini..

No. That would be silly.

Having _conditional_ code is not only bigger, it's orders of magnitude 
more complex and likely to break. It's much better to say: "We know at 
least one machine needs this" than it is to say "We know machine X needs 
this", because the latter has extra complexity that just doesn't buy you 
anything.

It's much better to treat everybody the same, if that works. That way, you 
don't have different code-paths.

		Linus

Re: 2.6.18 suspend regression on Intel Macs

[Frédéric Riss]

Le mardi 10 octobre 2006 à 08:33 -0700, Linus Torvalds a écrit :
> > If we do this we probably should at least key this of some DMI
> > identification for the mac mini..
> 
> No. That would be silly.
> 
> Having _conditional_ code is not only bigger, it's orders of magnitude 
> more complex and likely to break. It's much better to say: "We know at 
> least one machine needs this" than it is to say "We know machine X needs 
> this", because the latter has extra complexity that just doesn't buy you 
> anything.
> 
> It's much better to treat everybody the same, if that works. That way, you 
> don't have different code-paths.

So what's the plan? Should/Will the ACPI guys remove the bit-preserving
change brought in with the latest ACPICA merge?

Fred.

Re: 2.6.18 suspend regression on Intel Macs

[Arjan van de Ven]

On Tue, 2006-10-10 at 21:08 +0200, Frédéric Riss wrote:
> Le mardi 10 octobre 2006 à 08:33 -0700, Linus Torvalds a écrit :
> > > If we do this we probably should at least key this of some DMI
> > > identification for the mac mini..
> > 
> > No. That would be silly.
> > 
> > Having _conditional_ code is not only bigger, it's orders of magnitude 
> > more complex and likely to break. It's much better to say: "We know at 
> > least one machine needs this" than it is to say "We know machine X needs 
> > this", because the latter has extra complexity that just doesn't buy you 
> > anything.
> > 
> > It's much better to treat everybody the same, if that works. That way, you 
> > don't have different code-paths.
> 
> So what's the plan? Should/Will the ACPI guys remove the bit-preserving
> change brought in with the latest ACPICA merge?


it sounds like a good idea to at least put the workaround back for now,
until a more elegant solution (maybe something can be done to make it
not needed anymore) is found...
(or until it shows it breaks other machines at which point
reconsideration is also needed)

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com

Re: 2.6.18 suspend regression on Intel Macs

[Frédéric Riss]

Le mardi 10 octobre 2006 à 21:38 +0200, Arjan van de Ven a écrit :
> > So what's the plan? Should/Will the ACPI guys remove the bit-preserving
> > change brought in with the latest ACPICA merge?
> 
> 
> it sounds like a good idea to at least put the workaround back for now,
> until a more elegant solution (maybe something can be done to make it
> not needed anymore) is found...
> (or until it shows it breaks other machines at which point
> reconsideration is also needed)

The workaround hasn't been removed. It's still there,
drivers/acpi/pci_link.c:
788 
789 /* Make sure SCI is enabled again (Apple firmware bug?) */
790 acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1, ACPI_MTX_DO_NOT_LOCK);
791 

The thing is acpi_set_register doesn't permit anymore to write the SCI
bit since the last ACPI merge. Or maybe you meant that the
acpi_hw_register_write modifications should be reverted until a better
solution is found?

Fred.

Re: 2.6.18 suspend regression on Intel Macs

[Pavel Machek]

Hi!

> > > So what's the plan? Should/Will the ACPI guys remove the bit-preserving
> > > change brought in with the latest ACPICA merge?
> > 
> > 
> > it sounds like a good idea to at least put the workaround back for now,
> > until a more elegant solution (maybe something can be done to make it
> > not needed anymore) is found...
> > (or until it shows it breaks other machines at which point
> > reconsideration is also needed)
> 
> The workaround hasn't been removed. It's still there,
> drivers/acpi/pci_link.c:
> 788 
> 789 /* Make sure SCI is enabled again (Apple firmware bug?) */
> 790 acpi_set_register(ACPI_BITREG_SCI_ENABLE, 1, ACPI_MTX_DO_NOT_LOCK);
> 791 
> 
> The thing is acpi_set_register doesn't permit anymore to write the SCI
> bit since the last ACPI merge. Or maybe you meant that the
> acpi_hw_register_write modifications should be reverted until a better
> solution is found?

Maybe you can just create a patch that modifies ACPI not to mask the
SCI bit? Reverting big chunk of ACPI code is likely not the right
solution.

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

Re: 2.6.18 suspend regression on Intel Macs

[Matthew Garrett]

On Tue, Oct 10, 2006 at 12:41:28PM +0200, Arjan van de Ven wrote:

> "fix" for some value of the word.
> The problem is that this is very much against the spec, and also quite
> likely breaks a bunch of machines...

It works fine under Windows, which suggests that the Windows behaviour 
is to reenable the bit. I wouldn't really expect any existing hardware 
to expect any other sort of behaviour.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: 2.6.18 suspend regression on Intel Macs

[Linus Torvalds]

On Tue, 10 Oct 2006, Pavel Machek wrote:
> 
> Maybe you can just create a patch that modifies ACPI not to mask the
> SCI bit? Reverting big chunk of ACPI code is likely not the right
> solution.

I'm going to apply this after I've confirmed that it fixes the Mac Mini.

It would be nice if somebody documented what the heck that "bit 9" 
actually is that we're trying to preserve. I wonder if that one is any 
better..

It's entirely possible that what we should do in acpi_hw_register_write() 
is to always force SCI_EN to be on, but in the meantime, this would seem 
to be the minimal fix that undoes the damage done by the ACPI merge.

		Linus

---
diff --git a/drivers/acpi/hardware/hwregs.c b/drivers/acpi/hardware/hwregs.c
index 3143f36..fa58c1e 100644
--- a/drivers/acpi/hardware/hwregs.c
+++ b/drivers/acpi/hardware/hwregs.c
@@ -665,8 +665,6 @@ acpi_status acpi_hw_register_write(u8 us
 
 		/*
 		 * Perform a read first to preserve certain bits (per ACPI spec)
-		 *
-		 * Note: This includes SCI_EN, we never want to change this bit
 		 */
 		status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,
 					       ACPI_REGISTER_PM1_CONTROL,
diff --git a/include/acpi/aclocal.h b/include/acpi/aclocal.h
index a4d0e73..063c4b5 100644
--- a/include/acpi/aclocal.h
+++ b/include/acpi/aclocal.h
@@ -708,7 +708,7 @@ struct acpi_bit_register_info {
  * must be preserved.
  */
 #define ACPI_PM1_STATUS_PRESERVED_BITS          0x0800	/* Bit 11 */
-#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0201	/* Bit 9, Bit 0 (SCI_EN) */
+#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0200	/* Bit 9 (whatever) */
 
 /*
  * Register IDs

Re: 2.6.18 suspend regression on Intel Macs

[Frédéric Riss]

Le mardi 10 octobre 2006 à 14:49 -0700, Linus Torvalds a écrit :
> 
> On Tue, 10 Oct 2006, Pavel Machek wrote:
> > 
> > Maybe you can just create a patch that modifies ACPI not to mask the
> > SCI bit? Reverting big chunk of ACPI code is likely not the right
> > solution.
> 
> I'm going to apply this after I've confirmed that it fixes the Mac Mini.

I was about to send a patch doing exactly the same. It fixes the issue
for me. Thanks.

Fred.

Re: 2.6.18 suspend regression on Intel Macs

[Linus Torvalds]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 722 bytes --]



On Wed, 11 Oct 2006, Frédéric Riss wrote:
>
> I was about to send a patch doing exactly the same. It fixes the issue
> for me. Thanks.

Hmm. My Mac Mini doesn't restore properly even with it, but I suspect it's 
the old DRM "resume AGP in the wrong order" problem.

When trying to verify that, though, I noticed that if I enable the "keep 
console active over suspend", then it won't even suspend. It hangs after 
printing "i801_smbus 0000:00:1f.3: suspend".

I'm wondering what Pavel does for debugging these things, since the claim 
was that keeping printk() active would make debugging easier. As it is, it 
just seems to break suspend exactly because it wants to access devices 
that are turned off.

Pavel?

		Linus

Re: 2.6.18 suspend regression on Intel Macs

[Frédéric Riss]

Le mardi 10 octobre 2006 à 16:53 -0700, Linus Torvalds a écrit :
> 
> On Wed, 11 Oct 2006, Frédéric Riss wrote:
> >
> > I was about to send a patch doing exactly the same. It fixes the issue
> > for me. Thanks.
> 
> Hmm. My Mac Mini doesn't restore properly even with it, but I suspect it's 
> the old DRM "resume AGP in the wrong order" problem.

I should have mentioned that I've tested it by patching a 2.6.18 and not
the current git tree (current git wouldn't boot for me, need to
investigate). Maybe something got pulled recently that introduced new
issues?

Fred.

> When trying to verify that, though, I noticed that if I enable the "keep 
> console active over suspend", then it won't even suspend. It hangs after 
> printing "i801_smbus 0000:00:1f.3: suspend".
> 
> I'm wondering what Pavel does for debugging these things, since the claim 
> was that keeping printk() active would make debugging easier. As it is, it 
> just seems to break suspend exactly because it wants to access devices 
> that are turned off.
> 
> Pavel?
> 
> 		Linus

Re: 2.6.18 suspend regression on Intel Macs

[Len Brown]

On Tuesday 10 October 2006 17:49, Linus Torvalds wrote:
> 
> On Tue, 10 Oct 2006, Pavel Machek wrote:
> > 
> > Maybe you can just create a patch that modifies ACPI not to mask the
> > SCI bit? Reverting big chunk of ACPI code is likely not the right
> > solution.
> 
> I'm going to apply this after I've confirmed that it fixes the Mac Mini.

It is troubling that Linux now gets to add the burden of MacOS bug compatibility to
Windows bug compatibility.  I asked the apple folks and they
said they didn't see anyplace in MacOS where SCI_EN is restored
from the OS, so perhaps we are following a different path through
their firmware...

I don't think the risk here isn't that setting SCI_EN is going to break something.
The risk is that excluding it from ACPI_PM1_CONTROL_PRESERVED_BITS
will allow other writes to this register to clear that bit from the OS,
which is clearly counter to what the spec says to do.

But I looked through bugzilla to see if there are any systems whose
SCI isn't working, and I did not find any systems where the old
behaviour of not explicitly preserving SCI_EN breaks the SCI.

> It would be nice if somebody documented what the heck that "bit 9" 
> actually is that we're trying to preserve. I wonder if that one is any 
> better..

All I know is that bit 9 is to be "Ignored", which the spec says to preserve
along with other "Reserved" hardware register bits -- so we should
keep bit 9 in the mask.
 
> It's entirely possible that what we should do in acpi_hw_register_write() 
> is to always force SCI_EN to be on, but in the meantime, this would seem 
> to be the minimal fix that undoes the damage done by the ACPI merge.

Lets stick with the minimum patch until we learn more.

thanks,
-Len

Acked-by: Len Brown <len.brown@intel.com>

> 		Linus
> 
> ---
> diff --git a/drivers/acpi/hardware/hwregs.c b/drivers/acpi/hardware/hwregs.c
> index 3143f36..fa58c1e 100644
> --- a/drivers/acpi/hardware/hwregs.c
> +++ b/drivers/acpi/hardware/hwregs.c
> @@ -665,8 +665,6 @@ acpi_status acpi_hw_register_write(u8 us
>  
>  		/*
>  		 * Perform a read first to preserve certain bits (per ACPI spec)
> -		 *
> -		 * Note: This includes SCI_EN, we never want to change this bit
>  		 */
>  		status = acpi_hw_register_read(ACPI_MTX_DO_NOT_LOCK,
>  					       ACPI_REGISTER_PM1_CONTROL,
> diff --git a/include/acpi/aclocal.h b/include/acpi/aclocal.h
> index a4d0e73..063c4b5 100644
> --- a/include/acpi/aclocal.h
> +++ b/include/acpi/aclocal.h
> @@ -708,7 +708,7 @@ struct acpi_bit_register_info {
>   * must be preserved.
>   */
>  #define ACPI_PM1_STATUS_PRESERVED_BITS          0x0800	/* Bit 11 */
> -#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0201	/* Bit 9, Bit 0 (SCI_EN) */
> +#define ACPI_PM1_CONTROL_PRESERVED_BITS         0x0200	/* Bit 9 (whatever) */
>  
>  /*
>   * Register IDs
> -
> 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/
> 

Re: 2.6.18 suspend regression on Intel Macs

[Len Brown]

On Tuesday 10 October 2006 17:28, Matthew Garrett wrote:
> On Tue, Oct 10, 2006 at 12:41:28PM +0200, Arjan van de Ven wrote:
> 
> > "fix" for some value of the word.
> > The problem is that this is very much against the spec, and also quite
> > likely breaks a bunch of machines...
> 
> It works fine under Windows, which suggests that the Windows behaviour 
> is to reenable the bit. 

To me it suggests that both Windows and MacOS provoke the firmware
to re-enable this bit -- it doesn't suggest that the OS is doing it.

> I wouldn't really expect any existing hardware  
> to expect any other sort of behaviour.

I would.  In the known universe, the Mac-mini is the only machine that
seems to need us to explicitly set SCI_EN.

-Len

suspend debugging Re: 2.6.18 suspend regression on Intel Macs

[Pavel Machek]

Hi!

> I'm wondering what Pavel does for debugging these things, since the claim 
> was that keeping printk() active would make debugging easier. As it is, it 
> just seems to break suspend exactly because it wants to access devices 
> that are turned off.
> 
> Pavel?

I have framebuffer console here, and just leave printk enabled during
system. Last messages usually tell me where to look for problem...
Plus I test suspend quite often so that regressions show up quickly.

In the early days, plain vga console on machine where bios kicks video
card to work after resume did wonders, and I had to use hardware
debugger on one machine, too.

Another common trick is to test s2disk, first. It tends to catch many
driver problems.
							Pavel
-- 
Thanks for all the (sleeping) penguins.

Re: 2.6.18 suspend regression on Intel Macs

[Linus Torvalds]

On Wed, 11 Oct 2006, Len Brown wrote:
> 
> It is troubling that Linux now gets to add the burden of MacOS bug compatibility to
> Windows bug compatibility.  I asked the apple folks and they
> said they didn't see anyplace in MacOS where SCI_EN is restored
> from the OS, so perhaps we are following a different path through
> their firmware...

We definitely are. MacOS uses EFI. Linux (if you want to use a standard 
Linux distro image) uses Boot Camp, aka BIOS emulation.

> I don't think the risk here isn't that setting SCI_EN is going to break something.
> The risk is that excluding it from ACPI_PM1_CONTROL_PRESERVED_BITS
> will allow other writes to this register to clear that bit from the OS,
> which is clearly counter to what the spec says to do.

Is there _ever_ any valid reason to clear it? I wouldn't object at all to 
a patch that just forces it..

That said, I could imagine some strange AML sequence that clears that bit 
in order to do something really nasty (and then sets it again at the end). 
There's no limit to what horrors can lurk in peoples BIOSes.

		Linus