Paths

Table of Contentst

asmc: add system state and board identity sysctls
AcceptedPublic
Actions

Authored by guest-seuros on Thu, Jun 25, 9:48 PM.

Details

Reviewers

Group Reviewers

apple (x86)
drivers

Summary

Add dev.asmc.0.system subtree with read-only sysctls for SMC diagnostic
and identity keys: shutdown_cause (MSSD), sleep_cause (MSSP),
thermal_status (MSAL), time_of_day (CLKT), power_state (MSPS),
board_id (RPlt), and chip_gen (RGEN).

Each sysctl is registered only if the key exists on the hardware.

Test Plan

Tested on

Mac mini 2011
Mac mini 2012
Mac mini Late 2014
Mac mini Late 2018
MacBook Pro 2017 (MacBookPro14,5)
MacBook Pro 2015 (MacBookPro11,5)
MacBook Pro 3,1 2007
MacBook Pro Late 2013 13"
MacBook Pro 16,2 / A2251
iMac 2011 27"
iMac 2015 27" 5K

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74267
Build 71150: arc lint + arc unit

Event Timeline

guest-seuros created this revision.Thu, Jun 25, 9:48 PM

Herald added a subscriber: imp. · View Herald TranscriptThu, Jun 25, 9:48 PM

guest-seuros requested review of this revision.Thu, Jun 25, 9:48 PM

Harbormaster completed remote builds in B74267: Diff 180650.Thu, Jun 25, 9:48 PM

guest-seuros added a parent revision: D57844: asmc: try PIO before MMIO to avoid false T2 detection.Thu, Jun 25, 9:48 PM

guest-seuros added a child revision: D57854: asmc: deduplicate sensor converters and cause sysctls.Thu, Jun 25, 9:58 PM

guest-seuros added projects: apple (x86), drivers.

guest-seuros edited the test plan for this revision. (Show Details)Fri, Jun 26, 8:54 PM

adrian added inline comments.Fri, Jun 26, 10:39 PM

sys/dev/asmc/asmc.c
2609	We try hard to not have magic numbers in drivers, find/create a _var.h or _reg.h to put the firmware/register values like this into. (And then we can also add documentation about what we've figured out what the values mean, eg what "battery-cuv" means :-) )

guest-seuros marked an inline comment as done.Fri, Jun 26, 11:02 PM

guest-seuros added inline comments.

sys/dev/asmc/asmc.c
2609	This is just a temporary mapping not the final form of the driver. Some values here gathered from Wikis and online knowledge. The Values that are 100% tested are extracted to sys/dev/asmc/asmcvar.h ... I still need to update the man pages once the code is tested for a while.

ngie added a subscriber: ngie.Sat, Jun 27, 7:11 PM

ngie added inline comments.

sys/dev/asmc/asmc.c
667–670	This can fail/return `NULL`. That case should be handled gracefully.
2609	This is just a temporary mapping not the final form of the driver. Some values here gathered from Wikis and online knowledge. The Values that are 100% tested are extracted to sys/dev/asmc/asmcvar.h ... I still need to update the man pages once the code is tested for a while. Could you please create a struct for this for clarity, e.g., `struct asmc_event_t { int code; const char *description; }`, and have a separate function/macro like this, e.g., `asmc_event_description`? If you do that, then all you need to do is define these things in an array then loop through to map the descriptions to the codes.
2653	Why 48? Magic numbers should be self-describing constants.
2673	Why 48?
2716	Please avoid hardcoding lengths (this is just one example).
2724	Could you please describe what the acronyms mean in more detail, even if it's just in a comment?
2733	Why `EIO`?
2747–2748	Why 8/9?
2751

ngie mentioned this in D57844: asmc: try PIO before MMIO to avoid false T2 detection.Sat, Jun 27, 7:12 PM

adrian added reviewers: apple (x86), drivers.Sat, Jun 27, 8:31 PM

adrian added inline comments.Sat, Jun 27, 10:12 PM

sys/dev/asmc/asmc.c
667–670	This can fail/return `NULL`. That case should be handled gracefully. yeah, and sigh lots of other drivers don't even bother to check either. :( But yeah, @guest-seuros let's at least print an error and return if it's NULL, just to show we can do it better-er.

address comment

Harbormaster completed remote builds in B74343: Diff 180842.Sat, Jun 27, 10:43 PM

guest-seuros marked 5 inline comments as done.Sat, Jun 27, 11:09 PM

guest-seuros added inline comments.

sys/dev/asmc/asmc.c
2609	yes.
2653	cuz 47 is copyrighted by AI. 😆 Seriously, just headroom for the longest string possible. 38 is the max, i added 10 just to be on a safe side.
2733	EIO is the correct error here. The key existence was already verified during attach with asmc_key_getinfo(). If asmc_key_read fails later, it because the SMC transaction itself failed (timeout, NACK, bus error, or an SMC reset after attach). Personally, i named this event as the SMC is pleading the Fifth and something fatal happened. 😆
2747–2748	8 = SMC key data length, 9 = +1 for NULL terminator. example "Mac-XXXX".

looks good, will compile test it later before landing, thanks!

This revision is now accepted and ready to land.Sat, Jun 27, 11:11 PM

Revision Contents
Changeset List

Path

Size

sys/

dev/

asmc/

asmc.c

239 lines

asmcvar.h

22 lines

Diff 180650

View Options

sys/dev/asmc/asmc.c