Message-ID: <47388586.7010509@reed.com>
Date:	Mon, 12 Nov 2007 11:55:34 -0500
From:	"David P. Reed" <dpreed@...d.com>
To:	Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
CC:	Allessandro Zummo <a.zummo@...ertech.it>
Subject: [PATCH] x86: fix locking and sync bugs in x86_64 RTC code in time_64.c

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.

Signed-off-by: David P. Reed <dpreed@...d.com>
---
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
===================================================================
--- linux-2.6.orig/arch/x86/kernel/time_64.c
+++ 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);
 
     /*







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

Powered by blists - more mailing lists