[PATCH] x86: fix locking and sync bugs in x86_64 RTC code in time_64.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

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 <[email protected]>
---
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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux