From owner-svn-src-all@freebsd.org Mon Mar 12 15:34:55 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 9EAC4F4F88E for ; Mon, 12 Mar 2018 15:34:55 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from outbound1b.ore.mailhop.org (outbound1b.ore.mailhop.org [54.200.247.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 256648133E for ; Mon, 12 Mar 2018 15:34:54 +0000 (UTC) (envelope-from ian@freebsd.org) X-MHO-User: d6b70d63-260a-11e8-bb8e-b35b57339d60 X-Report-Abuse-To: https://support.duocircle.com/support/solutions/articles/5000540958-duocircle-standard-smtp-abuse-information X-Originating-IP: 67.177.211.60 X-Mail-Handler: DuoCircle Outbound SMTP Received: from ilsoft.org (unknown [67.177.211.60]) by outbound1.ore.mailhop.org (Halon) with ESMTPSA id d6b70d63-260a-11e8-bb8e-b35b57339d60; Mon, 12 Mar 2018 15:34:23 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id w2CFYpRP016438; Mon, 12 Mar 2018 09:34:51 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1520868891.84937.177.camel@freebsd.org> Subject: Re: svn commit: r330780 - in head/sys: amd64/include isa x86/isa From: Ian Lepore To: Konstantin Belousov Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Date: Mon, 12 Mar 2018 09:34:51 -0600 In-Reply-To: <20180311212539.GD76926@kib.kiev.ua> References: <201803111922.w2BJMwr8043084@repo.freebsd.org> <20180311195800.GC76926@kib.kiev.ua> <1520799639.84937.154.camel@freebsd.org> <20180311212539.GD76926@kib.kiev.ua> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.18.5.1 FreeBSD GNOME Team Port Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Mar 2018 15:34:55 -0000 On Sun, 2018-03-11 at 23:25 +0200, Konstantin Belousov wrote: > On Sun, Mar 11, 2018 at 02:20:39PM -0600, Ian Lepore wrote: > > > > On Sun, 2018-03-11 at 21:58 +0200, Konstantin Belousov wrote: > > > > > > On Sun, Mar 11, 2018 at 07:22:58PM +0000, Ian Lepore wrote: > > > > > > > > > > > > [...] > > > > Unfortunately, this reverts one type of wrong locking back to another. > >  The need is to prevent access to the atrtc hardware if efi is > > accessing it, and the locking I just reverted to that uses a sleepable > > mutex only protects against inittodr()/resettodr() access, but not > > against nvram(4) or if the atrtc is being used as an eventtimer (of > > course nobody uses it for that, but the driver supports it). > > > > I have some pending changes that cause the atrtc driver to just not > > attach at all if the efirtc driver attached, but they're stacked up > > behind some other changes in phab.  And that still doesn't fix the > > nvram(4) part of it. > Not attaching atrtc if efirtc is attached sounds reasonable. But then > you should also disable efirt attach if atrtc is on. One possible issue > is that efirt is typicall loadable, while atrtc is compiled into the > kernel, which means that efirt would become virtually unusable. > Even if efirt is loaded (in loader(8)) the efirtc driver will still attach instead of atrtc, because it's a direct child of nexus and gets the first opportunity to probe and attach.  But you raise a good point, I should make it handle the case where it gets kldload'd when atrtc is already attached. > For nvram(4), you can take the atrtc_time_lock around accesses in addition > to the atrtc_lock, instead of providing exclusivity on the level of drivers > attach. It occurs to me that an even better fix for all of this would be to remove support for atrtc being an eventtimer.  That allows removing the interrupt filter handler, and then there's no need for a spinlock at all, a sleepable mutex works fine for all accesses. Does anybody really need an eventtimer that runs only at a fixed periodic rate of 32khz in 2018? -- Ian