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