Date: Mon, 16 Mar 2015 11:00:26 -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. 4 Message-ID: <5506F00A.3030708@att.net> In-Reply-To: <20150317001401.X22641@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>
next in thread | previous in thread | raw e-mail | index | archive | help
On 03/16/2015 09:59 AM, Ian Smith wrote:
> On Sat, 14 Mar 2015 23:40:34 -0400, Anthony Jenkins wrote:
>
> > How about this one? :-) Sorry it's a week late.
>
> +static unsigned int atrtc_verbose = 0;
> +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__)
> +
>
> A sysctl is good, but it needs to default to 1 if we're ever going to
> find out what various vendors are wanting to do with CMOS settings;
> anyone annoyed by any extra messages outside boot or suspend/resume
> could turn it off - after we've had the opportunity to get a report.
I'd actually prefer debug level to be compile-time (there's no real need
for it to be runtime), but I really don't want to add a config(5) knob
(I can of course).
> I can only reiterate part of my message before last, of March 2nd:
>
> =======
>> +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;
That should probably be
bytes = (width + 7) >>3;
or
bytes = (width + 7) / 8; /* Integer division more expensive
on x86? */
because width == 1 would result in bytes = 0.
> 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?
> =======
>
> 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).
I /believe/ the ACPI spec disallows requests for width=0. Looking at
ACPIspec50.pdf, the smallest OperationRegion that can be defined is 1
byte, and the smallest Field within an OpRegion is 1 bit, and all
accesses to an OpRegion occur via Fields (Section 19.5.96
OperationRegion (Declare Operation Region)). Doesn't hurt to check
tho. Also OpRegions operations are synchronized, so I at least don't
have to worry about AML accessing CMOS all willy-nilly.
> Sorry if I'm too much of a stickler for defensive programming ..
Oh no problem, I can handle a bit of criticism :-)
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?5506F00A.3030708>