Date: Fri, 27 Feb 2015 23:26:16 -0500 From: Anthony Jenkins <Anthony.B.Jenkins@att.net> To: Bruce Evans <brde@optusnet.com.au>, Ian Smith <smithi@nimnet.asn.au> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] Message-ID: <54F14368.4020807@att.net> In-Reply-To: <20150228125857.D1277@besplex.bde.org> References: <20150222180817.GD27984@strugglingcoder.info> <54EB8C21.2080600@att.net> <2401337.2oUs7iAbtB@ralph.baldwin.cx> <54EF3D5D.4010106@att.net> <20150227222203.P38620@sola.nimnet.asn.au> <20150228125857.D1277@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------030205080106070406040103
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
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@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe@freebsd.org"
--------------030205080106070406040103
Content-Type: text/x-patch;
name="atrtc.c.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="atrtc.c.diff"
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? */
--------------030205080106070406040103--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54F14368.4020807>