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

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

[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).

Specifically during the testing of v3.0-rc2, Tobias found a peculiar issue with
his AMD machine: http://mid.gmane.org/20110613202622.GC20616@yumi.tdiedrich.de

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(-)

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

[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 <ranma+xen@tdiedrich.de>
Tested-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/kernel/cpu/cpufreq/powernow-k8.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
index 2368e38..7e4a664 100644
--- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
+++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
@@ -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) {
-- 
1.7.4.1

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

[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:

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

Reported-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
Tested-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/cpufreq/cpufreq_stats.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c
index 00d73fc..cacabcb 100644
--- a/drivers/cpufreq/cpufreq_stats.c
+++ b/drivers/cpufreq/cpufreq_stats.c
@@ -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);
-- 
1.7.4.1

Re: [PATCH] cpufreq bug fixes (stable/cpufreq.bugfixes) for 3.0 (or 3.1).

[Dave Jones]

On Wed, Jun 15, 2011 at 03:01:58PM -0400, Konrad Rzeszutek Wilk wrote:
 > 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

Re: [PATCH] cpufreq bug fixes (stable/cpufreq.bugfixes) for 3.0 (or 3.1).

[Konrad Rzeszutek Wilk]

On Wed, Jun 15, 2011 at 03:26:30PM -0400, Dave Jones wrote:
> On Wed, Jun 15, 2011 at 03:01:58PM -0400, Konrad Rzeszutek Wilk wrote:
>  > 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.

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

[Borislav Petkov]

On Wed, Jun 15, 2011 at 03:01:59PM -0400, Konrad Rzeszutek Wilk wrote:
> 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 <ranma+xen@tdiedrich.de>
> Tested-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  arch/x86/kernel/cpu/cpufreq/powernow-k8.c |    7 ++++++-
>  1 files changed, 6 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> index 2368e38..7e4a664 100644
> --- a/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> +++ b/arch/x86/kernel/cpu/cpufreq/powernow-k8.c
> @@ -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;

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.

-- 
Regards/Gruss,
    Boris.

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

[Konrad Rzeszutek Wilk]

> > @@ -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).

Aha! I can respin a patch for that tomorrow.
> 
> 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.

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

[Konrad Rzeszutek Wilk]

On Wed, Jun 15, 2011 at 06:26:08PM -0400, Konrad Rzeszutek Wilk wrote:
> > > @@ -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).
> 
> 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 <konrad.wilk@oracle.com>
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:

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 <bp@alien8.de>
CC: andre.przywara@amd.com
CC: Mark.Langsdorf@amd.com
Reported-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
Tested-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
[v1: Rebased on v3.0-rc2, reduced patch to deal with vid case]
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/cpufreq/powernow-k8.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 83479b6..fe53572 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -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) {
-- 
1.7.5.4

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

[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 <konrad.wilk@oracle.com>
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.

Suggested-by: Borislav Petkov <bp@alien8.de>
CC: andre.przywara@amd.com
CC: Mark.Langsdorf@amd.com
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/cpufreq/powernow-k8.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index fe53572..047c7b11 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -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);
-- 
1.7.5.4

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

[Borislav Petkov]

On Thu, Jun 16, 2011 at 10:24:40AM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 15, 2011 at 06:26:08PM -0400, Konrad Rzeszutek Wilk wrote:
> > > > @@ -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).
> > 
> > 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 <konrad.wilk@oracle.com>
> 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:

					follows:
> 
> 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 <bp@alien8.de>
> CC: andre.przywara@amd.com
> CC: Mark.Langsdorf@amd.com
> Reported-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
> Tested-by: Tobias Diedrich <ranma+xen@tdiedrich.de>
> [v1: Rebased on v3.0-rc2, reduced patch to deal with vid case]
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/cpufreq/powernow-k8.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 83479b6..fe53572 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -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.

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


Other than that:

Acked-by: Borislav Petkov <bp@alien8.de>

-- 
Regards/Gruss,
Boris.

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

[Borislav Petkov]

On Thu, Jun 16, 2011 at 10:28:29AM -0400, Konrad Rzeszutek Wilk wrote:
> 
> And as mentioned, the other patch that just deals with pstates.
> 
> From 0f3bc30b6bc7dad62e3b77063b69df44ca9a9f8e Mon Sep 17 00:00:00 2001
> From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 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

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

				an

> It also checks whether the frequency for the pstate is
> incorrect and if so errors out.
> 
> Suggested-by: Borislav Petkov <bp@alien8.de>
> CC: andre.przywara@amd.com
> CC: Mark.Langsdorf@amd.com
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/cpufreq/powernow-k8.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index fe53572..047c7b11 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -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.

>  	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.

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

Thanks.

-- 
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