Date: Sat, 28 Feb 2015 01:53:04 +1100 (EST) From: Ian Smith <smithi@nimnet.asn.au> To: Anthony Jenkins <Anthony.B.Jenkins@att.net> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] Message-ID: <20150227222203.P38620@sola.nimnet.asn.au> In-Reply-To: <54EF3D5D.4010106@att.net> References: <20150222180817.GD27984@strugglingcoder.info> <54EB8C21.2080600@att.net> <2401337.2oUs7iAbtB@ralph.baldwin.cx> <54EF3D5D.4010106@att.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
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() ?
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.
I'm unsure whether any stats or profiling still uses the RTC at all?
kern.clockrate: { hz = 1000, tick = 1000, profhz = 8126, stathz = 127 }
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 ..
> 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 .."
cheers, Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150227222203.P38620>