From owner-freebsd-acpi@FreeBSD.ORG Fri Feb 27 14:53:16 2015 Return-Path: Delivered-To: freebsd-acpi@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 8AD4B336; Fri, 27 Feb 2015 14:53:16 +0000 (UTC) Received: from sola.nimnet.asn.au (paqi.nimnet.asn.au [115.70.110.159]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id D725F6AE; Fri, 27 Feb 2015 14:53:14 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by sola.nimnet.asn.au (8.14.2/8.14.2) with ESMTP id t1REr5SI052357; Sat, 28 Feb 2015 01:53:05 +1100 (EST) (envelope-from smithi@nimnet.asn.au) Date: Sat, 28 Feb 2015 01:53:04 +1100 (EST) From: Ian Smith To: Anthony Jenkins Subject: Re: [PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)] In-Reply-To: <54EF3D5D.4010106@att.net> Message-ID: <20150227222203.P38620@sola.nimnet.asn.au> References: <20150222180817.GD27984@strugglingcoder.info> <54EB8C21.2080600@att.net> <2401337.2oUs7iAbtB@ralph.baldwin.cx> <54EF3D5D.4010106@att.net> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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: Fri, 27 Feb 2015 14:53:16 -0000 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