ACPI PM-Timer on K6-3 SiS5591: Houston...

ACPI PM-Timer on K6-3 SiS5591: Houston...

[Andreas Mohr]

Hello all,

forgive me for such a blunt description, but:
"bad hardware - PASS, good hardware - FAIL"
That's about what could be used as a summary for my recent observations
about our ACPI PM-Timer usage ;)

With a modified pmtmr_test on my stoneage system, I get the following
sequence (relevant excerpt only):


652419 <==
65241b <===================
65241d <==
65201e
65241f
652020
652421
652022
652423
652024
652425
652429
65202e
652431
652032
652433
652034
652435
652036
652437
652038 <==
65203a <====================
65203c <==
65203e
652040
652042
652044
652046
652048

Result: catastrophic timer behaviour (a large backwards skip is possible),
even in case we do a triple-read workaround, due to a floating bit at
0x0400 (possibly caused by underclocking from 400 to 150, but whatever...).

And my system does pass the bootup PM-Timer check quite often despite
this severe defect (2 in 4 bootups _did_ register my defective
acpi_pm clocksource).

That was the "PASS for broken hardware" part.

Now to the "FAIL for sufficiently-good hardware" part:

I realized that in historic versions (e.g. 2.6.12) read_pmtmr()
encompassed the _entire_ "triple-reading due to latch bug" logic.
Nowadays read_pmtmr() is the raw inline version of a single inl() only!
However despite this large change, the initial hardware check
(at init_acpi_pm_clocksource()) _kept using_ the now single-read read_pmtmr()
as if nothing had happened. Why nobody realized this during review is
beyond me.
The result is that even on latch-bugged systems which are capable to be used
successfully (via the triple-read workaround) the PM-Timer bootup check most
likely can fail quite often now due to now doing buggy single-reads
during initial check
(cannot verify this in real life due to lack of affected P3 hardware here).


Both issues are caused by very weak monotony verification in
init_acpi_pm_clocksource(), thus that init check should get improved
by leaps and bounds instead of stupidly exiting at the very first sign of a
working timer (or maybe even create a generic "verify counter increment" function to be used for all sorts of hardware counters of a configurable counter width?).
I.e. something like
if (timer_ok(timeout_loops, num_checks, counter_width, read_val_func()))
	"everything's ok"

And the triple-read/single-read switch should be moved out of the lateish
clocksource struct registration, to be available during generic read_pmtmr()
already, from the very beginning.

I.e.:
- "known good workaround" systems should provide workaround from the beginning
- initial timer check should then do at least 10 increment checks with
  10 of 10 successful
- if this improved logic fails, then we know that hardware _can_ be considered
  to be broken for use in current kernels
  (and should then be externally analysed using pmtmr_test)

I will try to work on this, but no promises (production system, not too much
time).



Other than this, migrating towards ACPI usage on this rotten system
seems to work just fine, which is quite some feat for Linux I'd think.
Except for the following bootup errors though:


Linux Plug and Play Support v0.97 (c) Adam Belay
pnp: PnP ACPI init
ACPI: bus type pnp registered
ACPI Error (dsopcode-0595): Field [IRQL] at 208 exceeds Buffer [BUF0] size 192 (bi
ts) [20080321]
ACPI Error (psparse-0530): Method parse/execution failed [\_SB_.PCI0.ISA_.FDC0._CR
S] (Node c781e270), AE_AML_BUFFER_LIMIT
ACPI Error (uteval-0233): Method execution failed [\_SB_.PCI0.ISA_.FDC0._CRS] (Nod
e c781e270), AE_AML_BUFFER_LIMIT
pnp 00:08: can't evaluate _CRS: 12300<6>pnp: PnP ACPI: found 13 devices
ACPI: ACPI bus type pnp unregistered



Note (hello Alan!) that this system might be a bug candidate for the recent
libata SiS5513 issue report, but I haven't done the libata migration yet.

# lspci
00:00.0 Host bridge: Silicon Integrated Systems [SiS] 5591/5592 Host (rev 02)
00:00.1 IDE interface: Silicon Integrated Systems [SiS] 5513 [IDE] (rev d0)
00:01.0 ISA bridge: Silicon Integrated Systems [SiS] SiS85C503/5513 (LPC Bridge) (rev 01)
00:01.1 Class ff00: Silicon Integrated Systems [SiS] ACPI
00:01.2 USB Controller: Silicon Integrated Systems [SiS] USB 1.0 Controller (rev 11)
00:02.0 PCI bridge: Silicon Integrated Systems [SiS] Virtual PCI-to-PCI bridge (AGP)

Thanks,

Andreas Mohr

Re: ACPI PM-Timer on K6-3 SiS5591: Houston...

[Dominik Brodowski]

Hi Andreas,

On Sun, Aug 10, 2008 at 12:17:30PM +0200, Andreas Mohr wrote:
> Result: catastrophic timer behaviour (a large backwards skip is possible),
> even in case we do a triple-read workaround, due to a floating bit at
> 0x0400 (possibly caused by underclocking from 400 to 150, but whatever...).

this isn't the bug which is handled by the read-three-times-workaround.
Instead, that handels the following PIIX4 errata:

 * PIIX4 Errata:
 *
 * The power management timer may return improper results when read.
 * Although the timer value settles properly after incrementing,
 * while incrementing there is a 3 ns window every 69.8 ns where the
 * timer value is indeterminate (a 4.2% chance that the data will be
 * incorrect when read). As a result, the ACPI free running count up
 * timer specification is violated due to erroneous reads.

> And my system does pass the bootup PM-Timer check quite often despite
> this severe defect (2 in 4 bootups _did_ register my defective
> acpi_pm clocksource).

No surprise there -- it is the first time I see such an error; and it might
actually be a bug specific to your computer's motherboard. 

> I realized that in historic versions (e.g. 2.6.12) read_pmtmr()
> encompassed the _entire_ "triple-reading due to latch bug" logic.
> Nowadays read_pmtmr() is the raw inline version of a single inl() only!
> However despite this large change, the initial hardware check
> (at init_acpi_pm_clocksource()) _kept using_ the now single-read read_pmtmr()
> as if nothing had happened.

See patch below. Is there a proper format modifier for cycle_t ?

> Both issues are caused by very weak monotony verification in
> init_acpi_pm_clocksource(), thus that init check should get improved
> by leaps and bounds instead of stupidly exiting at the very first sign of a
> working timer (or maybe even create a generic "verify counter increment" function to be used for all sorts of hardware counters of a configurable counter width?).
> I.e. something like
> if (timer_ok(timeout_loops, num_checks, counter_width, read_val_func()))
> 	"everything's ok"

Well, we could do something like this for sure, but I haven't seen any other
such bug report before...

> - "known good workaround" systems should provide workaround from the beginning
=> see patch below.
> - initial timer check should then do at least 10 increment checks with
>   10 of 10 successful
=> might do this, but currently I'm not yet convinced whether we really need
it.

Best,
	Dominik


acpi_pm.c: use proper read function also in errata mode.

When acpi_pm is used in errata mode (three reads instead of one), also the
acpi_pm init functions need to use three reads instead of just one.

Thanks to Andreas Mohr for spotting this issue.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.de>

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 5ca1d80..2c00edd 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -151,13 +151,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_LE,
  */
 static int verify_pmtmr_rate(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned long count, delta;
 
 	mach_prepare_counter();
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read()
 	mach_countup(&count);
-	value2 = read_pmtmr();
+	value2 = clocksource_acpi_pm.read()
 	delta = (value2 - value1) & ACPI_PM_MASK;
 
 	/* Check that the PMTMR delta is within 5% of what we expect */
@@ -177,7 +177,7 @@ static int verify_pmtmr_rate(void)
 
 static int __init init_acpi_pm_clocksource(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned int i;
 
 	if (!pmtmr_ioport)
@@ -187,9 +187,9 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read();
 	for (i = 0; i < 10000; i++) {
-		value2 = read_pmtmr();
+		value2 = clocksource_acpi_pm.read();
 		if (value2 == value1)
 			continue;
 		if (value2 > value1)
@@ -197,11 +197,11 @@ static int __init init_acpi_pm_clocksource(void)
 		if ((value2 < value1) && ((value2) < 0xFFF))
 			goto pm_good;
 		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#x, 0x%#x - aborting.\n", value1, value2);
+			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
 		return -EINVAL;
 	}
 	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#x - aborting.\n", value1);
+			" 0x%#llx - aborting.\n", value1);
 	return -ENODEV;
 
 pm_good:

Re: ACPI PM-Timer on K6-3 SiS5591: Houston...

[Arjan van de Ven]

On Sun, 10 Aug 2008 18:29:20 +0200
Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> Hi Andreas,
> 
> On Sun, Aug 10, 2008 at 12:17:30PM +0200, Andreas Mohr wrote:
> > Result: catastrophic timer behaviour (a large backwards skip is
> > possible), even in case we do a triple-read workaround, due to a
> > floating bit at 0x0400 (possibly caused by underclocking from 400
> > to 150, but whatever...).
> 
> this isn't the bug which is handled by the
> read-three-times-workaround. Instead, that handels the following
> PIIX4 errata:
> 
>  * PIIX4 Errata:
>  *
>  * The power management timer may return improper results when read.
>  * Although the timer value settles properly after incrementing,
>  * while incrementing there is a 3 ns window every 69.8 ns where the
>  * timer value is indeterminate (a 4.2% chance that the data will be
>  * incorrect when read). As a result, the ACPI free running count up
>  * timer specification is violated due to erroneous reads.
> 
> > And my system does pass the bootup PM-Timer check quite often
> > despite this severe defect (2 in 4 bootups _did_ register my
> > defective acpi_pm clocksource).
> 
> No surprise there -- it is the first time I see such an error; and it
> might actually be a bug specific to your computer's motherboard. 
> 

if it's really only one board that's faulty... a dmi blacklist might be
appropriate.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: ACPI PM-Timer on K6-3 SiS5591: Houston...

[Andreas Mohr]

On Sun, Aug 10, 2008 at 06:29:20PM +0200, Dominik Brodowski wrote:
> Hi Andreas,
> 
> On Sun, Aug 10, 2008 at 12:17:30PM +0200, Andreas Mohr wrote:
> > Result: catastrophic timer behaviour (a large backwards skip is possible),
> > even in case we do a triple-read workaround, due to a floating bit at
> > 0x0400 (possibly caused by underclocking from 400 to 150, but whatever...).
> 
> this isn't the bug which is handled by the read-three-times-workaround.
> Instead, that handels the following PIIX4 errata:

OK, right, technically this workaround is not related to this
different bug.
And it's in fact not this triple-read which has any weakness here but rather
the init check.

> > And my system does pass the bootup PM-Timer check quite often despite
> > this severe defect (2 in 4 bootups _did_ register my defective
> > acpi_pm clocksource).
> 
> No surprise there -- it is the first time I see such an error; and it might
> actually be a bug specific to your computer's motherboard. 

Yeah, might be motherboard only, but likely still chipset-global,
since probably not too many people tried this beast with ACPI / acpi_pm even
(we're not even talking about the usual Linux ACPI 2001 blacklist limit
with this board, more like 1999, 1998 or even 1997 stoneage).

OK, dmidecode said:
        Vendor: Award Software International, Inc.
        Version: 4.51 PG
        Release Date: 07/05/99

Might be a generic Award date value in this case, but still quite stoneage.


> > I realized that in historic versions (e.g. 2.6.12) read_pmtmr()
> > encompassed the _entire_ "triple-reading due to latch bug" logic.
> > Nowadays read_pmtmr() is the raw inline version of a single inl() only!
> > However despite this large change, the initial hardware check
> > (at init_acpi_pm_clocksource()) _kept using_ the now single-read read_pmtmr()
> > as if nothing had happened.
> 
> See patch below. Is there a proper format modifier for cycle_t ?

_DAMN_ you're fast! ;)

Technically it's related to the base type of cycle_t (i.e., u64 and thus
probably "unsigned long long"), thus %llx is the format specifier
that I'd have chosen as well.

> Well, we could do something like this for sure, but I haven't seen any other
> such bug report before...

I guess I'm treading on new land here...

> > - "known good workaround" systems should provide workaround from the beginning
> => see patch below.
> > - initial timer check should then do at least 10 increment checks with
> >   10 of 10 successful
> => might do this, but currently I'm not yet convinced whether we really need
> it.

Even if it's not a systematic chipset / layout error, then I'm sure there's always
the occasional custom-broken (read: damaged) system which would need a
useful check to avoid counter-related lockups.

IMHO the current init check is too weak, it will catch the very simplest
types of problems only, and that's not a good thing.

About Arjan's suggestion to use DMI blacklisting here: not the right
method here IMHO since one could easily catch such problems generically
and thus much more reliably than maintaining an ever-growing and thus
always-incomplete blacklist collection.
Anyway, he provided important input still ;)

Andreas Mohr

Re: ACPI PM-Timer on K6-3 SiS5591: Houston...

[Dominik Brodowski]

Hi Andreas,

On Sun, Aug 10, 2008 at 09:08:02PM +0200, Andreas Mohr wrote:
> And it's in fact not this triple-read which has any weakness here but rather
> the init check.

right.

> IMHO the current init check is too weak, it will catch the very simplest
> types of problems only, and that's not a good thing.

what about this?

Best,
	Dominik


acpi_pm.c: check for monotonicity

Expand the check for monotonicity by doing ten tests instead of one. 

Applies on top of "acpi_pm.c: use proper read function also in errata mode."

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.de>

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 2c00edd..f05c4fb 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -178,7 +178,7 @@ static int verify_pmtmr_rate(void)
 static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
-	unsigned int i;
+	unsigned int i, j, good = 0;
 
 	if (!pmtmr_ioport)
 		return -ENODEV;
@@ -187,24 +187,32 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = clocksource_acpi_pm.read();
-	for (i = 0; i < 10000; i++) {
-		value2 = clocksource_acpi_pm.read();
-		if (value2 == value1)
-			continue;
-		if (value2 > value1)
-			goto pm_good;
-		if ((value2 < value1) && ((value2) < 0xFFF))
-			goto pm_good;
-		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
-		return -EINVAL;
+	for (j = 0; j < 10; j++) {
+		value1 = clocksource_acpi_pm.read();
+		for (i = 0; i < 10000; i++) {
+			value2 = clocksource_acpi_pm.read();
+			if (value2 == value1)
+				continue;
+			if (value2 > value1)
+				good++;
+				break;
+			if ((value2 < value1) && ((value2) < 0xFFF))
+				good++;
+				break;
+			printk(KERN_INFO "PM-Timer had inconsistent results:"
+			       " 0x%#llx, 0x%#llx - aborting.\n",
+			       value1, value2);
+			return -EINVAL;
+		}
+		udelay(300 * i);
+	}
+
+	if (good != 10) {
+		printk(KERN_INFO "PM-Timer had no reasonable result:"
+		       " 0x%#llx - aborting.\n", value1);
+		return -ENODEV;
 	}
-	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#llx - aborting.\n", value1);
-	return -ENODEV;
 
-pm_good:
 	if (verify_pmtmr_rate() != 0)
 		return -ENODEV;
 

[git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Dominik Brodowski]

Hi,

anyone willing to take these two patches? Possibly 2.6.27 material; hasn't
been in -next or -mm.

Best,
	Dominik


The following changes since commit 796aadeb1b2db9b5d463946766c5bbfd7717158c:
  Linus Torvalds (1):
        Merge branch 'fixes' of git://git.kernel.org/.../davej/cpufreq

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git clocksource

Dominik Brodowski (2):
      acpi_pm.c: use proper read function also in errata mode.
      acpi_pm.c: check for monotonicity

 drivers/clocksource/acpi_pm.c |   50 +++++++++++++++++++++++-----------------
 1 files changed, 29 insertions(+), 21 deletions(-)

[PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

[Dominik Brodowski]

When acpi_pm is used in errata mode (three reads instead of one), also the
acpi_pm init functions need to use three reads instead of just one.

Thanks to Andreas Mohr for spotting this issue.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 5ca1d80..2c00edd 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -151,13 +151,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_LE,
  */
 static int verify_pmtmr_rate(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned long count, delta;
 
 	mach_prepare_counter();
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read()
 	mach_countup(&count);
-	value2 = read_pmtmr();
+	value2 = clocksource_acpi_pm.read()
 	delta = (value2 - value1) & ACPI_PM_MASK;
 
 	/* Check that the PMTMR delta is within 5% of what we expect */
@@ -177,7 +177,7 @@ static int verify_pmtmr_rate(void)
 
 static int __init init_acpi_pm_clocksource(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned int i;
 
 	if (!pmtmr_ioport)
@@ -187,9 +187,9 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read();
 	for (i = 0; i < 10000; i++) {
-		value2 = read_pmtmr();
+		value2 = clocksource_acpi_pm.read();
 		if (value2 == value1)
 			continue;
 		if (value2 > value1)
@@ -197,11 +197,11 @@ static int __init init_acpi_pm_clocksource(void)
 		if ((value2 < value1) && ((value2) < 0xFFF))
 			goto pm_good;
 		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#x, 0x%#x - aborting.\n", value1, value2);
+			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
 		return -EINVAL;
 	}
 	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#x - aborting.\n", value1);
+			" 0x%#llx - aborting.\n", value1);
 	return -ENODEV;
 
 pm_good:
-- 
1.5.4.3

[PATCH 2/2] acpi_pm.c: check for monotonicity

[Dominik Brodowski]

Expand the check for monotonicity by doing ten tests instead of one.

Applies on top of "acpi_pm.c: use proper read function also in errata mode."

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   42 ++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 2c00edd..f05c4fb 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -178,7 +178,7 @@ static int verify_pmtmr_rate(void)
 static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
-	unsigned int i;
+	unsigned int i, j, good = 0;
 
 	if (!pmtmr_ioport)
 		return -ENODEV;
@@ -187,24 +187,32 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = clocksource_acpi_pm.read();
-	for (i = 0; i < 10000; i++) {
-		value2 = clocksource_acpi_pm.read();
-		if (value2 == value1)
-			continue;
-		if (value2 > value1)
-			goto pm_good;
-		if ((value2 < value1) && ((value2) < 0xFFF))
-			goto pm_good;
-		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
-		return -EINVAL;
+	for (j = 0; j < 10; j++) {
+		value1 = clocksource_acpi_pm.read();
+		for (i = 0; i < 10000; i++) {
+			value2 = clocksource_acpi_pm.read();
+			if (value2 == value1)
+				continue;
+			if (value2 > value1)
+				good++;
+				break;
+			if ((value2 < value1) && ((value2) < 0xFFF))
+				good++;
+				break;
+			printk(KERN_INFO "PM-Timer had inconsistent results:"
+			       " 0x%#llx, 0x%#llx - aborting.\n",
+			       value1, value2);
+			return -EINVAL;
+		}
+		udelay(300 * i);
+	}
+
+	if (good != 10) {
+		printk(KERN_INFO "PM-Timer had no reasonable result:"
+		       " 0x%#llx - aborting.\n", value1);
+		return -ENODEV;
 	}
-	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#llx - aborting.\n", value1);
-	return -ENODEV;
 
-pm_good:
 	if (verify_pmtmr_rate() != 0)
 		return -ENODEV;
 
-- 
1.5.4.3

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Andrew Morton]

On Mon, 18 Aug 2008 21:03:25 +0200
Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> Hi,
> 
> anyone willing to take these two patches? Possibly 2.6.27 material; hasn't
> been in -next or -mm.
> 
> Best,
> 	Dominik
> 
> 
> The following changes since commit 796aadeb1b2db9b5d463946766c5bbfd7717158c:
>   Linus Torvalds (1):
>         Merge branch 'fixes' of git://git.kernel.org/.../davej/cpufreq
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git clocksource
> 
> Dominik Brodowski (2):
>       acpi_pm.c: use proper read function also in errata mode.
>       acpi_pm.c: check for monotonicity
> 
>  drivers/clocksource/acpi_pm.c |   50 +++++++++++++++++++++++-----------------
>  1 files changed, 29 insertions(+), 21 deletions(-)

A bare git URL is somewhat user-unfriendly.

<fiddle, fiddle>

: commit b985f0517e31c1204b5aafb94f86202948f00e16
: Author: Dominik Brodowski <linux@dominikbrodowski.net>
: Date:   Sun Aug 10 21:24:21 2008 +0200
: 
:     acpi_pm.c: use proper read function also in errata mode.
:     
:     When acpi_pm is used in errata mode (three reads instead of one), also the
:     acpi_pm init functions need to use three reads instead of just one.

hm, why?  Was there some observeable problem which this change improved?

:     Thanks to Andreas Mohr for spotting this issue.
:     
:     CC: John Stultz <johnstul@us.ibm.com>
:     CC: Thomas Gleixner <tglx@linutronix.de>
:     CC: Ingo Molnar <mingo@elte.hu>
:     Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
: 
: diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
: index 5ca1d80..2c00edd 100644
: --- a/drivers/clocksource/acpi_pm.c
: +++ b/drivers/clocksource/acpi_pm.c
: @@ -151,13 +151,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_LE,
:   */
:  static int verify_pmtmr_rate(void)
:  {
: -	u32 value1, value2;
: +	cycle_t value1, value2;
:  	unsigned long count, delta;
:  
:  	mach_prepare_counter();
: -	value1 = read_pmtmr();
: +	value1 = clocksource_acpi_pm.read()
:  	mach_countup(&count);
: -	value2 = read_pmtmr();
: +	value2 = clocksource_acpi_pm.read()
:  	delta = (value2 - value1) & ACPI_PM_MASK;
:  
:  	/* Check that the PMTMR delta is within 5% of what we expect */
: @@ -177,7 +177,7 @@ static int verify_pmtmr_rate(void)
:  
:  static int __init init_acpi_pm_clocksource(void)
:  {
: -	u32 value1, value2;
: +	cycle_t value1, value2;
:  	unsigned int i;
:  
:  	if (!pmtmr_ioport)
: @@ -187,9 +187,9 @@ static int __init init_acpi_pm_clocksource(void)
:  						clocksource_acpi_pm.shift);
:  
:  	/* "verify" this timing source: */
: -	value1 = read_pmtmr();
: +	value1 = clocksource_acpi_pm.read();
:  	for (i = 0; i < 10000; i++) {
: -		value2 = read_pmtmr();
: +		value2 = clocksource_acpi_pm.read();
:  		if (value2 == value1)
:  			continue;
:  		if (value2 > value1)
: @@ -197,11 +197,11 @@ static int __init init_acpi_pm_clocksource(void)
:  		if ((value2 < value1) && ((value2) < 0xFFF))
:  			goto pm_good;
:  		printk(KERN_INFO "PM-Timer had inconsistent results:"
: -			" 0x%#x, 0x%#x - aborting.\n", value1, value2);
: +			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
:  		return -EINVAL;
:  	}
:  	printk(KERN_INFO "PM-Timer had no reasonable result:"
: -			" 0x%#x - aborting.\n", value1);
: +			" 0x%#llx - aborting.\n", value1);
:  	return -ENODEV;
:  
:  pm_good:
: 


and

: commit a44299593315e055f28fe96b4767a85c46b2955f
: Author: Dominik Brodowski <linux@dominikbrodowski.net>
: Date:   Sun Aug 10 21:34:54 2008 +0200
: 
:     acpi_pm.c: check for monotonicity
:     
:     Expand the check for monotonicity by doing ten tests instead of one.

Why?

:     Applies on top of "acpi_pm.c: use proper read function also in errata mode."
:     
:     CC: John Stultz <johnstul@us.ibm.com>
:     CC: Thomas Gleixner <tglx@linutronix.de>
:     CC: Ingo Molnar <mingo@elte.hu>
:     Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
: 
: diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
: index 2c00edd..f05c4fb 100644
: --- a/drivers/clocksource/acpi_pm.c
: +++ b/drivers/clocksource/acpi_pm.c
: @@ -178,7 +178,7 @@ static int verify_pmtmr_rate(void)
:  static int __init init_acpi_pm_clocksource(void)
:  {
:  	cycle_t value1, value2;
: -	unsigned int i;
: +	unsigned int i, j, good = 0;
:  
:  	if (!pmtmr_ioport)
:  		return -ENODEV;
: @@ -187,24 +187,32 @@ static int __init init_acpi_pm_clocksource(void)
:  						clocksource_acpi_pm.shift);
:  
:  	/* "verify" this timing source: */
: -	value1 = clocksource_acpi_pm.read();
: -	for (i = 0; i < 10000; i++) {
: -		value2 = clocksource_acpi_pm.read();
: -		if (value2 == value1)
: -			continue;
: -		if (value2 > value1)
: -			goto pm_good;
: -		if ((value2 < value1) && ((value2) < 0xFFF))
: -			goto pm_good;
: -		printk(KERN_INFO "PM-Timer had inconsistent results:"
: -			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
: -		return -EINVAL;
: +	for (j = 0; j < 10; j++) {
: +		value1 = clocksource_acpi_pm.read();
: +		for (i = 0; i < 10000; i++) {
: +			value2 = clocksource_acpi_pm.read();
: +			if (value2 == value1)
: +				continue;
: +			if (value2 > value1)
: +				good++;
: +				break;
: +			if ((value2 < value1) && ((value2) < 0xFFF))
: +				good++;
: +				break;
: +			printk(KERN_INFO "PM-Timer had inconsistent results:"
: +			       " 0x%#llx, 0x%#llx - aborting.\n",
: +			       value1, value2);
: +			return -EINVAL;
: +		}
: +		udelay(300 * i);
: +	}
: +
: +	if (good != 10) {
: +		printk(KERN_INFO "PM-Timer had no reasonable result:"
: +		       " 0x%#llx - aborting.\n", value1);
: +		return -ENODEV;
:  	}
: -	printk(KERN_INFO "PM-Timer had no reasonable result:"
: -			" 0x%#llx - aborting.\n", value1);
: -	return -ENODEV;
:  
: -pm_good:
:  	if (verify_pmtmr_rate() != 0)
:  		return -ENODEV;
:  


I guess this file falls under Thomas's git-hrt tree.  I can queue the
patches up and spam Thomas with them, but I'm at a bit of a loss
regarding their priority due to the above questions.

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Dominik Brodowski]

Hi Andrew,

On Mon, Aug 18, 2008 at 12:19:24PM -0700, Andrew Morton wrote:
> >   git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git clocksource
> > 
> > Dominik Brodowski (2):
> >       acpi_pm.c: use proper read function also in errata mode.
> >       acpi_pm.c: check for monotonicity
> > 
> >  drivers/clocksource/acpi_pm.c |   50 +++++++++++++++++++++++-----------------
> >  1 files changed, 29 insertions(+), 21 deletions(-)
> 
> A bare git URL is somewhat user-unfriendly.

uh, sorry about that.

> : commit b985f0517e31c1204b5aafb94f86202948f00e16
> : Author: Dominik Brodowski <linux@dominikbrodowski.net>
> : Date:   Sun Aug 10 21:24:21 2008 +0200
> : 
> :     acpi_pm.c: use proper read function also in errata mode.
> :     
> :     When acpi_pm is used in errata mode (three reads instead of one), also the
> :     acpi_pm init functions need to use three reads instead of just one.
> 
> hm, why?  Was there some observeable problem which this change improved?

Indeed: on all affected hardware (some Intel ICH4, PIIX4 and PIIX4E chipsets)
there's about a 4.2% chance that initialization of the ACPI PMTMR fails. On
those chipsets, we need to read out the timer value at least three times to
get a correct result, for every once in a while (i.e. within a 3 ns window
every 69.8 ns) the read returns a bogus result. During normal operation we
work around this issue, but during initialization reading a bogus value may
lead to -EINVAL even though the hardware is usable.

> :     acpi_pm.c: check for monotonicity
> :     
> :     Expand the check for monotonicity by doing ten tests instead of one.
> 
> Why?

Indeed: http://lkml.org/lkml/2008/8/10/77 -- quote:
"Result: catastrophic timer behaviour (a large backwards skip is possible),"

The current check for monotonicity is way too weak. And at least on one
system out there PMTMR is unuseable, but the current check fails.

> I guess this file falls under Thomas's git-hrt tree.  I can queue the
> patches up and spam Thomas with them, but I'm at a bit of a loss
> regarding their priority due to the above questions.

That would be great, thanks.

	Dominik

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Andrew Morton]

On Mon, 18 Aug 2008 21:35:17 +0200
Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> Hi Andrew,
> 
> On Mon, Aug 18, 2008 at 12:19:24PM -0700, Andrew Morton wrote:
> > >   git://git.kernel.org/pub/scm/linux/kernel/git/brodo/pcmcia-2.6.git clocksource
> > > 
> > > Dominik Brodowski (2):
> > >       acpi_pm.c: use proper read function also in errata mode.
> > >       acpi_pm.c: check for monotonicity
> > > 
> > >  drivers/clocksource/acpi_pm.c |   50 +++++++++++++++++++++++-----------------
> > >  1 files changed, 29 insertions(+), 21 deletions(-)
> > 
> > A bare git URL is somewhat user-unfriendly.
> 
> uh, sorry about that.
> 
> > : commit b985f0517e31c1204b5aafb94f86202948f00e16
> > : Author: Dominik Brodowski <linux@dominikbrodowski.net>
> > : Date:   Sun Aug 10 21:24:21 2008 +0200
> > : 
> > :     acpi_pm.c: use proper read function also in errata mode.
> > :     
> > :     When acpi_pm is used in errata mode (three reads instead of one), also the
> > :     acpi_pm init functions need to use three reads instead of just one.
> > 
> > hm, why?  Was there some observeable problem which this change improved?
> 
> Indeed: on all affected hardware (some Intel ICH4, PIIX4 and PIIX4E chipsets)
> there's about a 4.2% chance that initialization of the ACPI PMTMR fails. On
> those chipsets, we need to read out the timer value at least three times to
> get a correct result, for every once in a while (i.e. within a 3 ns window
> every 69.8 ns) the read returns a bogus result. During normal operation we
> work around this issue, but during initialization reading a bogus value may
> lead to -EINVAL even though the hardware is usable.

That would be useful stuff for the changelog.

> > :     acpi_pm.c: check for monotonicity
> > :     
> > :     Expand the check for monotonicity by doing ten tests instead of one.
> > 
> > Why?
> 
> Indeed: http://lkml.org/lkml/2008/8/10/77 -- quote:
> "Result: catastrophic timer behaviour (a large backwards skip is possible),"
> 
> The current check for monotonicity is way too weak. And at least on one
> system out there PMTMR is unuseable, but the current check fails.

yeah, that does look like 2.6.27 material.  And possibly 2.6.26.x and
2.6.25.x?

> > I guess this file falls under Thomas's git-hrt tree.  I can queue the
> > patches up and spam Thomas with them, but I'm at a bit of a loss
> > regarding their priority due to the above questions.
> 
> That would be great, thanks.

Could I trouble you to resend them as plain-old-patches, with full
changelogs along with your thoughts about the suitablity for
2.6.2[5678].x please?

I could attempt to put all that together here, but I'd probably end up
with something inappropriate.

Thanks.

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Andreas Mohr]

Hi,

On Mon, Aug 18, 2008 at 09:35:17PM +0200, Dominik Brodowski wrote:
> Hi Andrew,
> 
> On Mon, Aug 18, 2008 at 12:19:24PM -0700, Andrew Morton wrote:
> > :     acpi_pm.c: check for monotonicity
> > :     
> > :     Expand the check for monotonicity by doing ten tests instead of one.
> > 
> > Why?
> 
> Indeed: http://lkml.org/lkml/2008/8/10/77 -- quote:
> "Result: catastrophic timer behaviour (a large backwards skip is possible),"
> 
> The current check for monotonicity is way too weak. And at least on one
> system out there PMTMR is unuseable, but the current check fails.

Just want to say thank you for promptly following up on this issue!

I wanted to test your 2nd patch (improved init), but was unable to do so
(currently too busy due to large changes, plus distance to production system,
downtime NOT appreciated ;).

Andreas Mohr

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Dominik Brodowski]

Hi,

> Could I trouble you to resend them as plain-old-patches, with full
> changelogs along with your thoughts about the suitablity for
> 2.6.2[5678].x please?

patches will be sent as replies to this message. Thanks for taking care of
this; I should have written more meaningful texts. Also, I failed to
remember the second patch hasn't been tested yet ..

On Mon, Aug 18, 2008 at 10:00:17PM +0200, Andreas Mohr wrote:
> I wanted to test your 2nd patch (improved init), but was unable to do so
> (currently too busy due to large changes, plus distance to production system,
> downtime NOT appreciated ;).

.. but still I think this is stuff for 2.6.27; not (yet) suitable for 2.6.26
and 2.6.25 based on the -stable rules.

Best,
	Dominik

[PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

[Dominik Brodowski]

On all hardware (some Intel ICH4, PIIX4 and PIIX4E chipsets) affected by a
hardware errata there's about a 4.2% chance that initialization of the ACPI
PMTMR fails. On those chipsets, we need to read out the timer value at least
three times to get a correct result, for every once in a while (i.e. within
a 3 ns window every 69.8 ns) the read returns a bogus result. During normal
operation we work around this issue, but during initialization reading a
bogus value may lead to -EINVAL even though the hardware is usable.

Thanks to Andreas Mohr for spotting this issue.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 5ca1d80..2c00edd 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -151,13 +151,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_LE,
  */
 static int verify_pmtmr_rate(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned long count, delta;
 
 	mach_prepare_counter();
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read()
 	mach_countup(&count);
-	value2 = read_pmtmr();
+	value2 = clocksource_acpi_pm.read()
 	delta = (value2 - value1) & ACPI_PM_MASK;
 
 	/* Check that the PMTMR delta is within 5% of what we expect */
@@ -177,7 +177,7 @@ static int verify_pmtmr_rate(void)
 
 static int __init init_acpi_pm_clocksource(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned int i;
 
 	if (!pmtmr_ioport)
@@ -187,9 +187,9 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read();
 	for (i = 0; i < 10000; i++) {
-		value2 = read_pmtmr();
+		value2 = clocksource_acpi_pm.read();
 		if (value2 == value1)
 			continue;
 		if (value2 > value1)
@@ -197,11 +197,11 @@ static int __init init_acpi_pm_clocksource(void)
 		if ((value2 < value1) && ((value2) < 0xFFF))
 			goto pm_good;
 		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#x, 0x%#x - aborting.\n", value1, value2);
+			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
 		return -EINVAL;
 	}
 	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#x - aborting.\n", value1);
+			" 0x%#llx - aborting.\n", value1);
 	return -ENODEV;
 
 pm_good:

[PATCH 2/2] acpi_pm.c: check for monotonicity

[Dominik Brodowski]

>From 13cc8b6ff0d8198102f60691c38c07151a693c7b Mon Sep 17 00:00:00 2001
From: Dominik Brodowski <linux@dominikbrodowski.net>
Date: Sun, 10 Aug 2008 21:34:54 +0200
Subject: [PATCH 2/2] acpi_pm.c: check for monotonicity

The current check for monotonicity is way too weak: Andreas Mohr reports
( http://lkml.org/lkml/2008/8/10/77  ) that on one of his test systems the
current check only triggers in 50% of all cases, leading to catastrophic timer
behaviour. To fix this issue, expand the check for monotonicity by doing ten
consecutive tests instead of one.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   42 ++++++++++++++++++++++++----------------
 1 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 2c00edd..f05c4fb 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -178,7 +178,7 @@ static int verify_pmtmr_rate(void)
 static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
-	unsigned int i;
+	unsigned int i, j, good = 0;
 
 	if (!pmtmr_ioport)
 		return -ENODEV;
@@ -187,24 +187,32 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = clocksource_acpi_pm.read();
-	for (i = 0; i < 10000; i++) {
-		value2 = clocksource_acpi_pm.read();
-		if (value2 == value1)
-			continue;
-		if (value2 > value1)
-			goto pm_good;
-		if ((value2 < value1) && ((value2) < 0xFFF))
-			goto pm_good;
-		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
-		return -EINVAL;
+	for (j = 0; j < 10; j++) {
+		value1 = clocksource_acpi_pm.read();
+		for (i = 0; i < 10000; i++) {
+			value2 = clocksource_acpi_pm.read();
+			if (value2 == value1)
+				continue;
+			if (value2 > value1)
+				good++;
+				break;
+			if ((value2 < value1) && ((value2) < 0xFFF))
+				good++;
+				break;
+			printk(KERN_INFO "PM-Timer had inconsistent results:"
+			       " 0x%#llx, 0x%#llx - aborting.\n",
+			       value1, value2);
+			return -EINVAL;
+		}
+		udelay(300 * i);
+	}
+
+	if (good != 10) {
+		printk(KERN_INFO "PM-Timer had no reasonable result:"
+		       " 0x%#llx - aborting.\n", value1);
+		return -ENODEV;
 	}
-	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#llx - aborting.\n", value1);
-	return -ENODEV;
 
-pm_good:
 	if (verify_pmtmr_rate() != 0)
 		return -ENODEV;
 

Re: [PATCH 2/2] acpi_pm.c: check for monotonicity

[Andreas Mohr]

Hi,

On Mon, Aug 18, 2008 at 10:11:15PM +0200, Dominik Brodowski wrote:
> +	if (good != 10) {
> +		printk(KERN_INFO "PM-Timer had no reasonable result:"
> +		       " 0x%#llx - aborting.\n", value1);
> +		return -ENODEV;
>  	}
> -	printk(KERN_INFO "PM-Timer had no reasonable result:"
> -			" 0x%#llx - aborting.\n", value1);
> -	return -ENODEV;

Technically spoken this log message could now be considered partially
outdated... (we're doing 10 evaluations after all, not one with a
precise end result).


Seeing a define for those several open-coded 10 loops values would be nice.

Andreas Mohr

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Andrew Morton]

On Mon, 18 Aug 2008 22:09:16 +0200
Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> Hi,
> 
> > Could I trouble you to resend them as plain-old-patches, with full
> > changelogs along with your thoughts about the suitablity for
> > 2.6.2[5678].x please?
> 
> patches will be sent as replies to this message. Thanks for taking care of
> this; I should have written more meaningful texts. Also, I failed to
> remember the second patch hasn't been tested yet ..
> 
> On Mon, Aug 18, 2008 at 10:00:17PM +0200, Andreas Mohr wrote:
> > I wanted to test your 2nd patch (improved init), but was unable to do so
> > (currently too busy due to large changes, plus distance to production system,
> > downtime NOT appreciated ;).
> 
> .. but still I think this is stuff for 2.6.27; not (yet) suitable for 2.6.26
> and 2.6.25 based on the -stable rules.
> 

(cc stable)

OK, thanks.  I tagged these as

Cc: <stable@kernel.org>         [2.6.26.x, 2.6.25.x but not immediately]

which is a bit rubbery.  What do you think are the criteria for
deciding when these are ready for the backports?  Something like "after
2.6.28-rc1 if nothing blew up"?

Re: [PATCH 2/2] acpi_pm.c: check for monotonicity

[Andrew Morton]

On Mon, 18 Aug 2008 22:18:44 +0200
Andreas Mohr <andi@lisas.de> wrote:

> Hi,
> 
> On Mon, Aug 18, 2008 at 10:11:15PM +0200, Dominik Brodowski wrote:
> > +	if (good != 10) {
> > +		printk(KERN_INFO "PM-Timer had no reasonable result:"
> > +		       " 0x%#llx - aborting.\n", value1);
> > +		return -ENODEV;
> >  	}
> > -	printk(KERN_INFO "PM-Timer had no reasonable result:"
> > -			" 0x%#llx - aborting.\n", value1);
> > -	return -ENODEV;
> 
> Technically spoken this log message could now be considered partially
> outdated... (we're doing 10 evaluations after all, not one with a
> precise end result).
> 
> 
> Seeing a define for those several open-coded 10 loops values would be nice.
> 

Also it's a bit dodgy printing a cycle_t with %llx.  We don't _know_
that cycle_t was implemented with `long long' - if this was always
true, we wouldn't (or shouldn't) have a cycle_t at all.

But it seems that it happens to work for all architectures which
implement acpi.

Re: [git pull?] clocksource: ACPI pmtmr bugfixes [Was: Re: ACPI PM-Timer on K6-3 SiS5591: Houston...]

[Dominik Brodowski]

On Mon, Aug 18, 2008 at 01:25:46PM -0700, Andrew Morton wrote:
> OK, thanks.  I tagged these as
> 
> Cc: <stable@kernel.org>         [2.6.26.x, 2.6.25.x but not immediately]
> 
> which is a bit rubbery.  What do you think are the criteria for
> deciding when these are ready for the backports?  Something like "after
> 2.6.28-rc1 if nothing blew up"?

That sounds perfectly reasonable.

Best,
	Dominik

Re: [PATCH 2/2] acpi_pm.c: check for monotonicity

[Dominik Brodowski]

Hi,

On Mon, Aug 18, 2008 at 01:28:58PM -0700, Andrew Morton wrote:
> > On Mon, Aug 18, 2008 at 10:11:15PM +0200, Dominik Brodowski wrote:
> > > +	if (good != 10) {
> > > +		printk(KERN_INFO "PM-Timer had no reasonable result:"
> > > +		       " 0x%#llx - aborting.\n", value1);
> > > +		return -ENODEV;
> > >  	}
> > > -	printk(KERN_INFO "PM-Timer had no reasonable result:"
> > > -			" 0x%#llx - aborting.\n", value1);
> > > -	return -ENODEV;
> > 
> > Technically spoken this log message could now be considered partially
> > outdated... (we're doing 10 evaluations after all, not one with a
> > precise end result).
> > 
> > 
> > Seeing a define for those several open-coded 10 loops values would be nice.
> > 
> 
> Also it's a bit dodgy printing a cycle_t with %llx.  We don't _know_
> that cycle_t was implemented with `long long' - if this was always
> true, we wouldn't (or shouldn't) have a cycle_t at all.
> 
> But it seems that it happens to work for all architectures which
> implement acpi.

Well, adding a format modifier just for this isn't necessary IMO, especially
as all acpi-implementing architectures seem to be fine. Regarding the other
issues: well, I don't care all that much about a define for something used
twice and this comment; but if you want to squash it into the other patch
I'm fine with it.

Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index f05c4fb..09d4650 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -175,6 +175,9 @@ static int verify_pmtmr_rate(void)
 #define verify_pmtmr_rate() (0)
 #endif
 
+/* Number of monotonicity checks to perform during initialization */
+#define ACPI_PM_MONOTONICITY_CHECKS 10
+
 static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
@@ -187,7 +190,7 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	for (j = 0; j < 10; j++) {
+	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
 		value1 = clocksource_acpi_pm.read();
 		for (i = 0; i < 10000; i++) {
 			value2 = clocksource_acpi_pm.read();
@@ -207,9 +210,9 @@ static int __init init_acpi_pm_clocksource(void)
 		udelay(300 * i);
 	}
 
-	if (good != 10) {
-		printk(KERN_INFO "PM-Timer had no reasonable result:"
-		       " 0x%#llx - aborting.\n", value1);
+	if (good != ACPI_PM_MONOTONICITY_CHECKS) {
+		printk(KERN_INFO "PM-Timer failed consistency check "
+		       " (0x%#llx) - aborting.\n", value1);
 		return -ENODEV;
 	}
 

Re: [PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

[Andrew Morton]

On Mon, 18 Aug 2008 22:10:33 +0200 Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> -	value1 = read_pmtmr();
> +	value1 = clocksource_acpi_pm.read()
>  	mach_countup(&count);
> -	value2 = read_pmtmr();
> +	value2 = clocksource_acpi_pm.read()

compiler got upset.  Did you send the wrong version?

Re: [PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

[Dominik Brodowski]

On Tue, Aug 19, 2008 at 02:43:34AM -0700, Andrew Morton wrote:
> On Mon, 18 Aug 2008 22:10:33 +0200 Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> 
> > -	value1 = read_pmtmr();
> > +	value1 = clocksource_acpi_pm.read()
> >  	mach_countup(&count);
> > -	value2 = read_pmtmr();
> > +	value2 = clocksource_acpi_pm.read()
> 
> compiler got upset.  Did you send the wrong version?

gcc version 4.2.3 (Ubuntu 4.2.3-2ubuntu7) on amd64 worked fine... what did
yours complain about?

Best,
	Dominik

Re: [PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

[Andrew Morton]

On Tue, 19 Aug 2008 11:49:38 +0200 Dominik Brodowski <linux@dominikbrodowski.net> wrote:

> On Tue, Aug 19, 2008 at 02:43:34AM -0700, Andrew Morton wrote:
> > On Mon, 18 Aug 2008 22:10:33 +0200 Dominik Brodowski <linux@dominikbrodowski.net> wrote:
> > 
> > > -	value1 = read_pmtmr();
> > > +	value1 = clocksource_acpi_pm.read()
> > >  	mach_countup(&count);
> > > -	value2 = read_pmtmr();
> > > +	value2 = clocksource_acpi_pm.read()
> > 
> > compiler got upset.  Did you send the wrong version?
> 
> gcc version 4.2.3 (Ubuntu 4.2.3-2ubuntu7) on amd64 worked fine... what did
> yours complain about?
> 

missing semicolons

[PATCH v2 1/2] acpi_pm.c: use proper read function also in errata mode

[Dominik Brodowski]

next try...


From: Dominik Brodowski <linux@dominikbrodowski.net>
Date: Fri, 22 Aug 2008 23:49:21 +0200
Subject: [PATCH 1/2] acpi_pm.c: use proper read function also in errata mode.

On all hardware (some Intel ICH4, PIIX4 and PIIX4E chipsets) affected by a
hardware errata there's about a 4.2% chance that initialization of the ACPI
PMTMR fails. On those chipsets, we need to read out the timer value at least
three times to get a correct result, for every once in a while (i.e. within
a 3 ns window every 69.8 ns) the read returns a bogus result. During normal
operation we work around this issue, but during initialization reading a
bogus value may lead to -EINVAL even though the hardware is usable.

Thanks to Andreas Mohr for spotting this issue.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 5ca1d80..860d033 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -151,13 +151,13 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_LE,
  */
 static int verify_pmtmr_rate(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned long count, delta;
 
 	mach_prepare_counter();
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read();
 	mach_countup(&count);
-	value2 = read_pmtmr();
+	value2 = clocksource_acpi_pm.read();
 	delta = (value2 - value1) & ACPI_PM_MASK;
 
 	/* Check that the PMTMR delta is within 5% of what we expect */
@@ -177,7 +177,7 @@ static int verify_pmtmr_rate(void)
 
 static int __init init_acpi_pm_clocksource(void)
 {
-	u32 value1, value2;
+	cycle_t value1, value2;
 	unsigned int i;
 
 	if (!pmtmr_ioport)
@@ -187,9 +187,9 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = read_pmtmr();
+	value1 = clocksource_acpi_pm.read();
 	for (i = 0; i < 10000; i++) {
-		value2 = read_pmtmr();
+		value2 = clocksource_acpi_pm.read();
 		if (value2 == value1)
 			continue;
 		if (value2 > value1)
@@ -197,11 +197,11 @@ static int __init init_acpi_pm_clocksource(void)
 		if ((value2 < value1) && ((value2) < 0xFFF))
 			goto pm_good;
 		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#x, 0x%#x - aborting.\n", value1, value2);
+			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
 		return -EINVAL;
 	}
 	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#x - aborting.\n", value1);
+			" 0x%#llx - aborting.\n", value1);
 	return -ENODEV;
 
 pm_good:
-- 
1.5.2.5

[PATCH v2 2/2] acpi_pm.c: check for monotonicity

[Dominik Brodowski]

From: Dominik Brodowski <linux@dominikbrodowski.net>
Date: Fri, 22 Aug 2008 23:51:23 +0200
Subject: [PATCH 2/2] acpi_pm.c: check for monotonicity

The current check for monotonicity is way too weak: Andreas Mohr reports
( http://lkml.org/lkml/2008/8/10/77  ) that on one of his test systems the
current check only triggers in 50% of all cases, leading to catastrophic timer
behaviour. To fix this issue, expand the check for monotonicity by doing ten
consecutive tests instead of one.

CC: John Stultz <johnstul@us.ibm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Dominik Brodowski <linux@dominikbrodowski.net>
---
 drivers/clocksource/acpi_pm.c |   46 +++++++++++++++++++++++++---------------
 1 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/drivers/clocksource/acpi_pm.c b/drivers/clocksource/acpi_pm.c
index 860d033..4eee533 100644
--- a/drivers/clocksource/acpi_pm.c
+++ b/drivers/clocksource/acpi_pm.c
@@ -21,6 +21,7 @@
 #include <linux/errno.h>
 #include <linux/init.h>
 #include <linux/pci.h>
+#include <linux/delay.h>
 #include <asm/io.h>
 
 /*
@@ -175,10 +176,13 @@ static int verify_pmtmr_rate(void)
 #define verify_pmtmr_rate() (0)
 #endif
 
+/* Number of monotonicity checks to perform during initialization */
+#define ACPI_PM_MONOTONICITY_CHECKS 10
+
 static int __init init_acpi_pm_clocksource(void)
 {
 	cycle_t value1, value2;
-	unsigned int i;
+	unsigned int i, j, good = 0;
 
 	if (!pmtmr_ioport)
 		return -ENODEV;
@@ -187,24 +191,32 @@ static int __init init_acpi_pm_clocksource(void)
 						clocksource_acpi_pm.shift);
 
 	/* "verify" this timing source: */
-	value1 = clocksource_acpi_pm.read();
-	for (i = 0; i < 10000; i++) {
-		value2 = clocksource_acpi_pm.read();
-		if (value2 == value1)
-			continue;
-		if (value2 > value1)
-			goto pm_good;
-		if ((value2 < value1) && ((value2) < 0xFFF))
-			goto pm_good;
-		printk(KERN_INFO "PM-Timer had inconsistent results:"
-			" 0x%#llx, 0x%#llx - aborting.\n", value1, value2);
-		return -EINVAL;
+	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
+		value1 = clocksource_acpi_pm.read();
+		for (i = 0; i < 10000; i++) {
+			value2 = clocksource_acpi_pm.read();
+			if (value2 == value1)
+				continue;
+			if (value2 > value1)
+				good++;
+				break;
+			if ((value2 < value1) && ((value2) < 0xFFF))
+				good++;
+				break;
+			printk(KERN_INFO "PM-Timer had inconsistent results:"
+			       " 0x%#llx, 0x%#llx - aborting.\n",
+			       value1, value2);
+			return -EINVAL;
+		}
+		udelay(300 * i);
+	}
+
+	if (good != ACPI_PM_MONOTONICITY_CHECKS) {
+		printk(KERN_INFO "PM-Timer failed consistency check "
+		       " (0x%#llx) - aborting.\n", value1);
+		return -ENODEV;
 	}
-	printk(KERN_INFO "PM-Timer had no reasonable result:"
-			" 0x%#llx - aborting.\n", value1);
-	return -ENODEV;
 
-pm_good:
 	if (verify_pmtmr_rate() != 0)
 		return -ENODEV;
 
-- 
1.5.2.5

Re: [PATCH v2 2/2] acpi_pm.c: check for monotonicity

[Jochen Voß]

Hi,

some minor comments:

2008/8/22 Dominik Brodowski <linux@dominikbrodowski.net>:
> +       for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
> +               value1 = clocksource_acpi_pm.read();
> +               for (i = 0; i < 10000; i++) {
> +                       value2 = clocksource_acpi_pm.read();
> +                       if (value2 == value1)
> +                               continue;
> +                       if (value2 > value1)
> +                               good++;
> +                               break;
> +                       if ((value2 < value1) && ((value2) < 0xFFF))
The brackets arout value2 are not needed and look strange.

> +                               good++;
> +                               break;
> +                       printk(KERN_INFO "PM-Timer had inconsistent results:"
> +                              " 0x%#llx, 0x%#llx - aborting.\n",
> +                              value1, value2);
> +                       return -EINVAL;
> +               }
> +               udelay(300 * i);
300*10000 microseconds seems like a long time to me.  Is this the
intended maximal delay?

> +       }
> +
> +       if (good != ACPI_PM_MONOTONICITY_CHECKS) {
> +               printk(KERN_INFO "PM-Timer failed consistency check "
> +                      " (0x%#llx) - aborting.\n", value1);
> +               return -ENODEV;
If the inner loop runs out once, you alreay know that you will later
abort here.  Maybe move the check directly after the inner loop to
avoid the additional delay (10*10000*300 microseconds = 30 seconds) in
case of failure?

I hope this helps,
Jochen
-- 
http://seehuhn.de/