Date: Tue, 5 Aug 2014 20:46:54 +1000 (EST) From: Ian Smith <smithi@nimnet.asn.au> To: Anthony Jenkins <Anthony.B.Jenkins@att.net> Cc: freebsd-acpi@freebsd.org Subject: Re: [PATCH] ACPI CMOS region support - submission? Message-ID: <20140805175541.G62404@sola.nimnet.asn.au> In-Reply-To: <53DD61BD.7050508@att.net> References: <53DD61BD.7050508@att.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 2 Aug 2014 18:10:05 -0400, Anthony Jenkins wrote:
> I made a few minor changes since the last incarnation:
>
> - Defined the CMOS address/data register addresses as macros
> - Defined the (apparent) I/O delay as a macro
Looks good, Anthony.
> I also verified the ACPI CMOS region code only accesses up to
> register 63 (0x3F - in previous emails I mistakenly said 0x7F).
Is 0x3f what the ACPI spec limits ACPI access to?
This is mildly surprising, as all modern RTC chips are at least 128
bytes; noone's actually used a MC146818 in over ten years, I gather.
> If/when this gets in, I'd like to add sysctl controls to e.g. allow
> ACPI access to the date/time registers (I currently return failure
> when attempting to write them via ACPI). I don't see anything in the
> spec (after re-reading it) that disallows ACPI from touching those.
I don't know about the spec, but I can't see allowing ACPI write access
to time/alarm/date registers as being a sensible idea; this could allow
completely out-of-band modification of time/alarm/date regs without the
necessary precautions needed for setting these, namely use of the SET
bit in status reg B (0x0b) to disable updates while setting time & date,
and anyway why allow changing time/date without the OS knowing about it?
Reading should be no problem, though without proper observance of UIP
bit timing, data may not be consistent.
I guess there may be a case for messing with the alarm bytes, though we
don't currently allow use of the alarm for wake from poweroff (S5) or
STR (S3) states, as doth Linux. I looked into this some years ago when
there were a few requests for the feature, however it would involve some
major reworking to always preserve the alarm interrupt bit through init
and suspend/resume, and appropriate clearing of same after use, along
with a utility to setup the alarm .. a potential to leave open, perhaps?
Which leads to my other concern here: that you are allowing out-of-band
access to especially status reg C (0x0c), which when read clears all
enabled interrupts, and the other status regs whose settings are also
too important to allow screwing with.
We used to use the RTC periodic interrupt as a clock source for a 128Hz
interrupt (1024Hz while profiling) but since mav@'s reworking of all the
clocks and timers sometime prior to 9.0-REL (IIRC), that's now rarely if
ever used as such, but remains an available clock or timer source.
Reg A (0x0a) is r/w and sets clock divider and rate selection bits. Do
we want ACPI to be able to mess with these? I think not. Readonly, ok.
Reg B (0x0b) is r/w and contains the aforementioned SET bit, the three
interrupt enable bits (PIE, AIE, UIE), the SQWE, DM, 24/12 and DSE bits,
again none of which should be allowed to be written to out-of-band.
And reg C (0x0c) is read-only, and as mentioned clears interrupts; again
not something that should be permitted without the OS knowing about it.
I suppose you have the MC146818 spec to hand?
Couple of things from your patch:
-static int rtc_reg = -1;
Indeed; I never could figure out the advantage in this, as how rarely
would you want to read or write the same reg twice in a row anyway?
static u_char rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF;
static u_char rtc_statusb = RTCSB_24HR;
+static void acpi_cmos_read(ACPI_PHYSICAL_ADDRESS address, UINT8 *buf, UINT32 buflen)
{
+ UINT32 offset;
RTC_LOCK;
+ for (offset = 0U; offset < buflen; ++offset) {
+ IO_DELAY();
+ outb(IO_RTC_ADDR, ((address + offset) | 0x80) & 0xFF);
+ IO_DELAY();
+ buf[offset] = inb(IO_RTC_DATA);
Sorry, but I don't get '| 0x80' there, and below in acpi_cmos_write ?
+int
+rtcin(int reg)
+{
+ u_char val;
+
+ acpi_cmos_read(reg & 0xFF, &val, 1);
+void
+writertc(int reg, u_char val)
+{
+ acpi_cmos_write(reg & 0xFF, &val, 1);
+}
Reg is already masked to 0xFF here anyway, and AFAICT every other access
system-wide is via rtcin() and writertc().
static int
+is_datetime_reg(ACPI_PHYSICAL_ADDRESS address)
+{
+ return address == 0x00 ||
+ address == 0x02 ||
+ address == 0x04 ||
+ address == 0x04 ||
+ (address >= 0x06 && address <= 0x09);
+}
I guess that second 0x04 should be 0x06? But why are you limiting to
even addresses, ie time regs but not the alarm regs? If anything, the
alarm regs seem more likely something BIOS/AML may? want to know about?
+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 >= 64U)
+ return AE_BAD_PARAMETER;
Ok, address is limited to 0x3f here, though strictly address should be
less than (64 - width>>3) for multibyte access? I guess non-ACPI access
via rtcin() and writertc() will still be able to access registers, well,
from 0 to 0xff ..
+ if (function == ACPI_WRITE &&
+ (is_datetime_reg(address) ||
+ (width > 8 && address <= 0x09)))
+ return AE_BAD_PARAMETER;
I'd exclude AT LEAST 0x0a - 0x0d for write, certainly 0x0c from read.
+ printf("%s: %-5s%02u address=%04lx value=%08x\n", + __FUNCTION__,
function == ACPI_READ ? "READ" : "WRITE", + width >> 3, address,
*((UINT32 *)value));
+ return AE_OK;
The printf is good to see. Maybe you'd eventually want to limit this to
when verbose booting is on? It would be useful to see with suspend and
resume messages, or anywhere else ACPI wanted R/W access to 'CMOS' RAM?
I tried hunting down a list of what CMOS locations various systems use,
and find some anecdotal info but nothing like 'manuf X uses bytes Y-Z'.
Probably I'm missing something really major here, on recent form :)
cheers, Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140805175541.G62404>