Re: [RFC 13/13] PM: standby: Add sysfs attribute for modern standby transitions

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Thu, 21 Nov 2024, Antheas Kapenekakis wrote:

> Add a sysfs attribute to allow informing the kernel about the current
> standby state, those being: "active", "screen_off", "sleep", and
> "resume" (to prepare turning the display on). The final modern
> standby state DRIPS is omitted, as that is entered during the kernel
> suspend process and userspace will never see it.
> 
> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev>
> ---
>  Documentation/ABI/testing/sysfs-power | 34 ++++++++++++
>  kernel/power/main.c                   | 75 +++++++++++++++++++++++++++
>  2 files changed, 109 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
> index a3942b1036e2..eff13980cc7c 100644
> --- a/Documentation/ABI/testing/sysfs-power
> +++ b/Documentation/ABI/testing/sysfs-power
> @@ -39,6 +39,40 @@ Description:
>  		See Documentation/admin-guide/pm/sleep-states.rst for more
>  		information.
>  
> +What:		/sys/power/standby
> +Date:		November 2024
> +Contact:	Antheas Kapenekakis <lkml@antheas.dev>
> +Description:
> +		The /sys/power/standby file controls the standby state of the
> +		system. Modern S0ix capable systems can enter a set of low power
> +		states while the kernel is still active. Transitioning into those
> +		states may 1) deactivate tertiary hardware, and 2) change the
> +		presentation of the device (e.g., pulse the suspend light, turn
> +		off the keyboard backlight).
> +
> +		Available states are "active" (fully active), "screen-off" (fully
> +		active but all displays of the system are off; virtual and real),
> +		"sleep" (major userspace components have been frozen; light
> +		background tasks may still run; this state may affect the power
> +		envelope of the device). The final state is DRIPS or LSP0, where
> +		the kernel suspends, and is entered by writing "mem" to
> +		/sys/power/state. There is a secondary sleep state called "resume"
> +		that can only be entered from "sleep" and is used in certain
> +		devices to boost the Power Limit (PLx) while remaining in sleep
> +		to hasten preparing for transitioning to "active".
> +
> +		Writing one of the above strings to this file causes the system
> +		to transition into the corresponding state, by firing the
> +		corresponding firmware notifications during the transition.
> +
> +		DRIPS or LSP0 (i.e., mem "s2idle") can only be entered from the
> +		"sleep" state. If the kernel is asked to transition to DRIPS from
> +		a different state, it will transition to "sleep" and then suspend.
> +		On wakeup, the kernel will transition back to the previous state.
> +
> +		See Documentation/admin-guide/pm/standby-states.rst for more
> +		information.
> +
>  What:		/sys/power/disk
>  Date:		September 2006
>  Contact:	Rafael J. Wysocki <rjw@rjwysocki.net>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6254814d4817..4377fdaf4a8d 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -748,6 +748,80 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(state);
>  
> +#ifdef CONFIG_SUSPEND
> +/*
> + * standby - control system s2idle standby state.
> + *
> + * show() returns available standby states, which may be "active", "screen_off",
> + * "sleep" and "resume" (still in sleep but preparing to turn on display).
> + * See Documentation/admin-guide/pm/standby-states.rst for a description of
> + * what they mean.
> + *
> + * store() accepts one of those strings, translates it into the proper
> + * enumerated value, and initiates a transition to that standby state.
> + *
> + * When the system suspends, it will first enter the state "sleep", suspend,
> + * and then restore the last state before entering "sleep". I.e., if userspace
> + * is not S0ix-aware, the transitions expected by Modern Standby devices will
> + * always be performed.
> + */
> +static ssize_t standby_show(struct kobject *kobj, struct kobj_attribute *attr,
> +			  char *buf)
> +{
> +	char *s = buf;

Instead of char *, add size_t len for the offset.

Order these to reverse xmas tree.

> +	standby_state_t i;
> +	standby_state_t curr = pm_standby_state();
> +
> +	if (curr < 0)
> +		return -EBUSY;
> +
> +	for (i = PM_STANDBY_MIN; i < PM_STANDBY_MAX; i++)
> +		if (standby_states[i])
> +			s += sprintf(s, curr == i ? "[%s] " : "%s ", standby_states[i]);

Do not use sprintf() for anything new.

For sysfs, sysfs_emit_at() (or sysfs_emit()) is the correct function.

You could consider using reverse logic + continue to bring down the 
indentation level.

> +
> +	if (s != buf)
> +		/* convert the last space to a newline */
> +		*(s - 1) = '\n';
> +	return (s - buf);
> +}
> +
> +static standby_state_t decode_standby_state(const char *buf, size_t n)
> +{
> +	standby_state_t state;
> +	char *p;
> +	int len;

size_t

> +	p = memchr(buf, '\n', n);
> +	len = p ? p - buf : n;
> +
> +	for (state = PM_STANDBY_MIN; state < PM_STANDBY_MAX; state++) {
> +		const char *label = standby_states[state];
> +
> +		if (label && len == strlen(label) && !strncmp(buf, label, len))

Isn't len == strlen(label) && !strncmp(buf, label, len) same as using just
using !strcmp() ?

> +			return state;
> +	}
> +
> +	return PM_STANDBY_MAX;
> +}
> +
> +static ssize_t standby_store(struct kobject *kobj, struct kobj_attribute *attr,
> +			   const char *buf, size_t n)
> +{
> +	int error;
> +	standby_state_t state;
> +
> +	state = decode_standby_state(buf, n);
> +
> +	if (state >= PM_STANDBY_MAX)
> +		return -EINVAL;
> +
> +	error = pm_standby_transition(state);
> +	return error ? error : n;

return error ?: n

> +}
> +
> +power_attr(standby);
> +#endif
> +
>  #ifdef CONFIG_PM_SLEEP
>  /*
>   * The 'wakeup_count' attribute, along with the functions defined in
> @@ -974,6 +1048,7 @@ static struct attribute * g[] = {
>  #ifdef CONFIG_SUSPEND
>  	&mem_sleep_attr.attr,
>  	&sync_on_suspend_attr.attr,
> +	&standby_attr.attr,
>  #endif
>  #ifdef CONFIG_PM_AUTOSLEEP
>  	&autosleep_attr.attr,
> 

-- 
 i.