Date: Tue, 18 Jul 2017 19:33:03 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: Ian Lepore <ian@freebsd.org>, Bruce Evans <brde@optusnet.com.au>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r320901 - in head/sys: amd64/amd64 isa x86/isa Message-ID: <20170718180856.P1105@besplex.bde.org> In-Reply-To: <20170717164310.GM1935@kib.kiev.ua> References: <201707120242.v6C2gvDB026199@repo.freebsd.org> <20170712224803.G1991@besplex.bde.org> <1500308973.22314.89.camel@freebsd.org> <20170717164310.GM1935@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 17 Jul 2017, Konstantin Belousov wrote: > On Mon, Jul 17, 2017 at 10:29:33AM -0600, Ian Lepore wrote: >> I assumed the reason the efirt code was using the same mutex that >> protected access to the at rtc was because under the hood, the efi rt >> clock is really the same hardware too, accessed via bios code. > > EFI spec states it explicitely, e.g. in the description of the GetTime() > function of the Time Services interface: > > During runtime, if a PC-AT CMOS device is present in the platform the > caller must synchronize access to the device before calling GetTime(). The clock_lock mutex still protects individual cmos registers, provided efi uses rtcin() etc., which it apparently doesn't. The new atrtc_time_lock mutex doesn't protect even the rtc cmos registers as a group. Other subsystems can still access the registers holding only their own lock or just clock_lock. Only write accesses would be harmful. Read accesses do occur if rtcintr() is active, but these only change the index register. Even the rtc subsystem has (groups of) write accesses that are not properly locked. These are for (re)initializations. Reinitializations occur for changing the rtc rate for profiling and for resuming. There is no locking except possibly Giant for even the rtc_status[ab] images vs their registers. Giant locking might work for that, but it doesn't work for settime() calling rtc code, at least after removing Giant from settime(). If efi accesses the rtc directly, then that is just broken and sharing the new mutex doesn't help much. The new mutex only prevents contention between efi code that uses it and rtc code that use it -- that is, just the rtc timer parts of the rtc code (as the name of the new mutex suggests). The following remain unprotected: - rtc initialization code - more seriously, rtc interrupt code and rtcin()/writertc() generally. resettodr_mtx was no better. But further abuse of clock_lock might have worked. Holding clock_lock around efi_runtime->rt_gettime() calls would prevent both rtc timer code and rtc_intr() from contending. I can't find rt_gettime(). I think these sources of contention are normally inactive if efi is used, but efi can still race with rtc initialization, the cmos driver, and possibly acpi code. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170718180856.P1105>