Skip to contentSkip to conversation

ACPI: fix cpufreq regression

PATCH In review Len Brown 2 participants 3 messages Jan 17, 2007 – Jan 24, 2007 ACPI

lore
Share & tools
lore

Len Brown

author reviewer
#0 lore raw

PATCH 1/1 ACPI: fix cpufreq regression

nobody else with a T60 noticed this since November?

---------- Forwarded Message ----------

Subject: [patch] ACPI: fix cpufreq regression
Date: Tuesday 16 January 2007 12:09
From: Ingo Molnar <[email protected]>
To: Andrew Morton <[email protected]>
Cc: [email protected], Dave Jones <[email protected]>, Linus Torvalds <[email protected]>, Len Brown <[email protected]>

Subject: [patch] ACPI: fix cpufreq regression
From: Ingo Molnar <[email protected]>

recently cpufreq support on my laptop (Lenovo T60) broke completely:
when it's plugged into AC it would never go higher than 1 GHz - neither 1.3 GHz nor 1.83 GHz is possible - no matter which governor (userspace, speed or ondemand) is used.

after some cpufreq debugging i tracked the regression back to the following (totally correct) bug-fix commit:

commit 0916bd3ebb7cefdd0f432e8491abe24f4b5a101e
Author: Dave Jones <[email protected]>
Date: Wed Nov 22 20:42:01 2006 -0500

[PATCH] Correct bound checking from the value returned from _PPC method.

this bugfix, which makes other laptops work, made a previously hidden (BIOS) bug visible on my laptop.

The bug is the following: if the _PPC (Performance Present Capabilities) optional ACPI object is queried /after/ bootup then the BIOS reports an incorrect value of '2'.

My laptop (Lenovo T60) has the following performance states supported:

0: 1833000
1: 1333000
2: 1000000

Per ACPI specification, a _PPC value of '0' means that all 3 performance states are usable. A _PPC value of '1' means states 1 .. 2 are usable, a value of '2' means only state '2' (slowest) is usable.

now, the _PPC object is optional, and it also comes with notification. Furthermore, when a CPU object is initialized, the _PPC object is initialized as well. So the following evaluation of the _PPC object is superfluous:

 [<c028ba5f>] acpi_processor_get_platform_limit+0xa1/0xaf
 [<c028c040>] acpi_processor_register_performance+0x3b9/0x3ef
 [<c0111a85>] acpi_cpufreq_cpu_init+0xb7/0x596
 [<c03dab74>] cpufreq_add_dev+0x160/0x4a8
 [<c02bed90>] sysdev_driver_register+0x5a/0xa0
 [<c03d9c4c>] cpufreq_register_driver+0xb4/0x176
 [<c068ac08>] acpi_cpufreq_init+0xe5/0xeb
 [<c010056e>] init+0x14f/0x3dd

and this is the point where my laptop's BIOS returns the incorrect value of '2'. Note that it has not sent any notification event, so the value is probably not really intentional (possibly spurious), and Windows likely doesnt query it after bootup either. Maybe the value is kept at '2' normally, and is only set to the real value when a true asynchronous event (such as AC plug event, battery switch, etc.) occurs.

So i /think/ this is a grey area of the ACPI spec: per the letter of the spec the _PPC value only changes when notified, so there's no reason to query it after the system has booted up. So in my opinion the best (and most compatible) strategy would be to do the change below, and to not evaluate the _PPC object in the acpi_processor_get_performance_info() call, but only evaluate it if _PPC is present during CPU object init, or if it's notified during an asynchronous event. This change is more permissive than the previous logic, so it definitely shouldnt break any existing system.

This also happens to fix my laptop, which is merrily chugging along at 1.83 GHz now. Yay!

---
 drivers/acpi/processor_perflib.c |    4 ----
 1 file changed, 4 deletions(-)
drivers/acpi/processor_perflib.c +0 -4
Index: linux/drivers/acpi/processor_perflib.c
===================================================================
@@ -322,10 +322,6 @@ static int acpi_processor_get_performanc
 	if (result)
 		return result;
 
-	result = acpi_processor_get_platform_limit(pr);
-	if (result)
-		return result;
-
 	return 0;
 }
 

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

-------------------------------------------------------

#1 lore raw
Quoted 1 line · Len Brown

nobody else with a T60 noticed this since November?

It might be a good idea to Cc [email protected] when asking such questions, as it will reach a lot of thinkpad power users that run Linux. I know a number of people subscribe to both linux-acpi and linux-thinkpad (including me), so there is some cross-polination, but still...

The *60 thinkpad owners have had a few bad months lately, as they are either hitting Lenovo bugs, or Lenovo is uncovering kernel bugs. E.g. the ACPI video for 2.6.19 (the one with fubar constants for the ACPI video events) and earlier will "have issues" with the newer thinkpad BIOSes and hang many thinkpads if you try to increase brightness (the buggy version of video.c will instead try to switch output ports, which doesn't work easily on all thinkpads, and hardlock a running X server). This is fixed in 2.6.20-rc.

OTOH, there are reports that the new BIOSes have removed functionality from the DSDT that allowed multiple cores to work in different clock speeds, so people may have overlooked the bug Ingo worked around...

If you'd like, I can try to act as a liason between linux-acpi and linux-thinkpad, and ask your question there.

Signature · 4 lines
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh
author reviewer
#2 lore raw
Quoted 4 lines · you
Quoted 1 line · Len Brown

nobody else with a T60 noticed this since November?

It might be a good idea to Cc [email protected]

okay, added to cc:-)

Quoted 3 lines

OTOH, there are reports that the new BIOSes have removed functionality from the DSDT that allowed multiple cores to work in different clock speeds, so people may have overlooked the bug Ingo worked around...

Intel's dual core products (used in Thinkpads) don't allow the cores to run at different speeds anyway. They use hardware coordination to set the package to the MAX of the requested frequencies.

The reason is that there is a single voltage to the package, so if you have to keep it high to run one core fast -- you might as well run the other core fast also.

Now there is some fine print to this...

There is a bit the BIOS can set to disable HW coordination. But if it does so, then it needs to provide some ACPI info telling the kernel to coordination speeds in software. (ie. the last request from either core will set the P-state of the entire package -- so you want to do a MAX of the requests in software and then do a single request to either core to implement a change).

So if HW coordination is erroneously disabled and the BIOS neglects to tell us, then the entire package P-state will flip around depending on the last request from either core.
One example of a failure would be if core0 requests fast, and then core1 requests slow, then core0 runs slow when
it wanted to run fast.

I'm not aware of thinkpads having this bug -- but IIR a Sossaman server board from Intel actually had it -- so who knows who else may...

cheers,
-Len