Bug 15174 - _PSS sanity check prevents CPU clock scaling -- introduced by 34d531e640cb805973cf656b15c716b961565cea
Summary: _PSS sanity check prevents CPU clock scaling -- introduced by 34d531e640cb805...
Status: REJECTED INVALID
Alias: None
Product: ACPI
Classification: Unclassified
Component: Power-Processor (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Zhang Rui
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-30 06:17 UTC by D. Hugh Redelmeier
Modified: 2010-03-26 03:05 UTC (History)
6 users (show)

See Also:
Kernel Version: 2.6.30
Subsystem:
Regression: Yes
Bisected commit-id:


Attachments
dmesg output from 2.6.18-128.7.1.el5 -- scaling worked (19.75 KB, text/plain)
2010-01-30 06:17 UTC, D. Hugh Redelmeier
Details
dmesg output from 2.6.18-164.el5 -- scaling does not work (19.74 KB, text/plain)
2010-01-30 06:18 UTC, D. Hugh Redelmeier
Details
acpidump of misbehaving machine (64.03 KB, application/octet-stream)
2010-01-30 06:19 UTC, D. Hugh Redelmeier
Details
_PSS disassembled, with added comments (2.25 KB, text/plain)
2010-01-30 06:20 UTC, D. Hugh Redelmeier
Details
patch to ignore bad _PSS entries (1.45 KB, patch)
2010-02-03 16:12 UTC, D. Hugh Redelmeier
Details | Diff
patch to ignore and fill bad _PSS entries [UNTESTED] (1.63 KB, text/plain)
2010-02-08 21:53 UTC, D. Hugh Redelmeier
Details
patch to ignore and fill bad _PSS entries (1.74 KB, patch)
2010-03-15 00:55 UTC, D. Hugh Redelmeier
Details | Diff
patch: ignore the invalid _PSS entries (2.38 KB, patch)
2010-03-22 09:21 UTC, Zhang Rui
Details | Diff

Description D. Hugh Redelmeier 2010-01-30 06:17:30 UTC
Created attachment 24785 [details]
dmesg output from 2.6.18-128.7.1.el5 -- scaling worked

[Originally reported https://bugzilla.redhat.com/show_bug.cgi?id=559357 and https://www.centos.org/modules/newbb/viewtopic.php?viewmode=flat&topic_id=22341&forum=44]

A new sanity check was added to linux/drivers/acpi/processor_perflib.c acpi_processor_get_performance_states.  The commit is 34d531e640cb805973cf656b15c716b961565cea "ACPI: sanity check _PSS frequency to prevent cpufreq crash"

This new check says that if any _PSS table freq entry is impossibly large (i.e. the frequency, expressed in kHz, would not fit in a u32), the whole table will be ignored.

Unfortunately, my machine's BIOS has some such entries.  It also has sane entries.  Before the patch, my system was stable and could do CPU frequency scaling.  After the patch, my system cannot do CPU frequency scaling.

These crazy _PSS values used to be ignored without ignoring the reasonable ones.  The ignoring was done in several routines in cpu/cpufreq/powernow-k8.c, but not acpi_processor_get_performance_states.  The patch must have been made because bad values were not ignored somewhere imporant.

My machine is an HP Pavilion a530n with a stock 3.11 BIOS (the latest on HP's support page; dated September 2004).  The machine has worked well in Linux all the years since then. http://h10025.www1.hp.com/ewfrf/wc/softwareDownloadIndex?softwareitem=pv-22976-2&lc=en&dlc=en&cc=ca&lang=en&os=228&product=404646

My system runs CentOS x86_64 kernels.  The problem appeared in the transition from kernel-2.6.18-128.7.1.el5 to kernel-2.6.18-164.el5.  I have tracked it down to the commit that was accepted in the Mainline 2.6.30.

My hope would be that the sanity checker could be changed to reject bad table entries but preserve any good ones.

Without the patch, dmesg shows:

powernow-k8: Pre-initialization of ACPI failed
powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3200+ processors (1 cpu cores)
(version 2.20.00)
powernow-k8: invalid freq entries 3300000 kHz vs. 2147483048 kHz
powernow-k8: invalid freq entries 3300000 kHz vs. 2147483048 kHz
powernow-k8: 0 : fid 0xc (2000 MHz), vid 0x2
powernow-k8: 1 : fid 0xa (1800 MHz), vid 0x6
powernow-k8: 2 : fid 0x0 (800 MHz), vid 0xa
powernow-k8: ph2 null fid transition 0xc

With the patch:

powernow-k8: Pre-initialization of ACPI failed
powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3200+ processors (1 cpu cores)
(version 2.20.00)
ACPI: Invalid BIOS _PSS frequency: 0x9999999 MHz
powernow-k8: BIOS error: maxvid exceeded with pstate 2
Comment 1 D. Hugh Redelmeier 2010-01-30 06:18:37 UTC
Created attachment 24786 [details]
 dmesg output from 2.6.18-164.el5 -- scaling does not work
Comment 2 D. Hugh Redelmeier 2010-01-30 06:19:22 UTC
Created attachment 24787 [details]
 acpidump of misbehaving machine
Comment 3 D. Hugh Redelmeier 2010-01-30 06:20:20 UTC
Created attachment 24788 [details]
 _PSS disassembled, with added comments
Comment 4 D. Hugh Redelmeier 2010-02-03 16:12:00 UTC
Created attachment 24897 [details]
patch to ignore bad _PSS entries

I'm looking at the code in acpi_processor_get_performance_states().

Is it important that it preserve the absolute position of entries in _PSS when it builds pr->performance->states[], or can it just leave out bad entries?

If it can just leave out entries, the attached patch might do the job.  Note: I have not even compiled it, let alone tested it, so it surely has bugs.

This is a patch relative to 2.6.32 (which I don't even have).
Comment 5 D. Hugh Redelmeier 2010-02-08 21:53:22 UTC
Created attachment 24959 [details]
patch to ignore and fill bad _PSS entries [UNTESTED]

I've started to look at the ACPI spec.

The _PPC method returns a value which is a subscript into the _PSS table.  Since the patch effectively changes subscripts of entries, it isn't going to work.

On my particular machine, _PPC will always return 0, so the patch would actually work.

I wonder what possessed the BIOS writers to put crap in the table?

Instead of deleting bad _PSS entries, the fix should perhaps replace them with harmless ones.  The logic of _PPC suggests that a harmless one would be a copy of the next valid one (the previous valid one could use too much power).  Unfortunately, there may not be a next valid one (that is the case on my machine).

I've added code to my patch so that when a good entry is encountered, it is used to fill slots that bad entries would have used.  The table is left short if bad entries are not followed by good ones.
Comment 6 Zhang Rui 2010-03-12 07:25:56 UTC
So the problem is:
if we should still make use of the valid _PSS elements when invalid _PSS element is found. right?
Comment 7 D. Hugh Redelmeier 2010-03-12 07:33:23 UTC
Zhang Rui: yes, the whole table should not be ignored just because it has some bad entries.

My second patch should be better than my first because it leaves the subscripts of the good entries unchanged.  Furthermore, it replaces each bad entry with the next good entry (if any).

I've not tested the code.  Nor do I have a deep understanding of the _PSS table.  So I hope someone more knowledgeable will check it.
Comment 8 Zhang Rui 2010-03-12 07:46:27 UTC
> -     pr->performance->state_count = pss->package.count;
> +     pr->performance->state_count = 0;
>       pr->performance->states =
>           kmalloc(sizeof(struct acpi_processor_px) * pss->package.count,
>                   GFP_KERNEL);
> @@ -286,7 +286,7 @@
> 
>       for (i = 0; i < pr->performance->state_count; i++) {

shouldn't we change this to
        for (i = 0; i < pss->package.count; i++) {
as pr->performance->state_count is 0 at this time.
 
> -             struct acpi_processor_px *px = &(pr->performance->states[i]);
> +             struct acpi_processor_px *px =
> &(pr->performance->states[pss->package.count]);
...

Okay, I'll refresh your patch and see if we can push it to upstream kernel.
But can you test it for me before sending it out?
Comment 9 D. Hugh Redelmeier 2010-03-12 08:02:30 UTC
Zang Rui: Yes, you are right about that test.  Thanks!

I will try to test the code in the next couple of days.
Comment 10 D. Hugh Redelmeier 2010-03-14 08:05:38 UTC
Testing.  I get a panic at the moment.  I'm still working on it.
Comment 11 D. Hugh Redelmeier 2010-03-15 00:55:11 UTC
Created attachment 25518 [details]
patch to ignore and fill bad _PSS entries

I tested a slight variant of this on my computer with bad _PSS entries.  The reason for testing a variant is that that computer is running CentOS 5 with their modified 2.6.18 kernel.

So: it works on one machine (more than I can say about the previous version).
Comment 12 Zhang Rui 2010-03-22 09:06:08 UTC
the buggy _PSS on this laptop:
                        Name (_PSS, Package (0x03)
                        {
                            Package (0x06)
                            {
                                0x000007D0,
                                ...
                            },

                            Package (0x06)
                            {
                                0x00000708,
                                ...
                            },

                            Package (0x06)
                            {
                                0x00000320,
                                ...
                            },

                            Package (0x06)
                            {
                                0x09999999,
                                0x00099999,
                                0x00999999,
                                0x00999999,
                                0x99999999,
                                0x99999999
                            },

                            Package (0x06)
                            {
                                0x09999999,
                                0x00099999,
                                0x00999999,
                                0x00999999,
                                0x99999999,
                                0x99999999
                            }
                        })
Comment 13 Zhang Rui 2010-03-22 09:21:44 UTC
Created attachment 25638 [details]
patch: ignore the invalid _PSS entries

please try this patch and see if it works for you.
Comment 14 D. Hugh Redelmeier 2010-03-22 15:10:46 UTC
Zhang Rui:

You didn't say what you changed in this patch.

It looks to me as if you changed the code that made sure that
(1) good entries retained their original subscript, and
(2) filled the hole left by a bad entry with the next legitimate entry (if any).

Why did you delete that part of the code?

That code is necessary, in theory, if you read how _PSS entries are used.  I explained this in #5.

That feature happens not to be needed in my computer, nor in the laptop you describe in #12.  This is because all the bad entries are at the end.  If you think that that is the only case that needs to be handled, the correct version of the code would just exit the loop on the first bad entry it finds (and a separate j variable would not be needed).

If there happens to be a bad entry before good entries, your patch will cause the good entries to no longer be at the expected subscript so the result will be wrong.  For example:
1: good1
2: bad
3: good3

becomes (with your version of the patch)
1: good1
2: good3
(3 no longer a valid subscript)

Since the _PPC method could return a value "3", we've turned a working system into a non-working system.

The previous patch would have created:
1: good1
2: good3
3: good3
which would leave _PPC still functioning.
Comment 15 Zhang Rui 2010-03-23 07:17:35 UTC
hah, I see.
Sorry for the mistake.

But I'm still wondering if we should cover the case you described above.
It's reasonable that _PSS may contain several good P-states and end with some invalid entries.
But the situation you described above sounds like a BIOS problem and we should take care of, to avoid some potential risks.

In both cases, we should stop parsing the left _PSS entries once an invalid entries is found, what do you think?
Comment 16 D. Hugh Redelmeier 2010-03-24 07:11:54 UTC
[This response is a bit rambling.  Sorry.]

You say "it is reasonable that _PSS may ... end with some invalid entries".  Why do you think that is reasonable?  It seems like a BIOS bug to me.

I would like Linux to be tolerant of BIOS bugs because we have no control over them.  In particular, BIOS writers seem to stop fixing the code once Windows runs and Windows is apparently tolerant of a lot of BIOS bugs.  I've reported BIOS bugs to manufacturers and have gotten no response.

The safest approach is the status quo: if you see a bad entry, distrust the whole thing.  After all: if an entry is wrong, how can you trust the BIOS writer to have gotten any of the entries right?

I don't like that because it causes problems with my machine.  My machine worked for years and years until the change to check for sanity of entries.

That change was added (as I understand it) because the Intel code could crash the machine when it (later) tried to use a bad entry.  The corresponding AMD code seems to check for sanity before actually using the value so it would not crash.  My machine's CPU is AMD; is yours?

The sanity checking that we're adjusting was added fairly recently so we don't know how many machines are affected like mine.  The code only got to CentOS recently.  You too seem to have an affected machine.  Do you know of others?  Are all the bad entries at the end of the table?

The patch I proposed should be safe and it should maximize the number of broken BIOSes that the kernel will support.  The simpler version which stops processing _PSS entries when the first bad one is encountered is not as general but it would support the only two examples I have of bad BIOSes (i.e. my machine and yours).

On balance, I prefer the patch I proposed.  I personally could live with the "accept entries until bad one encountered" version.
Comment 17 Robert Moore 2010-03-24 15:11:39 UTC
I think what happens in these cases is that the BIOS is dynamically changing the size of the _PSS package on the fly. The key would be this statement:

Name (_PSS, Package (0x03)

indicating that only 3 of the _PSS sub-packages are valid. So, the original ASL code has the worst-case size of _PSS, and the package is truncated by the BIOS as necessary.

The ACPICA code will truncate this _PSS package to three sub-packages and emit an exception. If this is not happening, perhaps there is a bug.

/*
 * NumElements was exhausted, but there are remaining elements in the
 * PackageList. Truncate the package to NumElements.
 *
 * Note: technically, this is an error, from ACPI spec: "It is an error
 * for NumElements to be less than the number of elements in the
 * PackageList". However, we just print a message and
 * no exception is returned. This provides Windows compatibility. Some
 * BIOSs will alter the NumElements on the fly, creating this type
 * of ill-formed package object.
 */

...

 ACPI_INFO((AE_INFO,
    "Actual Package length (0x%X) is larger than NumElements field (0x%X), truncated\n",
Comment 18 D. Hugh Redelmeier 2010-03-24 15:47:20 UTC
If I understand Robert's statement, it would seem that this whole problem could be eliminated by using the correct item count.  That sure would be cleaner and simpler than filtering entries.  Is that correct?

Robert: when you say "dynamic", do you mean that it can change after initialization or just that the BIOS changes it during bring-up?

Example of my ignorance: I didn't realize that the number in the "Package (0x3)" notation was a count.  I don't even know how to access that count in the C code.  (I could do the homework but I assume you guys know this already and could easily fix the code.)
Comment 19 D. Hugh Redelmeier 2010-03-24 15:53:49 UTC
As I mentioned earlier, I found that some Powernow-k8 code did similar sanity checks on the values.  If we can be convinced that all these checks are downstream of the code we are patching, then we could remove those checks.

Unfortunately bug fixing usually complicates code.  It would be nice if we could get some simplification in return.
Comment 20 Robert Moore 2010-03-24 15:59:53 UTC
By dynamic, I do in fact mean that the BIOS changes the size of the package during initialization.

The object returned from acpi_evaluate_object ("_PSS") in this case should be an ACPI_OBJECT of type Package with a count of three. And the ACPI_INFO message in #17 should be emitted. If this is not the behavior you are seeing, then I think something might be wrong in ACPICA.
Comment 21 D. Hugh Redelmeier 2010-03-24 21:04:46 UTC
Summary: it currently looks to me as if this bug is not in the current tree!  Sorry for wasting your time!

Details:

I've tracked down the code from #17 to here: patchwork.kernel.org/patch/45457/mbox/

This is a message dated 2009 Sept 4 from Robert and Lin Ming related to http://www.acpica.org/bugzilla/show_bug.cgi?id=802

My guess is that this patch would prevent my problem occurring.

That patch is not in the code base that I'm testing (CentOS 2.6.18-164.11.1.el5 x86_64).

The code in the CentOS kernel seems to make the package length the maximum of the specified length and the length of the initializer list.  From 
linux-2.6.18.x86_64/drivers/acpi/dispatcher/dsobject.c:

        /*
         * The package length (number of elements) will be the greater
         * of the specified length and the length of the initializer list
         */
        if (package_list_length > package_length) {
                obj_desc->package.count = package_list_length;
        }


Since Red Hat backported 34d531e640cb805973cf656b15c716b961565cea, causing my problem, I guess that they need to backport 7f646eda8ad9c53589a0379f061cb0448f5fa6db to cure it.  (UNTESTED)

Does this make sense?

(I don't even know if CentOS can run on a current kernel.org kernel so I don't know how to easily test a current kernel.)
Comment 22 Robert Moore 2010-03-24 21:10:49 UTC
I guess I should ask what version of ACPICA you are using. This issue was resolved nearly four years ago:

21 July 2006. Summary of changes for version 20060721:

1) ACPI CA Core Subsystem:

Implemented support within the AML interpreter for package objects that 
contain a larger AML length (package list length) than the package element 
count. In this case, the length of the package is truncated to match the 
package element count. Some BIOS code apparently modifies the package length 
on the fly, and this change supports this behavior. Provides compatibility 
with the MS AML interpreter. (With assistance from Fiodor Suietov)
Comment 23 D. Hugh Redelmeier 2010-03-25 00:14:05 UTC
I don't really know how to tell which version of ACPI CA is included in the CentOS 5.4 kernel (i.e. the RHEL 5.4 kernel).  Any suggestions?

The kernel.org version that it is based on is 2.6.18.  According to http://www.kernel.org/pub/linux/kernel/v2.6/ the ChangeLog-2.6.18 file was created 2006 Sept 20.

RHEL has backported a lot of stuff.  So the ACPI CA might be newer than that in 2.6.18.
Comment 24 Zhang Rui 2010-03-25 02:10:05 UTC
(In reply to comment #23)
> I don't really know how to tell which version of ACPI CA is included in the
> CentOS 5.4 kernel (i.e. the RHEL 5.4 kernel).  Any suggestions?
> 
check ACPI_CA_VERSION in include/acpi/acpixf.h
Comment 25 Robert Moore 2010-03-25 02:14:04 UTC
In older versions, ACPI_CA_VERSION is in include/acpi/acconfig.h

in either case, it looks like this:

/* Current ACPICA subsystem version in YYYYMMDD format */ 
#define ACPI_CA_VERSION                 0x20080729
Comment 26 D. Hugh Redelmeier 2010-03-25 03:29:02 UTC
dmesg shows this:
  ACPI: Core revision 20060707

In include/acpi/acconfig.h:
  #define ACPI_CA_VERSION                 0x20060707

So this version seems to be two weeks too old!

Is there a simple existing patch that would safely fix this?  One that I could recommend to Re Hat.
Comment 27 Robert Moore 2010-03-25 04:16:26 UTC
Well, this is a really old version of ACPICA, we've made literally hundreds of changes since then.

But the commit to our git tree is
ffd0eca830ee3f762e387fe5519fe34fc44b0231

at http://git.moblin.org/cgit.cgi/acpica/
Comment 28 Zhang Rui 2010-03-26 03:05:37 UTC
close this bug report as it is not an upstream kernel bug.

Note You need to log in before you can comment on or make changes to this bug.