From owner-freebsd-acpi@FreeBSD.ORG Sat Feb 28 04:26:29 2015 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 1CA041C9 for ; Sat, 28 Feb 2015 04:26:29 +0000 (UTC) Received: from nm5-vm3.bullet.mail.gq1.yahoo.com (nm5-vm3.bullet.mail.gq1.yahoo.com [98.136.218.178]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D8767E77 for ; Sat, 28 Feb 2015 04:26:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1425097582; bh=Ez+DRs+8ss41hAHJ6/4FCZoHcSva+lwN73TV2FaGTF8=; h=Date:From:To:CC:Subject:References:In-Reply-To:From:Subject; b=TlM3THKVKHDh6joVSSUrxNHMo98TFZsVoFl8696zJEr/p7wodRGG/EKkC6iGKZWlvOHi6d8PhRSSGHPMrkRDCyLGHJpnEAaNrpVQzWWFIq2J6Y9dGTlPiXfnOw5n6O/Jn9Q/arBatxYkC8jT1JyCr+zbt9UJ9ulgEBlXT7Bxil4= Received: from [98.137.12.59] by nm5.bullet.mail.gq1.yahoo.com with NNFMP; 28 Feb 2015 04:26:22 -0000 Received: from [98.138.226.63] by tm4.bullet.mail.gq1.yahoo.com with NNFMP; 28 Feb 2015 04:26:21 -0000 Received: from [127.0.0.1] by smtp214.mail.ne1.yahoo.com with NNFMP; 28 Feb 2015 04:26:21 -0000 X-Yahoo-Newman-Id: 830181.24320.bm@smtp214.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: 4HXArZsVM1lqPWqrUQuZ3boRlYvoEncEs6b15be4.jdHqvl AaZXwO2yorBdYc1Wf20u.zhC.QKzLrO4x.60dvSG6K9LkRl0VDH83..B9ZGw 8NPwu8gZ9b55k5vjFYZia0qh9bguOoUCtVcc1e_cqUFlF6bCML.9nw3gxILq XaYQnHaNrGd2D59uGSdfjQQFWyg.QF2ryiuenGcWuSApKSlNPwtTMFtgSbZV VleDX9mmOcIYIDtgQn_5XriMiTKhwM0Px5RBhvJc0y4RCTUODUbtcA1EHnzV aSvITNkTxF7n_dLWSLmACFT6.iVus2ysiNmLrzpU8l3uuWDERvM.gTcK3_jS lc18BBD6wFIaxEs6CWhr1gcmWTa1ve2QkU5Wqhka96wB2X4tmTtUeG2aZkXX GyeH_dmX3vLy783qNZdutxznEGOGyMmX2K3pRujQT0ki3e5ep2ddfTGfTU59 yYx4xFnBA7lUG4wi03h4FEUOk_O7BwPDppKWVErbtDO0Z1NRWGLgB02VGlXh 4usul8B1.40wGQhCOgLkwTKOcAKFZQb1AgfiMxtkdDdw6IS8L8xM- X-Yahoo-SMTP: OKD1keCswBBTAmAF1s00hLyKW3wE3YfSK0Eazl6b4VZG4LTqJxg- Message-ID: <54F14368.4020807@att.net> Date: Fri, 27 Feb 2015 23:26:16 -0500 From: Anthony Jenkins User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Bruce Evans , Ian Smith Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] 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> In-Reply-To: <20150228125857.D1277@besplex.bde.org> Content-Type: multipart/mixed; boundary="------------030205080106070406040103" Cc: freebsd-acpi@freebsd.org X-BeenThere: freebsd-acpi@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: ACPI and power management development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 28 Feb 2015 04:26:29 -0000 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 #include @@ -53,9 +54,17 @@ #include #include "clock_if.h" +#include +#include +#include + #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--