ACPI processor: force throttling state when BIOS returns incorrect value
Subsystems (3)
PATCH 1/2 ACPI processor: force throttling state when BIOS returns incorrect value
If the BIOS reports an invalid throttling state (which seems to be fairly common after system boot), a reset is done to state T0. Because of a check in acpi_processor_get_throttling_ptc(), the reset never actually gets executed, which results in the error reoccurring on every access of for example /proc/acpi/processor/CPU0/throttling.
Add a 'force' option to acpi_processor_set_throttling() to ensure the reset really takes effect.
http://bugzilla.kernel.org/show_bug.cgi?id=13389
---
This patch, together with the next one, fixes a regression introduced in 2.6.30, listed on the regression list. They have been available for 2.5 months now in bugzilla, but have not been picked up, despite various reminders and without any reason given.
Google shows that numerous people are hitting this issue. The issue is in itself relatively minor, but the bug in the code is clear.
The patches have been in all mu kernels and today testing has shown that throttling works correctly with the patches applied when the system overheats (http://bugzilla.kernel.org/show_bug.cgi?id=13918#c14).
Cheers,
FJP
drivers/acpi/processor_thermal.c +3 -3
@@ -66,7 +66,7 @@ static int acpi_processor_apply_limit(struct acpi_processor *pr)if (pr->limit.thermal.tx > tx) tx = pr->limit.thermal.tx; - result =acpi_processor_set_throttling (pr, tx); + result =acpi_processor_set_throttling (pr, tx, false);if (result) goto end; }
@@ -421,12 +421,12 @@ processor_set_cur_state(struct thermal_cooling_device *cdev,if (state <= max_pstate) {if (pr->flags.throttling && pr->throttling.state) - result =acpi_processor_set_throttling (pr,0 ); + result =acpi_processor_set_throttling (pr,0 , false);cpufreq_set_cur_state (pr->id, state); }else {cpufreq_set_cur_state (pr->id, max_pstate); result =acpi_processor_set_throttling (pr, - state - max_pstate); + state - max_pstate, false); }return result; }
drivers/acpi/processor_throttling.c +14 -12
@@ -62,7 +62,8 @@ struct throttling_tstate {#define THROTTLING_POSTCHANGE (2)static int acpi_processor_get_throttling (struct acpi_processor *pr); -int acpi_processor_set_throttling (struct acpi_processor *pr,int state); +int acpi_processor_set_throttling (struct acpi_processor *pr, +int state,bool force);static int acpi_processor_update_tsd_coord (void ) {
@@ -361,7 +362,7 @@ int acpi_processor_tstate_has_changed(struct acpi_processor *pr)*/ target_state = throttling_limit; } -return acpi_processor_set_throttling (pr, target_state); +return acpi_processor_set_throttling (pr, target_state, false); }/*
@@ -842,7 +843,7 @@ static int acpi_processor_get_throttling_ptc(struct acpi_processor *pr)ACPI_WARNING ((AE_INFO ,"Invalid throttling state, reset" )); state =0 ; - ret =acpi_processor_set_throttling (pr, state); + ret =acpi_processor_set_throttling (pr, state, true);if (ret)return ret; }
@@ -915,7 +916,7 @@ static int acpi_processor_get_fadt_info(struct acpi_processor *pr) }static int acpi_processor_set_throttling_fadt (struct acpi_processor *pr, -int state) +int state,bool force) {u32 value =0 ;u32 duty_mask =0 ;
@@ -930,7 +931,7 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr,if (!pr->flags.throttling)return -ENODEV ; -if (state == pr->throttling.state) +if (!force && (state == pr->throttling.state))return 0 ;if (state < pr->throttling_platform_limit)
@@ -988,7 +989,7 @@ static int acpi_processor_set_throttling_fadt(struct acpi_processor *pr, }static int acpi_processor_set_throttling_ptc (struct acpi_processor *pr, -int state) +int state,bool force) {int ret;acpi_integer value;
@@ -1002,7 +1003,7 @@ static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr,if (!pr->flags.throttling)return -ENODEV ; -if (state == pr->throttling.state) +if (!force && (state == pr->throttling.state))return 0 ;if (state < pr->throttling_platform_limit)
@@ -1018,7 +1019,8 @@ static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr,return 0 ; } -int acpi_processor_set_throttling (struct acpi_processor *pr,int state) +int acpi_processor_set_throttling (struct acpi_processor *pr, +int state,bool force) {cpumask_var_t saved_mask;int ret =0 ;
@@ -1070,7 +1072,7 @@ int acpi_processor_set_throttling(struct acpi_processor *pr, int state)/* FIXME: use work_on_cpu() */ set_cpus_allowed_ptr (current,cpumask_of (pr->id)); ret = p_throttling->acpi_processor_set_throttling (pr, - t_state.target_state); + t_state.target_state, force); }else {/* * When the T-state coordination is SW_ALL or HW_ALL,
@@ -1103,7 +1105,7 @@ int acpi_processor_set_throttling(struct acpi_processor *pr, int state)set_cpus_allowed_ptr (current,cpumask_of (i)); ret = match_pr->throttling.acpi_processor_set_throttling ( - match_pr, t_state.target_state); + match_pr, t_state.target_state, force); } }/*
@@ -1201,7 +1203,7 @@ int acpi_processor_get_throttling_info(struct acpi_processor *pr)ACPI_DEBUG_PRINT ((ACPI_DB_INFO ,"Disabling throttling (was T%d)\n" , pr->throttling.state)); - result =acpi_processor_set_throttling (pr,0 ); + result =acpi_processor_set_throttling (pr,0 , false);if (result) goto end; }
@@ -1307,7 +1309,7 @@ static ssize_t acpi_processor_write_throttling(struct file *file,if (strcmp (tmpbuf, charp) !=0 )return -EINVAL ; - result =acpi_processor_set_throttling (pr, state_val); + result =acpi_processor_set_throttling (pr, state_val, false);if (result)return result;
include/acpi/processor.h +3 -2
@@ -174,7 +174,7 @@ struct acpi_processor_throttling {cpumask_var_t shared_cpu_map;int (*acpi_processor_get_throttling) (struct acpi_processor * pr);int (*acpi_processor_set_throttling) (struct acpi_processor * pr, -int state); +int state,bool force);u32 address;u8 duty_offset;
@@ -321,7 +321,8 @@ static inline int acpi_processor_ppc_has_changed(struct acpi_processor *pr)/* in processor_throttling.c */ int acpi_processor_tstate_has_changed (struct acpi_processor *pr);int acpi_processor_get_throttling_info (struct acpi_processor *pr); -extern int acpi_processor_set_throttling (struct acpi_processor *pr,int state); +extern int acpi_processor_set_throttling (struct acpi_processor *pr, +int state,bool force);extern const struct file_operations acpi_processor_throttling_fops;extern void acpi_processor_throttling_init (void );/* in processor_idle.c */
PATCH 2/2 ACPI processor: remove superfluous warning message
This failure is very common on many platforms. Handling it in the ACPI processor driver is enough, and we don't need a warning message unless CONFIG_ACPI_DEBUG is set.
Based on a patch from Zhang Rui.
http://bugzilla.kernel.org/show_bug.cgi?id=13389
drivers/acpi/processor_throttling.c +2 -2
@@ -840,8 +840,8 @@ static int acpi_processor_get_throttling_ptc(struct acpi_processor *pr)if (ret >=0 ) { state =acpi_get_throttling_state (pr, value);if (state ==-1 ) { -ACPI_WARNING ((AE_INFO , -"Invalid throttling state, reset" )); +ACPI_DEBUG_PRINT ((ACPI_DB_INFO , +"Invalid throttling state, reset\n" )); state =0 ; ret =acpi_processor_set_throttling (pr, state, true);if (ret)
(cc stable)
Quoted 6 lines · On Mon, 17 Aug 2009 14:43:34 +0200 Frans Pop <[email protected]> wrote:
If the BIOS reports an invalid throttling state (which seems to be fairly common after system boot), a reset is done to state T0. Because of a check in
acpi_processor_get_throttling_ptc(), the reset never actually gets executed, which results in the error reoccurring on every access of for example/proc/acpi/processor/CPU0/throttling.
Quoted 7 lines
Add a 'force' option to
acpi_processor_set_throttling()to ensure the reset really takes effect.http://bugzilla.kernel.org/show_bug.cgi?id=13389
Signed-off-by Frans Pop <[email protected]>Acked-by Zhang Rui <[email protected]>
Unfortunately there are changes in linux-next which make this patch inapplicable in non-trivial ways.
So we'll be needing one flavour of the patch for 2.6.30.x and 2.6.31.x (the patch you just sent) and a different flavour for 2.6.32.
Or we preempt the pending linux-next changes and jam these patches into the tree first.
Quoted 6 lines
This patch, together with the next one, fixes a regression introduced in 2.6.30, listed on the regression list. They have been available for 2.5 months now in bugzilla, but have not been picked up, despite various reminders and without any reason given.
Quoted 6 lines
Google shows that numerous people are hitting this issue. The issue is in itself relatively minor, but the bug in the code is clear.
The patches have been in all mu kernels and today testing has shown that throttling works correctly with the patches applied when the system overheats (http://bugzilla.kernel.org/show_bug.cgi?id=13918#c14).
OK, that sucks.
Guys, what happened here? Fixing regressions surely is our number one hair-on-fire priority, yet the ACPI developers have found other things to be doing for two and a half months and now we have a mess on our hands.
Did this just fall through the cracks or is there some problem?
Len Brown
reviewerQuoted 1 line
Did this just fall through the cracks
yes, during summer vacation, the cracks can sometimes be large.
Thanks for handling this one Andrew.
-Len Brown, Intel Open Source Technology Center
ps. [email protected] isn't my e-mail address.
Quoted 3 lines · you
Quoted 1 line
Did this just fall through the cracks
yes, during summer vacation, the cracks can sometimes be large.
Nice to see you're back :-)
Quoted 5 lines
Thanks for handling this one Andrew.
-Len Brown, Intel Open Source Technology Center
ps. [email protected] isn't my e-mail address.
Hmmm. Then why is that address listed in bugzilla, both in the CC list for #13389 and in the history of that BR for both actions done by you? That's where I took it from...
Is it simply that you need to update your bugzilla account?
Cheers,
FJP
drivers/acpi/processor_throttling.c struct throttling_tstate { · line 67
Quoted 10 lines · On Mon, 17 Aug 2009 14:43:34 +0200 Frans Pop <[email protected]> wrote:
drivers/acpi/processor_throttling.c +2
-1@@ -62,7 +62,8 @@ struct throttling_tstate {#define THROTTLING_POSTCHANGE (2)static int acpi_processor_get_throttling (struct acpi_processor *pr); -int acpi_processor_set_throttling (struct acpi_processor *pr,int state); +int acpi_processor_set_throttling (struct acpi_processor *pr, +int state,bool force);
WARNING: externs should be avoided in .c files
#74: FILE: drivers/acpi/processor_throttling.c:65:
+int acpi_processor_set_throttling(struct acpi_processor *pr,
total: 0 errors, 1 warnings, 137 lines checked
checkpatch speaketh truth - there's already a declaration in acpi/processor.h anyway.
I'll leave it alone though. Cleaning up acpi code isn't on the agenda for today.
Please integrate checkpatch into your patch preparation tools. It finds stuff.
Thanks for picking up the patch Andrew.
Quoted 15 lines · Andrew Morton
drivers/acpi/processor_throttling.c struct throttling_tstate { · line 66
Quoted 9 lines · On Mon, 17 Aug 2009 14:43:34 +0200 Frans Pop <[email protected]> wrote:
drivers/acpi/processor_throttling.c +2
-1@@ -62,7 +62,8 @@ struct throttling_tstate {#define THROTTLING_POSTCHANGE (2)static int acpi_processor_get_throttling (struct acpi_processor *pr); -int acpi_processor_set_throttling (struct acpi_processor *pr,int state); +int acpi_processor_set_throttling (struct acpi_processor *pr, +int state,bool force);WARNING: externs should be avoided in .c files
#74: FILE:drivers/acpi/processor_throttling.c:65:
+int acpi_processor_set_throttling(structacpi_processor*pr,
Quoted 5 lines
checkpatch speaketh truth - there's already a declaration in
acpi/processor.hanyway.I'll leave it alone though. Cleaning up acpi code isn't on the agenda for today.
Yeah, but it wasn't an error introduced by my patch, so I chose to ignore it in order to keep my change straightforward. The whole file gives:
total: 6 errors, 15 warnings, 1326 lines checked
I really did not want to get into that, although this is the only error of that type. You also have to allow for my limited C skills :-/
Quoted 2 lines
Please integrate checkpatch into your patch preparation tools. It finds stuff.
I do run checkpatch frequently, but I don't yet have enough volume that I have my own a toolset for preparing patches. I do have a nice one for building kernels though, so who knows :-)
Cheers,
FJP