time: fix typo that makes sync_cmos_clock erratic

From: David P. Reed

Fix a typo in ntp.c that has caused updating of the persistent (RTC) clock when synced to NTP to behave erratically.

---
When debugging a freeze that arises on my AMD64 machines when I run the ntpd service, I added a number of printk's to monitor the sync_cmos_clock procedure. I discovered that it was not syncing to cmos RTC every 11 minutes as documented, but instead would keep trying every second for hours at a time. The reason turned out to be a typo in sync_cmos_clock, where it attempts to ensure that update_persistent_clock is called very close to 500 msec. after a 1 second boundary (required by the PC RTC's spec). That typo referred to "xtime" in one spot, rather than "now", which is derived from "xtime" but not equal to it. This makes the test erratic, creating a "coin-flip" that decides when update_persistent_clock is called - when it is called, which is rarely, it may be at any time during the one second period, rather than close to 500 msec, so the value written is needlessly incorrect, too.

I rewrote the code to fix the typo and to be a little more clear about the logic that was intended here. I have thoroughly tested this on several x86_64 machines, and also on one 32-bit machine. I have also submitted a patch to fix the freeze cited above, but both patches are independent, and should be treated separately.

The patch works in 2.6.24rc2, but it should also work in 2.6.23.

Index: linux-2.6/kernel/time/ntp.c
===================================================================
@@ -188,6 +188,7 @@ static DEFINE_TIMER(sync_cmos_timer, syn

static void sync_cmos_clock(unsigned long dummy)

{
    struct timespec now, next;
+    long delta_from_target_time;
    int fail = 1;

    /*
@@ -205,7 +206,10 @@ static void sync_cmos_clock(unsigned lon
        return;

    getnstimeofday(&now);

- if (abs(xtime.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
+ delta_from_target_time = abs(now.tv_nsec - (NSEC_PER_SEC / 2));
+
+ /* set CMOS clock, only if close enough to 500 msec point */
+ if (delta_from_target_time <= tick_nsec / 2)

        fail = update_persistent_clock(now);

    next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec;

From: David P. Reed

Fix two bugs in arch/x86/kernel/time_64.c that affect the x86_64 kernel. 1) a repeatable hard freeze due to interrupts when the ntpd service calls update_persistent_clock(), 2) potentially unstable PC RTC timer values when timer is read.

---
More explanation:
1) A repeatable but randomly timed freeze has been happening in Fedora 6 and 7 for the last year, whenever I run the ntpd service on my AMD64x2 HP Pavilion dv9000z laptop. This freeze is due to the use of spin_lock(&rtc_lock) under the assumption (per a bad comment) that set_rtc_mmss is called only with interrupts disabled. The call from ntp.c to update_persistent_clock is made with interrupts enabled.

2) the use of an incorrect technique for reading the standard PC RTC timer, which is documented to "disconnect" time registers from the bus while updates are in progress. The use of UIP flag while interrupts are disabled to protect a 244 microsecond window is one of the Motorola spec sheet's documented ways to read the RTC time registers reliably. I realize that not all "clones" of the

The patch updates the misleading comments and minimizes the amount of time that the kernel disables interrupts.

I have thoroughly tested this patch on a number of x86_64 machines with various numbers of cores and chipsets, using 2.6.24rc2 kernel source. Note that while testing the ntp code I found another bug in kernel/time/ntp.c which is independent of this fix - neither patch requires the other.

If possible, I'd love to see the patch merged with 2.6.24, and even with 2.6.23.

Index: linux-2.6/arch/x86/kernel/time_64.c
===================================================================
@@ -82,13 +82,12 @@ static int set_rtc_mmss(unsigned long no
    int retval = 0;
    int real_seconds, real_minutes, cmos_minutes;
    unsigned char control, freq_select;
+    unsigned long flags;
 
-/*
- * IRQs are disabled when we're called from the timer interrupt,
- * no need for spin_lock_irqsave()
+/*
+ * set_rtc_mmss is called when irqs are enabled, so disable irqs here
 */
-
-    spin_lock(&rtc_lock);
+    spin_lock_irqsave(&rtc_lock, flags);
 

/*
* Tell the clock it's being set and stop it.
@@ -138,7 +137,7 @@ static int set_rtc_mmss(unsigned long no

    CMOS_WRITE(control, RTC_CONTROL);
    CMOS_WRITE(freq_select, RTC_FREQ_SELECT);

- spin_unlock(&rtc_lock);
+ spin_unlock_irqrestore(&rtc_lock, flags);

    return retval;
}
@@ -163,22 +162,30 @@ unsigned long read_persistent_clock(void
    unsigned long flags;
    unsigned century = 0;

- spin_lock_irqsave(&rtc_lock, flags);
+ retry: spin_lock_irqsave(&rtc_lock, flags);
+ /* if UIP is clear, then we have >= 244 microseconds before RTC
+ * registers will be updated. Spec sheet says that this is the
+ * reliable way to read RTC - registers invalid (off bus) during update
+ */
+ if ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)) {

+      spin_unlock_irqrestore(&rtc_lock, flags);
+      cpu_relax();
+      goto retry;

+ }

- do {

-        sec = CMOS_READ(RTC_SECONDS);
-        min = CMOS_READ(RTC_MINUTES);
-        hour = CMOS_READ(RTC_HOURS);
-        day = CMOS_READ(RTC_DAY_OF_MONTH);
-        mon = CMOS_READ(RTC_MONTH);
-        year = CMOS_READ(RTC_YEAR);
+    /* now read all RTC registers while stable with interrupts disabled */
+
+    sec = CMOS_READ(RTC_SECONDS);
+    min = CMOS_READ(RTC_MINUTES);
+    hour = CMOS_READ(RTC_HOURS);
+    day = CMOS_READ(RTC_DAY_OF_MONTH);
+    mon = CMOS_READ(RTC_MONTH);
+    year = CMOS_READ(RTC_YEAR);

#ifdef CONFIG_ACPI
- if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
- acpi_gbl_FADT.century)
- century = CMOS_READ(acpi_gbl_FADT.century);
+ if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
+ acpi_gbl_FADT.century)
+ century = CMOS_READ(acpi_gbl_FADT.century);
#endif
- } while (sec != CMOS_READ(RTC_SECONDS));
-

    spin_unlock_irqrestore(&rtc_lock, flags);

    /*

From: David P. Reed

Fix a typo in ntp.c that has caused updating of the persistent (RTC) clock when synced to NTP to behave erratically.

---
When debugging a freeze that arises on my AMD64 machines when I run the ntpd service, I added a number of printk's to monitor the sync_cmos_clock procedure. I discovered that it was not syncing to cmos RTC every 11 minutes as documented, but instead would keep trying every
second for hours at a time. The reason turned out to be a typo in
sync_cmos_clock, where it attempts to ensure that update_persistent_clock is called very close to 500 msec. after a 1 second boundary (required by the PC RTC's spec). That typo referred to "xtime" in one spot, rather than "now", which is derived from "xtime" but not equal to it. This makes the test erratic, creating a "coin-flip" that decides when update_persistent_clock is called - when it is called, which is rarely, it may be at any time during the one second period, rather than close to 500 msec, so the value written is needlessly incorrect, too.

I rewrote the code to fix the typo and to be a little more clear about the logic that was intended here. I have thoroughly tested this on several x86_64 machines, and also on one 32-bit machine. I have also submitted a patch to fix the freeze cited above, but both patches are independent, and should be treated separately.

The patch works in 2.6.24rc2, but it should also work in 2.6.23.

Index: linux-2.6/kernel/time/ntp.c
===================================================================
@@ -188,6 +188,7 @@ static DEFINE_TIMER(sync_cmos_timer, syn

static void sync_cmos_clock(unsigned long dummy)

{
    struct timespec now, next;
+    long delta_from_target_time;
    int fail = 1;

    /*
@@ -205,7 +206,10 @@ static void sync_cmos_clock(unsigned lon
        return;

    getnstimeofday(&now);

- if (abs(xtime.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
+ delta_from_target_time = abs(now.tv_nsec - (NSEC_PER_SEC / 2));
+
+ /* set CMOS clock, only if close enough to 500 msec point */
+ if (delta_from_target_time <= tick_nsec / 2)

        fail = update_persistent_clock(now);

    next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec;

David,

1) Please send separate patches for separate problems.

2) Your patch is white space damaged and does not apply. Please run it through scripts/checkpatch.pl before submitting, maybe send it to yourself first and verify that it applies correctly.

Good catch.

of the what ?

Also put the detailed description into the patch comment, please.

Care to resend ?

Thanks,

tglx

David,

Good catch.

Whitespace damaged as well. Please fix and resend

Please put the explanation into the changelog.

Yup

Will make two patches and resend.

I've already got a set of patches brewing to fix the UIP problem across all the affected arches (11+).

Mathematics is the supreme nostalgia of our time.

Cool. More reason to separate this into two.

Fix hard freeze on x86_64 when the ntpd service calls update_persistent_clock()

A repeatable but randomly timed freeze has been happening in Fedora 6 and 7 for the last year, whenever I run the ntpd service on my AMD64x2 HP Pavilion dv9000z laptop. This freeze is due to the use of spin_lock(&rtc_lock) under the assumption (per a bad comment) that set_rtc_mmss is called only with interrupts disabled. The call from ntp.c to update_persistent_clock is made with interrupts enabled.

---
I have thoroughly tested this patch on a number of x86_64 machines with various numbers of cores and chipsets, using 2.6.24rc2 kernel source.

Index: linux-2.6/arch/x86/kernel/time_64.c
===================================================================
@@ -82,18 +82,15 @@ static int set_rtc_mmss(unsigned long no
 	int retval = 0;
 	int real_seconds, real_minutes, cmos_minutes;
 	unsigned char control, freq_select;
+	unsigned long flags;
 
 /*
- * IRQs are disabled when we're called from the timer interrupt,
- * no need for spin_lock_irqsave()
+ * set_rtc_mmss is called when irqs are enabled, so disable irqs here
  */
-
-	spin_lock(&rtc_lock);
-
+	spin_lock_irqsave(&rtc_lock, flags);
 /*
  * Tell the clock it's being set and stop it.
  */
-
 	control = CMOS_READ(RTC_CONTROL);
 	CMOS_WRITE(control | RTC_SET, RTC_CONTROL);
 

@@ -138,7 +135,7 @@ static int set_rtc_mmss(unsigned long no
 	CMOS_WRITE(control, RTC_CONTROL);
 	CMOS_WRITE(freq_select, RTC_FREQ_SELECT);
 
-	spin_unlock(&rtc_lock);
+	spin_unlock_irqrestore(&rtc_lock, flags);
 
 	return retval;
 }

Fix a typo in ntp.c that has caused updating of the persistent (RTC) clock when synced to NTP to behave erratically.

When debugging a freeze that arises on my AMD64 machines when I run the ntpd service, I added a number of printk's to monitor the sync_cmos_clock procedure. I discovered that it was not syncing to cmos RTC every 11 minutes as documented, but instead would keep trying every second for hours at a time. The reason turned out to be a typo in sync_cmos_clock, where it attempts to ensure that update_persistent_clock is called very close to 500 msec. after a 1 second boundary (required by the PC RTC's spec). That typo referred to "xtime" in one spot, rather than "now", which is derived from "xtime" but not equal to it. This makes the test erratic, creating a "coin-flip" that decides when update_persistent_clock is called - when it is called, which is rarely, it may be at any time during the one second period, rather than close to 500 msec, so the value written is needlessly incorrect, too.

---
I have thoroughly tested this on several x86_64 machines, and also on one 32-bit machine. I have also submitted a patch to fix the freeze cited above, but both patches are independent, and should be treated separately.

Index: linux-2.6/kernel/time/ntp.c
===================================================================
@@ -205,7 +205,7 @@ static void sync_cmos_clock(unsigned lon
 		return;
 
 	getnstimeofday(&now);
-	if (abs(xtime.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
+	if (abs(now.tv_nsec - (NSEC_PER_SEC / 2)) <= tick_nsec / 2)
 		fail = update_persistent_clock(now);
 
 	next.tv_nsec = (NSEC_PER_SEC / 2) - now.tv_nsec;

Correct potentially unstable PC RTC time register reading in time_64.c

Stop the use of an incorrect technique for reading the standard PC RTC timer, which is documented to "disconnect" time registers from the bus while updates are in progress. The use of UIP flag while interrupts are disabled to protect a 244 microsecond window is one of the Motorola spec sheet's documented ways to read the RTC time registers reliably.

The patch updates the misleading comments and also minimizes the amount of time that the kernel disables interrupts during the reading.

---

Index: linux-2.6/arch/x86/kernel/time_64.c
===================================================================
@@ -160,22 +160,30 @@ unsigned long read_persistent_clock(void
 	unsigned long flags;
 	unsigned century = 0;
 
-	spin_lock_irqsave(&rtc_lock, flags);
+ retry:	spin_lock_irqsave(&rtc_lock, flags);
+	/* if UIP is clear, then we have >= 244 microseconds before RTC
+	 * registers will be updated.  Spec sheet says that this is the
+	 * reliable way to read RTC - registers invalid (off bus) during update
+	 */
+	if ((CMOS_READ(RTC_FREQ_SELECT) & RTC_UIP)) {
+	  spin_unlock_irqrestore(&rtc_lock, flags);
+	  cpu_relax();
+	  goto retry;
+	}
 
-	do {
-		sec = CMOS_READ(RTC_SECONDS);
-		min = CMOS_READ(RTC_MINUTES);
-		hour = CMOS_READ(RTC_HOURS);
-		day = CMOS_READ(RTC_DAY_OF_MONTH);
-		mon = CMOS_READ(RTC_MONTH);
-		year = CMOS_READ(RTC_YEAR);
+	/* now read all RTC registers while stable with interrupts disabled */
+
+	sec = CMOS_READ(RTC_SECONDS);
+	min = CMOS_READ(RTC_MINUTES);
+	hour = CMOS_READ(RTC_HOURS);
+	day = CMOS_READ(RTC_DAY_OF_MONTH);
+	mon = CMOS_READ(RTC_MONTH);
+	year = CMOS_READ(RTC_YEAR);
 #ifdef CONFIG_ACPI
-		if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
-					acpi_gbl_FADT.century)
-			century = CMOS_READ(acpi_gbl_FADT.century);
+	if (acpi_gbl_FADT.header.revision >= FADT2_REVISION_ID &&
+				acpi_gbl_FADT.century)
+		century = CMOS_READ(acpi_gbl_FADT.century);
 #endif
-	} while (sec != CMOS_READ(RTC_SECONDS));
-
 	spin_unlock_irqrestore(&rtc_lock, flags);
 
 	/*

On Wed, 14 Nov 2007, David P. Reed wrote:

Still whitespace wreckage in your patches. I guess the kernel tree you made your patches against is already white space wrecked.

I fixed that up manually, but please be more careful about that next time.

While I think that the UIP change is correct and a must have, the locking change is not really useful. read_persistent_clock is called from exactly three places:

Right after boot, right before suspend and right after resume. None of those places is a hot path, where we really care about the interrupt enable/disable. IIRC, this is even called with interrupts disabled most of the time, so no real gain here.

Another reason not to do the locking change is the paravirt stuff which is coming for 64bit. I looked into the existing 32bit code and doing the same lock thing would introduce a real nasty hackery, which is definitely not worth the trouble.

Thanks,

tglx

There are a couple of things I don't understand on this one. And I presume you thought the other two bug fixing patches I sent before this were OK to go, since on my system

Um ... I fixed the whitespaces I detected from the first round with checkpatch.pl. Then for good measure I ran checkpatch.pl on the patches, then pasted the files directly into the emails. No problems detected. And I also just tried checkpatch.pl on the "sent" folder copy. No problems detected there.
Where was the whitespace? Was it in the patches? Would you mind showing me the output so I can do a better job in the future?

What locking change? I didn't change how locking works in read_persistent_clock at all.
I did introduce cpu_relax() because if anyone else ever calls from a hot path, that would be good practice and its' one line.

I presume time_64.c and time_32.c will be unified at some point, discarding time_64.c. There's no arch-specific reason to be separate. The current time_32.c depends on a different nmi path (that does some weird stuff saving and restoring the CMOS index register!), and I didn't dare usurp your long-term plan to unify architectures. But a simple cleanup here makes sense, lest someone copy the bad technique as if it were good.

I had to fix up all of them.

Never paste patches into mail. Also I should have checked your mail client earlier. Thunderbird is famous for this :) Documentation/email-clients.txt has some info on that.

Try to apply your own patches right from the sent folder copy. They will reject.

Well the output was a simple reject when applying the patch.

Whitespace damage was for example here:

	   unsigned long flags;

20 20 09 75 6e 73 69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a

where the correct patch should read:

20 09 75 6e 73 69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a

Your mailer or the paste added a second blank (0x20) at the beginning of the line.

Hot path calls to this code would be extremly stupid and are forbidden by the Penguin Law. :)

cpu_relax is not the problem, but you actuall changed the locking:

Old code:

    spin_lock();
    do {
       ....
    } while (stupid check);
    spin_unlock();

New code:

    while (1) {
        spin_lock();
        if (useful_check) {
             spin_unlock();
             cpu_relax();
        } else
             break;

}

So with the old code I can take out the inner loop and do what paravirtualization in the 32 bit code does:

	spin_lock_irqsave(&rtc_lock, flags);
	result = get_wallclock();
	spin_unlock_irqrestore(&rtc_lock, flags);

Where get_wallclock() either resolves to the plain rtc code or to the paravirtualized function depending on the boot mode of the kernel.

With your change we either would need to put lokck/unlock of rtc_lock into each incarnation of paravirt or do some nasty hack, where we convey the flags to the called function. Neither of this makes sense and is worth the work.

Yeah, those files are on the radar. The cleanup branch of the x86 git tree has a first go on this already. And I'm currently figuring out what we can do about this ugly CMOS index hack.

Anyway I keep the locking straight and simple as it is right now, the cpu_relax() works fine with a lock held, while we are waiting the bunch of usecs for UIP going away.

Thanks,

tglx

Replace use of outb to "unused" diagnostic port 0x80 for time delay with udelay based time delay on x86_64 architecture machines. Fix for bugs 9511 and 6307 in bugzilla, plus bugs reported in bugzilla.redhat.com.

Derived from suggestion (that didn't compile) by Pavel Machek, and tested, also based on measurements of typical timings of out's collated by Rene Herman from many in the community.

This patch fixes a number of bugs known to cause problems on HP Pavilion dv9000z and dv6000z laptops - in the form of solid freezes when hwclock is used to show or set the time. Also, it potentially improves bus utilization on SMP machines, by using a waiting process that doesn't tie up the ISA/LPC bus for 1 or 2 microseconds.

i386 family fixes (completely parallel) were not included, considering that such machines might involve more risk of problems on legacy machines.

Index: linux-2.6/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -269,10 +269,10 @@ static void putstr(const char *s)
        RM_SCREEN_INFO.orig_y = y;
 
        pos = (x + cols * y) * 2;       /* Update cursor position */
-       outb_p(14, vidport);
-       outb_p(0xff & (pos >> 9), vidport+1);
-       outb_p(15, vidport);
-       outb_p(0xff & (pos >> 1), vidport+1);
+       outb(14, vidport);
+       outb(0xff & (pos >> 9), vidport+1);
+       outb(15, vidport);
+       outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

long count) \

Here's the corresponding 32-bit patch. Applies fine to (and works on) v2.6.23.x as well after un-uniting the paths...

I do believe it would be better to do both; x86-64 machines can run 32-bit kernels fine and might certainly in the form of generic (installer) kernels and the like so if it's a fix lets fix it across the board -- the 2 us delay is going to be enough for everything out there, certainly given that _0_ is normally enough. With the arches just merged, minimising diferences should be a goal in itself I guess.

One thing though -- I believe we want to adjust the loops_per_jiffy default to make sure this works, or would work, pre-calibration as well? Not really sure how needed it is, but bogomips is defined as

	bogomips = loops_per_jiffy / (500000 / HZ)
			<=>
	loops_per_jiffy = bogomips * (500000 / HZ)

which with assuming any machine above 1G isn't affected by any of this and bogomips coming out to 2 * MHz on anything I myself have run linux on might suggest that

	loops_per_jiffy = 2 * 1000 * (500000 / HZ) = 1000000000 / HZ

could possibly be a sane if very large default? Alan?

index 9def935..6d41771 100644
@@ -229,10 +229,9 @@ static int __init obsolete_checksetup(char *line)
 }
 
 /*
- * This should be approx 2 Bo*oMips to start (note initial shift), and will
- * still work even if initially too large, it will just take slightly longer
+ * Initial value roughly corresponds to a 1 GHz CPU
  */
-unsigned long loops_per_jiffy = (1<<12);
+unsigned long loops_per_jiffy = 1000000000 / HZ;
 
 EXPORT_SYMBOL(loops_per_jiffy);

From: Pavel Machek <[email protected]>

32-bit part of the port 0x80 delay replacement.

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index fe881cd..9abc215 100644
@@ -3,6 +3,7 @@
 
 #include <linux/string.h>
 #include <linux/compiler.h>
+#include <linux/delay.h>
 
 /*
  * This file contains the definitions for the x86 IO instructions

@@ -17,17 +18,6 @@
  * mistake somewhere.
  */
 
-/*
- * Thanks to James van Artsdalen for a better timing-fix than
- * the two short jumps: using outb's to a nonexistent port seems
- * to guarantee better timings even on fast machines.
- *
- * On the other hand, I'd like to be sure of a non-existent port:
- * I feel a bit unsafe about using 0x80 (should be safe, though)
- *
- *		Linus
- */
-
  /*
   *  Bit simplified and optimized by Jan Hubicka
   *  Support of BIGMEM added by Gerhard Wichert, Siemens AG, July 1999.

@@ -252,7 +242,7 @@ static inline void flush_write_buffers(void)
 
 static inline void native_io_delay(void)
 {
-	asm volatile("outb %%al,$0x80" : : : "memory");
+	udelay(2);
 }
 
 #if defined(CONFIG_PARAVIRT)

They're needed because lots of people fomr some reason still boot 32bit kernels on 64bit machines.

Sure. Want me to make a new patch with the _p croutines out-of-line?

wow, cool fix! (I remember that there were other systems as well that are affected by port 0x80 muckery - i thought we had removed port 0x80 accesses long ago.)

how about the simpler fix below, as a first-level approach? We can then remove the _p in/out sequences after this.

this is also something for v2.6.24 merging.

Ingo

----------------------------->
Subject: x86: fix in/out_p delays
From: Ingo Molnar <[email protected]>

Debugged by David P. Reed <[email protected]>.

Do not use port 0x80, it can cause crashes, see:

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

instead of just removing _p postfixes en masse, lets just first remove the 0x80 port usage, then remove any unnecessary _p io ops gradually. It's more debuggable this way.

---
 arch/x86/boot/compressed/misc_32.c |    8 ++++----
 arch/x86/boot/compressed/misc_64.c |    8 ++++----
 arch/x86/kernel/quirks.c           |    9 +++++++++
 include/asm-x86/io_32.h            |    5 +----
 include/asm-x86/io_64.h            |    5 +----
 5 files changed, 19 insertions(+), 16 deletions(-)

Index: linux-x86.q/arch/x86/boot/compressed/misc_32.c
===================================================================
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -275,10 +275,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/kernel/quirks.c
===================================================================
@@ -6,6 +6,15 @@
 
 #include <asm/hpet.h>
 
+/*
+ * Some legacy devices need delays for IN/OUT sequences. Most are
+ * probably not needed but it's the safest to just do this short delay:
+ */
+void native_io_delay(void)
+{
+	udelay(1);
+}
+
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(v
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -35,10 +35,7 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

updated patch attached. (from the MakeItBuild'n'Stuff dept)

Ingo

-------------->
Subject: x86: fix in/out_p delays
From: Ingo Molnar <[email protected]>

Debugged by David P. Reed <[email protected]>.

Do not use port 0x80, it can cause crashes, see:

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

instead of just removing _p postfixes en masse, lets just first remove the 0x80 port usage, then remove any unnecessary _p io ops gradually. It's more debuggable this way.

---
 arch/x86/boot/compressed/misc_32.c |    8 ++++----
 arch/x86/boot/compressed/misc_64.c |    8 ++++----
 arch/x86/kernel/quirks.c           |   10 ++++++++++
 include/asm-x86/io_32.h            |    5 +----
 include/asm-x86/io_64.h            |    5 +----
 5 files changed, 20 insertions(+), 16 deletions(-)

Index: linux-x86.q/arch/x86/boot/compressed/misc_32.c
===================================================================
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -275,10 +275,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/kernel/quirks.c
===================================================================
@@ -3,9 +3,19 @@
  */
 #include <linux/pci.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
 #include <asm/hpet.h>
 
+/*
+ * Some legacy devices need delays for IN/OUT sequences. Most are
+ * probably not needed but it's the safest to just do this short delay:
+ */
+void native_io_delay(void)
+{
+	udelay(1);
+}
+
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(v
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -35,10 +35,7 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Your version does the same thing that the version from Pavel/David does for 32-bit at least.

the one below is against current upstream. (previous ones were against x86.git)

Ingo

------------------->
Subject: x86: fix in/out_p delays
From: Ingo Molnar <[email protected]>

Debugged by David P. Reed <[email protected]>.

Do not use port 0x80, it can cause crashes, see:

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

instead of just removing _p postfixes en masse, lets just first remove the 0x80 port usage, then remove any unnecessary _p io ops gradually. It's more debuggable this way.

---
 arch/x86/boot/compressed/misc_32.c |    8 ++++----
 arch/x86/boot/compressed/misc_64.c |    8 ++++----
 arch/x86/kernel/quirks.c           |   10 ++++++++++
 include/asm-x86/io_32.h            |    5 +----
 include/asm-x86/io_64.h            |   14 +++++---------
 5 files changed, 24 insertions(+), 21 deletions(-)

Index: linux-x86.q/arch/x86/boot/compressed/misc_32.c
===================================================================
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/kernel/quirks.c
===================================================================
@@ -3,9 +3,19 @@
  */
 #include <linux/pci.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
 #include <asm/hpet.h>
 
+/*
+ * Some legacy devices need delays for IN/OUT sequences. Most are
+ * probably not needed but it's the safest to just do this short delay:
+ */
+void native_io_delay(void)
+{
+	udelay(1);
+}
+
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(v
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -35,13 +35,7 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
-
-#ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
+extern void native_io_delay(void);
 
 /*
  * Talk about misusing macros..

@@ -54,7 +48,8 @@ __asm__ __volatile__ ("out" #s " %" s1 "
 
 #define __OUT(s,s1,x) \
 __OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s##_p,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); \
+native_io_delay(); } \
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;

@@ -64,7 +59,8 @@ __asm__ __volatile__ ("in" #s " %" s2 "1
 
 #define __IN(s,s1,i...) \
 __IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; \
+native_io_delay(); } \
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

well, if you carefully look at the code it's not the "same" but a similar but cleaner thing - it moves this quirk out of a common include file. I take back the "simpler" characterisation - my patch indeed ended up being almost the same as David's.

yep, i have updated the delay to 2 usecs. The latest patch is below, as queued up in x86.git. (not yet queued up for .24 - it's pending testing and more feedback, etc.)

Ingo

----------------->
Subject: x86: fix in_p/out_p crashes
From: David P. Reed <[email protected]>

Do not use port 0x80, it can cause crashes, see:

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

Replace use of outb to "unused" diagnostic port 0x80 for time delay with udelay based time delay on x86_64 architecture machines. Fix for bugs 9511 and 6307 in bugzilla, plus bugs reported in bugzilla.redhat.com.

Derived from suggestion (that didn't compile) by Pavel Machek, and tested, also based on measurements of typical timings of out's collated by Rene Herman from many in the community.

This patch fixes a number of bugs known to cause problems on HP Pavilion dv9000z and dv6000z laptops - in the form of solid freezes when hwclock is used to show or set the time. Also, it potentially improves bus utilization on SMP machines, by using a waiting process that doesn't tie up the ISA/LPC bus for 1 or 2 microseconds.

[ [email protected]: minor restructuring, 32-bit support. ]

---
 arch/x86/boot/compressed/misc_32.c |    8 ++++----
 arch/x86/boot/compressed/misc_64.c |    8 ++++----
 arch/x86/kernel/quirks.c           |   10 ++++++++++
 include/asm-x86/io_32.h            |    5 +----
 include/asm-x86/io_64.h            |   14 +++++---------
 5 files changed, 24 insertions(+), 21 deletions(-)

Index: linux-x86.q/arch/x86/boot/compressed/misc_32.c
===================================================================
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/kernel/quirks.c
===================================================================
@@ -3,9 +3,19 @@
  */
 #include <linux/pci.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
 #include <asm/hpet.h>
 
+/*
+ * Some legacy devices need delays for IN/OUT sequences. Most are
+ * probably not needed but it's the safest to just do this short delay:
+ */
+void native_io_delay(void)
+{
+	udelay(2);
+}
+
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(v
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -35,13 +35,7 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
-
-#ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
+extern void native_io_delay(void);
 
 /*
  * Talk about misusing macros..

@@ -54,7 +48,8 @@ __asm__ __volatile__ ("out" #s " %" s1 "
 
 #define __OUT(s,s1,x) \
 __OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s##_p,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); \
+native_io_delay(); } \
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;

@@ -64,7 +59,8 @@ __asm__ __volatile__ ("in" #s " %" s2 "1
 
 #define __IN(s,s1,i...) \
 __IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; \
+native_io_delay(); } \
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

Yes, I'd like feedback on the initial value thing:

http://lkml.org/lkml/2007/12/14/72

and Alan's comments here:

http://lkml.org/lkml/2007/12/12/221

And as to testing -- good luck finding a machine that cares at all ;-)

Rene.

actually, there's a whole lot more testing angle to a change like this than ancient boxes.

Ingo

Only any outb_p's used before loops_per_jiffy is calibrated are affected.

This pre-calibation thing is what's historically held this change back (it's been discussed dozens of times before). At 4096, not any machine is going to have an appreciable delay before calibration when switching from the outb to
0x80.

Rene.

the last version is the one below. Pending further discussion and testing. And David, i nominate your fix as the coolest Linux kernel fix of 2007 :-)

Ingo

-------------------------------->

Subject: x86: fix in_p/out_p crashes
From: David P. Reed <[email protected]>

Do not use port 0x80, it can cause crashes, see:

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

Replace use of outb to "unused" diagnostic port 0x80 for time delay with udelay based time delay on x86_64 architecture machines. Fix for bugs 9511 and 6307 in bugzilla, plus bugs reported in bugzilla.redhat.com.

Derived from suggestion (that didn't compile) by Pavel Machek, and tested, also based on measurements of typical timings of out's collated by Rene Herman from many in the community.

This patch fixes a number of bugs known to cause problems on HP Pavilion dv9000z and dv6000z laptops - in the form of solid freezes when hwclock is used to show or set the time. Also, it potentially improves bus utilization on SMP machines, by using a waiting process that doesn't tie up the ISA/LPC bus for 1 or 2 microseconds.

[ [email protected]: minor restructuring, 32-bit support. ]

---
 arch/x86/boot/compressed/misc_32.c |    8 ++++----
 arch/x86/boot/compressed/misc_64.c |    8 ++++----
 arch/x86/kernel/quirks.c           |   10 ++++++++++
 include/asm-x86/io_32.h            |    5 +----
 include/asm-x86/io_64.h            |   14 +++++---------
 5 files changed, 24 insertions(+), 21 deletions(-)

Index: linux-x86.q/arch/x86/boot/compressed/misc_32.c
===================================================================
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/boot/compressed/misc_64.c
===================================================================
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

Index: linux-x86.q/arch/x86/kernel/quirks.c
===================================================================
@@ -3,9 +3,19 @@
  */
 #include <linux/pci.h>
 #include <linux/irq.h>
+#include <linux/delay.h>
 
 #include <asm/hpet.h>
 
+/*
+ * Some legacy devices need delays for IN/OUT sequences. Most are
+ * probably not needed but it's the safest to just do this short delay:
+ */
+void native_io_delay(void)
+{
+	udelay(2);
+}
+
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
 static void __devinit quirk_intel_irqbalance(struct pci_dev *dev)

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(v
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -35,13 +35,7 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
-
-#ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
+extern void native_io_delay(void);
 
 /*
  * Talk about misusing macros..

@@ -54,7 +48,8 @@ __asm__ __volatile__ ("out" #s " %" s1 "
 
 #define __OUT(s,s1,x) \
 __OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s##_p,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); \
+native_io_delay(); } \
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;

@@ -64,7 +59,8 @@ __asm__ __volatile__ ("in" #s " %" s2 "1
 
 #define __IN(s,s1,i...) \
 __IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; \
+native_io_delay(); } \
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

yes - but there are a couple of early udelays, which would thus be affected.

i dont think this should matter: old systems that truly _need_ the ISA delay will be slow enough to not trip up. (nor are they really affected by these early delays - the delays were more for crappy ISA devices that get initialized later down, when the delay loop is already calibrated)

modern systems learned to depend on the PCI write posting side-effects of port 0x80 activities - those wont be helped by this initialization change either. That is a far more serious concern.

Ingo

True. At the moment though they're just always not delaying anywhere close the intended amount (on anything with more than 2 bogomips). Pre-calibration all this stuff is just broken it seems.

8253 (DMAC) and 8254 (PIT) have been reported in earlier versions of the thread. By Alan, I believe.

Rene.

kvm will forward a virtual machine's writes to port 0x80 to the real port. The reason is that the write is much faster than exiting and emulating it; the difference is measurable when compiling kernels.

Now if the cause is simply writing to port 0x80, then we must stop doing that. But if the reason is the back-to-back writes, when we can keep it, since the other writes will be trapped by kvm and emulated. Do you which is the case?

Any sufficiently difficult bug is indistinguishable from a feature.

I believe this will suffer from the issue that was raised: this will use udelay() long before loop calibration (and no, we can't just "be conservative" since there is no "conservative" value we can use.)

Worse, I suspect that at least the PIT, which may need to be used for udelay calibration, is one of the devices that may be affected. I have seen the Verilog for a contemporary chipset, and it can only access the PIT once per microsecond -- this actually has to do with the definition of the PIT; some of the PIT operations are ill-defined if allowed at a higher frequency than the PIT clock.

-hpa

Was reported before indeed:

http://linux.derkeiler.com/Mailing-Lists/Kernel/2003-09/5764.html

Rene.

They've been seen to be problems up to PII era machines. I'm not aware of any newer than that with this problem. We also don't need to touch the DMAC that early anyway that I can see - just the PIT.

In fact if we have a fast processor we have a TSC and APIC so we don't need the PIT ?

Alan

Well, the TSC may be unstable and the APIC may be disabled.

-hpa

It is still good to be able to disable APIC/TSC. Neither are particulary reliable time sources.

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

?? Just initialize bogomips to 6GHz equivalent... and we are fine until 6GHz cpus come out.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

How long will that take to boot on a 386?

-hpa

Well the dumb approach to fix that would seem to be to initialise it to

	cpu->family   3 -> 50MHz 4 -> 300Mhz 5-> etc...

Alan

As for kvm, I don't have enough info to know anything about that. Is there a test you'd like me to try?

I think you are also asking if the crash on these laptops is caused only by back-to-back writes. Actually, it doesn't seem to matter if they are back to back. I can cause the crash if the writes to 80 are very much spread out in time - it seems only to matter how many of them get executed - almost as if there is a buffer overflow. (And of course if you do back to back writes to other ports that are apparently fully unused, such as 0xED on my machine, no crash occurs).

I believe (though no one seems to have confirming documentation from the chipset or motherboard vendor) that port 80 is actually functional for some unknown function on these machines. (They do respond to "in" instructions faster than a bus cycle abort does - more evidence).

I searched the DSDT to see if there is any evidence of an ACPI use for this port, but found nothing.

This is normal. IN from port 0x80 is used by the DMA address map chip.

As far as I understand, there are other laptops with the same chipset which don't have this problem, so it's likely either a motherboard or firmware issue. My guess is that they probably let debugging code out in the field (trap port 0x80 in SMM, and then try to output it on some debugging bus.)

-hpa

-------------------------------------------------------------------------
SF.Net email is sponsored by:
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services
for just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Just a thought for a way to fix the "very early" timing needed to set up udelay to work in a way that works on all machines. Perhaps we haven't exploited the BIOS enough: The PC BIOS since the PC AT (286) has always had a standard "countdown timer" way to delay for n microseconds, which as far as I know still works. This can be used to measure the speed of a delay loop, without using ANY in/out instructions directly (the ones in the BIOS are presumably correctly delayed...).

So first thing in the boot sequence, one can calibrate a timing loop using this technique, and save the value to be used for all the "early" stuff.

Here's skeleton code from old ASM code I found lying around in my archives to use BIOS to measure how many unrolled short jumps can execute in 10 msec. Note that it can run without knowing anything whatsoever about port timing.

haltbyte db 0

calibrate:
les bx,haltbyte ; address of halt flag into es:bx mov ax,8300h
sub cx,cx
mov dx,10000 ; 10 msec. in cx:dx
int 15h
jc bad
sub dx,dx
sub cx,cx ; decrement counter in dx:cx
tloop:
jmp short $+2 ; 10 short jmps
jmp short $+2
jmp short $+2
jmp short $+2
jmp short $+2
jmp short $+2
jmp short $+2
jmp short $+2
jmp short $+2
test haltbyte
loopz tloop
jnz done
dec dx
jnz tloop

" overflowed 32 bits ... never happens, cancel BIOS event wait.
mov ax,8301h
int 15h
jmp error

done:
mov ax,cx
negl
" here dx:ax contains 32 bit loop count corresponding to 10 msec.
ret ; return 32-bit value

Doc on function 83h of int 15h should be available online.

If we enter from the 32-bit entrypoint, we already don't have access to the BIOS, though.

-hpa

i think the native_io_delay() in quirks.c signals the obvious solution:
a DMI (or otherwise) driven quirk that activates a port 0x80 based delay on such boards. Combined with an iodelay=port80 boot option as well perhaps, just in case someone hits a system that is not blacklisted yet. This way such crazy broken hardware can be mapped correctly - like we map such quirks in every other case. Perhaps even do this workaround on the PIT driver level. Instead of perpetuating the superstition of port 80 forever.

Ingo

The issue is -- how do you safely replace the outb pre-loops_per_jiffy calibration? I'm currently running with the attached hack (not submitted, only for 32-bit and discussion) the idea of which might be the best we can do?

(And the broken is the hardware that can't take writes to port 0x80, not the other way around. It's a well-known legacy PC thing).

Rene.

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index 8a322c9..c95d313 100644
@@ -222,6 +222,19 @@ void __init hpet_time_init(void)
 	time_init_hook();
 }
 
+static void port_io_delay(void)
+{
+	asm volatile ("outb %%al, $0x80": : : "memory");
+}
+
+static void udelay_io_delay(void)
+{
+	udelay(2);
+}
+
+void (*native_io_delay)(void) = port_io_delay;
+EXPORT_SYMBOL(native_io_delay);
+
 /*
  * This is called directly from init code; we must delay timer setup in the
  * HPET case as we can't make the decision to turn on HPET this early in the

@@ -233,5 +246,7 @@ void __init hpet_time_init(void)
 void __init time_init(void)
 {
 	tsc_init();
+	if (!tsc_disable)
+		native_io_delay = udelay_io_delay;
 	late_time_init = choose_time_init();
 }

index fe881cd..1b73f49 100644
@@ -250,10 +250,7 @@ static inline void flush_write_buffers(void)
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void (*native_io_delay)(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

Hello,

Just an idea : from what I've read, the problem (port 80 hanging) only occurs on 'modern' machines... So why not :
- use port 80 for old CPUs (PII, PIII) where it has never really been a problem,
- use the cpu->family to do a best match for CPU freq
thus we could avoid increasing boot time too much...

Paul

Yes, just posted a Patch-For-Comments that switches on the availability of a TSC (tsc_init sets tsc_disable also for !cpu_has_tsc) which would mean that only really old stuff would be using the outb still. A TSC is really all we need to have a sensible udelay().

Rene.

how about doing a known-NOP chipset cycle? For example:

inb(PIC_SLAVE_IMR)

? I.e. instead of trying to find an unused port, lets try to find a known-used platform register that has no side-effects if read. Use it unconditionally during early bootup and change it to an udelay after calibration. (or use it after early bootup too if a boot parameter has been specified) Or something like this.

Ingo

An inb is annoying in that it clobbers register al (well, with an inline native_io_delay it is at least) and more importantly -- the timing of this is going to vary wildly. We really want a register that is effectively guaranteed to be unused so that it dies on ISA/LPC or we might get _much_ faster PCI only decodes. Even reading port 0x80 itself varies wildly:

http://lkml.org/lkml/2007/12/12/309

It's really going to have to be a known _un_used register and (the write direction of) port 0x80 is used exactly for that reason. Port 0xed is a known "alternate diagnostic port" used by Phoenix BIOSes at least but Peter Anvin reported trouble with that one -- probably for the outb direction but assuming that means something was in fact responding, we'd have the same timing problem.

I believe we have two "good" options:

1) port 0xed was tested by the current reporter and found to be safe (and provide slow enough timing). If DMI based quirk hacks are available soon enough we can switch 0x80 to 0xed based on it. Are they?

2) the thing I posted in the message replied to where immediately after tsc_init() (which is before the PIT init) we switch to udelay() if we have a TSC which is ofcourse anything modern.

Rene.

It needs tobe a different chip to the main one (or macrocell anyway) - so PIC for PIT and vice versa. However since we know 0x80 works for everything on the planet but this one specifies of laptop which seems to need a firmware update its a very high risk approach.

Alan

I understand the risks of such a fundamental change, and it may be only a minor concern, but I did also point out that using an unused port causes the bus to be tied up for a microsecond or two, which matters on a fast SMP machine.

Of course all the other concerns you guys are worrying about are really important. I don't want to break anybody's running systems... I'd like to see my machine run smoothly, and all the other machines that may or may not have this problem (google "hwclock freeze" to see that I'm far from alone - I just have persevered in "bisecting" this problem with kernel tweaks for months, whereas the others did not or did not know how).

By the way, this laptop is really nice for Linux in lots of ways. Dual drives, so I set it up with software RAID for reliability, dual 64-bit processors, fast 3D graphics, etc. Great battery life. Just one last kernel issue.

I also note that curent machines like the problem machine have ACPI, and maybe those would be the ones that vendors might start to define port 80 to mean something. As I noted, it /seems/ to be only when ACPI is turned on that this problem happens on my machine - that's when the OS starts to be involved in servicing various things, so it suggests that maybe things change about port 80's unknown function on these machines when ACPI is servicing the system management code (that's not something I have been able to verify).

My belief is that my machine has some device that is responding to port 80 by doing something. And that something requires some other program to "service" port 80 in some way. But it sure would be nice to know. I can't personally sand off the top of the chipset to put probes into it - so my normal approach of putting a logic analyzer on the bus doesn't work.

PS: If I have time, I may try to build Rene's port 80 test for Windows and run it under WinXP on this machine (I still have a crappy little partition that boots it). If it freezes the same way, it's almost certain a design "feature", and if it doesn't freeze, we might suspect that there is compensating logic in either Windows ACPI code or some way that windows "sets up" the machine.

This change seems rather unlikely for 2.6.24 at this point (high risk), but could be good for 2.6.25.

One thing it should probably have for the early going, is a simple way to turn it on/off at boot time, so that we don't have people "stuck" unable to run the test kernels should something weird happen.

Alan / David / Ingo,

What do you think of the idea of a *temporary* boot flag for this, something like port80=on/off (pick a suitable name) ?

Cheers

My understanding is that the linux starts in real mode, and uses the BIOS for such things as reading the very first image. arch/x86/boot/main.c seems to use BIOS calls, and one can do what I wrote in C or asm. Good place to measure the appropriate delay timing, and pass it on forward. That's what I was suggesting, which is why I copied the ASM routine from my old code listing as I did.

Not always. We may enter from 32bit in some cases, and we may also not have a PC BIOS in the first place.

And I did point out I'd found locking cases that may be relying upon this

Port 0x80 means debug. You appear to have a laptop with some kind of buggy firmware that wants a BIOS update. Everyone use 0x80 for debug - its in the chipset hardware quite often.

Almost certainly a SMI trap.

That would be very interesting.

Alan

Yes, for the outbound direction.

DMI is just a data structure parked in memory, so it should at least be theoretically possible to get to it.

-hpa

It happens on *one single* "modern" machine...

Let's keep that in perspective.

-hpa

Uhm, no. You have no clue what the speed of the TSC is until you have been able to calibrate it against a fixed timesource - like the PIT.

-hpa

it hurts on other machines (like debug leds being useless), and it may be incorrect as soon as you insert leds-on-port-0x80-on-PCI card.

No, it is not critical but yes, I'd like to see it fixed.

Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

I don't think we are using outb_p before loops_per_jiffy are initialized -- I believe I'd see oopsen if we did. Factor 2400 increase is bad, but if it only converts 10x 1usec delay into 10x 24msec delay, it is not _that_ bad.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

As much as I like this patch, I do not think it is suitable for .24. Too risky, I'd say.

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

No kidding! We're talking about removing a hack that has been successful on thousands of pieces of hardware over 15 years because it breaks ONE machine.

If this should be done at all it should be done in the most careful manner possible. 2.6.25 would be an aggressive schedule.

-hpa

Yes. Hnng. Okay, this is going nowhere in a hurry, so back to the very first suggestion in this thread. How about this? This allows to switch from port 0x80 to port 0xed based on DMI.

David: I plugged in my own DMI values for testing, but obviously yours are needed. The values that are needed are retrieved by the "dmidecode" program which you will probably have installed (it might be in an sbin directory) or will be able to install through whatever package manager you use.

dmidecode -s baseboard-manufacturer
dmidecode -s baseboard-product-name

are the values you should plug into the .matches field in the dmi_system_id struct in this. It would be great if you could do that, test, and post back with those values. .ident should be a nice human name.

It's been tested on x86-32 and seems to work fine. It's not been tested on x86-64 but seems to stand a fair chance of working similarly.

It ofcourse remains possible to switch to a udelay() based method later on anyways but with all the pre-calibratin trouble, this might be the lowest risk method in the short run.

This is partly based on previous patches by Pavel Machek and David P. Reed.

I hope this is considered half-way correct/sane (note by the way that it's not a good idea to switch a "native_io_delay_port" value since plugging in a variable port would clobber register dx for every outb_p, which would then have to be reloaded for the next outb again). Comments appreciated.

 arch/x86/boot/compressed/misc_32.c |    8 ++---
 arch/x86/boot/compressed/misc_64.c |    8 ++---
 arch/x86/kernel/Makefile_32        |    2 -
 arch/x86/kernel/Makefile_64        |    2 -
 arch/x86/kernel/io_delay.c         |   53 
+++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup_32.c         |    2 +
 arch/x86/kernel/setup_64.c         |    2 +
 include/asm-x86/io_32.h            |   17 ++---------
 include/asm-x86/io_64.h            |   23 ++++++----------

commit 4a7e75776c648102488a89dbfad516448830ab1a

Author: Rene Herman <[email protected]>
Date:   Sun Dec 16 00:24:32 2007 +0100

foo

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index 6ea015a..43e5fcc 100644
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index a7bc93c..0cc1981 100644
@@ -8,7 +8,7 @@ CPPFLAGS_vmlinux.lds += -Ui386
 obj-y	:= process_32.o signal_32.o entry_32.o traps_32.o irq_32.o \
 		ptrace_32.o time_32.o ioport_32.o ldt_32.o setup_32.o i8259_32.o sys_i386_32.o \
 		pci-dma_32.o i386_ksyms_32.o i387_32.o bootflag.o e820_32.o\
-		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o
+		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index 5a88890..08a68f0 100644
@@ -11,7 +11,7 @@ obj-y	:= process_64.o signal_64.o entry_64.o traps_64.o irq_64.o \
 		x8664_ksyms_64.o i387_64.o syscall_64.o vsyscall_64.o \
 		setup64.o bootflag.o e820_64.o reboot_64.o quirks.o i8237.o \
 		pci-dma_64.o pci-nommu_64.o alternative.o hpet.o tsc_64.o bugs_64.o \
-		i8253.o
+		i8253.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index e1e18c3..6c3a3b4 100644
@@ -648,6 +648,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();;
+
 #ifdef CONFIG_X86_GENERICARCH
 	generic_apic_probe();
 #endif	

index 30d94d1..ec976ed 100644
@@ -311,6 +311,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();
+
 #ifdef CONFIG_SMP
 	/* setup to use the static apicid table during kernel startup */
 	x86_cpu_to_apicid_ptr = (void *)&x86_cpu_to_apicid_init;

index fe881cd..bf352e3 100644
@@ -250,24 +250,13 @@ static inline void flush_write_buffers(void)
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void io_delay_init(void);
+extern void (*native_io_delay)(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>
 #else
-
-static inline void slow_down_io(void) {
-	native_io_delay();
-#ifdef REALLY_SLOW_IO
-	native_io_delay();
-	native_io_delay();
-	native_io_delay();
-#endif
-}
-
+extern void slow_down_io(void);
 #endif
 
 #ifdef CONFIG_X86_NUMAQ

index a037b07..486a110 100644
@@ -35,13 +35,8 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
-
-#ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
+extern void io_delay_init(void);
+extern void slow_down_io(void);
 
 /*
  * Talk about misusing macros..

@@ -50,21 +45,21 @@
 static inline void out##s(unsigned x value, unsigned short port) {
 
 #define __OUT2(s,s1,s2) \
-__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1"
+__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1" : : "a" (value), "Nd" (port))
 
 #define __OUT(s,s1,x) \
-__OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s,x) __OUT2(s,s1,"w"); } \
+__OUT1(s##_p,x) __OUT2(s,s1,"w"); slow_down_io(); }
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;
 
 #define __IN2(s,s1,s2) \
-__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0"
+__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0" : "=a" (_v) : "Nd" (port))
 
-#define __IN(s,s1,i...) \
-__IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+#define __IN(s,s1) \
+__IN1(s) __IN2(s,s1,"w"); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w"); slow_down_io(); return _v; }
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

That actually wouldn't be that big of a deal. Switching values in and out of registers is dirt cheap (and MUCH cheaper than an indirect function call) -- of course, if there is a reason to do it for paravirtualization then that's fine; we're talking about something that makes even the slowest CPU operation look speedy anyhow.

If in*_p and out*_p are out-of-lined then %dx would be dead anyway, and so there is even less reason to deal with it.

In theory we could use an alternatives section and patch the instruction, too; seems like way overkill, though.

Note, however, that your code doesn't deal with io_delay()'s in the boot code (arch/x86/boot) at all, nor (obviously) io_delay()'s in boot loaders. In the boot code, access to DMI data is NOT available (we can't even use the INT 15h mover if we want to continue to support Loadlin.)

In the boot code, io_delay() is used to slow down accesses to the KBC, interrupt controller, INT13h logic, and the NMI gate, and to provide a fixed delay during A20 stabilization.

-hpa

Correction: DMI data are at least supposedly available via the PNPBIOS calls specified in the SMBIOS spec.

-hpa

Here we go.

# dmidecode -s baseboard-manufacturer
Quanta
# dmidecode -s baseboard-product-name
30B9

There do seem to be other systems, besides mine, that have the same problem. I think it's pretty likely that other machines that have this problem are Quanta machines, since Quanta is one of the primary ODM's that does HP laptops. Don't know about the product-name being common with the HP dv6000z family, which is another one reported to have this problem. We could try to ask all the reporters of hwclock freezes to report their results from dmidecode.

two or three i think (and an unknown of others where "random, unexplained freezes" were thought to be hw borkage), but yeah, it's still a very low proportion.

Ingo

                                                             ^----[*]

[*] "- none of which needs it anymore -"

there, fixed it for you ;-)

So lets keep this in perspective: this is a hack that only helps on a very low number of systems. (the PIT of one PII era chipset is known to be affected)

unfortunately this hack's side-effects are mis-used by an unknown number of drivers to mask PCI posting bugs. We want to figure out those bugs (safely and carefully) and we want to remove this hack from modern machines that dont need it. Doing anything else would be superstition.

anyway, we likely wont be doing anything about this in .24.

Ingo

Well, I suppose. With stuff inline, constantly reloading dx also bloats things up a bit but yes, out of line who cares. Do you think this version is better?

Thanks for the heads up (also saw the SMBIOS update to this) but those don't seem to be a problem in fact. David Reed has been running with the simple udelay(2) version of this and reported no more hangs. He moreover reported no trouble after booting with "acpi=off" meaning that things seem to be fine pre-acpi which the boot code (and this io_delay_init) is. So I believe we get to ignore those.

David: I've plugged in your DMI values in this. Could you perhaps test this to confirm that it works for you?

Any ACKs, NAKs or further comments from others in this thread also welcome.

Changelog in the patch.

 arch/x86/boot/compressed/misc_32.c |    8 ++---
 arch/x86/boot/compressed/misc_64.c |    8 ++---
 arch/x86/kernel/Makefile_32        |    2 -
 arch/x86/kernel/Makefile_64        |    2 -
 arch/x86/kernel/io_delay.c         |   54 
+++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup_32.c         |    2 +
 arch/x86/kernel/setup_64.c         |    2 +
 include/asm-x86/io_32.h            |   17 ++---------
 include/asm-x86/io_64.h            |   23 ++++++---------
 9 files changed, 80 insertions(+), 38 deletions(-)

Rene.

commit a17ccb1964b53fd4ab00d501b7f229a9a6cf91d1

Author: Rene Herman <[email protected]>
Date:   Sun Dec 16 13:36:39 2007 +0100

x86: provide a DMI based port 0x80 I/O delay override.

Certain (HP) laptops experience trouble from our port 0x80 I/O delay writes. This patch provides for a DMI based switch to the "alternate diagnostic port" 0xed (as used by some BIOSes as well) for these.

David P. Reed confirmed that port 0xed works for him and provides a proper delay. The symptoms of _not_ working are a hanging machine, with "hwclock" use being a direct trigger.

Earlier versions of this attempted to simply use udelay(2), with the 2 being a value tested to be a nicely conservative upper-bound with help from many on the linux-kernel mailinglist, but that approach has two problems.

First, pre-loops_per_jiffy calibration (which is post PIT init while some implementations of the PIT are actually one of the historically problematic devices that need the delay) udelay() isn't particularly well-defined. We could initialise loops_per_jiffy conservatively (and based on CPU family so as to not unduly delay old machines) which would sort of work, but still leaves problem 2.

Second, delaying isn't the only effect that a write to port 0x80 has. It's also a PCI posting barrier which some devices may be explicitly or implicitly relying on. Alan Cox did a survey and found evidence that additionally some drivers may be racy on SMP without the bus locking outb.

Switching to an inb() makes the timing too unpredictable and as such, this DMI based switch should be the safest approach for now. Any more invasive changes should get more rigid testing first. It's moreover only very few machines with the problem and a DMI based hack seems to fit that situation.

This does not change the io_delay() in the boot code which is using the same port 0x80 I/O delay but those do not appear to be a problem as David P. Reed reported the problem was already gone after using the udelay(2) version of this. He moreover reported that booting with "acpi=off" also fixed things and seeing as how ACPI isn't touched until after this DMI based I/O port switch I believe it's safe to leave the ones in the boot code be.

The DMI strings from David's HP Pavilion dv9000z are in there already and we need to get/verify the DMI info from other machines with the problem, notably the HP Pavilion dv6000z.

This patch is partly based on earlier patches from Pavel Machek and David P. Reed.

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index 6ea015a..43e5fcc 100644
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index a7bc93c..0cc1981 100644
@@ -8,7 +8,7 @@ CPPFLAGS_vmlinux.lds += -Ui386
 obj-y	:= process_32.o signal_32.o entry_32.o traps_32.o irq_32.o \
 		ptrace_32.o time_32.o ioport_32.o ldt_32.o setup_32.o i8259_32.o sys_i386_32.o \
 		pci-dma_32.o i386_ksyms_32.o i387_32.o bootflag.o e820_32.o\
-		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o
+		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index 5a88890..08a68f0 100644
@@ -11,7 +11,7 @@ obj-y	:= process_64.o signal_64.o entry_64.o traps_64.o irq_64.o \
 		x8664_ksyms_64.o i387_64.o syscall_64.o vsyscall_64.o \
 		setup64.o bootflag.o e820_64.o reboot_64.o quirks.o i8237.o \
 		pci-dma_64.o pci-nommu_64.o alternative.o hpet.o tsc_64.o bugs_64.o \
-		i8253.o
+		i8253.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index e1e18c3..6c3a3b4 100644
@@ -648,6 +648,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();;
+
 #ifdef CONFIG_X86_GENERICARCH
 	generic_apic_probe();
 #endif	

index 30d94d1..ec976ed 100644
@@ -311,6 +311,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();
+
 #ifdef CONFIG_SMP
 	/* setup to use the static apicid table during kernel startup */
 	x86_cpu_to_apicid_ptr = (void *)&x86_cpu_to_apicid_init;

index fe881cd..5d4e5e5 100644
@@ -250,24 +250,13 @@ static inline void flush_write_buffers(void)
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
-{
-	asm volatile("outb %%al,$0x80" : : : "memory");
-}
+extern void io_delay_init(void);
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>
 #else
-
-static inline void slow_down_io(void) {
-	native_io_delay();
-#ifdef REALLY_SLOW_IO
-	native_io_delay();
-	native_io_delay();
-	native_io_delay();
-#endif
-}
-
+extern void slow_down_io(void);
 #endif
 
 #ifdef CONFIG_X86_NUMAQ

index a037b07..486a110 100644
@@ -35,13 +35,8 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
-
-#ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
-#endif
+extern void io_delay_init(void);
+extern void slow_down_io(void);
 
 /*
  * Talk about misusing macros..

@@ -50,21 +45,21 @@
 static inline void out##s(unsigned x value, unsigned short port) {
 
 #define __OUT2(s,s1,s2) \
-__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1"
+__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1" : : "a" (value), "Nd" (port))
 
 #define __OUT(s,s1,x) \
-__OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s,x) __OUT2(s,s1,"w"); } \
+__OUT1(s##_p,x) __OUT2(s,s1,"w"); slow_down_io(); }
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;
 
 #define __IN2(s,s1,s2) \
-__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0"
+__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0" : "=a" (_v) : "Nd" (port))
 
-#define __IN(s,s1,i...) \
-__IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+#define __IN(s,s1) \
+__IN1(s) __IN2(s,s1,"w"); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w"); slow_down_io(); return _v; }
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

looks good to me. Could you please also provide three more controls that i suggested earlier:

- a boot option enabling/disabling the udelay based code
- a .config method of enabling/disabling the udelay based code
- a sysctl to toggle it

if we want to clean this all up we'll need as many controls as possible.

Ingo

It probably comes down to which version is bigger (you probably also want to try uninlining.)

Okay, so there is something inside ACPI which tickles this. Which brings further credibility that it's activating a debugging hack, probably inside the SuperIO/system controller chip.

It would be interesting to know exactly which part of ACPI triggers this. I bet it is a reference to system controller namespace.

-hpa

Yes, but the status quo has been *tested* on thousands of systems and is known to work. Thus, changing it puts things into unknown territory, even if only a small number of machines actually need the current configuration.

Heck, there are only a small number of 386/486 machines still in operation and being actively updated.

Again, 24 is "right out". 25 is a "maybe", IMO. Rene's fix could be an exception, since it is a DMI-keyed workaround for a specific machine and doesn't change behaviour in general.

-hpa

The process of safely making delicate changes here is beyond my responsibility as just a user - believe me, I'm not suggesting that a risky fix be put in .24. I can patch my own kernels, and I can even share an unofficial patch with others for now, or suggest that Fedora and Ubuntu add it to their downstream.

May I make a small suggestion, though. If the decision is a DMI-keyed switch from out-80 to udelay(2) gets put in, perhaps there should also be a way for people to test their own configuration for the underlying problem made available as a script. Though it is a "hack", all you need to freeze a problem system is to run a loop doing about 1000 "cat /dev/nvram > /dev/null" commands. If that leads to a freeze, one might ask to have the motherboard added to the DMI-key list.

Will test it by tomorrow morning.

Hi!

Can you freeze it by catting /dev/rtc, too? That may be significant, because that is readable for group audio (at least on some systems)... which would smell like "small security hole" to me.
Pavel

(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Heck, on my system (Fedora 7), it's mode 644...

-hpa

This version does the boot and the .config option but not the sysctl. It makes for clumsy code and I don't believe it provides for much added value as soon as you have the boot option. I am moreover not completely confident about things such as paravirt liking the possibility of the native_io_delay being changed out from under them at unpredictable times.

So how is this? Also fixes a few problems with the previous version.

 Documentation/kernel-parameters.txt |    8 ++
 arch/x86/Kconfig.debug              |    9 +++
 arch/x86/boot/compressed/misc_32.c  |    8 +-
 arch/x86/boot/compressed/misc_64.c  |    8 +-
 arch/x86/kernel/Makefile_32         |    2
 arch/x86/kernel/Makefile_64         |    2
 arch/x86/kernel/io_delay.c          |  103 
++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/setup_32.c          |    2
 arch/x86/kernel/setup_64.c          |    2
 include/asm-x86/io_32.h             |    6 --
 include/asm-x86/io_64.h             |   27 +++++----
 11 files changed, 152 insertions(+), 25 deletions(-)

Rene.

commit 5001121e449040aa9cc021f69bfb191662c13004

Author: Rene Herman <[email protected]>
Date:   Sun Dec 16 13:36:39 2007 +0100

x86: provide a DMI based port 0x80 I/O delay override.

Certain (HP) laptops experience trouble from our port 0x80 I/O delay writes. This patch provides for a DMI based switch to the "alternate diagnostic port" 0xed (as used by some BIOSes as well) for these.

David P. Reed confirmed that port 0xed works for him and provides a proper delay. The symptoms of _not_ working are a hanging machine, with "hwclock" use being a direct trigger.

Earlier versions of this attempted to simply use udelay(2), with the 2 being a value tested to be a nicely conservative upper-bound with help from many on the linux-kernel mailinglist but that approach has two problems.

First, pre-loops_per_jiffy calibration (which is post PIT init while some implementations of the PIT are actually one of the historically problematic devices that need the delay) udelay() isn't particularly well-defined. We could initialise loops_per_jiffy conservatively (and based on CPU family so as to not unduly delay old machines) which would sort of work, but...

Second, delaying isn't the only effect that a write to port 0x80 has. It's also a PCI posting barrier which some devices may be explicitly or implicitly relying on. Alan Cox did a survey and found evidence that additionally some drivers may be racy on SMP without the bus locking outb.

Switching to an inb() makes the timing too unpredictable and as such, this DMI based switch should be the safest approach for now. Any more invasive changes should get more rigid testing first. It's moreover only very few machines with the problem and a DMI based hack seems to fit that situation.

This also introduces a command-line parameter "io_delay" to override the DMI based choice again:

io_delay=<standard|alternate>

where "standard" means using the standard port 0x80 and "alternate" port 0xed.

At the request of Ingo Molnar this retains the udelay method as a config (CONFIG_UDELAY_IO_DELAY) and command-line ("io_delay=udelay") choice for testing purposes as well.

This does not change the io_delay() in the boot code which is using the same port 0x80 I/O delay but those do not appear to be a problem as David P. Reed reported the problem was already gone after using the udelay version. He moreover reported that booting with "acpi=off" also fixed things and seeing as how ACPI isn't touched until after this DMI based I/O port switch I believe it's safe to leave the ones in the boot code be.

The DMI strings from David's HP Pavilion dv9000z are in there already and we need to get/verify the DMI info from other machines with the problem, notably the HP Pavilion dv6000z.

This patch is partly based on earlier patches from Pavel Machek and David P. Reed.

index 33121d6..9dce154 100644
@@ -785,6 +785,14 @@ and is between 256 and 4096 characters. It is defined in the file
 			for translation below 32 bit and if not available
 			then look in the higher range.
 
+	io_delay=	[X86-32,X86-64] I/O delay method
+		standard
+			Standard port 0x80 delay
+		alternate
+			Alternate port 0xed delay
+		udelay
+			Simple two microsecond delay
+
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in
 			arch/alpha/kernel/core_marvel.c.

index 761ca7b..40aba67 100644
@@ -112,4 +112,13 @@ config IOMMU_LEAK
 	  Add a simple leak tracer to the IOMMU code. This is useful when you
 	  are debugging a buggy device driver that leaks IOMMU mappings.
 
+config UDELAY_IO_DELAY
+	bool "Delay I/O through udelay instead of outb"
+	depends on DEBUG_KERNEL
+	help
+	  Make inb_p/outb_p use udelay() based delays by default. Please note
+	  that udelay() does not have the same bus-level side-effects that
+	  the normal outb based delay does meaning this could cause drivers
+	  to change behaviour and/or bugs to surface.
+
 endmenu

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index 6ea015a..43e5fcc 100644
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index a7bc93c..0cc1981 100644
@@ -8,7 +8,7 @@ CPPFLAGS_vmlinux.lds += -Ui386
 obj-y	:= process_32.o signal_32.o entry_32.o traps_32.o irq_32.o \
 		ptrace_32.o time_32.o ioport_32.o ldt_32.o setup_32.o i8259_32.o sys_i386_32.o \
 		pci-dma_32.o i386_ksyms_32.o i387_32.o bootflag.o e820_32.o\
-		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o
+		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index 5a88890..08a68f0 100644
@@ -11,7 +11,7 @@ obj-y	:= process_64.o signal_64.o entry_64.o traps_64.o irq_64.o \
 		x8664_ksyms_64.o i387_64.o syscall_64.o vsyscall_64.o \
 		setup64.o bootflag.o e820_64.o reboot_64.o quirks.o i8237.o \
 		pci-dma_64.o pci-nommu_64.o alternative.o hpet.o tsc_64.o bugs_64.o \
-		i8253.o
+		i8253.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index e1e18c3..6c3a3b4 100644
@@ -648,6 +648,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();;
+
 #ifdef CONFIG_X86_GENERICARCH
 	generic_apic_probe();
 #endif	

index 30d94d1..ec976ed 100644
@@ -311,6 +311,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();
+
 #ifdef CONFIG_SMP
 	/* setup to use the static apicid table during kernel startup */
 	x86_cpu_to_apicid_ptr = (void *)&x86_cpu_to_apicid_init;

index fe881cd..a8d25c3 100644
@@ -250,10 +250,14 @@ static inline void flush_write_buffers(void)
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
+#ifndef CONFIG_UDELAY_IO_DELAY
+extern void io_delay_init(void);
+#else
+static inline void io_delay_init(void)
 {
-	asm volatile("outb %%al,$0x80" : : : "memory");
 }
+#endif
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

index a037b07..5bebaf9 100644
@@ -35,13 +35,24 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
+#ifndef CONFIG_UDELAY_IO_DELAY
+extern void io_delay_init(void);
+#else
+static inline void io_delay_init(void)
+{
+}
+#endif
+extern void native_io_delay(void);
 
+static inline void slow_down_io(void)
+{
+	native_io_delay();
 #ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
+	native_io_delay();
+	native_io_delay();
+	native_io_delay();
 #endif
+}
 
 /*
  * Talk about misusing macros..

@@ -50,21 +61,21 @@
 static inline void out##s(unsigned x value, unsigned short port) {
 
 #define __OUT2(s,s1,s2) \
-__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1"
+__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1" : : "a" (value), "Nd" (port))
 
 #define __OUT(s,s1,x) \
-__OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s,x) __OUT2(s,s1,"w"); } \
+__OUT1(s##_p,x) __OUT2(s,s1,"w"); slow_down_io(); }
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;
 
 #define __IN2(s,s1,s2) \
-__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0"
+__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0" : "=a" (_v) : "Nd" (port))
 
-#define __IN(s,s1,i...) \
-__IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+#define __IN(s,s1) \
+__IN1(s) __IN2(s,s1,"w"); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w"); slow_down_io(); return _v; }
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

slow_down_io() sort of needs to stay inline due to the REALLY_SLOW_IO thing. That stuff could use a cleanup, but that would be a diferent patch.

Do you expect a BIOS update to be able to fix it? If so, I guess any DMI hack should take BIOS version into account.

Rene.

I've not much opinion on the schedule as I've not the problem but yes, it's intended as the low risk option.

Rene.

Hard to know without knowing what it is.

-hpa

Might as well test the new version then. Ingo Molnar requested a few changes and this fixes a couple of problems as well.

Rene.

commit 5001121e449040aa9cc021f69bfb191662c13004

Author: Rene Herman <[email protected]>
Date:   Sun Dec 16 13:36:39 2007 +0100

x86: provide a DMI based port 0x80 I/O delay override.

Certain (HP) laptops experience trouble from our port 0x80 I/O delay writes. This patch provides for a DMI based switch to the "alternate diagnostic port" 0xed (as used by some BIOSes as well) for these.

David P. Reed confirmed that port 0xed works for him and provides a proper delay. The symptoms of _not_ working are a hanging machine, with "hwclock" use being a direct trigger.

Earlier versions of this attempted to simply use udelay(2), with the 2 being a value tested to be a nicely conservative upper-bound with help from many on the linux-kernel mailinglist but that approach has two problems.

First, pre-loops_per_jiffy calibration (which is post PIT init while some implementations of the PIT are actually one of the historically problematic devices that need the delay) udelay() isn't particularly well-defined. We could initialise loops_per_jiffy conservatively (and based on CPU family so as to not unduly delay old machines) which would sort of work, but...

Second, delaying isn't the only effect that a write to port 0x80 has. It's also a PCI posting barrier which some devices may be explicitly or implicitly relying on. Alan Cox did a survey and found evidence that additionally some drivers may be racy on SMP without the bus locking outb.

Switching to an inb() makes the timing too unpredictable and as such, this DMI based switch should be the safest approach for now. Any more invasive changes should get more rigid testing first. It's moreover only very few machines with the problem and a DMI based hack seems to fit that situation.

This also introduces a command-line parameter "io_delay" to override the DMI based choice again:

io_delay=<standard|alternate>

where "standard" means using the standard port 0x80 and "alternate" port 0xed.

At the request of Ingo Molnar this retains the udelay method as a config (CONFIG_UDELAY_IO_DELAY) and command-line ("io_delay=udelay") choice for testing purposes as well.

This does not change the io_delay() in the boot code which is using the same port 0x80 I/O delay but those do not appear to be a problem as David P. Reed reported the problem was already gone after using the udelay version. He moreover reported that booting with "acpi=off" also fixed things and seeing as how ACPI isn't touched until after this DMI based I/O port switch I believe it's safe to leave the ones in the boot code be.

The DMI strings from David's HP Pavilion dv9000z are in there already and we need to get/verify the DMI info from other machines with the problem, notably the HP Pavilion dv6000z.

This patch is partly based on earlier patches from Pavel Machek and David P. Reed.

index 33121d6..9dce154 100644
@@ -785,6 +785,14 @@ and is between 256 and 4096 characters. It is defined in the file
 			for translation below 32 bit and if not available
 			then look in the higher range.
 
+	io_delay=	[X86-32,X86-64] I/O delay method
+		standard
+			Standard port 0x80 delay
+		alternate
+			Alternate port 0xed delay
+		udelay
+			Simple two microsecond delay
+
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in
 			arch/alpha/kernel/core_marvel.c.

index 761ca7b..40aba67 100644
@@ -112,4 +112,13 @@ config IOMMU_LEAK
 	  Add a simple leak tracer to the IOMMU code. This is useful when you
 	  are debugging a buggy device driver that leaks IOMMU mappings.
 
+config UDELAY_IO_DELAY
+	bool "Delay I/O through udelay instead of outb"
+	depends on DEBUG_KERNEL
+	help
+	  Make inb_p/outb_p use udelay() based delays by default. Please note
+	  that udelay() does not have the same bus-level side-effects that
+	  the normal outb based delay does meaning this could cause drivers
+	  to change behaviour and/or bugs to surface.
+
 endmenu

index b74d60d..288e162 100644
@@ -276,10 +276,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index 6ea015a..43e5fcc 100644
@@ -269,10 +269,10 @@ static void putstr(const char *s)
 	RM_SCREEN_INFO.orig_y = y;
 
 	pos = (x + cols * y) * 2;	/* Update cursor position */
-	outb_p(14, vidport);
-	outb_p(0xff & (pos >> 9), vidport+1);
-	outb_p(15, vidport);
-	outb_p(0xff & (pos >> 1), vidport+1);
+	outb(14, vidport);
+	outb(0xff & (pos >> 9), vidport+1);
+	outb(15, vidport);
+	outb(0xff & (pos >> 1), vidport+1);
 }
 
 static void* memset(void* s, int c, unsigned n)

index a7bc93c..0cc1981 100644
@@ -8,7 +8,7 @@ CPPFLAGS_vmlinux.lds += -Ui386
 obj-y	:= process_32.o signal_32.o entry_32.o traps_32.o irq_32.o \
 		ptrace_32.o time_32.o ioport_32.o ldt_32.o setup_32.o i8259_32.o sys_i386_32.o \
 		pci-dma_32.o i386_ksyms_32.o i387_32.o bootflag.o e820_32.o\
-		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o
+		quirks.o i8237.o topology.o alternative.o i8253.o tsc_32.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index 5a88890..08a68f0 100644
@@ -11,7 +11,7 @@ obj-y	:= process_64.o signal_64.o entry_64.o traps_64.o irq_64.o \
 		x8664_ksyms_64.o i387_64.o syscall_64.o vsyscall_64.o \
 		setup64.o bootflag.o e820_64.o reboot_64.o quirks.o i8237.o \
 		pci-dma_64.o pci-nommu_64.o alternative.o hpet.o tsc_64.o bugs_64.o \
-		i8253.o
+		i8253.o io_delay.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-y				+= cpu/

index e1e18c3..6c3a3b4 100644
@@ -648,6 +648,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();;
+
 #ifdef CONFIG_X86_GENERICARCH
 	generic_apic_probe();
 #endif	

index 30d94d1..ec976ed 100644
@@ -311,6 +311,8 @@ void __init setup_arch(char **cmdline_p)
 
 	dmi_scan_machine();
 
+	io_delay_init();
+
 #ifdef CONFIG_SMP
 	/* setup to use the static apicid table during kernel startup */
 	x86_cpu_to_apicid_ptr = (void *)&x86_cpu_to_apicid_init;

index fe881cd..a8d25c3 100644
@@ -250,10 +250,14 @@ static inline void flush_write_buffers(void)
 
 #endif /* __KERNEL__ */
 
-static inline void native_io_delay(void)
+#ifndef CONFIG_UDELAY_IO_DELAY
+extern void io_delay_init(void);
+#else
+static inline void io_delay_init(void)
 {
-	asm volatile("outb %%al,$0x80" : : : "memory");
 }
+#endif
+extern void native_io_delay(void);
 
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>

index a037b07..5bebaf9 100644
@@ -35,13 +35,24 @@
   *  - Arnaldo Carvalho de Melo <[email protected]>
   */
 
-#define __SLOW_DOWN_IO "\noutb %%al,$0x80"
+#ifndef CONFIG_UDELAY_IO_DELAY
+extern void io_delay_init(void);
+#else
+static inline void io_delay_init(void)
+{
+}
+#endif
+extern void native_io_delay(void);
 
+static inline void slow_down_io(void)
+{
+	native_io_delay();
 #ifdef REALLY_SLOW_IO
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO __SLOW_DOWN_IO
-#else
-#define __FULL_SLOW_DOWN_IO __SLOW_DOWN_IO
+	native_io_delay();
+	native_io_delay();
+	native_io_delay();
 #endif
+}
 
 /*
  * Talk about misusing macros..

@@ -50,21 +61,21 @@
 static inline void out##s(unsigned x value, unsigned short port) {
 
 #define __OUT2(s,s1,s2) \
-__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1"
+__asm__ __volatile__ ("out" #s " %" s1 "0,%" s2 "1" : : "a" (value), "Nd" (port))
 
 #define __OUT(s,s1,x) \
-__OUT1(s,x) __OUT2(s,s1,"w") : : "a" (value), "Nd" (port)); } \
-__OUT1(s##_p,x) __OUT2(s,s1,"w") __FULL_SLOW_DOWN_IO : : "a" (value), "Nd" (port));} \
+__OUT1(s,x) __OUT2(s,s1,"w"); } \
+__OUT1(s##_p,x) __OUT2(s,s1,"w"); slow_down_io(); }
 
 #define __IN1(s) \
 static inline RETURN_TYPE in##s(unsigned short port) { RETURN_TYPE _v;
 
 #define __IN2(s,s1,s2) \
-__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0"
+__asm__ __volatile__ ("in" #s " %" s2 "1,%" s1 "0" : "=a" (_v) : "Nd" (port))
 
-#define __IN(s,s1,i...) \
-__IN1(s) __IN2(s,s1,"w") : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
-__IN1(s##_p) __IN2(s,s1,"w") __FULL_SLOW_DOWN_IO : "=a" (_v) : "Nd" (port) ,##i ); return _v; } \
+#define __IN(s,s1) \
+__IN1(s) __IN2(s,s1,"w"); return _v; } \
+__IN1(s##_p) __IN2(s,s1,"w"); slow_down_io(); return _v; }
 
 #define __INS(s) \
 static inline void ins##s(unsigned short port, void * addr, unsigned long count) \

As far as I can tell, the code still uses udelay() before calibration if io_delay=udelay?

Just so we're clear on that...

-hpa

Incidentally, I had the thought earlier today that port 0xf0 might be a suitable delay port. It is used only by the 387-emulating-a-287 hack for IRQ 13, which Linux doesn't use on 486+.

-hpa

Yes. This patch is explicitly about the alternate port and not about udelay.
As discussed (and changelogged) the calibration is just one problem with
PCI posting and possible SMP races the other ones. Ingo Molnar wanted it as a debugging thing already though.

Once we start discussing udelay() again I believe we should go with the simple per CPU-Family loops_per_jiffy initialization to fix that first problem (and I guess I could hack that in now) but then the bigger problem remains and will need a fair amount of testing at least and mostly on machines that are by now gathering dust in a few basements...

Rene

rene@7ixe4:~/src/port80$ su -c ./port80
cycles: out 2400, in 2400
rene@7ixe4:~/src/port80$ su -c ./portf0
cycles: out 2400, in 2400

(Duron 1300)

I suppose you mean using it instead of port 0x80 always and not just as an alternate port? For the latter 0xed is alright I guess...

Rene.

Well, we probably should leave the possibility in to use 0x80 -- for one thing, we need to use 0x80 on 386, and there is always the possibility that the switch will have different timing properties on some or all machines.

Note that this doesn't require that a machine actually implements port 0xf0 for FERR/IGNNE, it just requires that they don't use it for something else.

I would be rather inclined to try using port 0xf0 by default as long as family > 3[*] (with fallback to port 0x80) at least experimentally (-mm).

We *might* even be able to use port 0xf0 unconditionally in the setup code, since we're not using the FPU there (the only FPU instructions in the setup code are there to detect the FPU.)

One thing: although I believe most actual implementations of port 0xf0 implement it as a strobe alone (data is ignored), all documentation I've found, including "The Undocumented PC" specifically says "write 0x00 to this port." This *could* mean there are platforms which use other values than 0x00 for other hacks.

-hpa

[*] The following statements are equivalent:
- family > 3.
- CR0.NE is settable.
- EFLAGS.AC is settable.

FWIW, the criterion used in the kernel for when to use IRQ 13 is:

        /*
         * External FPU? Set up irq13 if so, for
         * original braindamaged IBM FERR coupling.
         */
        if (boot_cpu_data.hard_math && !cpu_has_fpu)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                setup_irq(FPU_IRQ, &fpu_irq);

In that case we can't actually use port 0xF0 (it is, however, safe to use it during setup, specifically before we can take our first FPU exception.)

-hpa

thanks Rene! I've added your patch to x86.git. I changed a few things ontop of it, see the additional changelog and delta patch below.

Ingo

------------>

- add the io_delay=none method
- make each method selectable from the kernel config
- simplify the delay code a bit by getting rid of an indirect function call
- add the /proc/sys/kernel/io_delay_type sysctl
- change 'standard' and 'alternate' to 0x80 and 0xed
- make the io delay config not depend on CONFIG_DEBUG_KERNEL

---
 Documentation/kernel-parameters.txt |   12 ++--
 arch/x86/Kconfig.debug              |   79 +++++++++++++++++++++++++--
 arch/x86/kernel/io_delay.c          |  103 ++++++++++++++++--------------------
 include/asm-x86/io_32.h             |    2 
 include/asm-x86/io_64.h             |    2 
 kernel/sysctl.c                     |    9 +++
 6 files changed, 138 insertions(+), 69 deletions(-)

Index: linux-x86.q/Documentation/kernel-parameters.txt
===================================================================
@@ -786,12 +786,14 @@ and is between 256 and 4096 characters. 
 			then look in the higher range.
 
 	io_delay=	[X86-32,X86-64] I/O delay method
-		standard
-			Standard port 0x80 delay
-		alternate
-			Alternate port 0xed delay
+		0x80
+			Standard port 0x80 based delay
+		0xed
+			Alternate port 0xed based delay (needed on some systems)
 		udelay
-			Simple two microsecond delay
+			Simple two microseconds delay
+		none
+			No delay
 
 	io7=		[HW] IO7 for Marvel based alpha systems
 			See comment before marvel_specify_io7 in

Index: linux-x86.q/include/asm-x86/io_32.h
===================================================================
@@ -259,6 +259,8 @@ static inline void io_delay_init(void)
 #endif
 extern void native_io_delay(void);
 
+extern int io_delay_type;
+
 #if defined(CONFIG_PARAVIRT)
 #include <asm/paravirt.h>
 #else

Index: linux-x86.q/include/asm-x86/io_64.h
===================================================================
@@ -44,6 +44,8 @@ static inline void io_delay_init(void)
 #endif
 extern void native_io_delay(void);
 
+extern int io_delay_type;
+
 static inline void slow_down_io(void)
 {
 	native_io_delay();

Index: linux-x86.q/kernel/sysctl.c
===================================================================
@@ -53,6 +53,7 @@
 #ifdef CONFIG_X86
 #include <asm/nmi.h>
 #include <asm/stacktrace.h>
+#include <asm/io.h>
 #endif
 
 static int deprecated_sysctl_warning(struct __sysctl_args *args);

@@ -683,6 +684,14 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
+	{
+		.ctl_name	= CTL_UNNUMBERED,
+		.procname	= "io_delay_type",
+		.data		= &io_delay_type,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= &proc_dointvec,
+	},
 #endif
 #if defined(CONFIG_MMU)
 	{

here's an updated rollup patch, against 2.6.24-rc4. David, could you please try this? This should work out of box on your system, without any boot option or other tweak needed.

Ingo

------------------------->
Subject: x86: provide a DMI based port 0x80 I/O delay override.
From: Rene Herman <[email protected]>

x86: provide a DMI based port 0x80 I/O delay override.

Certain (HP) laptops experience trouble from our port 0x80 I/O delay writes. This patch provides for a DMI based switch to the "alternate diagnostic port" 0xed (as used by some BIOSes as well) for these.

David P. Reed confirmed that port 0xed works for him and provides a proper delay. The symptoms of _not_ working are a hanging machine, with "hwclock" use being a direct trigger.

Earlier versions of this attempted to simply use udelay(2), with the 2 being a value tested to be a nicely conservative upper-bound with help from many on the linux-kernel mailinglist but that approach has two problems.

First, pre-loops_per_jiffy calibration (which is post PIT init while some implementations of the PIT are actually one of the historically problematic devices that need the delay) udelay() isn't particularly well-defined. We could initialise loops_per_jiffy conservatively (and based on CPU family so as to not unduly delay old machines) which would sort of work, but...

Second, delaying isn't the only effect that a write to port 0x80 has. It's also a PCI posting barrier which some devices may be explicitly or implicitly relying on. Alan Cox did a survey and found evidence that additionally some drivers may be racy on SMP without the bus locking outb.

Switching to an inb() makes the timing too unpredictable and as such, this DMI based switch should be the safest approach for now. Any more invasive changes should get more rigid testing first. It's moreover only very few machines with the problem and a DMI based hack seems to fit that situation.

This also introduces a command-line parameter "io_delay" to override the DMI based choice again:

	io_delay=<0x80|0xed|udelay|none>

where 0x80 means using the standard port 0x80 and 0xed means the alternate port 0xed.

All these methods can also be selected via the kernel .config, and can be runtime tuned via /proc/sys/kernel/io_delay_type (for debugging purposes).

The DMI strings from David's HP Pavilion dv9000z are in there already and we need to get/verify the DMI info from other machines with the problem, notably the HP Pavilion dv6000z.

This patch is partly based on earlier patches from Pavel Machek and David P. Reed.