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

[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

[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 |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/kernel/power/process.c b/kernel/power/process.c
index ca63401..8c7c323 100644
--- a/kernel/power/process.c
+++ b/kernel/power/process.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/syscalls.h>
 #include <linux/freezer.h>
+#include <linux/suspend_blocker.h>
 
 /* 
  * Timeout for stopping processes
@@ -36,6 +37,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);
 
@@ -62,6 +64,10 @@ static int try_to_freeze_tasks(bool sig_only)
 		} while_each_thread(g, p);
 		read_unlock(&tasklist_lock);
 		yield();			/* Yield is okay here */
+		if (todo && suspend_is_blocked()) {
+			wakeup = true;
+			break;
+		}
 		if (time_after(jiffies, end_time))
 			break;
 	} while (todo);
@@ -78,10 +84,16 @@ 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 "
-				"(%d tasks refusing to freeze):\n",
-				elapsed_csecs / 100, elapsed_csecs % 100, todo);
-		show_state();
+		if (wakeup) {
+			printk(KERN_ERR "Freezing of %s aborted\n",
+					sig_only ? "user space " : "tasks ");
+		} else {
+			printk(KERN_ERR
+			       "Freezing of tasks failed after %d.%02d seconds "
+			       "(%d tasks refusing to freeze):\n",
+			       elapsed_csecs / 100, elapsed_csecs % 100, todo);
+			show_state();
+		}
 		read_lock(&tasklist_lock);
 		do_each_thread(g, p) {
 			task_lock(p);
-- 
1.6.1

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

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

[Pavel Machek]

Hi!

> 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

Please fix try_to_freeze not to take significant time rather than
this...

								Pavel

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

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

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 12:57 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> 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
>
> Please fix try_to_freeze not to take significant time rather than
> this...

I though we covered this already. Most of the 20 second delays we saw
on earlier kernel version have been fixed, but this code can run for
20 seconds which is an unacceptable wakeup delay. Any uninterruptible
sleep in the kernel can delay the task freezing and there is no point
in completing this when we know suspend will fail.

-- 
Arve Hjønnevåg

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

[Pavel Machek]

On Wed 2009-05-06 18:51:11, Arve Hj?nnev?g wrote:
> On Tue, May 5, 2009 at 12:57 PM, Pavel Machek <pavel@ucw.cz> wrote:
> > Hi!
> >
> >> 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
> >
> > Please fix try_to_freeze not to take significant time rather than
> > this...
> 
> I though we covered this already. Most of the 20 second delays we saw
> on earlier kernel version have been fixed, but this code can run for
> 20 seconds which is an unacceptable wakeup delay. Any
> uninterruptible

If the code runs for 20 seconds, it is a bug to be fixed.

IOW the patch should not be needed when the bugs are fixed. You should
not need it now.

Can you drop it for now (or keep it in private tree or something). If
you have scenario where it takes significant time and that can't be
fixed, please present the scenario and we can reconsider it.

For now, it just makes stuff harder to review.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Arve Hjønnevåg]

On Thu, May 7, 2009 at 3:41 AM, Pavel Machek <pavel@ucw.cz> wrote:
> On Wed 2009-05-06 18:51:11, Arve Hj?nnev?g wrote:
>> On Tue, May 5, 2009 at 12:57 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> > Hi!
>> >
>> >> 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
>> >
>> > Please fix try_to_freeze not to take significant time rather than
>> > this...
>>
>> I though we covered this already. Most of the 20 second delays we saw
>> on earlier kernel version have been fixed, but this code can run for
>> 20 seconds which is an unacceptable wakeup delay. Any
>> uninterruptible
>
> If the code runs for 20 seconds, it is a bug to be fixed.

The code gives up after 20 seconds, it does not normally run for 20
seconds. It is arguably a bug that it gives up after 20 seconds since
the time required to freeze all the threads grows with the number of
threads that are running. It could still be making progress after 20
seconds. Since the time required to freeze all tasks is the number of
tasks times the time it takes to interrupt each task there is no way
to ensure that the time required is insignificant. If we do not abort
task freezing when there is a wakeup event, then the worst case wakeup
latency is guarantied to be worse than the worst case latency for any
other uninterruptible kernel call.

>
> IOW the patch should not be needed when the bugs are fixed. You should
> not need it now.
>
> Can you drop it for now (or keep it in private tree or something). If
> you have scenario where it takes significant time and that can't be
> fixed, please present the scenario and we can reconsider it.
>

It can take a significant amount of time. It has to wait for every
thread it signalled to freeze which seems to be every user-space
thread. On my device, with 200 threads, this took less than 10ms half
the time, up to 460ms in the worst case.

> For now, it just makes stuff harder to review.

I think this patch is needed whether there are bugs or not, to ensure
low latency wakeup.

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


-- 
Arve Hjønnevåg

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

[Nigel Cunningham]

Hi.

On Thu, 2009-05-07 at 16:49 -0700, Arve Hjønnevåg wrote:
> On Thu, May 7, 2009 at 3:41 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > If the code runs for 20 seconds, it is a bug to be fixed.
> 
> The code gives up after 20 seconds, it does not normally run for 20
> seconds. It is arguably a bug that it gives up after 20 seconds since
> the time required to freeze all the threads grows with the number of
> threads that are running. It could still be making progress after 20
> seconds. Since the time required to freeze all tasks is the number of
> tasks times the time it takes to interrupt each task there is no way
> to ensure that the time required is insignificant. If we do not abort
> task freezing when there is a wakeup event, then the worst case wakeup
> latency is guarantied to be worse than the worst case latency for any
> other uninterruptible kernel call.

I agree with Pavel here. If freezing takes 20 seconds, something is
wrong. (Remember that most tasks will not be running, and will therefore
respond to the pseudo-signal and freeze immediately).

In fact, I'd go further. In the thousands of times I've run the freezer
over the years, it has never taken more than 1 second - let alone 20 -
when freezing has been successful. A delay of 20 seconds was more
relevant when the value included the time for syncing data to disk.

Regards,

Nigel

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

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

[Arve Hjønnevåg]

2009/5/7 Nigel Cunningham <ncunningham@crca.org.au>:
> Hi.
>
> On Thu, 2009-05-07 at 16:49 -0700, Arve Hjønnevåg wrote:
>> On Thu, May 7, 2009 at 3:41 AM, Pavel Machek <pavel@ucw.cz> wrote:
>> > If the code runs for 20 seconds, it is a bug to be fixed.
>>
>> The code gives up after 20 seconds, it does not normally run for 20
>> seconds. It is arguably a bug that it gives up after 20 seconds since
>> the time required to freeze all the threads grows with the number of
>> threads that are running. It could still be making progress after 20
>> seconds. Since the time required to freeze all tasks is the number of
>> tasks times the time it takes to interrupt each task there is no way
>> to ensure that the time required is insignificant. If we do not abort
>> task freezing when there is a wakeup event, then the worst case wakeup
>> latency is guarantied to be worse than the worst case latency for any
>> other uninterruptible kernel call.
>
> I agree with Pavel here. If freezing takes 20 seconds, something is
> wrong. (Remember that most tasks will not be running, and will therefore
> respond to the pseudo-signal and freeze immediately).
>
> In fact, I'd go further. In the thousands of times I've run the freezer
> over the years, it has never taken more than 1 second - let alone 20 -

One second is to long.

> when freezing has been successful. A delay of 20 seconds was more
> relevant when the value included the time for syncing data to disk.

This patch does nothing with the 20 second timeout.

-- 
Arve Hjønnevåg

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

[Nigel Cunningham]

Hi.

On Thu, 2009-05-07 at 18:22 -0700, Arve Hjønnevåg wrote:
> 2009/5/7 Nigel Cunningham <ncunningham@crca.org.au>:
> > Hi.
> >
> > On Thu, 2009-05-07 at 16:49 -0700, Arve Hjønnevåg wrote:
> >> On Thu, May 7, 2009 at 3:41 AM, Pavel Machek <pavel@ucw.cz> wrote:
> >> > If the code runs for 20 seconds, it is a bug to be fixed.
> >>
> >> The code gives up after 20 seconds, it does not normally run for 20
> >> seconds. It is arguably a bug that it gives up after 20 seconds since
> >> the time required to freeze all the threads grows with the number of
> >> threads that are running. It could still be making progress after 20
> >> seconds. Since the time required to freeze all tasks is the number of
> >> tasks times the time it takes to interrupt each task there is no way
> >> to ensure that the time required is insignificant. If we do not abort
> >> task freezing when there is a wakeup event, then the worst case wakeup
> >> latency is guarantied to be worse than the worst case latency for any
> >> other uninterruptible kernel call.
> >
> > I agree with Pavel here. If freezing takes 20 seconds, something is
> > wrong. (Remember that most tasks will not be running, and will therefore
> > respond to the pseudo-signal and freeze immediately).
> >
> > In fact, I'd go further. In the thousands of times I've run the freezer
> > over the years, it has never taken more than 1 second - let alone 20 -
> 
> One second is to long.

I agree. But a one second timeout is still better than a 20s timeout.

> > when freezing has been successful. A delay of 20 seconds was more
> > relevant when the value included the time for syncing data to disk.
> 
> This patch does nothing with the 20 second timeout.

I know. Perhaps we should do something with it, though.

Nigel

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

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

[Rafael J. Wysocki]

On Friday 08 May 2009, Nigel Cunningham wrote:
> Hi.
> 
> On Thu, 2009-05-07 at 18:22 -0700, Arve Hjønnevåg wrote:
> > 2009/5/7 Nigel Cunningham <ncunningham@crca.org.au>:
> > > Hi.
> > >
> > > On Thu, 2009-05-07 at 16:49 -0700, Arve Hjønnevåg wrote:
> > >> On Thu, May 7, 2009 at 3:41 AM, Pavel Machek <pavel@ucw.cz> wrote:
> > >> > If the code runs for 20 seconds, it is a bug to be fixed.
> > >>
> > >> The code gives up after 20 seconds, it does not normally run for 20
> > >> seconds. It is arguably a bug that it gives up after 20 seconds since
> > >> the time required to freeze all the threads grows with the number of
> > >> threads that are running. It could still be making progress after 20
> > >> seconds. Since the time required to freeze all tasks is the number of
> > >> tasks times the time it takes to interrupt each task there is no way
> > >> to ensure that the time required is insignificant. If we do not abort
> > >> task freezing when there is a wakeup event, then the worst case wakeup
> > >> latency is guarantied to be worse than the worst case latency for any
> > >> other uninterruptible kernel call.
> > >
> > > I agree with Pavel here. If freezing takes 20 seconds, something is
> > > wrong. (Remember that most tasks will not be running, and will therefore
> > > respond to the pseudo-signal and freeze immediately).
> > >
> > > In fact, I'd go further. In the thousands of times I've run the freezer
> > > over the years, it has never taken more than 1 second - let alone 20 -
> > 
> > One second is to long.
> 
> I agree. But a one second timeout is still better than a 20s timeout.
> 
> > > when freezing has been successful. A delay of 20 seconds was more
> > > relevant when the value included the time for syncing data to disk.
> > 
> > This patch does nothing with the 20 second timeout.
> 
> I know. Perhaps we should do something with it, though.

I tried some time ago, but it didn't really work well.

The timeout is actually a workaround for the problem that we don't really
know if tasks are going to react to our freeze requests and how much time it is
going to take.  The current value of 20 s was chosen after a number of
experiments showing that in some cases the freezing _was_ going to take so
much time.  Of course the question is whether it makes sense to give up earlier
even if tasks would eventually freeze, but that's a different issue.

Now, I think the Arve's approach is reasonable.  If we know in advace that
we're not going to suspend, it's better to stop the freezing as soon as
possible.

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

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

[Nigel Cunningham]

Hi.

On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> The timeout is actually a workaround for the problem that we don't
> really
> know if tasks are going to react to our freeze requests and how much time it is
> going to take.  The current value of 20 s was chosen after a number of
> experiments showing that in some cases the freezing _was_ going to take so
> much time.  Of course the question is whether it makes sense to give up earlier
> even if tasks would eventually freeze, but that's a different issue.

What were the circumstances in which freezing could take 20s?

> Now, I think the Arve's approach is reasonable.  If we know in advace that
> we're not going to suspend, it's better to stop the freezing as soon as
> possible.

Agreed.

Regards,

Nigel

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

[Rafael J. Wysocki]

On Saturday 09 May 2009, Nigel Cunningham wrote:
> Hi.
> 
> On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> > The timeout is actually a workaround for the problem that we don't
> > really
> > know if tasks are going to react to our freeze requests and how much time it is
> > going to take.  The current value of 20 s was chosen after a number of
> > experiments showing that in some cases the freezing _was_ going to take so
> > much time.  Of course the question is whether it makes sense to give up earlier
> > even if tasks would eventually freeze, but that's a different issue.
> 
> What were the circumstances in which freezing could take 20s?

Compiling the kernel with "make -j<big number>" on a dual core system IIRC.

Generally, the situation in which there are many runnable tasks and new tasks
come and go etc.  I haven't repeated this benchmark recently, though, so it's
probably worth doing again.

Which has a little to do with the $subject patch IMO, because I think that
it's reasonable to stop freezing tasks as soon as we know that it won't be
necessary anyway.

Thanks,
Rafael

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

[Nigel Cunningham]

Hi.

On Sat, 2009-05-09 at 01:01 +0200, Rafael J. Wysocki wrote:
> On Saturday 09 May 2009, Nigel Cunningham wrote:
> > Hi.
> > 
> > On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> > > The timeout is actually a workaround for the problem that we don't
> > > really
> > > know if tasks are going to react to our freeze requests and how much time it is
> > > going to take.  The current value of 20 s was chosen after a number of
> > > experiments showing that in some cases the freezing _was_ going to take so
> > > much time.  Of course the question is whether it makes sense to give up earlier
> > > even if tasks would eventually freeze, but that's a different issue.
> > 
> > What were the circumstances in which freezing could take 20s?
> 
> Compiling the kernel with "make -j<big number>" on a dual core system IIRC.
> 
> Generally, the situation in which there are many runnable tasks and new tasks
> come and go etc.  I haven't repeated this benchmark recently, though, so it's
> probably worth doing again.
> 
> Which has a little to do with the $subject patch IMO, because I think that
> it's reasonable to stop freezing tasks as soon as we know that it won't be
> necessary anyway.

Yes, but it's still good to know.

Regards,

Nigel

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

[Pavel Machek]

On Sat 2009-05-09 01:01:01, Rafael J. Wysocki wrote:
> On Saturday 09 May 2009, Nigel Cunningham wrote:
> > Hi.
> > 
> > On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> > > The timeout is actually a workaround for the problem that we don't
> > > really
> > > know if tasks are going to react to our freeze requests and how much time it is
> > > going to take.  The current value of 20 s was chosen after a number of
> > > experiments showing that in some cases the freezing _was_ going to take so
> > > much time.  Of course the question is whether it makes sense to give up earlier
> > > even if tasks would eventually freeze, but that's a different issue.
> > 
> > What were the circumstances in which freezing could take 20s?
> 
> Compiling the kernel with "make -j<big number>" on a dual core system IIRC.

IIRC the number happened because I saw up to 5sec under some heavy
overload, and added some "safety margin".
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Rafael J. Wysocki]

On Tuesday 12 May 2009, Pavel Machek wrote:
> On Sat 2009-05-09 01:01:01, Rafael J. Wysocki wrote:
> > On Saturday 09 May 2009, Nigel Cunningham wrote:
> > > Hi.
> > > 
> > > On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> > > > The timeout is actually a workaround for the problem that we don't
> > > > really
> > > > know if tasks are going to react to our freeze requests and how much time it is
> > > > going to take.  The current value of 20 s was chosen after a number of
> > > > experiments showing that in some cases the freezing _was_ going to take so
> > > > much time.  Of course the question is whether it makes sense to give up earlier
> > > > even if tasks would eventually freeze, but that's a different issue.
> > > 
> > > What were the circumstances in which freezing could take 20s?
> > 
> > Compiling the kernel with "make -j<big number>" on a dual core system IIRC.
> 
> IIRC the number happened because I saw up to 5sec under some heavy
> overload, and added some "safety margin".

No, it appeared after a number of experiments I carried out a couple of years
ago. :-)

Best,
Rafael

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

[Pavel Machek]

On Tue 2009-05-12 18:55:36, Rafael J. Wysocki wrote:
> On Tuesday 12 May 2009, Pavel Machek wrote:
> > On Sat 2009-05-09 01:01:01, Rafael J. Wysocki wrote:
> > > On Saturday 09 May 2009, Nigel Cunningham wrote:
> > > > Hi.
> > > > 
> > > > On Fri, 2009-05-08 at 16:40 +0200, Rafael J. Wysocki wrote:
> > > > > The timeout is actually a workaround for the problem that we don't
> > > > > really
> > > > > know if tasks are going to react to our freeze requests and how much time it is
> > > > > going to take.  The current value of 20 s was chosen after a number of
> > > > > experiments showing that in some cases the freezing _was_ going to take so
> > > > > much time.  Of course the question is whether it makes sense to give up earlier
> > > > > even if tasks would eventually freeze, but that's a different issue.
> > > > 
> > > > What were the circumstances in which freezing could take 20s?
> > > 
> > > Compiling the kernel with "make -j<big number>" on a dual core system IIRC.
> > 
> > IIRC the number happened because I saw up to 5sec under some heavy
> > overload, and added some "safety margin".
> 
> No, it appeared after a number of experiments I carried out a couple of years
> ago. :-)

Ok, that was the "IIRC" part :-).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

[Pavel Machek]

Hi!

> > IOW the patch should not be needed when the bugs are fixed. You should
> > not need it now.
> >
> > Can you drop it for now (or keep it in private tree or something). If
> > you have scenario where it takes significant time and that can't be
> > fixed, please present the scenario and we can reconsider it.
> >
> 
> It can take a significant amount of time. It has to wait for every
> thread it signalled to freeze which seems to be every user-space
> thread. On my device, with 200 threads, this took less than 10ms half
> the time, up to 460ms in the worst case.

Ok, 460ms in the worst case is good reason to apply this patch, I
agree.
									Pavel

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

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

[Rafael J. Wysocki]

On Tuesday 05 May 2009, Pavel Machek wrote:
> Hi!
> 
> > 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
> 
> Please fix try_to_freeze not to take significant time rather than
> this...

FYI, I agree with this patch.

Best,
Rafael

[PATCH 4/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 7a7a026..0bb670f 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -19,6 +19,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);
@@ -233,6 +238,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);
@@ -332,6 +339,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);
@@ -582,6 +591,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 6b28048..e091ea8 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -81,6 +81,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.1

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

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

[Pavel Machek]

> 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>

Looks ok to  me, but I guess you should cc input maintainers.

> @@ -582,6 +591,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;

This returns 0 even when you destroy already destroyed blocker
(etc). Is that ok?



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

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

[Arve Hjønnevåg]

On Tue, May 5, 2009 at 1:02 PM, Pavel Machek <pavel@ucw.cz> 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.
>>
>> Signed-off-by: Arve Hj??nnev??g <arve@android.com>
>
> Looks ok to  me, but I guess you should cc input maintainers.

OK.

>
>> @@ -582,6 +591,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;
>
> This returns 0 even when you destroy already destroyed blocker
> (etc). Is that ok?

Yes. If events did not block suspend before the call, they still do
not block suspend after the call, so the requested state was reached.

-- 
Arve Hjønnevåg

[PATCH 5/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 6b9ca2d..cab9e9a 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.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 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 cab9e9a..ce87c65 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.
  */
@@ -230,6 +247,19 @@ void request_suspend_state(suspend_state_t state)
 	spin_unlock_irqrestore(&state_lock, irqflags);
 }
 
+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");
@@ -241,4 +271,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.1

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

[PATCH 7/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/main.c             |    4 +-
 kernel/power/power.h            |    6 +-
 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 84b423f..44bfaeb 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -126,6 +126,13 @@ config SUSPEND_BLOCKERS
 	  through /sys/power/request_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/main.c b/kernel/power/main.c
index 1899cda..022acca 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -322,8 +322,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/power.h b/kernel/power/power.h
index 2246b3b..5c30fb4 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -226,4 +226,8 @@ static inline void suspend_thaw_processes(void)
 
 /* kernel/power/suspend_block.c */
 void request_suspend_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_blocker.c b/kernel/power/suspend_blocker.c
index ce87c65..b5ce295 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);
 }
@@ -268,6 +439,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.1

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

[PATCH 8/9] PM: suspend_block: Add timeout support.

[Arve Hjønnevåg]

Add suspend_block_timeout to block suspend for a limited time.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |   10 ++
 include/linux/suspend_blocker.h          |    7 +-
 kernel/power/suspend_blocker.c           |  235 +++++++++++++++++++++++++----
 3 files changed, 218 insertions(+), 34 deletions(-)

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 9d78d4e..bc9d57a 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -92,6 +92,16 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+A driver can also call suspend_block_timeout to release the suspend_blocker
+after a delay:
+	suspend_block_timeout(&state->suspend_blocker, HZ);
+
+This works whether the suspend_blocker is already active or not. It is useful if
+the driver woke up other parts of the system that do not use suspend_blockers
+but still need to run. Avoid this when possible, since it will waste power
+if the timeout is long or may fail to finish needed work if the timeout is
+short. Calling suspend_block or suspend_unblock will cancel the timeout.
+
 User-space API
 ==============
 
diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index 3bb8a6a..e3ae136 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -22,8 +22,9 @@
 /**
  * struct suspend_blocker - the basic suspend_blocker structure
  * @link:	List entry for active or inactive list.
- * @flags:	Tracks initialized, active and stats state.
+ * @flags:	Tracks initialized, active, stats and autoexpire state.
  * @name:	Name used for debugging.
+ * @expires:	Time, in jiffies, to unblock suspend.
  * @count:	Number of times this blocker has been deacivated
  * @wakeup_count: Number of times this blocker was the first to block suspend
  *		after resume.
@@ -44,9 +45,11 @@ struct suspend_blocker {
 	struct list_head    link;
 	int                 flags;
 	const char         *name;
+	unsigned long       expires;
 #ifdef CONFIG_SUSPEND_BLOCKER_STATS
 	struct {
 		int             count;
+		int             expire_count;
 		int             wakeup_count;
 		ktime_t         total_time;
 		ktime_t         prevent_suspend_time;
@@ -62,6 +65,7 @@ struct suspend_blocker {
 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_block_timeout(struct suspend_blocker *blocker, long timeout);
 void suspend_unblock(struct suspend_blocker *blocker);
 bool suspend_blocker_is_active(struct suspend_blocker *blocker);
 bool suspend_is_blocked(void);
@@ -72,6 +76,7 @@ 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_block_timeout(struct suspend_blocker *bl, long t) {}
 static inline void suspend_unblock(struct suspend_blocker *blocker) {}
 static inline bool suspend_blocker_is_active(struct suspend_blocker *bl)
 								{ return 0; }
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index b5ce295..2725158 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -26,13 +26,15 @@ enum {
 	DEBUG_USER_STATE = 1U << 2,
 	DEBUG_SUSPEND = 1U << 3,
 	DEBUG_SUSPEND_BLOCKER = 1U << 4,
+	DEBUG_EXPIRE = 1U << 5,
 };
 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)
-#define SB_PREVENTING_SUSPEND     (1U << 10)
+#define SB_AUTO_EXPIRE            (1U << 10)
+#define SB_PREVENTING_SUSPEND     (1U << 11)
 
 static DEFINE_SPINLOCK(list_lock);
 static DEFINE_SPINLOCK(state_lock);
@@ -63,20 +65,53 @@ static struct suspend_blocker deleted_suspend_blockers;
 static ktime_t last_sleep_time_update;
 static bool wait_for_wakeup;
 
+static bool stats_get_expired_time(struct suspend_blocker *blocker,
+				   ktime_t *expire_time)
+{
+	struct timespec ts;
+	struct timespec kt;
+	struct timespec tomono;
+	struct timespec delta;
+	unsigned long seq;
+	long timeout;
+
+	if (!(blocker->flags & SB_AUTO_EXPIRE))
+		return false;
+	do {
+		seq = read_seqbegin(&xtime_lock);
+		timeout = blocker->expires - jiffies;
+		if (timeout > 0)
+			return false;
+		kt = current_kernel_time();
+		tomono = wall_to_monotonic;
+	} while (read_seqretry(&xtime_lock, seq));
+	jiffies_to_timespec(-timeout, &delta);
+	set_normalized_timespec(&ts, kt.tv_sec + tomono.tv_sec - delta.tv_sec,
+				kt.tv_nsec + tomono.tv_nsec - delta.tv_nsec);
+	*expire_time = timespec_to_ktime(ts);
+	return true;
+}
+
 static int print_blocker_stat(struct seq_file *m,
 			      struct suspend_blocker *blocker)
 {
 	int lock_count = blocker->stat.count;
+	int expire_count = blocker->stat.expire_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();
+		bool expired = stats_get_expired_time(blocker, &now);
+		if (!expired)
+			now = ktime_get();
 		add_time = ktime_sub(now, blocker->stat.last_time);
 		lock_count++;
-		active_time = add_time;
+		if (!expired)
+			active_time = add_time;
+		else
+			expire_count++;
 		total_time = ktime_add(total_time, add_time);
 		if (blocker->flags & SB_PREVENTING_SUSPEND)
 			prevent_suspend_time = ktime_add(prevent_suspend_time,
@@ -85,9 +120,10 @@ static int print_blocker_stat(struct seq_file *m,
 			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),
+	return seq_printf(m, "\"%s\"\t%d\t%d\t%d\t%lld\t%lld\t%lld\t%lld\t"
+		       "%lld\n", blocker->name, lock_count, expire_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));
 }
@@ -98,7 +134,7 @@ 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"
+	seq_puts(m, "name\tcount\texpire_count\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)
@@ -112,6 +148,7 @@ static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
 static void suspend_blocker_stat_init_locked(struct suspend_blocker *blocker)
 {
 	blocker->stat.count = 0;
+	blocker->stat.expire_count = 0;
 	blocker->stat.wakeup_count = 0;
 	blocker->stat.total_time = ktime_set(0, 0);
 	blocker->stat.prevent_suspend_time = ktime_set(0, 0);
@@ -124,6 +161,7 @@ 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.expire_count += bl->stat.expire_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(
@@ -133,14 +171,20 @@ static void suspend_blocker_stat_destroy_locked(struct suspend_blocker *bl)
 		deleted_suspend_blockers.stat.max_time, bl->stat.max_time);
 }
 
-static void suspend_unblock_stat_locked(struct suspend_blocker *blocker)
+static void suspend_unblock_stat_locked(struct suspend_blocker *blocker,
+					bool expired)
 {
 	ktime_t duration;
 	ktime_t now;
 	if (!(blocker->flags & SB_ACTIVE))
 		return;
-	now = ktime_get();
+	if (stats_get_expired_time(blocker, &now))
+		expired = true;
+	else
+		now = ktime_get();
 	blocker->stat.count++;
+	if (expired)
+		blocker->stat.expire_count++;
 	duration = ktime_sub(now, blocker->stat.last_time);
 	blocker->stat.total_time =
 		ktime_add(blocker->stat.total_time, duration);
@@ -163,6 +207,12 @@ static void suspend_block_stat_locked(struct suspend_blocker *blocker)
 		wait_for_wakeup = false;
 		blocker->stat.wakeup_count++;
 	}
+	if ((blocker->flags & SB_AUTO_EXPIRE) &&
+	    time_is_before_eq_jiffies(blocker->expires)) {
+		suspend_unblock_stat_locked(blocker, false);
+		blocker->stat.last_time = ktime_get();
+	}
+
 	if (!(blocker->flags & SB_ACTIVE))
 		blocker->stat.last_time = ktime_get();
 }
@@ -170,17 +220,22 @@ static void suspend_block_stat_locked(struct suspend_blocker *blocker)
 static void update_sleep_wait_stats_locked(bool done)
 {
 	struct suspend_blocker *blocker;
-	ktime_t now, elapsed, add;
+	ktime_t now, etime, elapsed, add;
+	bool expired;
 
 	now = ktime_get();
 	elapsed = ktime_sub(now, last_sleep_time_update);
 	list_for_each_entry(blocker, &active_blockers, link) {
+		expired = stats_get_expired_time(blocker, &etime);
 		if (blocker->flags & SB_PREVENTING_SUSPEND) {
-			add = elapsed;
+			if (expired)
+				add = ktime_sub(etime, last_sleep_time_update);
+			else
+				add = elapsed;
 			blocker->stat.prevent_suspend_time = ktime_add(
 				blocker->stat.prevent_suspend_time, add);
 		}
-		if (done)
+		if (done || expired)
 			blocker->flags &= ~SB_PREVENTING_SUSPEND;
 		else
 			blocker->flags |= SB_PREVENTING_SUSPEND;
@@ -201,8 +256,8 @@ 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 suspend_unblock_stat_locked(struct suspend_blocker *blocker,
+					   bool expired) {}
 static inline void update_sleep_wait_stats_locked(bool done) {}
 
 static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
@@ -222,12 +277,50 @@ static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
 
 #endif
 
+static void expire_suspend_blocker(struct suspend_blocker *blocker)
+{
+	suspend_unblock_stat_locked(blocker, true);
+	blocker->flags &= ~(SB_ACTIVE | SB_AUTO_EXPIRE);
+	list_del(&blocker->link);
+	list_add(&blocker->link, &inactive_blockers);
+	if (debug_mask & (DEBUG_SUSPEND_BLOCKER | DEBUG_EXPIRE))
+		pr_info("expired suspend blocker %s\n", blocker->name);
+}
+
 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);
+	list_for_each_entry(blocker, &active_blockers, link) {
+		if (blocker->flags & SB_AUTO_EXPIRE) {
+			long timeout = blocker->expires - jiffies;
+			if (timeout <= 0)
+				pr_info("suspend blocker %s, expired\n",
+					blocker->name);
+			else
+				pr_info("active suspend blocker %s, time left "
+					"%ld\n", blocker->name, timeout);
+		} else
+			pr_info("active suspend blocker %s\n", blocker->name);
+	}
+}
+
+static long max_suspend_blocker_timeout_locked(void)
+{
+	struct suspend_blocker *blocker, *n;
+	long max_timeout = 0;
+
+	list_for_each_entry_safe(blocker, n, &active_blockers, link) {
+		if (blocker->flags & SB_AUTO_EXPIRE) {
+			long timeout = blocker->expires - jiffies;
+			if (timeout <= 0)
+				expire_suspend_blocker(blocker);
+			else if (timeout > max_timeout)
+				max_timeout = timeout;
+		} else
+			return -1;
+	}
+	return max_timeout;
 }
 
 /**
@@ -241,9 +334,14 @@ static void print_active_blockers_locked(void)
  */
 bool suspend_is_blocked(void)
 {
+	long ret;
+	unsigned long irqflags;
 	if (!enable_suspend_blockers)
 		return 0;
-	return !list_empty(&active_blockers);
+	spin_lock_irqsave(&list_lock, irqflags);
+	ret = !!max_suspend_blocker_timeout_locked();
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return ret;
 }
 
 static void suspend_worker(struct work_struct *work)
@@ -268,14 +366,49 @@ static void suspend_worker(struct work_struct *work)
 	if (current_event_num == entry_event_num) {
 		if (debug_mask & DEBUG_SUSPEND)
 			pr_info("suspend: pm_suspend returned with no event\n");
-		suspend_block(&unknown_wakeup);
-		suspend_unblock(&unknown_wakeup);
+		suspend_block_timeout(&unknown_wakeup, HZ / 2);
 	}
 abort:
 	enable_suspend_blockers = false;
 }
 static DECLARE_WORK(suspend_work, suspend_worker);
 
+static void expire_suspend_blockers(unsigned long data)
+{
+	long timeout;
+	unsigned long irqflags;
+	if (debug_mask & DEBUG_EXPIRE)
+		pr_info("expire_suspend_blockers: start\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	if (debug_mask & DEBUG_SUSPEND)
+		print_active_blockers_locked();
+	timeout = max_suspend_blocker_timeout_locked();
+	if (debug_mask & DEBUG_EXPIRE)
+		pr_info("expire_suspend_blockers: done, timeout %ld\n",
+			timeout);
+	if (timeout == 0)
+		queue_work(suspend_work_queue, &suspend_work);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+}
+static DEFINE_TIMER(expire_timer, expire_suspend_blockers, 0, 0);
+
+static void update_suspend(struct suspend_blocker *blocker, long max_timeout)
+{
+	if (max_timeout > 0) {
+		if (debug_mask & DEBUG_EXPIRE)
+			pr_info("suspend_blocker: %s, start expire timer, "
+				"%ld\n", blocker->name, max_timeout);
+		mod_timer(&expire_timer, jiffies + max_timeout);
+	} else {
+		if (del_timer(&expire_timer))
+			if (debug_mask & DEBUG_EXPIRE)
+				pr_info("suspend_blocker: %s, stop expire "
+					"timer\n", blocker->name);
+		if (max_timeout == 0)
+			queue_work(suspend_work_queue, &suspend_work);
+	}
+}
+
 /**
  * suspend_blocker_init() - Initialize a suspend blocker
  * @blocker:	The suspend blocker to initialize.
@@ -319,17 +452,14 @@ void suspend_blocker_destroy(struct suspend_blocker *blocker)
 	suspend_blocker_stat_destroy_locked(blocker);
 	blocker->flags &= ~SB_INITIALIZED;
 	list_del(&blocker->link);
-	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
-		queue_work(suspend_work_queue, &suspend_work);
+	if (blocker->flags & SB_ACTIVE)
+		update_suspend(blocker, max_suspend_blocker_timeout_locked());
 	spin_unlock_irqrestore(&list_lock, irqflags);
 }
 EXPORT_SYMBOL(suspend_blocker_destroy);
 
-/**
- * suspend_block() - Block suspend
- * @blocker:	The suspend blocker to use
- */
-void suspend_block(struct suspend_blocker *blocker)
+static void __suspend_block(struct suspend_blocker *blocker, long timeout,
+			    bool has_timeout)
 {
 	unsigned long irqflags;
 
@@ -340,20 +470,56 @@ void suspend_block(struct suspend_blocker *blocker)
 	suspend_block_stat_locked(blocker);
 	blocker->flags |= SB_ACTIVE;
 	list_del(&blocker->link);
-	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
-		pr_info("suspend_block: %s\n", blocker->name);
-	list_add(&blocker->link, &active_blockers);
+	if (has_timeout) {
+		if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+			pr_info("suspend_block: %s, timeout %ld.%03lu\n",
+				blocker->name, timeout / HZ,
+				(timeout % HZ) * MSEC_PER_SEC / HZ);
+		blocker->expires = jiffies + timeout;
+		blocker->flags |= SB_AUTO_EXPIRE;
+		list_add_tail(&blocker->link, &active_blockers);
+	} else {
+		if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+			pr_info("suspend_block: %s\n", blocker->name);
+		blocker->expires = LONG_MAX;
+		blocker->flags &= ~SB_AUTO_EXPIRE;
+		/* Add to head so suspend_is_blocked only has to examine */
+		/* one entry */
+		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);
+	update_suspend(blocker, has_timeout ?
+			max_suspend_blocker_timeout_locked() : -1);
 	spin_unlock_irqrestore(&list_lock, irqflags);
 }
+
+/**
+ * suspend_block() - Block suspend
+ * @blocker:	The suspend blocker to use
+ */
+void suspend_block(struct suspend_blocker *blocker)
+{
+	__suspend_block(blocker, 0, false);
+}
 EXPORT_SYMBOL(suspend_block);
 
 /**
+ * suspend_block_timeout() - Block suspend for a limited time
+ * @blocker:	The suspend blocker to use.
+ * @timeout:	Timeout in jiffies before the suspend blocker auto-unblock
+ */
+void suspend_block_timeout(struct suspend_blocker *blocker, long timeout)
+{
+	__suspend_block(blocker, timeout, true);
+}
+EXPORT_SYMBOL(suspend_block_timeout);
+
+/**
  * suspend_unblock() - Unblock suspend
  * @blocker:	The suspend blocker to unblock.
  *
@@ -368,16 +534,15 @@ void suspend_unblock(struct suspend_blocker *blocker)
 
 	spin_lock_irqsave(&list_lock, irqflags);
 
-	suspend_unblock_stat_locked(blocker);
+	suspend_unblock_stat_locked(blocker, false);
 
 	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
 		pr_info("suspend_unblock: %s\n", blocker->name);
+	blocker->flags &= ~(SB_ACTIVE | SB_AUTO_EXPIRE);
 	list_del(&blocker->link);
 	list_add(&blocker->link, &inactive_blockers);
 
-	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
-		queue_work(suspend_work_queue, &suspend_work);
-	blocker->flags &= ~(SB_ACTIVE);
+	update_suspend(blocker, max_suspend_blocker_timeout_locked());
 	if (blocker == &main_suspend_blocker) {
 		if (debug_mask & DEBUG_SUSPEND)
 			print_active_blockers_locked();
@@ -392,6 +557,10 @@ EXPORT_SYMBOL(suspend_unblock);
  * @blocker:	The suspend blocker to check.
  *
  * Returns true if the suspend_blocker is currently active.
+ *
+ * If the suspend_blocker has a timeout, it does not check the timeout, but if
+ * the timeout had already expired when it was checked elsewhere this function
+ * will return false.
  */
 bool suspend_blocker_is_active(struct suspend_blocker *blocker)
 {
-- 
1.6.1

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

[PATCH 9/9] PM: suspend_block: Add timeout support to user-space suspend_blockers

[Arve Hjønnevåg]

This also allows a grace period to be set where suspend is blocked for a while
if a user-space suspend_blocker is destroyed while it was active.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |    7 +++++++
 include/linux/suspend_block_dev.h        |    1 +
 kernel/power/user_suspend_blocker.c      |   23 +++++++++++++++++++++++
 3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index bc9d57a..3612794 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -112,6 +112,8 @@ then call:
 
 To activate a suspend_blocker call:
     ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+or
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK_TIMEOUT, &timespec_timeout);
 
 To unblock call:
     ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
@@ -119,3 +121,8 @@ To unblock call:
 To destroy the suspend_blocker, close the device:
     close(fd);
 
+A module parameter, unclean_exit_grace_period, can be set to allow servers
+some time to restart if they crash with an active suspend_blocker. If the
+process dies or the device is closed while the suspend_blocker is active, a
+suspend_blocker will be held for the number of seconds specified.
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
index a7c0bf9..cff4943 100644
--- a/include/linux/suspend_block_dev.h
+++ b/include/linux/suspend_block_dev.h
@@ -20,6 +20,7 @@
 
 #define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
 #define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK_TIMEOUT	_IOW('s', 2, struct timespec)
 #define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 3)
 
 #endif
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
index 69b3ddb..b853f66 100644
--- a/kernel/power/user_suspend_blocker.c
+++ b/kernel/power/user_suspend_blocker.c
@@ -26,7 +26,12 @@ enum {
 static int debug_mask = DEBUG_FAILURE;
 module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
 
+static int unclean_exit_grace_period;
+module_param_named(unclean_exit_grace_period, unclean_exit_grace_period, int,
+						S_IRUGO | S_IWUSR | S_IWGRP);
+
 static DEFINE_MUTEX(ioctl_lock);
+static struct suspend_blocker unclean_exit_suspend_blocker;
 
 struct user_suspend_blocker {
 	struct suspend_blocker	blocker;
@@ -58,6 +63,8 @@ static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
 {
 	void __user *arg = (void __user *)_arg;
 	struct user_suspend_blocker *bl;
+	struct timespec ts;
+	unsigned long timeout;
 	long ret;
 
 	mutex_lock(&ioctl_lock);
@@ -75,6 +82,15 @@ static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
 		suspend_block(&bl->blocker);
 		ret = 0;
 		break;
+	case SUSPEND_BLOCKER_IOCTL_BLOCK_TIMEOUT:
+		if (copy_from_user(&ts, arg, sizeof(ts))) {
+			ret = -EFAULT;
+			goto done;
+		}
+		timeout  = timespec_to_jiffies(&ts);
+		suspend_block_timeout(&bl->blocker, timeout);
+		ret = 0;
+		break;
 	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
 		suspend_unblock(&bl->blocker);
 		ret = 0;
@@ -95,6 +111,10 @@ static int user_suspend_blocker_release(struct inode *inode, struct file *file)
 	struct user_suspend_blocker *bl = file->private_data;
 	if (!bl)
 		return 0;
+	if (suspend_blocker_is_active(&bl->blocker) &&
+	    unclean_exit_grace_period)
+		suspend_block_timeout(&unclean_exit_suspend_blocker,
+					unclean_exit_grace_period * HZ);
 	suspend_blocker_destroy(&bl->blocker);
 	kfree(bl);
 	return 0;
@@ -113,12 +133,15 @@ struct miscdevice user_suspend_blocker_device = {
 
 static int __init user_suspend_blocker_init(void)
 {
+	suspend_blocker_init(&unclean_exit_suspend_blocker,
+				"user-unclean-exit");
 	return misc_register(&user_suspend_blocker_device);
 }
 
 static void __exit user_suspend_blocker_exit(void)
 {
 	misc_deregister(&user_suspend_blocker_device);
+	suspend_blocker_destroy(&unclean_exit_suspend_blocker);
 }
 
 module_init(user_suspend_blocker_init);
-- 
1.6.1

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

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

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 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.

Sorry for joining the wakelock/suspend-block discussion a bit late,
but I have a very basic, and possibly stupid question on the need for
such a feature.

My basic question: is why not use idle (dynamic PM) instead of suspend
(static PM) for this?  

IOW, why not use CPUidle + PM QoS to achieve the same goal?  It seems
to me that you're trying to add some dynamic PM features onto a static
PM subsystem.  I could see PM QoS possibly needing some extending but
the basic abilities to constrain the low-power states are there.

The one reason I can think of is that you can always have broken
drivers or apps that prevent you from hitting idle.  But with tools
like powertop, it's pretty easy to find these problems and fix them.

If that is the primary concern, it seems to me that we're introducing
a new kernel API to work around broken drivers and apps that should be
fixed instead.

What am I missing?

Kevin

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

[Arve Hjønnevåg]

2009/5/6 Kevin Hilman <khilman@deeprootsystems.com>:
> Arve Hjønnevåg <arve@android.com> writes:
>
>> 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.
>
> Sorry for joining the wakelock/suspend-block discussion a bit late,
> but I have a very basic, and possibly stupid question on the need for
> such a feature.
>
> My basic question: is why not use idle (dynamic PM) instead of suspend
> (static PM) for this?
>

We use suspend in addition to idle, not instead of. On some systems
(msm) we can enter the same power state from idle as from suspend.
This reduces the importance of suspend, but entering suspend still has
benefits. The monotonic clock stops and all the periodic timers
(user-space and kernel) that are based on it stop with it. It also
reduces the amount of energy an untrusted application can waste since
it will not be allowed to run when the system is suspended.

> IOW, why not use CPUidle + PM QoS to achieve the same goal?  It seems
> to me that you're trying to add some dynamic PM features onto a static
> PM subsystem.  I could see PM QoS possibly needing some extending but
> the basic abilities to constrain the low-power states are there.

suspend blockers have no impact on this.

> The one reason I can think of is that you can always have broken
> drivers or apps that prevent you from hitting idle.  But with tools
> like powertop, it's pretty easy to find these problems and fix them.
>
Tools allow you to find apps that waste power, but they do not fix them.

> If that is the primary concern, it seems to me that we're introducing
> a new kernel API to work around broken drivers and apps that should be
> fixed instead.
>
> What am I missing?

Suspend blockers allow you to use suspend. On platforms that cannot
enter the same power state from idle this is essential. On platforms
can enter the same power state from idle as suspend, suspend reduce
the time spent running which in turn reduces the overall power draw.
It also offers some protection against bad apps which is important on
an open device.

-- 
Arve Hjønnevåg

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

[mark gross]

On Thu, May 07, 2009 at 03:42:27PM -0700, Arve Hjønnevåg wrote:
> 2009/5/6 Kevin Hilman <khilman@deeprootsystems.com>:
> > Arve Hjønnevåg <arve@android.com> writes:
> >
> >> 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.
> >
> > Sorry for joining the wakelock/suspend-block discussion a bit late,
> > but I have a very basic, and possibly stupid question on the need for
> > such a feature.
> >
> > My basic question: is why not use idle (dynamic PM) instead of suspend
> > (static PM) for this?
> >
> 
> We use suspend in addition to idle, not instead of. On some systems
> (msm) we can enter the same power state from idle as from suspend.
> This reduces the importance of suspend, but entering suspend still has
> benefits. The monotonic clock stops and all the periodic timers
> (user-space and kernel) that are based on it stop with it. It also
> reduces the amount of energy an untrusted application can waste since
> it will not be allowed to run when the system is suspended.

Yes CPUIDLE and pmqos are more "cooperative" power management, where
suspend is a preemptive form of PM.  I get a little itchy when the
preemptive PM is made automatic from the kernel level.

BTW Thanks.  I feel like I'm understanding your PM design a bit better
now.

--mgross

> 
> > IOW, why not use CPUidle + PM QoS to achieve the same goal?  It seems
> > to me that you're trying to add some dynamic PM features onto a static
> > PM subsystem.  I could see PM QoS possibly needing some extending but
> > the basic abilities to constrain the low-power states are there.
> 
> suspend blockers have no impact on this.
> 
> > The one reason I can think of is that you can always have broken
> > drivers or apps that prevent you from hitting idle.  But with tools
> > like powertop, it's pretty easy to find these problems and fix them.
> >
> Tools allow you to find apps that waste power, but they do not fix them.
> 
> > If that is the primary concern, it seems to me that we're introducing
> > a new kernel API to work around broken drivers and apps that should be
> > fixed instead.
> >
> > What am I missing?
> 
> Suspend blockers allow you to use suspend. On platforms that cannot
> enter the same power state from idle this is essential. On platforms
> can enter the same power state from idle as suspend, suspend reduce
> the time spent running which in turn reduces the overall power draw.
> It also offers some protection against bad apps which is important on
> an open device.
> 
> -- 
> Arve Hjønnevåg
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.linux-foundation.org
> https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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

[Kevin Hilman]

Arve Hjønnevåg <arve@android.com> writes:

> 2009/5/6 Kevin Hilman <khilman@deeprootsystems.com>:
>> Arve Hjønnevåg <arve@android.com> writes:
>>
>>> 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.
>>
>> Sorry for joining the wakelock/suspend-block discussion a bit late,
>> but I have a very basic, and possibly stupid question on the need for
>> such a feature.
>>
>> My basic question: is why not use idle (dynamic PM) instead of suspend
>> (static PM) for this?
>>
>
> We use suspend in addition to idle, not instead of. On some systems
> (msm) we can enter the same power state from idle as from suspend.
> This reduces the importance of suspend, but entering suspend still has
> benefits. The monotonic clock stops and all the periodic timers
> (user-space and kernel) that are based on it stop with it. It also
> reduces the amount of energy an untrusted application can waste since
> it will not be allowed to run when the system is suspended.
>
>> IOW, why not use CPUidle + PM QoS to achieve the same goal?  It seems
>> to me that you're trying to add some dynamic PM features onto a static
>> PM subsystem.  I could see PM QoS possibly needing some extending but
>> the basic abilities to constrain the low-power states are there.
>
> suspend blockers have no impact on this.
>
>> The one reason I can think of is that you can always have broken
>> drivers or apps that prevent you from hitting idle.  But with tools
>> like powertop, it's pretty easy to find these problems and fix them.
>>
> Tools allow you to find apps that waste power, but they do not fix them.
>
>> If that is the primary concern, it seems to me that we're introducing
>> a new kernel API to work around broken drivers and apps that should be
>> fixed instead.
>>
>> What am I missing?
>
> Suspend blockers allow you to use suspend. On platforms that cannot
> enter the same power state from idle this is essential. On platforms
> can enter the same power state from idle as suspend, suspend reduce
> the time spent running which in turn reduces the overall power draw.
> It also offers some protection against bad apps which is important on
> an open device.

Arve,

Thanks for the clarifications.  Like Mark, I feel like I have a better
grip on the motivation behind these changes.

Thanks,

Kevin

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

[Pavel Machek]

On Thu 2009-05-07 15:42:27, Arve Hj?nnev?g wrote:
> 2009/5/6 Kevin Hilman <khilman@deeprootsystems.com>:
> > Arve Hj?nnev?g <arve@android.com> writes:
> >
> >> 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.
> >
> > Sorry for joining the wakelock/suspend-block discussion a bit late,
> > but I have a very basic, and possibly stupid question on the need for
> > such a feature.
> >
> > My basic question: is why not use idle (dynamic PM) instead of suspend
> > (static PM) for this?
> >
> 
> We use suspend in addition to idle, not instead of. On some systems
> (msm) we can enter the same power state from idle as from suspend.
> This reduces the importance of suspend, but entering suspend still has
> benefits. The monotonic clock stops and all the periodic timers
> (user-space and kernel) that are based on it stop with it. It also
> reduces the amount of energy an untrusted application can waste since
> it will not be allowed to run when the system is suspended.
> 
> > IOW, why not use CPUidle + PM QoS to achieve the same goal?  It seems
> > to me that you're trying to add some dynamic PM features onto a static
> > PM subsystem.  I could see PM QoS possibly needing some extending but
> > the basic abilities to constrain the low-power states are there.
> 
> suspend blockers have no impact on this.
> 
> > The one reason I can think of is that you can always have broken
> > drivers or apps that prevent you from hitting idle.  But with tools
> > like powertop, it's pretty easy to find these problems and fix them.
> >
> Tools allow you to find apps that waste power, but they do not fix them.
> 
> > If that is the primary concern, it seems to me that we're introducing
> > a new kernel API to work around broken drivers and apps that should be
> > fixed instead.
> >
> > What am I missing?
> 
> Suspend blockers allow you to use suspend. On platforms that cannot
> enter the same power state from idle this is essential. On platforms
> can enter the same power state from idle as suspend, suspend reduce
> the time spent running which in turn reduces the overall power draw.
> It also offers some protection against bad apps which is important on
> an open device.
> 

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