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

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

[mark gross]

On Fri, Apr 30, 2010 at 03:36:54PM -0700, Arve Hjønnevåg wrote:
> Adds /sys/power/policy that selects the behaviour of /sys/power/state.
> After setting the policy to opportunistic, writes to /sys/power/state
> become non-blocking requests that specify which suspend state to enter
> when no suspend blockers are active. A special state, "on", stops the
> process by activating the "main" suspend blocker.
> 
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  Documentation/power/opportunistic-suspend.txt |  119 +++++++++++
>  include/linux/suspend_blocker.h               |   64 ++++++
>  kernel/power/Kconfig                          |   16 ++
>  kernel/power/Makefile                         |    1 +
>  kernel/power/main.c                           |   92 ++++++++-
>  kernel/power/power.h                          |    9 +
>  kernel/power/suspend.c                        |    4 +-
>  kernel/power/suspend_blocker.c                |  261 +++++++++++++++++++++++++
>  8 files changed, 559 insertions(+), 7 deletions(-)
>  create mode 100644 Documentation/power/opportunistic-suspend.txt
>  create mode 100755 include/linux/suspend_blocker.h
>  create mode 100644 kernel/power/suspend_blocker.c
> 
> diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
> new file mode 100644
> index 0000000..3d060e8
> --- /dev/null
> +++ b/Documentation/power/opportunistic-suspend.txt
> @@ -0,0 +1,119 @@
> +Opportunistic Suspend
> +=====================
> +
> +Opportunistic suspend is a feature allowing the system to be suspended (ie. put
> +into one of the available sleep states) automatically whenever it is regarded
> +as idle.  The suspend blockers framework described below is used to determine
> +when that happens.
> +
> +The /sys/power/policy sysfs attribute is used to switch the system between the
> +opportunistic and "forced" suspend behavior, where in the latter case the
> +system is only suspended if a specific value, corresponding to one of the
> +available system sleep states, is written into /sys/power/state.  However, in
> +the former, opportunistic, case the system is put into the sleep state
> +corresponding to the value written to /sys/power/state whenever there are no
> +active suspend blockers. The default policy is "forced". Also, suspend blockers
> +do not affect sleep states entered from idle.
> +
> +When the policy is "opportunisic", there is a special value, "on", that can be
> +written to /sys/power/state. This will block the automatic sleep request, as if
> +a suspend blocker was used by a device driver. This way the opportunistic
> +suspend may be blocked by user space whithout switching back to the "forced"
> +mode.

You know things would be so much easier if the policy was a one-shot
styled thing.  i.e. when enabled it does what it does, but upon resume
the policy must be re-enabled by user mode to get the opportunistic
behavior.  That way we don't need to grab the suspend blocker from the
wake up irq handler all the way up to user mode as the example below
calls out.  I suppose doing this would put a burden on the user mode code
to keep track of if it has no pending blockers registered after a wake
up from the suspend.  but that seems nicer to me than sprinkling
overlapping blocker critical sections from the mettle up to user mode.

Please consider making the policy a one shot API that needs to be
re-enabled after resume by user mode.  That would remove my issue with
the design.

--mgross

> +
> +A suspend blocker is an object used to inform the PM subsystem when the system
> +can or cannot be suspended in the "opportunistic" mode (the "forced" mode
> +ignores suspend blockers).  To use it, a device driver creates a struct
> +suspend_blocker that must be initialized with suspend_blocker_init(). Before
> +freeing the suspend_blocker structure or its name, suspend_blocker_destroy()
> +must be called on it.
> +
> +A suspend blocker is activated using suspend_block(), which prevents the PM
> +subsystem from putting the system into the requested sleep state in the
> +"opportunistic" mode until the suspend blocker is deactivated with
> +suspend_unblock(). Multiple suspend blockers may be active simultaneously, and
> +the system will not suspend as long as at least one of them is active.
> +
> +If opportunistic suspend is already in progress when suspend_block() is called,
> +it will abort the suspend, unless suspend_ops->enter has already been
> +executed. If suspend is aborted this way, the system is usually not fully
> +operational at that point. The suspend callbacks of some drivers may still be
> +running and it usually takes time to restore the system to the fully operational
> +state.
> +
> +Here's an example showing how a cell phone or other embedded system can handle
> +keystrokes (or other input events) in the presence of suspend blockers. 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. If it determines that
> +  the key should be ignored, it calls suspend_unblock on the
> +  process_input_events suspend_blocker and then calls select or poll. The
> +  system will automatically suspend again, since now no suspend blockers are
> +  active.
> +
> +If the key that was pressed instead should preform a simple action (for example,
> +adjusting the volume), this action can be performed right before calling
> +suspend_unblock on the process_input_events suspend_blocker. However, if the key
> +triggers a longer-running action, that action needs its own suspend_blocker and
> +suspend_block must be called on that suspend blocker before calling
> +suspend_unblock on the process_input_events suspend_blocker.
> +
> +                 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);
> +
> +When it no longer needs to run it calls suspend_unblock:
> +	suspend_unblock(&state->suspend_blocker);
> +
> +Calling suspend_block when the suspend blocker is active or suspend_unblock when
> +it is not active has no effect (i.e., these functions don't nest). 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_blocker.h b/include/linux/suspend_blocker.h
> new file mode 100755
> index 0000000..f9928cc
> --- /dev/null
> +++ b/include/linux/suspend_blocker.h
> @@ -0,0 +1,64 @@
> +/* include/linux/suspend_blocker.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_BLOCKER_H
> +#define _LINUX_SUSPEND_BLOCKER_H
> +
> +#include <linux/list.h>
> +
> +/**
> + * struct suspend_blocker - the basic suspend_blocker structure
> + * @link:	List entry for active or inactive list.
> + * @flags:	Tracks initialized and active state.
> + * @name:	Name used for debugging.
> + *
> + * When a suspend_blocker is active it prevents the system from entering
> + * opportunistic suspend.
> + *
> + * The suspend_blocker structure must be initialized by suspend_blocker_init()
> + */
> +
> +struct suspend_blocker {
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +	struct list_head    link;
> +	int                 flags;
> +	const char         *name;
> +#endif
> +};
> +
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +
> +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);
> +bool suspend_blocker_is_active(struct suspend_blocker *blocker);
> +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 suspend_blocker_is_active(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 5c36ea9..55a06a1 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -130,6 +130,22 @@ config SUSPEND_FREEZER
>  
>  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
>  
> +config OPPORTUNISTIC_SUSPEND
> +	bool "Suspend blockers"
> +	depends on PM_SLEEP
> +	select RTC_LIB
> +	default n
> +	---help---
> +	  Opportunistic sleep support.  Allows the system to be put into a sleep
> +	  state opportunistically, if it doesn't do any useful work at the
> +	  moment. The PM subsystem is switched into this mode of operation by
> +	  writing "opportunistic" into /sys/power/policy, while writing
> +	  "forced" to this file turns the opportunistic suspend feature off.
> +	  In the "opportunistic" mode suspend blockers are used to determine
> +	  when to suspend the system and the value written to /sys/power/state
> +	  determines the sleep state the system will be put into when there are
> +	  no active suspend blockers.
> +
>  config HIBERNATION_NVS
>  	bool
>  
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index 4319181..ee5276d 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PM)		+= main.o
>  obj-$(CONFIG_PM_SLEEP)		+= console.o
>  obj-$(CONFIG_FREEZER)		+= process.o
>  obj-$(CONFIG_SUSPEND)		+= suspend.o
> +obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
>  obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index b58800b..030ecdc 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -12,6 +12,7 @@
>  #include <linux/string.h>
>  #include <linux/resume-trace.h>
>  #include <linux/workqueue.h>
> +#include <linux/suspend_blocker.h>
>  
>  #include "power.h"
>  
> @@ -20,6 +21,27 @@ DEFINE_MUTEX(pm_mutex);
>  unsigned int pm_flags;
>  EXPORT_SYMBOL(pm_flags);
>  
> +struct policy {
> +	const char *name;
> +	bool (*valid_state)(suspend_state_t state);
> +	int (*set_state)(suspend_state_t state);
> +};
> +static struct policy policies[] = {
> +	{
> +		.name		= "forced",
> +		.valid_state	= valid_state,
> +		.set_state	= enter_state,
> +	},
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +	{
> +		.name		= "opportunistic",
> +		.valid_state	= request_suspend_valid_state,
> +		.set_state	= request_suspend_state,
> +	},
> +#endif
> +};
> +static int policy;
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  /* Routines for PM-transition notifications */
> @@ -146,6 +168,12 @@ struct kobject *power_kobj;
>   *
>   *	store() accepts one of those strings, translates it into the 
>   *	proper enumerated value, and initiates a suspend transition.
> + *
> + *	If policy is set to opportunistic, store() 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.
>   */
>  static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			  char *buf)
> @@ -155,12 +183,13 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  	int i;
>  
>  	for (i = 0; i < PM_SUSPEND_MAX; i++) {
> -		if (pm_states[i] && valid_state(i))
> +		if (pm_states[i] && policies[policy].valid_state(i))
>  			s += sprintf(s,"%s ", pm_states[i]);
>  	}
>  #endif
>  #ifdef CONFIG_HIBERNATION
> -	s += sprintf(s, "%s\n", "disk");
> +	if (!policy)
> +		s += sprintf(s, "%s\n", "disk");
>  #else
>  	if (s != buf)
>  		/* convert the last space to a newline */
> @@ -173,7 +202,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			   const char *buf, size_t n)
>  {
>  #ifdef CONFIG_SUSPEND
> -	suspend_state_t state = PM_SUSPEND_STANDBY;
> +	suspend_state_t state = PM_SUSPEND_ON;
>  	const char * const *s;
>  #endif
>  	char *p;
> @@ -184,7 +213,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	len = p ? p - buf : n;
>  
>  	/* First, check if we are requested to hibernate */
> -	if (len == 4 && !strncmp(buf, "disk", len)) {
> +	if (len == 4 && !strncmp(buf, "disk", len) && !policy) {
>  		error = hibernate();
>    goto Exit;
>  	}
> @@ -195,7 +224,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			break;
>  	}
>  	if (state < PM_SUSPEND_MAX && *s)
> -		error = enter_state(state);
> +		error = policies[policy].set_state(state);
>  #endif
>  
>   Exit:
> @@ -204,6 +233,55 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(state);
>  
> +/**
> + *	policy - set policy for state
> + */
> +
> +static ssize_t policy_show(struct kobject *kobj,
> +			   struct kobj_attribute *attr, char *buf)
> +{
> +	char *s = buf;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		if (i == policy)
> +			s += sprintf(s, "[%s] ", policies[i].name);
> +		else
> +			s += sprintf(s, "%s ", policies[i].name);
> +	}
> +	if (s != buf)
> +		/* convert the last space to a newline */
> +		*(s-1) = '\n';
> +	return (s - buf);
> +}
> +
> +static ssize_t policy_store(struct kobject *kobj,
> +			    struct kobj_attribute *attr,
> +			    const char *buf, size_t n)
> +{
> +	const char *s;
> +	char *p;
> +	int len;
> +	int i;
> +
> +	p = memchr(buf, '\n', n);
> +	len = p ? p - buf : n;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		s = policies[i].name;
> +		if (s && len == strlen(s) && !strncmp(buf, s, len)) {
> +			mutex_lock(&pm_mutex);
> +			policies[policy].set_state(PM_SUSPEND_ON);
> +			policy = i;
> +			mutex_unlock(&pm_mutex);
> +			return n;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +power_attr(policy);
> +
>  #ifdef CONFIG_PM_TRACE
>  int pm_trace_enabled;
>  
> @@ -231,6 +309,7 @@ power_attr(pm_trace);
>  
>  static struct attribute * g[] = {
>  	&state_attr.attr,
> +	&policy_attr.attr,
>  #ifdef CONFIG_PM_TRACE
>  	&pm_trace_attr.attr,
>  #endif
> @@ -247,7 +326,7 @@ static struct attribute_group attr_group = {
>  	.attrs = g,
>  };
>  
> -#ifdef CONFIG_PM_RUNTIME
> +#if defined(CONFIG_PM_RUNTIME) || defined(CONFIG_OPPORTUNISTIC_SUSPEND)
>  struct workqueue_struct *pm_wq;
>  EXPORT_SYMBOL_GPL(pm_wq);
>  
> @@ -266,6 +345,7 @@ static int __init pm_init(void)
>  	int error = pm_start_workqueue();
>  	if (error)
>  		return error;
> +	suspend_block_init();
>  	power_kobj = kobject_create_and_add("power", NULL);
>  	if (!power_kobj)
>  		return -ENOMEM;
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 46c5a26..67d10fd 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -236,3 +236,12 @@ static inline void suspend_thaw_processes(void)
>  {
>  }
>  #endif
> +
> +/* kernel/power/suspend_block.c */
> +extern int request_suspend_state(suspend_state_t state);
> +extern bool request_suspend_valid_state(suspend_state_t state);
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +extern void __init suspend_block_init(void);
> +#else
> +static inline void suspend_block_init(void) {}
> +#endif
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 56e7dbb..dc42006 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -16,10 +16,12 @@
>  #include <linux/cpu.h>
>  #include <linux/syscalls.h>
>  #include <linux/gfp.h>
> +#include <linux/suspend_blocker.h>
>  
>  #include "power.h"
>  
>  const char *const pm_states[PM_SUSPEND_MAX] = {
> +	[PM_SUSPEND_ON]		= "on",
>  	[PM_SUSPEND_STANDBY]	= "standby",
>  	[PM_SUSPEND_MEM]	= "mem",
>  };
> @@ -157,7 +159,7 @@ static int suspend_enter(suspend_state_t state)
>  
>  	error = sysdev_suspend(PMSG_SUSPEND);
>  	if (!error) {
> -		if (!suspend_test(TEST_CORE))
> +		if (!suspend_is_blocked() && !suspend_test(TEST_CORE))
>  			error = suspend_ops->enter(state);
>  		sysdev_resume();
>  	}
> diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
> new file mode 100644
> index 0000000..2c58b21
> --- /dev/null
> +++ b/kernel/power/suspend_blocker.c
> @@ -0,0 +1,261 @@
> +/* kernel/power/suspend_blocker.c
> + *
> + * Copyright (C) 2005-2010 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_blocker.h>
> +#include "power.h"
> +
> +extern struct workqueue_struct *pm_wq;
> +
> +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(list_lock);
> +static DEFINE_SPINLOCK(state_lock);
> +static LIST_HEAD(inactive_blockers);
> +static LIST_HEAD(active_blockers);
> +static int current_event_num;
> +struct suspend_blocker main_suspend_blocker;
> +static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
> +static bool enable_suspend_blockers;
> +
> +#define pr_info_time(fmt, args...) \
> +	do { \
> +		struct timespec ts; \
> +		struct rtc_time tm; \
> +		getnstimeofday(&ts); \
> +		rtc_time_to_tm(ts.tv_sec, &tm); \
> +		pr_info(fmt "(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n" , \
> +			args, \
> +			tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, \
> +			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
> +	} while (0);
> +
> +static void print_active_blockers_locked(void)
> +{
> +	struct suspend_blocker *blocker;
> +
> +	list_for_each_entry(blocker, &active_blockers, link)
> +		pr_info("active suspend blocker %s\n", blocker->name);
> +}
> +
> +/**
> + * suspend_is_blocked() - Check if suspend should be blocked
> + *
> + * 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)
> +{
> +	if (!enable_suspend_blockers)
> +		return 0;
> +	return !list_empty(&active_blockers);
> +}
> +
> +static void suspend_worker(struct work_struct *work)
> +{
> +	int ret;
> +	int entry_event_num;
> +
> +	enable_suspend_blockers = true;
> +	while (!suspend_is_blocked()) {
> +		entry_event_num = 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)
> +			pr_info_time("suspend: exit suspend, ret = %d ", ret);
> +
> +		if (current_event_num == entry_event_num)
> +			pr_info("suspend: pm_suspend returned with no event\n");
> +	}
> +	enable_suspend_blockers = false;
> +}
> +static DECLARE_WORK(suspend_work, suspend_worker);
> +
> +/**
> + * suspend_blocker_init() - Initialize a suspend blocker
> + * @blocker:	The suspend blocker to initialize.
> + * @name:	The name of the suspend blocker to show in debug messages.
> + *
> + * The suspend blocker struct and name must not be freed before calling
> + * suspend_blocker_destroy.
> + */
> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)
> +{
> +	unsigned long irqflags = 0;
> +
> +	WARN_ON(!name);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_init name=%s\n", name);
> +
> +	blocker->name = name;
> +	blocker->flags = SB_INITIALIZED;
> +	INIT_LIST_HEAD(&blocker->link);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_add(&blocker->link, &inactive_blockers);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +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)
> +{
> +	unsigned long irqflags;
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	blocker->flags &= ~SB_INITIALIZED;
> +	list_del(&blocker->link);
> +	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
> +		queue_work(pm_wq, &suspend_work);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_blocker_destroy);
> +
> +/**
> + * suspend_block() - Block suspend
> + * @blocker:	The suspend blocker to use
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_block(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_block: %s\n", blocker->name);
> +
> +	blocker->flags |= SB_ACTIVE;
> +	list_move(&blocker->link, &active_blockers);
> +	current_event_num++;
> +
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +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.
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_unblock(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_unblock: %s\n", blocker->name);
> +
> +	list_move(&blocker->link, &inactive_blockers);
> +
> +	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
> +		queue_work(pm_wq, &suspend_work);
> +	blocker->flags &= ~(SB_ACTIVE);
> +	if (blocker == &main_suspend_blocker) {
> +		if (debug_mask & DEBUG_SUSPEND)
> +			print_active_blockers_locked();
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_unblock);
> +
> +/**
> + * suspend_blocker_is_active() - Test if a suspend blocker is blocking suspend
> + * @blocker:	The suspend blocker to check.
> + *
> + * Returns true if the suspend_blocker is currently active.
> + */
> +bool suspend_blocker_is_active(struct suspend_blocker *blocker)
> +{
> +	WARN_ON(!(blocker->flags & SB_INITIALIZED));
> +
> +	return !!(blocker->flags & SB_ACTIVE);
> +}
> +EXPORT_SYMBOL(suspend_blocker_is_active);
> +
> +bool request_suspend_valid_state(suspend_state_t state)
> +{
> +	return (state == PM_SUSPEND_ON) || valid_state(state);
> +}
> +
> +int request_suspend_state(suspend_state_t state)
> +{
> +	unsigned long irqflags;
> +
> +	if (!request_suspend_valid_state(state))
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&state_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_USER_STATE)
> +		pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
> +			     state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> +			     requested_suspend_state, state,
> +			     ktime_to_ns(ktime_get()));
> +
> +	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);
> +	return 0;
> +}
> +
> +void __init suspend_block_init(void)
> +{
> +	suspend_blocker_init(&main_suspend_blocker, "main");
> +	suspend_block(&main_suspend_blocker);
> +}
> -- 
> 1.6.5.1
> 
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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

[Alan Stern]

On Mon, 3 May 2010, mark gross wrote:

> You know things would be so much easier if the policy was a one-shot
> styled thing.  i.e. when enabled it does what it does, but upon resume
> the policy must be re-enabled by user mode to get the opportunistic
> behavior.  That way we don't need to grab the suspend blocker from the
> wake up irq handler all the way up to user mode as the example below
> calls out.  I suppose doing this would put a burden on the user mode code
> to keep track of if it has no pending blockers registered after a wake
> up from the suspend.  but that seems nicer to me than sprinkling
> overlapping blocker critical sections from the mettle up to user mode.
> 
> Please consider making the policy a one shot API that needs to be
> re-enabled after resume by user mode.  That would remove my issue with
> the design.

This won't work right if a second IRQ arrives while the first is being
processed.  Suppose the kernel driver for the second IRQ doesn't
activate a suspend blocker, and suppose all the userspace handlers for
the first IRQ end (and the opportunistic policy is re-enabled) before
the userspace handler for the second IRQ can start.  Then the system
will go back to sleep before userspace can handle the second IRQ.

Alan Stern

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

[mark gross]

On Tue, May 04, 2010 at 09:59:59AM -0400, Alan Stern wrote:
> On Mon, 3 May 2010, mark gross wrote:
> 
> > You know things would be so much easier if the policy was a one-shot
> > styled thing.  i.e. when enabled it does what it does, but upon resume
> > the policy must be re-enabled by user mode to get the opportunistic
> > behavior.  That way we don't need to grab the suspend blocker from the
> > wake up irq handler all the way up to user mode as the example below
> > calls out.  I suppose doing this would put a burden on the user mode code
> > to keep track of if it has no pending blockers registered after a wake
> > up from the suspend.  but that seems nicer to me than sprinkling
> > overlapping blocker critical sections from the mettle up to user mode.
> > 
> > Please consider making the policy a one shot API that needs to be
> > re-enabled after resume by user mode.  That would remove my issue with
> > the design.
> 
> This won't work right if a second IRQ arrives while the first is being
> processed.  Suppose the kernel driver for the second IRQ doesn't
> activate a suspend blocker, and suppose all the userspace handlers for
> the first IRQ end (and the opportunistic policy is re-enabled) before
> the userspace handler for the second IRQ can start.  Then the system
> will go back to sleep before userspace can handle the second IRQ.
>

What?  If the suspend blocker API was a one shot styled api, after the
suspend, the one shot is over and the behavior is that you do not fall
back into suspend again until user mode re-enables the one-shot
behavior.  

With what I am proposing the next suspend would not happen until the
user mode code re-enables the suspend blocking behavior.  It would much
cleaner for everyone and I see zero down side WRT the example you just
posted.  

If user mode triggers a suspend by releasing the last blocker, you have
until pm_suspend('mem') turns off interrupts to cancel it.  That race
will never go away. 

Let me think this through, please check that I'm understanding the issue
please:
1) one-shot policy enable blocker PM suspend behavior;
2) release last blocker and call pm_suspend('mem') and suspend all the
way down.
3) get wake up event, one-shot policy over, no-blockers in list
4) go all the way to user mode, 
5) user mode decides to not grab a blocker, and re-enables one-shot
policy
6) pm_suspend('mem') called
7) interrupt comes in (say the modem rings)
8) modem driver handler needs to returns fail from pm_suspend call back,
device resumes (I am proposing changing the posted api for doing this.)
9) user mode figures out one-shot needs to be re-enabled and it grabs a
blocker to handle the call and re-enables the one-shot.

So the real problem grabbing blockers from ISR's trying to solve is to
reduce the window of time where a pm_suspend('mem') can be canceled.

My proposal is to;
1) change the kernel blocker api to be
"cancel-one-shot-policy-enable" that can be called from the ISR's for
the wake up devices before the IRQ's are disabled to cancel an in-flight
suspend.  I would make it 2 macros one goes in the pm_suspend callback
to return fail, and the other in the ISR to disable the one-shot-policy.
 
2) make the policy a one shot that user mode must re-enable after each
suspend attempted.

Would it help if I coded up the above proposal as patch to the current
(rev-6) of the blocker patchset?  I'm offering.

Because this part of the current design is broken, in that it demands
the sprinkling of blocker critical sections from the hardware up to the
user mode.  Its ugly, hard to get right and if more folks would take a
look at how well it worked out for the existing kernels on
android.git.kernel.org/kernels/ perhaps it would get more attention.

Finally, as an aside, lets look at the problem with the posted rev-6
patches:
1) enable suspend blocker policy
2) release user-mode blocker
3) start suspend
4) get a modem interrupt (incoming call)
5) ooops, IRQ came in after pm_suspend('mem') turned off interrupts.
bummer for you, thats life with this design.  Better hope your modem
hardware is modified to keep pulling on that wake up line because you're
past the point of no return now and going to suspend.

Grabbing a blocker in the ISR doesn't solve this.  So your hardware
needs to have a persistent wake up trigger that is robust against this
scenario.  And, if you have such hardware then why are we killing
ourselves to maximize the window of time between pm_suspend('mem') and
platform suspend where you can cancel the suspend?  The hardware will
simply wake up anyway.

(further, on my hardware I would modify the platform suspend call back
to make one last check to see if an IRQ came in, and cancel the suspend
there as a last chance.)

 
--mgross

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

[Alan Stern]

On Tue, 4 May 2010, mark gross wrote:

> On Tue, May 04, 2010 at 09:59:59AM -0400, Alan Stern wrote:
> > On Mon, 3 May 2010, mark gross wrote:
> > 
> > > You know things would be so much easier if the policy was a one-shot
> > > styled thing.  i.e. when enabled it does what it does, but upon resume
> > > the policy must be re-enabled by user mode to get the opportunistic
> > > behavior.  That way we don't need to grab the suspend blocker from the
> > > wake up irq handler all the way up to user mode as the example below
> > > calls out.  I suppose doing this would put a burden on the user mode code
> > > to keep track of if it has no pending blockers registered after a wake
> > > up from the suspend.  but that seems nicer to me than sprinkling
> > > overlapping blocker critical sections from the mettle up to user mode.
> > > 
> > > Please consider making the policy a one shot API that needs to be
> > > re-enabled after resume by user mode.  That would remove my issue with
> > > the design.
> > 
> > This won't work right if a second IRQ arrives while the first is being
> > processed.  Suppose the kernel driver for the second IRQ doesn't
> > activate a suspend blocker, and suppose all the userspace handlers for
> > the first IRQ end (and the opportunistic policy is re-enabled) before
> > the userspace handler for the second IRQ can start.  Then the system
> > will go back to sleep before userspace can handle the second IRQ.
> >
> 
> What?  If the suspend blocker API was a one shot styled api, after the
> suspend, the one shot is over and the behavior is that you do not fall
> back into suspend again until user mode re-enables the one-shot
> behavior.  

I was referring to your sentence: "That way we don't need to grab the 
suspend blocker from the wake up irq handler all the way up to user 
mode..."  The problem arises when kernel handlers don't do _something_ 
to prevent another suspend from occurring too soon.

> With what I am proposing the next suspend would not happen until the
> user mode code re-enables the suspend blocking behavior.  It would much
> cleaner for everyone and I see zero down side WRT the example you just
> posted.  
> 
> If user mode triggers a suspend by releasing the last blocker, you have
> until pm_suspend('mem') turns off interrupts to cancel it.  That race
> will never go away. 

(Actually the kernel can cancel the suspend even after interrupts are
turned off, if it has a reason to do so.)

In theory the race between interrupts and suspend don't need to be a
problem.  In practice it still is -- the PM core needs a few changes to
allow wakeup interrupts to cancel a suspend in progress.  Regardless, 
that's not what I was talking about.

> Let me think this through, please check that I'm understanding the issue
> please:
> 1) one-shot policy enable blocker PM suspend behavior;
> 2) release last blocker and call pm_suspend('mem') and suspend all the
> way down.
> 3) get wake up event, one-shot policy over, no-blockers in list
> 4) go all the way to user mode, 
> 5) user mode decides to not grab a blocker, and re-enables one-shot
> policy
> 6) pm_suspend('mem') called
> 7) interrupt comes in (say the modem rings)
> 8) modem driver handler needs to returns fail from pm_suspend call back,
> device resumes (I am proposing changing the posted api for doing this.)
> 9) user mode figures out one-shot needs to be re-enabled and it grabs a
> blocker to handle the call and re-enables the one-shot.

This is not the scenario I was describing.  Here's what I had in mind:

1) The system is asleep
2) Wakeup event occurs, one-shot policy over
3) Go all the way to user mode
4) A second wakeup interrupt occurs (say the modem rings)
5) The modem driver does not enable any suspend blockers
6) The modem driver queues an input event for userspace
7) The userspace handler invoked during 3) finishes and re-enables
	the one-shot policy
8) No suspend blockers are enabled, so the system goes to sleep
9) The userspace handler for the input event in 6) doesn't get to run

> So the real problem grabbing blockers from ISR's trying to solve is to
> reduce the window of time where a pm_suspend('mem') can be canceled.

No.  The real problem is how ISRs should prevent the system from
suspending before the events they generate can be handled by userspace.

> My proposal is to;
> 1) change the kernel blocker api to be
> "cancel-one-shot-policy-enable" that can be called from the ISR's for
> the wake up devices before the IRQ's are disabled to cancel an in-flight
> suspend.  I would make it 2 macros one goes in the pm_suspend callback
> to return fail, and the other in the ISR to disable the one-shot-policy.
>  
> 2) make the policy a one shot that user mode must re-enable after each
> suspend attempted.

Neither of these solves the race I described.

> Would it help if I coded up the above proposal as patch to the current
> (rev-6) of the blocker patchset?  I'm offering.
> 
> Because this part of the current design is broken, in that it demands
> the sprinkling of blocker critical sections from the hardware up to the
> user mode.  Its ugly, hard to get right and if more folks would take a
> look at how well it worked out for the existing kernels on
> android.git.kernel.org/kernels/ perhaps it would get more attention.

I agree, it is ugly and probably hard to get right.  But something like 
it is necessary to solve this race.

> Finally, as an aside, lets look at the problem with the posted rev-6
> patches:
> 1) enable suspend blocker policy
> 2) release user-mode blocker
> 3) start suspend
> 4) get a modem interrupt (incoming call)
> 5) ooops, IRQ came in after pm_suspend('mem') turned off interrupts.
> bummer for you, thats life with this design.  Better hope your modem
> hardware is modified to keep pulling on that wake up line because you're
> past the point of no return now and going to suspend.

That is not a problem for level-sensitive IRQs.  If interrupts have
been turned off then the modem will not receive any commands telling
it to stop requesting an interrupt.  So the IRQ line will remain active 
until the system goes to sleep, at which point it will immediately 
wake up the system.

For edge-sensitive interrupts the situation isn't as simple.  The
low-level IRQ code must handle this somehow.

> Grabbing a blocker in the ISR doesn't solve this.  So your hardware
> needs to have a persistent wake up trigger that is robust against this
> scenario.  And, if you have such hardware then why are we killing
> ourselves to maximize the window of time between pm_suspend('mem') and
> platform suspend where you can cancel the suspend?  The hardware will
> simply wake up anyway.

That's not what we are killing ourselves for.  The point of suspend
blockers is not to prevent races with the hardware.  It is to prevent 
userspace from being frozen while there's still work to be done.

Alan Stern

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

[Arve Hjønnevåg]

2010/5/3 mark gross <640e9920@gmail.com>:
> On Fri, Apr 30, 2010 at 03:36:54PM -0700, Arve Hjønnevåg wrote:
...
>> +When the policy is "opportunisic", there is a special value, "on", that can be
>> +written to /sys/power/state. This will block the automatic sleep request, as if
>> +a suspend blocker was used by a device driver. This way the opportunistic
>> +suspend may be blocked by user space whithout switching back to the "forced"
>> +mode.
>
> You know things would be so much easier if the policy was a one-shot
> styled thing.  i.e. when enabled it does what it does, but upon resume
> the policy must be re-enabled by user mode to get the opportunistic
> behavior.  That way we don't need to grab the suspend blocker from the
> wake up irq handler all the way up to user mode as the example below
> calls out.  I suppose doing this would put a burden on the user mode code
> to keep track of if it has no pending blockers registered after a wake
> up from the suspend.  but that seems nicer to me than sprinkling
> overlapping blocker critical sections from the mettle up to user mode.
>
> Please consider making the policy a one shot API that needs to be
> re-enabled after resume by user mode.  That would remove my issue with
> the design.
>

Making it one shot does not change where kernel code needs to block
suspend, but it does force user space to poll trying to suspend while
suspend is blocked by a driver.

-- 
Arve Hjønnevåg

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

[mark gross]

On Tue, May 04, 2010 at 01:16:25PM -0400, Alan Stern wrote:
> On Tue, 4 May 2010, mark gross wrote:
> 
> > On Tue, May 04, 2010 at 09:59:59AM -0400, Alan Stern wrote:
> > > On Mon, 3 May 2010, mark gross wrote:
> > > 
> > > > You know things would be so much easier if the policy was a one-shot
> > > > styled thing.  i.e. when enabled it does what it does, but upon resume
> > > > the policy must be re-enabled by user mode to get the opportunistic
> > > > behavior.  That way we don't need to grab the suspend blocker from the
> > > > wake up irq handler all the way up to user mode as the example below
> > > > calls out.  I suppose doing this would put a burden on the user mode code
> > > > to keep track of if it has no pending blockers registered after a wake
> > > > up from the suspend.  but that seems nicer to me than sprinkling
> > > > overlapping blocker critical sections from the mettle up to user mode.
> > > > 
> > > > Please consider making the policy a one shot API that needs to be
> > > > re-enabled after resume by user mode.  That would remove my issue with
> > > > the design.
> > > 
> > > This won't work right if a second IRQ arrives while the first is being
> > > processed.  Suppose the kernel driver for the second IRQ doesn't
> > > activate a suspend blocker, and suppose all the userspace handlers for
> > > the first IRQ end (and the opportunistic policy is re-enabled) before
> > > the userspace handler for the second IRQ can start.  Then the system
> > > will go back to sleep before userspace can handle the second IRQ.
> > >
> > 
> > What?  If the suspend blocker API was a one shot styled api, after the
> > suspend, the one shot is over and the behavior is that you do not fall
> > back into suspend again until user mode re-enables the one-shot
> > behavior.  
> 
> I was referring to your sentence: "That way we don't need to grab the 
> suspend blocker from the wake up irq handler all the way up to user 
> mode..."  The problem arises when kernel handlers don't do _something_ 
> to prevent another suspend from occurring too soon.
> 
> > With what I am proposing the next suspend would not happen until the
> > user mode code re-enables the suspend blocking behavior.  It would much
> > cleaner for everyone and I see zero down side WRT the example you just
> > posted.  
> > 
> > If user mode triggers a suspend by releasing the last blocker, you have
> > until pm_suspend('mem') turns off interrupts to cancel it.  That race
> > will never go away. 
> 
> (Actually the kernel can cancel the suspend even after interrupts are
> turned off, if it has a reason to do so.)
> 
> In theory the race between interrupts and suspend don't need to be a
> problem.  In practice it still is -- the PM core needs a few changes to
> allow wakeup interrupts to cancel a suspend in progress.  Regardless, 
> that's not what I was talking about.
> 
> > Let me think this through, please check that I'm understanding the issue
> > please:
> > 1) one-shot policy enable blocker PM suspend behavior;
> > 2) release last blocker and call pm_suspend('mem') and suspend all the
> > way down.
> > 3) get wake up event, one-shot policy over, no-blockers in list
> > 4) go all the way to user mode, 
> > 5) user mode decides to not grab a blocker, and re-enables one-shot
> > policy
> > 6) pm_suspend('mem') called
> > 7) interrupt comes in (say the modem rings)
> > 8) modem driver handler needs to returns fail from pm_suspend call back,
> > device resumes (I am proposing changing the posted api for doing this.)
> > 9) user mode figures out one-shot needs to be re-enabled and it grabs a
> > blocker to handle the call and re-enables the one-shot.
> 
> This is not the scenario I was describing.  Here's what I had in mind:
> 
> 1) The system is asleep
> 2) Wakeup event occurs, one-shot policy over
> 3) Go all the way to user mode
> 4) A second wakeup interrupt occurs (say the modem rings)
> 5) The modem driver does not enable any suspend blockers
> 6) The modem driver queues an input event for userspace
> 7) The userspace handler invoked during 3) finishes and re-enables
> 	the one-shot policy
> 8) No suspend blockers are enabled, so the system goes to sleep
In my sequence above I had the modem driver "magically" knowing to fail
this suspend attempt.  (that "magic" wasn't fully thought out though.)
> 9) The userspace handler for the input event in 6) doesn't get to run
> 
> > So the real problem grabbing blockers from ISR's trying to solve is to
> > reduce the window of time where a pm_suspend('mem') can be canceled.
> 
> No.  The real problem is how ISRs should prevent the system from
> suspending before the events they generate can be handled by userspace.

Thanks, I think I'm starting to get it.  From this it seems that the
system integrator needs to identify those wake up sources that need to
be able to block a suspend, and figure out a way of acknowledging from
user mode, that its now ok to allow a suspend to happen.

The rev-6 proposed way is for the integrator to implement overlapping
blocker sections from ISR up to user mode for selected wake up devices
(i.e. the modem)

There *has* to be a better way.

Can't we have some notification based thing that supports user mode
acks through a misc device or sysfs thing?   Anything to avoid the
overlapping blocker sections. 


> > My proposal is to;
> > 1) change the kernel blocker api to be
> > "cancel-one-shot-policy-enable" that can be called from the ISR's for
> > the wake up devices before the IRQ's are disabled to cancel an in-flight
> > suspend.  I would make it 2 macros one goes in the pm_suspend callback
> > to return fail, and the other in the ISR to disable the one-shot-policy.
> >  
> > 2) make the policy a one shot that user mode must re-enable after each
> > suspend attempted.
> 
> Neither of these solves the race I described.

True, you need an ack back from user mode for when its ok to allow
suspend to happen.  This ack is device specific and needs to be custom
built per product to its wake up sources.

> 
> > Would it help if I coded up the above proposal as patch to the current
> > (rev-6) of the blocker patchset?  I'm offering.
> > 
> > Because this part of the current design is broken, in that it demands
> > the sprinkling of blocker critical sections from the hardware up to the
> > user mode.  Its ugly, hard to get right and if more folks would take a
> > look at how well it worked out for the existing kernels on
> > android.git.kernel.org/kernels/ perhaps it would get more attention.
> 
> I agree, it is ugly and probably hard to get right.  But something like 
> it is necessary to solve this race.
> 
> > Finally, as an aside, lets look at the problem with the posted rev-6
> > patches:
> > 1) enable suspend blocker policy
> > 2) release user-mode blocker
> > 3) start suspend
> > 4) get a modem interrupt (incoming call)
> > 5) ooops, IRQ came in after pm_suspend('mem') turned off interrupts.
> > bummer for you, thats life with this design.  Better hope your modem
> > hardware is modified to keep pulling on that wake up line because you're
> > past the point of no return now and going to suspend.
> 
> That is not a problem for level-sensitive IRQs.  If interrupts have
> been turned off then the modem will not receive any commands telling
> it to stop requesting an interrupt.  So the IRQ line will remain active 
> until the system goes to sleep, at which point it will immediately 
> wake up the system.
> 
> For edge-sensitive interrupts the situation isn't as simple.  The
> low-level IRQ code must handle this somehow.
> 
> > Grabbing a blocker in the ISR doesn't solve this.  So your hardware
> > needs to have a persistent wake up trigger that is robust against this
> > scenario.  And, if you have such hardware then why are we killing
> > ourselves to maximize the window of time between pm_suspend('mem') and
> > platform suspend where you can cancel the suspend?  The hardware will
> > simply wake up anyway.
> 
> That's not what we are killing ourselves for.  The point of suspend
> blockers is not to prevent races with the hardware.  It is to prevent 
> userspace from being frozen while there's still work to be done.
ok.

I'm going to think on this some more.  There must be a cleaner way to do
this.

--mgross
 

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

[Matthew Garrett]

On Tue, May 04, 2010 at 06:50:50PM -0700, mark gross wrote:

> In my sequence above I had the modem driver "magically" knowing to fail
> this suspend attempt.  (that "magic" wasn't fully thought out though.)

If the modem driver knows to "magically" fail a suspend attempt until it 
knows that userspace has consumed the event, you have something that 
looks awfully like suspend blockers.

> There *has* to be a better way.

But nobody has reasonably proposed one and demonstrated that it works. 
We've had over a year to do so and failed, and I think it's pretty 
unreasonable to ask Google to attempt to rearchitect based on a 
hypothetical.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Alan Stern]

On Tue, 4 May 2010, mark gross wrote:

> Thanks, I think I'm starting to get it.  From this it seems that the
> system integrator needs to identify those wake up sources that need to
> be able to block a suspend, and figure out a way of acknowledging from
> user mode, that its now ok to allow a suspend to happen.

The second part is easy.  Userspace doesn't need to do anything special 
to acknowledge that a suspend is now okay; it just has to remove the 
conditions that led the driver to block suspends in the first place.

For example, if suspends are blocked because some input event has been
queued, emptying the input event queue should unblock suspends.

> The rev-6 proposed way is for the integrator to implement overlapping
> blocker sections from ISR up to user mode for selected wake up devices
> (i.e. the modem)
> 
> There *has* to be a better way.

Why?  What's wrong with overlapping blockers?  It's a very common
idiom.  For example, the same sort of thing is used when locking
subtrees of a tree: You lock the root node, and then use overlapping
locks on the nodes leading down to the subtree you're interested in.

> Can't we have some notification based thing that supports user mode
> acks through a misc device or sysfs thing?   Anything to avoid the
> overlapping blocker sections. 

Userspace acks aren't the issue; the issue is how (and when) kernel 
drivers should initiate a blocker.  Switching to notifications, misc 
devices, or sysfs won't help solve this issue.

> True, you need an ack back from user mode for when its ok to allow
> suspend to happen.  This ack is device specific and needs to be custom
> built per product to its wake up sources.

No and no.  Nothing special is needed.  All userspace needs to do is 
remove the condition that led to the blocker being enabled initially -- 
which is exactly what userspace would do normally anyway.

Alan Stern

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

[mark gross]

On Wed, May 05, 2010 at 02:31:31PM +0100, Matthew Garrett wrote:
> On Tue, May 04, 2010 at 06:50:50PM -0700, mark gross wrote:
> 
> > In my sequence above I had the modem driver "magically" knowing to fail
> > this suspend attempt.  (that "magic" wasn't fully thought out though.)
> 
> If the modem driver knows to "magically" fail a suspend attempt until it 
> knows that userspace has consumed the event, you have something that 
> looks awfully like suspend blockers.
> 
> > There *has* to be a better way.
> 
> But nobody has reasonably proposed one and demonstrated that it works. 
> We've had over a year to do so and failed, and I think it's pretty 
> unreasonable to ask Google to attempt to rearchitect based on a 
> hypothetical.
>

These are not new issues being raised. They've had over a year to
address them, and all thats really happened was some sed script changes
from wake_lock to suspend_blocker.  Nothing is really different
here.

Rearchitecting out of tree code is as silly thing for you to expect from
a community member.  

sigh, lets stop wasting time and just merge it then.

I'm finished with this thread until I do some rearchecting and post
something that looks better to me.  I'll look for this stuff in 2.6.34
or 35.

--mgross 
ps It think the name suspend blocker is worse than wake-lock.  I'd
change it back.

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

[Matthew Garrett]

On Wed, May 05, 2010 at 01:09:06PM -0700, mark gross wrote:
> On Wed, May 05, 2010 at 02:31:31PM +0100, Matthew Garrett wrote:
> > But nobody has reasonably proposed one and demonstrated that it works. 
> > We've had over a year to do so and failed, and I think it's pretty 
> > unreasonable to ask Google to attempt to rearchitect based on a 
> > hypothetical.
> >
> 
> These are not new issues being raised. They've had over a year to
> address them, and all thats really happened was some sed script changes
> from wake_lock to suspend_blocker.  Nothing is really different
> here.

Our issues haven't been addressed because we've given no indication as 
to how they can be addressed. For better or worse, our runtime 
powermanagement story isn't sufficient to satisfy Google's usecases. 
That would be fine, if we could tell them what changes needed to be made 
to satisfy their usecases. The Android people have said that they don't 
see a cleaner way of doing this. Are we seriously saying that they 
should prove themselves wrong, and if they can't they don't get their 
code in the kernel? This seems... problematic.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[mark gross]

On Wed, May 05, 2010 at 11:44:39AM -0400, Alan Stern wrote:
> On Tue, 4 May 2010, mark gross wrote:
> 
> > Thanks, I think I'm starting to get it.  From this it seems that the
> > system integrator needs to identify those wake up sources that need to
> > be able to block a suspend, and figure out a way of acknowledging from
> > user mode, that its now ok to allow a suspend to happen.
> 
> The second part is easy.  Userspace doesn't need to do anything special 
> to acknowledge that a suspend is now okay; it just has to remove the 
> conditions that led the driver to block suspends in the first place.
> 
> For example, if suspends are blocked because some input event has been
> queued, emptying the input event queue should unblock suspends.
> 
> > The rev-6 proposed way is for the integrator to implement overlapping
> > blocker sections from ISR up to user mode for selected wake up devices
> > (i.e. the modem)
> > 
> > There *has* to be a better way.
> 
> Why?  What's wrong with overlapping blockers?  It's a very common
> idiom.  For example, the same sort of thing is used when locking
> subtrees of a tree: You lock the root node, and then use overlapping
> locks on the nodes leading down to the subtree you're interested in.

Because in the kenel there is only a partial ordering of calling
sequences from IRQ to usermode.  I see a lot of custom out of tree code
being developed to deal with getting the overlapping blocker sections
right, per device.

 
> > Can't we have some notification based thing that supports user mode
> > acks through a misc device or sysfs thing?   Anything to avoid the
> > overlapping blocker sections. 
> 
> Userspace acks aren't the issue; the issue is how (and when) kernel 
> drivers should initiate a blocker.  Switching to notifications, misc 
> devices, or sysfs won't help solve this issue.

communicating non-local knowledge back down to the blocking object to
tell it that it can unblock is the issue
 
> > True, you need an ack back from user mode for when its ok to allow
> > suspend to happen.  This ack is device specific and needs to be custom
> > built per product to its wake up sources.
> 
> No and no.  Nothing special is needed.  All userspace needs to do is 
> remove the condition that led to the blocker being enabled initially -- 
> which is exactly what userspace would do normally anyway.

Oh, like tell the modem that user mode has handled the ring event and
its ok to un-block?

--mgross

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

[Alan Stern]

On Wed, 5 May 2010, mark gross wrote:

> > > True, you need an ack back from user mode for when its ok to allow
> > > suspend to happen.  This ack is device specific and needs to be custom
> > > built per product to its wake up sources.
> > 
> > No and no.  Nothing special is needed.  All userspace needs to do is 
> > remove the condition that led to the blocker being enabled initially -- 
> > which is exactly what userspace would do normally anyway.
> 
> Oh, like tell the modem that user mode has handled the ring event and
> its ok to un-block?

No, that's not how it works.  It would go like this:

	The modem IRQ handler queues its event to the input subsystem.  
	As it does so the input subsystem enables a suspend blocker, 
	causing the system to stay awake after the IRQ is done.

	The user program enables its own suspend blocker before reading
	the input queue.  When the queue is empty, the input subsystem
	releases its suspend blocker.

	When the user program finishes processing the event, it
	releases its suspend blocker.  Now the system can go back to
	sleep.

At no point does the user program have to communicate anything to the 
modem driver, and at no point does it have to do anything out of the 
ordinary except to enable and disable a suspend blocker.

Alan Stern

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

[Brian Swetland]

On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>
>> Oh, like tell the modem that user mode has handled the ring event and
>> its ok to un-block?
>
> No, that's not how it works.  It would go like this:
>
>        The modem IRQ handler queues its event to the input subsystem.
>        As it does so the input subsystem enables a suspend blocker,
>        causing the system to stay awake after the IRQ is done.
>
>        The user program enables its own suspend blocker before reading
>        the input queue.  When the queue is empty, the input subsystem
>        releases its suspend blocker.
>
>        When the user program finishes processing the event, it
>        releases its suspend blocker.  Now the system can go back to
>        sleep.
>
> At no point does the user program have to communicate anything to the
> modem driver, and at no point does it have to do anything out of the
> ordinary except to enable and disable a suspend blocker.

Exactly -- and you can use the same style of overlapping suspend
blockers with other drivers than input, if the input interface is not
suitable for the particular interaction.

Brian

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

[Tony Lindgren]

* Brian Swetland <swetland@google.com> [100505 14:34]:
> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >>
> >> Oh, like tell the modem that user mode has handled the ring event and
> >> its ok to un-block?
> >
> > No, that's not how it works.  It would go like this:
> >
> >        The modem IRQ handler queues its event to the input subsystem.
> >        As it does so the input subsystem enables a suspend blocker,
> >        causing the system to stay awake after the IRQ is done.

How about instead the modem driver fails to suspend until it's done?

Each driver could have a suspend_policy sysfs entry with options such
as [ forced | safe ]. The default would be forced. Forced would
be the current behaviour, while safe would refuse suspend until the
driver is done processing.

> >        The user program enables its own suspend blocker before reading
> >        the input queue.  When the queue is empty, the input subsystem
> >        releases its suspend blocker.

And also the input layer could refuse to suspend until it's done.

> >        When the user program finishes processing the event, it
> >        releases its suspend blocker.  Now the system can go back to
> >        sleep.

And here the user space just tries to suspend again when it's done?
It's not like you're trying to suspend all the time, so it should be
OK to retry a few times.

> > At no point does the user program have to communicate anything to the
> > modem driver, and at no point does it have to do anything out of the
> > ordinary except to enable and disable a suspend blocker.
> 
> Exactly -- and you can use the same style of overlapping suspend
> blockers with other drivers than input, if the input interface is not
> suitable for the particular interaction.

Would the suspend blockers still be needed somewhere in the example
above?

Regards,

Tony

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

[Brian Swetland]

On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Swetland <swetland@google.com> [100505 14:34]:
>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >>
>> >> Oh, like tell the modem that user mode has handled the ring event and
>> >> its ok to un-block?
>> >
>> > No, that's not how it works.  It would go like this:
>> >
>> >        The modem IRQ handler queues its event to the input subsystem.
>> >        As it does so the input subsystem enables a suspend blocker,
>> >        causing the system to stay awake after the IRQ is done.
>
> How about instead the modem driver fails to suspend until it's done?
>
> Each driver could have a suspend_policy sysfs entry with options such
> as [ forced | safe ]. The default would be forced. Forced would
> be the current behaviour, while safe would refuse suspend until the
> driver is done processing.
>
>> >        The user program enables its own suspend blocker before reading
>> >        the input queue.  When the queue is empty, the input subsystem
>> >        releases its suspend blocker.
>
> And also the input layer could refuse to suspend until it's done.
>
>> >        When the user program finishes processing the event, it
>> >        releases its suspend blocker.  Now the system can go back to
>> >        sleep.
>
> And here the user space just tries to suspend again when it's done?
> It's not like you're trying to suspend all the time, so it should be
> OK to retry a few times.

We actually are trying to suspend all the time -- that's our basic
model -- suspend whenever we can when something doesn't prevent it.

>> > At no point does the user program have to communicate anything to the
>> > modem driver, and at no point does it have to do anything out of the
>> > ordinary except to enable and disable a suspend blocker.
>>
>> Exactly -- and you can use the same style of overlapping suspend
>> blockers with other drivers than input, if the input interface is not
>> suitable for the particular interaction.
>
> Would the suspend blockers still be needed somewhere in the example
> above?

How often would we retry suspending?

If we fail to suspend, don't we have to resume all the drivers that
suspended before the one that failed?  (Maybe I'm mistaken here)

With the suspend block model we know the moment we're capable of
suspending and then can suspend at that moment.  Continually trying to
suspend seems like it'd be inefficient power-wise (we're going to be
doing a lot more work as we try to suspend over and over, or we're
going to retry after a timeout and spend extra time not suspended).

We can often spend minutes (possibly many) at a time preventing
suspend when the system is doing work that would be interrupted by a
full suspend.

Brian

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

[Tony Lindgren]

* Brian Swetland <swetland@google.com> [100505 16:51]:
> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Brian Swetland <swetland@google.com> [100505 14:34]:
> >> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >>
> >> >> Oh, like tell the modem that user mode has handled the ring event and
> >> >> its ok to un-block?
> >> >
> >> > No, that's not how it works.  It would go like this:
> >> >
> >> >        The modem IRQ handler queues its event to the input subsystem.
> >> >        As it does so the input subsystem enables a suspend blocker,
> >> >        causing the system to stay awake after the IRQ is done.
> >
> > How about instead the modem driver fails to suspend until it's done?
> >
> > Each driver could have a suspend_policy sysfs entry with options such
> > as [ forced | safe ]. The default would be forced. Forced would
> > be the current behaviour, while safe would refuse suspend until the
> > driver is done processing.
> >
> >> >        The user program enables its own suspend blocker before reading
> >> >        the input queue.  When the queue is empty, the input subsystem
> >> >        releases its suspend blocker.
> >
> > And also the input layer could refuse to suspend until it's done.
> >
> >> >        When the user program finishes processing the event, it
> >> >        releases its suspend blocker.  Now the system can go back to
> >> >        sleep.
> >
> > And here the user space just tries to suspend again when it's done?
> > It's not like you're trying to suspend all the time, so it should be
> > OK to retry a few times.
> 
> We actually are trying to suspend all the time -- that's our basic
> model -- suspend whenever we can when something doesn't prevent it.

Maybe that state could be kept in some userspace suspend policy manager?
 
> >> > At no point does the user program have to communicate anything to the
> >> > modem driver, and at no point does it have to do anything out of the
> >> > ordinary except to enable and disable a suspend blocker.
> >>
> >> Exactly -- and you can use the same style of overlapping suspend
> >> blockers with other drivers than input, if the input interface is not
> >> suitable for the particular interaction.
> >
> > Would the suspend blockers still be needed somewhere in the example
> > above?
> 
> How often would we retry suspending?

Well based on some timer, the same way the screen blanks? Or five
seconds of no audio play? So if the suspend fails, then reset whatever
userspace suspend policy timers.
 
> If we fail to suspend, don't we have to resume all the drivers that
> suspended before the one that failed?  (Maybe I'm mistaken here)

Sure, but I guess that should be a rare event that only happens when
you try to suspend and something interrupts the suspend.
 
> With the suspend block model we know the moment we're capable of
> suspending and then can suspend at that moment.  Continually trying to
> suspend seems like it'd be inefficient power-wise (we're going to be
> doing a lot more work as we try to suspend over and over, or we're
> going to retry after a timeout and spend extra time not suspended).
> 
> We can often spend minutes (possibly many) at a time preventing
> suspend when the system is doing work that would be interrupted by a
> full suspend.

Maybe you a userspace suspend policy manager would do the trick if
it knows when the screen is blanked and no audio has been played for
five seconds etc?

Regards,

Tony

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

[Arve Hjønnevåg]

On Wed, May 5, 2010 at 5:05 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Brian Swetland <swetland@google.com> [100505 16:51]:
>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Brian Swetland <swetland@google.com> [100505 14:34]:
>> >> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> >>
>> >> >> Oh, like tell the modem that user mode has handled the ring event and
>> >> >> its ok to un-block?
>> >> >
>> >> > No, that's not how it works.  It would go like this:
>> >> >
>> >> >        The modem IRQ handler queues its event to the input subsystem.
>> >> >        As it does so the input subsystem enables a suspend blocker,
>> >> >        causing the system to stay awake after the IRQ is done.
>> >
>> > How about instead the modem driver fails to suspend until it's done?
>> >
>> > Each driver could have a suspend_policy sysfs entry with options such
>> > as [ forced | safe ]. The default would be forced. Forced would
>> > be the current behaviour, while safe would refuse suspend until the
>> > driver is done processing.
>> >
>> >> >        The user program enables its own suspend blocker before reading
>> >> >        the input queue.  When the queue is empty, the input subsystem
>> >> >        releases its suspend blocker.
>> >
>> > And also the input layer could refuse to suspend until it's done.
>> >
>> >> >        When the user program finishes processing the event, it
>> >> >        releases its suspend blocker.  Now the system can go back to
>> >> >        sleep.
>> >
>> > And here the user space just tries to suspend again when it's done?
>> > It's not like you're trying to suspend all the time, so it should be
>> > OK to retry a few times.
>>
>> We actually are trying to suspend all the time -- that's our basic
>> model -- suspend whenever we can when something doesn't prevent it.
>
> Maybe that state could be kept in some userspace suspend policy manager?
>
>> >> > At no point does the user program have to communicate anything to the
>> >> > modem driver, and at no point does it have to do anything out of the
>> >> > ordinary except to enable and disable a suspend blocker.
>> >>
>> >> Exactly -- and you can use the same style of overlapping suspend
>> >> blockers with other drivers than input, if the input interface is not
>> >> suitable for the particular interaction.
>> >
>> > Would the suspend blockers still be needed somewhere in the example
>> > above?
>>
>> How often would we retry suspending?
>
> Well based on some timer, the same way the screen blanks? Or five
> seconds of no audio play? So if the suspend fails, then reset whatever
> userspace suspend policy timers.
>
>> If we fail to suspend, don't we have to resume all the drivers that
>> suspended before the one that failed?  (Maybe I'm mistaken here)
>
> Sure, but I guess that should be a rare event that only happens when
> you try to suspend and something interrupts the suspend.
>

This is not a rare event. For example, the matrix keypad driver blocks
suspend when a key is down so it can scan the matrix.

>> With the suspend block model we know the moment we're capable of
>> suspending and then can suspend at that moment.  Continually trying to
>> suspend seems like it'd be inefficient power-wise (we're going to be
>> doing a lot more work as we try to suspend over and over, or we're
>> going to retry after a timeout and spend extra time not suspended).
>>
>> We can often spend minutes (possibly many) at a time preventing
>> suspend when the system is doing work that would be interrupted by a
>> full suspend.
>
> Maybe you a userspace suspend policy manager would do the trick if
> it knows when the screen is blanked and no audio has been played for
> five seconds etc?
>

If user space has to initiate every suspend attempt, then you are
forcing it to poll whenever a driver needs to block suspend.

-- 
Arve Hjønnevåg

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

[Matthew Garrett]

On Wed, May 05, 2010 at 04:47:55PM -0700, Tony Lindgren wrote:

> How about instead the modem driver fails to suspend until it's done?

Because every attempted suspend requires freezing userspace, suspending 
devices until you hit one that refuses to suspend, resuming the devies 
that did suspend and then unfreezing userspace. That's not an attractive 
option.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100506 06:35]:
> On Wed, May 05, 2010 at 04:47:55PM -0700, Tony Lindgren wrote:
> 
> > How about instead the modem driver fails to suspend until it's done?
> 
> Because every attempted suspend requires freezing userspace, suspending 
> devices until you hit one that refuses to suspend, resuming the devies 
> that did suspend and then unfreezing userspace. That's not an attractive 
> option.

But how many times per day are you really suspending? Maybe few tens
of times at most if it's based on some user activity?

Or are you suspending constantly, tens of times per minute even if
there's no user activity?

Regards,

Tony

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

[Tony Lindgren]

* Arve Hjønnevåg <arve@android.com> [100505 21:11]:
> On Wed, May 5, 2010 at 5:05 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Brian Swetland <swetland@google.com> [100505 16:51]:
> >> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> > * Brian Swetland <swetland@google.com> [100505 14:34]:
> >> >> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> >> >> >>
> >> >> >> Oh, like tell the modem that user mode has handled the ring event and
> >> >> >> its ok to un-block?
> >> >> >
> >> >> > No, that's not how it works.  It would go like this:
> >> >> >
> >> >> >        The modem IRQ handler queues its event to the input subsystem.
> >> >> >        As it does so the input subsystem enables a suspend blocker,
> >> >> >        causing the system to stay awake after the IRQ is done.
> >> >
> >> > How about instead the modem driver fails to suspend until it's done?
> >> >
> >> > Each driver could have a suspend_policy sysfs entry with options such
> >> > as [ forced | safe ]. The default would be forced. Forced would
> >> > be the current behaviour, while safe would refuse suspend until the
> >> > driver is done processing.
> >> >
> >> >> >        The user program enables its own suspend blocker before reading
> >> >> >        the input queue.  When the queue is empty, the input subsystem
> >> >> >        releases its suspend blocker.
> >> >
> >> > And also the input layer could refuse to suspend until it's done.
> >> >
> >> >> >        When the user program finishes processing the event, it
> >> >> >        releases its suspend blocker.  Now the system can go back to
> >> >> >        sleep.
> >> >
> >> > And here the user space just tries to suspend again when it's done?
> >> > It's not like you're trying to suspend all the time, so it should be
> >> > OK to retry a few times.
> >>
> >> We actually are trying to suspend all the time -- that's our basic
> >> model -- suspend whenever we can when something doesn't prevent it.
> >
> > Maybe that state could be kept in some userspace suspend policy manager?
> >
> >> >> > At no point does the user program have to communicate anything to the
> >> >> > modem driver, and at no point does it have to do anything out of the
> >> >> > ordinary except to enable and disable a suspend blocker.
> >> >>
> >> >> Exactly -- and you can use the same style of overlapping suspend
> >> >> blockers with other drivers than input, if the input interface is not
> >> >> suitable for the particular interaction.
> >> >
> >> > Would the suspend blockers still be needed somewhere in the example
> >> > above?
> >>
> >> How often would we retry suspending?
> >
> > Well based on some timer, the same way the screen blanks? Or five
> > seconds of no audio play? So if the suspend fails, then reset whatever
> > userspace suspend policy timers.
> >
> >> If we fail to suspend, don't we have to resume all the drivers that
> >> suspended before the one that failed?  (Maybe I'm mistaken here)
> >
> > Sure, but I guess that should be a rare event that only happens when
> > you try to suspend and something interrupts the suspend.
> >
> 
> This is not a rare event. For example, the matrix keypad driver blocks
> suspend when a key is down so it can scan the matrix.

Sure, but how many times per day are you suspending?
 
> >> With the suspend block model we know the moment we're capable of
> >> suspending and then can suspend at that moment.  Continually trying to
> >> suspend seems like it'd be inefficient power-wise (we're going to be
> >> doing a lot more work as we try to suspend over and over, or we're
> >> going to retry after a timeout and spend extra time not suspended).
> >>
> >> We can often spend minutes (possibly many) at a time preventing
> >> suspend when the system is doing work that would be interrupted by a
> >> full suspend.
> >
> > Maybe you a userspace suspend policy manager would do the trick if
> > it knows when the screen is blanked and no audio has been played for
> > five seconds etc?
> >
> 
> If user space has to initiate every suspend attempt, then you are
> forcing it to poll whenever a driver needs to block suspend.

Hmm I don't follow you. If the userspace policy daemon timer times
out, the device suspends. If the device does not suspend because of
a blocking driver, then the timers get reset and you try again based
on some event such as when the screen blanks.

Regards,

Tony

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

[Matthew Garrett]

On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:

> Or are you suspending constantly, tens of times per minute even if
> there's no user activity?

In this case you'd be repeatedly trying to suspend until the modem 
driver stopped blocking it. It's pretty much a waste.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> 
> > Or are you suspending constantly, tens of times per minute even if
> > there's no user activity?
> 
> In this case you'd be repeatedly trying to suspend until the modem 
> driver stopped blocking it. It's pretty much a waste.

But then the userspace knows you're getting data from the modem, and
it can kick some inactivity timer that determines when to try to
suspend next.

Why would you need to constantly try to suspend in that case?

Regards,

Tony

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

[Matthew Garrett]

On Thu, May 06, 2010 at 10:14:53AM -0700, Tony Lindgren wrote:

> Why would you need to constantly try to suspend in that case?

Because otherwise you're awake for longer than you need to be.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Daniel Walker]

On Thu, 2010-05-06 at 10:14 -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> > On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> > 
> > > Or are you suspending constantly, tens of times per minute even if
> > > there's no user activity?
> > 
> > In this case you'd be repeatedly trying to suspend until the modem 
> > driver stopped blocking it. It's pretty much a waste.
> 
> But then the userspace knows you're getting data from the modem, and
> it can kick some inactivity timer that determines when to try to
> suspend next.

If the idle thread was doing the suspending then the inactivity timer by
it's self could block suspend. As long as the idle thread was setup to
check for timers. I'm sure that _isn't_ the point your trying to make.
It just makes gobs more sense to me that the idle thread does the
suspending .. Your idle, so depending on how long your idle then you
suspend.

Daniel

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100506 10:17]:
> On Thu, May 06, 2010 at 10:14:53AM -0700, Tony Lindgren wrote:
> 
> > Why would you need to constantly try to suspend in that case?
> 
> Because otherwise you're awake for longer than you need to be.

If your system is idle and your hardware supports off-while-idle,
then that really does not matter. There's not much of a difference
in power savings, we're already talking over 10 days on batteries
with just off-while-idle on omaps.

If your userspace keeps polling and has runaway timers, then you
could suspend it's parent process to idle the system?

Regards,

Tony

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

[Matthew Garrett]

On Thu, May 06, 2010 at 10:38:08AM -0700, Tony Lindgren wrote:

> If your userspace keeps polling and has runaway timers, then you
> could suspend it's parent process to idle the system?

If your userspace is suspended, how does it process the events that 
generated a system wakeup? If we had a good answer to that then suspend 
blockers would be much less necessary.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100506 10:39]:
> On Thu, May 06, 2010 at 10:38:08AM -0700, Tony Lindgren wrote:
> 
> > If your userspace keeps polling and has runaway timers, then you
> > could suspend it's parent process to idle the system?
> 
> If your userspace is suspended, how does it process the events that 
> generated a system wakeup? If we had a good answer to that then suspend 
> blockers would be much less necessary.

Well if your hardware runs off-while-idle or even just
retention-while-idle, then the basic shell works just fine waking up
every few seconds or so.

Then you could keep init/shell/suspend policy deamon running until
it's time to suspend the whole device. To cut down runaway timers,
you could already freeze the desktop/GUI/whatever earlier.

Regards,

Tony

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

[Tony Lindgren]

* Daniel Walker <dwalker@fifo99.com> [100506 10:30]:
> On Thu, 2010-05-06 at 10:14 -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> > > On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> > > 
> > > > Or are you suspending constantly, tens of times per minute even if
> > > > there's no user activity?
> > > 
> > > In this case you'd be repeatedly trying to suspend until the modem 
> > > driver stopped blocking it. It's pretty much a waste.
> > 
> > But then the userspace knows you're getting data from the modem, and
> > it can kick some inactivity timer that determines when to try to
> > suspend next.
> 
> If the idle thread was doing the suspending then the inactivity timer by
> it's self could block suspend. As long as the idle thread was setup to
> check for timers. I'm sure that _isn't_ the point your trying to make.
> It just makes gobs more sense to me that the idle thread does the
> suspending .. Your idle, so depending on how long your idle then you
> suspend.

The alternative logic I'm suggesting is get the GUI into idle mode as
soon as possible, then limp along with off-while-idle or
retention-while-idle until some timer expires, then suspend the whole
device.

Tony

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

[Matthew Garrett]

On Thu, May 06, 2010 at 11:33:35AM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100506 10:39]:
> > On Thu, May 06, 2010 at 10:38:08AM -0700, Tony Lindgren wrote:
> > 
> > > If your userspace keeps polling and has runaway timers, then you
> > > could suspend it's parent process to idle the system?
> > 
> > If your userspace is suspended, how does it process the events that 
> > generated a system wakeup? If we had a good answer to that then suspend 
> > blockers would be much less necessary.
> 
> Well if your hardware runs off-while-idle or even just
> retention-while-idle, then the basic shell works just fine waking up
> every few seconds or so.

And the untrusted userspace code that's waiting for a network packet? 
Adding a few seconds of latency isn't an option here.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Alan Stern]

On Thu, 6 May 2010, Tony Lindgren wrote:

> Well if your hardware runs off-while-idle or even just
> retention-while-idle, then the basic shell works just fine waking up
> every few seconds or so.
> 
> Then you could keep init/shell/suspend policy deamon running until
> it's time to suspend the whole device. To cut down runaway timers,
> you could already freeze the desktop/GUI/whatever earlier.

This comes down mostly to efficiency.  Although the suspend blocker
patch does the actual suspending in a workqueue thread, AFAIK there's
no reason it couldn't use a user thread instead.

The important difference lies in what happens when a suspend fails
because a driver is busy.  Without suspend blockers, the kernel has to
go through the whole procedure of freezing userspace and kernel threads
and then suspending a bunch of drivers before hitting the one that's
busy.  Then all that work has to be undone.  By contrast, with suspend
blockers the suspend attempt can fail right away with minimal overhead.

There's also a question of reliability.  With suspends controlled by 
userspace there is a possibility of races, which could lead the system 
to suspend when it shouldn't.  With control in the kernel, these races 
can be eliminated.

Alan Stern

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

[Daniel Walker]

On Thu, 2010-05-06 at 11:36 -0700, Tony Lindgren wrote:
> * Daniel Walker <dwalker@fifo99.com> [100506 10:30]:
> > On Thu, 2010-05-06 at 10:14 -0700, Tony Lindgren wrote:
> > > * Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> > > > On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> > > > 
> > > > > Or are you suspending constantly, tens of times per minute even if
> > > > > there's no user activity?
> > > > 
> > > > In this case you'd be repeatedly trying to suspend until the modem 
> > > > driver stopped blocking it. It's pretty much a waste.
> > > 
> > > But then the userspace knows you're getting data from the modem, and
> > > it can kick some inactivity timer that determines when to try to
> > > suspend next.
> > 
> > If the idle thread was doing the suspending then the inactivity timer by
> > it's self could block suspend. As long as the idle thread was setup to
> > check for timers. I'm sure that _isn't_ the point your trying to make.
> > It just makes gobs more sense to me that the idle thread does the
> > suspending .. Your idle, so depending on how long your idle then you
> > suspend.
> 
> The alternative logic I'm suggesting is get the GUI into idle mode as
> soon as possible, then limp along with off-while-idle or
> retention-while-idle until some timer expires, then suspend the whole
> device.

Could you elaborate on "off-while-idle" and "retention-while-idle" ? I'm
not sure I follow what you mean.

Daniel

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

[Arve Hjønnevåg]

2010/5/6 Tony Lindgren <tony@atomide.com>:
> * Arve Hjønnevåg <arve@android.com> [100505 21:11]:
>> On Wed, May 5, 2010 at 5:05 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Brian Swetland <swetland@google.com> [100505 16:51]:
>> >> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>> >> > * Brian Swetland <swetland@google.com> [100505 14:34]:
>> >> >> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> >> >> >>
>> >> >> >> Oh, like tell the modem that user mode has handled the ring event and
>> >> >> >> its ok to un-block?
>> >> >> >
>> >> >> > No, that's not how it works.  It would go like this:
>> >> >> >
>> >> >> >        The modem IRQ handler queues its event to the input subsystem.
>> >> >> >        As it does so the input subsystem enables a suspend blocker,
>> >> >> >        causing the system to stay awake after the IRQ is done.
>> >> >
>> >> > How about instead the modem driver fails to suspend until it's done?
>> >> >
>> >> > Each driver could have a suspend_policy sysfs entry with options such
>> >> > as [ forced | safe ]. The default would be forced. Forced would
>> >> > be the current behaviour, while safe would refuse suspend until the
>> >> > driver is done processing.
>> >> >
>> >> >> >        The user program enables its own suspend blocker before reading
>> >> >> >        the input queue.  When the queue is empty, the input subsystem
>> >> >> >        releases its suspend blocker.
>> >> >
>> >> > And also the input layer could refuse to suspend until it's done.
>> >> >
>> >> >> >        When the user program finishes processing the event, it
>> >> >> >        releases its suspend blocker.  Now the system can go back to
>> >> >> >        sleep.
>> >> >
>> >> > And here the user space just tries to suspend again when it's done?
>> >> > It's not like you're trying to suspend all the time, so it should be
>> >> > OK to retry a few times.
>> >>
>> >> We actually are trying to suspend all the time -- that's our basic
>> >> model -- suspend whenever we can when something doesn't prevent it.
>> >
>> > Maybe that state could be kept in some userspace suspend policy manager?
>> >
>> >> >> > At no point does the user program have to communicate anything to the
>> >> >> > modem driver, and at no point does it have to do anything out of the
>> >> >> > ordinary except to enable and disable a suspend blocker.
>> >> >>
>> >> >> Exactly -- and you can use the same style of overlapping suspend
>> >> >> blockers with other drivers than input, if the input interface is not
>> >> >> suitable for the particular interaction.
>> >> >
>> >> > Would the suspend blockers still be needed somewhere in the example
>> >> > above?
>> >>
>> >> How often would we retry suspending?
>> >
>> > Well based on some timer, the same way the screen blanks? Or five
>> > seconds of no audio play? So if the suspend fails, then reset whatever
>> > userspace suspend policy timers.
>> >
>> >> If we fail to suspend, don't we have to resume all the drivers that
>> >> suspended before the one that failed?  (Maybe I'm mistaken here)
>> >
>> > Sure, but I guess that should be a rare event that only happens when
>> > you try to suspend and something interrupts the suspend.
>> >
>>
>> This is not a rare event. For example, the matrix keypad driver blocks
>> suspend when a key is down so it can scan the matrix.
>
> Sure, but how many times per day are you suspending?
>

How many times we successfully suspend is irrelevant here. If the
driver blocks suspend the number of suspend attempts depend on your
poll frequency.

>> >> With the suspend block model we know the moment we're capable of
>> >> suspending and then can suspend at that moment.  Continually trying to
>> >> suspend seems like it'd be inefficient power-wise (we're going to be
>> >> doing a lot more work as we try to suspend over and over, or we're
>> >> going to retry after a timeout and spend extra time not suspended).
>> >>
>> >> We can often spend minutes (possibly many) at a time preventing
>> >> suspend when the system is doing work that would be interrupted by a
>> >> full suspend.
>> >
>> > Maybe you a userspace suspend policy manager would do the trick if
>> > it knows when the screen is blanked and no audio has been played for
>> > five seconds etc?
>> >
>>
>> If user space has to initiate every suspend attempt, then you are
>> forcing it to poll whenever a driver needs to block suspend.
>
> Hmm I don't follow you. If the userspace policy daemon timer times
> out, the device suspends. If the device does not suspend because of
> a blocking driver, then the timers get reset and you try again based
> on some event such as when the screen blanks.
>

This retry is what I call polling. You have to keep retrying until you
succeed. Also, using the screen blank timeout for this polling is not
a good idea. You do not want to toggle the screen off and on with with
every suspend attempt.

-- 
Arve Hjønnevåg

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

[Tony Lindgren]

* Daniel Walker <dwalker@fifo99.com> [100506 12:07]:
> On Thu, 2010-05-06 at 11:36 -0700, Tony Lindgren wrote:
> > * Daniel Walker <dwalker@fifo99.com> [100506 10:30]:
> > > On Thu, 2010-05-06 at 10:14 -0700, Tony Lindgren wrote:
> > > > * Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> > > > > On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> > > > > 
> > > > > > Or are you suspending constantly, tens of times per minute even if
> > > > > > there's no user activity?
> > > > > 
> > > > > In this case you'd be repeatedly trying to suspend until the modem 
> > > > > driver stopped blocking it. It's pretty much a waste.
> > > > 
> > > > But then the userspace knows you're getting data from the modem, and
> > > > it can kick some inactivity timer that determines when to try to
> > > > suspend next.
> > > 
> > > If the idle thread was doing the suspending then the inactivity timer by
> > > it's self could block suspend. As long as the idle thread was setup to
> > > check for timers. I'm sure that _isn't_ the point your trying to make.
> > > It just makes gobs more sense to me that the idle thread does the
> > > suspending .. Your idle, so depending on how long your idle then you
> > > suspend.
> > 
> > The alternative logic I'm suggesting is get the GUI into idle mode as
> > soon as possible, then limp along with off-while-idle or
> > retention-while-idle until some timer expires, then suspend the whole
> > device.
> 
> Could you elaborate on "off-while-idle" and "retention-while-idle" ? I'm
> not sure I follow what you mean.

Oh some SoC devices like omap hit retention or off modes in the idle loop.
That brings down the idle consumption of a running system to minimal
levels. It's basically the same as suspending the device in every idle loop.

The system wakes up every few seconds or so, but that already provides
battery life of over ten days or so on an idle system. Of course the
wakeup latencies are in milliseconds then.

Regards,

Tony

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100506 11:39]:
> On Thu, May 06, 2010 at 11:33:35AM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100506 10:39]:
> > > On Thu, May 06, 2010 at 10:38:08AM -0700, Tony Lindgren wrote:
> > > 
> > > > If your userspace keeps polling and has runaway timers, then you
> > > > could suspend it's parent process to idle the system?
> > > 
> > > If your userspace is suspended, how does it process the events that 
> > > generated a system wakeup? If we had a good answer to that then suspend 
> > > blockers would be much less necessary.
> > 
> > Well if your hardware runs off-while-idle or even just
> > retention-while-idle, then the basic shell works just fine waking up
> > every few seconds or so.
> 
> And the untrusted userspace code that's waiting for a network packet? 
> Adding a few seconds of latency isn't an option here.

Hmm well hitting retention and wake you can basically do between
jiffies. Hitting off mode in idle has way longer latencies,
but still in few hundred milliseconds or so, not seconds.

And cpuidle pretty much takes care of hitting the desired C state
for you. This setup is totally working on Nokia N900 for example,
it's hitting off modes in idle and running all the time, it never
suspends.

Regards,

Tony

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

[Tony Lindgren]

* Alan Stern <stern@rowland.harvard.edu> [100506 11:42]:
> On Thu, 6 May 2010, Tony Lindgren wrote:
> 
> > Well if your hardware runs off-while-idle or even just
> > retention-while-idle, then the basic shell works just fine waking up
> > every few seconds or so.
> > 
> > Then you could keep init/shell/suspend policy deamon running until
> > it's time to suspend the whole device. To cut down runaway timers,
> > you could already freeze the desktop/GUI/whatever earlier.
> 
> This comes down mostly to efficiency.  Although the suspend blocker
> patch does the actual suspending in a workqueue thread, AFAIK there's
> no reason it couldn't use a user thread instead.
> 
> The important difference lies in what happens when a suspend fails
> because a driver is busy.  Without suspend blockers, the kernel has to
> go through the whole procedure of freezing userspace and kernel threads
> and then suspending a bunch of drivers before hitting the one that's
> busy.  Then all that work has to be undone.  By contrast, with suspend
> blockers the suspend attempt can fail right away with minimal overhead.

But does that really matter if you're only few tens of times times per
day or so? I don't understand why you would want to try to suspend except
after some timeout of no user or network activity.
 
> There's also a question of reliability.  With suspends controlled by 
> userspace there is a possibility of races, which could lead the system 
> to suspend when it shouldn't.  With control in the kernel, these races 
> can be eliminated.

I agree the suspend needs to happen without races. But I think the
logic for when to suspend should be done in the userspace as it
can be device or user specific.

Regards,

Tony

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

[James Kosin]

On 5/5/2010 8:10 PM, Tony Lindgren wrote:
> * Brian Swetland <swetland@google.com> [100505 16:51]:
>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>>> * Brian Swetland <swetland@google.com> [100505 14:34]:
>>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
<<-- snip -->>
>>>>> At no point does the user program have to communicate anything to the
>>>>> modem driver, and at no point does it have to do anything out of the
>>>>> ordinary except to enable and disable a suspend blocker.
>>>>
>>>> Exactly -- and you can use the same style of overlapping suspend
>>>> blockers with other drivers than input, if the input interface is not
>>>> suitable for the particular interaction.
>>>
>>> Would the suspend blockers still be needed somewhere in the example
>>> above?
>>
>> How often would we retry suspending?
> 
> Well based on some timer, the same way the screen blanks? Or five
> seconds of no audio play? So if the suspend fails, then reset whatever
> userspace suspend policy timers.
>  

Tony,
Wouldn't this be handled by the idle task, or task manager?

When all tasks are suspended and not doing anything that should be the
first clue that a real suspend cycle could be attempted.

James

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

[Arve Hjønnevåg]

On Thu, May 6, 2010 at 7:41 PM, James Kosin <james.kosin.04@cnu.edu> wrote:
> On 5/5/2010 8:10 PM, Tony Lindgren wrote:
>> * Brian Swetland <swetland@google.com> [100505 16:51]:
>>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>> * Brian Swetland <swetland@google.com> [100505 14:34]:
>>>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> <<-- snip -->>
>>>>>> At no point does the user program have to communicate anything to the
>>>>>> modem driver, and at no point does it have to do anything out of the
>>>>>> ordinary except to enable and disable a suspend blocker.
>>>>>
>>>>> Exactly -- and you can use the same style of overlapping suspend
>>>>> blockers with other drivers than input, if the input interface is not
>>>>> suitable for the particular interaction.
>>>>
>>>> Would the suspend blockers still be needed somewhere in the example
>>>> above?
>>>
>>> How often would we retry suspending?
>>
>> Well based on some timer, the same way the screen blanks? Or five
>> seconds of no audio play? So if the suspend fails, then reset whatever
>> userspace suspend policy timers.
>>
>
> Tony,
> Wouldn't this be handled by the idle task, or task manager?
>
> When all tasks are suspended and not doing anything that should be the
> first clue that a real suspend cycle could be attempted.
>

One if the benefit we get from using suspend is that an unprivileged
app that does not have access to suspend blockers cannot prevent
suspend. You lose this advantage if you trigger suspend only from the
idle task.

-- 
Arve Hjønnevåg

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

[James Kosin]

On 5/6/2010 10:53 PM, Arve Hjønnevåg wrote:
> On Thu, May 6, 2010 at 7:41 PM, James Kosin <james.kosin.04@cnu.edu> wrote:
>   
>> On 5/5/2010 8:10 PM, Tony Lindgren wrote:
>>     
>>> * Brian Swetland <swetland@google.com> [100505 16:51]:
>>>       
>>>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>>         
>>>>> * Brian Swetland <swetland@google.com> [100505 14:34]:
>>>>>           
>>>>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>>>>             
>> <<-- snip -->>
>>     
>>>>>>> At no point does the user program have to communicate anything to the
>>>>>>> modem driver, and at no point does it have to do anything out of the
>>>>>>> ordinary except to enable and disable a suspend blocker.
>>>>>>>               
>>>>>> Exactly -- and you can use the same style of overlapping suspend
>>>>>> blockers with other drivers than input, if the input interface is not
>>>>>> suitable for the particular interaction.
>>>>>>             
>>>>> Would the suspend blockers still be needed somewhere in the example
>>>>> above?
>>>>>           
>>>> How often would we retry suspending?
>>>>         
>>> Well based on some timer, the same way the screen blanks? Or five
>>> seconds of no audio play? So if the suspend fails, then reset whatever
>>> userspace suspend policy timers.
>>>
>>>       
>> Tony,
>> Wouldn't this be handled by the idle task, or task manager?
>>
>> When all tasks are suspended and not doing anything that should be the
>> first clue that a real suspend cycle could be attempted.
>>
>>     
> One if the benefit we get from using suspend is that an unprivileged
> app that does not have access to suspend blockers cannot prevent
> suspend. You lose this advantage if you trigger suspend only from the
> idle task.
>
>   
If the process (privileged or unprivileged) doesn't want to suspend, why
not just provide an interface to allow suspend to be turned off at the
user level.  This could block the suspend cycle in itself, and you
shouldn't need fine grained off/on cycles.  If an application really
needs the system not to suspend then they (the user) should know the
consequences and power requirements for such a task.

I didn't say it had to be only from the idle task; but, that is the most
logical place.  If the other threads are not idle then they really
require work and will most likely already have a bock on the suspend anyway.

James

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

[Arve Hjønnevåg]

2010/5/6 James Kosin <james.kosin.04@cnu.edu>:
> On 5/6/2010 10:53 PM, Arve Hjønnevåg wrote:
>> On Thu, May 6, 2010 at 7:41 PM, James Kosin <james.kosin.04@cnu.edu> wrote:
>>
>>> On 5/5/2010 8:10 PM, Tony Lindgren wrote:
>>>
>>>> * Brian Swetland <swetland@google.com> [100505 16:51]:
>>>>
>>>>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>>>
>>>>>> * Brian Swetland <swetland@google.com> [100505 14:34]:
>>>>>>
>>>>>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>>>>>
>>> <<-- snip -->>
>>>
>>>>>>>> At no point does the user program have to communicate anything to the
>>>>>>>> modem driver, and at no point does it have to do anything out of the
>>>>>>>> ordinary except to enable and disable a suspend blocker.
>>>>>>>>
>>>>>>> Exactly -- and you can use the same style of overlapping suspend
>>>>>>> blockers with other drivers than input, if the input interface is not
>>>>>>> suitable for the particular interaction.
>>>>>>>
>>>>>> Would the suspend blockers still be needed somewhere in the example
>>>>>> above?
>>>>>>
>>>>> How often would we retry suspending?
>>>>>
>>>> Well based on some timer, the same way the screen blanks? Or five
>>>> seconds of no audio play? So if the suspend fails, then reset whatever
>>>> userspace suspend policy timers.
>>>>
>>>>
>>> Tony,
>>> Wouldn't this be handled by the idle task, or task manager?
>>>
>>> When all tasks are suspended and not doing anything that should be the
>>> first clue that a real suspend cycle could be attempted.
>>>
>>>
>> One if the benefit we get from using suspend is that an unprivileged
>> app that does not have access to suspend blockers cannot prevent
>> suspend. You lose this advantage if you trigger suspend only from the
>> idle task.
>>
>>
> If the process (privileged or unprivileged) doesn't want to suspend, why
> not just provide an interface to allow suspend to be turned off at the
> user level.  This could block the suspend cycle in itself, and you
> shouldn't need fine grained off/on cycles.  If an application really
> needs the system not to suspend then they (the user) should know the
> consequences and power requirements for such a task.
>
> I didn't say it had to be only from the idle task; but, that is the most
> logical place.  If the other threads are not idle then they really
> require work and will most likely already have a bock on the suspend anyway.
>

I think you missed my point. Unprivileged processes should not be
allowed to prevent suspend.

-- 
Arve Hjønnevåg

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

[James Kosin]

On 5/6/2010 11:10 PM, Arve Hjønnevåg wrote:
> 2010/5/6 James Kosin <james.kosin.04@cnu.edu>:
>   
>> On 5/6/2010 10:53 PM, Arve Hjønnevåg wrote:
>>     
>>> On Thu, May 6, 2010 at 7:41 PM, James Kosin <james.kosin.04@cnu.edu> wrote:
>>>
>>>       
>>>> On 5/5/2010 8:10 PM, Tony Lindgren wrote:
>>>>
>>>>         
>>>>> * Brian Swetland <swetland@google.com> [100505 16:51]:
>>>>>
>>>>>           
>>>>>> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
>>>>>>
>>>>>>             
>>>>>>> * Brian Swetland <swetland@google.com> [100505 14:34]:
>>>>>>>
>>>>>>>               
>>>>>>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>>>>>>>>
>>>>>>>>                 
>>>> <<-- snip -->>
>>>>
>>>>         
>>>>>>>>> At no point does the user program have to communicate anything to the
>>>>>>>>> modem driver, and at no point does it have to do anything out of the
>>>>>>>>> ordinary except to enable and disable a suspend blocker.
>>>>>>>>>
>>>>>>>>>                   
>>>>>>>> Exactly -- and you can use the same style of overlapping suspend
>>>>>>>> blockers with other drivers than input, if the input interface is not
>>>>>>>> suitable for the particular interaction.
>>>>>>>>
>>>>>>>>                 
>>>>>>> Would the suspend blockers still be needed somewhere in the example
>>>>>>> above?
>>>>>>>
>>>>>>>               
>>>>>> How often would we retry suspending?
>>>>>>
>>>>>>             
>>>>> Well based on some timer, the same way the screen blanks? Or five
>>>>> seconds of no audio play? So if the suspend fails, then reset whatever
>>>>> userspace suspend policy timers.
>>>>>
>>>>>
>>>>>           
>>>> Tony,
>>>> Wouldn't this be handled by the idle task, or task manager?
>>>>
>>>> When all tasks are suspended and not doing anything that should be the
>>>> first clue that a real suspend cycle could be attempted.
>>>>
>>>>
>>>>         
>>> One if the benefit we get from using suspend is that an unprivileged
>>> app that does not have access to suspend blockers cannot prevent
>>> suspend. You lose this advantage if you trigger suspend only from the
>>> idle task.
>>>
>>>
>>>       
>> If the process (privileged or unprivileged) doesn't want to suspend, why
>> not just provide an interface to allow suspend to be turned off at the
>> user level.  This could block the suspend cycle in itself, and you
>> shouldn't need fine grained off/on cycles.  If an application really
>> needs the system not to suspend then they (the user) should know the
>> consequences and power requirements for such a task.
>>
>> I didn't say it had to be only from the idle task; but, that is the most
>> logical place.  If the other threads are not idle then they really
>> require work and will most likely already have a bock on the suspend anyway.
>>
>>     
> I think you missed my point. Unprivileged processes should not be
> allowed to prevent suspend.
>
>   
Ah, you want a way for the system to suspend (and enforce the suspend)
when only unprivileged processes are the only thing running....

That would mean a lot of work defining the unprivileged (or privileged)
processes, and properly suspending (or enforcing) when needed.  Yuck. 
Sorry I commented then, this is really getting deep into what I love to
do at work.
 

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

[mgross]

On Thu, May 06, 2010 at 06:09:56PM +0100, Matthew Garrett wrote:
> On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> 
> > Or are you suspending constantly, tens of times per minute even if
> > there's no user activity?
> 
> In this case you'd be repeatedly trying to suspend until the modem 
> driver stopped blocking it. It's pretty much a waste.


lets not go off in the weeds for the wrong things now.  The answer to
the retry is at most one time.  The first time would be blocked, then
the suspend enable would need to re-enabled under user mode control that
would, buy that time, know it has to ack to the modem to stop rejecting
the suspend attempt.

duh.

--mgross

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

[mgross]

On Thu, May 06, 2010 at 10:14:53AM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100506 10:05]:
> > On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
> > 
> > > Or are you suspending constantly, tens of times per minute even if
> > > there's no user activity?
> > 
> > In this case you'd be repeatedly trying to suspend until the modem 
> > driver stopped blocking it. It's pretty much a waste.
> 
> But then the userspace knows you're getting data from the modem, and
> it can kick some inactivity timer that determines when to try to
> suspend next.

Thank you for thinking.

--mgross

> 
> Why would you need to constantly try to suspend in that case?
> 
> Regards,
> 
> Tony

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

[Arve Hjønnevåg]

On Thu, May 6, 2010 at 8:45 PM, mgross <markgross@thegnar.org> wrote:
> On Thu, May 06, 2010 at 06:09:56PM +0100, Matthew Garrett wrote:
>> On Thu, May 06, 2010 at 10:01:51AM -0700, Tony Lindgren wrote:
>>
>> > Or are you suspending constantly, tens of times per minute even if
>> > there's no user activity?
>>
>> In this case you'd be repeatedly trying to suspend until the modem
>> driver stopped blocking it. It's pretty much a waste.
>
>
> lets not go off in the weeds for the wrong things now.  The answer to
> the retry is at most one time.  The first time would be blocked, then
> the suspend enable would need to re-enabled under user mode control that
> would, buy that time, know it has to ack to the modem to stop rejecting
> the suspend attempt.
>

This is incorrect in the general case. User-space has no way of
knowing which driver blocked suspend or when it will stop blocking
suspend.

-- 
Arve Hjønnevåg

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

[Tony Lindgren]

* Arve Hjønnevåg <arve@android.com> [100506 17:05]:
> 2010/5/6 Tony Lindgren <tony@atomide.com>:
> > * Arve Hjønnevåg <arve@android.com> [100505 21:11]:

<snip>

> >> This is not a rare event. For example, the matrix keypad driver blocks
> >> suspend when a key is down so it can scan the matrix.
> >
> > Sure, but how many times per day are you suspending?
> >
> 
> How many times we successfully suspend is irrelevant here. If the
> driver blocks suspend the number of suspend attempts depend on your
> poll frequency.

I guess what I'm trying to say is that a failed suspend should be
a rare event.
 
> >> >> With the suspend block model we know the moment we're capable of
> >> >> suspending and then can suspend at that moment.  Continually trying to
> >> >> suspend seems like it'd be inefficient power-wise (we're going to be
> >> >> doing a lot more work as we try to suspend over and over, or we're
> >> >> going to retry after a timeout and spend extra time not suspended).
> >> >>
> >> >> We can often spend minutes (possibly many) at a time preventing
> >> >> suspend when the system is doing work that would be interrupted by a
> >> >> full suspend.
> >> >
> >> > Maybe you a userspace suspend policy manager would do the trick if
> >> > it knows when the screen is blanked and no audio has been played for
> >> > five seconds etc?
> >> >
> >>
> >> If user space has to initiate every suspend attempt, then you are
> >> forcing it to poll whenever a driver needs to block suspend.
> >
> > Hmm I don't follow you. If the userspace policy daemon timer times
> > out, the device suspends. If the device does not suspend because of
> > a blocking driver, then the timers get reset and you try again based
> > on some event such as when the screen blanks.
> >
> 
> This retry is what I call polling. You have to keep retrying until you
> succeed. Also, using the screen blank timeout for this polling is not
> a good idea. You do not want to toggle the screen off and on with with
> every suspend attempt.

The number of retries depends on the power management policy for your
device. And that is often device and use specific.

So having to retry suspend should be a rare event. The userspace should
mostly know when it's OK to suspend.

Regards,

Tony

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

[Tony Lindgren]

* James Kosin <james.kosin.04@cnu.edu> [100506 19:38]:
> On 5/5/2010 8:10 PM, Tony Lindgren wrote:
> > * Brian Swetland <swetland@google.com> [100505 16:51]:
> >> On Wed, May 5, 2010 at 4:47 PM, Tony Lindgren <tony@atomide.com> wrote:
> >>> * Brian Swetland <swetland@google.com> [100505 14:34]:
> >>>> On Wed, May 5, 2010 at 2:12 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> <<-- snip -->>
> >>>>> At no point does the user program have to communicate anything to the
> >>>>> modem driver, and at no point does it have to do anything out of the
> >>>>> ordinary except to enable and disable a suspend blocker.
> >>>>
> >>>> Exactly -- and you can use the same style of overlapping suspend
> >>>> blockers with other drivers than input, if the input interface is not
> >>>> suitable for the particular interaction.
> >>>
> >>> Would the suspend blockers still be needed somewhere in the example
> >>> above?
> >>
> >> How often would we retry suspending?
> > 
> > Well based on some timer, the same way the screen blanks? Or five
> > seconds of no audio play? So if the suspend fails, then reset whatever
> > userspace suspend policy timers.
> >  
> 
> Tony,
> Wouldn't this be handled by the idle task, or task manager?

My thinking is that suspend should be only triggered from the
userspace based on a device specic policy. Suspend breaks
standard Linux behaviour as all the timers will stop running.

You can already implement suspend-like idle states for various
processors that are controlled by cpuidle.

In those cases the device hits suspend or off modes in the kernel
idle loop. In this case the system keeps running waking every
few seconds or so when idle, and the timers behave the normal way.

Of course there can be tens or even few hundred millisecond
wake-up latencies in the case of hitting off and restoring the
kernel during idle, so interrupt response time in those cases
is longer.

> When all tasks are suspended and not doing anything that should be the
> first clue that a real suspend cycle could be attempted.

Yeah, but I don't think there's a generic way of figuring
out when it's OK to suspend. It depends on the device. It
can also depend on how the user wants to configure things.

Regards,

Tony

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

[Tony Lindgren]

* James Kosin <james.kosin.04@cnu.edu> [100506 20:14]:
> On 5/6/2010 11:10 PM, Arve Hjønnevåg wrote:
> >>>>         
> >>> One if the benefit we get from using suspend is that an unprivileged
> >>> app that does not have access to suspend blockers cannot prevent
> >>> suspend. You lose this advantage if you trigger suspend only from the
> >>> idle task.

This assumes that you're using cached values for echo mem > /sys/power/state
and the system keeps on running.

IMHO if you want to keep the system running, then you should just use
cpuidle and implement good idle modes.

Then when someting in the userspace knows you've been idle long enough,
you can suspend.

> >> If the process (privileged or unprivileged) doesn't want to suspend, why
> >> not just provide an interface to allow suspend to be turned off at the
> >> user level.  This could block the suspend cycle in itself, and you
> >> shouldn't need fine grained off/on cycles.  If an application really
> >> needs the system not to suspend then they (the user) should know the
> >> consequences and power requirements for such a task.
> >>
> >> I didn't say it had to be only from the idle task; but, that is the most
> >> logical place.  If the other threads are not idle then they really
> >> require work and will most likely already have a bock on the suspend anyway.
> >>
> >>     
> > I think you missed my point. Unprivileged processes should not be
> > allowed to prevent suspend.

You could just freeze the GUI process based on some policy if you
worry about misbehaving timers in various apps. This way the cpuidle
modes will allow you to run some userspace policy daemon. And then that
can suspend if needed based on how it's configured.
  
> Ah, you want a way for the system to suspend (and enforce the suspend)
> when only unprivileged processes are the only thing running....
> 
> That would mean a lot of work defining the unprivileged (or privileged)
> processes, and properly suspending (or enforcing) when needed.  Yuck. 
> Sorry I commented then, this is really getting deep into what I love to
> do at work.

Hmm, yeah sounds a bit messy.

Tony

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

[Matthew Garrett]

On Thu, May 06, 2010 at 07:05:41PM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100506 11:39]:
> > And the untrusted userspace code that's waiting for a network packet? 
> > Adding a few seconds of latency isn't an option here.
> 
> Hmm well hitting retention and wake you can basically do between
> jiffies. Hitting off mode in idle has way longer latencies,
> but still in few hundred milliseconds or so, not seconds.

The situation is this. You've frozen most of your userspace because you 
don't trust the applications. One of those applications has an open 
network socket, and policy indicates that receiving a network packet 
should generate a wakeup, allow the userspace application to handle the 
packet and then return to sleep. What mechanism do you use to do that?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Daniel Walker]

On Thu, 2010-05-06 at 19:00 -0700, Tony Lindgren wrote:

> Oh some SoC devices like omap hit retention or off modes in the idle loop.
> That brings down the idle consumption of a running system to minimal
> levels. It's basically the same as suspending the device in every idle loop.
> 
> The system wakes up every few seconds or so, but that already provides
> battery life of over ten days or so on an idle system. Of course the
> wakeup latencies are in milliseconds then.

MSM doesn't have those power states unfortunately .. Your kind of
suggesting what I was suggesting in that we should suspend in idle. Your
hardware can do it easier tho since your have power states that are
equal to suspend.

Daniel

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 10:08]:
> On Thu, May 06, 2010 at 07:05:41PM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100506 11:39]:
> > > And the untrusted userspace code that's waiting for a network packet? 
> > > Adding a few seconds of latency isn't an option here.
> > 
> > Hmm well hitting retention and wake you can basically do between
> > jiffies. Hitting off mode in idle has way longer latencies,
> > but still in few hundred milliseconds or so, not seconds.
> 
> The situation is this. You've frozen most of your userspace because you 
> don't trust the applications. One of those applications has an open 
> network socket, and policy indicates that receiving a network packet 
> should generate a wakeup, allow the userspace application to handle the 
> packet and then return to sleep. What mechanism do you use to do that?

I think the ideal way of doing this would be to have the system running
and hitting some deeper idle states using cpuidle. Then fix the apps
so timers don't wake up the system too often. Then everything would
just run in a normal way.

For the misbehaving stopped apps, maybe they could be woken
to deal with the incoming network data with sysfs_notify?

Regards,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 10:20:37AM -0700, Daniel Walker wrote:

> MSM doesn't have those power states unfortunately .. Your kind of
> suggesting what I was suggesting in that we should suspend in idle. Your
> hardware can do it easier tho since your have power states that are
> equal to suspend.

If your wakeup latencies are sufficiently low and you have fine-grained 
enough control over your hardware then suspend in idle is a reasonable 
thing to do - but if you have a userspace app that's spinning then 
that doesn't solve the issue.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Daniel Walker]

On Fri, 2010-05-07 at 18:36 +0100, Matthew Garrett wrote:
> On Fri, May 07, 2010 at 10:20:37AM -0700, Daniel Walker wrote:
> 
> > MSM doesn't have those power states unfortunately .. Your kind of
> > suggesting what I was suggesting in that we should suspend in idle. Your
> > hardware can do it easier tho since your have power states that are
> > equal to suspend.
> 
> If your wakeup latencies are sufficiently low and you have fine-grained 
> enough control over your hardware then suspend in idle is a reasonable 
> thing to do - but if you have a userspace app that's spinning then 
> that doesn't solve the issue.

If there's a userspace app spinning then you don't go idle (or that's my
assumption anyway). You mean like repeatedly blocking and unblocking
right?

Daniel

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

[Matthew Garrett]

On Fri, May 07, 2010 at 10:35:49AM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 10:08]:
> > The situation is this. You've frozen most of your userspace because you 
> > don't trust the applications. One of those applications has an open 
> > network socket, and policy indicates that receiving a network packet 
> > should generate a wakeup, allow the userspace application to handle the 
> > packet and then return to sleep. What mechanism do you use to do that?
> 
> I think the ideal way of doing this would be to have the system running
> and hitting some deeper idle states using cpuidle. Then fix the apps
> so timers don't wake up the system too often. Then everything would
> just run in a normal way.

Effective power management in the face of real-world applications is a 
reasonable usecase.

> For the misbehaving stopped apps, maybe they could be woken
> to deal with the incoming network data with sysfs_notify?

How would that work? Have the kernel send a sysfs_notify on every netwrk 
packet and have a monitor app listen for it and unfreeze the rest of 
userspace if it's frozen? That sounds expensive.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Daniel Walker <dwalker@fifo99.com> [100507 10:15]:
> On Thu, 2010-05-06 at 19:00 -0700, Tony Lindgren wrote:
> 
> > Oh some SoC devices like omap hit retention or off modes in the idle loop.
> > That brings down the idle consumption of a running system to minimal
> > levels. It's basically the same as suspending the device in every idle loop.
> > 
> > The system wakes up every few seconds or so, but that already provides
> > battery life of over ten days or so on an idle system. Of course the
> > wakeup latencies are in milliseconds then.
> 
> MSM doesn't have those power states unfortunately .. Your kind of
> suggesting what I was suggesting in that we should suspend in idle. Your
> hardware can do it easier tho since your have power states that are
> equal to suspend.

You might be able to implement suspend-while-idle with cpuidle and
a custom idle function on MSM. That is if MSM supports waking to
a timer event and some device interrupts. However, if it only wakes
to pressing some power button, then you will get missed timers
and the system won't behave in a normal way.

Maybe you could still kill -STOP the misbehaving apps, then keep the
idle system running until some timer expires, then when no activity,
echo mem > /sys/power/state?

Regards,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 10:40:43AM -0700, Daniel Walker wrote:
> On Fri, 2010-05-07 at 18:36 +0100, Matthew Garrett wrote:
> > If your wakeup latencies are sufficiently low and you have fine-grained 
> > enough control over your hardware then suspend in idle is a reasonable 
> > thing to do - but if you have a userspace app that's spinning then 
> > that doesn't solve the issue.
> 
> If there's a userspace app spinning then you don't go idle (or that's my
> assumption anyway). You mean like repeatedly blocking and unblocking
> right?

Right, that's the problem. idle-based suspend works fine if your 
applications let the system go idle, but if your applications are 
anything other than absolutely perfect in this respect then you consume 
significant power even if the device is sitting unused in someone's 
pocket.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Daniel Walker]

On Fri, 2010-05-07 at 18:51 +0100, Matthew Garrett wrote:
> On Fri, May 07, 2010 at 10:40:43AM -0700, Daniel Walker wrote:
> > On Fri, 2010-05-07 at 18:36 +0100, Matthew Garrett wrote:
> > > If your wakeup latencies are sufficiently low and you have fine-grained 
> > > enough control over your hardware then suspend in idle is a reasonable 
> > > thing to do - but if you have a userspace app that's spinning then 
> > > that doesn't solve the issue.
> > 
> > If there's a userspace app spinning then you don't go idle (or that's my
> > assumption anyway). You mean like repeatedly blocking and unblocking
> > right?
> 
> Right, that's the problem. idle-based suspend works fine if your 
> applications let the system go idle, but if your applications are 
> anything other than absolutely perfect in this respect then you consume 
> significant power even if the device is sitting unused in someone's 
> pocket.

True .. I'd wonder how an OMAP based devices deal with that issue, since
they would have that exact problem. According to what Tony is telling
us. Actually a bogus userspace can do a lot more than just consume power
you could hang the system too.

Daniel

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> On Fri, May 07, 2010 at 10:35:49AM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100507 10:08]:
> > > The situation is this. You've frozen most of your userspace because you 
> > > don't trust the applications. One of those applications has an open 
> > > network socket, and policy indicates that receiving a network packet 
> > > should generate a wakeup, allow the userspace application to handle the 
> > > packet and then return to sleep. What mechanism do you use to do that?
> > 
> > I think the ideal way of doing this would be to have the system running
> > and hitting some deeper idle states using cpuidle. Then fix the apps
> > so timers don't wake up the system too often. Then everything would
> > just run in a normal way.
> 
> Effective power management in the face of real-world applications is a 
> reasonable usecase.

Sure there's no easy solution to misbehaving apps.

> > For the misbehaving stopped apps, maybe they could be woken
> > to deal with the incoming network data with sysfs_notify?
> 
> How would that work? Have the kernel send a sysfs_notify on every netwrk 
> packet and have a monitor app listen for it and unfreeze the rest of 
> userspace if it's frozen? That sounds expensive.

Yeah maybe there are better ways of dealing with this.

Maybe deferred timers would help some so all the apps could
be allowed to run until some power management policy decides
to suspend the whole device.

Regards,

Tony

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

[Tony Lindgren]

* Daniel Walker <dwalker@fifo99.com> [100507 10:56]:
> On Fri, 2010-05-07 at 18:51 +0100, Matthew Garrett wrote:
> > On Fri, May 07, 2010 at 10:40:43AM -0700, Daniel Walker wrote:
> > > On Fri, 2010-05-07 at 18:36 +0100, Matthew Garrett wrote:
> > > > If your wakeup latencies are sufficiently low and you have fine-grained 
> > > > enough control over your hardware then suspend in idle is a reasonable 
> > > > thing to do - but if you have a userspace app that's spinning then 
> > > > that doesn't solve the issue.
> > > 
> > > If there's a userspace app spinning then you don't go idle (or that's my
> > > assumption anyway). You mean like repeatedly blocking and unblocking
> > > right?
> > 
> > Right, that's the problem. idle-based suspend works fine if your 
> > applications let the system go idle, but if your applications are 
> > anything other than absolutely perfect in this respect then you consume 
> > significant power even if the device is sitting unused in someone's 
> > pocket.
> 
> True .. I'd wonder how an OMAP based devices deal with that issue, since
> they would have that exact problem. According to what Tony is telling
> us. Actually a bogus userspace can do a lot more than just consume power
> you could hang the system too.

There's nothing being done on omaps specifically, up to the device
user space to deal with that. From the kernel point of view the
omaps just run, and if idle enough, the device starts hitting the
retention and off modes in idle. But the system keeps on running
all the time, no need to suspend really. 

I don't think there's a generic solution to the misbehaving apps.
I know a lot of work has been done over past five years or so
to minimize the timer usage in various apps. But if I installed
some app that keeps the system busy, it would drain the battery.

I guess some apps could be just stopped when the screen blanks
unless somehow certified for the timer usage or something similar..

Regards,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 11:01:52AM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> > Effective power management in the face of real-world applications is a 
> > reasonable usecase.
> 
> Sure there's no easy solution to misbehaving apps.

That's the point of the suspend blockers.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 11:23]:
> On Fri, May 07, 2010 at 11:01:52AM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> > > Effective power management in the face of real-world applications is a 
> > > reasonable usecase.
> > 
> > Sure there's no easy solution to misbehaving apps.
> 
> That's the point of the suspend blockers.

To me it sounds like suspending the whole system to deal with
some misbehaving apps is an overkill. Sounds like kill -STOP
the misbehaving apps should do the trick?

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 11:43:33AM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 11:23]:
> > On Fri, May 07, 2010 at 11:01:52AM -0700, Tony Lindgren wrote:
> > > * Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> > > > Effective power management in the face of real-world applications is a 
> > > > reasonable usecase.
> > > 
> > > Sure there's no easy solution to misbehaving apps.
> > 
> > That's the point of the suspend blockers.
> 
> To me it sounds like suspending the whole system to deal with
> some misbehaving apps is an overkill. Sounds like kill -STOP
> the misbehaving apps should do the trick?

Freezer cgroups would work better, but it doesn't really change the 
point - if that application has an open network socket, how do you know 
to resume that application when a packet comes in?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Daniel Walker]

On Fri, 2010-05-07 at 19:46 +0100, Matthew Garrett wrote:
> On Fri, May 07, 2010 at 11:43:33AM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100507 11:23]:
> > > On Fri, May 07, 2010 at 11:01:52AM -0700, Tony Lindgren wrote:
> > > > * Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> > > > > Effective power management in the face of real-world applications is a 
> > > > > reasonable usecase.
> > > > 
> > > > Sure there's no easy solution to misbehaving apps.
> > > 
> > > That's the point of the suspend blockers.
> > 
> > To me it sounds like suspending the whole system to deal with
> > some misbehaving apps is an overkill. Sounds like kill -STOP
> > the misbehaving apps should do the trick?
> 
> Freezer cgroups would work better, but it doesn't really change the 
> point - if that application has an open network socket, how do you know 
> to resume that application when a packet comes in?

suspend blockers can get abused also .. I had my phone in my pocket and
accidentally ran "Google Talk" or something. It must have kept the
screen on or kept the phone from suspending, so the battery drained
completely over the course of an hour or so.

Daniel

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

[Tony Lindgren]

* Daniel Walker <dwalker@fifo99.com> [100507 12:01]:
> On Fri, 2010-05-07 at 19:46 +0100, Matthew Garrett wrote:
> > On Fri, May 07, 2010 at 11:43:33AM -0700, Tony Lindgren wrote:
> > > * Matthew Garrett <mjg@redhat.com> [100507 11:23]:
> > > > On Fri, May 07, 2010 at 11:01:52AM -0700, Tony Lindgren wrote:
> > > > > * Matthew Garrett <mjg@redhat.com> [100507 10:46]:
> > > > > > Effective power management in the face of real-world applications is a 
> > > > > > reasonable usecase.
> > > > > 
> > > > > Sure there's no easy solution to misbehaving apps.
> > > > 
> > > > That's the point of the suspend blockers.
> > > 
> > > To me it sounds like suspending the whole system to deal with
> > > some misbehaving apps is an overkill. Sounds like kill -STOP
> > > the misbehaving apps should do the trick?
> > 
> > Freezer cgroups would work better, but it doesn't really change the 
> > point - if that application has an open network socket, how do you know 
> > to resume that application when a packet comes in?

No idea, but that still sounds a better situation to me than
trying to deal with that for a suspended system! :)
 
> suspend blockers can get abused also .. I had my phone in my pocket and
> accidentally ran "Google Talk" or something. It must have kept the
> screen on or kept the phone from suspending, so the battery drained
> completely over the course of an hour or so.

Yeah I guess there's nothing stopping that.

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 12:28:37PM -0700, Tony Lindgren wrote:
> * Daniel Walker <dwalker@fifo99.com> [100507 12:01]:
> > On Fri, 2010-05-07 at 19:46 +0100, Matthew Garrett wrote:
> > > Freezer cgroups would work better, but it doesn't really change the 
> > > point - if that application has an open network socket, how do you know 
> > > to resume that application when a packet comes in?
> 
> No idea, but that still sounds a better situation to me than
> trying to deal with that for a suspended system! :)

Suspend blocks deal with that problem. Nobody has yet demonstrated a 
workable alternative solution.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 12:29]:
> On Fri, May 07, 2010 at 12:28:37PM -0700, Tony Lindgren wrote:
> > * Daniel Walker <dwalker@fifo99.com> [100507 12:01]:
> > > On Fri, 2010-05-07 at 19:46 +0100, Matthew Garrett wrote:
> > > > Freezer cgroups would work better, but it doesn't really change the 
> > > > point - if that application has an open network socket, how do you know 
> > > > to resume that application when a packet comes in?
> > 
> > No idea, but that still sounds a better situation to me than
> > trying to deal with that for a suspended system! :)
> 
> Suspend blocks deal with that problem. Nobody has yet demonstrated a 
> workable alternative solution.

Well there are obviously two paths to take, I think both of
them should work. The alternative to suspend blockers is:

- Implement a decent kernel idle function for the hardware and
  use cpuidle to change the c states. The dyntick stuff should
  already work for most hardware.

- Fix the core userspace apps to minimize timers.

- Deal with broken apps whichever way you want in the userspace.

- Optionally, do echo mem > /sys/power/state based on some
  product specific logic in the userspace.

The advantage of this is that no kernel changes are needed,
except for implementing the custom idle function for the
hardware. And this kind of setup has been in use for about
five years.

And, you can keep the system running constantly if you have
hardware that supports good idle modes, then you don't even
need to suspend at all.

The core problems I see with suspend blockers are following,
please correct me if I'm wrong:

- It is caching the value of echo mem > /sys/power/state and
  misusing it for runtime power management as the system still
  keeps running after trying to suspend. Instead, the kernel
  idle function and cpuidle should be used if the kernel keeps
  running.

- They require patching all over the drivers and userspace.

- Once the system is suspended, it does not run. And the apps
  don't behave in a standard way because the system does not
  wake to timer interrupts.

I agree that we need to be able to echo mem > /sys/power/state
in an atomic way. So if there are problems with that, those
issues should be fixed.

Cheers,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 12:55:48PM -0700, Tony Lindgren wrote:

> - Deal with broken apps whichever way you want in the userspace.

If we could do this then there would be no real need for suspend 
blockers.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 13:24]:
> On Fri, May 07, 2010 at 12:55:48PM -0700, Tony Lindgren wrote:
> 
> > - Deal with broken apps whichever way you want in the userspace.
> 
> If we could do this then there would be no real need for suspend 
> blockers.

OK, I guess I don't understand all the details, need some kind
of common example I guess.

So for example, if I leave ping running in a a terminal, do you
have some way of preventing that from eating the battery?

In my scenario that program would keep on running until the
battery runs out, or something stops the program. But the system
keeps hitting retention mode in the idle loop.

How do you deal with programs like that?

Do you just suspend the whole system anyways at some point,
or do you have some other trick?

Regards,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 01:53:29PM -0700, Tony Lindgren wrote:

> So for example, if I leave ping running in a a terminal, do you
> have some way of preventing that from eating the battery?

It depends on policy. If all network packets generate wakeup events then 
no, that will carry on eating battery. If ICMP doesn't generate a wakeup 
event then the process won't be run.

> Do you just suspend the whole system anyways at some point,
> or do you have some other trick?

If nothing's holding any suspend blocks then the system will enter 
suspend. If the packet generates a wakeup then the kernel would block 
suspend until userspace has had the opportunity to do so. Once userspace 
has handled the packet then it could release the block and the system 
will immediately transition back into suspend.

Here's a different example. A process is waiting for a keypress, but 
because it's badly written it's also drawing to the screen at 60 frames 
per second and preventing the system from every going to idle. How do 
you quiesce the system while still ensuring that the keypress will be 
delivered to the application?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 13:58]:
> On Fri, May 07, 2010 at 01:53:29PM -0700, Tony Lindgren wrote:
> 
> > So for example, if I leave ping running in a a terminal, do you
> > have some way of preventing that from eating the battery?
> 
> It depends on policy. If all network packets generate wakeup events then 
> no, that will carry on eating battery. If ICMP doesn't generate a wakeup 
> event then the process won't be run.
> 
> > Do you just suspend the whole system anyways at some point,
> > or do you have some other trick?
> 
> If nothing's holding any suspend blocks then the system will enter 
> suspend. If the packet generates a wakeup then the kernel would block 
> suspend until userspace has had the opportunity to do so. Once userspace 
> has handled the packet then it could release the block and the system 
> will immediately transition back into suspend.

OK, then what would I need to do to keep that ping running if I wanted to?

> Here's a different example. A process is waiting for a keypress, but 
> because it's badly written it's also drawing to the screen at 60 frames 
> per second and preventing the system from every going to idle. How do 
> you quiesce the system while still ensuring that the keypress will be 
> delivered to the application?

I guess it depends. If it's a game and I'm waiting to hit the fire
button, then I don't want the system to suspend!

It's starting to sound like you're really using suspend blocks
to "certify" that the app is safe to keep running.

Maybe it could be done with some kind of process flag instead that
would tell "this process is safe to keep running from timer point of view"
and if that flag is not set, then assume it's OK to stop the process
at any point?

Regards,

Tony

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

[Daniel Walker]

On Fri, 2010-05-07 at 22:03 +0100, Matthew Garrett wrote:

> Here's a different example. A process is waiting for a keypress, but 
> because it's badly written it's also drawing to the screen at 60 frames 
> per second and preventing the system from every going to idle. How do 
> you quiesce the system while still ensuring that the keypress will be 
> delivered to the application?

To me it's somewhat of a negative for suspend blockers. Since to solve
the problem you give above you would have to use a suspend blocker in an
asynchronous way (locked in an interrupt, released in a thread too)
assuming I understand your example. I've had my share of semaphore
nightmares, and I'm not too excited to see a protection scheme (i.e. a
lock) which allows asynchronous usage like suspend blockers. 

Daniel

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

[Arve Hjønnevåg]

On Fri, May 7, 2010 at 2:25 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 13:58]:
>> On Fri, May 07, 2010 at 01:53:29PM -0700, Tony Lindgren wrote:
>>
>> > So for example, if I leave ping running in a a terminal, do you
>> > have some way of preventing that from eating the battery?
>>
>> It depends on policy. If all network packets generate wakeup events then
>> no, that will carry on eating battery. If ICMP doesn't generate a wakeup
>> event then the process won't be run.
>>
>> > Do you just suspend the whole system anyways at some point,
>> > or do you have some other trick?
>>
>> If nothing's holding any suspend blocks then the system will enter
>> suspend. If the packet generates a wakeup then the kernel would block
>> suspend until userspace has had the opportunity to do so. Once userspace
>> has handled the packet then it could release the block and the system
>> will immediately transition back into suspend.
>
> OK, then what would I need to do to keep that ping running if I wanted to?
>

Use a suspend blocker. For instance: "runsuspendblock ping <addr>".

>> Here's a different example. A process is waiting for a keypress, but
>> because it's badly written it's also drawing to the screen at 60 frames
>> per second and preventing the system from every going to idle. How do
>> you quiesce the system while still ensuring that the keypress will be
>> delivered to the application?
>
> I guess it depends. If it's a game and I'm waiting to hit the fire
> button, then I don't want the system to suspend!
>

We don't suspend while the screen is on. If the user pressed the power
button to turn the screen off, then I would not want the game to
prevent suspend.

> It's starting to sound like you're really using suspend blocks
> to "certify" that the app is safe to keep running.
>
> Maybe it could be done with some kind of process flag instead that
> would tell "this process is safe to keep running from timer point of view"
> and if that flag is not set, then assume it's OK to stop the process
> at any point?
>
> Regards,
>
> Tony
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm
>



-- 
Arve Hjønnevåg

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

[Arve Hjønnevåg]

On Fri, May 7, 2010 at 2:30 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> On Fri, 2010-05-07 at 22:03 +0100, Matthew Garrett wrote:
>
>> Here's a different example. A process is waiting for a keypress, but
>> because it's badly written it's also drawing to the screen at 60 frames
>> per second and preventing the system from every going to idle. How do
>> you quiesce the system while still ensuring that the keypress will be
>> delivered to the application?
>
> To me it's somewhat of a negative for suspend blockers. Since to solve
> the problem you give above you would have to use a suspend blocker in an
> asynchronous way (locked in an interrupt, released in a thread too)
> assuming I understand your example. I've had my share of semaphore
> nightmares, and I'm not too excited to see a protection scheme (i.e. a
> lock) which allows asynchronous usage like suspend blockers.
>

Why do you think this? The example in the documentation describe how
we handle key events.


-- 
Arve Hjønnevåg

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

[Matthew Garrett]

On Fri, May 07, 2010 at 02:30:20PM -0700, Daniel Walker wrote:
> On Fri, 2010-05-07 at 22:03 +0100, Matthew Garrett wrote:
> 
> > Here's a different example. A process is waiting for a keypress, but 
> > because it's badly written it's also drawing to the screen at 60 frames 
> > per second and preventing the system from every going to idle. How do 
> > you quiesce the system while still ensuring that the keypress will be 
> > delivered to the application?
> 
> To me it's somewhat of a negative for suspend blockers. Since to solve
> the problem you give above you would have to use a suspend blocker in an
> asynchronous way (locked in an interrupt, released in a thread too)
> assuming I understand your example. I've had my share of semaphore
> nightmares, and I'm not too excited to see a protection scheme (i.e. a
> lock) which allows asynchronous usage like suspend blockers. 

Check the input patch for an example of this.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Matthew Garrett]

On Fri, May 07, 2010 at 02:25:56PM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 13:58]:
> > Here's a different example. A process is waiting for a keypress, but 
> > because it's badly written it's also drawing to the screen at 60 frames 
> > per second and preventing the system from every going to idle. How do 
> > you quiesce the system while still ensuring that the keypress will be 
> > delivered to the application?
> 
> I guess it depends. If it's a game and I'm waiting to hit the fire
> button, then I don't want the system to suspend!
> 
> It's starting to sound like you're really using suspend blocks
> to "certify" that the app is safe to keep running.
> 
> Maybe it could be done with some kind of process flag instead that
> would tell "this process is safe to keep running from timer point of view"
> and if that flag is not set, then assume it's OK to stop the process
> at any point?

How do you know to wake the process up in response to the keypress?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 14:34]:
> On Fri, May 07, 2010 at 02:25:56PM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100507 13:58]:
> > > Here's a different example. A process is waiting for a keypress, but 
> > > because it's badly written it's also drawing to the screen at 60 frames 
> > > per second and preventing the system from every going to idle. How do 
> > > you quiesce the system while still ensuring that the keypress will be 
> > > delivered to the application?
> > 
> > I guess it depends. If it's a game and I'm waiting to hit the fire
> > button, then I don't want the system to suspend!
> > 
> > It's starting to sound like you're really using suspend blocks
> > to "certify" that the app is safe to keep running.
> > 
> > Maybe it could be done with some kind of process flag instead that
> > would tell "this process is safe to keep running from timer point of view"
> > and if that flag is not set, then assume it's OK to stop the process
> > at any point?
> 
> How do you know to wake the process up in response to the keypress?

Does it matter for processes that are not "certified"? Maybe you
could assume that you can keep it stopped until the screen is on
again, or some other policy.

Tony

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

[Daniel Walker]

On Fri, 2010-05-07 at 14:35 -0700, Arve Hjønnevåg wrote:
> On Fri, May 7, 2010 at 2:30 PM, Daniel Walker <dwalker@fifo99.com> wrote:
> > On Fri, 2010-05-07 at 22:03 +0100, Matthew Garrett wrote:
> >
> >> Here's a different example. A process is waiting for a keypress, but
> >> because it's badly written it's also drawing to the screen at 60 frames
> >> per second and preventing the system from every going to idle. How do
> >> you quiesce the system while still ensuring that the keypress will be
> >> delivered to the application?
> >
> > To me it's somewhat of a negative for suspend blockers. Since to solve
> > the problem you give above you would have to use a suspend blocker in an
> > asynchronous way (locked in an interrupt, released in a thread too)
> > assuming I understand your example. I've had my share of semaphore
> > nightmares, and I'm not too excited to see a protection scheme (i.e. a
> > lock) which allows asynchronous usage like suspend blockers.
> >
> 
> Why do you think this? The example in the documentation describe how
> we handle key events.

+- 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. If it determines that
+  the key should leave the screen off, it calls suspend_unblock on the
+  process_input_events suspend_blocker and then calls select or poll. The
+  system will automatically suspend again, since now no suspend blockers are
+  active.

This? Isn't this asynchronous on the input-event-queue since it's taken
in the interrupt , and release in the userspace thread?

Daniel

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

[Matthew Garrett]

On Fri, May 07, 2010 at 02:42:11PM -0700, Tony Lindgren wrote:
> * Matthew Garrett <mjg@redhat.com> [100507 14:34]:
> > How do you know to wake the process up in response to the keypress?
> 
> Does it matter for processes that are not "certified"? Maybe you
> could assume that you can keep it stopped until the screen is on
> again, or some other policy.

Yes, it matters. You don't necessarily know whether to turn the screen 
on until the app has had an opportunity to process the event. This is 
exactly the kind of use case that suspend blocks are intended to allow, 
so alternatives that don't permit that kind of use case aren't really 
adequate alternatives.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Tony Lindgren]

* Matthew Garrett <mjg@redhat.com> [100507 14:44]:
> On Fri, May 07, 2010 at 02:42:11PM -0700, Tony Lindgren wrote:
> > * Matthew Garrett <mjg@redhat.com> [100507 14:34]:
> > > How do you know to wake the process up in response to the keypress?
> > 
> > Does it matter for processes that are not "certified"? Maybe you
> > could assume that you can keep it stopped until the screen is on
> > again, or some other policy.
> 
> Yes, it matters. You don't necessarily know whether to turn the screen 
> on until the app has had an opportunity to process the event. This is 
> exactly the kind of use case that suspend blocks are intended to allow, 
> so alternatives that don't permit that kind of use case aren't really 
> adequate alternatives.

Hmm, I'm thinking there would not be any need to turn the screen on
for the broken apps until some other event such as a tap on the screen
triggers the need to turn the screen on.

If it's a critical app, then it should be fixed so it's safe to keep
running.

And yeah, I guess you could cgroups to categorize "timer certified"
and "broken" apps.

Regards,

Tony

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

[Matthew Garrett]

On Fri, May 07, 2010 at 03:00:26PM -0700, Tony Lindgren wrote:

> Hmm, I'm thinking there would not be any need to turn the screen on
> for the broken apps until some other event such as a tap on the screen
> triggers the need to turn the screen on.
> 
> If it's a critical app, then it should be fixed so it's safe to keep
> running.
> 
> And yeah, I guess you could cgroups to categorize "timer certified"
> and "broken" apps.

This is a perfectly valid model, but it's not one that matches Android.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

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

[Pavel Machek]

> >> One if the benefit we get from using suspend is that an unprivileged
> >> app that does not have access to suspend blockers cannot prevent
> >> suspend. You lose this advantage if you trigger suspend only from the
> >> idle task.
> >>
> >>
> > If the process (privileged or unprivileged) doesn't want to suspend, why
> > not just provide an interface to allow suspend to be turned off at the
> > user level.  This could block the suspend cycle in itself, and you
> > shouldn't need fine grained off/on cycles.  If an application really
> > needs the system not to suspend then they (the user) should know the
> > consequences and power requirements for such a task.
> >
> > I didn't say it had to be only from the idle task; but, that is the most
> > logical place.  If the other threads are not idle then they really
> > require work and will most likely already have a bock on the suspend anyway.
> >
> 
> I think you missed my point. Unprivileged processes should not be
> allowed to prevent suspend.

Currently, unpriviledged processes are allowed to eat power, for
example while(1). We should keep that behaviour at least by default.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Pavel Machek]

Hi!

> >>> Tony,
> >>> Wouldn't this be handled by the idle task, or task manager?
> >>>
> >>> When all tasks are suspended and not doing anything that should be the
> >>> first clue that a real suspend cycle could be attempted.
> >>>
> >>>
> >> One if the benefit we get from using suspend is that an unprivileged
> >> app that does not have access to suspend blockers cannot prevent
> >> suspend. You lose this advantage if you trigger suspend only from the
> >> idle task.
> >>
> >>
> > If the process (privileged or unprivileged) doesn't want to suspend, why
> > not just provide an interface to allow suspend to be turned off at the
> > user level.  This could block the suspend cycle in itself, and you
> > shouldn't need fine grained off/on cycles.  If an application really
> > needs the system not to suspend then they (the user) should know the
> > consequences and power requirements for such a task.
> >
> > I didn't say it had to be only from the idle task; but, that is the most
> > logical place.  If the other threads are not idle then they really
> > require work and will most likely already have a bock on the suspend anyway.

> I think you missed my point. Unprivileged processes should not be
> allowed to prevent suspend.

Currently, we do not suspend automatically, and not suspending when
*anything* is running is clearly backwards compatible....

 If suspend is disallowed, application doing while(1); will consume
more power than the one doing sleep(1) so.... I'm not sure how much
sense such "security policy" makes.

Anyway, for android use case, maybe you could have something like
ksuspendd, and adjust its priority? That way, you could have tasks
below the priority of ksuspendd, and those would not block suspend.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Pavel Machek]

Hi!

> >> >> How often would we retry suspending?
> >> >
> >> > Well based on some timer, the same way the screen blanks? Or five
> >> > seconds of no audio play? So if the suspend fails, then reset whatever
> >> > userspace suspend policy timers.
> >> >
> >> >> If we fail to suspend, don't we have to resume all the drivers that
> >> >> suspended before the one that failed?  (Maybe I'm mistaken here)
> >> >
> >> > Sure, but I guess that should be a rare event that only happens when
> >> > you try to suspend and something interrupts the suspend.
> >> >
> >>
> >> This is not a rare event. For example, the matrix keypad driver blocks
> >> suspend when a key is down so it can scan the matrix.
> >
> > Sure, but how many times per day are you suspending?
> 
> How many times we successfully suspend is irrelevant here. If the
> driver blocks suspend the number of suspend attempts depend on your
> poll frequency.

Actually, this is quite interesting question I'd like answer here:

"Sure, but how many times per day are you suspending?"

I suspect it may be in 1000s, but it would be cool to get better
answer -- so that people knew what we are talking about here.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Arve Hjønnevåg]

On Thu, May 27, 2010 at 11:43 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> >> How often would we retry suspending?
>> >> >
>> >> > Well based on some timer, the same way the screen blanks? Or five
>> >> > seconds of no audio play? So if the suspend fails, then reset whatever
>> >> > userspace suspend policy timers.
>> >> >
>> >> >> If we fail to suspend, don't we have to resume all the drivers that
>> >> >> suspended before the one that failed?  (Maybe I'm mistaken here)
>> >> >
>> >> > Sure, but I guess that should be a rare event that only happens when
>> >> > you try to suspend and something interrupts the suspend.
>> >> >
>> >>
>> >> This is not a rare event. For example, the matrix keypad driver blocks
>> >> suspend when a key is down so it can scan the matrix.
>> >
>> > Sure, but how many times per day are you suspending?
>>
>> How many times we successfully suspend is irrelevant here. If the
>> driver blocks suspend the number of suspend attempts depend on your
>> poll frequency.
>
> Actually, this is quite interesting question I'd like answer here:
>
> "Sure, but how many times per day are you suspending?"
>
> I suspect it may be in 1000s, but it would be cool to get better
> answer -- so that people knew what we are talking about here.

This is highly variable. 1000s would mean that you wake about once
every minute which is not uncommon, but more often than what typically
happens on an idle device. The nexus one has to wake up every 10
minutes to check the battery status check means your at least in the
100s, but the G1 could stay suspended for much longer than that.

The original question was about a driver blocking suspend though, and
if we were to just retry suspend until it succeeds in that case you
could easily get to 100000s suspend attempts in a day.

-- 
Arve Hjønnevåg

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

[Pavel Machek]

Hi!

> > > Why would you need to constantly try to suspend in that case?
> > 
> > Because otherwise you're awake for longer than you need to be.
> 
> If your system is idle and your hardware supports off-while-idle,
> then that really does not matter. There's not much of a difference
> in power savings, we're already talking over 10 days on batteries
> with just off-while-idle on omaps.

Makes me wish g1 was omap based... it looks like you have superior hw.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Brian Swetland]

On Fri, May 28, 2010 at 6:29 AM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> > > Why would you need to constantly try to suspend in that case?
>> >
>> > Because otherwise you're awake for longer than you need to be.
>>
>> If your system is idle and your hardware supports off-while-idle,
>> then that really does not matter. There's not much of a difference
>> in power savings, we're already talking over 10 days on batteries
>> with just off-while-idle on omaps.
>
> Makes me wish g1 was omap based... it looks like you have superior hw.

G1 will happily do 10 days idle (radio on) under typical network
conditions (roughly 4-5mA draw at the battery average in paging mode)
if you have data disabled and there's no reason for it to wake up,
process events, chat on the data network etc.  It'll go 25-30 days in
"airplane mode" (radio off) provided there are not excessive wakeups.

If you happen to be running a perfect userspace where every thread in
every process is blocked on something, it'll hit the exact same power
state out of idle.  If you have a less optimal userspace or random
third party nonoptimal apps, this becomes much harder, of course.
Which is why we do the wakelock thing.

OMAP does have a lot of nice auto-clock-down features compared to some
other SoCs, sometimes simplifying other parts of power management.

Brian