Date: Thu, 21 Feb 2013 07:52:37 -0700 From: Warner Losh <imp@bsdimp.com> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Warner Losh <imp@freebsd.org> Subject: Re: svn commit: r247068 - head/sys/x86/isa Message-ID: <9BB56E45-A272-46D7-AF3F-833424017F2D@bsdimp.com> In-Reply-To: <20130221225538.J1447@besplex.bde.org> References: <201302210040.r1L0e80m095140@svn.freebsd.org> <20130221225538.J1447@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Feb 21, 2013, at 5:31 AM, Bruce Evans wrote:
> On Thu, 21 Feb 2013, Warner Losh wrote:
>=20
>> Log:
>> Fix broken usage of splhigh() by removing it.
>=20
> 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
>> =
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D
>> --- 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;
>>=20
>> /* 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
>>=20
>> /* wait for time update to complete */
>> /* If RTCSA_TUP is zero, we have at least 244us before next =
update */
>=20
> 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 =3D splhigh();
>=20
> It used to do something...
>=20
>> - while (rtcin(RTC_STATUSA) & RTCSA_TUP) {
>> - splx(s);
>> - s =3D splhigh();
>> - }
>> + while (rtcin(RTC_STATUSA) & RTCSA_TUP)
>> + continue;
>=20
> 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 =3D 0;
>> ct.sec =3D readrtc(RTC_SEC);
>> ct.min =3D readrtc(RTC_MIN);
>>=20
>=20
> 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).
>=20
> 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=3Dtime 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?9BB56E45-A272-46D7-AF3F-833424017F2D>