Date: Sun, 11 Mar 2018 14:20:39 -0600 From: Ian Lepore <ian@freebsd.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r330780 - in head/sys: amd64/include isa x86/isa Message-ID: <1520799639.84937.154.camel@freebsd.org> In-Reply-To: <20180311195800.GC76926@kib.kiev.ua> References: <201803111922.w2BJMwr8043084@repo.freebsd.org> <20180311195800.GC76926@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1520799639.84937.154.camel>