Paths

Table of Contentst

acpi: Suspend-to-idle support (s2idle)
Needs RevisionPublic
Actions

Authored by obiwac on Jan 30 2025, 6:59 PM.

Details

Reviewers

jkim
jhb
imp
olce

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 67311
Build 64194: 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 ↗	(On Diff #150220)	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 ↗	(On Diff #150220)	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
714–726	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).
3434–3438	Please align the equal signs.

thanks for taking a look at this!

sys/dev/acpica/acpi.c
714–726	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).
3434–3438	they are aligned, with tabs. Phab is just displaying these wrong :)

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

sys/dev/acpica/acpi.c
3635	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.Wed, Dec 3, 12:34 PM

sys/dev/acpica/acpi.c
714–726	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.Wed, Dec 3, 2:03 PM

olce added inline comments.

sys/dev/acpica/acpi.c
67	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()`.
3394–3395	Does not compile on !x86. You need to declare `struct acpi_softc *sc;` and set it at the function's top unconditionally.
3483	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.
3526–3540	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.Wed, Dec 3, 2:03 PM

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

sys/dev/acpica/acpi.c
3434–3438	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 ;)