Page MenuHomeFreeBSD

acpi: Abstract over ACPI Sx sleep states with `enum sleep_type`
AbandonedPublic

Authored by obiwac on Jan 30 2025, 6:26 PM.
Tags
None
Referenced Files
F132343334: D48732.diff
Thu, Oct 16, 2:10 AM
Unknown Object (File)
Sun, Oct 12, 5:24 PM
Unknown Object (File)
Thu, Oct 9, 5:34 PM
Unknown Object (File)
Thu, Oct 2, 10:57 PM
Unknown Object (File)
Thu, Oct 2, 12:33 AM
Unknown Object (File)
Tue, Sep 30, 11:36 PM
Unknown Object (File)
Tue, Sep 30, 8:41 AM
Unknown Object (File)
Fri, Sep 26, 8:21 PM

Details

Reviewers
jkim
jhb
imp
Summary

This will probably be superseded by the D52036, D52043, D52044 stack.

Borrow the idea to abstract over ACPI Sx sleep types with enum sleep_type from Ben's s2idle patch, D17675, but use this everywhere it makes sense to.

This is in preparation for an s2idle patch, which will add a new non-ACPI sleep state (STYPE_SUSPEND_TO_IDLE).

Test Plan

All still works on the Framework 13 AMD Ryzen 7040 series, though I can not make sure that S3 sleep still works.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 62158
Build 59042: arc lint + arc unit

Event Timeline

obiwac created this revision.

This looks reasonable but I'm a bit worried about potential future issues from the way we conflate sleep_type with ACPI constants. in particular with < and > comparisons. Perhaps we deal with that if/when we encounter it though.

This looks reasonable but I'm a bit worried about potential future issues from the way we conflate sleep_type with ACPI constants. in particular with < and > comparisons. Perhaps we deal with that if/when we encounter it though.

I agree, perhaps we should break from the ACPI values completely. I do want to move enum sleep_type out of dev/acpica/acpivar.h and have it be generic sleep states as described in D51591, so this would make sense.

Currently there is the following in sys/sys/power.h:

/* Sleep state */
#define POWER_SLEEP_STATE_STANDBY	0x00
#define POWER_SLEEP_STATE_SUSPEND	0x01
#define POWER_SLEEP_STATE_HIBERNATE	0x02

But maybe we want to keep these constants separate to enum sleep_type, since they're used at a higher level than enum sleep_type (e.g. when POWER_SLEEP_STATE_SUSPEND is passed to acpi_pm_func, it chooses whether to do S3 or s2idle depending on the hw.acpi.suspend_state sysctl).

But then we might want to pull the mechanism for choosing which specific sleep type we do depending on the requested sleep state out from ACPI and make hw.acpi.suspend_state generic.

What are your opinions on this? And @jkim ?

What are your opinions on this?

I don't think I have enough background to have a strong opinion, but the shape of that proposal sounds good. My biggest concern is that this would result in some surprising or hard to debug failure in the future, but since this is something that is being actively considered (e.g. WIP in D51591) I think it would be fine to conflate them for now and then iterate on it from there.

Since other things transport this info deep into the storage stack, i like the notion of abstracting them so we don't churn that too much...

I concur with @imp comment that I like the notion of abstracting them so we don't churn that too much...

Abandoned in favour of the D52036, D52043, D52036 stack.