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@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
_______________________________________________
freebsd-acpi@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-acpi
To unsubscribe, send any mail to "freebsd-acpi-unsubscr...@freebsd.org"

Reply via email to