Page MenuHomeFreeBSD

asmc: add system state and board identity sysctls
AcceptedPublic

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

Details

Reviewers
adrian
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 74343
Build 71226: arc lint + arc unit

Event Timeline

sys/dev/asmc/asmc.c
2623

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 added inline comments.
sys/dev/asmc/asmc.c
2623

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 inline comments.
sys/dev/asmc/asmc.c
676–679

This can fail/return NULL. That case should be handled gracefully.

2623

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.

2667

Why 48? Magic numbers should be self-describing constants.

2687

Why 48?

2730

Please avoid hardcoding lengths (this is just one example).

2738

Could you please describe what the acronyms mean in more detail, even if it's just in a comment?

2747

Why EIO?

2761–2762

Why 8/9?

2765
sys/dev/asmc/asmc.c
676–679

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.

guest-seuros marked 2 inline comments as done.
guest-seuros edited the test plan for this revision. (Show Details)

address comment

guest-seuros added inline comments.
sys/dev/asmc/asmc.c
2623

yes.

2667

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.

2747

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. 😆

2761–2762

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