[PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Venkatesh Pallipadi]

Remove P-state read status after a P-state write in normal case. As 
that operation is expensive. There are no known failures of a set and we can 
assume success in the common case. acpi_pstate_strict parameter can be used 
to force it back on any systems that is known to have errors.

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

Index: linux-2.6.12/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
===================================================================
--- linux-2.6.12.orig/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	2005-08-16 09:41:52.977456128 -0700
+++ linux-2.6.12/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	2005-08-17 13:32:33.034651136 -0700
@@ -31,6 +31,7 @@
 #include <linux/cpufreq.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/compiler.h>
 #include <asm/io.h>
 #include <asm/delay.h>
 #include <asm/uaccess.h>
@@ -57,6 +58,8 @@
 
 static struct cpufreq_driver acpi_cpufreq_driver;
 
+static unsigned int acpi_pstate_strict;
+
 static int
 acpi_processor_write_port(
 	u16	port,
@@ -163,34 +166,44 @@
 	}
 
 	/*
-	 * Then we read the 'status_register' and compare the value with the
-	 * target state's 'status' to make sure the transition was successful.
-	 * Note that we'll poll for up to 1ms (100 cycles of 10us) before
-	 * giving up.
+	 * Assume the write went through when acpi_pstate_strict is not used.
+	 * As read status_register is an expensive operation and there 
+	 * are no specific error cases where an IO port write will fail.
 	 */
-
-	port = data->acpi_data.status_register.address;
-	bit_width = data->acpi_data.status_register.bit_width;
-
-	dprintk("Looking for 0x%08x from port 0x%04x\n",
-		(u32) data->acpi_data.states[state].status, port);
-
-	for (i=0; i<100; i++) {
-		ret = acpi_processor_read_port(port, bit_width, &value);
-		if (ret) {	
-			dprintk("Invalid port width 0x%04x\n", bit_width);
-			retval = ret;
-			goto migrate_end;
+	if (acpi_pstate_strict) {
+		/* Then we read the 'status_register' and compare the value 
+		 * with the target state's 'status' to make sure the 
+		 * transition was successful.
+		 * Note that we'll poll for up to 1ms (100 cycles of 10us) 
+		 * before giving up.
+		 */
+
+		port = data->acpi_data.status_register.address;
+		bit_width = data->acpi_data.status_register.bit_width;
+
+		dprintk("Looking for 0x%08x from port 0x%04x\n",
+			(u32) data->acpi_data.states[state].status, port);
+
+		for (i=0; i<100; i++) {
+			ret = acpi_processor_read_port(port, bit_width, &value);
+			if (ret) {	
+				dprintk("Invalid port width 0x%04x\n", bit_width);
+				retval = ret;
+				goto migrate_end;
+			}
+			if (value == (u32) data->acpi_data.states[state].status)
+				break;
+			udelay(10);
 		}
-		if (value == (u32) data->acpi_data.states[state].status)
-			break;
-		udelay(10);
+	} else {
+		i = 0;
+		value = (u32) data->acpi_data.states[state].status;
 	}
 
 	/* notify cpufreq */
 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
 
-	if (value != (u32) data->acpi_data.states[state].status) {
+	if (unlikely(value != (u32) data->acpi_data.states[state].status)) {
 		unsigned int tmp = cpufreq_freqs.new;
 		cpufreq_freqs.new = cpufreq_freqs.old;
 		cpufreq_freqs.old = tmp;
@@ -537,6 +550,8 @@
 	return;
 }
 
+module_param(acpi_pstate_strict, uint, 0644);
+MODULE_PARM_DESC(acpi_pstate_strict, "value 0 or non-zero. non-zero -> strict ACPI checks are performed during frequency changes.");
 
 late_initcall(acpi_cpufreq_init);
 module_exit(acpi_cpufreq_exit);

Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Dominik Brodowski]

Hi,

On Fri, Aug 26, 2005 at 05:10:52PM -0700, Venkatesh Pallipadi wrote:
>  	/*
> -	 * Then we read the 'status_register' and compare the value with the
> -	 * target state's 'status' to make sure the transition was successful.
> -	 * Note that we'll poll for up to 1ms (100 cycles of 10us) before
> -	 * giving up.
> +	 * Assume the write went through when acpi_pstate_strict is not used.
> +	 * As read status_register is an expensive operation and there 
> +	 * are no specific error cases where an IO port write will fail.
>  	 */

Well, the IO port write itself might not fail, but the transition itself --
and we're reading the _status_ register here, not the control register where
we've written to. And 8.4.4.1 of ACPI-sepc 3.0 does specifically mention
that transitions _can_ fail, so I think we should handle this possibility.

	Dominik

Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Venkatesh Pallipadi]

On Sun, Aug 28, 2005 at 08:09:41PM +0200, Dominik Brodowski wrote:
> Hi,
> 
> On Fri, Aug 26, 2005 at 05:10:52PM -0700, Venkatesh Pallipadi wrote:
> >  	/*
> > -	 * Then we read the 'status_register' and compare the value with the
> > -	 * target state's 'status' to make sure the transition was successful.
> > -	 * Note that we'll poll for up to 1ms (100 cycles of 10us) before
> > -	 * giving up.
> > +	 * Assume the write went through when acpi_pstate_strict is not used.
> > +	 * As read status_register is an expensive operation and there 
> > +	 * are no specific error cases where an IO port write will fail.
> >  	 */
> 
> Well, the IO port write itself might not fail, but the transition itself --
> and we're reading the _status_ register here, not the control register where
> we've written to. And 8.4.4.1 of ACPI-sepc 3.0 does specifically mention
> that transitions _can_ fail, so I think we should handle this possibility.

Yes. ACPI spec says transitions can fail. But, it doesn't fail often in 
practise. And even if it fails, I think, we should handle it without this 
read os STATUS register. The speedstep-centrino driver, which does similar
thing as acpi-cpufreq, does not do this status check after control MSR write.
We can skip the read of STATUS in cpi-cpufreq in a similar way. No?

I feel the overhead of doing the status read here is too high. As it uses SMM,
the latency for read of STATUS is almost same as write into CONTROL. If we 
think retaining the STATUS read is better, we should atleast double the 
transition time reported in _PSS to compensate for this extra overhead.

And reading the STATUS in a loop should go away. I don't see that it being 
mentioned in ACPI spec. The 1mS loop seems totally redundant.

Thanks,
Venki

Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Dominik Brodowski]

Hi,

On Mon, Aug 29, 2005 at 11:03:57AM -0700, Venkatesh Pallipadi wrote:
> Yes. ACPI spec says transitions can fail. But, it doesn't fail often in 
> practise. And even if it fails, I think, we should handle it without this 
> read os STATUS register.

How can we handle it, if we do not even know that it failed? How should the
user recognize something is broken?

> The speedstep-centrino driver, which does similar
> thing as acpi-cpufreq, does not do this status check after control MSR write.
> We can skip the read of STATUS in cpi-cpufreq in a similar way. No?

Well, regarding speedstep-centrino, it is news to me that the MSR write can
fail... if it can fail, we should check for it.

> And reading the STATUS in a loop should go away. I don't see that it being 
> mentioned in ACPI spec. The 1mS loop seems totally redundant.

It looks to me as somebody had experienced that the transition only
succeeded after waiting for some time.

But well, as you do know the ACPI spec better than I do, I'll accept your
evaluation that this patch won't cause any trouble.

	Dominik

RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Brown, Len]

>How can we handle it, if we do not even know that it failed? 
>How should the user recognize something is broken?

We probably shouldn't have used the word "fail" here.

I expect the IO and MSR accesses will always "succeed",
but they may not always have the exact effect the OS requested.
I think we (the OS) need to get used to this.

The HW reserves the right to second guess the OS for a
variety of reasons, including temperature, power,
reliability, or dependencies between different hardware
units that may not be exposed to the OS.  I think as
hardware becomes more complex and more power-optimized,
we'll see more of this, not less.

cheers,
-Len

RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path

[Brown, Len]

applied to acpi test tree.

thanks,
-Len 

>-----Original Message-----
>From: Venkatesh Pallipadi [mailto:venkatesh.pallipadi@intel.com] 
>Sent: Friday, August 26, 2005 8:11 PM
>To: Andrew Morton
>Cc: linux-kernel; Brown, Len
>Subject: [PATCH] acpi-cpufreq: Remove P-state read after a 
>P-state write in normal path
>
>
>
>Remove P-state read status after a P-state write in normal case. As 
>that operation is expensive. There are no known failures of a 
>set and we can 
>assume success in the common case. acpi_pstate_strict 
>parameter can be used 
>to force it back on any systems that is known to have errors.
>
>Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>
>Index: linux-2.6.12/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c
>===================================================================
>--- 
>linux-2.6.12.orig/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	
>2005-08-16 09:41:52.977456128 -0700
>+++ linux-2.6.12/arch/i386/kernel/cpu/cpufreq/acpi-cpufreq.c	
>2005-08-17 13:32:33.034651136 -0700
>@@ -31,6 +31,7 @@
> #include <linux/cpufreq.h>
> #include <linux/proc_fs.h>
> #include <linux/seq_file.h>
>+#include <linux/compiler.h>
> #include <asm/io.h>
> #include <asm/delay.h>
> #include <asm/uaccess.h>
>@@ -57,6 +58,8 @@
> 
> static struct cpufreq_driver acpi_cpufreq_driver;
> 
>+static unsigned int acpi_pstate_strict;
>+
> static int
> acpi_processor_write_port(
> 	u16	port,
>@@ -163,34 +166,44 @@
> 	}
> 
> 	/*
>-	 * Then we read the 'status_register' and compare the 
>value with the
>-	 * target state's 'status' to make sure the transition 
>was successful.
>-	 * Note that we'll poll for up to 1ms (100 cycles of 
>10us) before
>-	 * giving up.
>+	 * Assume the write went through when 
>acpi_pstate_strict is not used.
>+	 * As read status_register is an expensive operation and there 
>+	 * are no specific error cases where an IO port write will fail.
> 	 */
>-
>-	port = data->acpi_data.status_register.address;
>-	bit_width = data->acpi_data.status_register.bit_width;
>-
>-	dprintk("Looking for 0x%08x from port 0x%04x\n",
>-		(u32) data->acpi_data.states[state].status, port);
>-
>-	for (i=0; i<100; i++) {
>-		ret = acpi_processor_read_port(port, bit_width, &value);
>-		if (ret) {	
>-			dprintk("Invalid port width 0x%04x\n", 
>bit_width);
>-			retval = ret;
>-			goto migrate_end;
>+	if (acpi_pstate_strict) {
>+		/* Then we read the 'status_register' and 
>compare the value 
>+		 * with the target state's 'status' to make sure the 
>+		 * transition was successful.
>+		 * Note that we'll poll for up to 1ms (100 
>cycles of 10us) 
>+		 * before giving up.
>+		 */
>+
>+		port = data->acpi_data.status_register.address;
>+		bit_width = data->acpi_data.status_register.bit_width;
>+
>+		dprintk("Looking for 0x%08x from port 0x%04x\n",
>+			(u32) 
>data->acpi_data.states[state].status, port);
>+
>+		for (i=0; i<100; i++) {
>+			ret = acpi_processor_read_port(port, 
>bit_width, &value);
>+			if (ret) {	
>+				dprintk("Invalid port width 
>0x%04x\n", bit_width);
>+				retval = ret;
>+				goto migrate_end;
>+			}
>+			if (value == (u32) 
>data->acpi_data.states[state].status)
>+				break;
>+			udelay(10);
> 		}
>-		if (value == (u32) data->acpi_data.states[state].status)
>-			break;
>-		udelay(10);
>+	} else {
>+		i = 0;
>+		value = (u32) data->acpi_data.states[state].status;
> 	}
> 
> 	/* notify cpufreq */
> 	cpufreq_notify_transition(&cpufreq_freqs, CPUFREQ_POSTCHANGE);
> 
>-	if (value != (u32) data->acpi_data.states[state].status) {
>+	if (unlikely(value != (u32) 
>data->acpi_data.states[state].status)) {
> 		unsigned int tmp = cpufreq_freqs.new;
> 		cpufreq_freqs.new = cpufreq_freqs.old;
> 		cpufreq_freqs.old = tmp;
>@@ -537,6 +550,8 @@
> 	return;
> }
> 
>+module_param(acpi_pstate_strict, uint, 0644);
>+MODULE_PARM_DESC(acpi_pstate_strict, "value 0 or non-zero. 
>non-zero -> strict ACPI checks are performed during frequency 
>changes.");
> 
> late_initcall(acpi_cpufreq_init);
> module_exit(acpi_cpufreq_exit);
>