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 will be used by a rewrite of zzz(1), such that it can be used to enter s2idle.

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

55

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