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

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

[Arve Hjønnevåg]

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/suspend-blockers.txt |   97 +++++++++++++
 include/linux/suspend_blocker.h          |   60 ++++++++
 kernel/power/Kconfig                     |   11 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/main.c                      |   84 +++++++++++-
 kernel/power/power.h                     |    5 +
 kernel/power/suspend.c                   |    4 +-
 kernel/power/suspend_blocker.c           |  219 ++++++++++++++++++++++++++++++
 8 files changed, 477 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/power/suspend-blockers.txt
 create mode 100755 include/linux/suspend_blocker.h
 create mode 100644 kernel/power/suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
new file mode 100644
index 0000000..1c48514
--- /dev/null
+++ b/Documentation/power/suspend-blockers.txt
@@ -0,0 +1,97 @@
+Suspend blockers
+================
+
+Suspend blockers provide a mechanism for device drivers and user-space processes
+to prevent the system from entering suspend. By default writing to
+/sys/power/state will ignore suspend blockers. Writing "opportunistic" to
+/sys/power/policy will change the behaviour of /sys/power/state to repeatedly
+enter the requested state when no suspend blockers are active. Writing "on" to
+/sys/power/state will cancel the automatic sleep request. Suspend blockers do
+not affect sleep states entered from idle.
+
+To use a suspend blocker, a device driver needs a struct suspend_blocker object
+that has to be initialized with suspend_blocker_init. When no longer needed,
+the suspend blocker must be destroyed with suspend_blocker_destroy.
+
+A suspend blocker is activated using suspend_block, which prevents the system
+from entering suspend until the suspend blocker is deactivated with
+suspend_unblock. Many suspend blockers may be used simultaneously, and the
+system is prevented from entering suspend as long as at least one of them is
+active.
+
+If the suspend operation has already started when calling suspend_block on a
+suspend_blocker, it will abort the suspend operation as long it has not already
+reached called suspend_ops->enter. This means that calling suspend_block from an
+interrupt handler or a freezeable thread always works, but some or all drivers
+may be temporarily suspended.
+
+In cell phones or other embedded systems where powering the screen is a
+significant drain on the battery, suspend blockers can be used to allow
+user-space to decide whether a keystroke received while the system is suspended
+should cause the screen to be turned back on or allow the system to go back into
+suspend. 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 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.
+
+                 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. 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..21689cd
--- /dev/null
+++ b/include/linux/suspend_blocker.h
@@ -0,0 +1,60 @@
+/* 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
+
+/**
+ * struct suspend_blocker - the basic suspend_blocker structure
+ * @flags:	Tracks initialized and active state.
+ * @name:	Name used for debugging.
+ *
+ * When a suspend_blocker is active it prevents the system from entering
+ * suspend.
+ *
+ * The suspend_blocker structure must be initialized by suspend_blocker_init()
+ */
+
+struct suspend_blocker {
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	atomic_t            flags;
+	const char         *name;
+#endif
+};
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+
+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..f8fa246 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -130,6 +130,17 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config SUSPEND_BLOCKERS
+	bool "Suspend blockers"
+	depends on PM
+	select RTC_LIB
+	default n
+	---help---
+	  Suspend blocker support. Write "opportunistic" to /sys/power/policy
+	  to enable suspend blockers. Then, when user space requests a sleep
+	  state through /sys/power/state, the requested sleep state will be
+	  entered when no suspend blockers are active.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 4319181..f570801 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_SUSPEND_BLOCKERS)	+= 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..9061ea0 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_SUSPEND_BLOCKERS
+	{
+		.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,7 +183,7 @@ 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
@@ -173,7 +201,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;
@@ -195,7 +223,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 +232,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 +308,7 @@ power_attr(pm_trace);
 
 static struct attribute * g[] = {
 	&state_attr.attr,
+	&policy_attr.attr,
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 46c5a26..9b468d7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -236,3 +236,8 @@ 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);
+
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..1b345d1
--- /dev/null
+++ b/kernel/power/suspend_blocker.c
@@ -0,0 +1,219 @@
+/* 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"
+
+enum {
+	DEBUG_EXIT_SUSPEND = 1U << 0,
+	DEBUG_WAKEUP = 1U << 1,
+	DEBUG_USER_STATE = 1U << 2,
+	DEBUG_SUSPEND = 1U << 3,
+	DEBUG_SUSPEND_BLOCKER = 1U << 4,
+};
+static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+#define SB_INITIALIZED            (1U << 8)
+#define SB_ACTIVE                 (1U << 9)
+
+static DEFINE_SPINLOCK(state_lock);
+static atomic_t suspend_block_count;
+static atomic_t current_event_num;
+struct workqueue_struct *suspend_work_queue;
+struct suspend_blocker main_suspend_blocker;
+static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
+static bool enable_suspend_blockers;
+
+#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);
+
+/**
+ * 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 !!atomic_read(&suspend_block_count);
+}
+
+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 = atomic_read(&current_event_num);
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: enter suspend\n");
+		ret = pm_suspend(requested_suspend_state);
+		if (debug_mask & DEBUG_EXIT_SUSPEND)
+			pr_info_time("suspend: exit suspend, ret = %d ", ret);
+		if (atomic_read(&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)
+{
+	WARN_ON(!name);
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_init name=%s\n", name);
+
+	blocker->name = name;
+	atomic_set(&blocker->flags, SB_INITIALIZED);
+}
+EXPORT_SYMBOL(suspend_blocker_init);
+
+/**
+ * suspend_blocker_destroy() - Destroy a suspend blocker
+ * @blocker:	The suspend blocker to destroy.
+ */
+void suspend_blocker_destroy(struct suspend_blocker *blocker)
+{
+	int flags;
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
+	flags = atomic_xchg(&blocker->flags, 0);
+	WARN(!(flags & SB_INITIALIZED), "suspend_blocker_destroy called on "
+					"uninitialized suspend_blocker\n");
+	if (flags == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_blocker_destroy);
+
+/**
+ * suspend_block() - Block suspend
+ * @blocker:	The suspend blocker to use
+ */
+void suspend_block(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_block: %s\n", blocker->name);
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED,
+	    SB_INITIALIZED | SB_ACTIVE) == SB_INITIALIZED)
+		atomic_inc(&suspend_block_count);
+
+	atomic_inc(&current_event_num);
+}
+EXPORT_SYMBOL(suspend_block);
+
+/**
+ * suspend_unblock() - Unblock suspend
+ * @blocker:	The suspend blocker to unblock.
+ *
+ * If no other suspend blockers block suspend, the system will suspend.
+ */
+void suspend_unblock(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_unblock: %s\n", blocker->name);
+
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED | SB_ACTIVE,
+	    SB_INITIALIZED) == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_unblock);
+
+/**
+ * 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(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	return !!(atomic_read(&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;
+}
+
+static int __init suspend_block_init(void)
+{
+	suspend_work_queue = create_singlethread_workqueue("suspend");
+	if (!suspend_work_queue)
+		return -ENOMEM;
+
+	suspend_blocker_init(&main_suspend_blocker, "main");
+	suspend_block(&main_suspend_blocker);
+	return 0;
+}
+
+core_initcall(suspend_block_init);
-- 
1.6.5.1

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

Re: [PATCH 0/9] Suspend block api (version 4)

[Greg KH]

On Thu, Apr 22, 2010 at 06:08:49PM -0700, Arve Hj??nnev??g wrote:
> This patch series adds a suspend-block api that provides the same
> functionality as the android wakelock api. The code is mostly the
> same as version 3 posted last year, but the main entry point has
> changed back to /sys/power/state with a new /sys/power/policy to
> alter its behavior. Timeout support is not included in this initial
> patchset.

Yeah!  Thanks for respining this and pushing it out.  This should help
lots of drivers get merged after this is in-tree.

thanks again,

greg k-h

[PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

Add a misc device, "suspend_blocker", that allows user-space processes
to block auto suspend. The device has ioctls to create a suspend_blocker,
and to block and unblock suspend. To delete the suspend_blocker, close
the device.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |   17 ++++
 include/linux/suspend_block_dev.h        |   25 ++++++
 kernel/power/Kconfig                     |    9 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/user_suspend_blocker.c      |  128 ++++++++++++++++++++++++++++++
 5 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 1c48514..877bd8c 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..24bc5c7
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
+#define _LINUX_SUSPEND_BLOCK_DEV_H
+
+#include <linux/ioctl.h>
+
+#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index f8fa246..1ac50ee 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -141,6 +141,15 @@ config SUSPEND_BLOCKERS
 	  state through /sys/power/state, the requested sleep state will be
 	  entered when no suspend blockers are active.
 
+config USER_SUSPEND_BLOCKERS
+	bool "Userspace suspend blockers"
+	depends on SUSPEND_BLOCKERS
+	default y
+	---help---
+	  User-space suspend block api. Creates a misc device with ioctls
+	  to create, block and unblock a suspend_blocker. The suspend_blocker
+	  will be deleted when the device is closed.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index f570801..80086c6 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_SUSPEND_BLOCKERS)	+= suspend_blocker.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_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/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..a9be6f4
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,128 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 2009-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/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/suspend_blocker.h>
+#include <linux/suspend_block_dev.h>
+
+enum {
+	DEBUG_FAILURE	= BIT(0),
+};
+static int debug_mask = DEBUG_FAILURE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+static DEFINE_MUTEX(ioctl_lock);
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[0];
+};
+
+static int create_user_suspend_blocker(struct file *file, void __user *name,
+				 size_t name_len)
+{
+	struct user_suspend_blocker *bl;
+	if (file->private_data)
+		return -EBUSY;
+	if (name_len > NAME_MAX)
+		return -ENAMETOOLONG;
+	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+	if (copy_from_user(bl->name, name, name_len))
+		goto err_fault;
+	suspend_blocker_init(&bl->blocker, bl->name);
+	file->private_data = bl;
+	return 0;
+
+err_fault:
+	kfree(bl);
+	return -EFAULT;
+}
+
+static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
+				unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *bl;
+	long ret;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
+		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	bl = file->private_data;
+	if (!bl) {
+		ret = -ENOENT;
+		goto done;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&bl->blocker);
+		ret = 0;
+		break;
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&bl->blocker);
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTSUPP;
+	}
+done:
+	if (ret && debug_mask & DEBUG_FAILURE)
+		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
+			cmd, ret);
+	mutex_unlock(&ioctl_lock);
+	return ret;
+}
+
+static int user_suspend_blocker_release(struct inode *inode, struct file *file)
+{
+	struct user_suspend_blocker *bl = file->private_data;
+	if (!bl)
+		return 0;
+	suspend_blocker_destroy(&bl->blocker);
+	kfree(bl);
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.5.1

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

[PATCH 3/9] PM: suspend_block: Abort task freezing if a suspend_blocker is active.

[Arve Hjønnevåg]

If a suspend_blocker is active, suspend will fail anyway. Since
try_to_freeze_tasks can take up to 20 seconds to complete or fail, aborting
as soon as someone blocks suspend (e.g. from an interrupt handler) improves
the worst case wakeup latency.

On an older kernel where task freezing could fail for processes attached
to a debugger, this fixed a problem where the device sometimes hung for
20 seconds before the screen turned on.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 kernel/power/process.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index 71ae290..d8ebd50 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -15,6 +15,7 @@
 #include <linux/syscalls.h>
 #include <linux/freezer.h>
 #include <linux/delay.h>
+#include <linux/suspend_blocker.h>
 
 /* 
  * Timeout for stopping processes
@@ -38,6 +39,7 @@ static int try_to_freeze_tasks(bool sig_only)
 	struct timeval start, end;
 	u64 elapsed_csecs64;
 	unsigned int elapsed_csecs;
+	bool wakeup = false;
 
 	do_gettimeofday(&start);
 
@@ -63,6 +65,10 @@ static int try_to_freeze_tasks(bool sig_only)
 				todo++;
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
+		if (todo && suspend_is_blocked()) {
+			wakeup = true;
+			break;
+		}
 		if (!todo || time_after(jiffies, end_time))
 			break;
 
@@ -85,13 +91,15 @@ static int try_to_freeze_tasks(bool sig_only)
 		 * but it cleans up leftover PF_FREEZE requests.
 		 */
 		printk("\n");
-		printk(KERN_ERR "Freezing of tasks failed after %d.%02d seconds "
+		printk(KERN_ERR "Freezing of tasks %s after %d.%02d seconds "
 				"(%d tasks refusing to freeze):\n",
+				wakeup ? "aborted" : "failed",
 				elapsed_csecs / 100, elapsed_csecs % 100, todo);
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			task_lock(p);
-			if (freezing(p) && !freezer_should_skip(p))
+			if (freezing(p) && !freezer_should_skip(p)
+					&& elapsed_csecs > 100)
 				sched_show_task(p);
 			cancel_freezing(p);
 			task_unlock(p);
-- 
1.6.5.1

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

[PATCH 4/9] PM: suspend_block: Switch to list of active and inactive suspend blockers

[Arve Hjønnevåg]

This allows active suspend blockers to be listed

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 include/linux/suspend_blocker.h |    6 ++-
 kernel/power/suspend_blocker.c  |   85 +++++++++++++++++++++++++++-----------
 2 files changed, 65 insertions(+), 26 deletions(-)

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index 21689cd..1faa433 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -16,8 +16,11 @@
 #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.
  *
@@ -29,7 +32,8 @@
 
 struct suspend_blocker {
 #ifdef CONFIG_SUSPEND_BLOCKERS
-	atomic_t            flags;
+	struct list_head    link;
+	int                 flags;
 	const char         *name;
 #endif
 };
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 1b345d1..868e8f1 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -32,9 +32,11 @@ 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 atomic_t suspend_block_count;
-static atomic_t current_event_num;
+static LIST_HEAD(inactive_blockers);
+static LIST_HEAD(active_blockers);
+static int current_event_num;
 struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
@@ -52,6 +54,14 @@ static bool enable_suspend_blockers;
 			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
  *
@@ -65,7 +75,7 @@ bool suspend_is_blocked(void)
 {
 	if (!enable_suspend_blockers)
 		return 0;
-	return !!atomic_read(&suspend_block_count);
+	return !list_empty(&active_blockers);
 }
 
 static void suspend_worker(struct work_struct *work)
@@ -75,13 +85,13 @@ static void suspend_worker(struct work_struct *work)
 
 	enable_suspend_blockers = true;
 	while (!suspend_is_blocked()) {
-		entry_event_num = atomic_read(&current_event_num);
+		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 (atomic_read(&current_event_num) == entry_event_num)
+		if (current_event_num == entry_event_num)
 			pr_info("suspend: pm_suspend returned with no event\n");
 	}
 	enable_suspend_blockers = false;
@@ -98,13 +108,20 @@ static DECLARE_WORK(suspend_work, suspend_worker);
  */
 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;
-	atomic_set(&blocker->flags, SB_INITIALIZED);
+	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);
 
@@ -114,15 +131,17 @@ EXPORT_SYMBOL(suspend_blocker_init);
  */
 void suspend_blocker_destroy(struct suspend_blocker *blocker)
 {
-	int flags;
+	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);
-	flags = atomic_xchg(&blocker->flags, 0);
-	WARN(!(flags & SB_INITIALIZED), "suspend_blocker_destroy called on "
-					"uninitialized suspend_blocker\n");
-	if (flags == (SB_INITIALIZED | SB_ACTIVE))
-		if (atomic_dec_and_test(&suspend_block_count))
-			queue_work(suspend_work_queue, &suspend_work);
+	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(suspend_work_queue, &suspend_work);
+	spin_unlock_irqrestore(&list_lock, irqflags);
 }
 EXPORT_SYMBOL(suspend_blocker_destroy);
 
@@ -132,15 +151,20 @@ EXPORT_SYMBOL(suspend_blocker_destroy);
  */
 void suspend_block(struct suspend_blocker *blocker)
 {
-	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+	unsigned long irqflags;
+
+	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
+		return;
 
+	spin_lock_irqsave(&list_lock, irqflags);
+	blocker->flags |= SB_ACTIVE;
+	list_del(&blocker->link);
 	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
 		pr_info("suspend_block: %s\n", blocker->name);
-	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED,
-	    SB_INITIALIZED | SB_ACTIVE) == SB_INITIALIZED)
-		atomic_inc(&suspend_block_count);
+	list_add(&blocker->link, &active_blockers);
 
-	atomic_inc(&current_event_num);
+	current_event_num++;
+	spin_unlock_irqrestore(&list_lock, irqflags);
 }
 EXPORT_SYMBOL(suspend_block);
 
@@ -152,15 +176,26 @@ EXPORT_SYMBOL(suspend_block);
  */
 void suspend_unblock(struct suspend_blocker *blocker)
 {
-	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+	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_del(&blocker->link);
+	list_add(&blocker->link, &inactive_blockers);
 
-	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED | SB_ACTIVE,
-	    SB_INITIALIZED) == (SB_INITIALIZED | SB_ACTIVE))
-		if (atomic_dec_and_test(&suspend_block_count))
-			queue_work(suspend_work_queue, &suspend_work);
+	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
+		queue_work(suspend_work_queue, &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);
 
@@ -172,9 +207,9 @@ EXPORT_SYMBOL(suspend_unblock);
  */
 bool suspend_blocker_is_active(struct suspend_blocker *blocker)
 {
-	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+	WARN_ON(!(blocker->flags & SB_INITIALIZED));
 
-	return !!(atomic_read(&blocker->flags) & SB_ACTIVE);
+	return !!(blocker->flags & SB_ACTIVE);
 }
 EXPORT_SYMBOL(suspend_blocker_is_active);
 
-- 
1.6.5.1

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

[PATCH 5/9] PM: suspend_block: Add debugfs file

[Arve Hjønnevåg]

Report active and inactive suspend blockers in
/sys/kernel/debug/suspend_blockers.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 kernel/power/suspend_blocker.c |   44 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 868e8f1..047e910 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -17,6 +17,7 @@
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/suspend_blocker.h>
+#include <linux/debugfs.h>
 #include "power.h"
 
 enum {
@@ -41,6 +42,7 @@ struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
 static bool enable_suspend_blockers;
+static struct dentry *suspend_blocker_stats_dentry;
 
 #define pr_info_time(fmt, args...) \
 	do { \
@@ -54,6 +56,21 @@ static bool enable_suspend_blockers;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
+{
+	unsigned long irqflags;
+	struct suspend_blocker *blocker;
+
+	seq_puts(m, "name\tactive\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_for_each_entry(blocker, &inactive_blockers, link)
+		seq_printf(m, "\"%s\"\t0\n", blocker->name);
+	list_for_each_entry(blocker, &active_blockers, link)
+		seq_printf(m, "\"%s\"\t1\n", blocker->name);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return 0;
+}
+
 static void print_active_blockers_locked(void)
 {
 	struct suspend_blocker *blocker;
@@ -101,8 +118,8 @@ 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.
- *
+ * @name:	The name of the suspend blocker to show in debug messages and
+ *		/sys/kernel/debug/suspend_blockers.
  * The suspend blocker struct and name must not be freed before calling
  * suspend_blocker_destroy.
  */
@@ -240,6 +257,19 @@ int request_suspend_state(suspend_state_t state)
 	return 0;
 }
 
+static int suspend_blocker_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, suspend_blocker_stats_show, NULL);
+}
+
+static const struct file_operations suspend_blocker_stats_fops = {
+	.owner = THIS_MODULE,
+	.open = suspend_blocker_stats_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static int __init suspend_block_init(void)
 {
 	suspend_work_queue = create_singlethread_workqueue("suspend");
@@ -251,4 +281,14 @@ static int __init suspend_block_init(void)
 	return 0;
 }
 
+static int __init suspend_block_postcore_init(void)
+{
+	if (!suspend_work_queue)
+		return 0;
+	suspend_blocker_stats_dentry = debugfs_create_file("suspend_blockers",
+			S_IRUGO, NULL, NULL, &suspend_blocker_stats_fops);
+	return 0;
+}
+
 core_initcall(suspend_block_init);
+postcore_initcall(suspend_block_postcore_init);
-- 
1.6.5.1

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

[PATCH 6/9] PM: suspend_block: Add suspend_blocker stats

[Arve Hjønnevåg]

Report suspend block stats in /sys/kernel/debug/suspend_blockers.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 include/linux/suspend_blocker.h |   21 ++++-
 kernel/power/Kconfig            |    7 ++
 kernel/power/power.h            |    6 +-
 kernel/power/suspend.c          |    4 +-
 kernel/power/suspend_blocker.c  |  190 +++++++++++++++++++++++++++++++++++++--
 5 files changed, 218 insertions(+), 10 deletions(-)

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index 1faa433..3bb8a6a 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -17,12 +17,21 @@
 #define _LINUX_SUSPEND_BLOCKER_H
 
 #include <linux/list.h>
+#include <linux/ktime.h>
 
 /**
  * struct suspend_blocker - the basic suspend_blocker structure
  * @link:	List entry for active or inactive list.
- * @flags:	Tracks initialized and active state.
+ * @flags:	Tracks initialized, active and stats state.
  * @name:	Name used for debugging.
+ * @count:	Number of times this blocker has been deacivated
+ * @wakeup_count: Number of times this blocker was the first to block suspend
+ *		after resume.
+ * @total_time:	Total time this suspend blocker has prevented suspend.
+ * @prevent_suspend_time: Time this suspend blocker has prevented suspend while
+ *		user-space requested suspend.
+ * @max_time:	Max time this suspend blocker has been continuously active
+ * @last_time:	Monotonic clock when the active state last changed.
  *
  * When a suspend_blocker is active it prevents the system from entering
  * suspend.
@@ -35,6 +44,16 @@ struct suspend_blocker {
 	struct list_head    link;
 	int                 flags;
 	const char         *name;
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+	struct {
+		int             count;
+		int             wakeup_count;
+		ktime_t         total_time;
+		ktime_t         prevent_suspend_time;
+		ktime_t         max_time;
+		ktime_t         last_time;
+	} stat;
+#endif
 #endif
 };
 
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 1ac50ee..3fa2d33 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -141,6 +141,13 @@ config SUSPEND_BLOCKERS
 	  state through /sys/power/state, the requested sleep state will be
 	  entered when no suspend blockers are active.
 
+config SUSPEND_BLOCKER_STATS
+	bool "Suspend block stats"
+	depends on SUSPEND_BLOCKERS
+	default y
+	---help---
+	  Report suspend block stats in /sys/kernel/debug/suspend_blockers
+
 config USER_SUSPEND_BLOCKERS
 	bool "Userspace suspend blockers"
 	depends on SUSPEND_BLOCKERS
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 9b468d7..75b8849 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -240,4 +240,8 @@ static inline void suspend_thaw_processes(void)
 /* 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_SUSPEND_BLOCKER_STATS
+void about_to_enter_suspend(void);
+#else
+static inline void about_to_enter_suspend(void) {}
+#endif
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index dc42006..6d327ea 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -159,8 +159,10 @@ static int suspend_enter(suspend_state_t state)
 
 	error = sysdev_suspend(PMSG_SUSPEND);
 	if (!error) {
-		if (!suspend_is_blocked() && !suspend_test(TEST_CORE))
+		if (!suspend_is_blocked() && !suspend_test(TEST_CORE)) {
+			about_to_enter_suspend();
 			error = suspend_ops->enter(state);
+		}
 		sysdev_resume();
 	}
 
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 047e910..08d18c2 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -32,6 +32,7 @@ module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
 
 #define SB_INITIALIZED            (1U << 8)
 #define SB_ACTIVE                 (1U << 9)
+#define SB_PREVENTING_SUSPEND     (1U << 10)
 
 static DEFINE_SPINLOCK(list_lock);
 static DEFINE_SPINLOCK(state_lock);
@@ -42,6 +43,7 @@ struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
 static bool enable_suspend_blockers;
+static struct suspend_blocker unknown_wakeup;
 static struct dentry *suspend_blocker_stats_dentry;
 
 #define pr_info_time(fmt, args...) \
@@ -56,6 +58,153 @@ static struct dentry *suspend_blocker_stats_dentry;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+static struct suspend_blocker deleted_suspend_blockers;
+static ktime_t last_sleep_time_update;
+static bool wait_for_wakeup;
+
+static int print_blocker_stat(struct seq_file *m,
+			      struct suspend_blocker *blocker)
+{
+	int lock_count = blocker->stat.count;
+	ktime_t active_time = ktime_set(0, 0);
+	ktime_t total_time = blocker->stat.total_time;
+	ktime_t max_time = blocker->stat.max_time;
+	ktime_t prevent_suspend_time = blocker->stat.prevent_suspend_time;
+	if (blocker->flags & SB_ACTIVE) {
+		ktime_t now, add_time;
+		now = ktime_get();
+		add_time = ktime_sub(now, blocker->stat.last_time);
+		lock_count++;
+		active_time = add_time;
+		total_time = ktime_add(total_time, add_time);
+		if (blocker->flags & SB_PREVENTING_SUSPEND)
+			prevent_suspend_time = ktime_add(prevent_suspend_time,
+					ktime_sub(now, last_sleep_time_update));
+		if (add_time.tv64 > max_time.tv64)
+			max_time = add_time;
+	}
+
+	return seq_printf(m, "\"%s\"\t%d\t%d\t%lld\t%lld\t%lld\t%lld\t%lld\n",
+		       blocker->name, lock_count, blocker->stat.wakeup_count,
+		       ktime_to_ns(active_time), ktime_to_ns(total_time),
+		       ktime_to_ns(prevent_suspend_time), ktime_to_ns(max_time),
+		       ktime_to_ns(blocker->stat.last_time));
+}
+
+
+static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
+{
+	unsigned long irqflags;
+	struct suspend_blocker *blocker;
+
+	seq_puts(m, "name\tcount\twake_count\tactive_since"
+		 "\ttotal_time\tsleep_time\tmax_time\tlast_change\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_for_each_entry(blocker, &inactive_blockers, link)
+		print_blocker_stat(m, blocker);
+	list_for_each_entry(blocker, &active_blockers, link)
+		print_blocker_stat(m, blocker);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return 0;
+}
+
+static void suspend_blocker_stat_init_locked(struct suspend_blocker *blocker)
+{
+	blocker->stat.count = 0;
+	blocker->stat.wakeup_count = 0;
+	blocker->stat.total_time = ktime_set(0, 0);
+	blocker->stat.prevent_suspend_time = ktime_set(0, 0);
+	blocker->stat.max_time = ktime_set(0, 0);
+	blocker->stat.last_time = ktime_set(0, 0);
+}
+
+static void suspend_blocker_stat_destroy_locked(struct suspend_blocker *bl)
+{
+	if (!bl->stat.count)
+		return;
+	deleted_suspend_blockers.stat.count += bl->stat.count;
+	deleted_suspend_blockers.stat.total_time = ktime_add(
+		deleted_suspend_blockers.stat.total_time, bl->stat.total_time);
+	deleted_suspend_blockers.stat.prevent_suspend_time = ktime_add(
+		deleted_suspend_blockers.stat.prevent_suspend_time,
+		bl->stat.prevent_suspend_time);
+	deleted_suspend_blockers.stat.max_time = ktime_add(
+		deleted_suspend_blockers.stat.max_time, bl->stat.max_time);
+}
+
+static void suspend_unblock_stat_locked(struct suspend_blocker *blocker)
+{
+	ktime_t duration;
+	ktime_t now;
+	if (!(blocker->flags & SB_ACTIVE))
+		return;
+	now = ktime_get();
+	blocker->stat.count++;
+	duration = ktime_sub(now, blocker->stat.last_time);
+	blocker->stat.total_time =
+		ktime_add(blocker->stat.total_time, duration);
+	if (ktime_to_ns(duration) > ktime_to_ns(blocker->stat.max_time))
+		blocker->stat.max_time = duration;
+	blocker->stat.last_time = ktime_get();
+	if (blocker->flags & SB_PREVENTING_SUSPEND) {
+		duration = ktime_sub(now, last_sleep_time_update);
+		blocker->stat.prevent_suspend_time = ktime_add(
+			blocker->stat.prevent_suspend_time, duration);
+		blocker->flags &= ~SB_PREVENTING_SUSPEND;
+	}
+}
+
+static void suspend_block_stat_locked(struct suspend_blocker *blocker)
+{
+	if (wait_for_wakeup) {
+		if (debug_mask & DEBUG_WAKEUP)
+			pr_info("wakeup suspend blocker: %s\n", blocker->name);
+		wait_for_wakeup = false;
+		blocker->stat.wakeup_count++;
+	}
+	if (!(blocker->flags & SB_ACTIVE))
+		blocker->stat.last_time = ktime_get();
+}
+
+static void update_sleep_wait_stats_locked(bool done)
+{
+	struct suspend_blocker *blocker;
+	ktime_t now, elapsed, add;
+
+	now = ktime_get();
+	elapsed = ktime_sub(now, last_sleep_time_update);
+	list_for_each_entry(blocker, &active_blockers, link) {
+		if (blocker->flags & SB_PREVENTING_SUSPEND) {
+			add = elapsed;
+			blocker->stat.prevent_suspend_time = ktime_add(
+				blocker->stat.prevent_suspend_time, add);
+		}
+		if (done)
+			blocker->flags &= ~SB_PREVENTING_SUSPEND;
+		else
+			blocker->flags |= SB_PREVENTING_SUSPEND;
+	}
+	last_sleep_time_update = now;
+}
+
+void about_to_enter_suspend(void)
+{
+	wait_for_wakeup = true;
+}
+
+#else
+
+static inline void suspend_blocker_stat_init_locked(
+					struct suspend_blocker *blocker) {}
+static inline void suspend_blocker_stat_destroy_locked(
+					struct suspend_blocker *blocker) {}
+static inline void suspend_block_stat_locked(
+					struct suspend_blocker *blocker) {}
+static inline void suspend_unblock_stat_locked(
+					struct suspend_blocker *blocker) {}
+static inline void update_sleep_wait_stats_locked(bool done) {}
+
 static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
 {
 	unsigned long irqflags;
@@ -71,6 +220,8 @@ static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
 	return 0;
 }
 
+#endif
+
 static void print_active_blockers_locked(void)
 {
 	struct suspend_blocker *blocker;
@@ -101,16 +252,26 @@ static void suspend_worker(struct work_struct *work)
 	int entry_event_num;
 
 	enable_suspend_blockers = true;
-	while (!suspend_is_blocked()) {
-		entry_event_num = current_event_num;
+
+	if (suspend_is_blocked()) {
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: abort suspend\n");
+		goto abort;
+	}
+
+	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) {
 		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");
+		suspend_block(&unknown_wakeup);
+		suspend_unblock(&unknown_wakeup);
 	}
+abort:
 	enable_suspend_blockers = false;
 }
 static DECLARE_WORK(suspend_work, suspend_worker);
@@ -137,6 +298,7 @@ void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)
 	INIT_LIST_HEAD(&blocker->link);
 
 	spin_lock_irqsave(&list_lock, irqflags);
+	suspend_blocker_stat_init_locked(blocker);
 	list_add(&blocker->link, &inactive_blockers);
 	spin_unlock_irqrestore(&list_lock, irqflags);
 }
@@ -154,6 +316,7 @@ void suspend_blocker_destroy(struct suspend_blocker *blocker)
 	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
 		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
 	spin_lock_irqsave(&list_lock, irqflags);
+	suspend_blocker_stat_destroy_locked(blocker);
 	blocker->flags &= ~SB_INITIALIZED;
 	list_del(&blocker->link);
 	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
@@ -174,6 +337,7 @@ void suspend_block(struct suspend_blocker *blocker)
 		return;
 
 	spin_lock_irqsave(&list_lock, irqflags);
+	suspend_block_stat_locked(blocker);
 	blocker->flags |= SB_ACTIVE;
 	list_del(&blocker->link);
 	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
@@ -181,6 +345,10 @@ void suspend_block(struct suspend_blocker *blocker)
 	list_add(&blocker->link, &active_blockers);
 
 	current_event_num++;
+	if (blocker == &main_suspend_blocker)
+		update_sleep_wait_stats_locked(true);
+	else if (!suspend_blocker_is_active(&main_suspend_blocker))
+		update_sleep_wait_stats_locked(false);
 	spin_unlock_irqrestore(&list_lock, irqflags);
 }
 EXPORT_SYMBOL(suspend_block);
@@ -200,6 +368,8 @@ void suspend_unblock(struct suspend_blocker *blocker)
 
 	spin_lock_irqsave(&list_lock, irqflags);
 
+	suspend_unblock_stat_locked(blocker);
+
 	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
 		pr_info("suspend_unblock: %s\n", blocker->name);
 	list_del(&blocker->link);
@@ -211,6 +381,7 @@ void suspend_unblock(struct suspend_blocker *blocker)
 	if (blocker == &main_suspend_blocker) {
 		if (debug_mask & DEBUG_SUSPEND)
 			print_active_blockers_locked();
+		update_sleep_wait_stats_locked(false);
 	}
 	spin_unlock_irqrestore(&list_lock, irqflags);
 }
@@ -278,6 +449,11 @@ static int __init suspend_block_init(void)
 
 	suspend_blocker_init(&main_suspend_blocker, "main");
 	suspend_block(&main_suspend_blocker);
+	suspend_blocker_init(&unknown_wakeup, "unknown_wakeups");
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+	suspend_blocker_init(&deleted_suspend_blockers,
+				"deleted_suspend_blockers");
+#endif
 	return 0;
 }
 
-- 
1.6.5.1

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

[PATCH 7/9] PM: Add suspend blocking work.

[Arve Hjønnevåg]

Allow work to be queued that will block suspend while it is pending
or executing. To get the same functionality in the calling code often
requires a separate suspend_blocker for pending and executing work, or
additional state and locking.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 include/linux/workqueue.h |   45 ++++++++++++++++++++++++++++++++++++++++++++-
 kernel/workqueue.c        |   19 +++++++++++++++++++
 2 files changed, 63 insertions(+), 1 deletions(-)

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 9466e86..9c5a078 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -9,6 +9,7 @@
 #include <linux/linkage.h>
 #include <linux/bitops.h>
 #include <linux/lockdep.h>
+#include <linux/suspend_blocker.h>
 #include <asm/atomic.h>
 
 struct workqueue_struct;
@@ -26,7 +27,8 @@ struct work_struct {
 	atomic_long_t data;
 #define WORK_STRUCT_PENDING 0		/* T if work item pending execution */
 #define WORK_STRUCT_STATIC  1		/* static initializer (debugobjects) */
-#define WORK_STRUCT_FLAG_MASK (3UL)
+#define WORK_STRUCT_SUSPEND_BLOCKING  2	/* suspend blocking work */
+#define WORK_STRUCT_FLAG_MASK (7UL)
 #define WORK_STRUCT_WQ_DATA_MASK (~WORK_STRUCT_FLAG_MASK)
 	struct list_head entry;
 	work_func_t func;
@@ -48,6 +50,23 @@ static inline struct delayed_work *to_delayed_work(struct work_struct *work)
 	return container_of(work, struct delayed_work, work);
 }
 
+struct suspend_blocking_work {
+	struct work_struct work;
+	struct suspend_blocker suspend_blocker;
+};
+
+static inline struct suspend_blocking_work *
+to_suspend_blocking_work(struct work_struct *work)
+{
+	return container_of(work, struct suspend_blocking_work, work);
+}
+
+static inline struct suspend_blocker *
+work_to_suspend_blocker(struct work_struct *work)
+{
+	return &to_suspend_blocking_work(work)->suspend_blocker;
+}
+
 struct execute_work {
 	struct work_struct work;
 };
@@ -157,6 +176,30 @@ static inline void destroy_work_on_stack(struct work_struct *work) { }
 		init_timer_deferrable(&(_work)->timer);		\
 	} while (0)
 
+static inline void suspend_blocking_work_init(
+	struct suspend_blocking_work *work, work_func_t func, const char *name)
+{
+	INIT_WORK(&work->work, func);
+	suspend_blocker_init(&work->suspend_blocker, name);
+	set_bit(WORK_STRUCT_SUSPEND_BLOCKING, work_data_bits(&work->work));
+}
+
+/**
+ * suspend_blocking_work_destroy - Destroy suspend_blocking_work
+ * @work: The work item in question
+ *
+ * If the work was ever queued on more then one workqueue or on a multithreaded
+ * workqueue all these workqueues must be flushed before calling
+ * suspend_blocking_work_destroy. If only a single singlethreaded workqueue
+ * was used flush_work or cancel_work_sync can be used instead.
+ */
+
+static inline void
+suspend_blocking_work_destroy(struct suspend_blocking_work *work)
+{
+	suspend_blocker_destroy(&work->suspend_blocker);
+}
+
 /**
  * work_pending - Find out whether a work item is currently pending
  * @work: The work item in question
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index dee4865..b2aba90 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -68,6 +68,15 @@ struct workqueue_struct {
 #endif
 };
 
+static inline int work_blocks_suspend(struct work_struct *work)
+{
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	return test_bit(WORK_STRUCT_SUSPEND_BLOCKING, work_data_bits(work));
+#else
+	return false;
+#endif
+}
+
 #ifdef CONFIG_DEBUG_OBJECTS_WORK
 
 static struct debug_obj_descr work_debug_descr;
@@ -257,6 +266,10 @@ static void __queue_work(struct cpu_workqueue_struct *cwq,
 
 	debug_work_activate(work);
 	spin_lock_irqsave(&cwq->lock, flags);
+
+	if (work_blocks_suspend(work))
+		suspend_block(work_to_suspend_blocker(work));
+
 	insert_work(cwq, work, &cwq->worklist);
 	spin_unlock_irqrestore(&cwq->lock, flags);
 }
@@ -379,6 +392,7 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
 		struct work_struct *work = list_entry(cwq->worklist.next,
 						struct work_struct, entry);
 		work_func_t f = work->func;
+		bool current_work_blocks_suspend = work_blocks_suspend(work);
 #ifdef CONFIG_LOCKDEP
 		/*
 		 * It is permissible to free the struct work_struct
@@ -416,6 +430,9 @@ static void run_workqueue(struct cpu_workqueue_struct *cwq)
 		}
 
 		spin_lock_irq(&cwq->lock);
+		if (current_work_blocks_suspend &&
+		    (get_wq_data(work) == cwq) && !work_pending(work))
+			suspend_unblock(work_to_suspend_blocker(work));
 		cwq->current_work = NULL;
 	}
 	spin_unlock_irq(&cwq->lock);
@@ -671,6 +688,8 @@ static int __cancel_work_timer(struct work_struct *work,
 		wait_on_work(work);
 	} while (unlikely(ret < 0));
 
+	if (work_blocks_suspend(work))
+		suspend_unblock(work_to_suspend_blocker(work));
 	work_clear_pending(work);
 	return ret;
 }
-- 
1.6.5.1

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

[PATCH 8/9] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will block
suspend while the event queue is not empty. This allows userspace code to
process input events while the device appears to be asleep.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/input/evdev.c |   22 ++++++++++++++++++++++
 include/linux/input.h |    3 +++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..66e0d16 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,6 +20,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/suspend_blocker.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -43,6 +44,8 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	struct suspend_blocker suspend_blocker;
+	bool use_suspend_blocker;
 };
 
 static struct evdev *evdev_table[EVDEV_MINORS];
@@ -55,6 +58,8 @@ static void evdev_pass_event(struct evdev_client *client,
 	 * Interrupts are disabled, just acquire the lock
 	 */
 	spin_lock(&client->buffer_lock);
+	if (client->use_suspend_blocker)
+		suspend_block(&client->suspend_blocker);
 	client->buffer[client->head++] = *event;
 	client->head &= EVDEV_BUFFER_SIZE - 1;
 	spin_unlock(&client->buffer_lock);
@@ -234,6 +239,8 @@ static int evdev_release(struct inode *inode, struct file *file)
 	mutex_unlock(&evdev->mutex);
 
 	evdev_detach_client(evdev, client);
+	if (client->use_suspend_blocker)
+		suspend_blocker_destroy(&client->suspend_blocker);
 	kfree(client);
 
 	evdev_close_device(evdev);
@@ -335,6 +342,8 @@ static int evdev_fetch_next_event(struct evdev_client *client,
 	if (have_event) {
 		*event = client->buffer[client->tail++];
 		client->tail &= EVDEV_BUFFER_SIZE - 1;
+		if (client->use_suspend_blocker && client->head == client->tail)
+			suspend_unblock(&client->suspend_blocker);
 	}
 
 	spin_unlock_irq(&client->buffer_lock);
@@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCGSUSPENDBLOCK:
+		return put_user(client->use_suspend_blocker, ip);
+
+	case EVIOCSSUSPENDBLOCK:
+		spin_lock_irq(&client->buffer_lock);
+		if (!client->use_suspend_blocker && p)
+			suspend_blocker_init(&client->suspend_blocker, "evdev");
+		else if (client->use_suspend_blocker && !p)
+			suspend_blocker_destroy(&client->suspend_blocker);
+		client->use_suspend_blocker = !!p;
+		spin_unlock_irq(&client->buffer_lock);
+		return 0;
+
 	default:
 
 		if (_IOC_TYPE(cmd) != 'E')
diff --git a/include/linux/input.h b/include/linux/input.h
index 7ed2251..b2d93b4 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -82,6 +82,9 @@ struct input_absinfo {
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
 
+#define EVIOCGSUSPENDBLOCK	_IOR('E', 0x91, int)			/* get suspend block enable */
+#define EVIOCSSUSPENDBLOCK	_IOW('E', 0x91, int)			/* set suspend block enable */
+
 /*
  * Event types
  */
-- 
1.6.5.1

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

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Tejun Heo]

Hello,

On 04/23/2010 03:08 AM, Arve Hjønnevåg wrote:
> Allow work to be queued that will block suspend while it is pending
> or executing. To get the same functionality in the calling code often
> requires a separate suspend_blocker for pending and executing work, or
> additional state and locking.

Hmm... I think this can be implemented as pure wrapper around
workqueue instead of injecting a flag and code into workqueue core.
Adding @fn field to suspend_blocking_work struct and using a custom
work function to call it and then invoke suspend_unblock() should be
enough, right?  Oh, dedicated queue functions will be needed too.  I
don't think it's wise to meddle with workqueue core code for this.

Thanks.

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

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Oleg Nesterov]

On 04/23, Tejun Heo wrote:
>
> On 04/23/2010 03:08 AM, Arve Hjønnevåg wrote:
> > Allow work to be queued that will block suspend while it is pending
> > or executing. To get the same functionality in the calling code often
> > requires a separate suspend_blocker for pending and executing work, or
> > additional state and locking.
>
> Hmm... I think this can be implemented as pure wrapper around
> workqueue instead of injecting a flag and code into workqueue core.
> Adding @fn field to suspend_blocking_work struct and using a custom
> work function to call it and then invoke suspend_unblock() should be
> enough, right?  Oh, dedicated queue functions will be needed too.  I
> don't think it's wise to meddle with workqueue core code for this.

Completely agreed. The patch adds very "strange" hacks into workqueue
code to solve the very specific problems.


Besides, the patch doesn't look right. suspend_unblock() can be called
twice if you use cancel_work(). Perhaps this is not a problem, I dunno.
WORK_STRUCT_SUSPEND_BLOCKING needs to ensure that cpu_workqueue_struct
has a proper alignment. The unblock code in run_workqueue() is racy,
it can unblock after the work was queued on another CPU, cwq->lock can't
help.

Oleg.

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Arve Hjønnevåg]

On Fri, Apr 23, 2010 at 5:20 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 04/23, Tejun Heo wrote:
>>
>> On 04/23/2010 03:08 AM, Arve Hjønnevåg wrote:
>> > Allow work to be queued that will block suspend while it is pending
>> > or executing. To get the same functionality in the calling code often
>> > requires a separate suspend_blocker for pending and executing work, or
>> > additional state and locking.
>>
>> Hmm... I think this can be implemented as pure wrapper around
>> workqueue instead of injecting a flag and code into workqueue core.
>> Adding @fn field to suspend_blocking_work struct and using a custom
>> work function to call it and then invoke suspend_unblock() should be
>> enough, right?  Oh, dedicated queue functions will be needed too.  I
>> don't think it's wise to meddle with workqueue core code for this.
>
> Completely agreed. The patch adds very "strange" hacks into workqueue
> code to solve the very specific problems.
>

I want the suspend blocker active when the work is pending or running.
I did not see a way to do this on top of the workqueue api without
adding additional locking.

>
> Besides, the patch doesn't look right. suspend_unblock() can be called
> twice if you use cancel_work(). Perhaps this is not a problem, I dunno.

Calling suspend_unblock() twice is not a problem as long as
"unblocked" is the expected final state.

> WORK_STRUCT_SUSPEND_BLOCKING needs to ensure that cpu_workqueue_struct
> has a proper alignment.

OK.

> The unblock code in run_workqueue() is racy,
> it can unblock after the work was queued on another CPU, cwq->lock can't
> help.

If the work is both queued and starts running on another workqueue
between "get_wq_data(work) == cwq" and "!work_pending(work)", then
suspend_unblock will be called when it shouldn't. It should work fine
if I change to it check pending first though, since it cannot move
back to the current workqueue without locking cwq->lock first.

Or are you talking about the race when the callback is running on
multiple (cpu) workqueues at the same time. In that case the suspend
blocker is released when the callback returns from the last workqueue
is was queued on, not when all the callbacks have returned. On that
note, is it ever safe to use flush_work and cancel_work_sync for work
queues on anything other than a single single threaded workqueue?

-- 
Arve Hjønnevåg

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Arve Hjønnevåg]

2010/4/23 Arve Hjønnevåg <arve@android.com>:
> On Fri, Apr 23, 2010 at 5:20 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> On 04/23, Tejun Heo wrote:
>>>
>>> On 04/23/2010 03:08 AM, Arve Hjønnevåg wrote:
>>> > Allow work to be queued that will block suspend while it is pending
>>> > or executing. To get the same functionality in the calling code often
>>> > requires a separate suspend_blocker for pending and executing work, or
>>> > additional state and locking.
>>>
>>> Hmm... I think this can be implemented as pure wrapper around
>>> workqueue instead of injecting a flag and code into workqueue core.
>>> Adding @fn field to suspend_blocking_work struct and using a custom
>>> work function to call it and then invoke suspend_unblock() should be
>>> enough, right?  Oh, dedicated queue functions will be needed too.  I
>>> don't think it's wise to meddle with workqueue core code for this.
>>
>> Completely agreed. The patch adds very "strange" hacks into workqueue
>> code to solve the very specific problems.
>>
>
> I want the suspend blocker active when the work is pending or running.
> I did not see a way to do this on top of the workqueue api without
> adding additional locking.
>

I can remove the WORK_STRUCT_SUSPEND_BLOCKING flag and instead add an
argument to __queue_work and __cancel_work_timer (or use separate
copies) if you think that would be better. It would avoid increasing
the alignment requirement of cpu_workqueue_struct, but it will
probably add more code than the current patch.

-- 
Arve Hjønnevåg

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Tejun Heo]

Hello,

On 04/24/2010 12:49 AM, Arve Hjønnevåg wrote:
> I want the suspend blocker active when the work is pending or running.
> I did not see a way to do this on top of the workqueue api without
> adding additional locking.

Well, then add additional locking.  suspend_blocker is far away from
being a hot path and there's nothing wrong with additional locking as
long as everything is wrapped inside proper APIs.  Adding stuff to the
core code for things as specific as this is much worse.

> If the work is both queued and starts running on another workqueue
> between "get_wq_data(work) == cwq" and "!work_pending(work)", then
> suspend_unblock will be called when it shouldn't. It should work fine
> if I change to it check pending first though, since it cannot move
> back to the current workqueue without locking cwq->lock first.

The code is more fundamentally broken.  Once work->func has started
executing, the work pointer can't be dereferenced as work callback is
allowed to free or re-purpose the work data structure and in the same
manner you can't check for pending status after execution has
completed.

> Or are you talking about the race when the callback is running on
> multiple (cpu) workqueues at the same time. In that case the suspend
> blocker is released when the callback returns from the last workqueue
> is was queued on, not when all the callbacks have returned. On that
> note, is it ever safe to use flush_work and cancel_work_sync for work
> queues on anything other than a single single threaded workqueue?

flush_work() is guaranteed only to flush from the last queued cpu but
cancel_work_sync() will guarantee that no cpu is executing or holding
onto the work.  So, yeah, as long as the limitations of flush_work()
is understood, it's safe.

Going back to the original subject, just add simplistic outside
locking in suspend_blocker_work API (or whatever other name you
prefer).  That will be much cleaner and safer.  Let's think about
moving them into workqueue implementation proper after the number of
the API users grows to hundreds.

Thanks.

-- 
tejun

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Arve Hjønnevåg]

2010/4/23 Tejun Heo <tj@kernel.org>:
> Hello,
>
> On 04/24/2010 12:49 AM, Arve Hjønnevåg wrote:
>> I want the suspend blocker active when the work is pending or running.
>> I did not see a way to do this on top of the workqueue api without
>> adding additional locking.
>
> Well, then add additional locking.  suspend_blocker is far away from
> being a hot path and there's nothing wrong with additional locking as
> long as everything is wrapped inside proper APIs.  Adding stuff to the
> core code for things as specific as this is much worse.
>

OK, I'll try to do this. Do you want it in a separate header file as well?

>> If the work is both queued and starts running on another workqueue
>> between "get_wq_data(work) == cwq" and "!work_pending(work)", then
>> suspend_unblock will be called when it shouldn't. It should work fine
>> if I change to it check pending first though, since it cannot move
>> back to the current workqueue without locking cwq->lock first.
>
> The code is more fundamentally broken.  Once work->func has started
> executing, the work pointer can't be dereferenced as work callback is
> allowed to free or re-purpose the work data structure and in the same
> manner you can't check for pending status after execution has
> completed.
>

I only touch the work structure after the callback has returned for
suspend blocking work, which does not allow that.

>> Or are you talking about the race when the callback is running on
>> multiple (cpu) workqueues at the same time. In that case the suspend
>> blocker is released when the callback returns from the last workqueue
>> is was queued on, not when all the callbacks have returned. On that
>> note, is it ever safe to use flush_work and cancel_work_sync for work
>> queues on anything other than a single single threaded workqueue?
>
> flush_work() is guaranteed only to flush from the last queued cpu but
> cancel_work_sync() will guarantee that no cpu is executing or holding
> onto the work.  So, yeah, as long as the limitations of flush_work()
> is understood, it's safe.
>

Sorry, I didn't see the for_each_cpu(cpu, cpu_map) part of
wait_on_work(). cancel_work_sync() does look safe as long as the work
has not moved to completely different workqueue.

> Going back to the original subject, just add simplistic outside
> locking in suspend_blocker_work API (or whatever other name you
> prefer).  That will be much cleaner and safer.  Let's think about
> moving them into workqueue implementation proper after the number of
> the API users grows to hundreds.
>

OK.

-- 
Arve Hjønnevåg

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Tejun Heo]

Hello,

On 04/24/2010 09:21 AM, Arve Hjønnevåg wrote:
> OK, I'll try to do this. Do you want it in a separate header file as well?

I think it would fit better with the rest of suspend_blocker.h APIs
but it's not like I thought about it hard and long, so please take my
preference with a grain of salt.

> I only touch the work structure after the callback has returned for
> suspend blocking work, which does not allow that.

Oh, you're right.  I missed the current_work_blocks_suspend test.
Sorry about that.

Thanks.

-- 
tejun

Re: [PATCH 7/9] PM: Add suspend blocking work.

[Oleg Nesterov]

On 04/23, Arve Hjønnevåg wrote:
>
> On Fri, Apr 23, 2010 at 5:20 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>
> > The unblock code in run_workqueue() is racy,
> > it can unblock after the work was queued on another CPU, cwq->lock can't
> > help.
>
> If the work is both queued and starts running on another workqueue
> between "get_wq_data(work) == cwq" and "!work_pending(work)", then
> suspend_unblock will be called when it shouldn't. It should work fine
> if I change to it check pending first though, since it cannot move
> back to the current workqueue without locking cwq->lock first.

It can be queued on the same or different workqueue on another CPU,
right before run_workqueue() does unblock.

But I guess this doesn't matter. You already discussed this all with
Tejun while I was sleeping ;) And I agree very much, the new plan looks
much better imho.

Oleg.

[PATCH 9/9] power_supply: Block suspend while power supply change notifications are pending

[Arve Hjønnevåg]

When connecting usb or the charger the device would often go back to sleep
before the charge led and screen turned on.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 drivers/power/power_supply_core.c |    9 ++++++---
 include/linux/power_supply.h      |    2 +-
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/power/power_supply_core.c b/drivers/power/power_supply_core.c
index cce75b4..fdf30ba 100644
--- a/drivers/power/power_supply_core.c
+++ b/drivers/power/power_supply_core.c
@@ -39,7 +39,7 @@ static int __power_supply_changed_work(struct device *dev, void *data)
 static void power_supply_changed_work(struct work_struct *work)
 {
 	struct power_supply *psy = container_of(work, struct power_supply,
-						changed_work);
+						changed_work.work);
 
 	dev_dbg(psy->dev, "%s\n", __func__);
 
@@ -55,7 +55,7 @@ void power_supply_changed(struct power_supply *psy)
 {
 	dev_dbg(psy->dev, "%s\n", __func__);
 
-	schedule_work(&psy->changed_work);
+	schedule_work(&psy->changed_work.work);
 }
 EXPORT_SYMBOL_GPL(power_supply_changed);
 
@@ -155,7 +155,8 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 		goto dev_create_failed;
 	}
 
-	INIT_WORK(&psy->changed_work, power_supply_changed_work);
+	suspend_blocking_work_init(&psy->changed_work,
+				power_supply_changed_work, "power-supply");
 
 	rc = power_supply_create_attrs(psy);
 	if (rc)
@@ -172,6 +173,7 @@ int power_supply_register(struct device *parent, struct power_supply *psy)
 create_triggers_failed:
 	power_supply_remove_attrs(psy);
 create_attrs_failed:
+	suspend_blocking_work_destroy(&psy->changed_work);
 	device_unregister(psy->dev);
 dev_create_failed:
 success:
@@ -184,6 +186,7 @@ void power_supply_unregister(struct power_supply *psy)
 	flush_scheduled_work();
 	power_supply_remove_triggers(psy);
 	power_supply_remove_attrs(psy);
+	suspend_blocking_work_destroy(&psy->changed_work);
 	device_unregister(psy->dev);
 }
 EXPORT_SYMBOL_GPL(power_supply_unregister);
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index ebd2b8f..2232a9d 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -152,7 +152,7 @@ struct power_supply {
 
 	/* private */
 	struct device *dev;
-	struct work_struct changed_work;
+	struct suspend_blocking_work changed_work;
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	struct led_trigger *charging_full_trig;
-- 
1.6.5.1

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

Re: [PATCH 8/9] Input: Block suspend while event queue is not empty.

[Randy Dunlap]

On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:

> Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will block
> suspend while the event queue is not empty. This allows userspace code to
> process input events while the device appears to be asleep.


All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.

> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  drivers/input/evdev.c |   22 ++++++++++++++++++++++
>  include/linux/input.h |    3 +++
>  2 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index 2ee6c7a..66e0d16 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  		else
>  			return evdev_ungrab(evdev, client);
>  
> +	case EVIOCGSUSPENDBLOCK:
> +		return put_user(client->use_suspend_blocker, ip);
> +
> +	case EVIOCSSUSPENDBLOCK:
> +		spin_lock_irq(&client->buffer_lock);
> +		if (!client->use_suspend_blocker && p)
> +			suspend_blocker_init(&client->suspend_blocker, "evdev");
> +		else if (client->use_suspend_blocker && !p)
> +			suspend_blocker_destroy(&client->suspend_blocker);
> +		client->use_suspend_blocker = !!p;
> +		spin_unlock_irq(&client->buffer_lock);
> +		return 0;
> +
>  	default:
>  
>  		if (_IOC_TYPE(cmd) != 'E')
> diff --git a/include/linux/input.h b/include/linux/input.h
> index 7ed2251..b2d93b4 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -82,6 +82,9 @@ struct input_absinfo {
>  
>  #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
>  
> +#define EVIOCGSUSPENDBLOCK	_IOR('E', 0x91, int)			/* get suspend block enable */
> +#define EVIOCSSUSPENDBLOCK	_IOW('E', 0x91, int)			/* set suspend block enable */
> +
>  /*
>   * Event types
>   */
> -- 


thanks,
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 8/9] Input: Block suspend while event queue is not empty.

[Dmitry Torokhov]

On Friday 23 April 2010 01:56:25 pm Randy Dunlap wrote:
> On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:
> > Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will
> > block suspend while the event queue is not empty. This allows userspace
> > code to process input events while the device appears to be asleep.
> 
> All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt,
> please.

I do not see the reason for it to be in the kernel still. Have a process
that listens to all input devices (or subset of them), once events stop
coming initiate suspend.

> 
> > Signed-off-by: Arve Hjønnevåg <arve@android.com>
> > ---
> > 
> >  drivers/input/evdev.c |   22 ++++++++++++++++++++++
> >  include/linux/input.h |    3 +++
> >  2 files changed, 25 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> > index 2ee6c7a..66e0d16 100644
> > --- a/drivers/input/evdev.c
> > +++ b/drivers/input/evdev.c
> > @@ -585,6 +594,19 @@ static long evdev_do_ioctl(struct file *file,
> > unsigned int cmd,
> > 
> >  		else
> >  		
> >  			return evdev_ungrab(evdev, client);
> > 
> > +	case EVIOCGSUSPENDBLOCK:
> > +		return put_user(client->use_suspend_blocker, ip);
> > +
> > +	case EVIOCSSUSPENDBLOCK:
> > +		spin_lock_irq(&client->buffer_lock);
> > +		if (!client->use_suspend_blocker && p)
> > +			suspend_blocker_init(&client->suspend_blocker, "evdev");
> > +		else if (client->use_suspend_blocker && !p)
> > +			suspend_blocker_destroy(&client->suspend_blocker);
> > +		client->use_suspend_blocker = !!p;
> > +		spin_unlock_irq(&client->buffer_lock);
> > +		return 0;
> > +
> > 
> >  	default:
> >  		if (_IOC_TYPE(cmd) != 'E')
> > 
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index 7ed2251..b2d93b4 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -82,6 +82,9 @@ struct input_absinfo {
> > 
> >  #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release 
device */
> > 
> > +#define EVIOCGSUSPENDBLOCK	_IOR('E', 0x91, int)			/* get 
suspend block
> > enable */ +#define EVIOCSSUSPENDBLOCK	_IOW('E', 0x91, int)			/* 
set
> > suspend block enable */ +
> > 
> >  /*
> >  
> >   * Event types
> >   */
> 
> thanks,
> ---
> ~Randy
> *** Remember to use Documentation/SubmitChecklist when testing your code
> *** --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Dmitry

Re: [PATCH 8/9] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

2010/4/23 Randy Dunlap <randy.dunlap@oracle.com>:
> On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:
>
>> Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will block
>> suspend while the event queue is not empty. This allows userspace code to
>> process input events while the device appears to be asleep.
>
>
> All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt, please.
>

These two appear to already be covered by "'E'     all
linux/input.h           conflict!", but I will add a line the user
space suspend blocker ioctls added earlier in this patch set.

-- 
Arve Hjønnevåg

Re: [PATCH 8/9] Input: Block suspend while event queue is not empty.

[Arve Hjønnevåg]

On Fri, Apr 23, 2010 at 2:08 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Friday 23 April 2010 01:56:25 pm Randy Dunlap wrote:
>> On Thu, 22 Apr 2010 18:08:57 -0700 Arve Hjønnevåg wrote:
>> > Add an ioctl, EVIOCSSUSPENDBLOCK, to enable a suspend_blocker that will
>> > block suspend while the event queue is not empty. This allows userspace
>> > code to process input events while the device appears to be asleep.
>>
>> All new ioctls need to be added to Documentation/ioctl/ioctl-number.txt,
>> please.
>
> I do not see the reason for it to be in the kernel still. Have a process
> that listens to all input devices (or subset of them), once events stop
> coming initiate suspend.
>

I think the document added by the first patch explains this. The
solution you propose above will ignore a wakeup key pressed right
after user space decides to initiate suspend.

-- 
Arve Hjønnevåg

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:

> Report active and inactive suspend blockers in
> /sys/kernel/debug/suspend_blockers.


This looks more like user interface information than it does debug information.
If that's correct, it shouldn't be in debugfs.


> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  kernel/power/suspend_blocker.c |   44 ++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
> index 868e8f1..047e910 100644
> --- a/kernel/power/suspend_blocker.c
> +++ b/kernel/power/suspend_blocker.c
> @@ -17,6 +17,7 @@
>  #include <linux/rtc.h>
>  #include <linux/suspend.h>
>  #include <linux/suspend_blocker.h>
> +#include <linux/debugfs.h>
>  #include "power.h"
>  
>  enum {
> @@ -41,6 +42,7 @@ struct workqueue_struct *suspend_work_queue;
>  struct suspend_blocker main_suspend_blocker;
>  static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
>  static bool enable_suspend_blockers;
> +static struct dentry *suspend_blocker_stats_dentry;
>  
>  #define pr_info_time(fmt, args...) \
>  	do { \
> @@ -54,6 +56,21 @@ static bool enable_suspend_blockers;
>  			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
>  	} while (0);
>  
> +static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
> +{
> +	unsigned long irqflags;
> +	struct suspend_blocker *blocker;
> +
> +	seq_puts(m, "name\tactive\n");
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_for_each_entry(blocker, &inactive_blockers, link)
> +		seq_printf(m, "\"%s\"\t0\n", blocker->name);
> +	list_for_each_entry(blocker, &active_blockers, link)
> +		seq_printf(m, "\"%s\"\t1\n", blocker->name);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +	return 0;
> +}
> +
>  static void print_active_blockers_locked(void)
>  {
>  	struct suspend_blocker *blocker;
> @@ -101,8 +118,8 @@ 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.
> - *
> + * @name:	The name of the suspend blocker to show in debug messages and
> + *		/sys/kernel/debug/suspend_blockers.
>   * The suspend blocker struct and name must not be freed before calling
>   * suspend_blocker_destroy.
>   */
> @@ -240,6 +257,19 @@ int request_suspend_state(suspend_state_t state)
>  	return 0;
>  }
>  
> +static int suspend_blocker_stats_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, suspend_blocker_stats_show, NULL);
> +}
> +
> +static const struct file_operations suspend_blocker_stats_fops = {
> +	.owner = THIS_MODULE,
> +	.open = suspend_blocker_stats_open,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
>  static int __init suspend_block_init(void)
>  {
>  	suspend_work_queue = create_singlethread_workqueue("suspend");
> @@ -251,4 +281,14 @@ static int __init suspend_block_init(void)
>  	return 0;
>  }
>  
> +static int __init suspend_block_postcore_init(void)
> +{
> +	if (!suspend_work_queue)
> +		return 0;
> +	suspend_blocker_stats_dentry = debugfs_create_file("suspend_blockers",
> +			S_IRUGO, NULL, NULL, &suspend_blocker_stats_fops);
> +	return 0;
> +}
> +
>  core_initcall(suspend_block_init);
> +postcore_initcall(suspend_block_postcore_init);
> -- 


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Arve Hjønnevåg]

2010/4/23 Randy Dunlap <randy.dunlap@oracle.com>:
> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:
>
>> Report active and inactive suspend blockers in
>> /sys/kernel/debug/suspend_blockers.
>
>
> This looks more like user interface information than it does debug information.
> If that's correct, it shouldn't be in debugfs.

Where should it be? It is not allowed in sysfs, and procfs does not
appear to be open to new files.

-- 
Arve Hjønnevåg

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On 04/23/10 20:23, Arve Hjønnevåg wrote:
> 2010/4/23 Randy Dunlap <randy.dunlap@oracle.com>:
>> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:
>>
>>> Report active and inactive suspend blockers in
>>> /sys/kernel/debug/suspend_blockers.
>>
>>
>> This looks more like user interface information than it does debug information.
>> If that's correct, it shouldn't be in debugfs.
> 
> Where should it be? It is not allowed in sysfs, and procfs does not
> appear to be open to new files.

Why is it not allowed in sysfs?  due to more than one value per file
or something else?

I don't think that procfs is closed to new files, but you may have to
justify why it should be used.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Arve Hjønnevåg]

2010/4/23 Randy Dunlap <randy.dunlap@oracle.com>:
> On 04/23/10 20:23, Arve Hjønnevåg wrote:
>> 2010/4/23 Randy Dunlap <randy.dunlap@oracle.com>:
>>> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hjønnevåg wrote:
>>>
>>>> Report active and inactive suspend blockers in
>>>> /sys/kernel/debug/suspend_blockers.
>>>
>>>
>>> This looks more like user interface information than it does debug information.
>>> If that's correct, it shouldn't be in debugfs.
>>
>> Where should it be? It is not allowed in sysfs, and procfs does not
>> appear to be open to new files.
>
> Why is it not allowed in sysfs?  due to more than one value per file

Yes.

> or something else?
>
> I don't think that procfs is closed to new files, but you may have to
> justify why it should be used.
>

I used procfs in the original version and there were a lot of
objections to this, so I moved it to debugfs.

-- 
Arve Hjønnevåg

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Greg KH]

On Fri, Apr 23, 2010 at 01:58:53PM -0700, Randy Dunlap wrote:
> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:
> 
> > Report active and inactive suspend blockers in
> > /sys/kernel/debug/suspend_blockers.
> 
> 
> This looks more like user interface information than it does debug information.
> If that's correct, it shouldn't be in debugfs.

It's debug-like information, and has more than one value per file, so
debugfs seems like the proper place for it.  I have no objection to it
going there.

thanks,

greg k-h

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On 04/25/10 11:15, Greg KH wrote:
> On Fri, Apr 23, 2010 at 01:58:53PM -0700, Randy Dunlap wrote:
>> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:
>>
>>> Report active and inactive suspend blockers in
>>> /sys/kernel/debug/suspend_blockers.
>>
>>
>> This looks more like user interface information than it does debug information.
>> If that's correct, it shouldn't be in debugfs.
> 
> It's debug-like information, and has more than one value per file, so
> debugfs seems like the proper place for it.  I have no objection to it
> going there.

I have no objection if it really is debug info, but I'm not convinced
of that yet.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[tytso]

On Sun, Apr 25, 2010 at 12:53:01PM -0700, Randy Dunlap wrote:
> > It's debug-like information, and has more than one value per file, so
> > debugfs seems like the proper place for it.  I have no objection to it
> > going there.
> 
> I have no objection if it really is debug info, but I'm not convinced
> of that yet.

Well, I'll note right now we have a somewhat annoying gap.  If you
need to export multiple values such that they are consistent with each
other, what's the choice?  /proc, where some (but not all) kernel
developers will say, "eeeeeeviilllll".  /sys is explicitly for single
value per files only.  And then we have /debugfs, where some pendants
are kvetching about whether something is "really" debug information.

One of the things that we sometimes have to tell people who are trying
to navigate the maze of upstream submission is that sometimes you need
to know who to ignore, and that sometimes rules are guidelines
(despite pendants who will NACK based on rules like, "/proc,
eeeeewwww", or "/debugfs must only strictly be for debug information".

Telling embedded developers who only want to submit their driver that
they must create a whole new pseudo-filesystem just to export a single
file that in older, simpler times, would have just been thrown into
/proc is really not fair, and is precisely the sort of thing that may
cause them to say, "f*ck it, these is one too many flaming hoops to
jump through".  If we throw up too many barriers, in the long run it's
not actually doing Linux a service.

Sure, we need to make sure is code doesn't become a future burden, but
does a new file in /proc or something that might not _really_ be debug
information showing up in /debugfs really such a terrible thing in
terms of making the kernel less maintainable in the future?

Regards,

						- Ted

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On 04/25/10 17:00, tytso@mit.edu wrote:
> On Sun, Apr 25, 2010 at 12:53:01PM -0700, Randy Dunlap wrote:
>>> It's debug-like information, and has more than one value per file, so
>>> debugfs seems like the proper place for it.  I have no objection to it
>>> going there.
>>
>> I have no objection if it really is debug info, but I'm not convinced
>> of that yet.
> 
> Well, I'll note right now we have a somewhat annoying gap.  If you
> need to export multiple values such that they are consistent with each
> other, what's the choice?  /proc, where some (but not all) kernel
> developers will say, "eeeeeeviilllll".  /sys is explicitly for single
> value per files only.  And then we have /debugfs, where some pendants
> are kvetching about whether something is "really" debug information.

First of all, I am not a pendant.

> One of the things that we sometimes have to tell people who are trying
> to navigate the maze of upstream submission is that sometimes you need
> to know who to ignore, and that sometimes rules are guidelines
> (despite pendants who will NACK based on rules like, "/proc,
> eeeeewwww", or "/debugfs must only strictly be for debug information".
> 
> Telling embedded developers who only want to submit their driver that
> they must create a whole new pseudo-filesystem just to export a single
> file that in older, simpler times, would have just been thrown into
> /proc is really not fair, and is precisely the sort of thing that may
> cause them to say, "f*ck it, these is one too many flaming hoops to
> jump through".  If we throw up too many barriers, in the long run it's
> not actually doing Linux a service.

Yeah, I think that it should be in procfs.  It's not strictly closed
to new files.  (IOW, I'm sure that we can find a bunch of recent files
added to procfs.)

> Sure, we need to make sure is code doesn't become a future burden, but
> does a new file in /proc or something that might not _really_ be debug
> information showing up in /debugfs really such a terrible thing in
> terms of making the kernel less maintainable in the future?

I don't think that we want to make debugfs required to get decent
tuning info/stats from the kernel.  That's all.

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[tytso]

On Sun, Apr 25, 2010 at 05:23:06PM -0700, Randy Dunlap wrote:
> Yeah, I think that it should be in procfs.  It's not strictly closed
> to new files.  (IOW, I'm sure that we can find a bunch of recent files
> added to procfs.)

That's reasonable (I think the whole /proc is evil crusade is really
dumb) --- but at the same time, remember how frustrating it is for the
poor embedded developer who doesn't know who to ignore and what
"rules" to ignore.  They were told months ago /proc is evil, and so
they moved it to /debugfs, precisely because it was billed as "it has
no rules".  For all I know some helpful community developer may have
even suggested that to them.

It is extremely frustrating to embedded developers when they get
conflicting advice, especially in this case, when it was *in* /proc in
the first place.  I'm not sure what to do about this --- my approach
is to sometimes say, "f*ck it, that's stupid advice", and ship it to
Linus, who tends to be *much* less of a pendant than most of the
people who review code --- but that's because I know what I can
ignore.  (I seriously doubt Linus cares much about whether it ends up
the file ends up /debugfs or /proc, and would take the code either
way.)  But for someone who doesn't understand when you can do this,
and who tries to follow every single piece of criticism they get, the
end result can often be a huge amount fo wasted time and frustration.

It would be nice if we could get better at this, since I'm sure this
is not the only time when embedded code submissions have gotten what
the submitting developers might consider to be a runaround....

(And just to be clear, I'm not criticising your commends; my personal
preference is slightly in favor of /proc, but more than anythign else,
I consider it a very minor point.  I just want to point out that they
_started_ with the file in /proc and changed it to /debugfs based on
someone NACK'ing their patch, with a "/proc, eeeeewwww" comment.
Sometimes I think some code reviewers get too much of a sense of power
trip by thinking they can NACK other people's code over their own pet
peeves.... instead of approaching it from a somewhat more collegial
point of view trying to make the code better.  Present company
excepted, of course.  :-)

		       	      	     	     - Ted

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On 04/25/10 17:45, tytso@mit.edu wrote:
> On Sun, Apr 25, 2010 at 05:23:06PM -0700, Randy Dunlap wrote:
>> Yeah, I think that it should be in procfs.  It's not strictly closed
>> to new files.  (IOW, I'm sure that we can find a bunch of recent files
>> added to procfs.)
> 
> That's reasonable (I think the whole /proc is evil crusade is really
> dumb) --- but at the same time, remember how frustrating it is for the
> poor embedded developer who doesn't know who to ignore and what
> "rules" to ignore.  They were told months ago /proc is evil, and so
> they moved it to /debugfs, precisely because it was billed as "it has
> no rules".  For all I know some helpful community developer may have
> even suggested that to them.
> 
> It is extremely frustrating to embedded developers when they get
> conflicting advice, especially in this case, when it was *in* /proc in
> the first place.  I'm not sure what to do about this --- my approach
> is to sometimes say, "f*ck it, that's stupid advice", and ship it to
> Linus, who tends to be *much* less of a pendant than most of the
> people who review code --- but that's because I know what I can
> ignore.  (I seriously doubt Linus cares much about whether it ends up
> the file ends up /debugfs or /proc, and would take the code either
> way.)  But for someone who doesn't understand when you can do this,
> and who tries to follow every single piece of criticism they get, the
> end result can often be a huge amount fo wasted time and frustration.
> 
> It would be nice if we could get better at this, since I'm sure this
> is not the only time when embedded code submissions have gotten what
> the submitting developers might consider to be a runaround....

Agreed, we could/should do much better.
Agreed, I'm sure that it is frustrating to the contributors.
Agreed about Linus taking it either way.  :)

Thanks for your summary and bringing it up (yet again).

> (And just to be clear, I'm not criticising your commends; my personal
> preference is slightly in favor of /proc, but more than anythign else,
> I consider it a very minor point.  I just want to point out that they
> _started_ with the file in /proc and changed it to /debugfs based on
> someone NACK'ing their patch, with a "/proc, eeeeewwww" comment.
> Sometimes I think some code reviewers get too much of a sense of power
> trip by thinking they can NACK other people's code over their own pet
> peeves.... instead of approaching it from a somewhat more collegial
> point of view trying to make the code better.  Present company
> excepted, of course.  :-)


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Alan Stern]

On Sun, 25 Apr 2010, Randy Dunlap wrote:

> > Well, I'll note right now we have a somewhat annoying gap.  If you
> > need to export multiple values such that they are consistent with each
> > other, what's the choice?  /proc, where some (but not all) kernel
> > developers will say, "eeeeeeviilllll".  /sys is explicitly for single
> > value per files only.  And then we have /debugfs, where some pendants
> > are kvetching about whether something is "really" debug information.
> 
> First of all, I am not a pendant.

Are you sure about that?  At the moment, Arve may feel that you're 
hanging around his neck (although perhaps not looking very pretty!).
:-)

Alan Stern

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Brian Swetland]

On Sun, Apr 25, 2010 at 12:53 PM, Randy Dunlap <randy.dunlap@oracle.com> wrote:
> On 04/25/10 11:15, Greg KH wrote:
>> On Fri, Apr 23, 2010 at 01:58:53PM -0700, Randy Dunlap wrote:
>>> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:
>>>
>>>> Report active and inactive suspend blockers in
>>>> /sys/kernel/debug/suspend_blockers.
>>>
>>>
>>> This looks more like user interface information than it does debug information.
>>> If that's correct, it shouldn't be in debugfs.
>>
>> It's debug-like information, and has more than one value per file, so
>> debugfs seems like the proper place for it.  I have no objection to it
>> going there.
>
> I have no objection if it really is debug info, but I'm not convinced
> of that yet.

Our most common use of this information is to capture it in bugreports
(along with process lists, system/radio logs, memory stats, etc) as
one additional piece of data used to diagnose a misbehaving device
(most commonly to answer the "why does the battery seem to be draining
too quickly?" question).

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

Re: [PATCH 5/9] PM: suspend_block: Add debugfs file

[Randy Dunlap]

On 04/25/10 23:24, Brian Swetland wrote:
> On Sun, Apr 25, 2010 at 12:53 PM, Randy Dunlap <randy.dunlap@oracle.com> wrote:
>> On 04/25/10 11:15, Greg KH wrote:
>>> On Fri, Apr 23, 2010 at 01:58:53PM -0700, Randy Dunlap wrote:
>>>> On Thu, 22 Apr 2010 18:08:54 -0700 Arve Hj?nnev?g wrote:
>>>>
>>>>> Report active and inactive suspend blockers in
>>>>> /sys/kernel/debug/suspend_blockers.
>>>>
>>>>
>>>> This looks more like user interface information than it does debug information.
>>>> If that's correct, it shouldn't be in debugfs.
>>>
>>> It's debug-like information, and has more than one value per file, so
>>> debugfs seems like the proper place for it.  I have no objection to it
>>> going there.
>>
>> I have no objection if it really is debug info, but I'm not convinced
>> of that yet.
> 
> Our most common use of this information is to capture it in bugreports
> (along with process lists, system/radio logs, memory stats, etc) as
> one additional piece of data used to diagnose a misbehaving device
> (most commonly to answer the "why does the battery seem to be draining
> too quickly?" question).

OK, thanks for the info.  Sounds good (i.e., it is debug info).

-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Matt Helsley]

On Thu, Apr 22, 2010 at 06:08:51PM -0700, Arve Hjønnevåg wrote:
> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.
> 
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  Documentation/power/suspend-blockers.txt |   17 ++++
>  include/linux/suspend_block_dev.h        |   25 ++++++
>  kernel/power/Kconfig                     |    9 ++
>  kernel/power/Makefile                    |    1 +
>  kernel/power/user_suspend_blocker.c      |  128 ++++++++++++++++++++++++++++++
>  5 files changed, 180 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/suspend_block_dev.h
>  create mode 100644 kernel/power/user_suspend_blocker.c
> 
> diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
> index 1c48514..877bd8c 100644
> --- a/Documentation/power/suspend-blockers.txt
> +++ b/Documentation/power/suspend-blockers.txt
> @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
>  else
>  	suspend_block(&state->suspend_blocker);
>  
> +User-space API
> +==============
> +
> +To create a suspend_blocker from user-space, open the suspend_blocker device:
> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> +then call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);

Why not initialize the user suspend blocker struct by default and then
allow each BLOCK to specify the name? Also, my guess is it's not
really a name so much as a description of why we're blocking suspend,
right?

Should the kernel reject empty strings or strings composed only of
"non-printing" characters?

> +
> +To activate a suspend_blocker call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> +
> +To unblock call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);

lsof will show which tasks hold the device open but not which ones
are blocking suspend. If merely keeping the device open corresponded to
blocking suspend then this would be obvious and no ioctls would be
necessary -- just write() the name/description.

Do you block/unblock often enough that frequent open/close of the device are
a problem? Or has this idea been considered and discarded for other reasons?

> +
> +To destroy the suspend_blocker, close the device:
> +    close(fd);
> +

<snip>

> diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
> new file mode 100644
> index 0000000..a9be6f4
> --- /dev/null
> +++ b/kernel/power/user_suspend_blocker.c
> @@ -0,0 +1,128 @@
> +/* kernel/power/user_suspend_block.c
> + *
> + * Copyright (C) 2009-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/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/suspend_blocker.h>
> +#include <linux/suspend_block_dev.h>
> +
> +enum {
> +	DEBUG_FAILURE	= BIT(0),
> +};
> +static int debug_mask = DEBUG_FAILURE;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +static DEFINE_MUTEX(ioctl_lock);

nit: Usually locks protect data -- not functions.

Couldn't this be part of the user_suspend_blocker struct? That would allow
the description/name to change as described above.

> +
> +struct user_suspend_blocker {
> +	struct suspend_blocker	blocker;
> +	char			name[0];
> +};

<snip>

Cheers,
	-Matt Helsley

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

2010/4/22 Matt Helsley <matthltc@us.ibm.com>:
...
> On Thu, Apr 22, 2010 at 06:08:51PM -0700, Arve Hjønnevåg wrote:
>> +To create a suspend_blocker from user-space, open the suspend_blocker device:
>> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> +then call:
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
>
> Why not initialize the user suspend blocker struct by default and then
> allow each BLOCK to specify the name? Also, my guess is it's not
> really a name so much as a description of why we're blocking suspend,
> right?
>

There are stats tracked as long as the suspend blocker exists.
Specifying a new name every time you block suspend would make this
less useful.

> Should the kernel reject empty strings or strings composed only of
> "non-printing" characters?
>

Is there an existing function that check if a sting is "unsafe". If
so, I can add a call to this.

>> +
>> +To activate a suspend_blocker call:
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
>> +
>> +To unblock call:
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
>
> lsof will show which tasks hold the device open but not which ones
> are blocking suspend. If merely keeping the device open corresponded to
> blocking suspend then this would be obvious and no ioctls would be
> necessary -- just write() the name/description.

We track more information about suspend blockers than this.

>
> Do you block/unblock often enough that frequent open/close of the device are
> a problem?

Yes.

...
>> +
>> +static DEFINE_MUTEX(ioctl_lock);
>
> nit: Usually locks protect data -- not functions.
>
> Couldn't this be part of the user_suspend_blocker struct? That would allow
> the description/name to change as described above.

It mainly protects the allocation of that struct, so no. Allocating a
separate struct in open would work, but does not seem worth it at the
moment.

>
>> +
>> +struct user_suspend_blocker {
>> +     struct suspend_blocker  blocker;
>> +     char                    name[0];
>> +};
>
> <snip>
>
> Cheers,
>        -Matt Helsley
>



-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Greg KH]

On Thu, Apr 22, 2010 at 07:25:22PM -0700, Matt Helsley wrote:
> Should the kernel reject empty strings or strings composed only of
> "non-printing" characters?

The kernel doesn't care, if you want to use non-printing characters, who
is it to complain?  :)

thanks,

greg k-h

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

Hi!

> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.
> 
> Signed-off-by: Arve Hj??nnev??g <arve@android.com>

> --- a/Documentation/power/suspend-blockers.txt
> +++ b/Documentation/power/suspend-blockers.txt
> @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
>  else
>  	suspend_block(&state->suspend_blocker);
>  
> +User-space API
> +==============
> +
> +To create a suspend_blocker from user-space, open the suspend_blocker device:
> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> +then call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);


This seems like very wrong idea -- it uses different ioctl number for
each length AFAICT.

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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Alan Stern]

On Fri, 23 Apr 2010, Pavel Machek wrote:

> Hi!
> 
> > Add a misc device, "suspend_blocker", that allows user-space processes
> > to block auto suspend. The device has ioctls to create a suspend_blocker,
> > and to block and unblock suspend. To delete the suspend_blocker, close
> > the device.
> > 
> > Signed-off-by: Arve Hj??nnev??g <arve@android.com>
> 
> > --- a/Documentation/power/suspend-blockers.txt
> > +++ b/Documentation/power/suspend-blockers.txt
> > @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
> >  else
> >  	suspend_block(&state->suspend_blocker);
> >  
> > +User-space API
> > +==============
> > +
> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > +then call:
> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> 
> 
> This seems like very wrong idea -- it uses different ioctl number for
> each length AFAICT.

How about specifying the name by an ordinary write() call instead of
by an ioctl()?

Alan Stern

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[tytso]

On Fri, Apr 23, 2010 at 10:43:49AM +0200, Pavel Machek wrote:
> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > +then call:
> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> 
> 
> This seems like very wrong idea -- it uses different ioctl number for
> each length AFAICT.

Yep, and there's nothing wrong with that IMHO.  It's a clever use of
the _IOC encoding scheme.

						- Ted

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

On Fri, Apr 23, 2010 at 9:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 23 Apr 2010, Pavel Machek wrote:
>
>> Hi!
>>
>> > Add a misc device, "suspend_blocker", that allows user-space processes
>> > to block auto suspend. The device has ioctls to create a suspend_blocker,
>> > and to block and unblock suspend. To delete the suspend_blocker, close
>> > the device.
>> >
>> > Signed-off-by: Arve Hj??nnev??g <arve@android.com>
>>
>> > --- a/Documentation/power/suspend-blockers.txt
>> > +++ b/Documentation/power/suspend-blockers.txt
>> > @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
>> >  else
>> >     suspend_block(&state->suspend_blocker);
>> >
>> > +User-space API
>> > +==============
>> > +
>> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
>> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> > +then call:
>> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
>>
>>
>> This seems like very wrong idea -- it uses different ioctl number for
>> each length AFAICT.
>
> How about specifying the name by an ordinary write() call instead of
> by an ioctl()?
>

I prefer using ioctls. We have three operations at the moment. Init,
block and unblock. If we do init with write but block and unblock
using ioctls, it would be pretty strange. Specifying a command and
argument in a string to write is more complicated to parse than using
ioctls. Or did you have something else in mind?

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

On Fri 2010-04-23 20:20:47, Arve Hj?nnev?g wrote:
> On Fri, Apr 23, 2010 at 9:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Fri, 23 Apr 2010, Pavel Machek wrote:
> >
> >> Hi!
> >>
> >> > Add a misc device, "suspend_blocker", that allows user-space processes
> >> > to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> > and to block and unblock suspend. To delete the suspend_blocker, close
> >> > the device.
> >> >
> >> > Signed-off-by: Arve Hj??nnev??g <arve@android.com>
> >>
> >> > --- a/Documentation/power/suspend-blockers.txt
> >> > +++ b/Documentation/power/suspend-blockers.txt
> >> > @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
> >> >  else
> >> >     suspend_block(&state->suspend_blocker);
> >> >
> >> > +User-space API
> >> > +==============
> >> > +
> >> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> >> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> >> > +then call:
> >> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> >>
> >>
> >> This seems like very wrong idea -- it uses different ioctl number for
> >> each length AFAICT.
> >
> > How about specifying the name by an ordinary write() call instead of
> > by an ioctl()?
> >
> 
> I prefer using ioctls. We have three operations at the moment. Init,
> block and unblock. If we do init with write but block and unblock
> using ioctls, it would be pretty strange. Specifying a command and

Why would it be "strange"?

> argument in a string to write is more complicated to parse than using
> ioctls.

More complicated to parse?

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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

On Fri 2010-04-23 21:53:34, tytso@mit.edu wrote:
> On Fri, Apr 23, 2010 at 10:43:49AM +0200, Pavel Machek wrote:
> > > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> > > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > > +then call:
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> > 
> > 
> > This seems like very wrong idea -- it uses different ioctl number for
> > each length AFAICT.
> 
> Yep, and there's nothing wrong with that IMHO.  It's a clever use of
> the _IOC encoding scheme.

I'm not sure if "clever" is right word. So what if strlen is in 2GB
range, will macros still work correctly?

Will it be easy for strace to display such ioctls?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Alan Stern]

On Thu, 22 Apr 2010, [UTF-8] 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.

> --- /dev/null
> +++ b/Documentation/power/suspend-blockers.txt
> @@ -0,0 +1,97 @@
> +Suspend blockers
> +================
> +
> +Suspend blockers provide a mechanism for device drivers and user-space processes
> +to prevent the system from entering suspend. By default writing to
> +/sys/power/state will ignore suspend blockers. Writing "opportunistic" to
> +/sys/power/policy will change the behaviour of /sys/power/state to repeatedly
> +enter the requested state when no suspend blockers are active. Writing "on" to
> +/sys/power/state will cancel the automatic sleep request. Suspend blockers do
> +not affect sleep states entered from idle.

You should document that writing "forced" to /sys/power/policy causes
the /sys/power/state to return to normal operation (i.e., ignoring
suspend blockers).


> +In cell phones or other embedded systems where powering the screen is a
> +significant drain on the battery, suspend blockers can be used to allow
> +user-space to decide whether a keystroke received while the system is suspended
> +should cause the screen to be turned back on or allow the system to go back into
> +suspend.

The description below is incomplete and consequently doesn't make 
sense.

>  Use set_irq_wake or a platform specific api to make sure the keypad
> +interrupt wakes up the cpu.

And as part of waking up the CPU, the screen driver's resume method is
called.  Presumably this will turn the screen back on.  If it doesn't,
you need to explain why not.

> 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 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 is highly misleading.  Although you _say_ this decision is about
whether or not to leave the screen off, it _really_ is about whether or
not to let the system automatically suspend again.  In other words, the
exposition confuses suspending the system with turning off the screen.  
Unless you can keep the two notions separate and explain more clearly
what's going on, this whole section should be removed from the
documentation.


> +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. This allows drivers to update their state and
> +call suspend suspend_block or suspend_unblock based on the result.

But suspend_block() and suspend_unblock() don't nest.  You should 
mention this.


> --- /dev/null
> +++ b/include/linux/suspend_blocker.h
> @@ -0,0 +1,60 @@
...
> +/**
> + * struct suspend_blocker - the basic suspend_blocker structure
> + * @flags:	Tracks initialized and active state.
> + * @name:	Name used for debugging.
> + *
> + * When a suspend_blocker is active it prevents the system from entering
> + * suspend.
> + *
> + * The suspend_blocker structure must be initialized by suspend_blocker_init()
> + */
> +
> +struct suspend_blocker {
> +#ifdef CONFIG_SUSPEND_BLOCKERS
> +	atomic_t            flags;
> +	const char         *name;
> +#endif

Why is flags an atomic_t?  Are you worried that drivers might try to 
activate a suspend_blocker at the same time that it is being destroyed?
If this happens, does the code do the right thing?  I don't think it 
does -- if a race occurs, suspend_block() will leave flags set to the 
wrong value.  The same goes for suspend_unblock().

Since these routines don't nest, there is also the possibility of a
race between suspend_block() and suspend_unblock().  If the race goes
one way the blocker is active; the other way it isn't.  Given that such
problems already exist, why worry about what happens when the suspend
blocker is destroyed?

Alan Stern

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

[Alan Stern]

On Fri, 23 Apr 2010, Alan Stern wrote:

> On Thu, 22 Apr 2010, [UTF-8] Arve Hjønnevåg wrote:

> > +struct suspend_blocker {
> > +#ifdef CONFIG_SUSPEND_BLOCKERS
> > +	atomic_t            flags;
> > +	const char         *name;
> > +#endif
> 
> Why is flags an atomic_t?  Are you worried that drivers might try to 
> activate a suspend_blocker at the same time that it is being destroyed?
> If this happens, does the code do the right thing?  I don't think it 
> does -- if a race occurs, suspend_block() will leave flags set to the 
> wrong value.  The same goes for suspend_unblock().
> 
> Since these routines don't nest, there is also the possibility of a
> race between suspend_block() and suspend_unblock().  If the race goes
> one way the blocker is active; the other way it isn't.  Given that such
> problems already exist, why worry about what happens when the suspend
> blocker is destroyed?

Having now read the later patches, I see that you switch over to using 
a spinlock instead of an atomic_t.  My suggestion is to use a spinlock 
right from the start.  It will be less confusing.

Alan Stern

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

[Arve Hjønnevåg]

2010/4/23 Alan Stern <stern@rowland.harvard.edu>:
> On Thu, 22 Apr 2010, [UTF-8] Arve Hjønnevåg wrote:
...
>> +Calling suspend_block when the suspend blocker is active or suspend_unblock when
>> +it is not active has no effect. This allows drivers to update their state and
>> +call suspend suspend_block or suspend_unblock based on the result.
>
> But suspend_block() and suspend_unblock() don't nest.  You should
> mention this.
>

I'm not sure what you mean by this? I think the first sentence
dictates nesting is not supported.

I think the rest of your comments will be addressed in the next
version of the document, that I worked with Rafael on.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Rafael J. Wysocki]

On Tuesday 27 April 2010, Alan Stern wrote:
> On Mon, 26 Apr 2010, Arve Hjønnevåg wrote:
> 
> > > If you insist on using ioctl for init, you should use the standard
> > > convention for passing variable-length data.  The userspace program
> > > sets up a fixed-size buffer containing a pointer to the name and the
> > > name's length, and it passes the buffer's address as the ioctl
> > > argument.
> > 
> > Are you sure that is the standard? I searched for ioctls with NAME in
> > their name and only found one that passed the name that way. The rest
> > used fixed length string buffers, or passed the buffersize to _IOC
> > like I do. For instance, input.h has ioctls to read string and
> > bitmasks where user space specify the buffer size as an argument to
> > the ioctl macro. These pass data from the kernel to user space, but I
> > don't passing a string length is any worse than passing a buffer size.
> 
> You're right.  Okay, I withdraw my objection.

In the meantime, though, I thought that the suspend blocker might be created
by _open() if we found a way to automatically choose a name for it.  That'd be
kind of logical, since it's later destroyed by _release().

So, what about using the name of the process that opened the special device
file (or that name with'0' appended, or generally with a number appended) as
the suspend blocker name?

Rafael

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

2010/4/27 Rafael J. Wysocki <rjw@sisk.pl>:
> On Tuesday 27 April 2010, Alan Stern wrote:
>> On Mon, 26 Apr 2010, Arve Hjønnevåg wrote:
>>
>> > > If you insist on using ioctl for init, you should use the standard
>> > > convention for passing variable-length data.  The userspace program
>> > > sets up a fixed-size buffer containing a pointer to the name and the
>> > > name's length, and it passes the buffer's address as the ioctl
>> > > argument.
>> >
>> > Are you sure that is the standard? I searched for ioctls with NAME in
>> > their name and only found one that passed the name that way. The rest
>> > used fixed length string buffers, or passed the buffersize to _IOC
>> > like I do. For instance, input.h has ioctls to read string and
>> > bitmasks where user space specify the buffer size as an argument to
>> > the ioctl macro. These pass data from the kernel to user space, but I
>> > don't passing a string length is any worse than passing a buffer size.
>>
>> You're right.  Okay, I withdraw my objection.
>
> In the meantime, though, I thought that the suspend blocker might be created
> by _open() if we found a way to automatically choose a name for it.  That'd be
> kind of logical, since it's later destroyed by _release().
>
> So, what about using the name of the process that opened the special device
> file (or that name with'0' appended, or generally with a number appended) as
> the suspend blocker name?
>

I prefer to let user space choose the name since we use more than one
suspend blocker in the same process.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Alan Stern]

On Mon, 26 Apr 2010, Arve Hjønnevåg wrote:

> > If you insist on using ioctl for init, you should use the standard
> > convention for passing variable-length data.  The userspace program
> > sets up a fixed-size buffer containing a pointer to the name and the
> > name's length, and it passes the buffer's address as the ioctl
> > argument.
> 
> Are you sure that is the standard? I searched for ioctls with NAME in
> their name and only found one that passed the name that way. The rest
> used fixed length string buffers, or passed the buffersize to _IOC
> like I do. For instance, input.h has ioctls to read string and
> bitmasks where user space specify the buffer size as an argument to
> the ioctl macro. These pass data from the kernel to user space, but I
> don't passing a string length is any worse than passing a buffer size.

You're right.  Okay, I withdraw my objection.

Alan Stern

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Alan Stern]

On Sun, 25 Apr 2010, Arve Hjønnevåg wrote:

> >> > >> > +User-space API
> >> > >> > +==============
> >> > >> > +
> >> > >> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> >> > >> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> >> > >> > +then call:
> >> > >> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> >> > >>
> >> > >>
> >> > >> This seems like very wrong idea -- it uses different ioctl number for
> >> > >> each length AFAICT.
> >> > >
> >> > > How about specifying the name by an ordinary write() call instead of
> >> > > by an ioctl()?
> >> > >
> >> >
> >> > I prefer using ioctls. We have three operations at the moment. Init,
> >> > block and unblock. If we do init with write but block and unblock
> >> > using ioctls, it would be pretty strange. Specifying a command and
> >>
> >> Why would it be "strange"?
> >
> > Why indeed?  Using write() is the natural way to pass a data buffer
> > into the kernel, especially a variable-length buffer.
> >
> > Mixing ioctl() and write() might seem strange at first, but it has
> > plenty of precedent.  Consider adjusting the settings for a serial
> > port, for example.
> >
> That sound like to opposite situation to me. It uses ioctls for setup
> and read/write access the data stream.

Or you could say that it uses ioctls to send commands and read/write to 
pass data.  That's pretty much what you would be doing.

Let's put it another way: You find it excessively strange to do init
using write and to send commands via ioctl, but you don't find it
strange to abuse the ioctl number code for passing the string length?  
The old saying about motes and beams applies here...

If you insist on using ioctl for init, you should use the standard
convention for passing variable-length data.  The userspace program
sets up a fixed-size buffer containing a pointer to the name and the
name's length, and it passes the buffer's address as the ioctl
argument.

Alan Stern

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

2010/4/26 Alan Stern <stern@rowland.harvard.edu>:
> On Sun, 25 Apr 2010, Arve Hjønnevåg wrote:
>
>> >> > >> > +User-space API
>> >> > >> > +==============
>> >> > >> > +
>> >> > >> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
>> >> > >> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> >> > >> > +then call:
>> >> > >> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
>> >> > >>
>> >> > >>
>> >> > >> This seems like very wrong idea -- it uses different ioctl number for
>> >> > >> each length AFAICT.
>> >> > >
>> >> > > How about specifying the name by an ordinary write() call instead of
>> >> > > by an ioctl()?
>> >> > >
>> >> >
>> >> > I prefer using ioctls. We have three operations at the moment. Init,
>> >> > block and unblock. If we do init with write but block and unblock
>> >> > using ioctls, it would be pretty strange. Specifying a command and
>> >>
>> >> Why would it be "strange"?
>> >
>> > Why indeed?  Using write() is the natural way to pass a data buffer
>> > into the kernel, especially a variable-length buffer.
>> >
>> > Mixing ioctl() and write() might seem strange at first, but it has
>> > plenty of precedent.  Consider adjusting the settings for a serial
>> > port, for example.
>> >
>> That sound like to opposite situation to me. It uses ioctls for setup
>> and read/write access the data stream.
>
> Or you could say that it uses ioctls to send commands and read/write to
> pass data.  That's pretty much what you would be doing.
>
> Let's put it another way: You find it excessively strange to do init
> using write and to send commands via ioctl, but you don't find it
> strange to abuse the ioctl number code for passing the string length?
> The old saying about motes and beams applies here...
>
> If you insist on using ioctl for init, you should use the standard
> convention for passing variable-length data.  The userspace program
> sets up a fixed-size buffer containing a pointer to the name and the
> name's length, and it passes the buffer's address as the ioctl
> argument.

Are you sure that is the standard? I searched for ioctls with NAME in
their name and only found one that passed the name that way. The rest
used fixed length string buffers, or passed the buffersize to _IOC
like I do. For instance, input.h has ioctls to read string and
bitmasks where user space specify the buffer size as an argument to
the ioctl macro. These pass data from the kernel to user space, but I
don't passing a string length is any worse than passing a buffer size.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Alan Stern]

On Sat, 24 Apr 2010, Pavel Machek wrote:

> On Fri 2010-04-23 20:20:47, Arve Hj?nnev?g wrote:
> > On Fri, Apr 23, 2010 at 9:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Fri, 23 Apr 2010, Pavel Machek wrote:
> > >
> > >> Hi!
> > >>
> > >> > Add a misc device, "suspend_blocker", that allows user-space processes
> > >> > to block auto suspend. The device has ioctls to create a suspend_blocker,
> > >> > and to block and unblock suspend. To delete the suspend_blocker, close
> > >> > the device.
> > >> >
> > >> > Signed-off-by: Arve Hj??nnev??g <arve@android.com>
> > >>
> > >> > --- a/Documentation/power/suspend-blockers.txt
> > >> > +++ b/Documentation/power/suspend-blockers.txt
> > >> > @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
> > >> >  else
> > >> >     suspend_block(&state->suspend_blocker);
> > >> >
> > >> > +User-space API
> > >> > +==============
> > >> > +
> > >> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
> > >> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > >> > +then call:
> > >> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> > >>
> > >>
> > >> This seems like very wrong idea -- it uses different ioctl number for
> > >> each length AFAICT.
> > >
> > > How about specifying the name by an ordinary write() call instead of
> > > by an ioctl()?
> > >
> > 
> > I prefer using ioctls. We have three operations at the moment. Init,
> > block and unblock. If we do init with write but block and unblock
> > using ioctls, it would be pretty strange. Specifying a command and
> 
> Why would it be "strange"?

Why indeed?  Using write() is the natural way to pass a data buffer
into the kernel, especially a variable-length buffer.

Mixing ioctl() and write() might seem strange at first, but it has
plenty of precedent.  Consider adjusting the settings for a serial
port, for example.

> > argument in a string to write is more complicated to parse than using
> > ioctls.
> 
> More complicated to parse?

It shouldn't be -- especially if you assume that the init action must
always come first.  The first write would contain the suspend blocker's
name; all following writes would have to be either "on" or "off".  
That's not hard to parse.

Alan Stern

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

On Sat, Apr 24, 2010 at 7:44 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 24 Apr 2010, Pavel Machek wrote:
>
>> On Fri 2010-04-23 20:20:47, Arve Hj?nnev?g wrote:
>> > On Fri, Apr 23, 2010 at 9:43 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > > On Fri, 23 Apr 2010, Pavel Machek wrote:
>> > >
>> > >> Hi!
>> > >>
>> > >> > Add a misc device, "suspend_blocker", that allows user-space processes
>> > >> > to block auto suspend. The device has ioctls to create a suspend_blocker,
>> > >> > and to block and unblock suspend. To delete the suspend_blocker, close
>> > >> > the device.
>> > >> >
>> > >> > Signed-off-by: Arve Hj??nnev??g <arve@android.com>
>> > >>
>> > >> > --- a/Documentation/power/suspend-blockers.txt
>> > >> > +++ b/Documentation/power/suspend-blockers.txt
>> > >> > @@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
>> > >> >  else
>> > >> >     suspend_block(&state->suspend_blocker);
>> > >> >
>> > >> > +User-space API
>> > >> > +==============
>> > >> > +
>> > >> > +To create a suspend_blocker from user-space, open the suspend_blocker device:
>> > >> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> > >> > +then call:
>> > >> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
>> > >>
>> > >>
>> > >> This seems like very wrong idea -- it uses different ioctl number for
>> > >> each length AFAICT.
>> > >
>> > > How about specifying the name by an ordinary write() call instead of
>> > > by an ioctl()?
>> > >
>> >
>> > I prefer using ioctls. We have three operations at the moment. Init,
>> > block and unblock. If we do init with write but block and unblock
>> > using ioctls, it would be pretty strange. Specifying a command and
>>
>> Why would it be "strange"?
>
> Why indeed?  Using write() is the natural way to pass a data buffer
> into the kernel, especially a variable-length buffer.
>
> Mixing ioctl() and write() might seem strange at first, but it has
> plenty of precedent.  Consider adjusting the settings for a serial
> port, for example.
>
That sound like to opposite situation to me. It uses ioctls for setup
and read/write access the data stream.

>> > argument in a string to write is more complicated to parse than using
>> > ioctls.
>>
>> More complicated to parse?
>
> It shouldn't be -- especially if you assume that the init action must
> always come first.  The first write would contain the suspend blocker's
> name; all following writes would have to be either "on" or "off".
> That's not hard to parse.
>

Why should I have to parse a string at all? We already have a control
interface, ioctl, where user space can pass a command with data. If we
later want to add other commands we can easily add them without
breaking existing command. With your interface, where the first write
is a name, adding more initialization data later becomes harder. I
also don't like that wring the same string twice has a different
meaning the second time. With the ioctl interface, you forget to
initialize the suspend blocker, it block and unblock operations will
fail. With your interface you create a suspend blocker call "on" or
"off".

-- 
Arve Hjønnevåg

[RFC][PATCH 0/9] Suspend block api (version 3)

[Arve Hjønnevåg]

This patch series adds a suspend-block api that provides the same
functionality as the android wakelock api. This patch series removes
the sysdev that was used to abort suspend and adds a direct check in
suspend_enter instead. It also has some other code style changes.

---
Arve Hjønnevåg

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

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

[Arve Hjønnevåg]

Adds /sys/power/request_state, a non-blocking interface that specifies
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/suspend-blockers.txt |   94 +++++++++++++
 include/linux/suspend_blocker.h          |   60 +++++++++
 kernel/power/Kconfig                     |   10 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/main.c                      |   62 +++++++++-
 kernel/power/power.h                     |    4 +
 kernel/power/suspend_blocker.c           |  209 ++++++++++++++++++++++++++++++
 7 files changed, 439 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/power/suspend-blockers.txt
 create mode 100755 include/linux/suspend_blocker.h
 create mode 100644 kernel/power/suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
new file mode 100644
index 0000000..5a33ed6
--- /dev/null
+++ b/Documentation/power/suspend-blockers.txt
@@ -0,0 +1,94 @@
+Suspend blockers
+================
+
+Suspend blockers provide a mechanism for device drivers and user-space processes
+to prevent the system from entering suspend. Only suspend states requested
+through /sys/power/request_state are prevented. Writing to /sys/power/state
+will ignore suspend blockers, and sleep states entered from idle are unaffected.
+
+To use a suspend blocker, a device driver needs a struct suspend_blocker object
+that has to be initialized with suspend_blocker_init. When no longer needed,
+the suspend blocker must be destroyed with suspend_blocker_destroy.
+
+A suspend blocker is activated using suspend_block, which prevents the system
+from entering suspend until the suspend blocker is deactivated with
+suspend_unblock. Many suspend blockers may be used simultaneously, and the
+system is prevented from entering suspend as long as at least one of them is
+active.
+
+If the suspend operation has already started when calling suspend_block on a
+suspend_blocker, it will abort the suspend operation as long it has not already
+reached called suspend_ops->enter. This means that calling suspend_block from an
+interrupt handler or a freezeable thread always works, but some or all drivers
+may be temporarily suspended.
+
+In cell phones or other embedded systems where powering the screen is a
+significant drain on the battery, suspend blockers can be used to allow
+user-space to decide whether a keystroke received while the system is suspended
+should cause the screen to be turned back on or allow the system to go back into
+suspend. 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 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.
+
+                 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. 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..21689cd
--- /dev/null
+++ b/include/linux/suspend_blocker.h
@@ -0,0 +1,60 @@
+/* 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
+
+/**
+ * struct suspend_blocker - the basic suspend_blocker structure
+ * @flags:	Tracks initialized and active state.
+ * @name:	Name used for debugging.
+ *
+ * When a suspend_blocker is active it prevents the system from entering
+ * suspend.
+ *
+ * The suspend_blocker structure must be initialized by suspend_blocker_init()
+ */
+
+struct suspend_blocker {
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	atomic_t            flags;
+	const char         *name;
+#endif
+};
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+
+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 23bd4da..a3587d3 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -116,6 +116,16 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config SUSPEND_BLOCKERS
+	bool "Suspend blockers"
+	depends on PM
+	select RTC_LIB
+	default n
+	---help---
+	  Enable suspend blockers. When user space requests a sleep state
+	  through /sys/power/request_state, the requested sleep state will be
+	  entered when no suspend blockers are active.
+
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 720ea4f..e8c7f85 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -6,6 +6,7 @@ endif
 obj-$(CONFIG_PM)		+= main.o
 obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
+obj-$(CONFIG_SUSPEND_BLOCKERS)	+= suspend_blocker.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o disk.o snapshot.o swap.o user.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/main.c b/kernel/power/main.c
index f99ed6a..1899cda 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -22,6 +22,7 @@
 #include <linux/freezer.h>
 #include <linux/vmstat.h>
 #include <linux/syscalls.h>
+#include <linux/suspend_blocker.h>
 
 #include "power.h"
 
@@ -321,7 +322,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();
 	}
@@ -413,6 +414,7 @@ static void suspend_finish(void)
 
 
 static const char * const pm_states[PM_SUSPEND_MAX] = {
+	[PM_SUSPEND_ON]		= "on",
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
 };
@@ -561,6 +563,61 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(state);
 
+/**
+ *	request_state - control system power state.
+ *
+ *	This is similar to state, but it does not block until the system
+ *	resumes, and it will try to re-enter the state until another state is
+ *	requested. Suspend blockers are respected and the requested state will
+ *	only be entered when no suspend blockers are active.
+ *	Write "on" to cancel.
+ */
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+static ssize_t request_state_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	char *s = buf;
+	int i;
+
+	for (i = 0; i < PM_SUSPEND_MAX; i++) {
+		if (pm_states[i] && (i == PM_SUSPEND_ON || valid_state(i)))
+			s += sprintf(s, "%s ", pm_states[i]);
+	}
+	if (s != buf)
+		/* convert the last space to a newline */
+		*(s-1) = '\n';
+	return (s - buf);
+}
+
+static ssize_t request_state_store(struct kobject *kobj,
+				   struct kobj_attribute *attr,
+				   const char *buf, size_t n)
+{
+	suspend_state_t state = PM_SUSPEND_ON;
+	const char * const *s;
+	char *p;
+	int len;
+	int error = -EINVAL;
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
+		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
+			break;
+	}
+	if (state < PM_SUSPEND_MAX && *s)
+		if (state == PM_SUSPEND_ON || valid_state(state)) {
+			error = 0;
+			request_suspend_state(state);
+		}
+	return error ? error : n;
+}
+
+power_attr(request_state);
+#endif /* CONFIG_SUSPEND_BLOCKERS */
+
 #ifdef CONFIG_PM_TRACE
 int pm_trace_enabled;
 
@@ -588,6 +645,9 @@ power_attr(pm_trace);
 
 static struct attribute * g[] = {
 	&state_attr.attr,
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	&request_state_attr.attr,
+#endif
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 46b5ec7..2246b3b 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -223,3 +223,7 @@ static inline void suspend_thaw_processes(void)
 {
 }
 #endif
+
+/* kernel/power/suspend_block.c */
+void request_suspend_state(suspend_state_t state);
+
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
new file mode 100644
index 0000000..6b9ca2d
--- /dev/null
+++ b/kernel/power/suspend_blocker.c
@@ -0,0 +1,209 @@
+/* kernel/power/suspend_blocker.c
+ *
+ * Copyright (C) 2005-2008 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/suspend.h>
+#include <linux/suspend_blocker.h>
+#include "power.h"
+
+enum {
+	DEBUG_EXIT_SUSPEND = 1U << 0,
+	DEBUG_WAKEUP = 1U << 1,
+	DEBUG_USER_STATE = 1U << 2,
+	DEBUG_SUSPEND = 1U << 3,
+	DEBUG_SUSPEND_BLOCKER = 1U << 4,
+};
+static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+#define SB_INITIALIZED            (1U << 8)
+#define SB_ACTIVE                 (1U << 9)
+
+static DEFINE_SPINLOCK(state_lock);
+static atomic_t suspend_block_count;
+static atomic_t current_event_num;
+struct workqueue_struct *suspend_work_queue;
+struct suspend_blocker main_suspend_blocker;
+static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
+static bool enable_suspend_blockers;
+
+#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);
+
+/**
+ * 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 !!atomic_read(&suspend_block_count);
+}
+
+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 = atomic_read(&current_event_num);
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: enter suspend\n");
+		ret = pm_suspend(requested_suspend_state);
+		if (debug_mask & DEBUG_EXIT_SUSPEND)
+			pr_info_time("suspend: exit suspend, ret = %d ", ret);
+		if (atomic_read(&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)
+{
+	WARN_ON(!name);
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_init name=%s\n", name);
+
+	blocker->name = name;
+	atomic_set(&blocker->flags, SB_INITIALIZED);
+}
+EXPORT_SYMBOL(suspend_blocker_init);
+
+/**
+ * suspend_blocker_destroy() - Destroy a suspend blocker
+ * @blocker:	The suspend blocker to destroy.
+ */
+void suspend_blocker_destroy(struct suspend_blocker *blocker)
+{
+	int flags;
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
+	flags = atomic_xchg(&blocker->flags, 0);
+	WARN(!(flags & SB_INITIALIZED), "suspend_blocker_destroy called on "
+					"uninitialized suspend_blocker\n");
+	if (flags == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_blocker_destroy);
+
+/**
+ * suspend_block() - Block suspend
+ * @blocker:	The suspend blocker to use
+ */
+void suspend_block(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_block: %s\n", blocker->name);
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED,
+	    SB_INITIALIZED | SB_ACTIVE) == SB_INITIALIZED)
+		atomic_inc(&suspend_block_count);
+
+	atomic_inc(&current_event_num);
+}
+EXPORT_SYMBOL(suspend_block);
+
+/**
+ * suspend_unblock() - Unblock suspend
+ * @blocker:	The suspend blocker to unblock.
+ *
+ * If no other suspend blockers block suspend, the system will suspend.
+ */
+void suspend_unblock(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_unblock: %s\n", blocker->name);
+
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED | SB_ACTIVE,
+	    SB_INITIALIZED) == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_unblock);
+
+/**
+ * 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(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	return !!(atomic_read(&blocker->flags) & SB_ACTIVE);
+}
+EXPORT_SYMBOL(suspend_blocker_is_active);
+
+void request_suspend_state(suspend_state_t state)
+{
+	unsigned long irqflags;
+	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);
+}
+
+static int __init suspend_block_init(void)
+{
+	suspend_work_queue = create_singlethread_workqueue("suspend");
+	if (!suspend_work_queue)
+		return -ENOMEM;
+
+	suspend_blocker_init(&main_suspend_blocker, "main");
+	suspend_block(&main_suspend_blocker);
+	return 0;
+}
+
+core_initcall(suspend_block_init);
-- 
1.6.1

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

[PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

Add a misc device, "suspend_blocker", that allows user-space processes
to block auto suspend. The device has ioctls to create a suspend_blocker,
and to block and unblock suspend. To delete the suspend_blocker, close
the device.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |   17 ++++
 include/linux/suspend_block_dev.h        |   25 ++++++
 kernel/power/Kconfig                     |    9 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/user_suspend_blocker.c      |  125 ++++++++++++++++++++++++++++++
 5 files changed, 177 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 5a33ed6..9d78d4e 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -92,3 +92,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..a7c0bf9
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
+#define _LINUX_SUSPEND_BLOCK_DEV_H
+
+#include <linux/ioctl.h>
+
+#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 3)
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index a3587d3..84b423f 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -126,6 +126,15 @@ config SUSPEND_BLOCKERS
 	  through /sys/power/request_state, the requested sleep state will be
 	  entered when no suspend blockers are active.
 
+config USER_SUSPEND_BLOCKERS
+	bool "Userspace suspend blockers"
+	depends on SUSPEND_BLOCKERS
+	default y
+	---help---
+	  User-space suspend block api. Creates a misc device with ioctls
+	  to create, block and unblock a suspend_blocker. The suspend_blocker
+	  will be deleted when the device is closed.
+
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index e8c7f85..24ade91 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_BLOCKERS)	+= suspend_blocker.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o disk.o snapshot.o swap.o user.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..69b3ddb
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,125 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 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.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/suspend_blocker.h>
+#include <linux/suspend_block_dev.h>
+
+enum {
+	DEBUG_FAILURE	= BIT(0),
+};
+static int debug_mask = DEBUG_FAILURE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+static DEFINE_MUTEX(ioctl_lock);
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[0];
+};
+
+static int create_user_suspend_blocker(struct file *file, void __user *name,
+				 size_t name_len)
+{
+	struct user_suspend_blocker *bl;
+	if (file->private_data)
+		return -EBUSY;
+	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+	if (copy_from_user(bl->name, name, name_len))
+		goto err_fault;
+	suspend_blocker_init(&bl->blocker, bl->name);
+	file->private_data = bl;
+	return 0;
+
+err_fault:
+	kfree(bl);
+	return -EFAULT;
+}
+
+static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
+				unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *bl;
+	long ret;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
+		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	bl = file->private_data;
+	if (!bl) {
+		ret = -ENOENT;
+		goto done;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&bl->blocker);
+		ret = 0;
+		break;
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&bl->blocker);
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTSUPP;
+	}
+done:
+	if (ret && debug_mask & DEBUG_FAILURE)
+		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
+			cmd, ret);
+	mutex_unlock(&ioctl_lock);
+	return ret;
+}
+
+static int user_suspend_blocker_release(struct inode *inode, struct file *file)
+{
+	struct user_suspend_blocker *bl = file->private_data;
+	if (!bl)
+		return 0;
+	suspend_blocker_destroy(&bl->blocker);
+	kfree(bl);
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.1

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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

Hi!

> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.

Rafael proposed write() interface. I don't think you answered that.


> +static int create_user_suspend_blocker(struct file *file, void __user *name,
> +				 size_t name_len)
> +{
> +	struct user_suspend_blocker *bl;
> +	if (file->private_data)
> +		return -EBUSY;
> +	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
> +	if (!bl)
> +		return -ENOMEM;
> +	if (copy_from_user(bl->name, name, name_len))
> +		goto err_fault;
> +	suspend_blocker_init(&bl->blocker, bl->name);
> +	file->private_data = bl;
> +	return 0;

kzalloc on user-supplied argument. Sounds like bad idea.

Aha. And probably integer overflow in the same line. Ouch.

Plus you actually 'trust' the string from userspace. Someone passes
"evdev" there and can masquerade as a part of kernel.

> +static int user_suspend_blocker_release(struct inode *inode, struct file *file)
> +{
> +	struct user_suspend_blocker *bl = file->private_data;
> +	if (!bl)
> +		return 0;
> +	suspend_blocker_destroy(&bl->blocker);
> +	kfree(bl);
> +	return 0;
> +}

What happens if someone does dup() on such file descriptor? 


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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> Add a misc device, "suspend_blocker", that allows user-space processes
>> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> and to block and unblock suspend. To delete the suspend_blocker, close
>> the device.
>
> Rafael proposed write() interface. I don't think you answered that.
>

I think an ioctl interface makes more sense. With a write interface
you either have to do string parsing, or invent some other protocol
between the driver and user-space.

>
>> +static int create_user_suspend_blocker(struct file *file, void __user *name,
>> +                              size_t name_len)
>> +{
>> +     struct user_suspend_blocker *bl;
>> +     if (file->private_data)
>> +             return -EBUSY;
>> +     bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
>> +     if (!bl)
>> +             return -ENOMEM;
>> +     if (copy_from_user(bl->name, name, name_len))
>> +             goto err_fault;
>> +     suspend_blocker_init(&bl->blocker, bl->name);
>> +     file->private_data = bl;
>> +     return 0;
>
> kzalloc on user-supplied argument. Sounds like bad idea.
>
> Aha. And probably integer overflow in the same line. Ouch.
>

The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
on the architecture. Do you want a lower limit on the name length?

> Plus you actually 'trust' the string from userspace. Someone passes
> "evdev" there and can masquerade as a part of kernel.

Yes. I could add a prefix to userspace names, but it did not seem
worth the effort since the name is not used as a key for anything.

>
>> +static int user_suspend_blocker_release(struct inode *inode, struct file *file)
>> +{
>> +     struct user_suspend_blocker *bl = file->private_data;
>> +     if (!bl)
>> +             return 0;
>> +     suspend_blocker_destroy(&bl->blocker);
>> +     kfree(bl);
>> +     return 0;
>> +}
>
> What happens if someone does dup() on such file descriptor?
>

Then you have two file descriptors pointing to the same suspend
blocker. Release is called when the last reference goes away.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

On Wed 2009-05-06 18:42:59, Arve Hj?nnev?g wrote:
> On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> Add a misc device, "suspend_blocker", that allows user-space processes
> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> and to block and unblock suspend. To delete the suspend_blocker, close
> >> the device.
> >
> > Rafael proposed write() interface. I don't think you answered that.
> >
> 
> I think an ioctl interface makes more sense. With a write interface
> you either have to do string parsing, or invent some other protocol
> between the driver and user-space.

Or perhaps just use "userspace/%d" % pid as a name?

1) can't be faked that trivially

2) small / mostly fixed size

3) can use nicer write protocol?

> > kzalloc on user-supplied argument. Sounds like bad idea.
> >
> > Aha. And probably integer overflow in the same line. Ouch.
> >
> 
> The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
> on the architecture. Do you want a lower limit on the name length?

16K of unswappable kernel memory for name is a bit too much, yes.

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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

On Thu, May 7, 2009 at 3:32 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2009-05-06 18:42:59, Arve Hj?nnev?g wrote:
>> On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> >> Add a misc device, "suspend_blocker", that allows user-space processes
>> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> >> and to block and unblock suspend. To delete the suspend_blocker, close
>> >> the device.
>> >
>> > Rafael proposed write() interface. I don't think you answered that.
>> >
>>
>> I think an ioctl interface makes more sense. With a write interface
>> you either have to do string parsing, or invent some other protocol
>> between the driver and user-space.
>
> Or perhaps just use "userspace/%d" % pid as a name?

This would not be as useful. The point of naming the suspend blockers
is to that we can easily identify them in the stats and kernel logs.
Using the pid has two problems. First, the pid gives no hint about
what it is used for, you have to look up the process somewhere else.
Second, we use more than one suspend blocker from the same process.

>
> 1) can't be faked that trivially

I don't think this is a big problem. If you don't trust the apps that
you give suspend blocker access to use unique names, we can add a
prefix. This can be added in a later patch though.

>
> 2) small / mostly fixed size
>
> 3) can use nicer write protocol?
>
I don't think a write protocol is nicer. "ioctl(suspend_block_fd,
SUSPEND_BLOCKER_IOCTL_BLOCK);" seems less error prone and more
readable to me than "write(suspend_block_fd, "1", 1);",
"write(suspend_block_fd, "1", 2);" or "suspend_block_val = 1;
write(suspend_block_fd, &suspend_block_val,
sizeof(suspend_block_val));".

>> > kzalloc on user-supplied argument. Sounds like bad idea.
>> >
>> > Aha. And probably integer overflow in the same line. Ouch.
>> >
>>
>> The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
>> on the architecture. Do you want a lower limit on the name length?
>
> 16K of unswappable kernel memory for name is a bit too much, yes.

What if I just truncate it to NAME_MAX.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Rafael J. Wysocki]

On Friday 08 May 2009, Arve Hjønnevåg wrote:
> On Thu, May 7, 2009 at 3:32 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Wed 2009-05-06 18:42:59, Arve Hj?nnev?g wrote:
> >> On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
> >> > Hi!
> >> >
> >> >> Add a misc device, "suspend_blocker", that allows user-space processes
> >> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> >> and to block and unblock suspend. To delete the suspend_blocker, close
> >> >> the device.
> >> >
> >> > Rafael proposed write() interface. I don't think you answered that.
> >> >
> >>
> >> I think an ioctl interface makes more sense. With a write interface
> >> you either have to do string parsing, or invent some other protocol
> >> between the driver and user-space.
> >
> > Or perhaps just use "userspace/%d" % pid as a name?
> 
> This would not be as useful. The point of naming the suspend blockers
> is to that we can easily identify them in the stats and kernel logs.
> Using the pid has two problems. First, the pid gives no hint about
> what it is used for, you have to look up the process somewhere else.
> Second, we use more than one suspend blocker from the same process.
> 
> >
> > 1) can't be faked that trivially
> 
> I don't think this is a big problem. If you don't trust the apps that
> you give suspend blocker access to use unique names, we can add a
> prefix. This can be added in a later patch though.
> 
> >
> > 2) small / mostly fixed size
> >
> > 3) can use nicer write protocol?
> >
> I don't think a write protocol is nicer. "ioctl(suspend_block_fd,
> SUSPEND_BLOCKER_IOCTL_BLOCK);" seems less error prone and more
> readable to me than "write(suspend_block_fd, "1", 1);",
> "write(suspend_block_fd, "1", 2);" or "suspend_block_val = 1;
> write(suspend_block_fd, &suspend_block_val,
> sizeof(suspend_block_val));".
> 
> >> > kzalloc on user-supplied argument. Sounds like bad idea.
> >> >
> >> > Aha. And probably integer overflow in the same line. Ouch.
> >> >
> >>
> >> The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
> >> on the architecture. Do you want a lower limit on the name length?
> >
> > 16K of unswappable kernel memory for name is a bit too much, yes.
> 
> What if I just truncate it to NAME_MAX.

My main concern with the ioctl() interface is that you're adding a device
file just in order to have the ioctl()s available.  Usually, however, a device
file's purpose is to implement file operations (open, close, read, write),
while ioctl()s are used for doing things that the file operations are not
suitable for.

So, if your device only implements open(), close() and ioctl(), this is an
indication that there's something wrong with the interface, because it doesn't
do what one would generally expect it to do (ie. smells like an abuse).  That's
why I think it would be better to use read() and write() instead of ioctl().

Of course, there also is a problem that people generally don't like adding new
ioctl()s without a _really_ good reason.

Thanks,
Rafael

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

2009/5/8 Rafael J. Wysocki <rjw@sisk.pl>:
> On Friday 08 May 2009, Arve Hjønnevåg wrote:
>> On Thu, May 7, 2009 at 3:32 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > On Wed 2009-05-06 18:42:59, Arve Hj?nnev?g wrote:
>> >> On Tue, May 5, 2009 at 1:12 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> >> > Hi!
>> >> >
>> >> >> Add a misc device, "suspend_blocker", that allows user-space processes
>> >> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> >> >> and to block and unblock suspend. To delete the suspend_blocker, close
>> >> >> the device.
>> >> >
>> >> > Rafael proposed write() interface. I don't think you answered that.
>> >> >
>> >>
>> >> I think an ioctl interface makes more sense. With a write interface
>> >> you either have to do string parsing, or invent some other protocol
>> >> between the driver and user-space.
>> >
>> > Or perhaps just use "userspace/%d" % pid as a name?
>>
>> This would not be as useful. The point of naming the suspend blockers
>> is to that we can easily identify them in the stats and kernel logs.
>> Using the pid has two problems. First, the pid gives no hint about
>> what it is used for, you have to look up the process somewhere else.
>> Second, we use more than one suspend blocker from the same process.
>>
>> >
>> > 1) can't be faked that trivially
>>
>> I don't think this is a big problem. If you don't trust the apps that
>> you give suspend blocker access to use unique names, we can add a
>> prefix. This can be added in a later patch though.
>>
>> >
>> > 2) small / mostly fixed size
>> >
>> > 3) can use nicer write protocol?
>> >
>> I don't think a write protocol is nicer. "ioctl(suspend_block_fd,
>> SUSPEND_BLOCKER_IOCTL_BLOCK);" seems less error prone and more
>> readable to me than "write(suspend_block_fd, "1", 1);",
>> "write(suspend_block_fd, "1", 2);" or "suspend_block_val = 1;
>> write(suspend_block_fd, &suspend_block_val,
>> sizeof(suspend_block_val));".
>>
>> >> > kzalloc on user-supplied argument. Sounds like bad idea.
>> >> >
>> >> > Aha. And probably integer overflow in the same line. Ouch.
>> >> >
>> >>
>> >> The size is limited to _IOC_SIZEMASK with is 13 or 14 bits depending
>> >> on the architecture. Do you want a lower limit on the name length?
>> >
>> > 16K of unswappable kernel memory for name is a bit too much, yes.
>>
>> What if I just truncate it to NAME_MAX.
>
> My main concern with the ioctl() interface is that you're adding a device
> file just in order to have the ioctl()s available.  Usually, however, a device
> file's purpose is to implement file operations (open, close, read, write),
> while ioctl()s are used for doing things that the file operations are not
> suitable for.

In this case, we are adding a device file to get close (release).

> So, if your device only implements open(), close() and ioctl(), this is an
> indication that there's something wrong with the interface, because it doesn't
> do what one would generally expect it to do (ie. smells like an abuse).  That's
> why I think it would be better to use read() and write() instead of ioctl().

It not not clear to me how putting a control interface on top of read
and write is any less abusive.

> Of course, there also is a problem that people generally don't like adding new
> ioctl()s without a _really_ good reason.

ioctls are better suited for issuing commands than read/write.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

On Tue 2009-05-05 21:18:42, Arve Hj??nnev??g wrote:
> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.
> 

> +static int create_user_suspend_blocker(struct file *file, void __user *name,
> +				 size_t name_len)
> +{
> +	struct user_suspend_blocker *bl;
> +	if (file->private_data)
> +		return -EBUSY;
> +	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
...
> +static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
> +				unsigned long _arg)
> +{
> +	void __user *arg = (void __user *)_arg;
> +	struct user_suspend_blocker *bl;
> +	long ret;
> +
> +	mutex_lock(&ioctl_lock);
> +	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
> +		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
> +		goto done;
> +	}

Wait a moment, wtf is this? Not one ioctl but one ioctl per length of
string?!

								Pavel

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

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 1:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2009-05-05 21:18:42, Arve Hj??nnev??g wrote:
>> Add a misc device, "suspend_blocker", that allows user-space processes
>> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> and to block and unblock suspend. To delete the suspend_blocker, close
>> the device.
>>
>
>> +static int create_user_suspend_blocker(struct file *file, void __user *name,
>> +                              size_t name_len)
>> +{
>> +     struct user_suspend_blocker *bl;
>> +     if (file->private_data)
>> +             return -EBUSY;
>> +     bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
> ...
>> +static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
>> +                             unsigned long _arg)
>> +{
>> +     void __user *arg = (void __user *)_arg;
>> +     struct user_suspend_blocker *bl;
>> +     long ret;
>> +
>> +     mutex_lock(&ioctl_lock);
>> +     if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
>> +             ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
>> +             goto done;
>> +     }
>
> Wait a moment, wtf is this? Not one ioctl but one ioctl per length of
> string?!

This is not uncommon. _IOC encodes the size of the argument, and if
this is not a fixed size, then the raw ioctl number change based on
the size passed in. Look at input.h is you want other examples of
this.

-- 
Arve Hjønnevåg

Re: [PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Pavel Machek]

On Wed 2009-05-06 18:31:36, Arve Hj?nnev?g wrote:
> On Tue, May 5, 2009 at 1:16 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > On Tue 2009-05-05 21:18:42, Arve Hj??nnev??g wrote:
> >> Add a misc device, "suspend_blocker", that allows user-space processes
> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> and to block and unblock suspend. To delete the suspend_blocker, close
> >> the device.
> >>
> >
> >> +static int create_user_suspend_blocker(struct file *file, void __user *name,
> >> +                              size_t name_len)
> >> +{
> >> +     struct user_suspend_blocker *bl;
> >> +     if (file->private_data)
> >> +             return -EBUSY;
> >> +     bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
> > ...
> >> +static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
> >> +                             unsigned long _arg)
> >> +{
> >> +     void __user *arg = (void __user *)_arg;
> >> +     struct user_suspend_blocker *bl;
> >> +     long ret;
> >> +
> >> +     mutex_lock(&ioctl_lock);
> >> +     if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
> >> +             ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
> >> +             goto done;
> >> +     }
> >
> > Wait a moment, wtf is this? Not one ioctl but one ioctl per length of
> > string?!
> 
> This is not uncommon. _IOC encodes the size of the argument, and if
> this is not a fixed size, then the raw ioctl number change based on
> the size passed in. Look at input.h is you want other examples of
> this.

Ok, I found few examples in code:

pavel@amd:/data/l/linux$ grep -ri IOCSIZE_MASK .
./arch/alpha/include/asm/ioctl.h:#define IOCSIZE_MASK	(_IOC_SIZEMASK
<< _IOC_SIZESHIFT)
./arch/mips/include/asm/ioctl.h:#define IOCSIZE_MASK	(_IOC_SIZEMASK
<< _IOC_SIZESHIFT)
./arch/sparc/include/asm/ioctl.h:#define IOCSIZE_MASK
(_IOC_XSIZEMASK << _IOC_SIZESHIFT)
./drivers/input/joydev.c:		if ((cmd & ~IOCSIZE_MASK) ==
JSIOCGNAME(0)) {
./drivers/message/fusion/mptctl.c:	if ((cmd & ~IOCSIZE_MASK) ==
(MPTIOCINFO & ~IOCSIZE_MASK)) {
./drivers/message/fusion/mptctl.c:	else if ((cmd & ~IOCSIZE_MASK)
== (HP_GETHOSTINFO & ~IOCSIZE_MASK))
./drivers/mtd/mtdchar.c:	size = (cmd & IOCSIZE_MASK) >>
IOCSIZE_SHIFT;
./drivers/pcmcia/pcmcia_ioctl.c:    size = (cmd & IOCSIZE_MASK) >>
IOCSIZE_SHIFT;
./include/asm-generic/ioctl.h:#define IOCSIZE_MASK	(_IOC_SIZEMASK
<< _IOC_SIZESHIFT)

I still do not like introducing 16000 ioctls, and I'm not sure how our
ioctl compatibility layer playes with this.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[RFC][PATCH 0/9] Suspend block api (version 2)

[Arve Hjønnevåg]

This patch series adds a suspend-block api that provides the same
functionality as the android wakelock api. This patch series has
some documentation and code style fixes and it now uses debugfs
for stats. It also fixes some bugs in the user-space interface.

--
Arve Hjønnevåg 

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

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

[Arve Hjønnevåg]

Adds /sys/power/request_state, a non-blocking interface that specifies
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/suspend-blockers.txt |   95 +++++++++++
 include/linux/suspend_blocker.h          |   60 +++++++
 kernel/power/Kconfig                     |   10 +
 kernel/power/Makefile                    |    1 +
 kernel/power/main.c                      |   60 +++++++
 kernel/power/power.h                     |    6 +
 kernel/power/suspend_blocker.c           |  269 ++++++++++++++++++++++++++++++
 7 files changed, 501 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/power/suspend-blockers.txt
 create mode 100755 include/linux/suspend_blocker.h
 create mode 100644 kernel/power/suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
new file mode 100644
index 0000000..9248aa9
--- /dev/null
+++ b/Documentation/power/suspend-blockers.txt
@@ -0,0 +1,95 @@
+Suspend blockers
+================
+
+Suspend blockers provide a mechanism for device drivers and user-space processes
+to prevent the system from entering suspend. Only suspend states requested
+through /sys/power/request_state are prevented. Writing to /sys/power/state
+will ignore suspend blockers, and sleep states entered from idle are unaffected.
+
+To use a suspend blocker, a device driver needs a struct suspend_blocker object
+that has to be initialized with suspend_blocker_init. When no longer needed,
+the suspend blocker must be destroyed with suspend_blocker_destroy.
+
+A suspend blocker is activated using suspend_block, which prevents the system
+from entering suspend until the suspend blocker is deactivated with
+suspend_unblock. Many suspend blockers may be used simultaneously, and the
+system is prevented from entering suspend as long as at least one of them is
+active.
+
+If the suspend operation has already started when calling suspend_block on a
+suspend_blocker, it will abort the suspend operation as long it has not already
+reached the sysdev suspend stage. This means that calling suspend_block from an
+interrupt handler or a freezeable thread always works, but if you call
+suspend_block from a sysdev suspend handler you must also return an error from
+that handler to abort a suspend sequence that was already started.
+
+In cell phones or other embedded systems where powering the screen is a
+significant drain on the battery, suspend blockers can be used to allow
+user-space to decide whether a keystroke received while the system is suspended
+should cause the screen to be turned back on or allow the system to go back into
+suspend. 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 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.
+
+                 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. 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..21689cd
--- /dev/null
+++ b/include/linux/suspend_blocker.h
@@ -0,0 +1,60 @@
+/* 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
+
+/**
+ * struct suspend_blocker - the basic suspend_blocker structure
+ * @flags:	Tracks initialized and active state.
+ * @name:	Name used for debugging.
+ *
+ * When a suspend_blocker is active it prevents the system from entering
+ * suspend.
+ *
+ * The suspend_blocker structure must be initialized by suspend_blocker_init()
+ */
+
+struct suspend_blocker {
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	atomic_t            flags;
+	const char         *name;
+#endif
+};
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+
+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 23bd4da..a3587d3 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -116,6 +116,16 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config SUSPEND_BLOCKERS
+	bool "Suspend blockers"
+	depends on PM
+	select RTC_LIB
+	default n
+	---help---
+	  Enable suspend blockers. When user space requests a sleep state
+	  through /sys/power/request_state, the requested sleep state will be
+	  entered when no suspend blockers are active.
+
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 720ea4f..e8c7f85 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -6,6 +6,7 @@ endif
 obj-$(CONFIG_PM)		+= main.o
 obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
+obj-$(CONFIG_SUSPEND_BLOCKERS)	+= suspend_blocker.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o disk.o snapshot.o swap.o user.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/main.c b/kernel/power/main.c
index c9632f8..4681c1e 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -22,6 +22,7 @@
 #include <linux/freezer.h>
 #include <linux/vmstat.h>
 #include <linux/syscalls.h>
+#include <linux/suspend_blocker.h>
 
 #include "power.h"
 
@@ -392,6 +393,7 @@ static void suspend_finish(void)
 
 
 static const char * const pm_states[PM_SUSPEND_MAX] = {
+	[PM_SUSPEND_ON]		= "on",
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
 };
@@ -540,6 +542,61 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(state);
 
+/**
+ *	request_state - control system power state.
+ *
+ *	This is similar to state, but it does not block until the system
+ *	resumes, and it will try to re-enter the state until another state is
+ *	requested. Suspend blockers are respected and the requested state will
+ *	only be entered when no suspend blockers are active.
+ *	Write "on" to cancel.
+ */
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+static ssize_t request_state_show(struct kobject *kobj,
+				  struct kobj_attribute *attr, char *buf)
+{
+	char *s = buf;
+	int i;
+
+	for (i = 0; i < PM_SUSPEND_MAX; i++) {
+		if (pm_states[i] && (i == PM_SUSPEND_ON || valid_state(i)))
+			s += sprintf(s, "%s ", pm_states[i]);
+	}
+	if (s != buf)
+		/* convert the last space to a newline */
+		*(s-1) = '\n';
+	return (s - buf);
+}
+
+static ssize_t request_state_store(struct kobject *kobj,
+				   struct kobj_attribute *attr,
+				   const char *buf, size_t n)
+{
+	suspend_state_t state = PM_SUSPEND_ON;
+	const char * const *s;
+	char *p;
+	int len;
+	int error = -EINVAL;
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	for (s = &pm_states[state]; state < PM_SUSPEND_MAX; s++, state++) {
+		if (*s && len == strlen(*s) && !strncmp(buf, *s, len))
+			break;
+	}
+	if (state < PM_SUSPEND_MAX && *s)
+		if (state == PM_SUSPEND_ON || valid_state(state)) {
+			error = 0;
+			request_suspend_state(state);
+		}
+	return error ? error : n;
+}
+
+power_attr(request_state);
+#endif /* CONFIG_SUSPEND_BLOCKERS */
+
 #ifdef CONFIG_PM_TRACE
 int pm_trace_enabled;
 
@@ -567,6 +624,9 @@ power_attr(pm_trace);
 
 static struct attribute * g[] = {
 	&state_attr.attr,
+#ifdef CONFIG_SUSPEND_BLOCKERS
+	&request_state_attr.attr,
+#endif
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 46b5ec7..609e01e 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -223,3 +223,9 @@ static inline void suspend_thaw_processes(void)
 {
 }
 #endif
+
+#ifdef CONFIG_SUSPEND_BLOCKERS
+/* kernel/power/suspend_block.c */
+void request_suspend_state(suspend_state_t state);
+#endif
+
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
new file mode 100644
index 0000000..642b0db
--- /dev/null
+++ b/kernel/power/suspend_blocker.c
@@ -0,0 +1,269 @@
+/* kernel/power/suspend_blocker.c
+ *
+ * Copyright (C) 2005-2008 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/suspend.h>
+#include <linux/suspend_blocker.h>
+#include <linux/sysdev.h>
+#include "power.h"
+
+enum {
+	DEBUG_EXIT_SUSPEND = 1U << 0,
+	DEBUG_WAKEUP = 1U << 1,
+	DEBUG_USER_STATE = 1U << 2,
+	DEBUG_SUSPEND = 1U << 3,
+	DEBUG_SUSPEND_BLOCKER = 1U << 4,
+};
+static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+#define SB_INITIALIZED            (1U << 8)
+#define SB_ACTIVE                 (1U << 9)
+
+static DEFINE_SPINLOCK(state_lock);
+static atomic_t suspend_block_count;
+static atomic_t current_event_num;
+struct workqueue_struct *suspend_work_queue;
+struct suspend_blocker main_suspend_blocker;
+static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
+static bool enable_suspend_blockers;
+
+#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);
+
+/**
+ * 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 !!atomic_read(&suspend_block_count);
+}
+
+static void suspend_worker(struct work_struct *work)
+{
+	int ret;
+	int entry_event_num;
+
+	enable_suspend_blockers = true;
+
+retry:
+	if (suspend_is_blocked()) {
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: abort suspend\n");
+		goto abort;
+	}
+
+	entry_event_num = atomic_read(&current_event_num);
+	if (debug_mask & DEBUG_SUSPEND)
+		pr_info("suspend: enter suspend\n");
+	ret = pm_suspend(requested_suspend_state);
+	if (debug_mask & DEBUG_EXIT_SUSPEND)
+		pr_info_time("suspend: exit suspend, ret = %d ", ret);
+	if (atomic_read(&current_event_num) == entry_event_num) {
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: pm_suspend returned with no event\n");
+		goto retry;
+	}
+abort:
+	enable_suspend_blockers = false;
+}
+static DECLARE_WORK(suspend_work, suspend_worker);
+
+static int suspend_block_suspend(struct sys_device *dev, pm_message_t state)
+{
+	int ret = suspend_is_blocked() ? -EAGAIN : 0;
+	if (debug_mask & DEBUG_SUSPEND)
+		pr_info("suspend_block_suspend return %d\n", ret);
+	return ret;
+}
+
+static struct sysdev_class suspend_block_sysclass = {
+	.name = "suspend_block",
+	.suspend = suspend_block_suspend,
+};
+static struct sys_device suspend_block_sysdev = {
+	.cls = &suspend_block_sysclass,
+};
+
+/**
+ * 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)
+{
+	WARN_ON(!name);
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_init name=%s\n", name);
+
+	blocker->name = name;
+	atomic_set(&blocker->flags, SB_INITIALIZED);
+}
+EXPORT_SYMBOL(suspend_blocker_init);
+
+/**
+ * suspend_blocker_destroy() - Destroy a suspend blocker
+ * @blocker:	The suspend blocker to destroy.
+ */
+void suspend_blocker_destroy(struct suspend_blocker *blocker)
+{
+	int flags;
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
+	flags = atomic_xchg(&blocker->flags, 0);
+	WARN(!(flags & SB_INITIALIZED), "suspend_blocker_destroy called on "
+					"uninitialized suspend_blocker\n");
+	if (flags == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_blocker_destroy);
+
+/**
+ * suspend_block() - Block suspend
+ * @blocker:	The suspend blocker to use
+ */
+void suspend_block(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_block: %s\n", blocker->name);
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED,
+	    SB_INITIALIZED | SB_ACTIVE) == SB_INITIALIZED)
+		atomic_inc(&suspend_block_count);
+
+	atomic_inc(&current_event_num);
+}
+EXPORT_SYMBOL(suspend_block);
+
+/**
+ * suspend_unblock() - Unblock suspend
+ * @blocker:	The suspend blocker to unblock.
+ *
+ * If no other suspend blockers block suspend, the system will suspend.
+ */
+void suspend_unblock(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_unblock: %s\n", blocker->name);
+
+	if (atomic_cmpxchg(&blocker->flags, SB_INITIALIZED | SB_ACTIVE,
+	    SB_INITIALIZED) == (SB_INITIALIZED | SB_ACTIVE))
+		if (atomic_dec_and_test(&suspend_block_count))
+			queue_work(suspend_work_queue, &suspend_work);
+}
+EXPORT_SYMBOL(suspend_unblock);
+
+/**
+ * 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(!(atomic_read(&blocker->flags) & SB_INITIALIZED));
+
+	return !!(atomic_read(&blocker->flags) & SB_ACTIVE);
+}
+EXPORT_SYMBOL(suspend_blocker_is_active);
+
+void request_suspend_state(suspend_state_t state)
+{
+	unsigned long irqflags;
+	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);
+}
+
+static int __init suspend_block_init(void)
+{
+	int ret;
+
+	suspend_blocker_init(&main_suspend_blocker, "main");
+	suspend_block(&main_suspend_blocker);
+
+	ret = sysdev_class_register(&suspend_block_sysclass);
+	if (ret) {
+		pr_err("suspend_block_init: sysdev_class_register failed\n");
+		goto err_sysdev_class_register;
+	}
+	ret = sysdev_register(&suspend_block_sysdev);
+	if (ret) {
+		pr_err("suspend_block_init: suspend_block_sysdev failed\n");
+		goto err_sysdev_register;
+	}
+
+	suspend_work_queue = create_singlethread_workqueue("suspend");
+	if (suspend_work_queue == NULL) {
+		ret = -ENOMEM;
+		goto err_suspend_work_queue;
+	}
+
+	return 0;
+
+err_suspend_work_queue:
+	sysdev_unregister(&suspend_block_sysdev);
+err_sysdev_register:
+	sysdev_class_unregister(&suspend_block_sysclass);
+err_sysdev_class_register:
+	suspend_blocker_destroy(&main_suspend_blocker);
+	return ret;
+}
+
+static void  __exit suspend_block_exit(void)
+{
+	destroy_workqueue(suspend_work_queue);
+	sysdev_unregister(&suspend_block_sysdev);
+	sysdev_class_unregister(&suspend_block_sysclass);
+	suspend_blocker_destroy(&main_suspend_blocker);
+}
+
+core_initcall(suspend_block_init);
+module_exit(suspend_block_exit);
-- 
1.6.1

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

[PATCH 2/9] PM: suspend_block: Add driver to access suspend blockers from user-space

[Arve Hjønnevåg]

Add a misc device, "suspend_blocker", that allows user-space processes
to block auto suspend. The device has ioctls to create a suspend_blocker,
and to block and unblock suspend. To delete the suspend_blocker, close
the device.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |   17 ++++
 include/linux/suspend_block_dev.h        |   25 ++++++
 kernel/power/Kconfig                     |    9 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/user_suspend_blocker.c      |  125 ++++++++++++++++++++++++++++++
 5 files changed, 177 insertions(+), 0 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 9248aa9..d81e20e 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -93,3 +93,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..a7c0bf9
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
+#define _LINUX_SUSPEND_BLOCK_DEV_H
+
+#include <linux/ioctl.h>
+
+#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 3)
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index a3587d3..84b423f 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -126,6 +126,15 @@ config SUSPEND_BLOCKERS
 	  through /sys/power/request_state, the requested sleep state will be
 	  entered when no suspend blockers are active.
 
+config USER_SUSPEND_BLOCKERS
+	bool "Userspace suspend blockers"
+	depends on SUSPEND_BLOCKERS
+	default y
+	---help---
+	  User-space suspend block api. Creates a misc device with ioctls
+	  to create, block and unblock a suspend_blocker. The suspend_blocker
+	  will be deleted when the device is closed.
+
 config HIBERNATION
 	bool "Hibernation (aka 'suspend to disk')"
 	depends on PM && SWAP && ARCH_HIBERNATION_POSSIBLE
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index e8c7f85..24ade91 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_BLOCKERS)	+= suspend_blocker.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_HIBERNATION)	+= swsusp.o disk.o snapshot.o swap.o user.o
 
 obj-$(CONFIG_MAGIC_SYSRQ)	+= poweroff.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..69b3ddb
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,125 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 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.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/suspend_blocker.h>
+#include <linux/suspend_block_dev.h>
+
+enum {
+	DEBUG_FAILURE	= BIT(0),
+};
+static int debug_mask = DEBUG_FAILURE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+static DEFINE_MUTEX(ioctl_lock);
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[0];
+};
+
+static int create_user_suspend_blocker(struct file *file, void __user *name,
+				 size_t name_len)
+{
+	struct user_suspend_blocker *bl;
+	if (file->private_data)
+		return -EBUSY;
+	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+	if (copy_from_user(bl->name, name, name_len))
+		goto err_fault;
+	suspend_blocker_init(&bl->blocker, bl->name);
+	file->private_data = bl;
+	return 0;
+
+err_fault:
+	kfree(bl);
+	return -EFAULT;
+}
+
+static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
+				unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *bl;
+	long ret;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
+		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	bl = file->private_data;
+	if (!bl) {
+		ret = -ENOENT;
+		goto done;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&bl->blocker);
+		ret = 0;
+		break;
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&bl->blocker);
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTSUPP;
+	}
+done:
+	if (ret && debug_mask & DEBUG_FAILURE)
+		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
+			cmd, ret);
+	mutex_unlock(&ioctl_lock);
+	return ret;
+}
+
+static int user_suspend_blocker_release(struct inode *inode, struct file *file)
+{
+	struct user_suspend_blocker *bl = file->private_data;
+	if (!bl)
+		return 0;
+	suspend_blocker_destroy(&bl->blocker);
+	kfree(bl);
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.1

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