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

["David P. Reed" <dpreed@reed.com>]

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