Details

Reviewers

jkim
jhb
imp
olce
obiwac

Commits

rGc5daa5a4c32c: acpi_spmc: Add system power management controller driver

Summary

Add SPMC (system power management controller) driver as acpi_spmc. This is the device which provides the LPI device D-state constraints and allows for OSPM to send S0ix/modern standby entry/exit notifications. This supports the original Intel DSM (https://uefi.org/sites/default/files/resources/Intel_ACPI_Low_Power_S0_Idle.pdf, untested), the AMD DSM (tested), and the Microsoft DSM (partially tested, getting constraints seems to only work for the AMD DSM on my machine).

For entry/exit, all the notifications supported by the platform are called. I'm not sure of which ones are necessary and on which systems exactly, but my system needs at least one of the AMD or Microsoft regular entry notifications and the Microsoft modern standby one. I don't think it's necessarily an issue to do this, and Linux adopts the same strategy.

Before entry, acpi_spmc_check_constraints is called to notify of any violated power constraints. This will use acpi_pwr_get_state added in D48386 to get current device D-states, but the code for doing that is currently disabled because the D-state code changes had to be backed out.

This is a prerequisite to s2idle on AMD, as the EC (which we can't mask out the GPE's of because it notifies us of important wake events, such as the lid opening or the power button being pressed) is very noisy until the entry notifications are called: https://patchwork.kernel.org/project/linux-pm/patch/2279758.LZ0rCGQtcH@aspire.rjw.lan/ It still is very noisy (albeit less so) but hopefully that's something a driver for the AMD PMC will solve.

This is sort of a parallel to @bwidawsk 's D17676 revision, which went with device_post_suspend/resume device methods to call SPMC's entry/exit functions. Since we'll probably also need an AMD-specific PMC that runs after device_post_suspend in the near future, I decided against doing it this way and instead simply added acpi_spmc_enter/exit function pointers on the ACPI softc which acpi_spmc sets in probe to be called in a future patch before idling in acpi_EnterSleepState.

Another solution (what I believe Linux does after a cursory check) is to make acpi_spmc responsible for calling the entry/exit functions for any other PMC's there may be. My concern for doing this is if a system has an AMD PMC but no SPMC, if such a system exists.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 70109
Build 66992: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Harbormaster completed remote builds in B69973: Diff 170007.Sun, Jan 18, 2:39 PM

fixed most of the issues highlighted. Will address the allocating-memory-in-probe-function pattern and add an interface for late suspend/early resume (or maybe call this something like pmc_enter and pmc_exit? then again maybe on some platforms the PMC isn't necessarily intended to be called after device suspend) in the future.

Let's also avoid possible bad pattern spreading by copy-pasting (incidentally, I see the same pattern in acpi_ec.c).

that's probably where I got this pattern from in fact. I will update acpi_ec.c to fix this later on

sys/dev/acpica/acpi_spmc.c
460	Actually I need to check if `acpi_EvaluateDSMTyped()` is already always freeing the result pointer if returning an error status. other uses of this function in FreeBSD don't do this, so those would need to be fixed too if not
497–502	updated this because I was told the exact ordering was important, at least for AMD. Not sure yet how true this is for Intel, but might aswell
528–529	Which diagram are you referring to actually? I don't think I've seen that one. You're right, and in fact in order to be reflective of what Windows and Linux as of f198478 does, we should even be calling the AMD DSM first.

Remove SPMC callbacks on struct acpi_softc. I will replace them with new pmc_suspend and pmc_resume methods in another revision. This leaves the acpi_spmc_suspend and acpi_spmc_resume (renamed from acpi_spmc_enter/exit) functions unused at the moment. Lmk if this is okay or if you'd rather I add these in a different revision too.

Harbormaster completed remote builds in B69988: Diff 170057.Mon, Jan 19, 4:42 PM

In D48387#1251616, @obiwac wrote:

Fix issues pointed out by olce@ and apply some suggestions. Main changes:

Change DSM order again to match Windows & Linux.

Hold DSM set info in new struct dsm_set, so we can hold revision ID and DSM name.

Print out DSM name & revision.

Tunable to change Intel DSM set revision ID to use for debugging.

Great! Thanks.

In D48387#1251621, @obiwac wrote:

fixed most of the issues highlighted. Will address the allocating-memory-in-probe-function pattern and add an interface for late suspend/early resume (or maybe call this something like pmc_enter and pmc_exit? then again maybe on some platforms the PMC isn't necessarily intended to be called after device suspend) in the future.

Let's also avoid possible bad pattern spreading by copy-pasting (incidentally, I see the same pattern in acpi_ec.c).

that's probably where I got this pattern from in fact. I will update acpi_ec.c to fix this later on

And thanks again.

In D48387#1251983, @obiwac wrote:

Lmk if this is okay or if you'd rather I add these in a different revision too.

That's fine.

sys/dev/acpica/acpi_spmc.c
263–264	I'd add some comment before the loop saying that `sc->constraint_count` can be modified inside the loop (hence using `object->Package.Count` as a guard).
484–488	Comment stale now, can be removed too.

Remove struct acpi_spmc_private and just set DSM set flags directly on softc + style(9) improvements.

Harbormaster completed remote builds in B70008: Diff 170099.Tue, Jan 20, 10:32 AM

Comment about changing sc->constraint_count and more style(9) improvements.

Harbormaster completed remote builds in B70009: Diff 170100.Tue, Jan 20, 10:37 AM

Don't ever use Microsoft DSM UUID for getting constraints, as this isn't supported (checked on Intel acpidump).

Harbormaster completed remote builds in B70010: Diff 170102.Tue, Jan 20, 10:47 AM

obiwac marked 3 inline comments as done.Tue, Jan 20, 10:50 AM

obiwac added inline comments.

sys/dev/acpica/acpi_spmc.c
359–368	I just removed it entirely because I can confirm that on Intel the Microsoft DSM UUID doesn't contain that function anyway. See: https://raw.githubusercontent.com/linuxhw/ACPI/refs/heads/master/Notebook/Framework/Laptop/Laptop%2013/717EDB7C4975 and search for `11e00d56-ce64-47ce-837b-1f898f9aa461`.
460	it is

Make hw.acpi.spmc.intel_dsm_revision sysctl operate directly on Intel DSM set struct.

Harbormaster completed remote builds in B70021: Diff 170130.Tue, Jan 20, 3:36 PM

Almost there! One bug (BUS_PROBE_DEFAULT) and one strongly suggested change (potential bug) around sc->dsm_sets |= DSM_SET_MS;, the rest is mostly cosmetic.

Also, there are occurrences of a space followed by a TAB, could you replace the space by a TAB there?

sys/dev/acpica/acpi_spmc.c
170	I'd use a symbolic function index here. I've put this one as an example, as it is 5 instead of 1 and a more crucial function (after all, we don't really care if `DSM_GET_DEVICE_CONSTRAINTS` (1) is not available, we could function without it).
172	The Microsoft's spec does not mention functions 1 & 2, so they could as well be absent, and we must avoid them. I'd use something like `DSM_MODERN_ENTRY_NOTIF` (7), instead of 1.
174	Same reasoning as above.
183	You've set `dsm_sets` on the softc and expect it to survive afterwards, so you need to force attach (or duplicate the sets logic in `acpi_spmc_attach()`).
296–300	I'd keep the previous alignment here (letting just `struct acpi_spmc_constraint *constraint` be the odd man out), but up to you.
365	Same alignment remark as above.
376–379	I'd move this comment just before `is_amd`, suppressing the ", so use the Intel one." part.

This revision now requires changes to proceed.Fri, Jan 23, 5:44 PM

obiwac added inline comments.Fri, Jan 23, 6:37 PM

sys/dev/acpica/acpi_spmc.c
170	actually this is checking for enum functions (1 means bit 0 not DSM #1). I think this is the canonical way of checking if a DSM exists at all. although we should probably (and might aswell) check that all the functions we necessarily need from a given DSM set are available here. Will do that in a bit
183	thanks, this was a misunderstanding of how this worked on my end :)
296–300	i tend to agree but i was previously told that style advised against this now and I do see in style(9) they don't align this

olce added inline comments.Fri, Jan 23, 9:49 PM

sys/dev/acpica/acpi_spmc.c
170	Ha ha... You're right. Too much wine at lunch :-p... Sorry for the noise. Checking for the necessary functions would be great (and ideally printing some message if a DSM is present but does not have the expected function), up to you.
296–300	Yeah, `style(9)` does not require it, and effectively it's not done in the few examples there, but it does not explicitly discourage it either. (More of a note for myself to ask and possibly amend `style(9)`.) Anyway, I do not insist.

Add dsms_needed mask to each DSM set definition to check the result of enum functions (acpi_DSMQuery()) against.
Default AMD DSM set revision ID to 2 as my ASL suggests it should be.
Add debug.acpi.spmc.amd_dsm_revision sysctl so we can easily test emulating Linux's behaviour of using revision ID 0 (probably won't make a different irl - at least my ASL doesn't ever check for revision ID in DSM implementations).
Return 0 in probe instead of BUS_PROBE_DEFAULT.
Fix style/comment placement.

Harbormaster completed remote builds in B70091: Diff 170298.Fri, Jan 23, 10:18 PM

obiwac marked 2 inline comments as done.Fri, Jan 23, 10:20 PM

obiwac added inline comments.

sys/dev/acpica/acpi_spmc.c
170	in the meantime I made it not accept the DSM set if it doesn't have all of the expected DSM functions. Maybe i should accept it so long as enum functions exists, and warn if not all the expected DSMs are available. I don't think anything bad can happen if you call a DSM ID that doesn't really exist.
296–300	i will align it because it bothers me too to be honest :)

acpi_spmc_check_dsm_set() function which enables DSM set so long as enum functions DSM is supported (bit 1), but warns if incomplete vs what we expect.

Harbormaster completed remote builds in B70093: Diff 170306.Fri, Jan 23, 10:37 PM

Align declarations.

I think we're completely done with this!!! I do just need to test this on my machine again since the most recent changes.

Harbormaster completed remote builds in B70095: Diff 170308.Fri, Jan 23, 10:40 PM

obiwac marked 2 inline comments as done.Fri, Jan 23, 10:40 PM

Newline after DSM set unexpected enum functions warning messages + fix DSM revision sysctl default values.

Harbormaster completed remote builds in B70108: Diff 170336.Sat, Jan 24, 5:25 PM

Typo in comments

Harbormaster completed remote builds in B70109: Diff 170337.Sat, Jan 24, 5:35 PM

Looks good!

I'd just fix a comment that is wrong (see inline comments).

(Style: I still see a few spurious whitespaces at end of lines; that could be fixed while in tree.)

sys/dev/acpica/acpi_spmc.c
241–244	So it seems that I was wrong assuming that bit 0 (1) indicates that the enumeration function itself is supported. This would be quite redundant, and the ACPI spec actually says that this bit indicates support for at least one function other than enumeration itself (function index 0). It's fine to assume the DSM is not present at all if enumeration doesn't return 1 in this bit. Nothing too important, but could you please fix the comment accordingly, to clear any confusion (and inspiration) for further readers?

This revision is now accepted and ready to land.Mon, Jan 26, 1:25 PM

will fix trailing whitespaces on commit

sys/dev/acpica/acpi_spmc.c
241–244	sure thing, will do this on commit

In D48387#1254885, @olce wrote:

(Style: I still see a few spurious whitespaces at end of lines; that could be fixed while in tree.)

My bad, whitespace is perfect, I looked up another file to conclude this... Sorry for the noise.

Closed by commit rGc5daa5a4c32c: acpi_spmc: Add system power management controller driver (authored by obiwac). · Explain WhyMon, Jan 26, 1:43 PM

This revision was automatically updated to reflect the committed changes.

obiwac marked an inline comment as done.

obiwac added a commit: rGc5daa5a4c32c: acpi_spmc: Add system power management controller driver.

This breaks the build, please fix or revert.

sys/dev/acpica/acpi.c
4621	`ACPI_SPMC` is never defined.
sys/dev/acpica/acpi_spmc.c
588	This is never used
597	This is never used

thanks for pointing this out. Have a fix, just building with ACPI_DEBUG to make sure everything is fixed...

sys/dev/acpica/acpi_spmc.c
588	these will be used in the next revision in the stack, D48735

Use eventhandler hooks instead of new acpi_pmc interface

Harbormaster completed remote builds in B70149: Diff 170479.Mon, Jan 26, 6:44 PM

oops, wrong revision

Harbormaster completed remote builds in B70151: Diff 170481.Mon, Jan 26, 6:47 PM

accepting so I can close this again manually

This revision is now accepted and ready to land.Mon, Jan 26, 7:25 PM

obiwac closed this revision.Mon, Jan 26, 7:25 PM

acpi_spmc: Add SPMC (system power management controller) driver
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 170337

share/man/man4/acpi.4

sys/conf/files

sys/dev/acpica/acpi.c

sys/dev/acpica/acpi_spmc.c

acpi_spmc: Add SPMC (system power management controller) driverClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 170337

share/man/man4/acpi.4

sys/conf/files

sys/dev/acpica/acpi.c

sys/dev/acpica/acpi_spmc.c

acpi_spmc: Add SPMC (system power management controller) driver
ClosedPublic
Actions

Revision Contents
Changeset List