On Wednesday 15 April 2009, Arve Hjønnevåg wrote:> Adds /sys/power/request_state, a non-blocking interface that specifies> which suspend state to enter when no suspend blockers are active. A> special state, "on", stops the process by activating the "main" suspend> blocker.
Finally I have some time to look at this more thoroughly, sorry for the longdelay.
I'll try not to repeate the others' comments, but I guess that may not betotally avoidable.
> Signed-off-by: Arve Hjønnevåg <arve@xxxxxxxxxxx>> ---> Documentation/power/suspend-blockers.txt | 76 +++++++++> include/linux/suspend_block.h | 61 +++++++> kernel/power/Kconfig | 10 ++> kernel/power/Makefile | 1 +> kernel/power/main.c | 62 +++++++> kernel/power/power.h | 6 +> kernel/power/suspend_block.c | 257 ++++++++++++++++++++++++++++++> 7 files changed, 473 insertions(+), 0 deletions(-)> create mode 100644 Documentation/power/suspend-blockers.txt> create mode 100755 include/linux/suspend_block.h> create mode 100644 kernel/power/suspend_block.c> > diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt> new file mode 100644> index 0000000..743b870> --- /dev/null> +++ b/Documentation/power/suspend-blockers.txt> @@ -0,0 +1,76 @@> +Suspend blockers> +================> +> +A suspend_blocker prevents the system from entering suspend.
I'd say "suspend blocker", without the underscore.
Also, I'd try to provide a general explanation of how this is supposed towork here, as an introduction. Something like:
"Suspend blockers provide a mechanism allowing device drivers and userspace processes to prevent the system from entering a sleep state, such asthe ACPI S3 state.
To use a suspend blocker, a device driver needs a struct suspend_blocker objectthat has to be initialized with the help of suspend_blocker_init(). When nolonger needed, the suspend blocker must be garbage collected with the helpof suspend_blocker_destroy().
A suspend blocker is activated using suspend_block(), which prevents thesystem from entering any sleep state until the suspend blocker is deactivatedwith suspend_unblock(). Many suspend blockers may be used simultaneouslyand the system is prevented from entering sleep states as long as at least oneof them is active."
BTW, I don't really like the names "suspend_block" and "suspend_unblock".See below for possible alternatives.
> +If the suspend operation has already started when calling suspend_block on a> +suspend_blocker, it will abort the suspend operation as long it has not already> +reached the sysdev suspend stage. This means that calling suspend_block from an> +interrupt handler or a freezeable thread always works, but if you call> +block_suspend
Surely suspend_block() ?
> from a sysdev suspend handler you must also return an error from> +that handler to abort suspend.
Well, if a sysdev suspend handler returns an error code, suspend will beaborted anyway, so I'm not sure what the point in using suspend blockers fromsysdev suspend handlers really is. Perhaps it's better to say "don't usesuspend blockers from withing sysdev suspend handlers"?
> +
I'd say "For example" here.
> +Suspend blockers can be used to allow user-space to decide which keys should> +wake the full system up and turn the screen on. Use set_irq_wake or a platform> +specific api to make sure the keypad interrupt wakes up the cpu. Once the keypad> +driver has resumed, the sequence of events can look like this:> +- The Keypad driver gets an interrupt. It then calls suspend_block on the> + keypad-scan suspend_blocker and starts scanning the keypad matrix.> +- The keypad-scan code detects a key change and reports it to the input-event> + driver.> +- The input-event driver sees the key change, enqueues an event, and calls> + suspend_block on the input-event-queue suspend_blocker.> +- The keypad-scan code detects that no keys are held and calls suspend_unblock> + on the keypad-scan suspend_blocker.> +- The user-space input-event thread returns from select/poll, calls> + suspend_block on the process-input-events suspend_blocker and then calls read> + on the input-event device.> +- The input-event driver dequeues the key-event and, since the queue is now> + empty, it calls suspend_unblock on the input-event-queue suspend_blocker.> +- The user-space input-event thread returns from read. It determines that the> + key should not wake up the full system, calls suspend_unblock on the> + process-input-events suspend_blocker and calls select or poll.> +> + Key pressed Key released> + | |> +keypad-scan ++++++++++++++++++> +input-event-queue +++ +++> +process-input-events +++ +++> +> +> +Driver API> +==========> +> +A driver can use the suspend block api by adding a suspend_blocker variable to> +its state and calling suspend_blocker_init. For instance:> +struct state {> + struct suspend_blocker suspend_blocker;> +}> +> +init() {> + suspend_blocker_init(&state->suspend_blocker, "suspend-blocker-name");> +}> +> +Before freeing the memory, suspend_blocker_destroy must be called:> +> +uninit() {> + suspend_blocker_destroy(&state->suspend_blocker);> +}> +> +When the driver determines that it needs to run (usually in an interrupt> +handler) it calls suspend_block:> + suspend_block(&state->suspend_blocker);
suspend_blocker_activate() ?suspend_blocker_enable() ?suspend_blocker_start() ?
> +> +When it no longer needs to run it calls suspend_unblock:> + suspend_unblock(&state->suspend_blocker);
suspend_blocker_deactivate() ?suspend_blocker_disable() ?suspend_blocker_finish() ?
> +> +Calling suspend_block when the suspend blocker is active or suspend_unblock when> +it is not active has no effect. This allows drivers to update their state and> +call suspend suspend_block or suspend_unblock based on the result.> +For instance:> +> +if (list_empty(&state->pending_work))> + suspend_unblock(&state->suspend_blocker);> +else> + suspend_block(&state->suspend_blocker);> +> diff --git a/include/linux/suspend_block.h b/include/linux/suspend_block.h> new file mode 100755> index 0000000..7820c60> --- /dev/null> +++ b/include/linux/suspend_block.h
suspend_blockers.h perhaps?
> @@ -0,0 +1,61 @@> +/* include/linux/suspend_block.h> + *> + * Copyright (C) 2007-2009 Google, Inc.> + *> + * This software is licensed under the terms of the GNU General Public> + * License version 2, as published by the Free Software Foundation, and> + * may be copied, distributed, and modified under those terms.> + *> + * This program is distributed in the hope that it will be useful,> + * but WITHOUT ANY WARRANTY; without even the implied warranty of> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the> + * GNU General Public License for more details.> + *> + */> +> +#ifndef _LINUX_SUSPEND_BLOCK_H> +#define _LINUX_SUSPEND_BLOCK_H
SUSPEND_BLOCKERS_H ?
> +> +/* A suspend_blocker prevents the system from entering suspend when active.> + */> +> +struct suspend_blocker {> +#ifdef CONFIG_SUSPEND_BLOCK
CONFIG_SUSPEND_BLOCKERS ?
> + atomic_t flags;> + const char *name;> +#endif> +};> +> +#ifdef CONFIG_SUSPEND_BLOCK> +> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name);> +void suspend_blocker_destroy(struct suspend_blocker *blocker);> +void suspend_block(struct suspend_blocker *blocker);> +void suspend_unblock(struct suspend_blocker *blocker);> +> +/* is_blocking_suspend returns true if the suspend_blocker is currently active.> + */> +bool is_blocking_suspend(struct suspend_blocker *blocker);
suspend_blocker_is_active() ?suspend_blocker_enabled() ?
> +> +/* suspend_is_blocked can be used by generic power management code to abort> + * suspend.> + *> + * To preserve backward compatibility suspend_is_blocked returns 0 unless it> + * is called during suspend initiated from the suspend_block code.> + */> +bool suspend_is_blocked(void);> +> +#else> +> +static inline void suspend_blocker_init(struct suspend_blocker *blocker,> + const char *name) {}> +static inline void suspend_blocker_destroy(struct suspend_blocker *blocker) {}> +static inline void suspend_block(struct suspend_blocker *blocker) {}> +static inline void suspend_unblock(struct suspend_blocker *blocker) {}> +static inline bool is_blocking_suspend(struct suspend_blocker *bl) { return 0; }> +static inline bool suspend_is_blocked(void) { return 0; }> +> +#endif> +> +#endif> +> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig> index 23bd4da..9d1df13 100644> --- a/kernel/power/Kconfig> +++ b/kernel/power/Kconfig> @@ -116,6 +116,16 @@ config SUSPEND_FREEZER> > Turning OFF this setting is NOT recommended! If in doubt, say Y.> > +config SUSPEND_BLOCK> + bool "Suspend block"> + depends on PM> + select RTC_LIB
depends on RTC_LIB
select doesn't really work and I don't think it ever will.
> + default n> + ---help---> + Enable suspend_block.
Enable suspend blockers.
> When user space requests a sleep state through> + /sys/power/request_state, the requested sleep state will be entered> + when no suspend_blockers are active.
Well, if you don't want suspend blockers to block suspend started via/sys/power/state, it's worth making it crystal clear in the documentation too.
> +> config HIBERNATION> bool "Hibernation (aka 'suspend to disk')"> depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE> diff --git a/kernel/power/Makefile b/kernel/power/Makefile> index 720ea4f..29cdc9e 100644> --- a/kernel/power/Makefile> +++ b/kernel/power/Makefile> @@ -6,6 +6,7 @@ endif> obj-$(CONFIG_PM) += main.o> obj-$(CONFIG_PM_SLEEP) += console.o> obj-$(CONFIG_FREEZER) += process.o> +obj-$(CONFIG_SUSPEND_BLOCK) += suspend_block.o> obj-$(CONFIG_HIBERNATION) += swsusp.o disk.o snapshot.o swap.o user.o> > obj-$(CONFIG_MAGIC_SYSRQ) += poweroff.o> diff --git a/kernel/power/main.c b/kernel/power/main.c> index c9632f8..e4c6b20 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/suspend_block.h>> > #include "power.h"> > @@ -392,6 +393,9 @@ static void suspend_finish(void)> > > static const char * const pm_states[PM_SUSPEND_MAX] = {> +#ifdef CONFIG_SUSPEND_BLOCK> + [PM_SUSPEND_ON] = "on",> +#endif
Why #ifdef?
> [PM_SUSPEND_STANDBY] = "standby",> [PM_SUSPEND_MEM] = "mem",> };> @@ -540,6 +544,61 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,> > power_attr(state);> > +/**> + * 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> + * requested. Suspend blockers are respected and the requested state will> + * only be entered when no suspend blockers are active.> + * Write "on" to cancel.> + */> +> +#ifdef CONFIG_SUSPEND_BLOCK> +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] && (i == PM_SUSPEND_ON || 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);> +}> +> +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;> + request_suspend_state(state);> + }> + return error ? error : n;> +}> +> +power_attr(request_state);> +#endif /* CONFIG_SUSPEND_BLOCK */> +> #ifdef CONFIG_PM_TRACE> int pm_trace_enabled;> > @@ -567,6 +626,9 @@ power_attr(pm_trace);> > static struct attribute * g[] = {> &state_attr.attr,> +#ifdef CONFIG_SUSPEND_BLOCK> + &request_state_attr.attr,> +#endif> #ifdef CONFIG_PM_TRACE> &pm_trace_attr.attr,> #endif> diff --git a/kernel/power/power.h b/kernel/power/power.h> index 46b5ec7..2414a74 100644> --- a/kernel/power/power.h> +++ b/kernel/power/power.h> @@ -223,3 +223,9 @@ static inline void suspend_thaw_processes(void)> {> }> #endif> +> +#ifdef CONFIG_SUSPEND_BLOCK> +/* kernel/power/suspend_block.c */> +void request_suspend_state(suspend_state_t state);> +#endif
Is the #ifdef really necessary?
> +> diff --git a/kernel/power/suspend_block.c b/kernel/power/suspend_block.c> new file mode 100644> index 0000000..b4f2191> --- /dev/null> +++ b/kernel/power/suspend_block.c> @@ -0,0 +1,257 @@> +/* kernel/power/suspend_block.c> + *> + * Copyright (C) 2005-2008 Google, Inc.> + *> + * This software is licensed under the terms of the GNU General Public> + * License version 2, as published by the Free Software Foundation, and> + * may be copied, distributed, and modified under those terms.> + *> + * This program is distributed in the hope that it will be useful,> + * but WITHOUT ANY WARRANTY; without even the implied warranty of> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the> + * GNU General Public License for more details.> + *> + */> +> +#include <linux/module.h>> +#include <linux/rtc.h>> +#include <linux/suspend.h>> +#include <linux/suspend_block.h>> +#include <linux/sysdev.h>> +#include "power.h"> +> +enum {> + DEBUG_EXIT_SUSPEND = 1U << 0,> + DEBUG_WAKEUP = 1U << 1,> + DEBUG_USER_STATE = 1U << 2,> + DEBUG_SUSPEND = 1U << 3,> + DEBUG_SUSPEND_BLOCKER = 1U << 4,> +};> +static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);> +> +#define SB_INITIALIZED (1U << 8)> +#define SB_ACTIVE (1U << 9)> +> +static DEFINE_SPINLOCK(state_lock);> +static atomic_t suspend_block_count;> +static atomic_t current_event_num;> +struct workqueue_struct *suspend_work_queue;> +struct suspend_blocker main_suspend_blocker;> +static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;> +static bool enable_suspend_blockers;> +> +bool suspend_is_blocked(void)> +{> + if (WARN_ONCE(!enable_suspend_blockers, "ignoring suspend blockers\n"))> + return 0;> + return !!atomic_read(&suspend_block_count);> +}> +> +static void suspend_worker(struct work_struct *work)> +{> + int ret;> + int entry_event_num;> +> + enable_suspend_blockers = 1;
'true' instead of '1', please.
> +> +retry:> + if (suspend_is_blocked()) {> + if (debug_mask & DEBUG_SUSPEND)> + pr_info("suspend: abort suspend\n");> + goto abort;
If that were in a loop you could just use 'break' here.
> + }> +> + entry_event_num = atomic_read(¤t_event_num);> + if (debug_mask & DEBUG_SUSPEND)> + pr_info("suspend: enter suspend\n");> + ret = pm_suspend(requested_suspend_state);> + if (debug_mask & DEBUG_EXIT_SUSPEND) {> + struct timespec ts;> + struct rtc_time tm;> + getnstimeofday(&ts);> + rtc_time_to_tm(ts.tv_sec, &tm);> + pr_info("suspend: exit suspend, ret = %d "> + "(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n", ret,> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,> + tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec);> + }> + if (atomic_read(¤t_event_num) == entry_event_num) {> + if (debug_mask & DEBUG_SUSPEND)> + pr_info("suspend: pm_suspend returned with no event\n");> + goto retry;
Put that into a loop and use 'continue' here? Or use 'break' to go out ofthe loop?
> + }> +abort:> + enable_suspend_blockers = 0;
'false' instead of '0', please.
> +}> +static DECLARE_WORK(suspend_work, suspend_worker);> +> +static int suspend_block_suspend(struct sys_device *dev, pm_message_t state)> +{> + int ret = suspend_is_blocked() ? -EAGAIN : 0;> + if (debug_mask & DEBUG_SUSPEND)> + pr_info("suspend_block_suspend return %d\n", ret);> + return ret;> +}> +> +static struct sysdev_class suspend_block_sysclass = {> + .name = "suspend_block",> + .suspend = suspend_block_suspend,> +};> +static struct sys_device suspend_block_sysdev = {> + .cls = &suspend_block_sysclass,> +};> +
Hmm. Perhaps add the suspend_is_blocked() check at the beginning ofsysdev_suspend() instead of this? Surely you don't want to suspendany sysdevs with any suspend blockers active, right?
> +/**> + * suspend_blocker_init() - Initialize a suspend blocker> + * @blocker: The suspend blocker to initialize.> + * @name: The name of the suspend blocker to show in> + * /proc/suspend_blockers> + */> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)> +{> + if (name)> + blocker->name = name;> + BUG_ON(!blocker->name);
Do you really want to crash the kernel in this case? WARN_ON() might be better IMO.
Also, this assumes that the caller won't release the memory used to store the'name' string. it might be a good idea to point that out in the kerneldoccomment.
> +> + if (debug_mask & DEBUG_SUSPEND_BLOCKER)> + pr_info("suspend_blocker_init name=%s\n", blocker->name);> +> + atomic_set(&blocker->flags, SB_INITIALIZED);> +}> +EXPORT_SYMBOL(suspend_blocker_init);> +> +/**> + * suspend_blocker_destroy() - Destroy a suspend blocker> + * @blocker: The suspend blocker to destroy.> + */> +void suspend_blocker_destroy(struct suspend_blocker *blocker)> +{> + int flags;> + if (debug_mask & DEBUG_SUSPEND_BLOCKER)> + pr_info("suspend_blocker_destroy name=%s\n", blocker->name);> + flags = atomic_xchg(&blocker->flags, 0);> + WARN(!(flags & SB_INITIALIZED), "suspend_blocker_destroy called on "> + "uninitialized suspend_blocker\n");> + if (flags == (SB_INITIALIZED | SB_ACTIVE))> + if (atomic_dec_and_test(&suspend_block_count))> + queue_work(suspend_work_queue, &suspend_work);> +}> +EXPORT_SYMBOL(suspend_blocker_destroy);> +> +/**> + * suspend_block() - Block suspend> + * @blocker: The suspend blocker to use> + */> +void suspend_block(struct suspend_blocker *blocker)> +{> + BUG_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
WARN_ON() and exit instead? BUG_ON() is a bit drastic IMHO.
> +> + if (debug_mask & DEBUG_SUSPEND_BLOCKER)> + pr_info("suspend_block: %s\n", blocker->name);> + if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED,> + SB_INITIALIZED | SB_ACTIVE) == SB_INITIALIZED)> + atomic_inc(&suspend_block_count);> +> + atomic_inc(¤t_event_num);> +}> +EXPORT_SYMBOL(suspend_block);> +> +/**> + * suspend_unblock() - Unblock suspend> + * @blocker: The suspend blocker to unblock.> + *> + * If no other suspend blockers block suspend, the system will suspend.> + */> +void suspend_unblock(struct suspend_blocker *blocker)> +{> + if (debug_mask & DEBUG_SUSPEND_BLOCKER)> + pr_info("suspend_unblock: %s\n", blocker->name);> +> + if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED | SB_ACTIVE,> + SB_INITIALIZED) == (SB_INITIALIZED | SB_ACTIVE))> + if (atomic_dec_and_test(&suspend_block_count))> + queue_work(suspend_work_queue, &suspend_work);> +}> +EXPORT_SYMBOL(suspend_unblock);> +> +/**> + * is_blocking_suspend() - Test if a suspend blocker is blocking suspend> + * @blocker: The suspend blocker to check.> + */> +bool is_blocking_suspend(struct suspend_blocker *blocker)> +{> + return !!(atomic_read(&blocker->flags) & SB_ACTIVE);
Check SB_INITIALIZED too, perhaps? WARN_ON(!SB_INITIALIZED)?
> +}> +EXPORT_SYMBOL(is_blocking_suspend);> +> +void request_suspend_state(suspend_state_t state)> +{> + unsigned long irqflags;> + spin_lock_irqsave(&state_lock, irqflags);> + if (debug_mask & DEBUG_USER_STATE) {> + struct timespec ts;> + struct rtc_time tm;> + getnstimeofday(&ts);> + rtc_time_to_tm(ts.tv_sec, &tm);> + pr_info("request_suspend_state: %s (%d->%d) at %lld "> + "(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n",> + state != PM_SUSPEND_ON ? "sleep" : "wakeup",> + requested_suspend_state, state,> + ktime_to_ns(ktime_get()),> + tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday,> + tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec);> + }> + requested_suspend_state = state;> + if (state == PM_SUSPEND_ON)> + suspend_block(&main_suspend_blocker);> + else> + suspend_unblock(&main_suspend_blocker);> + spin_unlock_irqrestore(&state_lock, irqflags);> +}> +> +static int __init suspend_block_init(void)> +{> + int ret;> +> + suspend_blocker_init(&main_suspend_blocker, "main");> + suspend_block(&main_suspend_blocker);> +> + ret = sysdev_class_register(&suspend_block_sysclass);> + if (ret) {> + pr_err("suspend_block_init: sysdev_class_register failed\n");> + goto err_sysdev_class_register;> + }> + ret = sysdev_register(&suspend_block_sysdev);> + if (ret) {> + pr_err("suspend_block_init: suspend_block_sysdev failed\n");> + goto err_sysdev_register;> + }> +> + suspend_work_queue = create_singlethread_workqueue("suspend");
Do we need a separate workqueue for this purpose?
> + if (suspend_work_queue == NULL) {> + ret = -ENOMEM;> + goto err_suspend_work_queue;> + }> +> + return 0;> +> +err_suspend_work_queue:> + sysdev_unregister(&suspend_block_sysdev);> +err_sysdev_register:> + sysdev_class_unregister(&suspend_block_sysclass);> +err_sysdev_class_register:> + suspend_blocker_destroy(&main_suspend_blocker);> + return ret;> +}> +> +static void __exit suspend_block_exit(void)> +{> + destroy_workqueue(suspend_work_queue);> + sysdev_unregister(&suspend_block_sysdev);> + sysdev_class_unregister(&suspend_block_sysclass);> + suspend_blocker_destroy(&main_suspend_blocker);> +}> +> +core_initcall(suspend_block_init);> +module_exit(suspend_block_exit);
Thanks,Rafael_______________________________________________linux-pm mailing listlinux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx://lists.linux-foundation.org/mailman/listinfo/linux-pm
