ACPI: EC: clean up tmp variable before reuse

Fix breakage introduced by following patch 27663c5855b10af9ec67bc7dfba001426ba21222 is first bad commit commit 27663c5855b10af9ec67bc7dfba001426ba21222

Author: Matthew Wilcox <[email protected]>
Date:   Fri Oct 10 02:22:59 2008 -0400

acpi_evaluate_integer() does not clear passed variable if there is an error at evaluation.
So if we ignore error, we must supply initialized variable.

References: http://bugzilla.kernel.org/show_bug.cgi?id=11917

	    http://bugzilla.kernel.org/show_bug.cgi?id=11896

Subject: ACPI: Cleanup :Initialize EC global lock based on the return status
From: Zhao Yakui <[email protected]>

Initialize the EC global lock based on the returned value of _GLK object. Only when AE_OK is returned by the acpi_evaluate_integer, the EC global lock will be assigned based on the return value of _GLK object.Otherwise it means that there is no _GLK object and the global lock won't be required when EC is accessed.
If the return value of _GLK object is not zero, it means that GLobal lock will be required when EC is accessed.
If the return value of _GLK object is zero, it means that GLobal lock won't be required when EC is accessed.

Signed-off-by Zhao Yakui <[email protected]>

cc Alexey Starikovskiy <[email protected]>

cc Alan Jenkins <[email protected]>

---
 drivers/acpi/ec.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================

drivers/acpi/ec.c +22 -2

@@ -752,8 +752,28 @@ ec_parse_device(acpi_handle handle, u32 
 	ec->gpe = tmp;
 	/* Use the global lock for all EC transactions? */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
-	ec->global_lock = tmp;
+	/*
+	 * Only when AE_OK is returned by acpi_evaluate_interger,
+	 * the ec->global_lock will be assigned based on the returned
+	 * value by _GLK. Otherwise it means that there is no _GLK object
+	 * and global lock won't be required when EC is accessed.
+	 * If the return value of _GLK object is not zero, it means that
+	 * global lock will be required when EC is accessed.
+	 * If the return value of _GLK object is zero, it means that
+	 * global lock won't be required when EC is accessed.
+	 */
+	status = acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (ACPI_SUCCESS(status)) {
+		/*
+		 * If the return value is not zero, it means that global lock
+		 * is required when EC is accessed
+		 */
+		if (tmp)
+			ec->global_lock = 1;
+		else
+			ec->global_lock = 0;
+	} else
+		ec->global_lock = 0;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
 }

NAK

Quoted 58 lines · Zhao Yakui

Subject: ACPI: Cleanup :Initialize EC global lock based on the return status
From: Zhao Yakui <[email protected]>

Initialize the EC global lock based on the returned value of _GLK object. Only when AE_OK is returned by the acpi_evaluate_integer, the EC global lock will be assigned based on the return value of _GLK object.Otherwise it means that there is no _GLK object and the global lock won't be required when EC is accessed.
If the return value of _GLK object is not zero, it means that GLobal lock will be required when EC is accessed.
If the return value of _GLK object is zero, it means that GLobal lock won't be required when EC is accessed.

Signed-off-by Zhao Yakui <[email protected]>

cc Alexey Starikovskiy <[email protected]>

cc Alan Jenkins <[email protected]>

---
 drivers/acpi/ec.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================

drivers/acpi/ec.c +22 -2

@@ -752,8 +752,28 @@ ec_parse_device(acpi_handle handle, u32 
 	ec->gpe = tmp;
 	/* Use the global lock for all EC transactions? */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
-	ec->global_lock = tmp;
+	/*
+	 * Only when AE_OK is returned by acpi_evaluate_interger,
+	 * the ec->global_lock will be assigned based on the returned
+	 * value by _GLK. Otherwise it means that there is no _GLK object
+	 * and global lock won't be required when EC is accessed.
+	 * If the return value of _GLK object is not zero, it means that
+	 * global lock will be required when EC is accessed.
+	 * If the return value of _GLK object is zero, it means that
+	 * global lock won't be required when EC is accessed.
+	 */
+	status = acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (ACPI_SUCCESS(status)) {
+		/*
+		 * If the return value is not zero, it means that global lock
+		 * is required when EC is accessed
+		 */
+		if (tmp)
+			ec->global_lock = 1;
+		else
+			ec->global_lock = 0;
+	} else
+		ec->global_lock = 0;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
 }

Quoted 1 line · Alexey Starikovskiy

NAK

Can you elaborate, a bit, please?

Quoted 60 lines

Quoted 58 lines · Zhao Yakui

Subject: ACPI: Cleanup :Initialize EC global lock based on the return status
From: Zhao Yakui <[email protected]>

Initialize the EC global lock based on the returned value of _GLK object. Only when AE_OK is returned by the acpi_evaluate_integer, the EC global lock will be assigned based on the return value of _GLK object.Otherwise it means that there is no _GLK object and the global lock won't be required when EC is accessed.
If the return value of _GLK object is not zero, it means that GLobal lock will be required when EC is accessed.
If the return value of _GLK object is zero, it means that GLobal lock won't be required when EC is accessed.

Signed-off-by Zhao Yakui <[email protected]>

cc Alexey Starikovskiy <[email protected]>

cc Alan Jenkins <[email protected]>

---
 drivers/acpi/ec.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================

drivers/acpi/ec.c +22 -2

@@ -752,8 +752,28 @@ ec_parse_device(acpi_handle handle, u32 
 	ec->gpe = tmp;
 	/* Use the global lock for all EC transactions? */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
-	ec->global_lock = tmp;
+	/*
+	 * Only when AE_OK is returned by acpi_evaluate_interger,
+	 * the ec->global_lock will be assigned based on the returned
+	 * value by _GLK. Otherwise it means that there is no _GLK object
+	 * and global lock won't be required when EC is accessed.
+	 * If the return value of _GLK object is not zero, it means that
+	 * global lock will be required when EC is accessed.
+	 * If the return value of _GLK object is zero, it means that
+	 * global lock won't be required when EC is accessed.
+	 */
+	status = acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (ACPI_SUCCESS(status)) {
+		/*
+		 * If the return value is not zero, it means that global lock
+		 * is required when EC is accessed
+		 */
+		if (tmp)
+			ec->global_lock = 1;
+		else
+			ec->global_lock = 0;
+	} else
+		ec->global_lock = 0;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
 }

NAK

commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]>
Date: Fri Oct 10 02:22:59 2008 -0400
ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit

Quoted 58 lines · Zhao Yakui

Subject: ACPI: Cleanup :Initialize EC global lock based on the return status
From: Zhao Yakui <[email protected]>

Initialize the EC global lock based on the returned value of _GLK object. Only when AE_OK is returned by the acpi_evaluate_integer, the EC global lock will be assigned based on the return value of _GLK object.Otherwise it means that there is no _GLK object and the global lock won't be required when EC is accessed.
If the return value of _GLK object is not zero, it means that GLobal lock will be required when EC is accessed.
If the return value of _GLK object is zero, it means that GLobal lock won't be required when EC is accessed.

Signed-off-by Zhao Yakui <[email protected]>

cc Alexey Starikovskiy <[email protected]>

cc Alan Jenkins <[email protected]>

---
 drivers/acpi/ec.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

Index: linux-2.6/drivers/acpi/ec.c
===================================================================

drivers/acpi/ec.c +22 -2

@@ -752,8 +752,28 @@ ec_parse_device(acpi_handle handle, u32 
 	ec->gpe = tmp;
 	/* Use the global lock for all EC transactions? */
 	tmp = 0;
-	acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
-	ec->global_lock = tmp;
+	/*
+	 * Only when AE_OK is returned by acpi_evaluate_interger,
+	 * the ec->global_lock will be assigned based on the returned
+	 * value by _GLK. Otherwise it means that there is no _GLK object
+	 * and global lock won't be required when EC is accessed.
+	 * If the return value of _GLK object is not zero, it means that
+	 * global lock will be required when EC is accessed.
+	 * If the return value of _GLK object is zero, it means that
+	 * global lock won't be required when EC is accessed.
+	 */
+	status = acpi_evaluate_integer(handle, "_GLK", NULL, &tmp);
+	if (ACPI_SUCCESS(status)) {
+		/*
+		 * If the return value is not zero, it means that global lock
+		 * is required when EC is accessed
+		 */
+		if (tmp)
+			ec->global_lock = 1;
+		else
+			ec->global_lock = 0;
+	} else
+		ec->global_lock = 0;
 	ec->handle = handle;
 	return AE_CTRL_TERMINATE;
 }

Quoted 1 line · Alexey Starikovskiy

NAK

Will you please describe the detailed reason?

In the bug 11917 the regression is related with the following commit:

Quoted 4 lines

commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]>
Date: Fri Oct 10 02:22:59 2008 -0400
ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit

kernels

But IMO the main reason is that EC driver misuses the Linux-ACPI
utility interface.(acpi_evaluate_integer).

It will be better to determine whether the return value of ACPI
object is effective according to the return status. In such case the code still can work well even after the Linux-ACPI utility interface is changed again.

Quoted 11 lines · Zhao Yakui

Quoted 1 line · Alexey Starikovskiy

NAK

Will you please describe the detailed reason?

In the bug 11917 the regression is related with the following commit:

Quoted 4 lines

commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]>
Date: Fri Oct 10 02:22:59 2008 -0400
ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit

kernels

Quoted 2 lines

But IMO the main reason is that EC driver misuses the Linux-ACPI
utility interface.(acpi_evaluate_integer).

Did you _read_ the interface specification?
It explicitly states that if any function call does not succeed it will not change the data passed to it.
So it is again only your not-so-humble opinion.

Quoted 4 lines

It will be better to determine whether the return value of ACPI
object is effective according to the return status. In such case the code still can work well even after the Linux-ACPI utility interface is changed again.

Code works fine until someone tries to optimize it...

Regards,
Alex.

Quoted 18 lines · Alexey Starikovskiy

Quoted 16 lines · Zhao Yakui

Quoted 2 lines · Alexey Starikovskiy

NAK

Will you please describe the detailed reason?

In the bug 11917 the regression is related with the following commit:

Quoted 4 lines

commit 27663c5855b10af9ec67bc7dfba001426ba21222
Author: Matthew Wilcox <[email protected]>
Date: Fri Oct 10 02:22:59 2008 -0400
ACPI: Change acpi_evaluate_integer to support 64-bit on 32-bit

kernels

But IMO the main reason is that EC driver misuses the Linux-ACPI
utility interface.(acpi_evaluate_integer).

Quoted 5 lines

Did you _read_ the interface specification?
It explicitly states that if any function call does not succeed it will not change the data passed to it.
So it is again only your not-so-humble opinion.

What do you mean "not-so-humble"?

Quoted 7 lines

Quoted 7 lines

It will be better to determine whether the return value of ACPI
object is effective according to the return status. In such case the code still can work well even after the Linux-ACPI utility interface is changed again.

Quoted 2 lines

Code works fine until someone tries to optimize it...

I agree that your patch can work well. But it depends on the
internal realization of Linux-ACPI utility interface. If Linux-ACPI utility interface is changed, maybe it will be broken. If so, why not to determine whether the return value is effective based on the return status of Linux-ACPI utility?

Quoted 6 lines

Regards,
Alex.

From: Zhao Yakui <[email protected]>

On some boxes there exist both RSDT and XSDT table. But unfortunately sometimes there exists the following error when XSDT table is used:
a. 32/64X address mismatch
b. The 32/64X FACS address mismatch

In such case the boot option of "acpi=rsdt" is provided so that
RSDT is tried instead of XSDT table when the system can't work well.

http://bugzilla.kernel.org/show_bug.cgi?id=8246

Signed-off-by Zhao Yakui <[email protected]>

cc Thomas Renninger <[email protected]>

---
 Documentation/kernel-parameters.txt |    2 ++
 arch/ia64/kernel/acpi.c             |    1 +
 arch/x86/kernel/acpi/boot.c         |    6 +++++-
 drivers/acpi/tables/tbutils.c       |    3 ++-
 include/acpi/acpixf.h               |    1 +
 5 files changed, 11 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================

arch/x86/kernel/acpi/boot.c +5 -1

@@ -47,7 +47,7 @@
 #endif
 
 static int __initdata acpi_force = 0;
-
+u32 acpi_rsdt_forced;
 #ifdef	CONFIG_ACPI
 int acpi_disabled = 0;
 #else

@@ -1783,6 +1783,10 @@ static int __init parse_acpi(char *arg)
 			disable_acpi();
 		acpi_ht = 1;
 	}
+	/* acpi=rsdt use RSDT instead of XSDT */
+	else if (strcmp(arg, "rsdt") == 0) {
+		acpi_rsdt_forced = 1;
+	}
 	/* "acpi=noirq" disables ACPI interrupt routing */
 	else if (strcmp(arg, "noirq") == 0) {
 		acpi_noirq_set();

Index: linux-2.6/include/acpi/acpixf.h
===================================================================

include/acpi/acpixf.h +1 -0

@@ -48,6 +48,7 @@
 #include "actypes.h"
 #include "actbl.h"
 
+extern u32 acpi_rsdt_forced;
 /*
  * Global interfaces
  */

Index: linux-2.6/drivers/acpi/tables/tbutils.c
===================================================================

drivers/acpi/tables/tbutils.c +2 -1

@@ -420,7 +420,8 @@ acpi_tb_parse_root_table(acpi_physical_a
 
 	/* Differentiate between RSDT and XSDT root tables */
 
-	if (rsdp->revision > 1 && rsdp->xsdt_physical_address) {
+	if (rsdp->revision > 1 && rsdp->xsdt_physical_address
+			&& !acpi_rsdt_forced) {
 		/*
 		 * Root table is an XSDT (64-bit physical addresses). We must use the
 		 * XSDT if the revision is > 1 and the XSDT pointer is present, as per

Index: linux-2.6/arch/ia64/kernel/acpi.c
===================================================================

arch/ia64/kernel/acpi.c +1 -0

@@ -65,6 +65,7 @@ EXPORT_SYMBOL(pm_idle);
 void (*pm_power_off) (void);
 EXPORT_SYMBOL(pm_power_off);
 
+u32 acpi_rsdt_forced;
 unsigned int acpi_cpei_override;
 unsigned int acpi_cpei_phys_cpuid;
 

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================

Documentation/kernel-parameters.txt +2 -1

@@ -139,6 +139,8 @@ and is between 256 and 4096 characters. 
 			ht -- run only enough ACPI to enable Hyper Threading
 			strict -- Be less tolerant of platforms that are not
 				strictly ACPI specification compliant.
+			rsdt -- RSDT is used instead of XSDT table when both
+				exits.
 
 			See also Documentation/power/pm.txt, pci=noacpi
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

applied

--
Len Brown, Intel Open Source Technology Center

Quoted 9 lines · Zhao Yakui

From: Zhao Yakui <[email protected]>

On some boxes there exist both RSDT and XSDT table. But unfortunately sometimes there exists the following error when XSDT table is used:
a. 32/64X address mismatch
b. The 32/64X FACS address mismatch

In such case the boot option of "acpi=rsdt" is provided so that
RSDT is tried instead of XSDT table when the system can't work well.

Quoted 94 lines

http://bugzilla.kernel.org/show_bug.cgi?id=8246

Signed-off-by Zhao Yakui <[email protected]>

cc Thomas Renninger <[email protected]>

---
 Documentation/kernel-parameters.txt |    2 ++
 arch/ia64/kernel/acpi.c             |    1 +
 arch/x86/kernel/acpi/boot.c         |    6 +++++-
 drivers/acpi/tables/tbutils.c       |    3 ++-
 include/acpi/acpixf.h               |    1 +
 5 files changed, 11 insertions(+), 2 deletions(-)

Index: linux-2.6/arch/x86/kernel/acpi/boot.c
===================================================================

arch/x86/kernel/acpi/boot.c +5 -1

@@ -47,7 +47,7 @@
 #endif
 
 static int __initdata acpi_force = 0;
-
+u32 acpi_rsdt_forced;
 #ifdef	CONFIG_ACPI
 int acpi_disabled = 0;
 #else

@@ -1783,6 +1783,10 @@ static int __init parse_acpi(char *arg)
 			disable_acpi();
 		acpi_ht = 1;
 	}
+	/* acpi=rsdt use RSDT instead of XSDT */
+	else if (strcmp(arg, "rsdt") == 0) {
+		acpi_rsdt_forced = 1;
+	}
 	/* "acpi=noirq" disables ACPI interrupt routing */
 	else if (strcmp(arg, "noirq") == 0) {
 		acpi_noirq_set();

Index: linux-2.6/include/acpi/acpixf.h
===================================================================

include/acpi/acpixf.h +1 -0

@@ -48,6 +48,7 @@
 #include "actypes.h"
 #include "actbl.h"
 
+extern u32 acpi_rsdt_forced;
 /*
  * Global interfaces
  */

Index: linux-2.6/drivers/acpi/tables/tbutils.c
===================================================================

drivers/acpi/tables/tbutils.c +2 -1

@@ -420,7 +420,8 @@ acpi_tb_parse_root_table(acpi_physical_a
 
 	/* Differentiate between RSDT and XSDT root tables */
 
-	if (rsdp->revision > 1 && rsdp->xsdt_physical_address) {
+	if (rsdp->revision > 1 && rsdp->xsdt_physical_address
+			&& !acpi_rsdt_forced) {
 		/*
 		 * Root table is an XSDT (64-bit physical addresses). We must use the
 		 * XSDT if the revision is > 1 and the XSDT pointer is present, as per

Index: linux-2.6/arch/ia64/kernel/acpi.c
===================================================================

arch/ia64/kernel/acpi.c +1 -0

@@ -65,6 +65,7 @@ EXPORT_SYMBOL(pm_idle);
 void (*pm_power_off) (void);
 EXPORT_SYMBOL(pm_power_off);
 
+u32 acpi_rsdt_forced;
 unsigned int acpi_cpei_override;
 unsigned int acpi_cpei_phys_cpuid;
 

Index: linux-2.6/Documentation/kernel-parameters.txt
===================================================================

Documentation/kernel-parameters.txt +2 -1

@@ -139,6 +139,8 @@ and is between 256 and 4096 characters.
 			ht -- run only enough ACPI to enable Hyper Threading
 			strict -- Be less tolerant of platforms that are not
 				strictly ACPI specification compliant.
+			rsdt -- RSDT is used instead of XSDT table when both
+				exits.
 
 			See also Documentation/power/pm.txt, pci=noacpi
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines

Quoted 4 lines

In such case the boot option of "acpi=rsdt" is provided so that
RSDT is tried instead of XSDT table when the system can't work well.

Great, now we still need a dmi blacklist with machines which are known broken. Then we are at the patchset I posted about a year ago -> scnr.

I am going to post the missing two patches to make R40e and R50e work.

Quoted 6 lines

Quoted 2 lines

Quoted 2 lines

In such case the boot option of "acpi=rsdt" is provided so that
RSDT is tried instead of XSDT table when the system can't work well.

Great, now we still need a dmi blacklist with machines which are known broken. Then we are at the patchset I posted about a year ago -> scnr.

Quoted 1 line

I am going to post the missing two patches to make R40e and R50e work.

please do not.

this option is just for manual debugging.
as i wrote when i nacked your original series,
the object should be to figure out which path to take w/o using dmi.

IMO this cannot generally be done, because chances are high that machines which do not support Windows likely will break.
Chances are high that a machine which does not support Windows uses 64 bit addresses and leaves the 32 bit ones uninitialized, a spec valid configuration which will then break.

Quoted 5 lines · Thomas Renninger

IMO this cannot generally be done, because chances are high that machines which do not support Windows likely will break.
Chances are high that a machine which does not support Windows uses 64 bit addresses and leaves the 32 bit ones uninitialized, a spec valid configuration which will then break.

Bear in mind that the values in the 32-bit entries are *io port* addresses, not physical memory addresses. There's only 16 bits of io ports, so the probability of the 64-bit values being programmed correctly and the 32-bit ones containing a valid but not-working set is tiny.

If you know of any machines that behave this way, I'd be impressed - and it'll be far easier to dmi whitelist them than the other way around.

Quoted 3 lines · On Friday 09 January 2009 13:34:16 Matthew Garrett wrote:

If you know of any machines that behave this way, I'd be impressed - and it'll be far easier to dmi whitelist them than the other way around.

What do you mean with "easy"?

99.998% of all machines work fine with the current, spec conform implementation.
There are the two ThinkPads for which the vendor admitted that the BIOS is broken which work better with the rsdt blacklisted.

Quoted 5 lines · Thomas Renninger

Quoted 3 lines · On Friday 09 January 2009 13:34:16 Matthew Garrett wrote:

If you know of any machines that behave this way, I'd be impressed - and it'll be far easier to dmi whitelist them than the other way around.

Quoted 6 lines

What do you mean with "easy"?

99.998% of all machines work fine with the current, spec conform implementation.
There are the two ThinkPads for which the vendor admitted that the BIOS is broken which work better with the rsdt blacklisted.

Two Thinkpads, an entire range of HP workstations, how many others?

We have no idea which bugs are being triggered by this. Refusing to fix it because we'll potentially break some machines that may not even exist is not a sensible plan.

Quoted 6 lines · Matthew Garrett

Quoted 4 lines · Thomas Renninger

99.998% of all machines work fine with the current, spec conform implementation.
There are the two ThinkPads for which the vendor admitted that the BIOS is broken which work better with the rsdt blacklisted.

Quoted 1 line

Two Thinkpads, an entire range of HP workstations, how many others?

You wanted to provide more info about the HPs. Are they even sold? Have you informed HP that their BIOSes are broken?
It's easy and safe (also in respect of not breaking Windows) for them to fix them. If we do not wait some more months, I am confident they are going to do it, tell me privately if you need contacts.

Quoted 3 lines

We have no idea which bugs are being triggered by this. Refusing to fix it because we'll potentially break some machines that may not even exist is not a sensible plan.

The plan is to stay to the spec. Is that so wrong?

There are netbooks which only support Linux and I hope there will be a lot more machines only supporting Linux as OS in the future.
Such Windows compatibility, Spec violating fixes will break them. It's the broken BIOSes, those machines the vendor never ever tried booting Linux which need the boot param and blacklist adding...

Maybe we come away with this change this time, maybe we hurt someone badly who did everything right: stick to the spec, tested and verified that Linux works fine.

{

Subject: ACPI: Add the MWAIT C-state mask to avoid overflow
From: Zhao Yakui <[email protected]>

The Cx Register address obtained from the _CST object is used as the mait hints if the register type is FFixedHW. And it is used to check whether the Cx type is supported or not.

On some boxes the following Cx state package is obtained from _CST object:

Quoted 1 line

{

                ResourceTemplate ()
                {
                    Register (FFixedHW,
                        0x01,               // Bit Width
                        0x02,               // Bit Offset
                        0x0000000000889759, // Address
                        0x03,               // Access Size
                        )
                },

                0x03,
                0xF5,
                0x015E }

In such case we should use the bit[7:4] of Cx address to check whether
the Cx type is supported or not.

Add the MWAIT C-state mask to avoid overflow

Signed-off-by Zhao Yakui <[email protected]>

Acked-by Venki Pallipadi <[email protected]>

---
 arch/x86/kernel/acpi/cstate.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/acpi/cstate.c
===================================================================

arch/x86/kernel/acpi/cstate.c +3 -2

@@ -56,6 +56,7 @@ static struct cstate_entry *cpu_cstate_e
 static short mwait_supported[ACPI_PROCESSOR_MAX_POWER];
 
 #define MWAIT_SUBSTATE_MASK	(0xf)
+#define MWAIT_CSTATE_MASK	(0xf)
 #define MWAIT_SUBSTATE_SIZE	(4)
 
 #define CPUID_MWAIT_LEAF (5)

@@ -98,7 +99,8 @@ int acpi_processor_ffh_cstate_probe(unsi
 	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
 
 	/* Check whether this particular cx_type (in CST) is supported or not */
-	cstate_type = (cx->address >> MWAIT_SUBSTATE_SIZE) + 1;
+	cstate_type = ((cx->address >> MWAIT_SUBSTATE_SIZE) &
+			MWAIT_CSTATE_MASK) + 1;
 	edx_part = edx >> (cstate_type * MWAIT_SUBSTATE_SIZE);
 	num_cstate_subtype = edx_part & MWAIT_SUBSTATE_MASK;
 
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Subject: ACPI: Add the MWAIT C-state mask to avoid overflow
From: Zhao Yakui <[email protected]>

The Cx Register address obtained from the _CST object is used as the mait hints if the register type is FFixedHW. And it is used to check whether the Cx type is supported or not.

On some boxes the following Cx state package is obtained from _CST object:

Quoted 1 line

{

                ResourceTemplate ()
                {
                    Register (FFixedHW,
                        0x01,               // Bit Width
                        0x02,               // Bit Offset
                        0x0000000000889759, // Address
                        0x03,               // Access Size
                        )
                },

                0x03,
                0xF5,
                0x015E }

In such case we should use the bit[7:4] of Cx address to check whether
the Cx type is supported or not.

Add the MWAIT C-state mask to avoid overflow

Signed-off-by Zhao Yakui <[email protected]>

Acked-by Venki Pallipadi <[email protected]>

---
 arch/x86/kernel/acpi/cstate.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/kernel/acpi/cstate.c
===================================================================

arch/x86/kernel/acpi/cstate.c +3 -1

@@ -56,6 +56,7 @@ static struct cstate_entry *cpu_cstate_e
 static short mwait_supported[ACPI_PROCESSOR_MAX_POWER];
 
 #define MWAIT_SUBSTATE_MASK	(0xf)
+#define MWAIT_CSTATE_MASK	(0xf)
 #define MWAIT_SUBSTATE_SIZE	(4)
 
 #define CPUID_MWAIT_LEAF (5)

@@ -98,7 +99,8 @@ int acpi_processor_ffh_cstate_probe(unsi
 	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
 
 	/* Check whether this particular cx_type (in CST) is supported or not */
-	cstate_type = (cx->address >> MWAIT_SUBSTATE_SIZE) + 1;
+	cstate_type = ((cx->address >> MWAIT_SUBSTATE_SIZE) &
+			MWAIT_CSTATE_MASK) + 1;
 	edx_part = edx >> (cstate_type * MWAIT_SUBSTATE_SIZE);
 	num_cstate_subtype = edx_part & MWAIT_SUBSTATE_MASK;
 

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -26

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

Quoted 10 lines · Zhao Yakui

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

I haven't looked at the patch in detail, but what I would do would be to use the ACPI PM timer by default and _if_ the results from that are not reasonable, _then_ fall back to the clocksource. Is this what you have implemented?

Rafael

Quoted 196 lines

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +41 -25

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

drivers/acpi/processor_idle.c static int acpi_idle_enter_bm(struct cpu · line 1694

Quoted 8 lines

drivers/acpi/processor_idle.c +1 -1

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 12 lines · Rafael J. Wysocki

Quoted 10 lines · Zhao Yakui

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Quoted 3 lines

I haven't looked at the patch in detail, but what I would do would be to use the ACPI PM timer by default and _if_ the results from that are not reasonable, _then_ fall back to the clocksource. Is this what you have implemented?

What you said is also OK. But it is not easy to detect whether the result by using ACPI PM timer is not reasonable. If doing so, another clocksource(for example: hpet) should also be used in cpuidle driver and compared with ACPI PM timer. (Sometimes the ACPI PM timer will be used as the current clocksource. In such case it will be duplicated).

So the current clocksource will be used to count the C-state time instead of ACPI PM timer. If the current clocksource is changed by user, OS will select the reliable clocksource as the current one.

Quoted 10 lines · Zhao Yakui

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

I haven't looked at the patch in detail, but what I would do would be to use the ACPI PM timer by default and _if_ the results from that are not reasonable, _then_ fall back to the clocksource. Is this what you have implemented?

What you said is also OK. But it is not easy to detect whether the

Rafael

Quoted 196 lines

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +41 -25

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

drivers/acpi/processor_idle.c static int acpi_idle_enter_bm(struct cpu · line 1694

Quoted 8 lines

drivers/acpi/processor_idle.c +1 -1

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -26

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

Patch looks ok in general. However, I am wonderng whether we reall need ktime_get() here or can we use getnstimeofday() directly. Using

getnstimeofday() should be cheaper, especially if we are doing frequent calls from multiple CPUs on every idle enter/exit. And I don't think we need to worry about monotonic clock from ktime here...

Thanks,
Venki

Quoted 215 lines · Yakui

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -26

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

Patch looks ok in general. However, I am wonderng whether we reall need ktime_get() here or can we use getnstimeofday() directly. Using getnstimeofday() should be cheaper, especially if we are doing frequent calls from multiple CPUs on every idle enter/exit. And I don't think we need to worry about monotonic clock from ktime here...

Thanks,
Venki

Quoted 215 lines · Yakui

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -26

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Based on the Venki's suggestion the function of ktime_get_real is used as it is unnecessary to use the monotonic time.

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by Venkatesh Pallipadi <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -27

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get_real();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get_real();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get_real();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get_real();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {
 
 
--

To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Subject: ACPI: Use clocksource to get the C-state time instead of ACPI PM timer
From: Zhao Yakui <[email protected]>

On most boxes the ACPI PM timer is 24-bit counter that runs on 3.579545MHz
clock. In such case the max C-state sleep time should be less than 4687ms when it is used to record C2/C3 duration time.
But on some boxes the max C-state sleep time is more than 4687ms. In such
case the overflow happens and the C-state duration time can't be counted accurately.
Use clocksource to get the C-state time instead of ACPI PM timer

Based on the Venki's suggestion the function of ktime_get_real is used as it is unnecessary to use the monotonic time.

Signed-off-by Yakui Zhao <[email protected]>

Signed-off-by Alexs Shi <[email protected]>

Signed-off-by Shaohua Li <[email protected]>

Signed-off-by Venkatesh Pallipadi <[email protected]>

---
 drivers/acpi/processor_idle.c |   68 +++++++++++++++++++++++++-----------------
 1 file changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/acpi/processor_idle.c
===================================================================

drivers/acpi/processor_idle.c +42 -26

@@ -397,8 +397,8 @@ static void acpi_processor_idle(void)
 	struct acpi_processor_cx *cx = NULL;
 	struct acpi_processor_cx *next_state = NULL;
 	int sleep_ticks = 0;
-	u32 t1, t2 = 0;
-
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	/*
 	 * Interrupts must be disabled during bus mastering calculations and
 	 * for C2/C3 transitions.

@@ -543,23 +543,26 @@ static void acpi_processor_idle(void)
 		break;
 
 	case ACPI_STATE_C2:
-		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get_real();
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		/* Invoke C2 */
 		acpi_state_timer_broadcast(pr, cx, 1);
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
-
+		kt2 = ktime_get_real();
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 		/* TSC halts in C2, so notify users */
 		if (tsc_halts_in_c(ACPI_STATE_C2))
 			mark_tsc_unstable("possible TSC halt in C2");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval and
+		 * then convert it to microsecond
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -605,13 +608,13 @@ static void acpi_processor_idle(void)
 		}
 
 		/* Get start time (ticks) */
-		t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt1 = ktime_get_real();
 		/* Invoke C3 */
 		/* Tell the scheduler that we are going deep-idle: */
 		sched_clock_idle_sleep_event();
 		acpi_cstate_enter(cx);
 		/* Get end time (ticks) */
-		t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+		kt2 = ktime_get_real();
 		if (pr->flags.bm_check && pr->flags.bm_control) {
 			/* Enable bus master arbitration */
 			atomic_dec(&c3_cpu_count);

@@ -623,8 +626,13 @@ static void acpi_processor_idle(void)
 		if (tsc_halts_in_c(ACPI_STATE_C3))
 			mark_tsc_unstable("TSC halts in C3");
 #endif
+		/*
+		 * Use the function of ktime_sub to get the time interval
+		 * and convert it to microsecond.
+		 */
+		sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
 		/* Compute time (ticks) that we were actually asleep */
-		sleep_ticks = ticks_elapsed(t1, t2);
+		sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 		/* Tell the scheduler how much we idled: */
 		sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1455,7 +1463,8 @@ static inline void acpi_idle_do_entry(st
 static int acpi_idle_enter_c1(struct cpuidle_device *dev,
 			      struct cpuidle_state *state)
 {
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
 

@@ -1476,14 +1485,18 @@ static int acpi_idle_enter_c1(struct cpu
 	if (pr->flags.bm_check)
 		acpi_idle_update_bm_rld(pr, cx);
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 	local_irq_enable();
 	cx->usage++;
-
-	return ticks_elapsed_in_us(t1, t2);
+	/*
+	 * Use the ktime_sub to get the time interval and then convert
+	 * it to microseconds
+	 */
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	return sleep_interval;
 }
 
 /**

@@ -1496,7 +1509,8 @@ static int acpi_idle_enter_simple(struct
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1533,18 +1547,19 @@ static int acpi_idle_enter_simple(struct
 	if (cx->type == ACPI_STATE_C3)
 		ACPI_FLUSH_CPU_CACHE();
 
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	/* Tell the scheduler that we are going deep-idle: */
 	sched_clock_idle_sleep_event();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 #if defined (CONFIG_GENERIC_TIME) && defined (CONFIG_X86)
 	/* TSC could halt in idle, so notify users */
 	if (tsc_halts_in_c(cx->type))
 		mark_tsc_unstable("TSC halts in idle");;
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);

@@ -1556,7 +1571,7 @@ static int acpi_idle_enter_simple(struct
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 static int c3_cpu_count;

@@ -1574,7 +1589,8 @@ static int acpi_idle_enter_bm(struct cpu
 {
 	struct acpi_processor *pr;
 	struct acpi_processor_cx *cx = cpuidle_get_statedata(state);
-	u32 t1, t2;
+	ktime_t  kt1, kt2;
+	u64 sleep_interval;
 	int sleep_ticks = 0;
 
 	pr = __get_cpu_var(processors);

@@ -1643,10 +1659,9 @@ static int acpi_idle_enter_bm(struct cpu
 	} else if (!pr->flags.bm_check) {
 		ACPI_FLUSH_CPU_CACHE();
 	}
-
-	t1 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt1 = ktime_get_real();
 	acpi_idle_do_entry(cx);
-	t2 = inl(acpi_gbl_FADT.xpm_timer_block.address);
+	kt2 = ktime_get_real();
 
 	/* Re-enable bus master arbitration */
 	if (pr->flags.bm_check && pr->flags.bm_control) {

@@ -1661,7 +1676,8 @@ static int acpi_idle_enter_bm(struct cpu
 	if (tsc_halts_in_c(ACPI_STATE_C3))
 		mark_tsc_unstable("TSC halts in idle");
 #endif
-	sleep_ticks = ticks_elapsed(t1, t2);
+	sleep_interval = ktime_to_us(ktime_sub(kt2, kt1));
+	sleep_ticks = US_TO_PM_TIMER_TICKS(sleep_interval);
 	/* Tell the scheduler how much we idled: */
 	sched_clock_idle_wakeup_event(sleep_ticks*PM_TIMER_TICK_NS);
 

@@ -1672,7 +1688,7 @@ static int acpi_idle_enter_bm(struct cpu
 
 	acpi_state_timer_broadcast(pr, cx, 0);
 	cx->time += sleep_ticks;
-	return ticks_elapsed_in_us(t1, t2);
+	return sleep_interval;
 }
 
 struct cpuidle_driver acpi_idle_driver = {