RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
>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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
applied to acpi test tree.
thanks,
-Len
>-Original Message-
>From: Venkatesh Pallipadi [mailto:[EMAIL PROTECTED]
>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 <[EMAIL PROTECTED]>
>
>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
> #include
> #include
>+#include
> #include
> #include
> #include
>@@ -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, );
>- 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, );
>+ 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(_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);
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] acpi-cpufreq: Remove P-state read after a P-state write in normal path
applied to acpi test tree.
thanks,
-Len
-Original Message-
From: Venkatesh Pallipadi [mailto:[EMAIL PROTECTED]
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 [EMAIL PROTECTED]
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; i100; 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; i100; 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);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
