Page MenuHomeFreeBSD

Add ACPI support for RTC/CMOS device
ClosedPublic

Authored by wulf on Feb 24 2019, 12:40 AM.

Details

Summary

This is an attempt to resurrect PR/207419 by Anthony Jenkins [1]

Implementation of ACPI RTC/CMOS interface.

FreeBSD base system does not provide an ACPI handler for the PC/AT RTC/CMOS device with PnP ID PNP0B00; on recent HP laptops, the absence of this handler causes suspend/resume and poweroff(8) to hang/fail, emitting e.g. [2], [3]

ACPI Error: No handler for Region [RCM0] (0xfffff80005796480) [SystemCMOS] (20150818/evregion-176)
ACPI Error: Region SystemCMOS (ID=5) has no handler (20150818/exfldio-317)

These HP laptops have BIOSes which attempt to query the RTC date/time registers via ACPI before suspending/powering off.

ACPI-5.0 specification section 9.15 describes the ACPI interface to the RTC/CMOS device (atrtc(4) in FreeBSD).

Original patch has been discussed in several threads on acpi@ mailing list in 2013-2015 e.g. [4] that finally led to fixing most of concerns. Now I am trying to fix remaining ones.
Improvements over PR version of the patch:

  • ACPI and ISA atrtc(4) drivers do not share common device_t structure anymore so they got different attach/detach methods and ACPI CMOS handler is installed from ACPI driver only. That should help to avoid any problems with ACPI-less installs;
  • ACPI - related code is compiled out if ACPI support is disabled in kernel config. ACPI version of atrtc(4) is compiled out in that case too;
  • Added a knob to enable/disable CMOS handler in ACPI subsystem-friendly way via setting of debug.acpi.disabled loader tunable;
  • CMOS reads and writes are logged through ACPI_VPRINT facility so they do not generate spam on ttyv0 terminal screen by default;

[1] https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=207419
[2]. https://wiki.freebsd.org/Laptops/HP_Envy_6Z-1100
[3]. https://wiki.freebsd.org/Laptops/HP_Notebook_15-af104ur
[4] http://freebsd.1045724.x6.nabble.com/No-acpi-dell-4-td5991030.html
[5] https://github.com/torvalds/linux/blob/master/drivers/acpi/acpi_cmos_rtc.c

Test Plan

Check that affected HP laptops are able to suspend/resume
Check that 'ACPI Error: No handler for Region [SystemCMOS]' terminal messages are disappeared

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

I don't know much about acpi, but from the viewpoint of what I know about the atrtc driver, this looks okay. Using rtcin/out_locked() as I mentioned above would be a small performance enhancement. I don't have a ton of free time these days and may not get around to reviewing a revised diff. I'm okay with making the changes I noted above or not; either way is fine with me.

sys/x86/isa/atrtc.c
271 ↗(On Diff #54272)

This probably should be...

mtx_lock_spin(&atrtc_lock);
while (len-- > 0)
    *ptr++ = rtcin_locked(reg++);
mtx_unlock_spin(&atrtc_lock);

Likewise in rtcout_region() below using rtcout_locked().

326 ↗(On Diff #54272)

Why does this check bitwidth > 32? That seems to arbitrarily limit cmos access to a max of 4 bytes at a time. The linux driver cited in the comments with this change doens't have any such check. (FWIW, the bios's access to the cmos at susupend/resume time is likely limited to reading a single byte containing the century number to extend the 2-digit value in the rtc regs.)

This revision is now accepted and ready to land.Feb 24 2019, 7:33 PM
wulf added inline comments.
sys/x86/isa/atrtc.c
271 ↗(On Diff #54272)

Locking is intentionally de-optimized to raise interrupt handler and ntpd priorities. ACPI is a second class citizen here.

326 ↗(On Diff #54272)

Hmmm. I followed all the discussions on this patch on acpi@ list threads dated back to 2014-2015 and was not able find exact origin of this limit. Fortunately original author is subscribed to this review so I can forward this question to him:

Hi Anthony, is it necessary to check "bitwidth > 32" here? What is the reason behind this?

sys/x86/isa/atrtc.c
326 ↗(On Diff #54272)

I honestly don't recall...vaguely remember seeing a 32-bit limit somewhere (I think I had originally put it in to simplify debug printf()s), but if there is no technical reason for a limit, then I'm happy to lose it.

This revision was automatically updated to reflect the committed changes.
wulf marked an inline comment as done.