[PATCH] ACPI CMOS region support rev. 5

[Anthony Jenkins]

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 at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
> To unsubscribe, send any mail to "freebsd-acpi-unsubscribe at freebsd.org"