Re: [PATCH 1/8] PM: Add suspend block api.

["Arve Hjønnevåg" <arve@android.com>]

2009/4/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 15 April 2009, Arve Hjønnevåg wrote:
>> 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?

suspend_blocker.h?

>
>> @@ -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 ?

OK.

>
>> +     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() ?
OK.
> 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.

Nothing depends on RTC_LIB, it is only selected.

>
>> +     default n
>> +     ---help---
>> +       Enable suspend_block.
>
> Enable suspend blockers.

OK.

>
>> 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.

I added this to the intro.

>
>> +
>>  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?

Removed.

>
>>       [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?
>

No, but it gives a compile error instead of a link error if you call
request_suspend_state when it is not available so it is useful.

>> +
>> 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.

OK.

>
>> +
>> +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.

do you think this is better:
while(1) {
        if (exception1) {
                ...
                break;
        }
        ...
        if (exception2) {
                ...
                continue;
        }
        break;
}
>
>> +     }
>> +
>> +     entry_event_num = atomic_read(&current_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(&current_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 of
> the loop?

The retry is a workaround for the lack of suspend_block_timeout.

>
>> +     }
>> +abort:
>> +     enable_suspend_blockers = 0;
>
> 'false' instead of '0', please.

OK.

>
>> +}
>> +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 of
> sysdev_suspend() instead of this?  Surely you don't want to suspend
> any sysdevs with any suspend blockers active, right?

You could make the same argument for any device. Using a sysdev makes
the patch easier to apply.

>
>> +/**
>> + * 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.

OK.

>
> 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 kerneldoc
> comment.
>
>> +
>> +     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(&current_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)?

Added WARN_ON.

>
>> +}
>> +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?
>

Yes. Some drivers wait on work that runs on the main workqueue in
their suspend handler.

>> +     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
>



-- 
Arve Hjønnevåg