svn commit: r247068 - head/sys/x86/isa

[Bruce Evans]

On Thu, 21 Feb 2013, Warner Losh wrote:

> Log:
>  Fix broken usage of splhigh() by removing it.

This is more broken than before.  The splhigh() served to indicate
missing locking.

> Modified: head/sys/x86/isa/atrtc.c
> ==============================================================================
> --- head/sys/x86/isa/atrtc.c	Thu Feb 21 00:36:12 2013	(r247067)
> +++ head/sys/x86/isa/atrtc.c	Thu Feb 21 00:40:08 2013	(r247068)
> @@ -328,7 +328,6 @@ static int
> atrtc_gettime(device_t dev, struct timespec *ts)
> {
> 	struct clocktime ct;
> -	int s;
>
> 	/* Look if we have a RTC present and the time is valid */
> 	if (!(rtcin(RTC_STATUSD) & RTCSD_PWR)) {
> @@ -338,11 +337,8 @@ atrtc_gettime(device_t dev, struct times
>
> 	/* wait for time update to complete */
> 	/* If RTCSA_TUP is zero, we have at least 244us before next update */

As the comment says, this is time-critical code.  It needs to do something
to prevent it being preempted for more than 244 usec

> -	s = splhigh();

It used to do something...

> -	while (rtcin(RTC_STATUSA) & RTCSA_TUP) {
> -		splx(s);
> -		s = splhigh();
> -	}
> +	while (rtcin(RTC_STATUSA) & RTCSA_TUP)
> +		continue;

You should probably have changed this to a critical section like you did
in ppc.  Disabling hardware interrupts would be even better.

There is a problem with the "show rtc" command in ddb.  It was born
broken (racy), and the races were turned into deadlocks by adding
locking in rtcin().  So if you trace through this code, then
"show rtc" while holding the lock in rtcin() will deadlock.  It is a
bug for ddb to call any code that might try to acquire a mutex, but
"show rtc" always calls rtcin() and rtcin() always tries to aquire a
mutex.  Similar deadlocks on the i8254 lock in DELAY() are worked
around by not trying to acquire the lock in kdb mode.

> 	ct.nsec = 0;
> 	ct.sec = readrtc(RTC_SEC);
> 	ct.min = readrtc(RTC_MIN);
>

There are 8 or 9 readrtc()'s altogether.  These must be atomic, and all
within the 244 usec limit.  There is considerable danger of exceeding the
limit without even being preempted.  Each readrtc() does 1 or 4 isa bus
accesses.  I've seen a single isa bus access taking 139 usec (delayed by
PCI DMA).

Another way of doing this without any locking against preemption or
timing magic is to read the time before and after.  If it took more than
244 usec from before seeing RTCSA_TUP deasserted to after the last
readrtc(), then retry.

My version still depends on splhigh() working, but it does more
sophisticated waiting that gives a full second to read the registers
without getting more than a stale time.  The above reads an RTC value
that may be stale by almost 1 second.  My version busy-waits until
just after after the counter rolls over.  This is only suitable for
boot=time initialization.  Debugging printfs show that this never got
preempted over the last few years -- the whole operation always
completed within about 40 usec of seeing the rollover.

Linux does even more sophisticated waiting for writing the registers,
so as not to be off by another second in the write operation.

Bruce