From owner-svn-src-head@freebsd.org Sun Mar 11 20:20:44 2018 Return-Path: Delivered-To: svn-src-head@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 0502AF4BE6B for ; Sun, 11 Mar 2018 20:20:44 +0000 (UTC) (envelope-from ian@freebsd.org) Received: from pmta2.delivery6.ore.mailhop.org (pmta2.delivery6.ore.mailhop.org [54.200.129.228]) (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 836477CE9D for ; Sun, 11 Mar 2018 20:20:43 +0000 (UTC) (envelope-from ian@freebsd.org) X-MHO-User: 8c7d885f-2569-11e8-b951-f99fef315fd9 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 outbound2.ore.mailhop.org (Halon) with ESMTPSA id 8c7d885f-2569-11e8-b951-f99fef315fd9; Sun, 11 Mar 2018 20:19:49 +0000 (UTC) Received: from rev (rev [172.22.42.240]) by ilsoft.org (8.15.2/8.15.2) with ESMTP id w2BKKd4A014277; Sun, 11 Mar 2018 14:20:40 -0600 (MDT) (envelope-from ian@freebsd.org) Message-ID: <1520799639.84937.154.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: Sun, 11 Mar 2018 14:20:39 -0600 In-Reply-To: <20180311195800.GC76926@kib.kiev.ua> References: <201803111922.w2BJMwr8043084@repo.freebsd.org> <20180311195800.GC76926@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-head@freebsd.org X-Mailman-Version: 2.1.25 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: Sun, 11 Mar 2018 20:20:44 -0000 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: > > > > Author: ian > > Date: Sun Mar 11 19:22:58 2018 > > New Revision: 330780 > > URL: https://svnweb.freebsd.org/changeset/base/330780 > > > > Log: > >   Eliminate atrtc_time_lock, and use atrtc_lock for efirtc locking. > > > > Modified: > >   head/sys/amd64/include/efi.h > >   head/sys/isa/rtc.h > >   head/sys/x86/isa/atrtc.c > > > > Modified: head/sys/amd64/include/efi.h > > ============================================================================== > > --- head/sys/amd64/include/efi.h Sun Mar 11 19:14:01 2018 (r330779) > > +++ head/sys/amd64/include/efi.h Sun Mar 11 19:22:58 2018 (r330780) > > @@ -48,9 +48,9 @@ > >  #ifdef _KERNEL > >  #include > >   > > -#define EFI_TIME_LOCK() mtx_lock(&atrtc_time_lock); > > -#define EFI_TIME_UNLOCK() mtx_unlock(&atrtc_time_lock); > > -#define EFI_TIME_OWNED() mtx_assert(&atrtc_time_lock, MA_OWNED); > > +#define EFI_TIME_LOCK() mtx_lock_spin(&atrtc_lock); > > +#define EFI_TIME_UNLOCK() mtx_unlock_spin(&atrtc_lock); > > +#define EFI_TIME_OWNED() mtx_assert(&atrtc_lock, MA_OWNED); > I do not understand how this can work. You changed EFI_TIME_LOCK() to > spinlock.  As result, the spinlock is taken before efi_enter() is called. > efi_enter() locks the pmap and efi_lock, both of which are sleepable > mutexes.  Also, arch efirt code is allowed to take arch-specific > sleepable mutexes and perhaps some spinlocks (but currently does not). > > The first context switch when either of the listed sleepable mutexes > are contested and the thread is taken off CPU would panic.  I suspect > that some invariants check might fire earlier. > Oops, sure enough; I reverted it. I think I knew that about efi_enter() at one time, but I had completely forgotten it did any locking.  I just realized my only efi-capable machine that I tested this on has virtually all debugging and witness disabled, so I didn't really give it much of a test after all. 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. -- Ian