Re: [PATCH 03/13] PM: Implement wakelock api.

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Thursday 05 February 2009, Arve Hjønnevåg wrote:
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  kernel/power/Kconfig    |   19 ++
>  kernel/power/Makefile   |    1 +
>  kernel/power/power.h    |    7 +
>  kernel/power/wakelock.c |  598 +++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 625 insertions(+), 0 deletions(-)
>  create mode 100644 kernel/power/wakelock.c
> 
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 23bd4da..6e3da6e 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -116,6 +116,25 @@ config SUSPEND_FREEZER
>  
>  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
>  
> +config HAS_WAKELOCK
> +	bool
> +
> +config WAKELOCK
> +	bool "Wake lock"
> +	depends on PM && RTC_CLASS
> +	default n
> +	select HAS_WAKELOCK
> +	---help---
> +	  Enable wakelocks. When user space request a sleep state the
> +	  sleep request will be delayed until no wake locks are held.
> +
> +config WAKELOCK_STAT
> +	bool "Wake lock stats"
> +	depends on WAKELOCK
> +	default y
> +	---help---
> +	  Report wake lock stats in /proc/wakelocks
> +
>  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 d7a1016..8d8672b 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -6,6 +6,7 @@ endif
>  obj-y				:= main.o
>  obj-$(CONFIG_PM_SLEEP)		+= console.o
>  obj-$(CONFIG_FREEZER)		+= process.o
> +obj-$(CONFIG_WAKELOCK)		+= wakelock.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/power.h b/kernel/power/power.h
> index 46b5ec7..1527174 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -223,3 +223,10 @@ static inline void suspend_thaw_processes(void)
>  {
>  }
>  #endif
> +
> +#ifdef CONFIG_WAKELOCK
> +/* kernel/power/wakelock.c */
> +extern struct workqueue_struct *suspend_work_queue;
> +extern struct wake_lock main_wake_lock;
> +extern suspend_state_t requested_suspend_state;
> +#endif
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> new file mode 100644
> index 0000000..c9e22f9
> --- /dev/null
> +++ b/kernel/power/wakelock.c
> @@ -0,0 +1,598 @@
> +/* kernel/power/wakelock.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/platform_device.h>
> +#include <linux/rtc.h>
> +#include <linux/suspend.h>
> +#include <linux/syscalls.h> /* sys_sync */
> +#include <linux/wakelock.h>
> +#ifdef CONFIG_WAKELOCK_STAT
> +#include <linux/proc_fs.h>
> +#endif
> +#include "power.h"
> +
> +enum {
> +	DEBUG_EXIT_SUSPEND = 1U << 0,
> +	DEBUG_WAKEUP = 1U << 1,
> +	DEBUG_SUSPEND = 1U << 2,
> +	DEBUG_EXPIRE = 1U << 3,
> +	DEBUG_WAKE_LOCK = 1U << 4,
> +};
> +static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +#define WAKE_LOCK_TYPE_MASK              (0x0f)
> +#define WAKE_LOCK_INITIALIZED            (1U << 8)
> +#define WAKE_LOCK_ACTIVE                 (1U << 9)
> +#define WAKE_LOCK_AUTO_EXPIRE            (1U << 10)
> +#define WAKE_LOCK_PREVENTING_SUSPEND     (1U << 11)
> +
> +static DEFINE_SPINLOCK(list_lock);
> +static LIST_HEAD(inactive_locks);
> +static struct list_head active_wake_locks[WAKE_LOCK_TYPE_COUNT];
> +static int current_event_num;
> +struct workqueue_struct *suspend_work_queue;
> +struct wake_lock main_wake_lock;
> +suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
> +static struct wake_lock unknown_wakeup;
> +
> +#ifdef CONFIG_WAKELOCK_STAT
> +static struct wake_lock deleted_wake_locks;
> +static ktime_t last_sleep_time_update;
> +static int wait_for_wakeup;

Care to add kerneldoc comments to the functions?

> +int get_expired_time(struct wake_lock *lock, ktime_t *expire_time)

I would use 'bool'.

> +{
> +	struct timespec ts;
> +	struct timespec kt;
> +	struct timespec tomono;
> +	struct timespec delta;
> +	unsigned long seq;
> +	long timeout;
> +
> +	if (!(lock->flags & WAKE_LOCK_AUTO_EXPIRE))
> +		return 0;
> +	do {
> +		seq = read_seqbegin(&xtime_lock);
> +		timeout = lock->expires - jiffies;

Can it overflow?

Anyway, you are using struct timespec in computations, so why don't you
store it in the lock structure?

> +		if (timeout > 0)
> +			return 0;
> +		kt = current_kernel_time();
> +		tomono = wall_to_monotonic;
> +	} while (read_seqretry(&xtime_lock, seq));
> +	jiffies_to_timespec(-timeout, &delta);
> +	set_normalized_timespec(&ts, kt.tv_sec + tomono.tv_sec - delta.tv_sec,
> +				kt.tv_nsec + tomono.tv_nsec - delta.tv_nsec);
> +	*expire_time = timespec_to_ktime(ts);
> +	return 1;
> +}
> +
> +
> +static int print_lock_stat(char *buf, struct wake_lock *lock)
> +{
> +	int lock_count = lock->stat.count;
> +	int expire_count = lock->stat.expire_count;
> +	ktime_t active_time = ktime_set(0, 0);
> +	ktime_t total_time = lock->stat.total_time;
> +	ktime_t max_time = lock->stat.max_time;
> +	ktime_t prevent_suspend_time = lock->stat.prevent_suspend_time;
> +	if (lock->flags & WAKE_LOCK_ACTIVE) {
> +		ktime_t now, add_time;
> +		int expired = get_expired_time(lock, &now);
> +		if (!expired)
> +			now = ktime_get();
> +		add_time = ktime_sub(now, lock->stat.last_time);
> +		lock_count++;
> +		if (!expired)
> +			active_time = add_time;
> +		else
> +			expire_count++;
> +		total_time = ktime_add(total_time, add_time);
> +		if (lock->flags & WAKE_LOCK_PREVENTING_SUSPEND)
> +			prevent_suspend_time = ktime_add(prevent_suspend_time,
> +					ktime_sub(now, last_sleep_time_update));
> +		if (add_time.tv64 > max_time.tv64)
> +			max_time = add_time;
> +	}
> +
> +	return sprintf(buf, "\"%s\"\t%d\t%d\t%d\t%lld\t%lld\t%lld\t%lld\t"
> +		       "%lld\n", lock->name, lock_count, expire_count,
> +		       lock->stat.wakeup_count, ktime_to_ns(active_time),
> +		       ktime_to_ns(total_time),
> +		       ktime_to_ns(prevent_suspend_time), ktime_to_ns(max_time),
> +		       ktime_to_ns(lock->stat.last_time));
> +}
> +

Why did you decide to put that in /proc ?

> +static int wakelocks_read_proc(char *page, char **start, off_t off,
> +			       int count, int *eof, void *data)
> +{
> +	unsigned long irqflags;
> +	struct wake_lock *lock;
> +	int len = 0;
> +	char *p = page;
> +	int type;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +
> +	p += sprintf(p, "name\tcount\texpire_count\twake_count\tactive_since"
> +		     "\ttotal_time\tsleep_time\tmax_time\tlast_change\n");
> +	list_for_each_entry(lock, &inactive_locks, link) {
> +		p += print_lock_stat(p, lock);
> +	}
> +	for (type = 0; type < WAKE_LOCK_TYPE_COUNT; type++) {
> +		list_for_each_entry(lock, &active_wake_locks[type], link)
> +			p += print_lock_stat(p, lock);
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +
> +	*start = page + off;
> +
> +	len = p - page;
> +	if (len > off)
> +		len -= off;
> +	else
> +		len = 0;
> +
> +	return len < count ? len  : count;
> +}
> +
> +static void wake_unlock_stat_locked(struct wake_lock *lock, int expired)
> +{
> +	ktime_t duration;
> +	ktime_t now;
> +	if (!(lock->flags & WAKE_LOCK_ACTIVE))
> +		return;
> +	if (get_expired_time(lock, &now))
> +		expired = 1;

I'd use 'true'.

> +	else
> +		now = ktime_get();
> +	lock->stat.count++;
> +	if (expired)
> +		lock->stat.expire_count++;
> +	duration = ktime_sub(now, lock->stat.last_time);
> +	lock->stat.total_time = ktime_add(lock->stat.total_time, duration);
> +	if (ktime_to_ns(duration) > ktime_to_ns(lock->stat.max_time))
> +		lock->stat.max_time = duration;
> +	lock->stat.last_time = ktime_get();
> +	if (lock->flags & WAKE_LOCK_PREVENTING_SUSPEND) {
> +		duration = ktime_sub(now, last_sleep_time_update);
> +		lock->stat.prevent_suspend_time = ktime_add(
> +			lock->stat.prevent_suspend_time, duration);
> +		lock->flags &= ~WAKE_LOCK_PREVENTING_SUSPEND;
> +	}
> +}
> +
> +static void update_sleep_wait_stats_locked(int done)
> +{
> +	struct wake_lock *lock;
> +	ktime_t now, etime, elapsed, add;
> +	int expired;
> +
> +	now = ktime_get();
> +	elapsed = ktime_sub(now, last_sleep_time_update);
> +	list_for_each_entry(lock, &active_wake_locks[WAKE_LOCK_SUSPEND], link) {
> +		expired = get_expired_time(lock, &etime);
> +		if (lock->flags & WAKE_LOCK_PREVENTING_SUSPEND) {
> +			if (expired)
> +				add = ktime_sub(etime, last_sleep_time_update);
> +			else
> +				add = elapsed;
> +			lock->stat.prevent_suspend_time = ktime_add(
> +				lock->stat.prevent_suspend_time, add);
> +		}
> +		if (done || expired)
> +			lock->flags &= ~WAKE_LOCK_PREVENTING_SUSPEND;
> +		else
> +			lock->flags |= WAKE_LOCK_PREVENTING_SUSPEND;
> +	}
> +	last_sleep_time_update = now;
> +}
> +#endif
> +
> +
> +static void expire_wake_lock(struct wake_lock *lock)
> +{
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wake_unlock_stat_locked(lock, 1);
> +#endif
> +	lock->flags &= ~(WAKE_LOCK_ACTIVE | WAKE_LOCK_AUTO_EXPIRE);
> +	list_del(&lock->link);
> +	list_add(&lock->link, &inactive_locks);
> +	if (debug_mask & (DEBUG_WAKE_LOCK | DEBUG_EXPIRE))
> +		pr_info("expired wake lock %s\n", lock->name);
> +}
> +
> +static void print_active_locks(int type)
> +{
> +	unsigned long irqflags;
> +	struct wake_lock *lock;
> +
> +	BUG_ON(type >= WAKE_LOCK_TYPE_COUNT);
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_for_each_entry(lock, &active_wake_locks[type], link) {
> +		if (lock->flags & WAKE_LOCK_AUTO_EXPIRE) {
> +			long timeout = lock->expires - jiffies;
> +			if (timeout <= 0)
> +				pr_info("wake lock %s, expired\n", lock->name);
> +			else
> +				pr_info("active wake lock %s, time left %ld\n",
> +					lock->name, timeout);
> +		} else
> +			pr_info("active wake lock %s\n", lock->name);
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +
> +static long has_wake_lock_locked(int type)

I'd change the name, it suggests something different from what the function
does.

> +{
> +	struct wake_lock *lock, *n;
> +	long max_timeout = 0;
> +
> +	BUG_ON(type >= WAKE_LOCK_TYPE_COUNT);
> +	list_for_each_entry_safe(lock, n, &active_wake_locks[type], link) {
> +		if (lock->flags & WAKE_LOCK_AUTO_EXPIRE) {
> +			long timeout = lock->expires - jiffies;

I think time_after() is for things like this.

> +			if (timeout <= 0)
> +				expire_wake_lock(lock);
> +			else if (timeout > max_timeout)
> +				max_timeout = timeout;
> +		} else
> +			return -1;
> +	}
> +	return max_timeout;
> +}
> +
> +long has_wake_lock(int type)
> +{
> +	long ret;
> +	unsigned long irqflags;
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	ret = has_wake_lock_locked(type);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +	return ret;
> +}
> +
> +static void suspend(struct work_struct *work)
> +{
> +	int ret;
> +	int entry_event_num;
> +
> +	if (has_wake_lock(WAKE_LOCK_SUSPEND)) {
> +		if (debug_mask & DEBUG_SUSPEND)
> +			pr_info("suspend: abort suspend\n");
> +		return;
> +	}
> +
> +	entry_event_num = current_event_num;
> +	sys_sync();

pm_suspend() will do it, you don't need to.

> +	if (debug_mask & DEBUG_SUSPEND)
> +		pr_info("suspend: enter suspend\n");

Shouldn't that check if someone has taken a wakelock in the meantime?

> +	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 (current_event_num == entry_event_num) {
> +		if (debug_mask & DEBUG_SUSPEND)
> +			pr_info("suspend: pm_suspend returned with no event\n");
> +		wake_lock_timeout(&unknown_wakeup, HZ / 2);
> +	}
> +}
> +static DECLARE_WORK(suspend_work, suspend);
> +
> +static void expire_wake_locks(unsigned long data)
> +{
> +	long has_lock;
> +	unsigned long irqflags;
> +	if (debug_mask & DEBUG_EXPIRE)
> +		pr_info("expire_wake_locks: start\n");
> +	if (debug_mask & DEBUG_SUSPEND)
> +		print_active_locks(WAKE_LOCK_SUSPEND);
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	has_lock = has_wake_lock_locked(WAKE_LOCK_SUSPEND);
> +	if (debug_mask & DEBUG_EXPIRE)
> +		pr_info("expire_wake_locks: done, has_lock %ld\n", has_lock);
> +	if (has_lock == 0)
> +		queue_work(suspend_work_queue, &suspend_work);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +static DEFINE_TIMER(expire_timer, expire_wake_locks, 0, 0);
> +
> +static int power_suspend_late(struct platform_device *pdev, pm_message_t state)
> +{
> +	int ret = has_wake_lock(WAKE_LOCK_SUSPEND) ? -EAGAIN : 0;
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wait_for_wakeup = 1;
> +#endif
> +	if (debug_mask & DEBUG_SUSPEND)
> +		pr_info("power_suspend_late return %d\n", ret);
> +	return ret;
> +}
> +
> +static struct platform_driver power_driver = {
> +	.driver.name = "power",
> +	.suspend_late = power_suspend_late,
> +};
> +static struct platform_device power_device = {
> +	.name = "power",
> +};

What's this and what's it for?

> +
> +void wake_lock_init(struct wake_lock *lock, int type, const char *name)
> +{
> +	unsigned long irqflags = 0;
> +
> +	if (name)
> +		lock->name = name;

Hm.  I'd rather reserve memory for the name and copy it from the address
provided by the caller.

> +	BUG_ON(!lock->name);

Isn't it a bit too drastic?  Perhaps make it return a value so that the caller
can check if it has the lock?

> +
> +	if (debug_mask & DEBUG_WAKE_LOCK)
> +		pr_info("wake_lock_init name=%s\n", lock->name);
> +#ifdef CONFIG_WAKELOCK_STAT
> +	lock->stat.count = 0;
> +	lock->stat.expire_count = 0;
> +	lock->stat.wakeup_count = 0;
> +	lock->stat.total_time = ktime_set(0, 0);
> +	lock->stat.prevent_suspend_time = ktime_set(0, 0);
> +	lock->stat.max_time = ktime_set(0, 0);
> +	lock->stat.last_time = ktime_set(0, 0);
> +#endif
> +	lock->flags = (type & WAKE_LOCK_TYPE_MASK) | WAKE_LOCK_INITIALIZED;
> +
> +	INIT_LIST_HEAD(&lock->link);
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_add(&lock->link, &inactive_locks);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(wake_lock_init);
> +
> +void wake_lock_destroy(struct wake_lock *lock)
> +{
> +	unsigned long irqflags;
> +	if (debug_mask & DEBUG_WAKE_LOCK)
> +		pr_info("wake_lock_destroy name=%s\n", lock->name);
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	lock->flags &= ~WAKE_LOCK_INITIALIZED;
> +#ifdef CONFIG_WAKELOCK_STAT
> +	if (lock->stat.count) {
> +		deleted_wake_locks.stat.count += lock->stat.count;
> +		deleted_wake_locks.stat.expire_count += lock->stat.expire_count;
> +		deleted_wake_locks.stat.total_time =
> +			ktime_add(deleted_wake_locks.stat.total_time,
> +				  lock->stat.total_time);
> +		deleted_wake_locks.stat.prevent_suspend_time =
> +			ktime_add(deleted_wake_locks.stat.prevent_suspend_time,
> +				  lock->stat.prevent_suspend_time);
> +		deleted_wake_locks.stat.max_time =
> +			ktime_add(deleted_wake_locks.stat.max_time,
> +				  lock->stat.max_time);
> +	}
> +#endif
> +	list_del(&lock->link);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(wake_lock_destroy);
> +
> +static void wake_lock_internal(

What  about __wake_lock() ?

> +	struct wake_lock *lock, long timeout, int has_timeout)

What's the point of the last argument?  Wouldn't timeout == 0 be sufficient?

Also, does it make sense to pass negative timeout to it?

> +{
> +	int type;
> +	unsigned long irqflags;
> +	long expire_in;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	type = lock->flags & WAKE_LOCK_TYPE_MASK;
> +	BUG_ON(type >= WAKE_LOCK_TYPE_COUNT);
> +	BUG_ON(!(lock->flags & WAKE_LOCK_INITIALIZED));

I don't like these BUG_ON()s.  Please do something less drastic instead.

> +#ifdef CONFIG_WAKELOCK_STAT
> +	if (type == WAKE_LOCK_SUSPEND && wait_for_wakeup) {
> +		if (debug_mask & DEBUG_WAKEUP)
> +			pr_info("wakeup wake lock: %s\n", lock->name);
> +		wait_for_wakeup = 0;
> +		lock->stat.wakeup_count++;
> +	}
> +	if ((lock->flags & WAKE_LOCK_AUTO_EXPIRE) &&
> +	    (long)(lock->expires - jiffies) <= 0) {
> +		wake_unlock_stat_locked(lock, 0);
> +		lock->stat.last_time = ktime_get();
> +	}
> +#endif
> +	if (!(lock->flags & WAKE_LOCK_ACTIVE)) {
> +		lock->flags |= WAKE_LOCK_ACTIVE;
> +#ifdef CONFIG_WAKELOCK_STAT
> +		lock->stat.last_time = ktime_get();
> +#endif
> +	}
> +	list_del(&lock->link);
> +	if (has_timeout) {
> +		if (debug_mask & DEBUG_WAKE_LOCK)
> +			pr_info("wake_lock: %s, type %d, timeout %ld.%03lu\n",
> +				lock->name, type, timeout / HZ,
> +				(timeout % HZ) * MSEC_PER_SEC / HZ);
> +		lock->expires = jiffies + timeout;
> +		lock->flags |= WAKE_LOCK_AUTO_EXPIRE;
> +		list_add_tail(&lock->link, &active_wake_locks[type]);
> +	} else {
> +		if (debug_mask & DEBUG_WAKE_LOCK)
> +			pr_info("wake_lock: %s, type %d\n", lock->name, type);
> +		lock->expires = LONG_MAX;
> +		lock->flags &= ~WAKE_LOCK_AUTO_EXPIRE;
> +		list_add(&lock->link, &active_wake_locks[type]);
> +	}

Why not to use list_add_tail() in both cases?

> +	if (type == WAKE_LOCK_SUSPEND) {
> +		current_event_num++;
> +#ifdef CONFIG_WAKELOCK_STAT
> +		if (lock == &main_wake_lock)
> +			update_sleep_wait_stats_locked(1);
> +		else if (!wake_lock_active(&main_wake_lock))
> +			update_sleep_wait_stats_locked(0);
> +#endif
> +		if (has_timeout)
> +			expire_in = has_wake_lock_locked(type);
> +		else
> +			expire_in = -1;

	expire_i = has_timeout ? has_wake_lock_locked(type) : -1;

(there is something like this above too).

> +		if (expire_in > 0) {
> +			if (debug_mask & DEBUG_EXPIRE)
> +				pr_info("wake_lock: %s, start expire timer, "
> +					"%ld\n", lock->name, expire_in);
> +			mod_timer(&expire_timer, jiffies + expire_in);

What exactly are you trying to achieve here?

> +		} else {
> +			if (del_timer(&expire_timer))
> +				if (debug_mask & DEBUG_EXPIRE)
> +					pr_info("wake_lock: %s, stop expire "
> +						"timer\n", lock->name);
> +			if (expire_in == 0)
> +				queue_work(suspend_work_queue, &suspend_work);

This appears to only happen if timeout = 0 is passed to this function while
has_timeout == true.  Is it correct?

> +		}
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +
> +void wake_lock(struct wake_lock *lock)
> +{
> +	wake_lock_internal(lock, 0, 0);
> +}
> +EXPORT_SYMBOL(wake_lock);
> +
> +void wake_lock_timeout(struct wake_lock *lock, long timeout)
> +{
> +	wake_lock_internal(lock, timeout, 1);
> +}
> +EXPORT_SYMBOL(wake_lock_timeout);
> +
> +void wake_unlock(struct wake_lock *lock)
> +{
> +	int type;
> +	unsigned long irqflags;
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	type = lock->flags & WAKE_LOCK_TYPE_MASK;
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wake_unlock_stat_locked(lock, 0);
> +#endif
> +	if (debug_mask & DEBUG_WAKE_LOCK)
> +		pr_info("wake_unlock: %s\n", lock->name);
> +	lock->flags &= ~(WAKE_LOCK_ACTIVE | WAKE_LOCK_AUTO_EXPIRE);
> +	list_del(&lock->link);
> +	list_add(&lock->link, &inactive_locks);
> +	if (type == WAKE_LOCK_SUSPEND) {
> +		long has_lock = has_wake_lock_locked(type);
> +		if (has_lock > 0) {
> +			if (debug_mask & DEBUG_EXPIRE)
> +				pr_info("wake_unlock: %s, start expire timer, "
> +					"%ld\n", lock->name, has_lock);
> +			mod_timer(&expire_timer, jiffies + has_lock);
> +		} else {
> +			if (del_timer(&expire_timer))
> +				if (debug_mask & DEBUG_EXPIRE)
> +					pr_info("wake_unlock: %s, stop expire "
> +						"timer\n", lock->name);
> +			if (has_lock == 0)
> +				queue_work(suspend_work_queue, &suspend_work);
> +		}
> +		if (lock == &main_wake_lock) {
> +			if (debug_mask & DEBUG_SUSPEND)
> +				print_active_locks(WAKE_LOCK_SUSPEND);
> +#ifdef CONFIG_WAKELOCK_STAT
> +			update_sleep_wait_stats_locked(0);
> +#endif
> +		}
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(wake_unlock);
> +
> +int wake_lock_active(struct wake_lock *lock)
> +{
> +	return !!(lock->flags & WAKE_LOCK_ACTIVE);
> +}
> +EXPORT_SYMBOL(wake_lock_active);
> +
> +static int __init wakelocks_init(void)
> +{
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(active_wake_locks); i++)
> +		INIT_LIST_HEAD(&active_wake_locks[i]);
> +
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wake_lock_init(&deleted_wake_locks, WAKE_LOCK_SUSPEND,
> +			"deleted_wake_locks");
> +#endif
> +	wake_lock_init(&main_wake_lock, WAKE_LOCK_SUSPEND, "main");
> +	wake_lock(&main_wake_lock);
> +	wake_lock_init(&unknown_wakeup, WAKE_LOCK_SUSPEND, "unknown_wakeups");
> +
> +	ret = platform_device_register(&power_device);
> +	if (ret) {
> +		pr_err("wakelocks_init: platform_device_register failed\n");
> +		goto err_platform_device_register;
> +	}
> +	ret = platform_driver_register(&power_driver);
> +	if (ret) {
> +		pr_err("wakelocks_init: platform_driver_register failed\n");
> +		goto err_platform_driver_register;
> +	}

Is this really a platform thing?

What exactly is the benefit of having the 'power_device' and 'power_driver'
registered?.

> +
> +	suspend_work_queue = create_singlethread_workqueue("suspend");
> +	if (suspend_work_queue == NULL) {
> +		ret = -ENOMEM;
> +		goto err_suspend_work_queue;
> +	}
> +
> +#ifdef CONFIG_WAKELOCK_STAT
> +	create_proc_read_entry("wakelocks", S_IRUGO, NULL,
> +				wakelocks_read_proc, NULL);
> +#endif
> +
> +	return 0;
> +
> +err_suspend_work_queue:
> +	platform_driver_unregister(&power_driver);
> +err_platform_driver_register:
> +	platform_device_unregister(&power_device);
> +err_platform_device_register:
> +	wake_lock_destroy(&unknown_wakeup);
> +	wake_lock_destroy(&main_wake_lock);
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wake_lock_destroy(&deleted_wake_locks);
> +#endif
> +	return ret;
> +}
> +
> +static void  __exit wakelocks_exit(void)
> +{
> +#ifdef CONFIG_WAKELOCK_STAT
> +	remove_proc_entry("wakelocks", NULL);
> +#endif
> +	destroy_workqueue(suspend_work_queue);
> +	platform_driver_unregister(&power_driver);
> +	platform_device_unregister(&power_device);
> +	wake_lock_destroy(&unknown_wakeup);
> +	wake_lock_destroy(&main_wake_lock);
> +#ifdef CONFIG_WAKELOCK_STAT
> +	wake_lock_destroy(&deleted_wake_locks);
> +#endif
> +}
> +
> +core_initcall(wakelocks_init);
> +module_exit(wakelocks_exit);

In general, I found this mechanism overly complicated.

As I said previously, I don't really see a benefit of using multiple wakelocks
over using a single reference counter with certain timer mechanism triggering
suspend in the counter is 0.

Also, I'd really prefer to see the patch without the CONFIG_WAKELOCK_STAT
thing included, first.  It would have been much easier to read.

Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm