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

[Warner Losh]

On Feb 21, 2013, at 5:31 AM, Bruce Evans wrote:

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

Depends on what you mean by more :)  It is less depessimized after the nopification of splhigh().

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

That's a long time...

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

I'll replace with a critical section.

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

kbd_active is what I need to check, right? I'll fix that while here.

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

By computing a time difference, or by checking RTCSA_TUP?

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

So this isn't a problem on fast machines?

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

Yea, that would be nice, I may have to look into that, once I'm done fetching these stones.

Warner