Re: [PATCH] x86: on x86_64, correct reading of PC RTC when update in progress in time_64.c

[Thomas Gleixner <tglx@linutronix.de>]

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);
> 
>  	/*
> 
>