[PATCH] ACPI CMOS region support rev. 3 [was Re: No acpi_dell(4)]

[Anthony Jenkins]

On 03/01/2015 09:29 AM, Ian Smith wrote:
> On Fri, 27 Feb 2015 23:26:16 -0500, Anthony Jenkins wrote:
> > On 02/27/15 21:56, Bruce Evans wrote:
> > > On Sat, 28 Feb 2015, Ian Smith wrote:
>
> I'm going to contract this down to a couple of points, ok fellas?
>
> > > > Don't worry about any extra locking; this is going to be used at
> boot
> > > > and/or resume we assume;
> > > Bleah... I hate assumptions like that.
>
> So let's log everything at least while this is experimental in head,
> get lots of people using all sorts of hardware to report any
> surprises, and find out whenever else such service might be requested?

No problem adding logging.  The bit I'm confused about (unless I'm
misunderstanding the argument) is why

    # ugly pseudocode
    function original_cmos_xfer():
        lock();
        transfer byte to/from CMOS
        unlock();

    function acpi_cmos_xfer():
        foreach byte in num_bytes:
            call original_cmos_xfer(byte)
        end foreach


is preferable to

    # ugly pseudocode
    function acpi_cmos_xfer():
        lock();
        foreachbyte in num_bytes:
            transfer byte to/from CMOS
        end foreach
        unlock();

    function original_cmos_xfer():
        call acpi_cmos_xfer(num_bytes = 1);


There was no "extra locking", but I did introduce an extra function call
which would slow down CMOS accesses to the RTC for some performance
timers; I assume that's the main complaint.  To me, the former
pseudocode is "incorrect" because it allows the possibility of another
thread of execution mucking with a multibyte CMOS access by ACPIBIOS.  I
agree it's /probably/ unlikely tho.

> Sure, add a sysctl hw.acpi.cmosaccess.STFU for when we find benign
> stuff being logged at other times than boot and suspend/resume, but
> when Jo Blo reports here or in questions@ or laptop@ that her Hidden
> Dragon Kneetop won't boot / suspend / resume / whatever with these
> funny lines in /var/log/messages, we'll know what it's about without
> undertaling the sort of research you had to do to get your HP Envy
> going, eh? :)

Agreed; just saving logging tweaks for last.

>
> % sysctl smithi.STFU=1        # enough already ..
>
> > +static ACPI_STATUS
> > +acpi_rtc_cmos_handler(UINT32 function, ACPI_PHYSICAL_ADDRESS address,
> > +    UINT32 width, UINT64 *value, void *context, void *region_context)
> > +{
> > +    struct atrtc_softc *sc;
> > +
> > +    sc = (struct atrtc_softc *)context;
> > +    if (!value || !sc)
> > +            return AE_BAD_PARAMETER;
> > +    if (width > 32 || (width & 0x07) || address >= 64)
> > +            return AE_BAD_PARAMETER;
>
> Width 0 passes, and address 61 width 32 passes.  Maybe simpler:
>     int bytes;        /* or size, whatever, above */
>     bytes = width >> 3;
> and substitute 'bytes' subsequently, and here, perhaps:
>
>     if (bytes < 1 || bytes > 4 || (width & 0x07) || address > (64 -
> bytes))
>
> > +    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?

Thought I had done/fixed that... I'll check.

> Failing this _really_ needs a printf, as below + 'REJECTED' ono, IMO.

Agreed.

Thanks,
Anthony

>
> > +
> > +    switch (function) {
> > +            case ACPI_READ:
> > +                    acpi_cmos_read(address, (UINT8 *)value, width
> >> 3);
> > +                    break;
> > +            case ACPI_WRITE:
> > +                    acpi_cmos_write(address, (const UINT8 *)value,
> > +                        width >> 3);
> > +                    break;
> > +            default:
> > +                    return AE_BAD_PARAMETER;
> > +    }
> > +    printf("%s: %-5s%02u address=%04lx value=%08x\n",
> > +        __FUNCTION__, function == ACPI_READ ? "READ" : "WRITE",
> width >> 3,
> > +        address, *((UINT32 *)value));
> > +    return AE_OK;
> > +}
>
> cheers, Ian