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

[Ian Smith]

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?

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? :)

% 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?

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

 > +
 > +    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