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