Re: [PATCH 07/13] PM: wakelock: Add /sys/power/request_state

Rafael J. Wysocki
2009-02-07T22:54:25Z

On Thursday 05 February 2009, Arve Hjønnevåg wrote:
> This is a non-blocking interface that specifies which suspend state to
> enter when no wakelocks are locked. A special state, "on", stops the
> process by locking the "main" wakelock.
>
> If early-suspend support is enabled, early suspend handlers will be called
> when a state other than "on" is written. When writing "on", the corresonding
> resume handlers are called.
>
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
> kernel/power/main.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 74 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index e2aae3e..676a8b0 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -22,6 +22,7 @@
> #include <linux/freezer.h>
> #include <linux/vmstat.h>
> #include <linux/syscalls.h>
> +#include <linux/wakelock.h>
>
> #include "power.h"
>
> @@ -388,6 +389,9 @@ static void suspend_finish(void)
>
>
> static const char * const pm_states[PM_SUSPEND_MAX] = {
> +#ifdef CONFIG_WAKELOCK
> + [PM_SUSPEND_ON] = "on",
> +#endif
> [PM_SUSPEND_STANDBY] = "standby",
> [PM_SUSPEND_MEM] = "mem",
> };
> @@ -545,6 +549,73 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
> power_attr(state);
> #endif
>
> +/**
> + * request_state - control system power state.
> + *
> + * This is similar to state, but it does not block until the system
> + * resumes, and it will try to re-enter the state until another state is
> + * requsted. Wakelocks are respected and the requested state will only
> + * be entered when no wakelocks are held. Write "on" to cancel.
> + *
> + * If CONFIG_EARLYSUSPEND is set, early_suspend hooks are called when
> + * the requested state changes to or from "on"
> + */
> +
> +#ifdef CONFIG_WAKELOCK
> +static ssize_t request_state_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + char *s = buf;
> + int i;
> +
> + for (i = 0; i < PM_SUSPEND_MAX; i++) {
> + if (pm_states[i] && valid_state(i))
> + s += sprintf(s,"%s ", pm_states[i]);
> + }
> + if (s != buf)
> + /* convert the last space to a newline */
> + *(s-1) = '\n';
> + return (s - buf);
> +}

Isn't the above duplicated code?

> +
> +static ssize_t request_state_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t n)
> +{
> + suspend_state_t state = PM_SUSPEND_ON;
> + const char * const *s;
> + char *p;
> + int len;
> + int error = -EINVAL;
> +
> + p = memchr(buf, '\n', n);
> + len = p ? p - buf : n;
> +
> + for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
> + if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
> + break;
> + }
> + if (state < PM_SUSPEND_MAX && *s)
> + if (state == PM_SUSPEND_ON || valid_state(state)) {
> + error = 0;
> +#ifdef CONFIG_EARLYSUSPEND
> + request_suspend_state(state);
> +#else

Remove this #ifdef please.

> + if (!mutex_trylock(&pm_mutex))
> + return -EBUSY;
> + requested_suspend_state = state;
> + if (state == PM_SUSPEND_ON)
> + wake_lock(&main_wake_lock);
> + else
> + wake_unlock(&main_wake_lock);

What's the main_wake_lock and what exactly is its role?

> + mutex_unlock(&pm_mutex);
> +#endif
> + }
> + return error ? error : n;
> +}
> +
> +power_attr(request_state);
> +#endif
> +
> #ifdef CONFIG_PM_TRACE
> int pm_trace_enabled;
>
> @@ -574,6 +645,9 @@ static struct attribute * g[] = {
> #ifndef CONFIG_DISABLE_SYS_POWER_STATE
> &state_attr.attr,
> #endif
> +#ifdef CONFIG_WAKELOCK
> + &request_state_attr.attr,
> +#endif
> #ifdef CONFIG_PM_TRACE
> &pm_trace_attr.attr,
> #endif

I don't think all of this is really necessary. IMO we should create bus
type-specific interfaces for putting devices into low power states at run time,
that will work along with the existing suspend-resume infrastructure,
instead of this.

Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org