Date: Sat, 28 Feb 2015 13:56:12 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Smith <smithi@nimnet.asn.au> Cc: Anthony Jenkins <Anthony.B.Jenkins@att.net>, freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] Message-ID: <20150228125857.D1277@besplex.bde.org> In-Reply-To: <20150227222203.P38620@sola.nimnet.asn.au> References: <20150222180817.GD27984@strugglingcoder.info> <54EB8C21.2080600@att.net> <2401337.2oUs7iAbtB@ralph.baldwin.cx> <54EF3D5D.4010106@att.net> <20150227222203.P38620@sola.nimnet.asn.au>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 28 Feb 2015, Ian Smith wrote:
> On Thu, 26 Feb 2015 10:35:57 -0500, Anthony Jenkins wrote:
> > On 02/25/2015 02:53 PM, John Baldwin wrote:
> > > On Monday, February 23, 2015 03:22:57 PM Anthony Jenkins wrote:
> [.. omissions reflecting change of subject ..]
> > >> I'd like to make the acpi_wmi(4) interface easier to use, but my backlog
> > >> of contributions I'm sitting on is only growing.
>
> > > I've been waiting to see if you were going to post an update to rev 3 of your
> > > CMOS patch after Ian's last round of feedback FWIW. Much of his feedback
> > > seemed relevant (and I know you've already accepted some other rounds of
> > > feedback on that patch before then, hence 'v3')
> > >
> > I am... I've just been stalling, mostly because it "works for me" and I
> > didn't understand some of the critiques, particularly the
> > "pessimization" one (over my head I think). I'll toss what I have out
> > there for further review; I'll shoot for today or tomorrow.
> >
> > One of the things I felt I had to do in the CMOS handler was allow ACPI
> > to perform multibyte accesses to the CMOS region, but the existing CMOS
> > read/write functions were only byte accessors, and each byte access was
> > locked. A multibyte access would lock, read/write a byte, unlock, lock,
> > read/write a byte, unlock.... So I wrote multibyte accessors (which had
> > some issues I think I corrected) and had the original RTC CMOS accessor
> > functions call the multibyte ones. The multibyte accessors performed
> > the locking, so a multibyte access would lock, read/write a byte,
> > read/write a byte..., unlock.
> >
> > I believe one of the recommendations was to "put it back the way it
> > was", which I did, along with failing any attempt by ACPIBIOS to access
> > multiple bytes.
>
> You can safely ignore the deeper and rather sideways discussion Bruce
> and I engaged in; I was bewildered by the idea of 'good pessimisations'
> too, which is why I pursued it, arguably too far, but I learned things.
"Good pessimization" = a good thing to do if you want to pessimize the
software to sell new hardware.
> However the takeaway was agreement that for multiple reasons, multibyte
> acpi_cmos access should loop on using the system rtcin() and writertc()
> as is - modulo your useful macros esp. IO_DELAY() documenting inb(0x84)
> - and that setting bit 7 on IO_RTC_ADDR, _possibly_ disabling NMI on
> some older hardware, is inadvisable. As you pointed out, PNP0B00 only
> wants to access CMOS from 0x00-0x3f anyway, so why not just mask reg
> with 0x3f while calling rtcin()/writertc() ?
I don't really like IO_NDELAY() even though it is clearer. 386BSD had
a macro for this, at least in asm code. It was named cryptically as
DUMMY_NOPS. It was sprinkled ad nauseum for 3 reasons:
- the author didn't understand some i386 complications related to the
8259 interrupt controller, and delays reduced the bugs
- some delays were actually needed
- some delays were used due to FUD.
All these inb(0x84) delays have gone away except for a few in the RTC
routines and 1 in the i8254 timer part DELAY(). The delays in the RTC
routines are probably now FUD even if they were needed in 1990. The
delay in DELAY() really is still needed, to fake DELAY(1) when the
i8254 is unavailable locked out. There the code is ifdefed for pc98
so as to use 0x5f instead of 0x84. The RTC routines never used 0x5f
or even avoided using 0x84.
The delays were more needed in 1990 than now. Now the ISA bus is behind
a PCI bus or two, or possibly faked. Accessing it tends to take about
twice as long as in a 1990's system configured with minimal ISA delays,
and modern hardware shouldn't need such long delays anyway, or can be
smarter and force them iff necessary.
> Don't worry about any extra locking; this is going to be used at boot
> and/or resume we assume; atrtc_settime() for one is called by default
> every 1800 seconds if running ntpd, and it doesn't bother holding the
> lock through multiple writertc() calls. Any (optional) user of the RTC
> as an interval timer source, particularly at high rates, will appreciate
> the existing pre-selection of last register used, as Bruce explained.
atrtc_settime() is too buggy to use except in emergency. One of its
bugs is that it is sloppy about setting times. atrtc_gettime()
(spelling?) is also sloppy about getting times. The combined error
is about 1-2 seconds. So it is useless to set the clock unless it it
has changed by more than 1 second. But in 1800 seconds, the RTC
should not have drifted more than a few milliseconds. The next of its
bugs is that it has no locking. Races from this can result in writing
its registers inconsistently. This makes witing to it unless it has
changed by more than 1 second worse than useless. Except the next
write is likely to fix it up after losing a race on the previous write.
It would be a reasonable fix to read after write to check that the
write worked. This only loses if the system crashes just after a
write that doesn't work or if another process reads the bad result
before it can be fixed.
> I'm unsure whether any stats or profiling still uses the RTC at all?
> kern.clockrate: { hz = 1000, tick = 1000, profhz = 8126, stathz = 127 }
Non power-of-2 rates for profhz and stathz on i386 indicate that the
lapic timer is being used for them (and the RTC is not used for anything
except initialization).
> So I would suggest:
>
> . reading any bytes except 0x0c is safe (though in the case of time or
> date bytes, possibly invalid if read while what the datasheet calls
> the UIP bit is set):
> #define RTC_STATUSA 0x0a /* status register A */
> #define RTCSA_TUP 0x80 /* time update, don't look now */
>
> . the only conceivable bytes permissable to allow writing are:
>
> . below 0x10: bytes 1, 3 and 5 (alarm seconds, minute, hour). While
> they're no use whilever we don't support wake on alarm and should
> also be written during non-update periods, should be safe enough.
>
> . 0x10 - 0x2f: none, unless you're prepared to copy the procedure in
> /sys/dev/nvram/nvram.c to calculate and write the checksum to 0x2e
> and 0x2f. I think we should just deny and log such access, unless
> and until someone shows log messages demonstrating a perceived need.
>
> . 0x30 through 0x3f: I guess you could allow write access, at least I
> haven't heard of anything using that area for anything, but check ..
> I recall reading that we don't use this, at least on x86:
> #define RTC_CENTURY 0x32 /* current century */
>
> . in any case, log all access, accepted or denied, at kern.notice ono.
> > acpi_rtc_cmos_handler: READ 01 address=0006 value=00000006
> > acpi_rtc_cmos_handler: READ 01 address=0004 value=00000015
> > acpi_rtc_cmos_handler: READ 01 address=0002 value=00000006
> Bruce called them ugly, but I'm happy such access is so obvious ..
Er, I would call that either debugging cruft or spamming the logs :-).
> > I think the reason behind having an ACPI CMOS handler is to give the OS
> > a say when ACPIBIOS wants to access CMOS, to prevent it from stepping on
> > the toes of an RTC CMOS driver who's also twiddling CMOS registers and
> > (presumably) knows the state of the device.
>
> Indeed. Virtually all of the state, apart from the default reset state,
> is stored in RTC status/control registers, though different consumers
> presumably know more. I haven't explored the code that may use the RTC
> as an eventtimer - albeit of quality 0.
>
> The OS needs more than a 'say'; once booted it's in charge of time and
> any functions relating to the RTC, and may choose to provide services to
> ACPI AML code, which is, far all the kernel knows, untrustworthy code;
> vendor, TLA? and user-updateable. "Let's be careful out there .."
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150228125857.D1277>