[PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]

Sat Feb 28 04:26:29 UTC 2015

On 02/27/15 21:56, Bruce Evans wrote:
> 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.

Meh... sounds like it's bad (TM), whatever it is...

>
>> 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.
>

I don't mind pulling the inb(0x84) calls, if we're assuming FreeBSD 
won't be running on circa 1990 hardware.  I don't really want to 
maintain atrtc(4), just stick an ACPI window on it.  I can if I must though.

>> Don't worry about any extra locking; this is going to be used at boot
>> and/or resume we assume;

Bleah... I hate assumptions like that.

>> 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 reverted that part and am calling the original atrtc() CMOS byte 
accessors in a loop from my ACPI accessors.

>
>> 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 */

Right now I'm still just blocking all writes... I'll have to see if I'm 
throwing a kernel message in those cases.

No wait, I take that back, I'm allowing writes to 0x00-0x09 and 
0x0E-0xFF.  I'll adjust that.  I'm not printing any errors, just 
returning AE_BAD_PARAMETER.

>>
>>  . 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 :-).

Yeah that was for my own edification :-)  How would I make those 
tunable/quieter?

>
>> > 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 .."

Agreed.  Thanks again for reviewing this stuff, sorry for sitting on it 
so long.  I just wasn't sure how to voice my uncertainty.

Also I'm not sure what the coding standard for FreeBSD is (e.g. 
whitespace formatting).

Anthony

>
> Bruce
> _______________________________________________
> freebsd-acpi at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe at freebsd.org"

-------------- next part --------------
A non-text attachment was scrubbed...
Name: atrtc.c.diff
Type: text/x-patch
Size: 4534 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-acpi/attachments/20150227/31b74835/attachment.bin>