sys/power: Generic sleep types

[obiwac]

Summary

This revision is sort of a draft :) Want to get feedback on this design.

The idea behind this is to pull out the sleep types (stype) from ACPI, as was previously being done in D48732, and to pass this sleep type to power_pm_fn instead of passing the existing sleep state. This is a little awkward because we already kinda have generic sleep states (POWER_SLEEP_STATE_*), but these are not precise enough to build upon.

I think "stype" is a poor name. Ideally we should permute the POWER_SLEEP_STATE_* and STYPE names, though idk if we can/wanna change that. Better ideas welcome. Maybe we should just replace POWER_SLEEP_STATE_* entirely if allowed.

This revision also adds generic equivalents to hw.acpi.suspend_state etc sysctls, e.g. kern.power.suspend.

I intend to have the PM backend (e.g. ACPI) report supported stypes when registering in power_pm_register in a subsequent revision, and to have a kern.power.supported_stype sysctl similar to hw.acpi.supported_sleep_state. The default values to kern.power.suspend etc will depend on the reported supported sleep states.

S1 and S2 are rolled into POWER_STYPE_STANDBY, and the hw.acpi.suspend_sx sysctl chooses which underlying ACPI state to enter
when standbying (see acpi_stype_to_sstate).

This is sort of out of scope for this revision, but currently in acpi_pm_func I'm converting the stype straight to an ACPI sleep state. The next revision will make acpi_EnterSleepState itself accept an stype. This isn't ideal but we kinda need to do this to support s2idle (which is not an ACPI S-state) in future revisions. A "proper" solution would involve pulling out a lot of the logic in acpi_EnterSleepState, maybe into sys/kern/subr_power.c, but that requires a lot of
reworking.

[obiwac]

sys/dev/acpica/acpi.c

4664–4665

maybe need to check if sstate supported before calling acpi_EnterSleepState

sys/sys/power.h

53–55

any particular reason we couldn't make this an enum?

85–87

these are public because acpi_apm needs to know which stype corresponds to standby and suspend in the APMIO_STANDBY and APMIO_SUSPEND ioctls respectively. Currently reads acpi_sc->acpi_standby/suspend_sx but this will be removed.

[obiwac]

sys/kern/subr_power.c

89

being done in D52044

[markj]

Maybe we should just replace POWER_SLEEP_STATE_* entirely if allowed.

The stype/sstate distinction here is pretty confusing. Looking at how POWER_SLEEP_STATE_* are used, I'm 98% sure it's ok to just replace that interface if that makes things cleaner.

I think "stype" is a poor name. Ideally we should permute the POWER_SLEEP_STATE_* and STYPE names, though idk if we can/wanna change that.

I think it's ok to change them. It seems to me that POWER_SLEEP_STATE_* should perhaps be named in terms of requesting a transition to particular sleep state or type, e.g., POWER_SLEEP_REQUEST_STANDBY, but that's just an offhand thought.

sys/kern/subr_power.c

91

Do we need some kind of synchronization (e.g. a mutex) for these updates?

98

sys/sys/power.h

38

Why do you need types.h?

53–55

I don't see any particular reason. It seems to me that despite the lack of a _KERNEL guard, this header is effectively kernel-only.

Is there some user-facing interfacing interface which makes use of these constants? Looking at power.h consumers, it seems not. I'd take the opportunity to slap some _KERNEL guards around this header (not necessarily in this patch specifically).

76

[obiwac]

Respond to feedback

[obiwac]

thanks for reviewing!

sys/kern/subr_power.c

91

Yep, because there are places in the ACPI code which e.g. check one of these values and then passes it on to a function call further down the line.

Is locking this value the preferred solution here, or just making sure operations using these values are atomic w.r.t. these sysctls? FWIW the current ACPI doesn't lock acpi_sc->acpi_suspend_sx when accessing it.

sys/sys/power.h

38

Was for SYSCTL_HANDLER_ARGS, but I guess params.h is what I'm supposed to be including

53–55

Yeah there's no interface which uses these, D52395

Will make POWER_SLEEP_STATE_ and POWER_PROFILE_ enums in a different revision.

[markj]

sys/sys/power.h

38

SYSCTL_HANDLER_ARGS is defined in sys/sysctl.h, so I had expected/hoped that you don't need either types.h or param.h. But sysctl.h isn't yet self-contained (for the kernel) and consumers still need to include sys/types.h to get a definition for size_t. So the original include of sys/types.h was right. sys/param.h is overkill: it's a superset of sys/types.h and in general headers should include only what they explicitly need.

The fact that sysctl.h doesn't bring in sys/types.h itself is a small bug. In general we've been moving towards making headers self-contained, but at the moment there are many exceptions. That's a separate issue though.

[obiwac]

sys/sys/power.h

38

okay, will move it back to types.h then. Should I open another revision to simply have sys/sysctl.h include sys/types.h or is there more to this than that (e.g. not including all of sys/types.h or something)?

[markj]

sys/sys/power.h

38

I think that's probably fine. Or, we can move the _types.h include and typedef at the end of the file, in the !_KERNEL block, to the top of the file.

[obiwac]

make power_sysctl_stype private

[obiwac]

sys/sys/power.h

38

Did this in D52443, but the header doesn't need sys/sysctl.h anymore.

[markj]

I'm not a big fan of the POWER_STYPE_* naming, but hopefully that can be improved and reconciled with the POWER_SLEEP_STATE_* definitions at some point.

[obiwac]

In D52036#1198920, @markj wrote:

I'm not a big fan of the POWER_STYPE_* naming, but hopefully that can be improved and reconciled with the POWER_SLEEP_STATE_* definitions at some point.

Are you mostly saying this because of the POWER_STYPE_* names themselves or because of the confusion between the terms "sleep state" and "stype"?

I like your earlier idea of naming this in term of requesting a sleep transition. Will try this out when I open the revision to turn the POWER_SLEEP_STATE defines into an enum

[markj]

In D52036#1198930, @obiwac wrote:

In D52036#1198920, @markj wrote:

I'm not a big fan of the POWER_STYPE_* naming, but hopefully that can be improved and reconciled with the POWER_SLEEP_STATE_* definitions at some point.

Are you mostly saying this because of the POWER_STYPE_* names themselves or because of the confusion between the terms "sleep state" and "stype"?

Really just the latter, though "STYPE" is a bit obscure for my taste.

I like your earlier idea of naming this in term of requesting a sleep transition. Will try this out when I open the revision to turn the POWER_SLEEP_STATE defines into an enum

Sounds good!

[obiwac]

In D52036#1198932, @markj wrote:

Really just the latter, though "STYPE" is a bit obscure for my taste.

yeah, I would like to call it sleep states but that's just gonna cause confusion I think esp with ACPI sstates

[mckusick]

Mentor approval.