Date: Tue, 17 Mar 2015 23:28:21 +1100 (EST) From: Ian Smith <smithi@nimnet.asn.au> To: Anthony Jenkins <Anthony.B.Jenkins@att.net> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support rev. 5 Message-ID: <20150317222704.K22641@sola.nimnet.asn.au> In-Reply-To: <55073442.5060005@att.net> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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?
+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?
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.
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.
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?
cheers, Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150317222704.K22641>