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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[email protected]"
Index: sys/x86/isa/atrtc.c
===================================================================
--- sys/x86/isa/atrtc.c (revision 278930)
+++ sys/x86/isa/atrtc.c (working copy)
@@ -31,6 +31,7 @@
__FBSDID("$FreeBSD$");
#include "opt_isa.h"
+#include "opt_acpi.h"
#include <sys/param.h>
#include <sys/systm.h>
@@ -53,9 +54,17 @@
#include <machine/intr_machdep.h>
#include "clock_if.h"
+#include <contrib/dev/acpica/include/acpi.h>
+#include <contrib/dev/acpica/include/accommon.h>
+#include <dev/acpica/acpivar.h>
+
#define RTC_LOCK do { if (!kdb_active) mtx_lock_spin(&clock_lock); } while (0)
#define RTC_UNLOCK do { if (!kdb_active) mtx_unlock_spin(&clock_lock); } while (0)
+#define IO_DELAY() (void)inb(0x84)
+#define IO_RTC_ADDR (IO_RTC + 0)
+#define IO_RTC_DATA (IO_RTC + 1)
+
int atrtcclock_disable = 0;
static int rtc_reg = -1;
@@ -73,10 +82,10 @@
RTC_LOCK;
if (rtc_reg != reg) {
- inb(0x84);
+ IO_DELAY();
outb(IO_RTC, reg);
rtc_reg = reg;
- inb(0x84);
+ IO_DELAY();
}
val = inb(IO_RTC + 1);
RTC_UNLOCK;
@@ -89,16 +98,36 @@
RTC_LOCK;
if (rtc_reg != reg) {
- inb(0x84);
+ IO_DELAY();
outb(IO_RTC, reg);
rtc_reg = reg;
- inb(0x84);
+ IO_DELAY();
}
outb(IO_RTC + 1, val);
- inb(0x84);
+ IO_DELAY();
RTC_UNLOCK;
}
+static void
+acpi_cmos_read(ACPI_PHYSICAL_ADDRESS address, UINT8 *buf, UINT32 buflen)
+{
+ UINT32 offset;
+
+ for (offset = 0; offset < buflen; ++offset) {
+ buf[offset] = rtcin(address + offset) & 0xff;
+ }
+}
+
+static void
+acpi_cmos_write(ACPI_PHYSICAL_ADDRESS address, const UINT8 *buf, UINT32 buflen)
+{
+ UINT32 offset;
+
+ for (offset = 0; offset < buflen; ++offset) {
+ writertc(address + offset, buf[offset]);
+ }
+}
+
static __inline int
readrtc(int port)
{
@@ -161,9 +190,58 @@
struct resource *intr_res;
void *intr_handler;
struct eventtimer et;
+ ACPI_HANDLE acpi_handle; /* Handle of the PNP0B00 node */
};
static int
+acpi_check_rtc_byteaccess(int is_read, u_long addr)
+{
+ int retval = 1; /* Success */
+
+ if (is_read) {
+ /* Reading 0x0C will muck with interrupts */
+ if (addr == 0x0C)
+ retval = 0;
+ } else {
+ if (!((addr <= 0x05 && (addr & 0x01)) ||
+ (addr >= 0x30 && addr < 0x40)))
+ retval = 0;
+ }
+ return retval;
+}
+
+static ACPI_STATUS
+acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address,
+ UINT32 width, UINT64 *value, void *context, void *region_context)
+{
+ struct atrtc_softc *sc;
+
+ sc = (struct atrtc_softc *)context;
+ if (!value || !sc)
+ return AE_BAD_PARAMETER;
+ if (width > 32 || (width & 0x07) || address >= 64)
+ return AE_BAD_PARAMETER;
+ if (!acpi_check_rtc_byteaccess(function == ACPI_READ, address))
+ return AE_BAD_PARAMETER;
+
+ switch (function) {
+ case ACPI_READ:
+ acpi_cmos_read(address, (UINT8 *)value, width >> 3);
+ break;
+ case ACPI_WRITE:
+ acpi_cmos_write(address, (const UINT8 *)value,
+ width >> 3);
+ break;
+ default:
+ return AE_BAD_PARAMETER;
+ }
+ printf("%s: %-5s%02u address=%04lx value=%08x\n",
+ __FUNCTION__, function == ACPI_READ ? "READ" : "WRITE", width >> 3,
+ address, *((UINT32 *)value));
+ return AE_OK;
+}
+
+static int
rtc_start(struct eventtimer *et, sbintime_t first, sbintime_t period)
{
@@ -245,10 +323,17 @@
int i;
sc = device_get_softc(dev);
+ sc->acpi_handle = acpi_get_handle(dev);
sc->port_res = bus_alloc_resource(dev, SYS_RES_IOPORT, &sc->port_rid,
IO_RTC, IO_RTC + 1, 2, RF_ACTIVE);
if (sc->port_res == NULL)
device_printf(dev, "Warning: Couldn't map I/O.\n");
+ if (ACPI_FAILURE(AcpiInstallAddressSpaceHandler(sc->acpi_handle,
+ ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler, NULL, sc)))
+ {
+ device_printf(dev, "Error registering ACPI CMOS address space handler.\n");
+ return 0;
+ }
atrtc_start();
clock_register(dev, 1000000);
bzero(&sc->et, sizeof(struct eventtimer));
@@ -286,6 +371,15 @@
return(0);
}
+static int atrtc_detach(device_t dev)
+{
+ struct atrtc_softc *sc;
+
+ sc = device_get_softc(dev);
+ AcpiRemoveAddressSpaceHandler(sc->acpi_handle, ACPI_ADR_SPACE_CMOS, acpi_rtc_cmos_handler);
+ return bus_generic_detach(dev);
+}
+
static int
atrtc_resume(device_t dev)
{
@@ -366,7 +460,7 @@
/* Device interface */
DEVMETHOD(device_probe, atrtc_probe),
DEVMETHOD(device_attach, atrtc_attach),
- DEVMETHOD(device_detach, bus_generic_detach),
+ DEVMETHOD(device_detach, atrtc_detach),
DEVMETHOD(device_shutdown, bus_generic_shutdown),
DEVMETHOD(device_suspend, bus_generic_suspend),
/* XXX stop statclock? */
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "[email protected]"
