From owner-freebsd-acpi@FreeBSD.ORG Tue Mar 17 13:05:23 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 8A683651 for ; Tue, 17 Mar 2015 13:05:23 +0000 (UTC) Received: from nm21-vm6.bullet.mail.ne1.yahoo.com (nm21-vm6.bullet.mail.ne1.yahoo.com [98.138.91.114]) (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 4B673BB2 for ; Tue, 17 Mar 2015 13:05:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=att.net; s=s1024; t=1426597345; bh=dJY2WFve6K/KC8xYUFd1Lhhn9pXd9szUncS9vKZLcGk=; h=Date:From:To:CC:Subject:References:In-Reply-To:From:Subject; b=jOk+3bTEYWdQCj4LCOASPaKWx3XTobWur9emALa0y3NkJ9shohXPZfwb2XgCDZTydLa3KaFmm4Rf7iiXvZnGcpSX6r6bAnhqvNpzTAn5Qnd8yiKESlNHDl2t7N1Zw0fELCVMI0HQ97jBbywy144RSjNjhZd4m5u7SX8F7NSF2VI= Received: from [98.138.226.177] by nm21.bullet.mail.ne1.yahoo.com with NNFMP; 17 Mar 2015 13:02:25 -0000 Received: from [98.138.84.44] by tm12.bullet.mail.ne1.yahoo.com with NNFMP; 17 Mar 2015 13:02:25 -0000 Received: from [127.0.0.1] by smtp112.mail.ne1.yahoo.com with NNFMP; 17 Mar 2015 13:02:24 -0000 X-Yahoo-Newman-Id: 993104.4951.bm@smtp112.mail.ne1.yahoo.com X-Yahoo-Newman-Property: ymail-3 X-YMail-OSG: Wi5eXLYVM1k.SMw_8q0FRChh7kwHzQDCPlecVpQOSx7yBDS UleQfnLf6_G3DLetTMmFaFMUedLj1S6po4ag7IHIW3IXfGIgCPxPxpT6Y8yJ C95BNJXSDoAtqXuknktP3JMgMNsND6dRv8XYKgaJ2p5p5Pu0rBb9.QAokJwP M3TzStihail.IYDyZ3x.dpK65P1u9J7sbNMvZhPLzCoBr8mSleeFVYcd1ApD lS9tYHUJV4qkhJztfwqD6Yvgdfjhu2eo1ZLVS6HdV7CwE53eLzy6DR8hYwnG 6b0bAfpdNhdu0N0A4Ro9v07ioYxbK8TJmzXlnEJNnlItNDOVpmpHxN.msSyf 4Ap_AA7Svu2pBXZY4R2MXTpOo5R17Nl41akms7wvpUavoTL.uekgdMWLvwdU 91SdGYOcT8qZqlB8SuPuXmA_PLEueyAb3W_B57oSTMJlqsNwy7oFldzQkzb9 Ceid4gRKbtxviz6Xrjmzc_j_zA8wTeZs_TwQ7AWGxE2LKHjasfznct4_Lp43 rbLo.MkDkoVODyE31nYCPTLOGxB_ob9gV5LZ3q6PXl8cLdXnP X-Yahoo-SMTP: OKD1keCswBBTAmAF1s00hLyKW3wE3YfSK0Eazl6b4VZG4LTqJxg- Message-ID: <550825DE.7030406@att.net> Date: Tue, 17 Mar 2015 09:02:22 -0400 From: Anthony Jenkins User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Ian Smith Subject: Re: [PATCH] ACPI CMOS region support rev. 5 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> <54F14368.4020807@att.net> <20150302002647.W42658@sola.nimnet.asn.au> <54F5E53D.1090601@att.net> <20150306025800.U46361@sola.nimnet.asn.au> <54F9D7E6.4050807@att.net> <5504FF32.3020202@att.net> <20150317001401.X22641@sola.nimnet.asn.au> <5506F00A.3030708@att.net> <5506FBE3.1000009@att.net> <20150317041624.K22641@sola.nimnet.asn.au> <55073442.5060005@att.net> <20150317222704.K22641@sola.nimnet.asn.au> In-Reply-To: <20150317222704.K22641@sola.nimnet.asn.au> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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: Tue, 17 Mar 2015 13:05:23 -0000 On 03/17/2015 08:28 AM, Ian Smith wrote: > On Mon, 16 Mar 2015 15:51:30 -0400, Anthony Jenkins wrote: > > On 03/16/2015 01:49 PM, Ian Smith wrote: > > > On Mon, 16 Mar 2015 11:50:59 -0400, Anthony Jenkins wrote: > > > > On 03/16/2015 11:00 AM, Anthony Jenkins wrote: > > > > > On 03/16/2015 09:59 AM, Ian Smith wrote: > > > > >> On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote: > > > > >>> + if (!acpi_check_rtc_byteaccess(function == ACPI_READ, address)) > > > > >>> + return AE_BAD_PARAMETER; > > > > >> acpi_check_rtc_byteaccess() needs to be called per byte of 1, 2 or 4 > > > > >> bytes - or pass it 'bytes' also, and loop over each of them within? > > > > >> ======= > > > > >> > > > > >> Otherwise (for example) a 2 byte read from 0x0b or 4 byte read from > > > > >> 0x09-0x0b will read 0x0c (clearing interrupts), or a 2 or 4 byte write > > > > >> to (say) 0x01 will also write to 0x02 and 0x04 (clobbering the time). > > > > > Right, this is an (incorrect) hybrid of a few attempts, probably from > > > > > around the time I lost my SSD and only had a single backup copy of my > > > > > work to go from. In one revision I had disallowed all multibyte > > > > > accesses (width > 8) since IMHO it was more consistent/correct with the > > > > > suggested locking. I wasn't ignoring your suggestion, just making one > > > > > or a few changes at a time (generally the simpler ones). > > > > > > > > Okay now I remember why I was reluctant to do this - suppose ACPIBIOS > > > > does a multibyte op on a set of bytes whose last byte fails > > > > acpi_check_rtc_byteaccess(). I will have already performed n-1 > > > > accesses. At one point I had a revision (acpi_check_rtc_access()?) that > > > > permitted/denied the entire request (it took the starting address and > > > > byte length), but I guess that got lost too. I'll just recreate it... > > > > > > Yep, validating all access before doing any sounds like the way to go. > > > > > > Also, bytes = width >> 3 is ok, since you then affirm !(width & 0x07), > > > so non-multiples of 8 bits are invalidated anyway. You should still > > > check that width (or bytes) > 0, even if 0 should never be passed. > > > > Oh yeah, forgot about that! > > > > > I guess the Big Kids will start playing once this hits bugzilla? :) > > > > I'm just glad I get to learn how to commit stuff to FreeBSD. > > > > Here's another iteration...comments welcome. Think I got (most) > > everything in there. I need to unit test acpi_check_rtc_access() to > > make sure it DTRT? > > You've fixed all my concerns, thanks Anthony. A couple of questions: > > +#ifndef ATRTC_VERBOSE > +#define ATRTC_VERBOSE 1 > +#endif > > Where else might ATRTC_VERBOSE be set otherwise? I'm picturing a (future?) config(5) knob, e.g. device atrtc options ATRTC_VERBOSE=1 so it can be set at compile time. > +static unsigned int atrtc_verbose = ATRTC_VERBOSE; > +SYSCTL_UINT(_debug, OID_AUTO, atrtc_verbose, CTLFLAG_RWTUN, > + &atrtc_verbose, 0, "AT-RTC Debug Level (0-2)"); > +#define ATRTC_DBG_PRINTF(level, format, ...) \ > + if (atrtc_verbose >= level) printf(format, ##__VA_ARGS__) > > Genuine question, I don't know .. but would not that 0 in SYSCTL_UINT > initialise atrtc_verbose to 0? Or does it keep its existing setting? > Or would replacing 0 there with ATRTC_VERBOSE work, or be clearer? Ahh, good catch... guess I don't even need the variable initializer. > static int > +acpi_check_rtc_access(int is_read, u_long addr, u_long len) > +{ > + int retval = 1; /* Success */ > + > + if (is_read) { > + /* Reading 0x0C will muck with interrupts */ > + if (addr + len - 1 >= 0x0C && addr <= 0x0c) > + retval = 0; > > Looks alright to me, given my uncertainty with C operator precedence. > > + } else { > + /* Allow single-byte writes to alarm registers and > + * addr >= 0x30, else deny. > + */ > + if (!((len == 1 && (addr <= 5 && (addr & 1))) || addr >= 0x30)) > + retval = 0; > + } > + return retval; > +} > > After a short struggle unwinding brackets, this looks right; but I will > suggest clarifying the comment: > > + /* Allow single-byte writes to alarm registers and > - + * addr >= 0x30, else deny. > + + * multi-byte writes to addr >= 0x30, else deny. Okay. > > I still wonder if there isn't a global acpi_loaded_and_running variable > so you could avoid even attempting ACPI init calls, perhaps making this > not so dependent on ACPI, at least at runtime. I haven't (yet) been able to find a compile-time flag that tells me if the kernel supports ACPI; I'm /pretty/ sure the ACPI headers I'm #include'ing will exist for every build of FreeBSD. > But perhaps jkim's concern is more regarding building on platforms not > supporting ACPI at all? Is that the (only?) issue with this on ARM? Ehh... I'll wait for him to chime in. I could try cross-compiling the kernel for an ARM box, but I doubt sys/x86/isa/atrtc.c is even picked up by those. Thanks, Anthony > cheers, Ian > _______________________________________________ > 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"