From owner-svn-src-head@FreeBSD.ORG Thu Feb 21 12:31:25 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 6D421FEA; Thu, 21 Feb 2013 12:31:25 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id 0DAE0A66; Thu, 21 Feb 2013 12:31:24 +0000 (UTC) Received: from c211-30-173-106.carlnfd1.nsw.optusnet.com.au (c211-30-173-106.carlnfd1.nsw.optusnet.com.au [211.30.173.106]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id r1LCVFfA009577 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 21 Feb 2013 23:31:17 +1100 Date: Thu, 21 Feb 2013 23:31:15 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Warner Losh Subject: Re: svn commit: r247068 - head/sys/x86/isa In-Reply-To: <201302210040.r1L0e80m095140@svn.freebsd.org> Message-ID: <20130221225538.J1447@besplex.bde.org> References: <201302210040.r1L0e80m095140@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.0 cv=Zty1sKHG c=1 sm=1 a=l9K4Gy_p75cA:10 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=JzwRw_2MAAAA:8 a=P-gUh28lWcoA:10 a=i8LIHgZwk8PlJfKH8WMA:9 a=CjuIK1q_8ugA:10 a=TEtd8y5WR3g2ypngnwZWYw==:117 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Feb 2013 12:31:25 -0000 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