[git pull request] ACPI patches for 2.6.34-rc6

[git pull request] ACPI patches for 2.6.34-rc6

[Len Brown]

Hi Linus,

please pull from: 

git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux-acpi-2.6.git release

The big DMI list in sleep.c a temporary brute force and low-risk workaround.
We will endeavor to delete it entirely in 2.6.35 with a clean fix
that was deemed too risky for this late in the .34 release cycle.

This will update the files shown below.

thanks!

Len Brown
Intel Open Source Technology Center


ps. individual patches are available on linux-acpi@vger.kernel.org
and a consolidated plain patch is available here:
http://ftp.kernel.org/pub/linux/kernel/people/lenb/acpi/patches/2.6.34/acpi-release-20100121-2.6.34-rc6.diff.gz

 drivers/acpi/acpi_pad.c        |    2 +-
 drivers/acpi/bus.c             |    2 +-
 drivers/acpi/hest.c            |    4 +
 drivers/acpi/power_meter.c     |    2 +-
 drivers/acpi/sbshc.c           |    2 +-
 drivers/acpi/sleep.c           |  144 ++++++++++++++++++++++++++++++++++++++++
 drivers/pnp/pnpacpi/rsparser.c |   26 +------
 drivers/pnp/resource.c         |    4 +
 8 files changed, 160 insertions(+), 26 deletions(-)

through these commits:

Alex Chiang (1):
      ACPI: DMI init_set_sci_en_on_resume for multiple Lenovo ThinkPads

Bjorn Helgaas (2):
      PNPACPI: compute Address Space length rather than using _LEN
      PNP: don't check for conflicts with bridge windows

Dan Carpenter (4):
      ACPI: silence kmemcheck false positive
      acpi_pad: "processor_aggregator" name too long
      power_meter: acpi_device_class "power_meter_resource" too long
      sbshc: acpi_device_class "smbus_host_controller" too long

Kamal Mostafa (1):
      ACPI: sleep: init_set_sci_en_on_resume for Dell Studio 155x

Shaohua Li (1):
      ACPI: fix acpi_hest_firmware_first_pci() caused oops

with this log:

commit 1468cf0542663f873410b83d8bb61ae779e3a845
Merge: f238b41 5cc4a0f ea5bc73 11439a6 4bdae98
Author: Len Brown <len.brown@intel.com>
Date:   Thu May 6 22:04:31 2010 -0400

    Merge branches 'bugzilla-14337', 'bugzilla-14998', 'bugzilla-15407', 'bugzilla-15903' and 'misc-2.6.34' into release

commit ea5bc73f4f56449b2d450068d492bcd17a675d7a
Author: Kamal Mostafa <kamal@canonical.com>
Date:   Tue Apr 27 14:02:40 2010 -0700

    ACPI: sleep: init_set_sci_en_on_resume for Dell Studio 155x
    
    Add Dell Studio models (1558, 1557, 1555) to the 'set_sci_en_on_resume'
    list to fix hang on resume.
    
    BugLink: http://bugs.launchpad.net/bugs/553498
    
    Signed-off-by: Kamal Mostafa <kamal@canonical.com>
    Acked-by: Alex Chiang <achiang@canonical.com>
    Cc: stable@kernel.org
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 4bdae98f1a456ea1fea5ea02f9249d23bceab75b
Author: Shaohua Li <shaohua.li@intel.com>
Date:   Thu Apr 8 11:16:15 2010 +0800

    ACPI: fix acpi_hest_firmware_first_pci() caused oops
    
    acpi_hest_firmware_first_pci() could be called when acpi is disabled
    and cause system oops.
    
    Signed-off-by: Shaohua Li <shaohua.li@intel.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 97c227cb51ddcf52c66f7a7fba69237026418b56
Author: Dan Carpenter <error27@gmail.com>
Date:   Tue Apr 27 14:06:05 2010 -0700

    sbshc: acpi_device_class "smbus_host_controller" too long
    
    acpi_device_class can only be 19 characters and a NULL terminator.
    
    With the current name we get a buffer overflow in acpi_smbus_hc_add()
    when we do:
    	strcpy(acpi_device_class(device), ACPI_SMB_HC_CLASS);
    
    Signed-off-by: Dan Carpenter <error27@gmail.com>
    Cc: Alexey Starikovskiy <astarikovskiy@suse.de>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 18262714ca0fb65c290b8ea1807b2b02bb52d0e3
Author: Dan Carpenter <error27@gmail.com>
Date:   Tue Apr 27 14:01:07 2010 -0700

    power_meter: acpi_device_class "power_meter_resource" too long
    
    acpi_device_class can only be 19 characters and a NULL terminator.
    
    The current code has a buffer overflow in acpi_power_meter_add():
           strcpy(acpi_device_class(device), ACPI_POWER_METER_CLASS);
    
    Signed-off-by: Dan Carpenter <error27@gmail.com>
    Cc: Len Brown <lenb@kernel.org>
    Cc: "Darrick J. Wong" <djwong@us.ibm.com>
    Cc: <stable@kernel.org>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit a40770a9537c72b555667851845e73484b22ba17
Author: Dan Carpenter <error27@gmail.com>
Date:   Tue Apr 27 14:06:05 2010 -0700

    acpi_pad: "processor_aggregator" name too long
    
    cpi_device_class can only be 19 characters and a NULL terminator.
    
    With the current name we get a buffer overflow in acpi_pad_add()
    	strcpy(acpi_device_class(device), ACPI_PROCESSOR_AGGREGATOR_CLASS);
    
    [akpm@linux-foundation.org: call it acpi_pad, per Shaohua Li]
    Signed-off-by: Dan Carpenter <error27@gmail.com>
    Cc: walter harms <wharms@bfs.de>
    Acked-by: Shaohua Li <shaohua.li@intel.com>
    Cc: Len Brown <lenb@kernel.org>
    Acked-by: Pavel Machek <pavel@ucw.cz>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 11439a6fd90b4861df64b4f983726e1c54977ab7
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Mon May 3 10:47:21 2010 -0600

    PNP: don't check for conflicts with bridge windows
    
    With fa35b4926, I broke a lot of PNP resource assignment.  That commit made
    PNPACPI include bridge windows as PNP resources, and PNP resource assignment
    treats any enabled overlapping PNP resources as conflicts.  Since PCI host
    bridge windows typically include most of the I/O port space, this makes PNP
    port assigments fail.
    
    The PCI host bridge driver will eventually use those PNP window resources,
    so we should make PNP ignore them when checking for conflicts.
    
    This fixes https://bugzilla.kernel.org/show_bug.cgi?id=15903
    
    Reported-and-tested-by: Pavel Kysilka <goldenfish@linuxsoft.cz>
    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 07bedca29b0973f36a6b6db36936deed367164ed
Author: Alex Chiang <achiang@canonical.com>
Date:   Tue Apr 20 08:03:14 2010 -0600

    ACPI: DMI init_set_sci_en_on_resume for multiple Lenovo ThinkPads
    
    Multiple Lenovo ThinkPad models with Intel Core i5/i7 CPUs can
    successfully suspend/resume once, and then hang on the second s/r
    cycle.
    
    We got confirmation that this was due to a BIOS defect. The BIOS
    did not properly set SCI_EN coming out of S3. The BIOS guys
    hinted that The Other Leading OS ignores the fact that hardware
    owns the bit and sets it manually.
    
    In any case, an existing DMI table exists for machines where this
    defect is a known problem. Lenovo promise to fix their BIOS, but
    for folks who either won't or can't upgrade their BIOS, allow
    Linux to workaround the issue.
    
    https://bugzilla.kernel.org/show_bug.cgi?id=15407
    https://bugs.launchpad.net/ubuntu/+source/linux/+bug/532374
    
    Confirmed by numerous testers in the launchpad bug that using
    acpi_sleep=sci_force_enable fixes the issue. We add the machines
    to acpisleep_dmi_table[] to automatically enable this workaround.
    
    Cc: stable@kernel.org
    Cc: Colin King <colin.king@canonical.com>
    Signed-off-by: Alex Chiang <achiang@canonical.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit f238b414a74a13c3d62e31a08e81b585d750df74
Author: Bjorn Helgaas <bjorn.helgaas@hp.com>
Date:   Tue Apr 27 14:45:38 2010 -0600

    PNPACPI: compute Address Space length rather than using _LEN
    
    ACPI _CRS Address Space Descriptors have _MIN, _MAX, and _LEN.  Linux has
    been computing Address Spaces as [_MIN to _MIN + _LEN - 1].  Based on the
    tests in the bug reports below, Windows apparently uses [_MIN to _MAX].
    
    Per spec (ACPI 4.0, Table 6-40), for _CRS fixed-size, fixed location
    descriptors, "_LEN must be (_MAX - _MIN + 1)", and when that's true, it
    doesn't matter which way we compute the end.  But of course, there are
    BIOSes that don't follow this rule, and we're better off if Linux handles
    those exceptions the same way as Windows.
    
    This patch makes Linux use [_MIN to _MAX], as Windows seems to do.  This
    effectively reverts 3162b6f0c5e and replaces it with simpler code.
    
        https://bugzilla.kernel.org/show_bug.cgi?id=14337 (round)
        https://bugzilla.kernel.org/show_bug.cgi?id=15480 (truncate)
    
    Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

commit 5cc4a0f6b72878ea4e96fdb392d5d24c892a988e
Author: Dan Carpenter <error27@gmail.com>
Date:   Tue Apr 27 00:23:37 2010 +0200

    ACPI: silence kmemcheck false positive
    
    This addresses: https://bugzilla.kernel.org/show_bug.cgi?id=14998
    
    We copy some strings into "event" but we leave the space after the NULL
    terminators uninitialized.  Later in acpi_bus_receive_event() we copy
    the whole struct to another buffer with memcpy().  If the new buffer is
    stored on the stack, kmemcheck prints a warning about the unitialized
    space after the NULL terminators.
    
    It's true that the space is uninitialized, but it's harmless.  The
    buffer is only used in acpi_system_read_event() and we don't read past
    the NULL terminators.
    
    This patch changes the kmalloc() to kzalloc() so that we initialize the
    memory and silence the kmemcheck warning.
    
    Reported-by: Christian Casteyde <casteyde.christian@free.fr>
    Signed-off-by: Dan Carpenter <error27@gmail.com>
    Signed-off-by: Len Brown <len.brown@intel.com>

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Thu, 6 May 2010, Len Brown wrote:
> 
> The big DMI list in sleep.c a temporary brute force and low-risk workaround.
> We will endeavor to delete it entirely in 2.6.35 with a clean fix
> that was deemed too risky for this late in the .34 release cycle.

Btw, why don't we just force the SCI_EN by hand?

We did that for the Apple Mac Mini bug too. It was always the right thing 
to do.

There is _no_ reason not to force it. If the BIOS set it, it's a no-op. If 
the BIOS didn't set it, it's a bug that _must_ be fixed.

I really don't see the reason for that DMI list. What could _possibly_ go 
wrong for just setting the damn bit that we _know_ has to be set?

How could setting SCI_EN _ever_ be a bug? Seriously? Why are we doing this 
conditionally, especially considering that even the commit message here 
explicitly states that Windows does it unconditionally?

And no, I'm not interested in "that toilet paper spec says you can't touch 
it". I care about _reality_.

				Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Thu, May 06, 2010 at 07:34:10PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 6 May 2010, Len Brown wrote:
> > 
> > The big DMI list in sleep.c a temporary brute force and low-risk workaround.
> > We will endeavor to delete it entirely in 2.6.35 with a clean fix
> > that was deemed too risky for this late in the .34 release cycle.
> 
> Btw, why don't we just force the SCI_EN by hand?

The spec says we can't. I've posted a patch to do so if it's still not 
set after we've tried doing it the right way, but I'm not keen on 
pushing it into a release at this point.

> There is _no_ reason not to force it. If the BIOS set it, it's a no-op. If 
> the BIOS didn't set it, it's a bug that _must_ be fixed.

Kind of. The "correct" way to do it is to write to an ioport, and doing 
that usually triggers an SMI. So writing the bit by hand without 
ensuring that we do that is a difference in behaviour, while we've 
simultaneously seen some evidence triggering that SMI may break some 
machines - it's conceivable that Windows does it on boot and writes it 
by hand on resume and so every machine in the real world would be fine 
with us doing it by hand, but we haven't tested that yet and it doesn't 
seem like a great risk to take at this point in the development cycle. 
But yes, that DMI list will go in .35. We'll figure out who breaks and 
why at that point.

> How could setting SCI_EN _ever_ be a bug? Seriously? Why are we doing this 
> conditionally, especially considering that even the commit message here 
> explicitly states that Windows does it unconditionally?

If the southbridge comes up with GPEs enabled in a way that expects one 
thing and we write the bit differently without SMM being involved in the 
process then there's potential misery. I suspect that any such misery is 
entirely due to BIOS vendors being insane, but unfortunately scientific 
testing has demonstrated that BIOS vendors are insane with a P of < 
0.001 and an N of MAX_INT.

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

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Fri, 7 May 2010, Matthew Garrett wrote:
> 
> The spec says we can't. I've posted a patch to do so if it's still not 
> set after we've tried doing it the right way, but I'm not keen on 
> pushing it into a release at this point.

I'm ok with the "release at this point".

But the "spec says we can't" is utter GARBAGE. We _know_ the ACPI spec is 
broken, and almost no BIOSes really follow it. So "spec says" is not a 
game we play. 

The ACPI spec is toilet paper compared to "real world". And anybody who 
looks at the current DMI tables for "oh, do this" realizes that this is 
_not_ an uncommon thing, and should damn well realize that this means that 
clearly Windows doesn't honor the spec _either_.

At that point, the spec isn't just toilet paper, it's toilet paper that MS 
has wiped their butt on. So f*ck "spec says". 

> > There is _no_ reason not to force it. If the BIOS set it, it's a no-op. If 
> > the BIOS didn't set it, it's a bug that _must_ be fixed.
> 
> Kind of. The "correct" way to do it is to write to an ioport, and doing 
> that usually triggers an SMI.

The "correct" way to do it is to basically do what works, and that in turn 
generally means "do what Winddows does, because that's the only thing 
that ever got tested".

I'm perfectly happy with "try to do it the right way, and check the end 
result: and if SCI_EN still isn't set, do it by hand".

So I don't think we need to do the write _unconditionally_, but it sure as 
hell shouldn't be conditional on some DMI table. Because all the DMI table 
tells is is that we do it _wrong_ right now, since clearly Windows doesn't 
have this issue.

		Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Fri, May 07, 2010 at 02:06:53PM -0700, Linus Torvalds wrote:
> On Fri, 7 May 2010, Matthew Garrett wrote:
> > 
> > The spec says we can't. I've posted a patch to do so if it's still not 
> > set after we've tried doing it the right way, but I'm not keen on 
> > pushing it into a release at this point.
> 
> I'm ok with the "release at this point".

http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commit;h=3b12303e6bda6e05579d899fb71cb3e9d3bc26ba

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

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Fri, 7 May 2010, Matthew Garrett wrote:

> On Fri, May 07, 2010 at 02:06:53PM -0700, Linus Torvalds wrote:
> > On Fri, 7 May 2010, Matthew Garrett wrote:
> > > 
> > > The spec says we can't. I've posted a patch to do so if it's still not 
> > > set after we've tried doing it the right way, but I'm not keen on 
> > > pushing it into a release at this point.
> > 
> > I'm ok with the "release at this point".
> 
> http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commit;h=3b12303e6bda6e05579d899fb71cb3e9d3bc26ba

Well, is there any reason to have 'set_sci_en_on_resume' at all then?

I mean, the only reason for that config thing in the first place is that 
we didn't do this from the beginning. No?

And I'd hate to then carry the DMI table along forever just to set a 
variable that isn't worth setting any more..

IOW, I don't disagree with the patch. I just don't think it went far 
enough.

		Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Fri, May 07, 2010 at 02:21:08PM -0700, Linus Torvalds wrote:
> On Fri, 7 May 2010, Matthew Garrett wrote:
> 
> > On Fri, May 07, 2010 at 02:06:53PM -0700, Linus Torvalds wrote:
> > > On Fri, 7 May 2010, Matthew Garrett wrote:
> > > > 
> > > > The spec says we can't. I've posted a patch to do so if it's still not 
> > > > set after we've tried doing it the right way, but I'm not keen on 
> > > > pushing it into a release at this point.
> > > 
> > > I'm ok with the "release at this point".
> > 
> > http://git.kernel.org/?p=linux/kernel/git/lenb/linux-acpi-2.6.git;a=commit;h=3b12303e6bda6e05579d899fb71cb3e9d3bc26ba
> 
> Well, is there any reason to have 'set_sci_en_on_resume' at all then?

I'd hope not, but it depends on what Windows does on resume. If it 
doesn't do the SMM call and just does the register write instead, then 
it may be that some machines are on that list because the SMM call 
breaks them rather than because they need the register to be set by 
hand. I'm planning on instrumenting it to check, but haven't had time to 
do so yet.

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

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Rafael J. Wysocki]

On Friday 07 May 2010, Linus Torvalds wrote:
> 
> On Thu, 6 May 2010, Len Brown wrote:
> > 
> > The big DMI list in sleep.c a temporary brute force and low-risk workaround.
> > We will endeavor to delete it entirely in 2.6.35 with a clean fix
> > that was deemed too risky for this late in the .34 release cycle.
> 
> Btw, why don't we just force the SCI_EN by hand?

Because there are boxes that are broken right away by this.

I have one on my desk in front of me. :-)

Rafael

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Sat, 8 May 2010, Rafael J. Wysocki wrote:

> On Friday 07 May 2010, Linus Torvalds wrote:
> > 
> > Btw, why don't we just force the SCI_EN by hand?
> 
> Because there are boxes that are broken right away by this.
> 
> I have one on my desk in front of me. :-)

Also if we check whether it was already set before?

The thing is, if that bit is clear (_after_ we've done the regular ACPI 
setup), the box can't work correctly as far as I know.

			Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Rafael J. Wysocki]

On Saturday 08 May 2010, Linus Torvalds wrote:
> 
> On Sat, 8 May 2010, Rafael J. Wysocki wrote:
> 
> > On Friday 07 May 2010, Linus Torvalds wrote:
> > > 
> > > Btw, why don't we just force the SCI_EN by hand?
> > 
> > Because there are boxes that are broken right away by this.
> > 
> > I have one on my desk in front of me. :-)
> 
> Also if we check whether it was already set before?

Yes.

> The thing is, if that bit is clear (_after_ we've done the regular ACPI 
> setup), the box can't work correctly as far as I know.

That's correct.  But on that particular box acpi_enable() is necessary to
make things work.  Setting SCI_EN by hand makes it hang solid.

Rafael

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Sat, 8 May 2010, Rafael J. Wysocki wrote:

> On Saturday 08 May 2010, Linus Torvalds wrote:
> > 
> > On Sat, 8 May 2010, Rafael J. Wysocki wrote:
> > 
> > > On Friday 07 May 2010, Linus Torvalds wrote:
> > > > 
> > > > Btw, why don't we just force the SCI_EN by hand?
> > > 
> > > Because there are boxes that are broken right away by this.
> > > 
> > > I have one on my desk in front of me. :-)
> > 
> > Also if we check whether it was already set before?
> 
> Yes.

I don't think you understood the question.

If we check whether it was already set before, we wouldn't _do_ it by 
hand.

> > The thing is, if that bit is clear (_after_ we've done the regular ACPI 
> > setup), the box can't work correctly as far as I know.
> 
> That's correct.  But on that particular box acpi_enable() is necessary to
> make things work.  Setting SCI_EN by hand makes it hang solid.

Let me explain what I think we should do with actual code:

	acpi_enable();
	if (test_if_that_f_cking_bit_still_isnt_set)
		set_it_by_hand();

wouldn't that work on your box? The acpi_enable() seems to work for you, 
so it wouldn't actually ever set it by hand. 

But the problem is that on a number of boxes, acpi_enable() apparently 
doesn't do what it should do. Because the stupid BIOS "knows" it is 
already in ACPI mode, but it forgot to actually set the bit!

			Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Rafael J. Wysocki]

On Saturday 08 May 2010, Linus Torvalds wrote:
> 
> On Sat, 8 May 2010, Rafael J. Wysocki wrote:
...
> 
> Let me explain what I think we should do with actual code:
> 
> 	acpi_enable();
> 	if (test_if_that_f_cking_bit_still_isnt_set)
> 		set_it_by_hand();
> 
> wouldn't that work on your box?

Yes, it would.

> The acpi_enable() seems to work for you, so it wouldn't actually ever set it
> by hand. 
>
> But the problem is that on a number of boxes, acpi_enable() apparently 
> doesn't do what it should do. Because the stupid BIOS "knows" it is 
> already in ACPI mode, but it forgot to actually set the bit!

To calrify, I think the approach in the Matthew's patch is correct, but since I have
some bad experience with that particular thing, I prefer to make that change in
2.6.35 and then move on to drop the flag entirely.

Rafael

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Sat, 8 May 2010, Rafael J. Wysocki wrote:
> 
> To calrify, I think the approach in the Matthew's patch is correct, but since I have
> some bad experience with that particular thing, I prefer to make that change in
> 2.6.35 and then move on to drop the flag entirely.

Oh,yes. I'm not suggesting we do it late in the -rc cycle. 

I just don't think the DMI table is a good idea, so in the longer run I 
wan tto get rid of it, rather than have it grow (by quite a few entries, 
in this case).

There are valid reasons for DMI tables in many cases, but in this case I 
think the reason is simply that we do the wrong thing, so then we ended up 
with a DMI table to "fix" the wrong thing we do.

			Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Len Brown]

> On Sat, 8 May 2010, Rafael J. Wysocki wrote:
> > 
> > To calrify, I think the approach in the Matthew's patch is correct, but since I have
> > some bad experience with that particular thing, I prefer to make that change in
> > 2.6.35 and then move on to drop the flag entirely.
> 
> Oh,yes. I'm not suggesting we do it late in the -rc cycle. 
> 
> I just don't think the DMI table is a good idea, so in the longer run I 
> wan tto get rid of it, rather than have it grow (by quite a few entries, 
> in this case).
> 
> There are valid reasons for DMI tables in many cases, but in this case I 
> think the reason is simply that we do the wrong thing, so then we ended up 
> with a DMI table to "fix" the wrong thing we do.

Hi Linus,
Matthew, Rafael and I all agree with you on every aspect of this issue.

The DMI list is temporary.  Matthew's patch to do as you say
is already queued for 2.6.35.

I belive that Rafael was prudent in recommending we delay
the "real" fix until .35, as we discovered that the broken
machines suffered a 3-second delay on resume, polling for the SCI_EN
that the BIOS would never set, and so the proposed fix for that 
is queued for 2.6.35 as well.

thanks,
-Len

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Fri, May 07, 2010 at 10:36:00PM +0100, Matthew Garrett wrote:
> On Fri, May 07, 2010 at 02:21:08PM -0700, Linus Torvalds wrote:
> > Well, is there any reason to have 'set_sci_en_on_resume' at all then?
> 
> I'd hope not, but it depends on what Windows does on resume. If it 
> doesn't do the SMM call and just does the register write instead, then 
> it may be that some machines are on that list because the SMM call 
> breaks them rather than because they need the register to be set by 
> hand. I'm planning on instrumenting it to check, but haven't had time to 
> do so yet.

I've now checked the behaviour of Windows. It turns out that it never 
makes the ACPI enable SMM call on resume. This is consistent with 
http://bugzilla.kernel.org/show_bug.cgi?id=13745 which shows a bug 
being introduced by us making the enable call in the first place. 
Merging my patch and removing the blacklist would re-break these 
machines. Instead, we should just unconditionally set SCI_EN since this 
is the tested configuration. I'll send a followup patch.

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

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Tue, 11 May 2010, Matthew Garrett wrote:
> 
> I've now checked the behaviour of Windows. It turns out that it never 
> makes the ACPI enable SMM call on resume. This is consistent with 
> http://bugzilla.kernel.org/show_bug.cgi?id=13745 which shows a bug 
> being introduced by us making the enable call in the first place. 
> Merging my patch and removing the blacklist would re-break these 
> machines. Instead, we should just unconditionally set SCI_EN since this 
> is the tested configuration. I'll send a followup patch.

Hmm. But that's the thing that Rafael claims doesn't work on his machine. 

Maybe windows does something else? Or do you _see_ windows doing that 
write?

Note that our acpi_enable() won't do anything either if the machine comes 
up in ACPI mode, so maybe you checked the behavior on that kind of 
machine, and Windows does the same? IOW, if acpi_hw_get_mode() already 
returns ACPI_SYS_MODE_ACPI, the whole thing is a no-op.

Finally, it's possible that we really should just write the dang SCI_EN 
bit directly, but that what we do wrong is that doing so with

	acpi_write_bit_register()

will write it _even_if_ the bit was already set. That could explain 
Rafael's problems too - writing the register directly may be the 
RightThing(tm), but writing it if the bit was already set may well cause 
some confusion.

				Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Tue, May 11, 2010 at 10:42:52AM -0700, Linus Torvalds wrote:

> Maybe windows does something else? Or do you _see_ windows doing that 
> write?

I see Windows write the SCI_EN bit without reading it first and without 
calling the ACPI enable SMM function.

> will write it _even_if_ the bit was already set. That could explain 
> Rafael's problems too - writing the register directly may be the 
> RightThing(tm), but writing it if the bit was already set may well cause 
> some confusion.

Like I said, I don't see any reads on resume - only on system boot.

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

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Linus Torvalds]

On Tue, 11 May 2010, Matthew Garrett wrote:
> 
> I see Windows write the SCI_EN bit without reading it first and without 
> calling the ACPI enable SMM function.

Hmm. So presumably Windows doesn't even do them bit-by-bit, but simply 
does a "restore PM1 Control register value" on resume.

And I guess it probably does the same for the PM1_ENABLE register and the 
PM2 control registers too?

Just restoring the whole register values _does_ sound a lot simpler than 
worrying about individual bits.

		Linus

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Tue, May 11, 2010 at 11:17:34AM -0700, Linus Torvalds wrote:
> 
> 
> On Tue, 11 May 2010, Matthew Garrett wrote:
> > 
> > I see Windows write the SCI_EN bit without reading it first and without 
> > calling the ACPI enable SMM function.
> 
> Hmm. So presumably Windows doesn't even do them bit-by-bit, but simply 
> does a "restore PM1 Control register value" on resume.

Yup.

> And I guess it probably does the same for the PM1_ENABLE register and the 
> PM2 control registers too?

Haven't checked those, but could do so easily enough.

(I say easily enough. Turns out that getting Windows to do S3 under qemu 
is a fair amount of work)
-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Robert Wörle]

> I've now checked the behaviour of Windows. It turns out that it never 
> makes the ACPI enable SMM call on resume. This is consistent with 
> http://bugzilla.kernel.org/show_bug.cgi?id=13745 which shows a bug 
> being introduced by us making the enable call in the first place. 
> Merging my patch and removing the blacklist would re-break these 
> machines. Instead, we should just unconditionally set SCI_EN since this 
> is the tested configuration. I'll send a followup patch.
>
>   
Dear Matthew

Could you tell me how  you "check" windows and its behaviour ?
I am stuck on S3 resume problem with some unit and i would love to get 
any information ( even from windows, which does S3 fine) to help fix this

Re: [git pull request] ACPI patches for 2.6.34-rc6

[Matthew Garrett]

On Wed, May 12, 2010 at 06:07:26PM +0200, Robert Wörle wrote:

> Could you tell me how  you "check" windows and its behaviour ?
> I am stuck on S3 resume problem with some unit and i would love to get  
> any information ( even from windows, which does S3 fine) to help fix this

Running Windows under qemu. I'm afraid it's not practical to do the same 
with real hardware.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
--
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