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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
