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
Thomas Gleixner 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.
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?
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:
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.
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.
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.
Thanks,
tglx
Signed-off-by: David P. Reed <[email protected]>
---
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 [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]
