Linux Kernel - [PATCH] PM-Timer: doesn't use workaround if chipset is not buggy

Author

[PATCH] PM-Timer: doesn't use workaround if chipset is not buggy

OGAWA Hirofumi

2006-03-31, 12:25 am

Con Kolivas <kernel@kolivas.org> writes:

> Looks good. Just some minor grammar comments

Thanks a lot for doing it. Fixed.

And finally, I added "if (cur_timer != &timer_pmtmr)" to pmtmr_bug_check()
to avoid incorrect warning.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Current timer_pm.c reads I/O port triple times, in order to avoid the
bug of chipset. But I/O port is slow.

2.6.16 (pmtmr)
Simple gettimeofday: 3.6532 microseconds

2.6.16+patch (pmtmr)
Simple gettimeofday: 1.4582 microseconds

[if chip is buggy, probably it will be 7us or more in 4.2% of probability.]

This patch adds blacklist of buggy chip, and if chip is not buggy,
this uses fast normal version instead of slow workaround version.

If chip is buggy, warnings "pmtmr is slow". But sounds like there is
gray zone. I found the PIIX4 errata, but I couldn't find the ICH4
errata. But some motherboard seems to have problem.

So, if found a ICH4, also warnings, and uses a workaround version. If
user's ICH4 is good actually, user can specify the "pmtmr_good" boot
parameter to use fast version.

Acked-by: John Stultz <johnstul@us.ibm.com>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

arch/i386/kernel/timers/timer_pm.c | 98 +++++++++++++++++++++++++++++++------
1 file changed, 84 insertions(+), 14 deletions(-)

diff -puN arch/i386/kernel/timers/timer_pm.c~pm-kill-workaround arch/i386/kernel/timers/timer_pm.c
--- linux-2.6/arch/i386/kernel/timers/timer_pm.c~pm-kill-workaround 2006-03-23 01:29:42.000000000 +0900
+++ linux-2.6-hirofumi/arch/i386/kernel/timers/timer_pm.c 2006-03-23 01:51:01.000000000 +0900
@@ -15,6 +15,7 @@
#include <linux/module.h>
#include <linux/device.h>
#include <linux/init.h>
+#include <linux/pci.h>
#include <asm/types.h>
#include <asm/timer.h>
#include <asm/smp.h>
@@ -45,24 +46,31 @@ static seqlock_t monotonic_lock = SEQLOC

#define ACPI_PM_MASK 0xFFFFFF /* limit it to 24 bits */

+static int pmtmr_need_workaround __read_mostly = 1;
+
/*helper function to safely read acpi pm timesource*/
static inline u32 read_pmtmr(void)
{
- u32 v1=0,v2=0,v3=0;
- /* It has been reported that because of various broken
- * chipsets (ICH4, PIIX4 and PIIX4E) where the ACPI PM time
- * source is not latched, so you must read it multiple
- * times to insure a safe value is read.
- */
- do {
- v1 = inl(pmtmr_ioport);
- v2 = inl(pmtmr_ioport);
- v3 = inl(pmtmr_ioport);
- } while ((v1 > v2 && v1 < v3) || (v2 > v3 && v2 < v1)
- || (v3 > v1 && v3 < v2));
+ if (pmtmr_need_workaround) {
+ u32 v1, v2, v3;
+
+ /* It has been reported that because of various broken
+ * chipsets (ICH4, PIIX4 and PIIX4E) where the ACPI PM time
+ * source is not latched, so you must read it multiple
+ * times to insure a safe value is read.
+ */
+ do {
+ v1 = inl(pmtmr_ioport);
+ v2 = inl(pmtmr_ioport);
+ v3 = inl(pmtmr_ioport);
+ } while ((v1 > v2 && v1 < v3) || (v2 > v3 && v2 < v1)
+ || (v3 > v1 && v3 < v2));
+
+ /* mask the output to 24 bits */
+ return v2 & ACPI_PM_MASK;
+ }

- /* mask the output to 24 bits */
- return v2 & ACPI_PM_MASK;
+ return inl(pmtmr_ioport) & ACPI_PM_MASK;
}

@@ -263,6 +271,68 @@ struct init_timer_opts __initdata timer_
.opts = &timer_pmtmr,
};

+#ifdef CONFIG_PCI
+/*
+ * 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.
+ */
+static int __init pmtmr_bug_check(void)
+{
+ static struct pci_device_id gray_list[] __initdata = {
+ /* these chipsets may have bug. */
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801DB_0) },
+ { },
+ };
+ struct pci_dev *dev;
+ int pmtmr_has_bug = 0;
+ u8 rev;
+
+ if (cur_timer != &timer_pmtmr || !pmtmr_need_workaround)
+ return 0;
+
+ dev = pci_get_device(PCI_VENDOR_ID_INTEL,
+ PCI_DEVICE_ID_INTEL_82371AB_3, NULL);
+ if (dev) {
+ pci_read_config_byte(dev, PCI_REVISION_ID, &rev);
+ /* the bug has been fixed in PIIX4M */
+ if (rev < 3) {
+ printk(KERN_WARNING
+ "* Found PM-Timer Bug on this chipset. Due to workarounds for a bug,\n"
+ "* this time source is slow. Consider trying other time sources (clock=)\n");
+ pmtmr_has_bug = 1;
+ }
+ pci_dev_put(dev);
+ }
+
+ if (pci_dev_present(gray_list)) {
+ printk(KERN_WARNING
+ "* This chipset may have PM-Timer Bug. Due to workarounds for a bug,\n"
+ "* this time source is slow. If you are sure your timer does not have\n"
+ "* this bug, please use \"pmtmr_good\" to disable the workaround\n");
+ pmtmr_has_bug = 1;
+ }
+
+ if (!pmtmr_has_bug)
+ pmtmr_need_workaround = 0;
+
+ return 0;
+}
+device_initcall(pmtmr_bug_check);
+#endif
+
+static int __init pmtr_good_setup(char *__str)
+{
+ pmtmr_need_workaround = 0;
+ return 0;
+}
+__setup("pmtmr_good", pmtr_good_setup);
+
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Dominik Brodowski <linux@brodo.de>");
MODULE_DESCRIPTION("Power Management Timer (PMTMR) as primary timing source for x86");
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

OGAWA Hirofumi

2006-03-31, 12:25 am

Andrew Morton <akpm@osdl.org> writes:

> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> wrote:
>
> Can this code use the PCI quirk infrastructure?

Yes. However, since we need to check there is _not_ those chipsets,
it's tricky. Probably it's a following or similar code, um.. IMHO it
seems not simple enough. Idea?

static void __devinit blacklist_check()
{
has_bug = 1;
}
DECLARE_PCI_FIXUP_EARLY(blacklist);

static void __devinit graylist_check()
{
has_bug = 2;
}
DECLARE_PCI_FIXUP_EARLY(graylist);

static int __init pmtmr_bug_check()
{
if (!has_bug)
pmtmr_need_workaround = 0;
else
...
}
late_initcall(pmtmr_bug_check);
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Andreas Mohr

2006-03-31, 12:26 am

Hi,

On Thu, Mar 23, 2006 at 03:49:18AM +0900, OGAWA Hirofumi wrote:
> This patch adds blacklist of buggy chip, and if chip is not buggy,
> this uses fast normal version instead of slow workaround version.

Nice!
Testing on ICH5 (P4HT) hasn't shown any problems so far.

> If chip is buggy, warnings "pmtmr is slow". But sounds like there is
> gray zone. I found the PIIX4 errata, but I couldn't find the ICH4
> errata. But some motherboard seems to have problem.

Same here, the ICH4 trace is nowhere to be found.

Should I do a public request for chipset testing?
(I wrote a small test app here that would hopefully identify a buggy
chipset).

Data that I have collected from internet snippets (mostly Intel errata
documents):
Affected (PCI ID / rev):
- ICH4???
- PIIX4 A0 (0x7113 / 00?), A1 (0x7113 / 00?), B0 (0x7113 / 01?)
- PIIX4E A0 (0x7113 / 02?)
Probably fixed (PCI ID / rev):
- PIIX4M A0 (0x7113 / 03?)

My Toshiba Satellite 4280 seems to have non-buggy PIIX4M
(since it's PCI rev. 03), haven't had time to test reliability yet, though.

> So, if found a ICH4, also warnings, and uses a workaround version. If
> user's ICH4 is good actually, user can specify the "pmtmr_good" boot
> parameter to use fast version.

Good, that's how it should be done as long as it's not entirely known which
chipsets (both Intel and possibly also non-Intel!) are buggy.

Also, I think that since triple-reading wastes 3% of system time, it would
be very nice to be able to eventually write a software workaround for buggy
systems, too.
This could perhaps be done by keeping the *old* value in a global yet
thread-safe variable and checking whether the new value is safely related
to it if within short jiffies distance (but since jiffies are probably
calculated from pmtmr readings... :-P).

Andreas Mohr
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

OGAWA Hirofumi

2006-03-31, 12:26 am

Andreas Mohr <andi@rhlx01.fht-esslingen.de> writes:

> Should I do a public request for chipset testing?
> (I wrote a small test app here that would hopefully identify a buggy
> chipset).

I think almost ICH4 is not buggy. But probably current approach is safe.
So, I added "pmtmr_good" to disable the workaround instead.

I posted probably similar one for mainly ICH4 users.
http://marc.theaimsgroup.com/?l=lin...97656924494&w=2

> Data that I have collected from internet snippets (mostly Intel errata
> documents):
> Affected (PCI ID / rev):
> - ICH4???
> - PIIX4 A0 (0x7113 / 00?), A1 (0x7113 / 00?), B0 (0x7113 / 01?)
> - PIIX4E A0 (0x7113 / 02?)
> Probably fixed (PCI ID / rev):
> - PIIX4M A0 (0x7113 / 03?)
>
> My Toshiba Satellite 4280 seems to have non-buggy PIIX4M
> (since it's PCI rev. 03), haven't had time to test reliability yet, though.

I tested PIIX4E (yes, really buggy), ICH7, VT88237. And ICH6 was
reported as sane.

Thanks.
--
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/