Messages in this thread | | | Date | Thu, 15 Nov 2007 20:33:54 +0100 (CET) | From | Thomas Gleixner <> | Subject | Re: [PATCH] x86: on x86_64, correct reading of PC RTC when update in progress in time_64.c |
| |
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.
> 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 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
> Signed-off-by: David P. Reed <dpreed@reed.com> > --- > 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); > > /* > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |