[PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Venkatesh Pallipadi]

On some CPUs, we can see transient MSR values (which are not present in _PSS) in
IA32_PERF_STATUS MSR, while CPU is doing some automatic P-state transition 
(like TM2). 
Current code will return frequency as 0 in such cases. Fix it by retrying the 
get after a delay and use lowest possible frequency as the current frequency 
in worst case.

Thanks to Matt Domsch for identifying and root-causing this failure.

Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>

--- linux-2.6.10-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c.org	2004-12-07 18:55:57.000000000 -0800
+++ linux-2.6.10-rc3/arch/i386/kernel/cpu/cpufreq/speedstep-centrino.c	2004-12-09 16:18:30.000000000 -0800
@@ -22,6 +22,8 @@
 #include <linux/init.h>
 #include <linux/cpufreq.h>
 #include <linux/config.h>
+#include <linux/delay.h>
+#include <linux/compiler.h>
 
 #ifdef CONFIG_X86_SPEEDSTEP_CENTRINO_ACPI
 #include <linux/acpi.h>
@@ -277,7 +279,7 @@ static int centrino_verify_cpu_id(const 
 }
 
 /* To be called only after centrino_model is initialized */
-static unsigned extract_clock(unsigned msr, unsigned int cpu)
+static unsigned extract_clock(unsigned msr, unsigned int cpu, int failsafe)
 {
 	int i;
 
@@ -299,15 +301,19 @@ static unsigned extract_clock(unsigned m
 	msr &= 0xffff;
 	for (i=0;centrino_model[cpu]->op_points[i].frequency != CPUFREQ_TABLE_END; i++) {
 		if (msr == centrino_model[cpu]->op_points[i].index)
-		return centrino_model[cpu]->op_points[i].frequency;
+			return centrino_model[cpu]->op_points[i].frequency;
 	}
-	return 0;
+	if (failsafe)
+		return centrino_model[cpu]->op_points[i-1].frequency;
+	else
+		return 0;
 }
 
 /* Return the current CPU frequency in kHz */
 static unsigned int get_cur_freq(unsigned int cpu)
 {
 	unsigned l, h;
+	unsigned clock_freq;
 	cpumask_t saved_mask;
 
 	saved_mask = current->cpus_allowed;
@@ -316,8 +322,34 @@ static unsigned int get_cur_freq(unsigne
 		return 0;
 
 	rdmsr(MSR_IA32_PERF_STATUS, l, h);
+	clock_freq = extract_clock(l, cpu, 0);
+
+	if (unlikely(clock_freq == 0)) {
+		/*
+		 * On some CPUs, we can see transient MSR values (which are
+		 * not present in _PSS), while CPU is doing some automatic
+		 * P-state transition (like TM2). Allow CPU to stabilize at
+		 * some freq and retry.
+		 * If we continue to see transients for long time, just return
+		 * the lowest possible frequency as best guess.
+		 */
+		int retries = 0;
+#define MAX_EXTRACT_CLOCK_RETRIES	5
+		while (clock_freq == 0 && retries < MAX_EXTRACT_CLOCK_RETRIES) {
+			udelay(100);
+			retries++;
+			rdmsr(MSR_IA32_PERF_STATUS, l, h);
+			clock_freq = extract_clock(l, cpu, 0);
+		}
+
+		if (clock_freq == 0) {
+			rdmsr(MSR_IA32_PERF_STATUS, l, h);
+			clock_freq = extract_clock(l, cpu, 1);
+		}
+	}
+
 	set_cpus_allowed(current, saved_mask);
-	return extract_clock(l, cpu);
+	return clock_freq;
 }
 
 
@@ -426,10 +458,10 @@ static int centrino_cpu_init_acpi(struct
 			continue;
 		}
 		
-		if (extract_clock(centrino_model[cpu]->op_points[i].index, cpu) !=
+		if (extract_clock(centrino_model[cpu]->op_points[i].index, cpu, 0) !=
 		    (centrino_model[cpu]->op_points[i].frequency)) {
 			dprintk("Invalid encoded frequency (%u vs. %u)\n",
-				extract_clock(centrino_model[cpu]->op_points[i].index, cpu),
+				extract_clock(centrino_model[cpu]->op_points[i].index, cpu, 0),
 				centrino_model[cpu]->op_points[i].frequency);
 			result = -EINVAL;
 			goto err_kfree_all;
@@ -605,8 +637,8 @@ static int centrino_target (struct cpufr
 	}
 
 	freqs.cpu = cpu;
-	freqs.old = extract_clock(oldmsr, cpu);
-	freqs.new = extract_clock(msr, cpu);
+	freqs.old = extract_clock(oldmsr, cpu, 0);
+	freqs.new = extract_clock(msr, cpu, 0);
 
 	dprintk("target=%dkHz old=%d new=%d msr=%04x\n",
 		target_freq, freqs.old, freqs.new, msr);

Re: [PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Dominik Brodowski]

On Thu, Dec 09, 2004 at 05:24:36PM -0800, Venkatesh Pallipadi wrote:
> 
> On some CPUs, we can see transient MSR values (which are not present in _PSS) in
> IA32_PERF_STATUS MSR, while CPU is doing some automatic P-state transition 
> (like TM2). 
> Current code will return frequency as 0 in such cases. Fix it by retrying the 
> get after a delay and use lowest possible frequency as the current frequency 
> in worst case.
> 
> Thanks to Matt Domsch for identifying and root-causing this failure.
> 
> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
Signed-off-by: Dominik Brodowski <linux@brodo.de>

Thanks,
	Dominik

Re: [PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Jeremy Fitzhardinge]

On Thu, 2004-12-09 at 17:24 -0800, Venkatesh Pallipadi wrote:
> 
> 
> On some CPUs, we can see transient MSR values (which are not present in _PSS) in
> IA32_PERF_STATUS MSR, while CPU is doing some automatic P-state transition 
> (like TM2). 
> Current code will return frequency as 0 in such cases. Fix it by retrying the 
> get after a delay and use lowest possible frequency as the current frequency 
> in worst case.
> 
> Thanks to Matt Domsch for identifying and root-causing this failure.

Doesn't this suggest there's a more subtle problem?  If the CPU is
changing speeds autonomously, then it means the TSC calibration will be
broken.  Is there some way of finding out about these transitions?

Also, why not just report the actual MSR frequency, rather than looking
for a table/_PSS match?  Are the MSR values meaningfully encoded?

	J

Re: [PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Dominik Brodowski]

On Fri, Dec 10, 2004 at 02:11:35AM -0800, Jeremy Fitzhardinge wrote:
> On Thu, 2004-12-09 at 17:24 -0800, Venkatesh Pallipadi wrote:
> > 
> > 
> > On some CPUs, we can see transient MSR values (which are not present in _PSS) in
> > IA32_PERF_STATUS MSR, while CPU is doing some automatic P-state transition 
> > (like TM2). 
> > Current code will return frequency as 0 in such cases. Fix it by retrying the 
> > get after a delay and use lowest possible frequency as the current frequency 
> > in worst case.
> > 
> > Thanks to Matt Domsch for identifying and root-causing this failure.
> 
> Doesn't this suggest there's a more subtle problem?  If the CPU is
> changing speeds autonomously, then it means the TSC calibration will be
> broken.  Is there some way of finding out about these transitions?

The TSC runs at a fixed frequency on these CPUs [Venki will submit a patch
for that soon], so it isn't a problem for the kernel AFAICS.

> Also, why not just report the actual MSR frequency, rather than looking
> for a table/_PSS match?  Are the MSR values meaningfully encoded?

AFAIK, the encoding may be very chip specific.

Thanks,
	Dominik

RE: [PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Dominik Brodowski [mailto:linux@dominikbrodowski.de] 
>Sent: Friday, December 10, 2004 6:48 AM
>To: Jeremy Fitzhardinge
>Cc: Pallipadi, Venkatesh; davej@redhat.com; cpufreq list
>Subject: Re: [PATCH] speedstep-centrino: Avoid returning zero 
>freq on transient MSR values
>
>> Doesn't this suggest there's a more subtle problem?  If the CPU is
>> changing speeds autonomously, then it means the TSC 
>calibration will be
>> broken.  Is there some way of finding out about these transitions?
>
>The TSC runs at a fixed frequency on these CPUs [Venki will 
>submit a patch
>for that soon], so it isn't a problem for the kernel AFAICS.

Yes. These are Pentium 4 based processsors and with Enhanced Speedstep
on these processors, tsc rate will not change with CPU frequency change.

>> Also, why not just report the actual MSR frequency, rather 
>than looking
>> for a table/_PSS match?  Are the MSR values meaningfully encoded?
>
>AFAIK, the encoding may be very chip specific.

The MSR value encoding is not well defined and can change. 
I agree that the udelay in this patch is kind of ugly, as it can slow
down get_freq(). But, the transient value are uncommon, so we may not go
through that code path very frequently. Other, way to solve it is to
look at CONTROL MSR, whenever we have this transient value.
I will work on a patch from that angle and we can probably pick the
better of the two patches.

Thanks,
Venki

RE: [PATCH] speedstep-centrino: Avoid returning zero freq on transient MSR values

[Jeremy Fitzhardinge]

On Fri, 2004-12-10 at 10:37 -0800, Pallipadi, Venkatesh wrote:
> Yes. These are Pentium 4 based processsors and with Enhanced Speedstep
> on these processors, tsc rate will not change with CPU frequency change.

Can the Pentium M not also change speed by itself?  The documentation of
the TM2 seems to say that it can.

	J