[PATCH] ACPI CMOS region support rev. 3
Ian Smith
smithi at nimnet.asn.au
Tue Sep 2 17:45:23 UTC 2014
On Fri, 29 Aug 2014 11:30:49 -0400, Anthony Jenkins wrote:
> Okay here's another revision of the patch, hopefully I made good
> progress in cleaning up the style and addressing everyone's
> concerns. Comments/criticisms welcome as always!
>
> Patches:
> * Current: http://www.qtchat.org/docs/atrtc.c-20140829.patch
> * Previous: http://www.qtchat.org/docs/atrtc.c.patch
> Changes:
> * Removed 'U' suffix from integer literals.
> * Ensured all (my) lines were <= 40 columns
> * Changed continuation indent to 4 columns.
> * Replaced is_datetime_reg() with more generic acpi_check_rtc_byteaccess() to encapsulate (deeper) validation
> of ACPI CMOS accesses.
> * Replaced some ACPI Windows-like typenames with FreeBSD typenames where possible.
> Booting my HP Envy 14 laptop and suspend/resuming shows the only ACPI CMOS accesses are byte reads of registers
> 0x06, 0x04 and 0x02:
>
> uhub4: at usbus0, port 1, addr 1 (disconnected)
> ugen0.2: <Areson> at usbus0 (disconnected)
> ums0: at uhub4, port 3, addr 1 (disconnected)
> uhid0: at uhub4, port 3, addr 1 (disconnected)
> uhub3: at usbus1, port 1, addr 1 (disconnected)
> uhub2: at usbus2, port 1, addr 1 (disconnected)
> ugen2.2: <vendor 0x7392> at usbus2 (disconnected)
> urtwn0: at uhub2, port 1, addr 2 (disconnected)
> uhub1: at usbus3, port 1, addr 1 (disconnected)
> ugen3.2: <vendor 0x0cf3> at usbus3 (disconnected)
> ubt0: at uhub1, port 4, addr 2 (disconnected)
> uhub0: at usbus4, port 1, addr 1 (disconnected)
> ugen4.2: <SuYin> at usbus4 (disconnected)
> acpi_rtc_cmos_handler: READ 01 address=0006 value=00000006
> acpi_rtc_cmos_handler: READ 01 address=0004 value=00000015
> acpi_rtc_cmos_handler: READ 01 address=0002 value=00000006
> vgapci0: child drmn0 requested pci_set_powerstate
> info: [drm] PCIE GART of 512M enabled (table at 0x0000000000040000).
> drmn0: info: WB enabled
> drmn0: info: fence driver on ring 0 use gpu addr 0x0000000020000c00 and cpu addr 0x0xfffff800b686cc00
> drmn0: info: fence driver on ring 1 use gpu addr 0x0000000020000c04 and cpu addr 0x0xfffff800b686cc04
> drmn0: info: fence driver on ring 2 use gpu addr 0x0000000020000c08 and cpu addr 0x0xfffff800b686cc08
> drmn0: info: fence driver on ring 3 use gpu addr 0x0000000020000c0c and cpu addr 0x0xfffff800b686cc0c
> drmn0: info: fence driver on ring 4 use gpu addr 0x0000000020000c10 and cpu addr 0x0xfffff800b686cc10
> info: [drm] ring test on 0 succeeded in 3 usecs
> info: [drm] ring test on 3 succeeded in 2 usecs
> info: [drm] ring test on 4 succeeded in 2 usecs
> info: [drm] ib test on ring 0 succeeded in 0 usecs
> info: [drm] ib test on ring 3 succeeded in 0 usecs
> info: [drm] ib test on ring 4 succeeded in 0 usecs
> re0: link state changed to DOWN
> psm0: system resume hook called.
> psm0: current command byte: 0065 (reinitialize).
Ok, good knowing what your HP Envy 14 wants to know; minute, hour and
day of week. You'd said that without this it immediately resumed from
suspend; I suppose that it might want to calculate (within a week) how
long it had been suspended or when when it should awaken - perhaps to do
with the wake on alarm settings in BIOS setup (?) - and that failing to
retrieve those made it decide it was best (or due) to resume. Rather
odd, when it would also need to read day, hour and minute on resuming to
compare, but apparently doesn't do so - or at least, not via ACPI ..
> Backlight still doesn't resume, but can still ssh(1) into laptop.
I'd tend to treat and pursue this as a separate problem; it's been a
common issue on a number of laptops. Maybe try freebsd-mobile@ too?
> Thanks,
> Anthony Jenkins
Before quoting some issues with your patch, I'll say what I think:
0) It would be better to leave the system rtcin() and writertc() more
or less as is, including its present I/O timing and the optimisation for
repetitive access, allowing (eg) use of RTC periodic interrupts at high
rates, as Bruce discussed (albeit dismissively :) WRT profiling. Your
IODELAY() macro is useful documentation, I had to hunt that up myself.
I'm wondering if mav@ (cc'd) has anything to say about all this?
There are 18 calls to each of rtcin() and writertc() just within
atrtc.c itself, and other callers (in 9.3 sources) in:
smithi at x200:/sys % find . -type f -exec egrep -l 'rtcin\(|writertc\(' {} \; | uniq
/sys/isa/rtc.h
/sys/mips/malta/malta_machdep.c
/sys/i386/xen/clock.c
/sys/i386/i386/machdep.c
/sys/dev/fb/vga.c
/sys/dev/acpi_support/acpi_ibm.c
/sys/dev/nvram/nvram.c
/sys/dev/fdc/fdc.c
/sys/x86/isa/atrtc.c
/sys/pc98/cbus/fdc.c
/sys/dev/fb/vga.c depends on 2 bits in RTC_EQUIPMENT which is defined in
that file - albeit for older VGA/EGA/CGA systems - as:
/* this should really be in `rtc.h' */
#define RTC_EQUIPMENT 0x14
And looking in rtc.h, these also should be clearly out of bounds for
ACPI writing - although nvram.c enables writes from RTC_DIAG to 0x7f, it
also recalculates the checksum over bytes 0x10-0x2d, stored in 0x2e and
0c2f, as some (most?) systems will _fail to boot_ on invalid checksum.
#define RTC_STATUSD 0x0d /* status register D (R) Lost Power */
#define RTC_DIAG 0x0e /* status register E - bios diagnostic
#define RTCDG_BITS "\020\010clock_battery\007ROM_cksum\006config_unit\005memory_size\004fixed_disk\003invalid_time"
#define RTC_RESET 0x0f /* status register F - reset code byte
#define RTC_FDISKETTE 0x10 /* diskette drive type in upper/lower nibble */
also maybe not good to allow messing with, once already running:
#define RTC_BASELO 0x15 /* low byte of basemem size */
#define RTC_BASEHI 0x16 /* high byte of basemem size */
#define RTC_EXTLO 0x17 /* low byte of extended mem size */
#define RTC_EXTHI 0x18 /* low byte of extended mem size */
#define RTC_CENTURY 0x32 /* current century */ /* UNUSED on x86 */
1) Subsuming rtcin() and writertc() into your multibyte loops heavily
pessimises the otherwise always single-byte accesses with an extra
function call & return, address math, masking and buffering.
-int
-rtcin(int reg)
+static void
+acpi_cmos_read(ACPI_PHYSICAL_ADDRESS address, UINT8 *buf, UINT32
buflen)
{
- u_char val;
+ UINT32 offset;
RTC_LOCK;
- if (rtc_reg != reg) {
- inb(0x84);
- outb(IO_RTC, reg);
- rtc_reg = reg;
- inb(0x84);
+ for (offset = 0; offset < buflen; ++offset) {
+ IO_DELAY();
+ outb(IO_RTC_ADDR, ((address + offset) | 0x80) & 0xFF);
+ IO_DELAY();
+ buf[offset] = inb(IO_RTC_DATA);
}
- val = inb(IO_RTC + 1);
RTC_UNLOCK;
- return (val);
}
[..]
+int
+rtcin(int reg)
+{
+ u_char val;
+
+ acpi_cmos_read(reg & 0xFF, &val, 1);
+ return (val);
+}
As Bruce also suggested, acpi_cmos_read|write should call rtcin() or
writertc() in its loop, thus providing slower I/O only to ACPI callers,
which after all are likely only on shutdown or suspend/resume anyway
and are certainly not timing-critical - as servicing interrupts is,
especially at say 8kHz, the maximum rate using a 32.768kHz source.
I'm almost certain that we should never set bit 7 on address. If it
inhibits NMI, ite shouldn't, and if it does nothing, it's useless. As
was, rtcin()/writertc() allow setting that bit (should they even?) but
no present callers seem to do so. Presumably any that did would need
to clear it again 'later'. Please correct any mistaken analysis.
2) Your acpi_check_rtc_byteaccess() only checks the first byte of any
multi-byte access. Reading 4 bytes from 0x09 WILL read 0x0c. Writing 2
bytes to 0x09 WILL clobber 0x0a, reg A; 4 bytes, reg B also (C is r/o)
static int
+acpi_check_rtc_byteaccess(int is_read, u_long addr)
+{
+ int retval = 1; /* Success */
+
+ if (is_read) {
+ /* Reading 0x0C will muck with interrupts */
+ if (addr == 0x0C)
+ retval = 0;
+ } else {
+ /* No writing RTC control regs */
+ if (addr >= 0x0A && addr <= 0x0D)
+ retval = 0;
+ }
+ return retval;
+}
Another good reason to have acpi_cmos_read() and acpi_cmos_write() loop
on calling rtcin() or writertc() for multibyte access is that then you
can then easily call acpi_check_rtc_byteaccess() per byte in the loop.
+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;
Shouldn't that be? '|| address > (64 - (width >> 3))'
Ignore gratuitous (); I'm unsure of C operator precedence.
+ if (!acpi_check_rtc_byteaccess(function == ACPI_READ, address))
+ return AE_BAD_PARAMETER;
+
+ 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;
+ }
3) Further to read/write ranges .. in atrtc.c see atrtc_settime() and
atrtc_gettime() for the procedure needed to update time/date/alarm
registers. You can't just poke any of those bytes and expect defined
results; the datasheet is pretty clear about that.
I think that until some real circumstance reveals a real need otherwise,
it shouldn't be allowing ACPI to write to anywhere below 0x10, possibly
0x18, and that all such attempts and indeed all AE_BAD_PARAMETER errors
should be logged, to see what AML code's trying to do what where when.
Reading anything but 0x0c is no problem, though statistically reading
time/date/alarm bytes without observing UIP bit protocol will return
undefined values (maybe 0xff) occasionally.
Hopefully helpful criticism. Not being a C programmer I've no skin in
the FreeBSD game, but feel perhaps strangely protective about allowing
out-of-tree code to write RTC or used CMOS bytes without kernel knowing
about it, even if we only actually depend on it at boot and on resume.
cheers, Ian
More information about the freebsd-acpi
mailing list