cpufreq bug fixes (stable/cpufreq.bugfixes) for 3.0 (or 3.1).

PATCH In review Konrad Rzeszutek Wilk 4 participants 11 messages Jun 15, 2011 – Jun 16, 2011

Subsystems (2)

x86 CPU frequency scaling framework

lore

Share & tools

lore

Konrad Rzeszutek Wilk

author

2011-06-15 19:01 UTC #0 lore raw

PATCH 1/1 cpufreq bug fixes (stable/cpufreq.bugfixes) for 3.0 (or 3.1).

The two patches attached make the cpufreq and powernowk8 driver more robust on badly behaving machines (or hypervisors not passing through proper rmsdr calls).

Specifically during the testing of v3.0-rc2, Tobias found a peculiar issue with his AMD machine: http://mid.gmane.org/[email protected]

It looks as the Xen hypervisor lacks passthrough RDMSR entries for the PowerNow K8 voltage settings (MSR_FIDVID_STATUS and MSR_FIDVID_CTL) resulting in the writes/reads giving the value zero. The end result being that the cpufreq stats code BUG()s on incorrect index of the freq table.

The patches are also located at:

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git stable/cpufreq.bugfixes

The crash looks as follow:

powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3700+ (1 cpu cores) (version 2.20.00)
powernow-k8: fid 0x2 (1000 MHz), vid 0x12
powernow-k8: fid 0xa (1800 MHz), vid 0xa
powernow-k8: fid 0xc (2000 MHz), vid 0x8
powernow-k8: fid 0xe (2200 MHz), vid 0x8
Marking TSC unstable due to cpufreq changes
powernow-k8: fid trans failed, fid 0x2, curr 0x0

BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b
.. snip..
Pid: 1, comm: swapper Not tainted 3.0.0-rc2 #45 MICRO-STAR INTERNATIONAL CO., LTD MS-7094/MS-7094
..snip..
Call Trace:
 [<ffffffff81479248>] cpufreq_stat_notifier_trans+0x48/0x7c
 [<ffffffff81095d68>] notifier_call_chain+0x32/0x5e
 [<ffffffff81095e6b>] __srcu_notifier_call_chain+0x47/0x63
 [<ffffffff81095e96>] srcu_notifier_call_chain+0xf/0x11
 [<ffffffff81477e7a>] cpufreq_notify_transition+0x111/0x134
 [<ffffffff8147b0d4>] powernowk8_target+0x53b/0x617
 [<ffffffff8147723a>] __cpufreq_driver_target+0x2e/0x30
 [<ffffffff8147a127>] cpufreq_governor_dbs+0x339/0x356
 [<ffffffff81477394>] __cpufreq_governor+0xa8/0xe9
 [<ffffffff81477525>] __cpufreq_set_policy+0x132/0x13e
 [<ffffffff8147848d>] cpufreq_add_dev_interface+0x272/0x28c

Konrad Rzeszutek Wilk (2):
[CPUFREQ] powernow-k8: Don't notify of successful transition if we failed. [CPUFREQ]: Don't set stat->last_index to -1 if the pol->cur has incorrect value.

 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |    7 ++++++-
 drivers/cpufreq/cpufreq_stats.c           |    8 +++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

Konrad Rzeszutek Wilk

author

2011-06-15 19:01 UTC #1 lore raw

PATCH 1/2 powernow-k8: Don't notify of successful transition if we failed.

Before this patch if we failed the transition (either p-state or voltage) we would still try to submit the "new" frequencies to cpufreq. That is incorrect - also we could submit a non-existing frequency value which would cause cpufreq to crash. The ultimate fix is in cpufreq to deal with incorrect values, but this patch improves the error recovery in the AMD powernowk8 driver.

The failure that was reported was as follow:

BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b

...

And transition fails and data->currfid ends up with 0. Since the machine does not support 800Mhz value when the calculation is done ('find_khz_freq_from_fid(data->currfid);') it reports the new frequency as 800000 which is bogus. This patch fixes the issue during target setting.

The patch however does not fix the issue in 'powernowk8_cpu_init' where the pol->cur can also be set with the 800000 value:

          pol->cur = find_khz_freq_from_fid(data->currfid);

dprintk("policy current frequency %d kHz\n", pol->cur);

/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {

The fix for that looks to update cpufreq_frequency_table_cpuinfo to check pol->cur.... but that would cause an regression in how the acpi-cpufreq driver works (it sets cpu->cur after calling cpufreq_frequency_table_cpuinfo). Instead the fix will be to let cpufreq gracefully handle bogus data.

Reported-by Tobias Diedrich <[email protected]>

Tested-by Tobias Diedrich <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

arch/x86/kernel/cpu/cpufreq/powernow-k8.c +6 -1

index 2368e38..7e4a664 100644
@@ -1079,8 +1079,10 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
 	}
 
 	res = transition_fid_vid(data, fid, vid);
-	freqs.new = find_khz_freq_from_fid(data->currfid);
+	if (res)
+		return res;
 
+	freqs.new = find_khz_freq_from_fid(data->currfid);
 	for_each_cpu(i, data->available_cores) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);

@@ -1112,6 +1114,9 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	}
 
 	res = transition_pstate(data, pstate);
+	if (res)
+		return res;
+
 	freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate);
 
 	for_each_cpu(i, data->available_cores) {

Signature · 1 line

1.7.4.1

Borislav Petkov

maintainer

2011-06-15 22:16 UTC #2 lore raw

Re: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful transition if we failed.

Quoted 65 lines · Konrad Rzeszutek Wilk

Before this patch if we failed the transition (either p-state or voltage) we would still try to submit the "new" frequencies to cpufreq. That is incorrect - also we could submit a non-existing frequency value which would cause cpufreq to crash. The ultimate fix is in cpufreq to deal with incorrect values, but this patch improves the error recovery in the AMD powernowk8 driver.
The failure that was reported was as follow:
powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3700+ (1 cpu cores) (version 2.20.00)
powernow-k8: fid 0x2 (1000 MHz), vid 0x12
powernow-k8: fid 0xa (1800 MHz), vid 0xa
powernow-k8: fid 0xc (2000 MHz), vid 0x8
powernow-k8: fid 0xe (2200 MHz), vid 0x8
Marking TSC unstable due to cpufreq changes
powernow-k8: fid trans failed, fid 0x2, curr 0x0
BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b
...
And transition fails and data->currfid ends up with 0. Since the machine does not support 800Mhz value when the calculation is done ('find_khz_freq_from_fid(data->currfid);') it reports the new frequency as 800000 which is bogus. This patch fixes the issue during target setting.
The patch however does not fix the issue in 'powernowk8_cpu_init' where the pol->cur can also be set with the 800000 value:
          pol->cur = find_khz_freq_from_fid(data->currfid);
dprintk("policy current frequency %d kHz\n", pol->cur);
/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {
The fix for that looks to update cpufreq_frequency_table_cpuinfo to check pol->cur.... but that would cause an regression in how the acpi-cpufreq driver works (it sets cpu->cur after calling cpufreq_frequency_table_cpuinfo). Instead the fix will be to let cpufreq gracefully handle bogus data.
Reported-by Tobias Diedrich <[email protected]>
Tested-by Tobias Diedrich <[email protected]>
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)
arch/x86/kernel/cpu/cpufreq/powernow-k8.c +3 -1
index 2368e38..7e4a664 100644
@@ -1079,8 +1079,10 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
 	}
 
 	res = transition_fid_vid(data, fid, vid);
-	freqs.new = find_khz_freq_from_fid(data->currfid);
+	if (res)
+		return res;
 
+	freqs.new = find_khz_freq_from_fid(data->currfid);
 	for_each_cpu(i, data->available_cores) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);

arch/x86/kernel/cpu/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1118

Quoted 5 lines

arch/x86/kernel/cpu/cpufreq/powernow-k8.c +2 -0

@@ -1112,6 +1114,9 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	}
 
 	res = transition_pstate(data, pstate);
+	if (res)
+		return res;

That's wrong because transition_pstate() returns 0 unconditionally (at least it does so on 3.0-rc3). But this change accidentally fixes a different bug because res is used uninitialized, containing stack garbage otherwise.

A proper fix should be to check against data->max_hw_pstate and check whether the entry is not CPUFREQ_ENTRY_INVALID (look at fill_powernow_table_pstate() for example).

I'm guessing this oops happens when powernow-k8 is loaded in the guest and that the actual power management is done in the hypervisor?

Adding some more people to CC.

Signature · 2 lines

Regards/Gruss,
    Boris.

Konrad Rzeszutek Wilk

author

2011-06-15 22:26 UTC #3 lore raw

Re: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful transition if we failed.

Quoted 12 lines

arch/x86/kernel/cpu/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1118
Quoted 6 lines
arch/x86/kernel/cpu/cpufreq/powernow-k8.c +2 -0
@@ -1112,6 +1114,9 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	}
 
 	res = transition_pstate(data, pstate);
+	if (res)
+		return res;
That's wrong because transition_pstate() returns 0 unconditionally (at least it does so on 3.0-rc3). But this change accidentally fixes a different bug because res is used uninitialized, containing stack garbage otherwise.

Quoted 3 lines

A proper fix should be to check against data->max_hw_pstate and check whether the entry is not CPUFREQ_ENTRY_INVALID (look at fill_powernow_table_pstate() for example).

Aha! I can respin a patch for that tomorrow.

Quoted 3 lines

I'm guessing this oops happens when powernow-k8 is loaded in the guest and that the actual power management is done in the hypervisor?

Yes.

Konrad Rzeszutek Wilk

author Tested-by Tobias Diedrich

2011-06-16 14:24 UTC #4 lore raw

Re: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful transition if we failed.

Quoted 12 lines · Konrad Rzeszutek Wilk

Quoted 12 lines
Review static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1118
Quoted 6 lines
(quoted diff) +2 -0
@@ -1112,6 +1114,9 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	}
 
 	res = transition_pstate(data, pstate);
+	if (res)
+		return res;
That's wrong because transition_pstate() returns 0 unconditionally (at least it does so on 3.0-rc3). But this change accidentally fixes a different bug because res is used uninitialized, containing stack garbage otherwise.

Quoted 5 lines

Quoted 3 lines
A proper fix should be to check against data->max_hw_pstate and check whether the entry is not CPUFREQ_ENTRY_INVALID (look at fill_powernow_table_pstate() for example).
Aha! I can respin a patch for that tomorrow.

I divided it in two patches - so that the Reported/Tested-by tag is on the translate_vid patch with its full glory of explanation. The pstate checking is removed.

Will post the other patch under a different subject.

From 3ae9a2094d893ab1a500833a48cb29e0f1c81ed8 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Thu, 16 Jun 2011 09:02:57 -0400
Subject: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful
transition if we failed (vid case).

Before this patch if we failed the vid transition would still try to submit the "new" frequencies to cpufreq.
That is incorrect - also we could submit a non-existing frequency value which would cause cpufreq to crash. The ultimate fix is in cpufreq to deal with incorrect values, but this patch improves the error recovery in the AMD powernowk8 driver.

The failure that was reported was as follow:

BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b

...

The patch however does not fix the issue in 'powernowk8_cpu_init' where the pol->cur can also be set with the 800000 value:

          pol->cur = find_khz_freq_from_fid(data->currfid);

dprintk("policy current frequency %d kHz\n", pol->cur);

/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {

CC Borislav Petkov <[email protected]>

CC [email protected]

Reported-by Tobias Diedrich <[email protected]>

Tested-by Tobias Diedrich <[email protected]>

[v1: Rebased on v3.0-rc2, reduced patch to deal with vid case]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 drivers/cpufreq/powernow-k8.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

drivers/cpufreq/powernow-k8.c +2 -0

index 83479b6..fe53572 100644
@@ -1079,6 +1079,8 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
 	}
 
 	res = transition_fid_vid(data, fid, vid);
+	if (res)
+		return res;
 	freqs.new = find_khz_freq_from_fid(data->currfid);
 
 	for_each_cpu(i, data->available_cores) {

Signature · 1 line

1.7.5.4

Borislav Petkov

maintainer Acked-by

2011-06-16 15:44 UTC #5 lore raw

Re: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful transition if we failed.

Quoted 31 lines · Konrad Rzeszutek Wilk

Quoted 12 lines · Konrad Rzeszutek Wilk
Quoted 12 lines
drivers/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1118
Quoted 6 lines
drivers/cpufreq/powernow-k8.c +2 -0
@@ -1112,6 +1114,9 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	}
 
 	res = transition_pstate(data, pstate);
+	if (res)
+		return res;
That's wrong because transition_pstate() returns 0 unconditionally (at least it does so on 3.0-rc3). But this change accidentally fixes a different bug because res is used uninitialized, containing stack garbage otherwise.
Quoted 5 lines
Quoted 3 lines
A proper fix should be to check against data->max_hw_pstate and check whether the entry is not CPUFREQ_ENTRY_INVALID (look at fill_powernow_table_pstate() for example).
Aha! I can respin a patch for that tomorrow.
I divided it in two patches - so that the Reported/Tested-by tag is on the translate_vid patch with its full glory of explanation. The pstate checking is removed.
Will post the other patch under a different subject.
From 3ae9a2094d893ab1a500833a48cb29e0f1c81ed8 Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Thu, 16 Jun 2011 09:02:57 -0400
Subject: [PATCH 1/2] [CPUFREQ] powernow-k8: Don't notify of successful
transition if we failed (vid case).

Quoted 8 lines

Before this patch if we failed the vid transition would still try to submit the "new" frequencies to cpufreq.
That is incorrect - also we could submit a non-existing frequency value which would cause cpufreq to crash. The ultimate fix is in cpufreq to deal with incorrect values, but this patch improves the error recovery in the AMD powernowk8 driver.
The failure that was reported was as follow:

follows:

Quoted 49 lines

powernow-k8: Found 1 AMD Athlon(tm) 64 Processor 3700+ (1 cpu cores) (version 2.20.00)
powernow-k8: fid 0x2 (1000 MHz), vid 0x12
powernow-k8: fid 0xa (1800 MHz), vid 0xa
powernow-k8: fid 0xc (2000 MHz), vid 0x8
powernow-k8: fid 0xe (2200 MHz), vid 0x8
Marking TSC unstable due to cpufreq changes
powernow-k8: fid trans failed, fid 0x2, curr 0x0
BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b
...
And transition fails and data->currfid ends up with 0. Since the machine does not support 800Mhz value when the calculation is done ('find_khz_freq_from_fid(data->currfid);') it reports the new frequency as 800000 which is bogus. This patch fixes the issue during target setting.
The patch however does not fix the issue in 'powernowk8_cpu_init' where the pol->cur can also be set with the 800000 value:
          pol->cur = find_khz_freq_from_fid(data->currfid);
dprintk("policy current frequency %d kHz\n", pol->cur);
/* min/max the cpu is capable of */
if (cpufreq_frequency_table_cpuinfo(pol, data->powernow_table)) {
The fix for that looks to update cpufreq_frequency_table_cpuinfo to check pol->cur.... but that would cause an regression in how the acpi-cpufreq driver works (it sets cpu->cur after calling cpufreq_frequency_table_cpuinfo). Instead the fix will be to let cpufreq gracefully handle bogus data (another patch).
CC Borislav Petkov <[email protected]>
CC [email protected]
CC [email protected]
Reported-by Tobias Diedrich <[email protected]>
Tested-by Tobias Diedrich <[email protected]>
[v1: Rebased on v3.0-rc2, reduced patch to deal with vid case]
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
---
 drivers/cpufreq/powernow-k8.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/cpufreq/powernow-k8.c +0 -0
index 83479b6..fe53572 100644

drivers/cpufreq/powernow-k8.c static int transition_frequency_fidvid(struct powernow_k8_data *data, · line 1083

Quoted 5 lines

drivers/cpufreq/powernow-k8.c +2 -0

@@ -1079,6 +1079,8 @@ static int transition_frequency_fidvid(struct powernow_k8_data *data,
 	}
 
 	res = transition_fid_vid(data, fid, vid);
+	if (res)
+		return res;

a newline here please.

drivers/cpufreq/powernow-k8.c static int transition_frequency_fidvid(struct powernow_k8_data *data, · line 1086

Quoted 3 lines

drivers/cpufreq/powernow-k8.c +0 -0

 	freqs.new = find_khz_freq_from_fid(data->currfid);
 
 	for_each_cpu(i, data->available_cores) {

Other than that:

Acked-by Borislav Petkov <[email protected]>

Signature · 2 lines

Regards/Gruss,
Boris.

Konrad Rzeszutek Wilk

author

2011-06-15 19:02 UTC #6 lore raw

PATCH 2/2 : Don't set stat->last_index to -1 if the pol->cur has incorrect value.

In reply to Konrad Rzeszutek Wilk

If the driver submitted an non-existing pol>cur value (say it used the default initialized value of zero), when the cpufreq stats tries to setup its initial values it incorrectly sets stat->last_index to -1 (or 0xfffff...). And cpufreq_stats_update tries to update at that index location and fails.

This can be caused by:

stat->last_index = freq_table_get_index(stat, policy->cur);

not finding the appropiate frequency in the table (b/c the policy->cur is wrong) and we end up crashing. The fix however is concentrated in the 'cpufreq_stats_update' as the last_index (and old_index) are updated there. Which means it can reset the last_index to -1 again and on the next iteration cause a crash.

Without this patch, the following crash is observed:

BUG: unable to handle kernel paging request at ffff880807e07b78
IP: [<ffffffff81479163>] cpufreq_stats_update+0x46/0x5b
.. snip..
Pid: 1, comm: swapper Not tainted 3.0.0-rc2 #45 MICRO-STAR INTERNATIONAL CO., LTD MS-7094/MS-7094
..snip..
Call Trace:
 [<ffffffff81479248>] cpufreq_stat_notifier_trans+0x48/0x7c
 [<ffffffff81095d68>] notifier_call_chain+0x32/0x5e
 [<ffffffff81095e6b>] __srcu_notifier_call_chain+0x47/0x63
 [<ffffffff81095e96>] srcu_notifier_call_chain+0xf/0x11
 [<ffffffff81477e7a>] cpufreq_notify_transition+0x111/0x134
 [<ffffffff8147b0d4>] powernowk8_target+0x53b/0x617
 [<ffffffff8147723a>] __cpufreq_driver_target+0x2e/0x30
 [<ffffffff8147a127>] cpufreq_governor_dbs+0x339/0x356
 [<ffffffff81477394>] __cpufreq_governor+0xa8/0xe9
 [<ffffffff81477525>] __cpufreq_set_policy+0x132/0x13e
 [<ffffffff8147848d>] cpufreq_add_dev_interface+0x272/0x28c

Reported-by Tobias Diedrich <[email protected]>

Tested-by Tobias Diedrich <[email protected]>

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 drivers/cpufreq/cpufreq_stats.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

drivers/cpufreq/cpufreq_stats.c +5 -3

index 00d73fc..cacabcb 100644
@@ -288,11 +288,13 @@ static int cpufreq_stat_notifier_trans(struct notifier_block *nb,
 	old_index = stat->last_index;
 	new_index = freq_table_get_index(stat, freq->new);
 
-	cpufreq_stats_update(freq->cpu);
-	if (old_index == new_index)
+	/* We can't do stat->time_in_state[-1]= .. */
+	if (old_index == -1 || new_index == -1)
 		return 0;
 
-	if (old_index == -1 || new_index == -1)
+	cpufreq_stats_update(freq->cpu);
+
+	if (old_index == new_index)
 		return 0;
 
 	spin_lock(&cpufreq_stats_lock);

Signature · 1 line

1.7.4.1

Dave Jones

2011-06-15 19:26 UTC #7 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 2 lines · Konrad Rzeszutek Wilk

The two patches attached make the cpufreq and powernowk8 driver more robust on badly behaving machines (or hypervisors not passing through proper rmsdr calls).

I applied these (after fixing up the change of filename in the powernow driver) I'll send Linus a pull req to make sure these get into 3.0 later.

thanks,

Dave

Konrad Rzeszutek Wilk

author

2011-06-15 21:13 UTC #8 lore raw

Quoted 6 lines · Dave Jones

Quoted 2 lines · Konrad Rzeszutek Wilk
The two patches attached make the cpufreq and powernowk8 driver more robust on badly behaving machines (or hypervisors not passing through proper rmsdr calls).
I applied these (after fixing up the change of filename in the powernow driver) I'll send Linus a pull req to make sure these get into 3.0 later.

Thank you. Sorry about missing the directory move and you having to do extra legwork.

Konrad Rzeszutek Wilk

author

2011-06-16 14:28 UTC #9 lore raw

PATCH 1/1 powernow-k8: Don't try to transition if the pstate is incorrect or there is no freq for it.

In reply to Konrad Rzeszutek Wilk

And as mentioned, the other patch that just deals with pstates.

From 0f3bc30b6bc7dad62e3b77063b69df44ca9a9f8e Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Wed, 15 Jun 2011 11:08:02 -0400
Subject: [PATCH] [CPUFREQ] powernow-k8: Don't try to transition if the pstate is incorrect or there is no freq for it.

This patch auguments the pstate transition code to error out (instead of returning 0) when a incorrect pstate is provided.

It also checks whether the frequency for the pstate is incorrect and if so errors out.

CC [email protected]

Signed-off-by Konrad Rzeszutek Wilk <[email protected]>

---
 drivers/cpufreq/powernow-k8.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

drivers/cpufreq/powernow-k8.c +5 -1

index fe53572..047c7b11 100644
@@ -1103,11 +1103,15 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	/* get MSR index for hardware pstate transition */
 	pstate = index & HW_PSTATE_MASK;
 	if (pstate > data->max_hw_pstate)
-		return 0;
+		return -EINVAL;
+
 	freqs.old = find_khz_freq_from_pstate(data->powernow_table,
 			data->currpstate);
 	freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate);
 
+	if (freqs.new == CPUFREQ_ENTRY_INVALID)
+		return -EINVAL;
+
 	for_each_cpu(i, data->available_cores) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);

Signature · 1 line

1.7.5.4

Borislav Petkov

2011-06-16 17:06 UTC #10 lore raw

Re: [PATCH] [CPUFREQ] powernow-k8: Don't try to transition if the pstate is incorrect or there is no freq for it.

Quoted 9 lines · Konrad Rzeszutek Wilk

And as mentioned, the other patch that just deals with pstates.
From 0f3bc30b6bc7dad62e3b77063b69df44ca9a9f8e Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <[email protected]>
Date: Wed, 15 Jun 2011 11:08:02 -0400
Subject: [PATCH] [CPUFREQ] powernow-k8: Don't try to transition if the pstate is incorrect or there is no freq for it.
This patch auguments the pstate transition code to error out

augments

Quoted 1 line

(instead of returning 0) when a incorrect pstate is provided.

Quoted 16 lines

It also checks whether the frequency for the pstate is incorrect and if so errors out.
Suggested-by Borislav Petkov <[email protected]>
CC [email protected]
CC [email protected]
Signed-off-by Konrad Rzeszutek Wilk <[email protected]>
---
 drivers/cpufreq/powernow-k8.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
drivers/cpufreq/powernow-k8.c +0 -0
index fe53572..047c7b11 100644

drivers/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1107

Quoted 6 lines

drivers/cpufreq/powernow-k8.c +2 -1

@@ -1103,11 +1103,15 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	/* get MSR index for hardware pstate transition */
 	pstate = index & HW_PSTATE_MASK;
 	if (pstate > data->max_hw_pstate)
-		return 0;
+		return -EINVAL;
+

Hehe, this was a bug, actually, good catch!

powernowk8_target is actually considering ret = 0 a success and ret != 0 a failure but we were returning a 0 when our target pstate is bogus.

drivers/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1114

Quoted 7 lines

drivers/cpufreq/powernow-k8.c +3 -0

 	freqs.old = find_khz_freq_from_pstate(data->powernow_table,
 			data->currpstate);
 	freqs.new = find_khz_freq_from_pstate(data->powernow_table, pstate);
 
+	if (freqs.new == CPUFREQ_ENTRY_INVALID)
+		return -EINVAL;
+

No need for that since freqs.new is determined from pstate and you're checking pstate above.

drivers/cpufreq/powernow-k8.c static int transition_frequency_pstate(struct powernow_k8_data *data, · line 1117

Quoted 3 lines

drivers/cpufreq/powernow-k8.c +0 -0

 	for_each_cpu(i, data->available_cores) {
 		freqs.cpu = i;
 		cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);

Thanks.

Signature · 8 lines

Regards/Gruss,
Boris.

Advanced Micro Devices GmbH
Einsteinring 24, 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632