power: Power device and ioctl for state transitions

[obiwac]

Summary

Create new /dev/power node with super simple ioctl for initiating sleep
state transitions.

This is meant as a generic interface to replace the ACPI- and
APM-specific interfaces. This allows for non-ACPI states to be entered,
such as s2idle when setting kern.power.suspend=s2idle.

Sponsored by: The FreeBSD Foundation

Test Plan

I am looking for feedback on this interface!

This is used in a rewrite of zzz(8), such that it can be used to enter suspend-to-idle: D56918

Will also write a manpage for this.

[markj]

Why exactly do you use a device interface rather than a sysctl? Is it just to make the functionality available to non-root users (i.e., members of the operator group)?

Do you have plans to expand the interface?

[obiwac]

I thought about making this a sysctl and am still open to that, but having this as a device node does give us room to expand the interface in the future. No specific plans, but we may e.g. want to be able to report state change events.

having this be accessible by the operator group without CTLFLAG_ANYBODY was another reason, yes

[markj]

sys/kern/subr_power.c

77

Extra newline.

78

SI_SUB_CONFIGURE is fine for this purpose.

85

I think we probably want to ensure that the device file was opened for writing, at least for this ioctl: check (fflags & FWRITE) != 0.

sys/sys/power.h

56

enums shouldn't be used in binary interfaces, I believe C doesn't provide guarantees about the width of an enum type. It should be unsigned int or uint32_t.

[obiwac]

• uint32_t for PIOTRANSITION
• check writable for PIOTRANSITION
• style

[markj]

sys/kern/subr_power.c

88

I would hoist this out of the specific case, it's a bit safer to check for FWRITE by default and bypass it for some select few ioctls.

90

Probably you want to check that trans is a valid enum member, since power_pm_suspend() does not return an error.

[obiwac]

• move FWRITE check outside switch()
• check trans is in enum power_sstate_transition

[obiwac]

sys/kern/subr_power.c

90

yeah, in fact, looking at where power_pm_suspend() is called, it probably makes sense to make the default: panic

[obiwac]

casting to enum power_sstate_transition can make uint32_t signed

[markj]

Looks ok with one nit fixed.

sys/kern/subr_power.c

91

Nitpicky, but IMO it's better to check the bounds before assigning to an enum variable, rather than implicitly assuming that the two types are the same width. Something like:

uint32_t trans;
...
trans = *(uint32_t)data;
if (trans >= POWER_SSTATE_TRANSITION_COUNT)
    return (EINVAL);
power_pm_suspend((enum power_sstate_transition)trans);

[obiwac]

Check trans bounds while still unsigned

[olce]

Overall looks good, but I have some remarks/questions given that that change now exposes the POWER_SSTATE_TRANSITION_* constants to userland.

1. I think we should consider renaming these constants, perhaps doing something as radical as POWER_SSTATE_TRANSITION_* => POWER_*, or perhaps keeping STATE (instead of SSTATE), before they are made publicly available (after which, we will have to provide them (almost) indefinitely).
2. Is this the granularity we want to expose? I'm OK with what is available right now, and we can later add more to that list in order to allow requesting *precise* states (i.e., not rely on the value of the power_*_stype variables to know which internal state we will finally go into). But just wanted to hear your thoughts.

sys/kern/subr_power.c

77

Not sure if that really matters, but SI_SUB_PSEUDO might be more appropriate.

[obiwac]

I think we should consider renaming these constants, perhaps doing something as radical as POWER_SSTATE_TRANSITION_* => POWER_*, or perhaps keeping STATE (instead of SSTATE), before they are made publicly available (after which, we will have to provide them (almost) indefinitely).

I kind of rather keep the word "transition", as this differentiates it from sleep types and they aren't really states in and of themselves (e.g. POWER_SSTATE_TRANSITION_SUSPEND could be either s2idle or s2mem). But probably we can rename this to just POWER_TRANSITION_* at least.

I am still open for other naming suggestions anyways because honestly I'm not the biggest biggest fan of these names; I can imagine they'd be kinda confusing.

Is this the granularity we want to expose? I'm OK with what is available right now, and we can later add more to that list in order to allow requesting *precise* states (i.e., not rely on the value of the power_*_stype variables to know which internal state we will finally go into). But just wanted to hear your thoughts.

I definitely think there's value in having a generic "suspend transition" which doesn't have to concern itself with if its entering s2idle or s2mem. This is not the kind of distinction a user program should concern itself with. We could definitely have a different ioctl further down the line which allows us to select a specific sleep type though (but I don't think there's as much value in this).

[obiwac]

Rebase onto D56920

[obiwac]

rebase to after D56953

[obiwac]

oops, completely screwed up the diff here!

[olce]

sys/kern/subr_power.c

90

I'd consider changing power_pm_suspend() itself to check for argument validity and return an error (it shouldn't panic, of course) that could just be propagated here.

sys/sys/power.h

56

I'd rather we use T for "Transition".

[obiwac]

Respond to olce@'s comments:

• Make power_pm_suspend() return an error code
• Don't check for transition enum validity in PIOTRANSITION handler; power_pm_suspend() now does this for us.
• SI_SUB_CONFIGURE -> SI_SUB_PSEUDO

[obiwac]

sys/kern/subr_power.c

90

yes, this is a good idea

[olce]

sys/kern/subr_power.c

92

I'd check here that the result of the conversion equals the original value, in order to detect an enum overflow. Else some very high values of cmd will be aliased to lower ones, making the call work although cmd is not supposed to be supported (and possible garbage that happened to go through).

[obiwac]

check enum overflow