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.
What locking change? I didn't change how locking works in read_persistent_clock at all.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.
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 ofI 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.
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
Signed-off-by: David P. Reed <dpreed@xxxxxxxx>
---
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
@@ -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);
/*