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.
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(a...
[ message continues ]

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
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,
-

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
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

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

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

-

Never paste patches into mail. Also I should have checked your mail

client earlier. Thunderbird is famous for this :)
Try to apply your own patches right from the sent folder copy. They
Well the output was a simple reject when applying the patch.
Whitespace damage was for example here:
	   unsigned long flags;

20 20 09 75 6e 73  69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a
where the correct patch should read:
20 09 75 6e 73  69 67 6e 65 64 20 6c 6f 6e 67 20 66 6c 61 67 73 3b 0a
Your mailer or the paste added a second blank (0x20) at the beginning
Hot path calls to this code would be extremly stupid and are forbidden

by the Penguin Law. :)
cpu_relax is not the problem, but you actuall changed the locking:
Old code:
    spin_lock();

    do {

       ....

    } while (stupid check);

    spin_unlock();
New code:

    while (1) {

        spin_lock();

        if (useful_check) {

             spin_unlock();

             cpu_relax();

        } else

             break;

   }
So with the old code I can take out the inner loop and do what

paravirtualization in the 32 bit code does:
	spin_lock_irqsave(&rtc_lock, flags);

	result = get_wallclock();

	spin_unlock_irqrestore(&rtc_lock, flags);
Where get_wallclock() either resolves to the plain rtc code or to the

paravirtualized function depending on the boot mode of the kernel.
With your change we either would need to put lokck/unlock of rtc_lock

into each incarnation of paravirt or do some nasty hack, where we

convey the flags to the called function. Neither of this makes sense
Yeah, those files are on the radar. The cleanup branch of the x86 git

tree has a first go on this already. And I'm currently figuring out

what we can do about this ugly CMOS index hack.
Anyway I keep the locking straight and simple as it is right now, the

cpu_relax() works fine with a lock held, while we are waiting the

bunch of usecs for UIP going away.
Thanks,
	tglx

-