Re: [PATCH 04/11] PM: Implement early suspend api

Nigel Cunningham
2009-01-14T09:48:37Z

Hi.

On Tue, 2009-01-13 at 17:27 -0800, Arve Hjønnevåg wrote:
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
> kernel/power/Kconfig | 12 +++
> kernel/power/Makefile | 1 +
> kernel/power/earlysuspend.c | 178 +++++++++++++++++++++++++++++++++++++++++++
> kernel/power/power.h | 6 ++
> 4 files changed, 197 insertions(+), 0 deletions(-)
> create mode 100644 kernel/power/earlysuspend.c
>
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 6e3da6e..50690d8 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -119,6 +119,9 @@ config SUSPEND_FREEZER
> config HAS_WAKELOCK
> bool
>
> +config HAS_EARLYSUSPEND
> + bool
> +
> config WAKELOCK
> bool "Wake lock"
> depends on PM && RTC_CLASS
> @@ -135,6 +138,15 @@ config WAKELOCK_STAT
> ---help---
> Report wake lock stats in /proc/wakelocks
>
> +config EARLYSUSPEND
> + bool "Early suspend"
> + depends on WAKELOCK
> + default y
> + select HAS_EARLYSUSPEND
> + ---help---
> + Call early suspend handlers when the user requested sleep state
> + changes.
> +
> 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 401ddfe..f0f7b15 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -6,6 +6,7 @@ endif
> obj-y := main.o
> obj-$(CONFIG_PM_SLEEP) += process.o console.o
> obj-$(CONFIG_WAKELOCK) += wakelock.o
> +obj-$(CONFIG_EARLYSUSPEND) += earlysuspend.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/earlysuspend.c b/kernel/power/earlysuspend.c
> new file mode 100644
> index 0000000..84bed51
> --- /dev/null
> +++ b/kernel/power/earlysuspend.c
> @@ -0,0 +1,178 @@
> +/* kernel/power/earlysuspend.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/earlysuspend.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/rtc.h>
> +#include <linux/syscalls.h> /* sys_sync */
> +#include <linux/wakelock.h>
> +#include <linux/workqueue.h>
> +
> +#include "power.h"
> +
> +enum {
> + DEBUG_USER_STATE = 1U << 0,
> + DEBUG_SUSPEND = 1U << 2,
> +};

Is there a reason DEBUG_SUSPEND isn't 1U << 1? If so, it might be good
to document that here.

> +static int debug_mask = DEBUG_USER_STATE;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +static DEFINE_MUTEX(early_suspend_lock);
> +static LIST_HEAD(early_suspend_handlers);
> +static void early_suspend(struct work_struct *work);
> +static void late_resume(struct work_struct *work);
> +static DECLARE_WORK(early_suspend_work, early_suspend);
> +static DECLARE_WORK(late_resume_work, late_resume);
> +static DEFINE_SPINLOCK(state_lock);
> +enum {
> + SUSPEND_REQUESTED = 0x1,
> + SUSPENDED = 0x2,
> + SUSPEND_REQUESTED_AND_SUSPENDED = SUSPEND_REQUESTED | SUSPENDED,
> +};
> +static int state;
> +
> +void register_early_suspend(struct early_suspend *handler)
> +{
> + struct list_head *pos;
> +
> + mutex_lock(&early_suspend_lock);
> + list_for_each(pos, &early_suspend_handlers) {
> + struct early_suspend *e;
> + e = list_entry(pos, struct early_suspend, link);

list_for_each_entry?

> + if (e->level > handler->level)
> + break;
> + }
> + list_add_tail(&handler->link, pos);
> + if ((state & SUSPENDED) && handler->suspend)
> + handler->suspend(handler);
> + mutex_unlock(&early_suspend_lock);
> +}
> +EXPORT_SYMBOL(register_early_suspend);
> +
> +void unregister_early_suspend(struct early_suspend *handler)
> +{
> + mutex_lock(&early_suspend_lock);
> + list_del(&handler->link);
> + mutex_unlock(&early_suspend_lock);
> +}
> +EXPORT_SYMBOL(unregister_early_suspend);
> +
> +static void early_suspend(struct work_struct *work)
> +{
> + struct early_suspend *pos;
> + unsigned long irqflags;
> + int abort = 0;
> +
> + mutex_lock(&early_suspend_lock);
> + spin_lock_irqsave(&state_lock, irqflags);
> + if (state == SUSPEND_REQUESTED)
> + state |= SUSPENDED;
> + else
> + abort = 1;
> + spin_unlock_irqrestore(&state_lock, irqflags);
> +
> + if (abort) {
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("early_suspend: abort, state %d\n", state);
> + mutex_unlock(&early_suspend_lock);
> + goto abort;
> + }
> +
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("early_suspend: call handlers\n");
> + list_for_each_entry(pos, &early_suspend_handlers, link) {
> + if (pos->suspend != NULL)

Just "if (pos->suspend)" is sufficient.

> + pos->suspend(pos);
> + }
> + mutex_unlock(&early_suspend_lock);
> +
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("early_suspend: sync\n");
> +
> + sys_sync();

Why the sync here?

> +abort:
> + spin_lock_irqsave(&state_lock, irqflags);
> + if (state == SUSPEND_REQUESTED_AND_SUSPENDED)
> + wake_unlock(&main_wake_lock);
> + spin_unlock_irqrestore(&state_lock, irqflags);
> +}
> +
> +static void late_resume(struct work_struct *work)
> +{
> + struct early_suspend *pos;
> + unsigned long irqflags;
> + int abort = 0;
> +
> + mutex_lock(&early_suspend_lock);
> + spin_lock_irqsave(&state_lock, irqflags);
> + if (state == SUSPENDED)
> + state &= ~SUSPENDED;

Why not just say state = 0?

> + else
> + abort = 1;
> + spin_unlock_irqrestore(&state_lock, irqflags);
> +
> + if (abort) {
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("late_resume: abort, state %d\n", state);
> + goto abort;
> + }
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("late_resume: call handlers\n");
> + list_for_each_entry_reverse(pos, &early_suspend_handlers, link)
> + if (pos->resume != NULL)

if (pos->resume)

> + pos->resume(pos);
> + if (debug_mask & DEBUG_SUSPEND)
> + pr_info("late_resume: done\n");
> +abort:
> + mutex_unlock(&early_suspend_lock);
> +}
> +
> +void request_suspend_state(suspend_state_t new_state)
> +{
> + unsigned long irqflags;
> + int old_sleep;
> +
> + spin_lock_irqsave(&state_lock, irqflags);
> + old_sleep = state & SUSPEND_REQUESTED;
> + 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",
> + new_state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> + requested_suspend_state, new_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);
> + }
> + if (!old_sleep && new_state != PM_SUSPEND_ON) {
> + state |= SUSPEND_REQUESTED;
> + queue_work(suspend_work_queue, &early_suspend_work);
> + } else if (old_sleep && new_state == PM_SUSPEND_ON) {
> + state &= ~SUSPEND_REQUESTED;
> + wake_lock(&main_wake_lock);
> + queue_work(suspend_work_queue, &late_resume_work);
> + }
> + requested_suspend_state = new_state;
> + spin_unlock_irqrestore(&state_lock, irqflags);
> +}
> +
> +suspend_state_t get_suspend_state(void)
> +{
> + return requested_suspend_state;
> +}
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 1527174..7ce9637 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -230,3 +230,9 @@ extern struct workqueue_struct *suspend_work_queue;
> extern struct wake_lock main_wake_lock;
> extern suspend_state_t requested_suspend_state;
> #endif
> +
> +#ifdef CONFIG_EARLYSUSPEND
> +/* kernel/power/earlysuspend.c */
> +void request_suspend_state(suspend_state_t state);
> +suspend_state_t get_suspend_state(void);
> +#endif
> --
> 1.6.1

Regards,

Nigel

_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org