Details

Reviewers

jkim
jhb
imp
olce

Commits

rG7669cbd0f064: acpi: Suspend-to-idle support (s2idle)
rG38f941deb6fa: x86: Add intr_enable_src()

Summary

Add a new STYPE_SUSPEND_TO_IDLE sleep type to the enum sleep_type added by D48732.

Factor out do_standby, do_sleep, and add a new do_idle function for idling the CPU (a future patch will make this an idle loop and not just a simple cpu_idle() call). In do_idle, SCIs (interrupt 9) are enabled to allow wake events to break the CPU out of idle.

Also records all the steps made instead of just the last one in slp_state, which allows for more flexible unwinding (will be useful to not have to goto breakout if the SPMC entry call fails).

This is a prerequisite for the firmware to enter the S0ix states. When suspending to idle, the system stays in an ACPI S0 state, but the CPUs are idled and devices are suspended/resumed before and after this as they would be when entering any other sleep type (except for AWAKE and POWEROFF, of course). An entry notification to the SPMC (D48387) is needed to actually kick the firmware modern standby.

All the hw.acpi sysctls that previously supported S1-S5 now also support an s2idle option, and s2idle is always advertised as a supported option under hw.acpi.supported_sleep_states.

A lot of this borrows from Ben's patch: D17675. The main functional difference with that patch is that suspend-to-idle is a wholly separate sleep type in this one as opposed to being an alternative implementation for S3.

Test Plan

Tested on the Framework 13 AMD Ryzen 7040 series. When certain GPEs are emitted, the SCI is triggered and the CPU exits out of idle. So with this patch, the system exits s2idle very quickly after entering it. Another patch is needed to create an idle loop, where the CPU is immediately idled again if the last SCI was not for a wake event. The SPMC (D48387) entry notification lets the firmware know to be less noisy, which also alleviates this issue.

% sysctl hw.acpi.supported_sleep_state
hw.acpi.supported_sleep_state: S4 S5 s2idle

To test this out, you can set a press of the power button to suspend to idle e.g.:

% sysctl hw.acpi.power_button_state=s2idle
hw.acpi.power_button_state: S5 -> s2idle

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 62161
Build 59045: arc lint + arc unit

Event Timeline

obiwac requested review of this revision.Jan 30 2025, 6:59 PM

obiwac created this revision.

Harbormaster completed remote builds in B62161: Diff 150220.Jan 30 2025, 6:59 PM

obiwac added a parent revision: D48732: acpi: Abstract over ACPI Sx sleep states with `enum sleep_type`.Jan 30 2025, 6:59 PM

obiwac edited the summary of this revision. (Show Details)Jan 30 2025, 7:04 PM

obiwac added a child revision: D48735: acpi: Call SPMC entry/exit hooks if SPMC device is set.Jan 30 2025, 7:07 PM

obiwac edited the test plan for this revision. (Show Details)

obiwac mentioned this in D48720: acpi: Suspend-to-idle support (S0i).Jan 30 2025, 7:10 PM

shuriku_shurik.kiev.ua added a subscriber: shuriku_shurik.kiev.ua.Jan 31 2025, 9:17 AM

matias_pizarro.net added a subscriber: matias_pizarro.net.Jun 9 2025, 6:31 AM

Herald added a subscriber: ziaee. · View Herald TranscriptJun 9 2025, 6:31 AM

obiwac added a child revision: D51591: linuxkpi: Support s2idle in `pm_suspend_target_state`.Jul 28 2025, 11:48 AM

emaste added a subscriber: emaste.Jul 31 2025, 1:48 AM

emaste added inline comments.

sys/dev/acpica/acpivar.h
62	In particular, my comment in the other review about < and > comparisons against these values seems at odds with `STYPE_SUSPEND_TO_IDLE`'s location in this list.

obiwac added inline comments.Jul 31 2025, 7:17 PM

sys/dev/acpica/acpivar.h
62	Yeah, though there's not much we can do about this except avoid doing < and > comparisons.

obiwac edited parent revisions, added: D52705: acpi: Use sleep types defined in sys/power.h; removed: D48732: acpi: Abstract over ACPI Sx sleep states with `enum sleep_type`.Sep 24 2025, 12:19 PM

rebase

Harbormaster completed remote builds in B67311: Diff 162807.Sep 25 2025, 11:09 AM

(Unfinished.)

sys/dev/acpica/acpi.c
652–671	If `POWER_STYPE_HIBERNATE` is not supported, then `acpi_sleep_button_stype` will stay at `POWER_STYPE_UNKNOWN`, whereas we still could set it to `POWER_STYPE_SUSPEND_TO_IDLE`. So I would just change the upper bound of the preceding loop to `POWER_STYPE_HIBERNATE - 1` (I'm finding directly putting `POWER_STYPE_SUSPEND_TO_IDLE`/`POWER_STYPE_SUSPEND_TO_MEM` more fragile with respect to potential changes in states, and slightly less clear about the intent) and then just test if `acpi_sleep_button_stype` is `POWER_STYPE_UNKNOWN`, in which case it is set `POWER_STYPE_SUSPEND_TO_IDLE` (keeping the `acpi_supported_stypes[POWER_STYPE_SUSPEND_TO_IDLE]` guard also is less fragile, even if currently not necessary).
3309–3314	Please align the equal signs.

thanks for taking a look at this!

sys/dev/acpica/acpi.c
652–671	If POWER_STYPE_HIBERNATE is not supported, then acpi_sleep_button_stype will stay at POWER_STYPE_UNKNOWN, whereas we still could set it to POWER_STYPE_SUSPEND_TO_IDLE. Good point. Not sure I understand why the loop should only go up to `POWER_STYPE_HIBERNATE - 1` though? I do still want hibernate to be selected if s2idle is not supported. I think I should check if `sc->acpi_sleep_button_stype == POWER_STYPE_UNKNOWN \|\| sc->acpi_sleep_button_stype == POWER_STYPE_HIBERNATE` instead. In the future I will want SUSPEND_TO_IDLE to only be preferred (or even be supported at all?) if the FADT says that the platform supports S0ix. So there still will be a situation where I might prefer hibernate if supported (i.e. no S3 or S0ix).
3309–3314	they are aligned, with tabs. Phab is just displaying these wrong :)

olce added inline comments.Dec 3 2025, 12:31 PM

sys/dev/acpica/acpi.c
3507	What's the purpose of this wait? If this is not for debugging, then we should try to document it. There's also as `DELAY()` below with `acpi_sleep_delay` as the number of seconds. `acpi_sleep_delay` is set to 1 by default (see `acpi_attach()`). It may be possible to fuse both delays if the one you added is to be kept.

olce added inline comments.Dec 3 2025, 12:34 PM

sys/dev/acpica/acpi.c
652–671	Yes, I suggested reducing the loop iterations mistakenly having in mind that suspend-to-idle would always be supported (as in the current code). Having that comparison instead is fine then.

olce requested changes to this revision.Dec 3 2025, 2:03 PM

olce added inline comments.

sys/dev/acpica/acpi.c
61	Has to be conditional on `__amd64__` or `__i386__`. Does not compile on other architectures (well, the header actually exists also on powerpc, but then the functions needed do not exist in it). See also inline comments for `do_idle()`.
3269–3271	Does not compile on !x86. You need to declare `struct acpi_softc *sc;` and set it at the function's top unconditionally.
3358	You could merge both "then" for these two `if`, as given `state == ACPI_STATE_S3 \|\| state == ACPI_STATE_S4` is true, their guard is the same.
3401–3415	Both `intr_suspend()` and `intr_enable_src()` are x86-specific so: This does not compile on other architectures, and the whole `do_idle()` should be guarded, as well as the call to it. Suspend-to-idle should in fact be disallowed higher up in the stack on !X86, probably at the beginning of `acpi_EnterSleepState()`.

This revision now requires changes to proceed.Dec 3 2025, 2:03 PM

olce added inline comments.Dec 3 2025, 5:28 PM

sys/dev/acpica/acpi.c
3309–3314	Indeed. I downloaded and applied the patches locally, that's simpler. :-)

ack your review comments, I have a couple changes I need to bring out of a stash commit before I address your comments. My local commit log is getting pretty messy from all the suspend debugging stuff I'm trying ;)

sys/dev/acpica/acpi.c
3507	yeah that was just left over from debugging

Fix issues pointed out by olce@.

Harbormaster completed remote builds in B69504: Diff 168720.Mon, Dec 29, 6:06 PM

Finally got around to updating this diff @olce :)

sys/dev/acpica/acpi.c
3401–3415	Suspend-to-idle should in fact be disallowed higher up in the stack on !X86, probably at the beginning of acpi_EnterSleepState(). I've actually just removed it entirely from `acpi_supported_stypes` entirely on !x86

obiwac added a child revision: D54410: acpi: Implement s2idle loop.Mon, Dec 29, 9:04 PM

Looks good, except for one thing (see the inline comment in do_sleep()).

sys/dev/acpica/acpi.c
670	I would add a comment here or before the preceding loop that, as `POWER_STYPE_SUSPEND_TO_MEM` is lower than `POWER_STYPE_SUSPEND_TO_IDLE`, the former is preferred, this makes it easier to understand what's going on with the sleep button.
3345	It seems important to keep the original logic here. If we set `SCI_EN` now, we may mess up with the call to AcpiEnable() below (which waits for `SCI_EN` to be set by the hardware). Also, since S4 is (almost) equivalent to S5 as for hardware state and wake-up, I don't think the power button workaround is warranted in this case.
3552	Style. And so on for other tests below.
3565

This revision now requires changes to proceed.Mon, Jan 12, 1:46 PM

Fix more issues pointed out by olce@.

sys/dev/acpica/acpi.c
670	Not sure I'm understanding you here. is there something that would otherwise imply that `POWER_STYPE_SUSPEND_TO_IDLE` would be preferred to `POWER_STYPE_SUSPEND_TO_MEM`? FWIW there is already the following comment a bit above mentioning s2mem is preferred to s2idle at the moment: /* * Dispatch the default sleep type to devices. The lid switch is set * to UNKNOWN by default to avoid surprising users. The sleep button * prefers s2mem instead of s2idle at the moment as s2idle may not work * reliably on all machines. In the future, we should set this to s2idle * when ACPI_FADT_LOW_POWER_S0 is set. */ Maybe I could bring the part about s2mem vs s2idle down to the comment before the loop?
3345	Oops thanks for catching that, I didn't intend to remove that check

Looks good (if you could move the comment about the sleep button, that would be great, see inline comment).

sys/dev/acpica/acpi.c
670	I've just found this comment this morning before reading your comment... :-) Moving that part closer to the loop would be great, yes.

This revision is now accepted and ready to land.Tue, Jan 13, 8:35 AM

Move comment.

This revision now requires review to proceed.Tue, Jan 13, 4:08 PM

obiwac marked 2 inline comments as done.Tue, Jan 13, 4:33 PM

Harbormaster completed remote builds in B69844: Diff 169550.Wed, Jan 14, 12:50 AM

Harbormaster completed remote builds in B69857: Diff 169569.

This revision was not accepted when it landed; it landed in state Needs Review.Wed, Jan 14, 12:54 AM

Closed by commit rG38f941deb6fa: x86: Add intr_enable_src() (authored by obiwac). · Explain Why

This revision was automatically updated to reflect the committed changes.

obiwac added a commit: rG38f941deb6fa: x86: Add intr_enable_src().

obiwac added a commit: rG7669cbd0f064: acpi: Suspend-to-idle support (s2idle).

acpi: Suspend-to-idle support (s2idle)
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 150220

share/man/man4/acpi.4

sys/dev/acpica/acpi.c

sys/dev/acpica/acpivar.h

sys/x86/include/intr_machdep.h

sys/x86/x86/intr_machdep.c

acpi: Suspend-to-idle support (s2idle)ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 150220

share/man/man4/acpi.4

sys/dev/acpica/acpi.c

sys/dev/acpica/acpivar.h

sys/x86/include/intr_machdep.h

sys/x86/x86/intr_machdep.c

acpi: Suspend-to-idle support (s2idle)
ClosedPublic
Actions

Revision Contents
Changeset List