Re: [PATCH 01/13] PM: Add wake lock 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>
> ---
>  Documentation/power/wakelocks.txt |   91 ++++++++++++++++++++++++++++++++++++
>  include/linux/wakelock.h          |   92 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 183 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/power/wakelocks.txt
>  create mode 100755 include/linux/wakelock.h
> 
> diff --git a/Documentation/power/wakelocks.txt b/Documentation/power/wakelocks.txt
> new file mode 100644
> index 0000000..219bb11
> --- /dev/null
> +++ b/Documentation/power/wakelocks.txt
> @@ -0,0 +1,91 @@
> +Wakelocks
> +=========
> +
> +A locked wakelock, depending on its type, prevents the system from entering
> +suspend or other low-power states. When creating a wakelock, you can select
> +if it prevents suspend or low-power idle states.  If the type is set to
> +WAKE_LOCK_SUSPEND, the wakelock prevents a full system suspend. If the type
> +is WAKE_LOCK_IDLE, low-power states that cause large interrupt latencies, or
> +that disable a set of interrupts, will not be entered from idle until the
> +wakelocks are released. Unless the type is specified, this document refers
> +to wakelocks with the type set to WAKE_LOCK_SUSPEND.
> +
> +If the suspend operation has already started when locking a wakelock, it will
> +abort the suspend operation as long it has not already reached the suspend_late
> +stage. This means that locking a wakelock from an interrupt handler or a
> +freezeable thread always works, but if you lock a wakelock from a suspend_late
> +handler you must also return an error from that handler to abort suspend.
> +
> +Wakelocks 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 locks the keypad-scan wakelock
> +  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 locks
> +  the input-event-queue wakelock.
> +- The keypad-scan code detects that no keys are held and unlocks the
> +  keypad-scan wakelock.
> +- The user-space input-event thread returns from select/poll, locks the
> +  process-input-events wakelock and then calls read in the input-event device.
> +- The input-event driver dequeues the key-event and, since the queue is now
> +  empty, it unlocks the input-event-queue wakelock.
> +- The user-space input-event thread returns from read. It determines that the
> +  key should not wake up the full system, releases the process-input-events
> +  wakelock and calls select or poll.

Hm, can you please explain to me what this example is supposed to illustrate?

If my understanding is correct, a wakelock is a mechanism that, if held, will
prevent the system from (automatically) entering a sleep state, but why do we
need a number of such wakelocks instead of just one reference counter with the
rule that (automatic) suspend can only happen if the counter is zero?

Then, code paths wanting to prevent the suspend from happening would only need
to increase the counter and it shouldn't be difficult to provide a user space
interface for that.

> +
> +                 Key pressed   Key released
> +                     |             |
> +keypad-scan          ++++++++++++++++++
> +input-event-queue        +++          +++
> +process-input-events       +++          +++
> +
> +
> +Driver API
> +==========
> +
> +A driver can use the wakelock api by adding a wakelock variable to its state
> +and calling wake_lock_init. For instance:
> +struct state {
> +	struct wakelock wakelock;
> +}
> +
> +init() {
> +	wake_lock_init(&state->wakelock, WAKE_LOCK_SUSPEND, "wakelockname");
> +}
> +
> +Before freeing the memory, wake_lock_destroy must be called:
> +
> +uninit() {
> +	wake_lock_destroy(&state->wakelock);
> +}
> +
> +When the driver determines that it needs to run (usually in an interrupt
> +handler) it calls wake_lock:
> +	wake_lock(&state->wakelock);
> +
> +When it no longer needs to run it calls wake_unlock:
> +	wake_unlock(&state->wakelock);
> +
> +It can also call wake_lock_timeout to release the wakelock after a delay:
> +	wake_lock_timeout(&state->wakelock, HZ);
> +
> +This works whether the wakelock is already held or not. It is useful if the
> +driver woke up other parts of the system that do not use wakelocks but
> +still need to run. Avoid this when possible, since it will waste power
> +if the timeout is long or may fail to finish needed work if the timeout is
> +short.

And what's this call needed for?

> +
> +
> +User-space API
> +==============
> +
> +Write "lockname" or "lockname timeout" to /sys/power/wake_lock lock and if
> +needed create a wakelock. The timeout here is specified nanoseconds.
> +Write "lockname" to /sys/power/wake_unlock to unlock a user wakelock.

Is there a mechanism allowing us to see what wakelocks have been created by
the user land?  Something like this would be useful for debugging.

> +Do not use randomly generated wakelock names as there is no api to free
> +a user-space wakelock.

This appears to be very fragile.  Please rethink it.

> +
> diff --git a/include/linux/wakelock.h b/include/linux/wakelock.h
> new file mode 100755
> index 0000000..b17e993
> --- /dev/null
> +++ b/include/linux/wakelock.h
> @@ -0,0 +1,92 @@
> +/* include/linux/wakelock.h
> + *
> + * Copyright (C) 2007-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.
> + *
> + */
> +
> +#ifndef _LINUX_WAKELOCK_H
> +#define _LINUX_WAKELOCK_H
> +
> +#include <linux/list.h>
> +#include <linux/ktime.h>
> +
> +/* A wake_lock prevents the system from entering suspend or other low power
> + * states when active. If the type is set to WAKE_LOCK_SUSPEND, the wake_lock
> + * prevents a full system suspend. If the type is WAKE_LOCK_IDLE, low power
> + * states that cause large interrupt latencies or that disable a set of
> + * interrupts will not entered from idle until the wake_locks are released.
> + */
> +
> +enum {
> +	WAKE_LOCK_SUSPEND, /* Prevent suspend */
> +	WAKE_LOCK_IDLE,    /* Prevent low power idle */
> +	WAKE_LOCK_TYPE_COUNT
> +};
> +
> +struct wake_lock {
> +#ifdef CONFIG_HAS_WAKELOCK
> +	struct list_head    link;
> +	int                 flags;
> +	const char         *name;
> +	unsigned long       expires;
> +#ifdef CONFIG_WAKELOCK_STAT
> +	struct {
> +		int             count;
> +		int             expire_count;
> +		int             wakeup_count;
> +		ktime_t         total_time;
> +		ktime_t         prevent_suspend_time;
> +		ktime_t         max_time;
> +		ktime_t         last_time;
> +	} stat;
> +#endif
> +#endif
> +};
> +
> +#ifdef CONFIG_HAS_WAKELOCK
> +
> +void wake_lock_init(struct wake_lock *lock, int type, const char *name);
> +void wake_lock_destroy(struct wake_lock *lock);
> +void wake_lock(struct wake_lock *lock);
> +void wake_lock_timeout(struct wake_lock *lock, long timeout);
> +void wake_unlock(struct wake_lock *lock);
> +
> +/* wake_lock_active returns a non-zero value if the wake_lock is currently
> + * locked. If the wake_lock has a timeout, it does not check the timeout,
> + * but if the timeout had already expired when it was checked elsewhere
> + * this function will return 0.
> + */
> +int wake_lock_active(struct wake_lock *lock);

What's the purpose of this function?

> +
> +/* has_wake_lock returns 0 if no wake locks of the specified type are active,
> + * and non-zero if one or more wake locks are held. Specifically it returns
> + * -1 if one or more wake locks with no timeout are active or the
> + * number of jiffies until all active wake locks time out.
> + */
> +long has_wake_lock(int type);

And this?

> +
> +#else
> +
> +static inline void wake_lock_init(struct wake_lock *lock, int type,
> +					const char *name) {}
> +static inline void wake_lock_destroy(struct wake_lock *lock) {}
> +static inline void wake_lock(struct wake_lock *lock) {}
> +static inline void wake_lock_timeout(struct wake_lock *lock, long timeout) {}
> +static inline void wake_unlock(struct wake_lock *lock) {}
> +
> +static inline int wake_lock_active(struct wake_lock *lock) { return 0; }
> +static inline long has_wake_lock(int type) { return 0; }
> +
> +#endif
> +
> +#endif
> +

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