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

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

[Arve Hjønnevåg]

Adds 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_block.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_block.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 743b870..35bc713 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -74,3 +74,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 9d1df13..2cf4c4d 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -126,6 +126,15 @@ config SUSPEND_BLOCK
 	  /sys/power/request_state, the requested sleep state will be entered
 	  when no suspend_blockers are active.
 
+config USER_SUSPEND_BLOCK
+	bool "Userspace suspend blockers"
+	depends on SUSPEND_BLOCK
+	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 29cdc9e..5c524f1 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_BLOCK)	+= suspend_block.o
+obj-$(CONFIG_USER_SUSPEND_BLOCK)	+= user_suspend_block.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_block.c b/kernel/power/user_suspend_block.c
new file mode 100644
index 0000000..782407c
--- /dev/null
+++ b/kernel/power/user_suspend_block.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_block.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/8] PM: suspend_block: Add driver to access suspend blockers from user-space

[Rafael J. Wysocki]

On Wednesday 15 April 2009, Arve Hjønnevåg wrote:
> Adds ioctls to create a suspend_blocker, and to block and unblock suspend.
> To delete the suspend_blocker, close the device.

The changelog is minimalistic, so to speak.

Can you please describe the idea in more detail in it?

Also, perhaps you could use write() to initialize the suspend blocker?
I don't know really, it's just a thought.

You might not need ioctls() at all if you used write() like
write(name) -> initialize
write(1) -> activate
write(0) -> deactivate
write(1) -> activate
write(0) -> deactivate
etc.
read() -> check status (0 -> inactive, 1 -> active)
or something like this.

It might be extended for timeouts too. ;-)

> 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_block.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_block.c
> 
> diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
> index 743b870..35bc713 100644
> --- a/Documentation/power/suspend-blockers.txt
> +++ b/Documentation/power/suspend-blockers.txt
> @@ -74,3 +74,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 9d1df13..2cf4c4d 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -126,6 +126,15 @@ config SUSPEND_BLOCK
>  	  /sys/power/request_state, the requested sleep state will be entered
>  	  when no suspend_blockers are active.
>  
> +config USER_SUSPEND_BLOCK
> +	bool "Userspace suspend blockers"
> +	depends on SUSPEND_BLOCK
> +	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 29cdc9e..5c524f1 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_BLOCK)	+= suspend_block.o
> +obj-$(CONFIG_USER_SUSPEND_BLOCK)	+= user_suspend_block.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_block.c b/kernel/power/user_suspend_block.c
> new file mode 100644
> index 0000000..782407c
> --- /dev/null
> +++ b/kernel/power/user_suspend_block.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_block.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);


-- 
Everyone knows that debugging is twice as hard as writing a program
in the first place.  So if you're as clever as you can be when you write it,
how will you ever debug it? --- Brian Kernighan
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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

[Arve Hjønnevåg]

Adds /sys/power/policy that selects the behaviour of /sys/power/state.
After setting the policy to opportunistic, writes to /sys/power/state
become non-blocking requests that specify which suspend state to enter
when no suspend blockers are active. A special state, "on", stops the
process by activating the "main" suspend blocker.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/opportunistic-suspend.txt |  114 +++++++++++
 include/linux/suspend_blocker.h               |   64 ++++++
 kernel/power/Kconfig                          |   16 ++
 kernel/power/Makefile                         |    1 +
 kernel/power/main.c                           |   89 ++++++++-
 kernel/power/power.h                          |    5 +
 kernel/power/suspend.c                        |    4 +-
 kernel/power/suspend_blocker.c                |  269 +++++++++++++++++++++++++
 8 files changed, 556 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/power/opportunistic-suspend.txt
 create mode 100755 include/linux/suspend_blocker.h
 create mode 100644 kernel/power/suspend_blocker.c

diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
new file mode 100644
index 0000000..1a29d10
--- /dev/null
+++ b/Documentation/power/opportunistic-suspend.txt
@@ -0,0 +1,114 @@
+Opportunistic Suspend
+=====================
+
+Opportunistic suspend is a feature allowing the system to be suspended (ie. put
+into one of the available sleep states) automatically whenever it is regarded
+as idle.  The suspend blockers framework described below is used to determine
+when that happens.
+
+The /sys/power/policy sysfs attribute is used to switch the system between the
+opportunistic and "forced" suspend behavior, where in the latter case the
+system is only suspended if a specific value, corresponding to one of the
+available system sleep states, is written into /sys/power/state.  However, in
+the former, opportunistic, case the system is put into the sleep state
+corresponding to the value written to /sys/power/state whenever there are no
+active suspend blockers. The default policy is "forced". Also, suspend blockers
+do not affect sleep states entered from idle.
+
+When the policy is "opportunisic", there is a special value, "on", that can be
+written to /sys/power/state. This will block the automatic sleep request, as if
+a suspend blocker was used by a device driver. This way the opportunistic
+suspend may be blocked by user space whithout switching back to the "forced"
+mode.
+
+A suspend blocker is an object used to inform the PM subsystem when the system
+can or cannot be suspended in the "opportunistic" mode (the "forced" mode
+ignores suspend blockers).  To use it, a device driver creates a struct
+suspend_blocker that must be initialized with suspend_blocker_init(). Before
+freeing the suspend_blocker structure or its name, suspend_blocker_destroy()
+must be called on it.
+
+A suspend blocker is activated using suspend_block(), which prevents the PM
+subsystem from putting the system into the requested sleep state in the
+"opportunistic" mode until the suspend blocker is deactivated with
+suspend_unblock(). Multiple suspend blockers may be active simultaneously, and
+the system will not suspend as long as at least one of them is active.
+
+If opportunistic suspend is already in progress when suspend_block() is called,
+it will abort the suspend, unless suspend_ops->enter has already been
+executed. If suspend is aborted this way, the system is usually not fully
+operational at that point. The suspend callbacks of some drivers may still be
+running and it usually takes time to restore the system to the fully operational
+state.
+
+For example, 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 (i.e., these functions don't nest). This allows
+drivers to update their state and call suspend suspend_block or suspend_unblock
+based on the result.
+For instance:
+
+if (list_empty(&state->pending_work))
+	suspend_unblock(&state->suspend_blocker);
+else
+	suspend_block(&state->suspend_blocker);
+
diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
new file mode 100755
index 0000000..f9928cc
--- /dev/null
+++ b/include/linux/suspend_blocker.h
@@ -0,0 +1,64 @@
+/* include/linux/suspend_blocker.h
+ *
+ * Copyright (C) 2007-2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCKER_H
+#define _LINUX_SUSPEND_BLOCKER_H
+
+#include <linux/list.h>
+
+/**
+ * struct suspend_blocker - the basic suspend_blocker structure
+ * @link:	List entry for active or inactive list.
+ * @flags:	Tracks initialized and active state.
+ * @name:	Name used for debugging.
+ *
+ * When a suspend_blocker is active it prevents the system from entering
+ * opportunistic suspend.
+ *
+ * The suspend_blocker structure must be initialized by suspend_blocker_init()
+ */
+
+struct suspend_blocker {
+#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
+	struct list_head    link;
+	int                 flags;
+	const char         *name;
+#endif
+};
+
+#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
+
+void suspend_blocker_init(struct suspend_blocker *blocker, const char *name);
+void suspend_blocker_destroy(struct suspend_blocker *blocker);
+void suspend_block(struct suspend_blocker *blocker);
+void suspend_unblock(struct suspend_blocker *blocker);
+bool suspend_blocker_is_active(struct suspend_blocker *blocker);
+bool suspend_is_blocked(void);
+
+#else
+
+static inline void suspend_blocker_init(struct suspend_blocker *blocker,
+					const char *name) {}
+static inline void suspend_blocker_destroy(struct suspend_blocker *blocker) {}
+static inline void suspend_block(struct suspend_blocker *blocker) {}
+static inline void suspend_unblock(struct suspend_blocker *blocker) {}
+static inline bool suspend_blocker_is_active(struct suspend_blocker *bl)
+								{ return 0; }
+static inline bool suspend_is_blocked(void) { return 0; }
+
+#endif
+
+#endif
+
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 5c36ea9..55a06a1 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -130,6 +130,22 @@ config SUSPEND_FREEZER
 
 	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
 
+config OPPORTUNISTIC_SUSPEND
+	bool "Suspend blockers"
+	depends on PM_SLEEP
+	select RTC_LIB
+	default n
+	---help---
+	  Opportunistic sleep support.  Allows the system to be put into a sleep
+	  state opportunistically, if it doesn't do any useful work at the
+	  moment. The PM subsystem is switched into this mode of operation by
+	  writing "opportunistic" into /sys/power/policy, while writing
+	  "forced" to this file turns the opportunistic suspend feature off.
+	  In the "opportunistic" mode suspend blockers are used to determine
+	  when to suspend the system and the value written to /sys/power/state
+	  determines the sleep state the system will be put into when there are
+	  no active suspend blockers.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 4319181..ee5276d 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_PM)		+= main.o
 obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
+obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
diff --git a/kernel/power/main.c b/kernel/power/main.c
index b58800b..5f0af6c 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -12,6 +12,7 @@
 #include <linux/string.h>
 #include <linux/resume-trace.h>
 #include <linux/workqueue.h>
+#include <linux/suspend_blocker.h>
 
 #include "power.h"
 
@@ -20,6 +21,27 @@ DEFINE_MUTEX(pm_mutex);
 unsigned int pm_flags;
 EXPORT_SYMBOL(pm_flags);
 
+struct policy {
+	const char *name;
+	bool (*valid_state)(suspend_state_t state);
+	int (*set_state)(suspend_state_t state);
+};
+static struct policy policies[] = {
+	{
+		.name		= "forced",
+		.valid_state	= valid_state,
+		.set_state	= enter_state,
+	},
+#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
+	{
+		.name		= "opportunistic",
+		.valid_state	= request_suspend_valid_state,
+		.set_state	= request_suspend_state,
+	},
+#endif
+};
+static int policy;
+
 #ifdef CONFIG_PM_SLEEP
 
 /* Routines for PM-transition notifications */
@@ -146,6 +168,12 @@ struct kobject *power_kobj;
  *
  *	store() accepts one of those strings, translates it into the 
  *	proper enumerated value, and initiates a suspend transition.
+ *
+ *	If policy is set to opportunistic, store() does not block until the
+ *	system resumes, and it will try to re-enter the state until another
+ *	state is requested. Suspend blockers are respected and the requested
+ *	state will only be entered when no suspend blockers are active.
+ *	Write "on" to cancel.
  */
 static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 			  char *buf)
@@ -155,12 +183,13 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
 	int i;
 
 	for (i = 0; i < PM_SUSPEND_MAX; i++) {
-		if (pm_states[i] && valid_state(i))
+		if (pm_states[i] && policies[policy].valid_state(i))
 			s += sprintf(s,"%s ", pm_states[i]);
 	}
 #endif
 #ifdef CONFIG_HIBERNATION
-	s += sprintf(s, "%s\n", "disk");
+	if (!policy)
+		s += sprintf(s, "%s\n", "disk");
 #else
 	if (s != buf)
 		/* convert the last space to a newline */
@@ -173,7 +202,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 			   const char *buf, size_t n)
 {
 #ifdef CONFIG_SUSPEND
-	suspend_state_t state = PM_SUSPEND_STANDBY;
+	suspend_state_t state = PM_SUSPEND_ON;
 	const char * const *s;
 #endif
 	char *p;
@@ -184,7 +213,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 	len = p ? p - buf : n;
 
 	/* First, check if we are requested to hibernate */
-	if (len == 4 && !strncmp(buf, "disk", len)) {
+	if (len == 4 && !strncmp(buf, "disk", len) && !policy) {
 		error = hibernate();
   goto Exit;
 	}
@@ -195,7 +224,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 			break;
 	}
 	if (state < PM_SUSPEND_MAX && *s)
-		error = enter_state(state);
+		error = policies[policy].set_state(state);
 #endif
 
  Exit:
@@ -204,6 +233,55 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
 
 power_attr(state);
 
+/**
+ *	policy - set policy for state
+ */
+
+static ssize_t policy_show(struct kobject *kobj,
+			   struct kobj_attribute *attr, char *buf)
+{
+	char *s = buf;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(policies); i++) {
+		if (i == policy)
+			s += sprintf(s, "[%s] ", policies[i].name);
+		else
+			s += sprintf(s, "%s ", policies[i].name);
+	}
+	if (s != buf)
+		/* convert the last space to a newline */
+		*(s-1) = '\n';
+	return (s - buf);
+}
+
+static ssize_t policy_store(struct kobject *kobj,
+			    struct kobj_attribute *attr,
+			    const char *buf, size_t n)
+{
+	const char *s;
+	char *p;
+	int len;
+	int i;
+
+	p = memchr(buf, '\n', n);
+	len = p ? p - buf : n;
+
+	for (i = 0; i < ARRAY_SIZE(policies); i++) {
+		s = policies[i].name;
+		if (s && len == strlen(s) && !strncmp(buf, s, len)) {
+			mutex_lock(&pm_mutex);
+			policies[policy].set_state(PM_SUSPEND_ON);
+			policy = i;
+			mutex_unlock(&pm_mutex);
+			return n;
+		}
+	}
+	return -EINVAL;
+}
+
+power_attr(policy);
+
 #ifdef CONFIG_PM_TRACE
 int pm_trace_enabled;
 
@@ -231,6 +309,7 @@ power_attr(pm_trace);
 
 static struct attribute * g[] = {
 	&state_attr.attr,
+	&policy_attr.attr,
 #ifdef CONFIG_PM_TRACE
 	&pm_trace_attr.attr,
 #endif
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 46c5a26..9b468d7 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -236,3 +236,8 @@ static inline void suspend_thaw_processes(void)
 {
 }
 #endif
+
+/* kernel/power/suspend_block.c */
+extern int request_suspend_state(suspend_state_t state);
+extern bool request_suspend_valid_state(suspend_state_t state);
+
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 56e7dbb..dc42006 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -16,10 +16,12 @@
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
 #include <linux/gfp.h>
+#include <linux/suspend_blocker.h>
 
 #include "power.h"
 
 const char *const pm_states[PM_SUSPEND_MAX] = {
+	[PM_SUSPEND_ON]		= "on",
 	[PM_SUSPEND_STANDBY]	= "standby",
 	[PM_SUSPEND_MEM]	= "mem",
 };
@@ -157,7 +159,7 @@ static int suspend_enter(suspend_state_t state)
 
 	error = sysdev_suspend(PMSG_SUSPEND);
 	if (!error) {
-		if (!suspend_test(TEST_CORE))
+		if (!suspend_is_blocked() && !suspend_test(TEST_CORE))
 			error = suspend_ops->enter(state);
 		sysdev_resume();
 	}
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
new file mode 100644
index 0000000..9459361
--- /dev/null
+++ b/kernel/power/suspend_blocker.c
@@ -0,0 +1,269 @@
+/* kernel/power/suspend_blocker.c
+ *
+ * Copyright (C) 2005-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/rtc.h>
+#include <linux/suspend.h>
+#include <linux/suspend_blocker.h>
+#include "power.h"
+
+enum {
+	DEBUG_EXIT_SUSPEND = 1U << 0,
+	DEBUG_WAKEUP = 1U << 1,
+	DEBUG_USER_STATE = 1U << 2,
+	DEBUG_SUSPEND = 1U << 3,
+	DEBUG_SUSPEND_BLOCKER = 1U << 4,
+};
+static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+#define SB_INITIALIZED            (1U << 8)
+#define SB_ACTIVE                 (1U << 9)
+
+static DEFINE_SPINLOCK(list_lock);
+static DEFINE_SPINLOCK(state_lock);
+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;
+static bool enable_suspend_blockers;
+
+#define pr_info_time(fmt, args...) \
+	do { \
+		struct timespec ts; \
+		struct rtc_time tm; \
+		getnstimeofday(&ts); \
+		rtc_time_to_tm(ts.tv_sec, &tm); \
+		pr_info(fmt "(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n" , \
+			args, \
+			tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, \
+			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
+	} while (0);
+
+static void print_active_blockers_locked(void)
+{
+	struct suspend_blocker *blocker;
+
+	list_for_each_entry(blocker, &active_blockers, link)
+		pr_info("active suspend blocker %s\n", blocker->name);
+}
+
+/**
+ * suspend_is_blocked() - Check if suspend should be blocked
+ *
+ * suspend_is_blocked can be used by generic power management code to abort
+ * suspend.
+ *
+ * To preserve backward compatibility suspend_is_blocked returns 0 unless it
+ * is called during suspend initiated from the suspend_block code.
+ */
+bool suspend_is_blocked(void)
+{
+	if (!enable_suspend_blockers)
+		return 0;
+	return !list_empty(&active_blockers);
+}
+
+static void suspend_worker(struct work_struct *work)
+{
+	int ret;
+	int entry_event_num;
+
+	enable_suspend_blockers = true;
+	while (!suspend_is_blocked()) {
+		entry_event_num = current_event_num;
+
+		if (debug_mask & DEBUG_SUSPEND)
+			pr_info("suspend: enter suspend\n");
+
+		ret = pm_suspend(requested_suspend_state);
+
+		if (debug_mask & DEBUG_EXIT_SUSPEND)
+			pr_info_time("suspend: exit suspend, ret = %d ", ret);
+
+		if (current_event_num == entry_event_num)
+			pr_info("suspend: pm_suspend returned with no event\n");
+	}
+	enable_suspend_blockers = false;
+}
+static DECLARE_WORK(suspend_work, suspend_worker);
+
+/**
+ * suspend_blocker_init() - Initialize a suspend blocker
+ * @blocker:	The suspend blocker to initialize.
+ * @name:	The name of the suspend blocker to show in debug messages.
+ *
+ * The suspend blocker struct and name must not be freed before calling
+ * suspend_blocker_destroy.
+ */
+void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)
+{
+	unsigned long irqflags = 0;
+
+	WARN_ON(!name);
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_init name=%s\n", name);
+
+	blocker->name = name;
+	blocker->flags = SB_INITIALIZED;
+	INIT_LIST_HEAD(&blocker->link);
+
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_add(&blocker->link, &inactive_blockers);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+}
+EXPORT_SYMBOL(suspend_blocker_init);
+
+/**
+ * suspend_blocker_destroy() - Destroy a suspend blocker
+ * @blocker:	The suspend blocker to destroy.
+ */
+void suspend_blocker_destroy(struct suspend_blocker *blocker)
+{
+	unsigned long irqflags;
+	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
+		return;
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
+
+	spin_lock_irqsave(&list_lock, irqflags);
+	blocker->flags &= ~SB_INITIALIZED;
+	list_del(&blocker->link);
+	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
+		queue_work(suspend_work_queue, &suspend_work);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+}
+EXPORT_SYMBOL(suspend_blocker_destroy);
+
+/**
+ * suspend_block() - Block suspend
+ * @blocker:	The suspend blocker to use
+ *
+ * It is safe to call this function from interrupt context.
+ */
+void suspend_block(struct suspend_blocker *blocker)
+{
+	unsigned long irqflags;
+
+	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
+		return;
+
+	spin_lock_irqsave(&list_lock, irqflags);
+	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);
+
+	current_event_num++;
+	spin_unlock_irqrestore(&list_lock, irqflags);
+}
+EXPORT_SYMBOL(suspend_block);
+
+/**
+ * suspend_unblock() - Unblock suspend
+ * @blocker:	The suspend blocker to unblock.
+ *
+ * If no other suspend blockers block suspend, the system will suspend.
+ *
+ * It is safe to call this function from interrupt context.
+ */
+void suspend_unblock(struct suspend_blocker *blocker)
+{
+	unsigned long irqflags;
+
+	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
+		return;
+
+	spin_lock_irqsave(&list_lock, irqflags);
+
+	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
+		pr_info("suspend_unblock: %s\n", blocker->name);
+
+	list_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);
+	if (blocker == &main_suspend_blocker) {
+		if (debug_mask & DEBUG_SUSPEND)
+			print_active_blockers_locked();
+	}
+	spin_unlock_irqrestore(&list_lock, irqflags);
+}
+EXPORT_SYMBOL(suspend_unblock);
+
+/**
+ * suspend_blocker_is_active() - Test if a suspend blocker is blocking suspend
+ * @blocker:	The suspend blocker to check.
+ *
+ * Returns true if the suspend_blocker is currently active.
+ */
+bool suspend_blocker_is_active(struct suspend_blocker *blocker)
+{
+	WARN_ON(!(blocker->flags & SB_INITIALIZED));
+
+	return !!(blocker->flags & SB_ACTIVE);
+}
+EXPORT_SYMBOL(suspend_blocker_is_active);
+
+bool request_suspend_valid_state(suspend_state_t state)
+{
+	return (state == PM_SUSPEND_ON) || valid_state(state);
+}
+
+int request_suspend_state(suspend_state_t state)
+{
+	unsigned long irqflags;
+
+	if (!request_suspend_valid_state(state))
+		return -ENODEV;
+
+	spin_lock_irqsave(&state_lock, irqflags);
+
+	if (debug_mask & DEBUG_USER_STATE)
+		pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
+			     state != PM_SUSPEND_ON ? "sleep" : "wakeup",
+			     requested_suspend_state, state,
+			     ktime_to_ns(ktime_get()));
+
+	requested_suspend_state = state;
+	if (state == PM_SUSPEND_ON)
+		suspend_block(&main_suspend_blocker);
+	else
+		suspend_unblock(&main_suspend_blocker);
+	spin_unlock_irqrestore(&state_lock, irqflags);
+	return 0;
+}
+
+static int __init suspend_block_init(void)
+{
+	suspend_work_queue = create_singlethread_workqueue("suspend");
+	if (!suspend_work_queue)
+		return -ENOMEM;
+
+	suspend_blocker_init(&main_suspend_blocker, "main");
+	suspend_block(&main_suspend_blocker);
+	return 0;
+}
+
+core_initcall(suspend_block_init);
-- 
1.6.5.1

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

[PATCH 2/8] 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/ioctl/ioctl-number.txt          |    3 +-
 Documentation/power/opportunistic-suspend.txt |   17 ++++
 include/linux/suspend_block_dev.h             |   25 +++++
 kernel/power/Kconfig                          |    9 ++
 kernel/power/Makefile                         |    1 +
 kernel/power/user_suspend_blocker.c           |  128 +++++++++++++++++++++++++
 6 files changed, 182 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index dd5806f..e2458f7 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
 'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
 		linux/ixjuser.h		<http://www.quicknet.net>
 'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
-'s'	all	linux/cdk.h
+'s'	all	linux/cdk.h	conflict!
+'s'	all	linux/suspend_block_dev.h	conflict!
 't'	00-7F	linux/if_ppp.h
 't'	80-8F	linux/isdn_ppp.h
 't'	90	linux/toshiba.h
diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
index 1a29d10..639da73 100644
--- a/Documentation/power/opportunistic-suspend.txt
+++ b/Documentation/power/opportunistic-suspend.txt
@@ -112,3 +112,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..24bc5c7
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
+#define _LINUX_SUSPEND_BLOCK_DEV_H
+
+#include <linux/ioctl.h>
+
+#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 55a06a1..fe5a2f2 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -146,6 +146,15 @@ config OPPORTUNISTIC_SUSPEND
 	  determines the sleep state the system will be put into when there are
 	  no active suspend blockers.
 
+config USER_SUSPEND_BLOCKERS
+	bool "Userspace suspend blockers"
+	depends on OPPORTUNISTIC_SUSPEND
+	default y
+	---help---
+	  User-space suspend block api. Creates a misc device with ioctls
+	  to create, block and unblock a suspend_blocker. The suspend_blocker
+	  will be deleted when the device is closed.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index ee5276d..78f703b 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..a9be6f4
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,128 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/suspend_blocker.h>
+#include <linux/suspend_block_dev.h>
+
+enum {
+	DEBUG_FAILURE	= BIT(0),
+};
+static int debug_mask = DEBUG_FAILURE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+static DEFINE_MUTEX(ioctl_lock);
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[0];
+};
+
+static int create_user_suspend_blocker(struct file *file, void __user *name,
+				 size_t name_len)
+{
+	struct user_suspend_blocker *bl;
+	if (file->private_data)
+		return -EBUSY;
+	if (name_len > NAME_MAX)
+		return -ENAMETOOLONG;
+	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+	if (copy_from_user(bl->name, name, name_len))
+		goto err_fault;
+	suspend_blocker_init(&bl->blocker, bl->name);
+	file->private_data = bl;
+	return 0;
+
+err_fault:
+	kfree(bl);
+	return -EFAULT;
+}
+
+static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
+				unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *bl;
+	long ret;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
+		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	bl = file->private_data;
+	if (!bl) {
+		ret = -ENOENT;
+		goto done;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&bl->blocker);
+		ret = 0;
+		break;
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&bl->blocker);
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTSUPP;
+	}
+done:
+	if (ret && debug_mask & DEBUG_FAILURE)
+		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
+			cmd, ret);
+	mutex_unlock(&ioctl_lock);
+	return ret;
+}
+
+static int user_suspend_blocker_release(struct inode *inode, struct file *file)
+{
+	struct user_suspend_blocker *bl = file->private_data;
+	if (!bl)
+		return 0;
+	suspend_blocker_destroy(&bl->blocker);
+	kfree(bl);
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.5.1

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

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

[Arve Hjønnevåg]

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

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

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

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

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

[PATCH 4/8] 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 9459361..ee43490 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;
@@ -105,8 +122,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.
  */
@@ -255,6 +272,19 @@ int request_suspend_state(suspend_state_t state)
 	return 0;
 }
 
+static int suspend_blocker_stats_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, suspend_blocker_stats_show, NULL);
+}
+
+static const struct file_operations suspend_blocker_stats_fops = {
+	.owner = THIS_MODULE,
+	.open = suspend_blocker_stats_open,
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
 static int __init suspend_block_init(void)
 {
 	suspend_work_queue = create_singlethread_workqueue("suspend");
@@ -266,4 +296,14 @@ static int __init suspend_block_init(void)
 	return 0;
 }
 
+static int __init suspend_block_postcore_init(void)
+{
+	if (!suspend_work_queue)
+		return 0;
+	suspend_blocker_stats_dentry = debugfs_create_file("suspend_blockers",
+			S_IRUGO, NULL, NULL, &suspend_blocker_stats_fops);
+	return 0;
+}
+
 core_initcall(suspend_block_init);
+postcore_initcall(suspend_block_postcore_init);
-- 
1.6.5.1

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

[PATCH 5/8] PM: suspend_block: Add suspend_blocker stats

[Arve Hjønnevåg]

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

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

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index f9928cc..c80764c 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
  * opportunistic 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 fe5a2f2..4bcba07 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -146,6 +146,13 @@ config OPPORTUNISTIC_SUSPEND
 	  determines the sleep state the system will be put into when there are
 	  no active suspend blockers.
 
+config SUSPEND_BLOCKER_STATS
+	bool "Suspend block stats"
+	depends on OPPORTUNISTIC_SUSPEND
+	default y
+	---help---
+	  Report suspend block stats in /sys/kernel/debug/suspend_blockers
+
 config USER_SUSPEND_BLOCKERS
 	bool "Userspace suspend blockers"
 	depends on OPPORTUNISTIC_SUSPEND
diff --git a/kernel/power/power.h b/kernel/power/power.h
index 9b468d7..75b8849 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -240,4 +240,8 @@ static inline void suspend_thaw_processes(void)
 /* kernel/power/suspend_block.c */
 extern int request_suspend_state(suspend_state_t state);
 extern bool request_suspend_valid_state(suspend_state_t state);
-
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+void about_to_enter_suspend(void);
+#else
+static inline void about_to_enter_suspend(void) {}
+#endif
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index dc42006..6d327ea 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -159,8 +159,10 @@ static int suspend_enter(suspend_state_t state)
 
 	error = sysdev_suspend(PMSG_SUSPEND);
 	if (!error) {
-		if (!suspend_is_blocked() && !suspend_test(TEST_CORE))
+		if (!suspend_is_blocked() && !suspend_test(TEST_CORE)) {
+			about_to_enter_suspend();
 			error = suspend_ops->enter(state);
+		}
 		sysdev_resume();
 	}
 
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index ee43490..2d43f37 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,20 +252,31 @@ 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: enter suspend\n");
+			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);
+	ret = pm_suspend(requested_suspend_state);
 
-		if (debug_mask & DEBUG_EXIT_SUSPEND)
-			pr_info_time("suspend: exit suspend, ret = %d ", ret);
+	if (debug_mask & DEBUG_EXIT_SUSPEND)
+		pr_info_time("suspend: exit suspend, ret = %d ", ret);
 
-		if (current_event_num == entry_event_num)
+	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);
 	}
+abort:
 	enable_suspend_blockers = false;
 }
 static DECLARE_WORK(suspend_work, suspend_worker);
@@ -141,6 +303,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);
 }
@@ -160,6 +323,7 @@ void suspend_blocker_destroy(struct suspend_blocker *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))
@@ -182,6 +346,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);
 
@@ -191,6 +356,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);
@@ -212,6 +381,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);
 
@@ -224,6 +395,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);
 }
@@ -293,6 +465,11 @@ static int __init suspend_block_init(void)
 
 	suspend_blocker_init(&main_suspend_blocker, "main");
 	suspend_block(&main_suspend_blocker);
+	suspend_blocker_init(&unknown_wakeup, "unknown_wakeups");
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+	suspend_blocker_init(&deleted_suspend_blockers,
+				"deleted_suspend_blockers");
+#endif
 	return 0;
 }
 
-- 
1.6.5.1

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

[PATCH 6/8] PM: Add suspend blocking work.

[Arve Hjønnevåg]

Allow work to be queued that will block suspend while it is pending
or executing. To get the same functionality in the calling code often
requires a separate suspend_blocker for pending and executing work, or
additional state and locking. This implementation does add additional
state and locking, but this can be removed later if we add support for
suspend blocking work to the core workqueue code.

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

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index c80764c..bf41a57 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -18,6 +18,7 @@
 
 #include <linux/list.h>
 #include <linux/ktime.h>
+#include <linux/workqueue.h>
 
 /**
  * struct suspend_blocker - the basic suspend_blocker structure
@@ -57,6 +58,38 @@ struct suspend_blocker {
 #endif
 };
 
+/**
+ * struct suspend_blocking_work - the basic suspend_blocking_work structure
+ * @work:		Standard work struct.
+ * @suspend_blocker:	Suspend blocker.
+ * @func:		Callback.
+ * @lock:		Spinlock protecting pending and running state.
+ * @active:		Number of cpu workqueues where work is pending or
+ *			callback is running.
+ *
+ * When suspend blocking work is pending or its callback is running it prevents
+ * the system from entering opportunistic suspend.
+ *
+ * The suspend_blocking_work structure must be initialized by
+ * suspend_blocking_work_init().
+ */
+
+struct suspend_blocking_work {
+	struct work_struct work;
+#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
+	struct suspend_blocker suspend_blocker;
+	work_func_t func;
+	spinlock_t lock;
+	int active;
+#endif
+};
+
+static inline struct suspend_blocking_work *to_suspend_blocking_work(
+	struct work_struct *work)
+{
+	return container_of(work, struct suspend_blocking_work, work);
+}
+
 #ifdef CONFIG_OPPORTUNISTIC_SUSPEND
 
 void suspend_blocker_init(struct suspend_blocker *blocker, const char *name);
@@ -66,6 +99,14 @@ void suspend_unblock(struct suspend_blocker *blocker);
 bool suspend_blocker_is_active(struct suspend_blocker *blocker);
 bool suspend_is_blocked(void);
 
+void suspend_blocking_work_init(struct suspend_blocking_work *work,
+				work_func_t func, const char *name);
+void suspend_blocking_work_destroy(struct suspend_blocking_work *work);
+int queue_suspend_blocking_work(struct workqueue_struct *wq,
+				struct suspend_blocking_work *work);
+int schedule_suspend_blocking_work(struct suspend_blocking_work *work);
+int cancel_suspend_blocking_work_sync(struct suspend_blocking_work *work);
+
 #else
 
 static inline void suspend_blocker_init(struct suspend_blocker *blocker,
@@ -77,6 +118,32 @@ static inline bool suspend_blocker_is_active(struct suspend_blocker *bl)
 								{ return 0; }
 static inline bool suspend_is_blocked(void) { return 0; }
 
+static inline void suspend_blocking_work_init(
+	struct suspend_blocking_work *work, work_func_t func, const char *name)
+{
+	INIT_WORK(&work->work, func);
+}
+static inline void suspend_blocking_work_destroy(
+	struct suspend_blocking_work *work)
+{
+	cancel_work_sync(&work->work);
+}
+static inline int queue_suspend_blocking_work(
+	struct workqueue_struct *wq, struct suspend_blocking_work *work)
+{
+	return queue_work(wq, &work->work);
+}
+static inline int schedule_suspend_blocking_work(
+	struct suspend_blocking_work *work)
+{
+	return schedule_work(&work->work);
+}
+static inline int cancel_suspend_blocking_work_sync(
+	struct suspend_blocking_work *work)
+{
+	return cancel_work_sync(&work->work);
+}
+
 #endif
 
 #endif
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 2d43f37..f9c6206 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -484,3 +484,110 @@ static int __init suspend_block_postcore_init(void)
 
 core_initcall(suspend_block_init);
 postcore_initcall(suspend_block_postcore_init);
+
+static void suspend_blocking_work_complete(struct suspend_blocking_work *work)
+{
+	unsigned long flags;
+
+	WARN_ON(!work->active);
+	spin_lock_irqsave(&work->lock, flags);
+	if (!--work->active)
+		suspend_unblock(&work->suspend_blocker);
+	spin_unlock_irqrestore(&work->lock, flags);
+}
+
+static void suspend_blocking_work_func(struct work_struct *work)
+{
+	struct suspend_blocking_work *sbwork = to_suspend_blocking_work(work);
+
+	sbwork->func(work);
+	suspend_blocking_work_complete(sbwork);
+}
+
+/**
+ * suspend_blocking_work_init - Initialize suspend_blocking_work
+ * @work: The work item in question.
+ * @func: Callback.
+ * @name: Name for suspend blocker.
+ *
+ */
+void suspend_blocking_work_init(struct suspend_blocking_work *work,
+				work_func_t func, const char *name)
+{
+	INIT_WORK(&work->work, suspend_blocking_work_func);
+	suspend_blocker_init(&work->suspend_blocker, name);
+	work->func = func;
+	spin_lock_init(&work->lock);
+	work->active = 0;
+}
+EXPORT_SYMBOL(suspend_blocking_work_init);
+
+/**
+ * cancel_suspend_blocking_work_sync - Cancel suspend_blocking_work
+ * @work: The work item in question
+ */
+int cancel_suspend_blocking_work_sync(struct suspend_blocking_work *work)
+{
+	int ret;
+
+	ret = cancel_work_sync(&work->work);
+	if (ret)
+		suspend_blocking_work_complete(work);
+	return ret;
+}
+EXPORT_SYMBOL(cancel_suspend_blocking_work_sync);
+
+/**
+ * suspend_blocking_work_destroy - Destroy suspend_blocking_work
+ * @work: The work item in question
+ *
+ * If the work was ever queued on more then one workqueue all but the last
+ * workqueue must be flushed before calling suspend_blocking_work_destroy.
+ */
+void suspend_blocking_work_destroy(struct suspend_blocking_work *work)
+{
+	cancel_suspend_blocking_work_sync(work);
+	WARN_ON(work->active);
+	suspend_blocker_destroy(&work->suspend_blocker);
+}
+EXPORT_SYMBOL(suspend_blocking_work_destroy);
+
+/**
+ * queue_suspend_blocking_work - Queue suspend blocking work
+ * @wq:		Workqueue to queue work on.
+ * @work:	The work item in question.
+ */
+int queue_suspend_blocking_work(struct workqueue_struct *wq,
+				struct suspend_blocking_work *work)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&work->lock, flags);
+	suspend_block(&work->suspend_blocker);
+	ret = queue_work(wq, &work->work);
+	if (ret)
+		work->active++;
+	spin_unlock_irqrestore(&work->lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL(queue_suspend_blocking_work);
+
+/**
+ * schedule_suspend_blocking_work - Queue suspend blocking work
+ * @work:	The work item in question.
+ */
+int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
+{
+	int ret;
+	unsigned long flags;
+
+	spin_lock_irqsave(&work->lock, flags);
+	suspend_block(&work->suspend_blocker);
+	ret = schedule_work(&work->work);
+	if (ret)
+		work->active++;
+	spin_unlock_irqrestore(&work->lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL(schedule_suspend_blocking_work);
-- 
1.6.5.1

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

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

[Arve Hjønnevåg]

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

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

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

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

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

[Arve Hjønnevåg]

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

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

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

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

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

[Pavel Machek]

On Tue 2010-04-27 21:31:54, Arve Hj??nnev??g wrote:
> 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>

ack.

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

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

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Pavel Machek]

Hi!

> Allow work to be queued that will block suspend while it is pending
> or executing. To get the same functionality in the calling code often
> requires a separate suspend_blocker for pending and executing work, or
> additional state and locking. This implementation does add additional
> state and locking, but this can be removed later if we add support for
> suspend blocking work to the core workqueue code.
> 
> Signed-off-by: Arve Hj??nnev??g <arve@android.com>

Seems sane. ACK.

> +struct suspend_blocking_work {
> +	struct work_struct work;
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND

Cound we name it C_AUTO_SUSPEND... to reduce length and typo
potential?

> +	struct suspend_blocker suspend_blocker;
> +	work_func_t func;
> +	spinlock_t lock;
> +	int active;

Is the lock internal-use, or is API user allowed to use it?
								Pavel

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

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

[Pavel Machek]

Hi!

> Adds /sys/power/policy that selects the behaviour of /sys/power/state.
> After setting the policy to opportunistic, writes to /sys/power/state
> become non-blocking requests that specify which suspend state to enter
> when no suspend blockers are active. A special state, "on", stops the
> process by activating the "main" suspend blocker.

I really don't like how this changes semantics of 'state'. I guess I'd
prefer leaving state as is -- forced transition to hibernation while
system is set to opportunistically suspend seems sane -- and adding
something like

/sys/power/autosleep

with 'off' or 'suspend' values?


> 
> Signed-off-by: Arve Hj??nnev??g <arve@android.com>
> ---
>  Documentation/power/opportunistic-suspend.txt |  114 +++++++++++
>  include/linux/suspend_blocker.h               |   64 ++++++
>  kernel/power/Kconfig                          |   16 ++
>  kernel/power/Makefile                         |    1 +
>  kernel/power/main.c                           |   89 ++++++++-
>  kernel/power/power.h                          |    5 +
>  kernel/power/suspend.c                        |    4 +-
>  kernel/power/suspend_blocker.c                |  269 +++++++++++++++++++++++++
>  8 files changed, 556 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/power/opportunistic-suspend.txt
>  create mode 100755 include/linux/suspend_blocker.h
>  create mode 100644 kernel/power/suspend_blocker.c
> 
> diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
> new file mode 100644
> index 0000000..1a29d10
> --- /dev/null
> +++ b/Documentation/power/opportunistic-suspend.txt
> @@ -0,0 +1,114 @@
> +Opportunistic Suspend
> +=====================
> +
> +Opportunistic suspend is a feature allowing the system to be suspended (ie. put
> +into one of the available sleep states) automatically whenever it is regarded
> +as idle.  The suspend blockers framework described below is used to determine
> +when that happens.
> +
> +The /sys/power/policy sysfs attribute is used to switch the system between the
> +opportunistic and "forced" suspend behavior, where in the latter case the
> +system is only suspended if a specific value, corresponding to one of the
> +available system sleep states, is written into /sys/power/state.  However, in
> +the former, opportunistic, case the system is put into the sleep state
> +corresponding to the value written to /sys/power/state whenever there are no
> +active suspend blockers. The default policy is "forced". Also, suspend blockers
> +do not affect sleep states entered from idle.
> +
> +When the policy is "opportunisic", there is a special value, "on", that can be
> +written to /sys/power/state. This will block the automatic sleep request, as if
> +a suspend blocker was used by a device driver. This way the opportunistic
> +suspend may be blocked by user space whithout switching back to the "forced"
> +mode.
> +
> +A suspend blocker is an object used to inform the PM subsystem when the system
> +can or cannot be suspended in the "opportunistic" mode (the "forced" mode
> +ignores suspend blockers).  To use it, a device driver creates a struct
> +suspend_blocker that must be initialized with suspend_blocker_init(). Before
> +freeing the suspend_blocker structure or its name, suspend_blocker_destroy()
> +must be called on it.
> +
> +A suspend blocker is activated using suspend_block(), which prevents the PM
> +subsystem from putting the system into the requested sleep state in the
> +"opportunistic" mode until the suspend blocker is deactivated with
> +suspend_unblock(). Multiple suspend blockers may be active simultaneously, and
> +the system will not suspend as long as at least one of them is active.
> +
> +If opportunistic suspend is already in progress when suspend_block() is called,
> +it will abort the suspend, unless suspend_ops->enter has already been
> +executed. If suspend is aborted this way, the system is usually not fully
> +operational at that point. The suspend callbacks of some drivers may still be
> +running and it usually takes time to restore the system to the fully operational
> +state.
> +
> +For example, 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 (i.e., these functions don't nest). This allows
> +drivers to update their state and call suspend suspend_block or suspend_unblock
> +based on the result.
> +For instance:
> +
> +if (list_empty(&state->pending_work))
> +	suspend_unblock(&state->suspend_blocker);
> +else
> +	suspend_block(&state->suspend_blocker);
> +
> diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
> new file mode 100755
> index 0000000..f9928cc
> --- /dev/null
> +++ b/include/linux/suspend_blocker.h
> @@ -0,0 +1,64 @@
> +/* include/linux/suspend_blocker.h
> + *
> + * Copyright (C) 2007-2009 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _LINUX_SUSPEND_BLOCKER_H
> +#define _LINUX_SUSPEND_BLOCKER_H
> +
> +#include <linux/list.h>
> +
> +/**
> + * struct suspend_blocker - the basic suspend_blocker structure
> + * @link:	List entry for active or inactive list.
> + * @flags:	Tracks initialized and active state.
> + * @name:	Name used for debugging.
> + *
> + * When a suspend_blocker is active it prevents the system from entering
> + * opportunistic suspend.
> + *
> + * The suspend_blocker structure must be initialized by suspend_blocker_init()
> + */
> +
> +struct suspend_blocker {
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +	struct list_head    link;
> +	int                 flags;
> +	const char         *name;
> +#endif
> +};
> +
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +
> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name);
> +void suspend_blocker_destroy(struct suspend_blocker *blocker);
> +void suspend_block(struct suspend_blocker *blocker);
> +void suspend_unblock(struct suspend_blocker *blocker);
> +bool suspend_blocker_is_active(struct suspend_blocker *blocker);
> +bool suspend_is_blocked(void);
> +
> +#else
> +
> +static inline void suspend_blocker_init(struct suspend_blocker *blocker,
> +					const char *name) {}
> +static inline void suspend_blocker_destroy(struct suspend_blocker *blocker) {}
> +static inline void suspend_block(struct suspend_blocker *blocker) {}
> +static inline void suspend_unblock(struct suspend_blocker *blocker) {}
> +static inline bool suspend_blocker_is_active(struct suspend_blocker *bl)
> +								{ return 0; }
> +static inline bool suspend_is_blocked(void) { return 0; }
> +
> +#endif
> +
> +#endif
> +
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 5c36ea9..55a06a1 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -130,6 +130,22 @@ config SUSPEND_FREEZER
>  
>  	  Turning OFF this setting is NOT recommended! If in doubt, say Y.
>  
> +config OPPORTUNISTIC_SUSPEND
> +	bool "Suspend blockers"
> +	depends on PM_SLEEP
> +	select RTC_LIB
> +	default n
> +	---help---
> +	  Opportunistic sleep support.  Allows the system to be put into a sleep
> +	  state opportunistically, if it doesn't do any useful work at the
> +	  moment. The PM subsystem is switched into this mode of operation by
> +	  writing "opportunistic" into /sys/power/policy, while writing
> +	  "forced" to this file turns the opportunistic suspend feature off.
> +	  In the "opportunistic" mode suspend blockers are used to determine
> +	  when to suspend the system and the value written to /sys/power/state
> +	  determines the sleep state the system will be put into when there are
> +	  no active suspend blockers.
> +
>  config HIBERNATION_NVS
>  	bool
>  
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index 4319181..ee5276d 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_PM)		+= main.o
>  obj-$(CONFIG_PM_SLEEP)		+= console.o
>  obj-$(CONFIG_FREEZER)		+= process.o
>  obj-$(CONFIG_SUSPEND)		+= suspend.o
> +obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
>  obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index b58800b..5f0af6c 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -12,6 +12,7 @@
>  #include <linux/string.h>
>  #include <linux/resume-trace.h>
>  #include <linux/workqueue.h>
> +#include <linux/suspend_blocker.h>
>  
>  #include "power.h"
>  
> @@ -20,6 +21,27 @@ DEFINE_MUTEX(pm_mutex);
>  unsigned int pm_flags;
>  EXPORT_SYMBOL(pm_flags);
>  
> +struct policy {
> +	const char *name;
> +	bool (*valid_state)(suspend_state_t state);
> +	int (*set_state)(suspend_state_t state);
> +};
> +static struct policy policies[] = {
> +	{
> +		.name		= "forced",
> +		.valid_state	= valid_state,
> +		.set_state	= enter_state,
> +	},
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +	{
> +		.name		= "opportunistic",
> +		.valid_state	= request_suspend_valid_state,
> +		.set_state	= request_suspend_state,
> +	},
> +#endif
> +};
> +static int policy;
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  /* Routines for PM-transition notifications */
> @@ -146,6 +168,12 @@ struct kobject *power_kobj;
>   *
>   *	store() accepts one of those strings, translates it into the 
>   *	proper enumerated value, and initiates a suspend transition.
> + *
> + *	If policy is set to opportunistic, store() does not block until the
> + *	system resumes, and it will try to re-enter the state until another
> + *	state is requested. Suspend blockers are respected and the requested
> + *	state will only be entered when no suspend blockers are active.
> + *	Write "on" to cancel.
>   */
>  static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			  char *buf)
> @@ -155,12 +183,13 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  	int i;
>  
>  	for (i = 0; i < PM_SUSPEND_MAX; i++) {
> -		if (pm_states[i] && valid_state(i))
> +		if (pm_states[i] && policies[policy].valid_state(i))
>  			s += sprintf(s,"%s ", pm_states[i]);
>  	}
>  #endif
>  #ifdef CONFIG_HIBERNATION
> -	s += sprintf(s, "%s\n", "disk");
> +	if (!policy)
> +		s += sprintf(s, "%s\n", "disk");
>  #else
>  	if (s != buf)
>  		/* convert the last space to a newline */
> @@ -173,7 +202,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			   const char *buf, size_t n)
>  {
>  #ifdef CONFIG_SUSPEND
> -	suspend_state_t state = PM_SUSPEND_STANDBY;
> +	suspend_state_t state = PM_SUSPEND_ON;
>  	const char * const *s;
>  #endif
>  	char *p;
> @@ -184,7 +213,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	len = p ? p - buf : n;
>  
>  	/* First, check if we are requested to hibernate */
> -	if (len == 4 && !strncmp(buf, "disk", len)) {
> +	if (len == 4 && !strncmp(buf, "disk", len) && !policy) {
>  		error = hibernate();
>    goto Exit;
>  	}
> @@ -195,7 +224,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			break;
>  	}
>  	if (state < PM_SUSPEND_MAX && *s)
> -		error = enter_state(state);
> +		error = policies[policy].set_state(state);
>  #endif
>  
>   Exit:
> @@ -204,6 +233,55 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(state);
>  
> +/**
> + *	policy - set policy for state
> + */
> +
> +static ssize_t policy_show(struct kobject *kobj,
> +			   struct kobj_attribute *attr, char *buf)
> +{
> +	char *s = buf;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		if (i == policy)
> +			s += sprintf(s, "[%s] ", policies[i].name);
> +		else
> +			s += sprintf(s, "%s ", policies[i].name);
> +	}
> +	if (s != buf)
> +		/* convert the last space to a newline */
> +		*(s-1) = '\n';
> +	return (s - buf);
> +}
> +
> +static ssize_t policy_store(struct kobject *kobj,
> +			    struct kobj_attribute *attr,
> +			    const char *buf, size_t n)
> +{
> +	const char *s;
> +	char *p;
> +	int len;
> +	int i;
> +
> +	p = memchr(buf, '\n', n);
> +	len = p ? p - buf : n;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		s = policies[i].name;
> +		if (s && len == strlen(s) && !strncmp(buf, s, len)) {
> +			mutex_lock(&pm_mutex);
> +			policies[policy].set_state(PM_SUSPEND_ON);
> +			policy = i;
> +			mutex_unlock(&pm_mutex);
> +			return n;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +power_attr(policy);
> +
>  #ifdef CONFIG_PM_TRACE
>  int pm_trace_enabled;
>  
> @@ -231,6 +309,7 @@ power_attr(pm_trace);
>  
>  static struct attribute * g[] = {
>  	&state_attr.attr,
> +	&policy_attr.attr,
>  #ifdef CONFIG_PM_TRACE
>  	&pm_trace_attr.attr,
>  #endif
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index 46c5a26..9b468d7 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -236,3 +236,8 @@ static inline void suspend_thaw_processes(void)
>  {
>  }
>  #endif
> +
> +/* kernel/power/suspend_block.c */
> +extern int request_suspend_state(suspend_state_t state);
> +extern bool request_suspend_valid_state(suspend_state_t state);
> +
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 56e7dbb..dc42006 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -16,10 +16,12 @@
>  #include <linux/cpu.h>
>  #include <linux/syscalls.h>
>  #include <linux/gfp.h>
> +#include <linux/suspend_blocker.h>
>  
>  #include "power.h"
>  
>  const char *const pm_states[PM_SUSPEND_MAX] = {
> +	[PM_SUSPEND_ON]		= "on",
>  	[PM_SUSPEND_STANDBY]	= "standby",
>  	[PM_SUSPEND_MEM]	= "mem",
>  };
> @@ -157,7 +159,7 @@ static int suspend_enter(suspend_state_t state)
>  
>  	error = sysdev_suspend(PMSG_SUSPEND);
>  	if (!error) {
> -		if (!suspend_test(TEST_CORE))
> +		if (!suspend_is_blocked() && !suspend_test(TEST_CORE))
>  			error = suspend_ops->enter(state);
>  		sysdev_resume();
>  	}
> diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
> new file mode 100644
> index 0000000..9459361
> --- /dev/null
> +++ b/kernel/power/suspend_blocker.c
> @@ -0,0 +1,269 @@
> +/* kernel/power/suspend_blocker.c
> + *
> + * Copyright (C) 2005-2010 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/rtc.h>
> +#include <linux/suspend.h>
> +#include <linux/suspend_blocker.h>
> +#include "power.h"
> +
> +enum {
> +	DEBUG_EXIT_SUSPEND = 1U << 0,
> +	DEBUG_WAKEUP = 1U << 1,
> +	DEBUG_USER_STATE = 1U << 2,
> +	DEBUG_SUSPEND = 1U << 3,
> +	DEBUG_SUSPEND_BLOCKER = 1U << 4,
> +};
> +static int debug_mask = DEBUG_EXIT_SUSPEND | DEBUG_WAKEUP | DEBUG_USER_STATE;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +#define SB_INITIALIZED            (1U << 8)
> +#define SB_ACTIVE                 (1U << 9)
> +
> +static DEFINE_SPINLOCK(list_lock);
> +static DEFINE_SPINLOCK(state_lock);
> +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;
> +static bool enable_suspend_blockers;
> +
> +#define pr_info_time(fmt, args...) \
> +	do { \
> +		struct timespec ts; \
> +		struct rtc_time tm; \
> +		getnstimeofday(&ts); \
> +		rtc_time_to_tm(ts.tv_sec, &tm); \
> +		pr_info(fmt "(%d-%02d-%02d %02d:%02d:%02d.%09lu UTC)\n" , \
> +			args, \
> +			tm.tm_year + 1900, tm.tm_mon + 1, tm.tm_mday, \
> +			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
> +	} while (0);
> +
> +static void print_active_blockers_locked(void)
> +{
> +	struct suspend_blocker *blocker;
> +
> +	list_for_each_entry(blocker, &active_blockers, link)
> +		pr_info("active suspend blocker %s\n", blocker->name);
> +}
> +
> +/**
> + * suspend_is_blocked() - Check if suspend should be blocked
> + *
> + * suspend_is_blocked can be used by generic power management code to abort
> + * suspend.
> + *
> + * To preserve backward compatibility suspend_is_blocked returns 0 unless it
> + * is called during suspend initiated from the suspend_block code.
> + */
> +bool suspend_is_blocked(void)
> +{
> +	if (!enable_suspend_blockers)
> +		return 0;
> +	return !list_empty(&active_blockers);
> +}
> +
> +static void suspend_worker(struct work_struct *work)
> +{
> +	int ret;
> +	int entry_event_num;
> +
> +	enable_suspend_blockers = true;
> +	while (!suspend_is_blocked()) {
> +		entry_event_num = current_event_num;
> +
> +		if (debug_mask & DEBUG_SUSPEND)
> +			pr_info("suspend: enter suspend\n");
> +
> +		ret = pm_suspend(requested_suspend_state);
> +
> +		if (debug_mask & DEBUG_EXIT_SUSPEND)
> +			pr_info_time("suspend: exit suspend, ret = %d ", ret);
> +
> +		if (current_event_num == entry_event_num)
> +			pr_info("suspend: pm_suspend returned with no event\n");
> +	}
> +	enable_suspend_blockers = false;
> +}
> +static DECLARE_WORK(suspend_work, suspend_worker);
> +
> +/**
> + * suspend_blocker_init() - Initialize a suspend blocker
> + * @blocker:	The suspend blocker to initialize.
> + * @name:	The name of the suspend blocker to show in debug messages.
> + *
> + * The suspend blocker struct and name must not be freed before calling
> + * suspend_blocker_destroy.
> + */
> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)
> +{
> +	unsigned long irqflags = 0;
> +
> +	WARN_ON(!name);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_init name=%s\n", name);
> +
> +	blocker->name = name;
> +	blocker->flags = SB_INITIALIZED;
> +	INIT_LIST_HEAD(&blocker->link);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_add(&blocker->link, &inactive_blockers);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_blocker_init);
> +
> +/**
> + * suspend_blocker_destroy() - Destroy a suspend blocker
> + * @blocker:	The suspend blocker to destroy.
> + */
> +void suspend_blocker_destroy(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	blocker->flags &= ~SB_INITIALIZED;
> +	list_del(&blocker->link);
> +	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
> +		queue_work(suspend_work_queue, &suspend_work);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_blocker_destroy);
> +
> +/**
> + * suspend_block() - Block suspend
> + * @blocker:	The suspend blocker to use
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_block(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	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);
> +
> +	current_event_num++;
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_block);
> +
> +/**
> + * suspend_unblock() - Unblock suspend
> + * @blocker:	The suspend blocker to unblock.
> + *
> + * If no other suspend blockers block suspend, the system will suspend.
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_unblock(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_unblock: %s\n", blocker->name);
> +
> +	list_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);
> +	if (blocker == &main_suspend_blocker) {
> +		if (debug_mask & DEBUG_SUSPEND)
> +			print_active_blockers_locked();
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_unblock);
> +
> +/**
> + * suspend_blocker_is_active() - Test if a suspend blocker is blocking suspend
> + * @blocker:	The suspend blocker to check.
> + *
> + * Returns true if the suspend_blocker is currently active.
> + */
> +bool suspend_blocker_is_active(struct suspend_blocker *blocker)
> +{
> +	WARN_ON(!(blocker->flags & SB_INITIALIZED));
> +
> +	return !!(blocker->flags & SB_ACTIVE);
> +}
> +EXPORT_SYMBOL(suspend_blocker_is_active);
> +
> +bool request_suspend_valid_state(suspend_state_t state)
> +{
> +	return (state == PM_SUSPEND_ON) || valid_state(state);
> +}
> +
> +int request_suspend_state(suspend_state_t state)
> +{
> +	unsigned long irqflags;
> +
> +	if (!request_suspend_valid_state(state))
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&state_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_USER_STATE)
> +		pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
> +			     state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> +			     requested_suspend_state, state,
> +			     ktime_to_ns(ktime_get()));
> +
> +	requested_suspend_state = state;
> +	if (state == PM_SUSPEND_ON)
> +		suspend_block(&main_suspend_blocker);
> +	else
> +		suspend_unblock(&main_suspend_blocker);
> +	spin_unlock_irqrestore(&state_lock, irqflags);
> +	return 0;
> +}
> +
> +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);

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

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Tejun Heo]

Hello,

> +static void suspend_blocking_work_complete(struct suspend_blocking_work *work)
> +{
> +	unsigned long flags;
> +
> +	WARN_ON(!work->active);
> +	spin_lock_irqsave(&work->lock, flags);
> +	if (!--work->active)
> +		suspend_unblock(&work->suspend_blocker);
> +	spin_unlock_irqrestore(&work->lock, flags);
> +}

Maybe work->active can be an atomic_t and the lock can be removed?

> +/**
> + * suspend_blocking_work_destroy - Destroy suspend_blocking_work
> + * @work: The work item in question
> + *
> + * If the work was ever queued on more then one workqueue all but the last
> + * workqueue must be flushed before calling suspend_blocking_work_destroy.

As it's calling cancel_work_sync(), the above is not true.  As long as
no one is trying to queue it again, suspend_blocking_work_destroy() is
safe to call regardless of how the work has been used.

> +void suspend_blocking_work_destroy(struct suspend_blocking_work *work)
> +{
> +	cancel_suspend_blocking_work_sync(work);
> +	WARN_ON(work->active);
> +	suspend_blocker_destroy(&work->suspend_blocker);
> +}
> +EXPORT_SYMBOL(suspend_blocking_work_destroy);

Other than the above, it looks good to me.

Thanks.

-- 
tejun

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Arve Hjønnevåg]

2010/4/27 Tejun Heo <tj@kernel.org>:
> Hello,
>
>> +static void suspend_blocking_work_complete(struct suspend_blocking_work *work)
>> +{
>> +     unsigned long flags;
>> +
>> +     WARN_ON(!work->active);
>> +     spin_lock_irqsave(&work->lock, flags);
>> +     if (!--work->active)
>> +             suspend_unblock(&work->suspend_blocker);
>> +     spin_unlock_irqrestore(&work->lock, flags);
>> +}
>
> Maybe work->active can be an atomic_t and the lock can be removed?
>

I need the spinlock to prevent the work from getting re-queued before
suspend_unblock.

>> +/**
>> + * suspend_blocking_work_destroy - Destroy suspend_blocking_work
>> + * @work: The work item in question
>> + *
>> + * If the work was ever queued on more then one workqueue all but the last
>> + * workqueue must be flushed before calling suspend_blocking_work_destroy.
>
> As it's calling cancel_work_sync(), the above is not true.  As long as
> no one is trying to queue it again, suspend_blocking_work_destroy() is
> safe to call regardless of how the work has been used.
>

I'm not sure what the best terminology is here, but cancel_work_sync()
only waits for work running on all the cpu-workqueues of the last
workqueue. So, if the caller queued the work on more than one
workqueue, suspend_blocking_work_destroy does not ensure that the
suspend_blocking_work structure is not still in use (it should trigger
the WARN_ON though).

>> +void suspend_blocking_work_destroy(struct suspend_blocking_work *work)
>> +{
>> +     cancel_suspend_blocking_work_sync(work);
>> +     WARN_ON(work->active);
>> +     suspend_blocker_destroy(&work->suspend_blocker);
>> +}
>> +EXPORT_SYMBOL(suspend_blocking_work_destroy);
>
> Other than the above, it looks good to me.
>
> Thanks.
>
> --
> tejun
>



-- 
Arve Hjønnevåg

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Tejun Heo]

Hello,

On 04/28/2010 09:02 AM, Arve Hjønnevåg wrote:
>> Maybe work->active can be an atomic_t and the lock can be removed?
> 
> I need the spinlock to prevent the work from getting re-queued before
> suspend_unblock.

OIC.

> I'm not sure what the best terminology is here, but cancel_work_sync()
> only waits for work running on all the cpu-workqueues of the last
> workqueue. So, if the caller queued the work on more than one
> workqueue, suspend_blocking_work_destroy does not ensure that the
> suspend_blocking_work structure is not still in use (it should trigger
> the WARN_ON though).

Right, I was thinking about different cpu_workqueues and yeah, the
terminology gets pretty confusing.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

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

[Alan Stern]

On Tue, 27 Apr 2010, [UTF-8] Arve Hjønnevåg wrote:

> +For example, 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       +++          +++

This is better than before, but it still isn't ideal.  Here's what I 
mean:

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

That's not right.  Handling the screen doesn't need suspend blockers:
The program decides what to do and then either turns on the screen or
else writes "mem" to /sys/power/state.  What suspend blockers add is
the ability to resolve races and satisfy multiple constraints when
going into suspend -- which has nothing to do with operating the
screen.

I _think_ what you're trying to get at can be expressed this way:

	Here's an example showing how a cell phone or other embedded
	system can handle keystrokes (or other input events) in the 
	presence of suspend blockers.  Use set_irq_wake...

	...

      - The user-space input-event thread returns from read.  It 
	carries out whatever activities are appropriate (for example,
	powering up the display screen, running other programs, and so 
	on).  When it is finished, it calls suspend_unblock on the
	process_input_events suspend_blocker and then calls select or
	poll.  The system will automatically suspend again when it is
	idle and no suspend blockers remain active.


> +/**
> + * suspend_block() - Block suspend
> + * @blocker:	The suspend blocker to use
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_block(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	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);

Here and in suspend_unblock(), you can use list_move() in place of 
list_del() followed by list_add().

Alan Stern

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Oleg Nesterov]

On 04/27, Arve Hjønnevåg wrote:
>
> Allow work to be queued that will block suspend while it is pending
> or executing. To get the same functionality in the calling code often
> requires a separate suspend_blocker for pending and executing work, or
> additional state and locking. This implementation does add additional
> state and locking, but this can be removed later if we add support for
> suspend blocking work to the core workqueue code.

I think this patch is fine.

Just one silly question,

> +int queue_suspend_blocking_work(struct workqueue_struct *wq,
> +				struct suspend_blocking_work *work)
> +{
> +	int ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&work->lock, flags);
> +	suspend_block(&work->suspend_blocker);
> +	ret = queue_work(wq, &work->work);
> +	if (ret)
> +		work->active++;

why not

	ret = queue_work(wq, &work->work);
	if (ret) {
		suspend_block(&work->suspend_blocker);
		work->active++;
	}

?

Afaics, we can't race with work->func() doing unblock, we hold work-lock.
And this way the code looks more clear.

Sorry, I had no chance to read the previous patches. After the quick look
at 1/8 I think it is OK to call suspend_block() twice, but still...

Or I missed something? Just curious.


Hmm... actually, queue_work() can also fail if we race with cancel_ which
temporary sets WORK_STRUCT_PENDING. In that case suspend_block() won't
be paired by unblock ?


> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
> +{
> ...
> +	ret = schedule_work(&work->work);

Off-topic. We should probably export keventd_wq to avoid the duplications
like this.

Oleg.

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Tejun Heo]

On 04/28/2010 09:40 PM, Oleg Nesterov wrote:
>> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
>> +{
>> ...
>> +	ret = schedule_work(&work->work);
> 
> Off-topic. We should probably export keventd_wq to avoid the duplications
> like this.

Yeah, had about the same thought.  cmwq exports it so I didn't suggest
it at this point but then again we don't really know whether or when
that series is going in so it might be a good idea to make that change
now.  Hmm...

Thanks.

-- 
tejun

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

[Rafael J. Wysocki]

On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
> Adds /sys/power/policy that selects the behaviour of /sys/power/state.
> After setting the policy to opportunistic, writes to /sys/power/state
> become non-blocking requests that specify which suspend state to enter
> when no suspend blockers are active. A special state, "on", stops the
> process by activating the "main" suspend blocker.
> 
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
...
> @@ -20,6 +21,27 @@ DEFINE_MUTEX(pm_mutex);
>  unsigned int pm_flags;
>  EXPORT_SYMBOL(pm_flags);
>  
> +struct policy {
> +	const char *name;
> +	bool (*valid_state)(suspend_state_t state);
> +	int (*set_state)(suspend_state_t state);
> +};
> +static struct policy policies[] = {
> +	{
> +		.name		= "forced",
> +		.valid_state	= valid_state,
> +		.set_state	= enter_state,
> +	},
> +#ifdef CONFIG_OPPORTUNISTIC_SUSPEND
> +	{
> +		.name		= "opportunistic",
> +		.valid_state	= request_suspend_valid_state,
> +		.set_state	= request_suspend_state,
> +	},
> +#endif
> +};
> +static int policy;
> +
>  #ifdef CONFIG_PM_SLEEP
>  
>  /* Routines for PM-transition notifications */
> @@ -146,6 +168,12 @@ struct kobject *power_kobj;
>   *
>   *	store() accepts one of those strings, translates it into the 
>   *	proper enumerated value, and initiates a suspend transition.
> + *
> + *	If policy is set to opportunistic, store() does not block until the
> + *	system resumes, and it will try to re-enter the state until another
> + *	state is requested. Suspend blockers are respected and the requested
> + *	state will only be entered when no suspend blockers are active.
> + *	Write "on" to cancel.
>   */
>  static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  			  char *buf)
> @@ -155,12 +183,13 @@ static ssize_t state_show(struct kobject *kobj, struct kobj_attribute *attr,
>  	int i;
>  
>  	for (i = 0; i < PM_SUSPEND_MAX; i++) {
> -		if (pm_states[i] && valid_state(i))
> +		if (pm_states[i] && policies[policy].valid_state(i))
>  			s += sprintf(s,"%s ", pm_states[i]);
>  	}
>  #endif
>  #ifdef CONFIG_HIBERNATION
> -	s += sprintf(s, "%s\n", "disk");
> +	if (!policy)
> +		s += sprintf(s, "%s\n", "disk");
>  #else
>  	if (s != buf)
>  		/* convert the last space to a newline */
> @@ -173,7 +202,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			   const char *buf, size_t n)
>  {
>  #ifdef CONFIG_SUSPEND
> -	suspend_state_t state = PM_SUSPEND_STANDBY;
> +	suspend_state_t state = PM_SUSPEND_ON;
>  	const char * const *s;
>  #endif
>  	char *p;
> @@ -184,7 +213,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  	len = p ? p - buf : n;
>  
>  	/* First, check if we are requested to hibernate */
> -	if (len == 4 && !strncmp(buf, "disk", len)) {
> +	if (len == 4 && !strncmp(buf, "disk", len) && !policy) {
>  		error = hibernate();
>    goto Exit;
>  	}
> @@ -195,7 +224,7 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  			break;
>  	}
>  	if (state < PM_SUSPEND_MAX && *s)
> -		error = enter_state(state);
> +		error = policies[policy].set_state(state);
>  #endif
>  
>   Exit:
> @@ -204,6 +233,55 @@ static ssize_t state_store(struct kobject *kobj, struct kobj_attribute *attr,
>  
>  power_attr(state);
>  
> +/**
> + *	policy - set policy for state
> + */
> +
> +static ssize_t policy_show(struct kobject *kobj,
> +			   struct kobj_attribute *attr, char *buf)
> +{
> +	char *s = buf;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		if (i == policy)
> +			s += sprintf(s, "[%s] ", policies[i].name);
> +		else
> +			s += sprintf(s, "%s ", policies[i].name);
> +	}
> +	if (s != buf)
> +		/* convert the last space to a newline */
> +		*(s-1) = '\n';
> +	return (s - buf);
> +}
> +
> +static ssize_t policy_store(struct kobject *kobj,
> +			    struct kobj_attribute *attr,
> +			    const char *buf, size_t n)
> +{
> +	const char *s;
> +	char *p;
> +	int len;
> +	int i;
> +
> +	p = memchr(buf, '\n', n);
> +	len = p ? p - buf : n;
> +
> +	for (i = 0; i < ARRAY_SIZE(policies); i++) {
> +		s = policies[i].name;
> +		if (s && len == strlen(s) && !strncmp(buf, s, len)) {
> +			mutex_lock(&pm_mutex);
> +			policies[policy].set_state(PM_SUSPEND_ON);
> +			policy = i;
> +			mutex_unlock(&pm_mutex);
> +			return n;
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +power_attr(policy);
> +
>  #ifdef CONFIG_PM_TRACE
>  int pm_trace_enabled;
>  

Would you mind if I changed the above so that "policy" doesn't even show up
if CONFIG_OPPORTUNISTIC_SUSPEND is unset?

...
> +static void suspend_worker(struct work_struct *work)
> +{
> +	int ret;
> +	int entry_event_num;
> +
> +	enable_suspend_blockers = true;
> +	while (!suspend_is_blocked()) {
> +		entry_event_num = current_event_num;
> +
> +		if (debug_mask & DEBUG_SUSPEND)
> +			pr_info("suspend: enter suspend\n");
> +
> +		ret = pm_suspend(requested_suspend_state);
> +
> +		if (debug_mask & DEBUG_EXIT_SUSPEND)
> +			pr_info_time("suspend: exit suspend, ret = %d ", ret);
> +
> +		if (current_event_num == entry_event_num)
> +			pr_info("suspend: pm_suspend returned with no event\n");

Hmm, what exactly is this for?  It looks like a debug thing to me.  I'd use
pr_debug() here and in both debug printk()s above.  Would you agree?

> +	}
> +	enable_suspend_blockers = false;
> +}
> +static DECLARE_WORK(suspend_work, suspend_worker);
> +
> +/**
> + * suspend_blocker_init() - Initialize a suspend blocker
> + * @blocker:	The suspend blocker to initialize.
> + * @name:	The name of the suspend blocker to show in debug messages.
> + *
> + * The suspend blocker struct and name must not be freed before calling
> + * suspend_blocker_destroy.
> + */
> +void suspend_blocker_init(struct suspend_blocker *blocker, const char *name)
> +{
> +	unsigned long irqflags = 0;
> +
> +	WARN_ON(!name);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_init name=%s\n", name);
> +
> +	blocker->name = name;
> +	blocker->flags = SB_INITIALIZED;
> +	INIT_LIST_HEAD(&blocker->link);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	list_add(&blocker->link, &inactive_blockers);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_blocker_init);

Is there a strong objection to changing that (and the other instances below) to
EXPORT_SYMBOL_GPL?

> +
> +/**
> + * suspend_blocker_destroy() - Destroy a suspend blocker
> + * @blocker:	The suspend blocker to destroy.
> + */
> +void suspend_blocker_destroy(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_blocker_destroy name=%s\n", blocker->name);
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	blocker->flags &= ~SB_INITIALIZED;
> +	list_del(&blocker->link);
> +	if ((blocker->flags & SB_ACTIVE) && list_empty(&active_blockers))
> +		queue_work(suspend_work_queue, &suspend_work);
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_blocker_destroy);
> +
> +/**
> + * suspend_block() - Block suspend
> + * @blocker:	The suspend blocker to use
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_block(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +	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);
> +
> +	current_event_num++;
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_block);
> +
> +/**
> + * suspend_unblock() - Unblock suspend
> + * @blocker:	The suspend blocker to unblock.
> + *
> + * If no other suspend blockers block suspend, the system will suspend.
> + *
> + * It is safe to call this function from interrupt context.
> + */
> +void suspend_unblock(struct suspend_blocker *blocker)
> +{
> +	unsigned long irqflags;
> +
> +	if (WARN_ON(!(blocker->flags & SB_INITIALIZED)))
> +		return;
> +
> +	spin_lock_irqsave(&list_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_SUSPEND_BLOCKER)
> +		pr_info("suspend_unblock: %s\n", blocker->name);
> +
> +	list_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);
> +	if (blocker == &main_suspend_blocker) {
> +		if (debug_mask & DEBUG_SUSPEND)
> +			print_active_blockers_locked();
> +	}
> +	spin_unlock_irqrestore(&list_lock, irqflags);
> +}
> +EXPORT_SYMBOL(suspend_unblock);
> +
> +/**
> + * suspend_blocker_is_active() - Test if a suspend blocker is blocking suspend
> + * @blocker:	The suspend blocker to check.
> + *
> + * Returns true if the suspend_blocker is currently active.
> + */
> +bool suspend_blocker_is_active(struct suspend_blocker *blocker)
> +{
> +	WARN_ON(!(blocker->flags & SB_INITIALIZED));
> +
> +	return !!(blocker->flags & SB_ACTIVE);
> +}
> +EXPORT_SYMBOL(suspend_blocker_is_active);
> +
> +bool request_suspend_valid_state(suspend_state_t state)
> +{
> +	return (state == PM_SUSPEND_ON) || valid_state(state);
> +}
> +
> +int request_suspend_state(suspend_state_t state)
> +{
> +	unsigned long irqflags;
> +
> +	if (!request_suspend_valid_state(state))
> +		return -ENODEV;
> +
> +	spin_lock_irqsave(&state_lock, irqflags);
> +
> +	if (debug_mask & DEBUG_USER_STATE)
> +		pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
> +			     state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> +			     requested_suspend_state, state,
> +			     ktime_to_ns(ktime_get()));
> +
> +	requested_suspend_state = state;
> +	if (state == PM_SUSPEND_ON)
> +		suspend_block(&main_suspend_blocker);
> +	else
> +		suspend_unblock(&main_suspend_blocker);
> +	spin_unlock_irqrestore(&state_lock, irqflags);
> +	return 0;
> +}

I think the two functions above should be static, shouldn't they?

> +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);

Hmm.  Why don't you want to put that initialization into pm_init() (in
kernel/power/main.c)?

Also, we already have one PM workqueue.  It is used for runtime PM, but I guess
it may be used just as well for the opportunistic suspend.  It is freezable,
but would it hurt?

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

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

[Rafael J. Wysocki]

On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.
> 
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
...
> +
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/suspend_blocker.h>
> +#include <linux/suspend_block_dev.h>
> +
> +enum {
> +	DEBUG_FAILURE	= BIT(0),
> +};
> +static int debug_mask = DEBUG_FAILURE;

What's the exact purpose of this?

> +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];
> +};

Why is this not in a header?

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

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Rafael J. Wysocki]

On Wednesday 28 April 2010, Tejun Heo wrote:
> On 04/28/2010 09:40 PM, Oleg Nesterov wrote:
> >> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
> >> +{
> >> ...
> >> +	ret = schedule_work(&work->work);
> > 
> > Off-topic. We should probably export keventd_wq to avoid the duplications
> > like this.
> 
> Yeah, had about the same thought.  cmwq exports it so I didn't suggest
> it at this point but then again we don't really know whether or when
> that series is going in

As soon as there are no major objections.  At least to my tree.

> so it might be a good idea to make that change now.  Hmm...

I'd rather like a follow-up patch changing that, if poss.

Thanks,
Rafael

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Rafael J. Wysocki]

On Wednesday 28 April 2010, Oleg Nesterov wrote:
> On 04/27, Arve Hjønnevåg wrote:
> >
> > Allow work to be queued that will block suspend while it is pending
> > or executing. To get the same functionality in the calling code often
> > requires a separate suspend_blocker for pending and executing work, or
> > additional state and locking. This implementation does add additional
> > state and locking, but this can be removed later if we add support for
> > suspend blocking work to the core workqueue code.
> 
> I think this patch is fine.
> 
> Just one silly question,
> 
> > +int queue_suspend_blocking_work(struct workqueue_struct *wq,
> > +				struct suspend_blocking_work *work)
> > +{
> > +	int ret;
> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&work->lock, flags);
> > +	suspend_block(&work->suspend_blocker);
> > +	ret = queue_work(wq, &work->work);
> > +	if (ret)
> > +		work->active++;
> 
> why not
> 
> 	ret = queue_work(wq, &work->work);
> 	if (ret) {
> 		suspend_block(&work->suspend_blocker);
> 		work->active++;
> 	}
> 
> ?
> 
> Afaics, we can't race with work->func() doing unblock, we hold work-lock.
> And this way the code looks more clear.

Agreed.  Arve, any objections to doing that?

> Sorry, I had no chance to read the previous patches. After the quick look
> at 1/8 I think it is OK to call suspend_block() twice, but still...

It is.

> Or I missed something? Just curious.
> 
> 
> Hmm... actually, queue_work() can also fail if we race with cancel_ which
> temporary sets WORK_STRUCT_PENDING. In that case suspend_block() won't
> be paired by unblock ?
> 
> 
> > +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
> > +{
> > ...
> > +	ret = schedule_work(&work->work);
> 
> Off-topic. We should probably export keventd_wq to avoid the duplications
> like this.

Please see my reply to Tejun. :-)

Rafael

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Arve Hjønnevåg]

2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 28 April 2010, Oleg Nesterov wrote:
>> On 04/27, Arve Hjønnevåg wrote:
>> >
>> > Allow work to be queued that will block suspend while it is pending
>> > or executing. To get the same functionality in the calling code often
>> > requires a separate suspend_blocker for pending and executing work, or
>> > additional state and locking. This implementation does add additional
>> > state and locking, but this can be removed later if we add support for
>> > suspend blocking work to the core workqueue code.
>>
>> I think this patch is fine.
>>
>> Just one silly question,
>>
>> > +int queue_suspend_blocking_work(struct workqueue_struct *wq,
>> > +                           struct suspend_blocking_work *work)
>> > +{
>> > +   int ret;
>> > +   unsigned long flags;
>> > +
>> > +   spin_lock_irqsave(&work->lock, flags);
>> > +   suspend_block(&work->suspend_blocker);
>> > +   ret = queue_work(wq, &work->work);
>> > +   if (ret)
>> > +           work->active++;
>>
>> why not
>>
>>       ret = queue_work(wq, &work->work);
>>       if (ret) {
>>               suspend_block(&work->suspend_blocker);
>>               work->active++;
>>       }
>>
>> ?
>>
>> Afaics, we can't race with work->func() doing unblock, we hold work-lock.
>> And this way the code looks more clear.
>
> Agreed.  Arve, any objections to doing that?
>

I need to fix the race, but I can easily fix it in
cancel_suspend_blocking_work_sync instead. If the suspend blocker is
active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can
tell if the work is constantly re-queued or if the workqueue is stuck.

>> Sorry, I had no chance to read the previous patches. After the quick look
>> at 1/8 I think it is OK to call suspend_block() twice, but still...
>
> It is.
>
>> Or I missed something? Just curious.
>>
>>
>> Hmm... actually, queue_work() can also fail if we race with cancel_ which
>> temporary sets WORK_STRUCT_PENDING. In that case suspend_block() won't
>> be paired by unblock ?
>>
>>
>> > +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
>> > +{
>> > ...
>> > +   ret = schedule_work(&work->work);
>>
>> Off-topic. We should probably export keventd_wq to avoid the duplications
>> like this.
>
> Please see my reply to Tejun. :-)
>
> Rafael
>



-- 
Arve Hjønnevåg

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Rafael J. Wysocki]

On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 28 April 2010, Oleg Nesterov wrote:
> >> On 04/27, Arve Hjønnevåg wrote:
> >> >
> >> > Allow work to be queued that will block suspend while it is pending
> >> > or executing. To get the same functionality in the calling code often
> >> > requires a separate suspend_blocker for pending and executing work, or
> >> > additional state and locking. This implementation does add additional
> >> > state and locking, but this can be removed later if we add support for
> >> > suspend blocking work to the core workqueue code.
> >>
> >> I think this patch is fine.
> >>
> >> Just one silly question,
> >>
> >> > +int queue_suspend_blocking_work(struct workqueue_struct *wq,
> >> > +                           struct suspend_blocking_work *work)
> >> > +{
> >> > +   int ret;
> >> > +   unsigned long flags;
> >> > +
> >> > +   spin_lock_irqsave(&work->lock, flags);
> >> > +   suspend_block(&work->suspend_blocker);
> >> > +   ret = queue_work(wq, &work->work);
> >> > +   if (ret)
> >> > +           work->active++;
> >>
> >> why not
> >>
> >>       ret = queue_work(wq, &work->work);
> >>       if (ret) {
> >>               suspend_block(&work->suspend_blocker);
> >>               work->active++;
> >>       }
> >>
> >> ?
> >>
> >> Afaics, we can't race with work->func() doing unblock, we hold work-lock.
> >> And this way the code looks more clear.
> >
> > Agreed.  Arve, any objections to doing that?
> >
> 
> I need to fix the race, but I can easily fix it in
> cancel_suspend_blocking_work_sync instead. If the suspend blocker is
> active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can
> tell if the work is constantly re-queued or if the workqueue is stuck.

Well, perhaps that's worth adding a comment to the code.  The debug part is not
immediately visible from the code itself.

Rafael

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

[Arve Hjønnevåg]

2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
>> Add a misc device, "suspend_blocker", that allows user-space processes
>> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> and to block and unblock suspend. To delete the suspend_blocker, close
>> the device.
>>
>> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ...
>> +
>> +#include <linux/fs.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/slab.h>
>> +#include <linux/suspend_blocker.h>
>> +#include <linux/suspend_block_dev.h>
>> +
>> +enum {
>> +     DEBUG_FAILURE   = BIT(0),
>> +};
>> +static int debug_mask = DEBUG_FAILURE;
>
> What's the exact purpose of this?

To show errors returned to user space. I can turn it off by default if you want.

>
>> +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];
>> +};
>
> Why is this not in a header?

It's private to this file.

>
> Rafael
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Arve Hjønnevåg

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

[Rafael J. Wysocki]

On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
> >> Add a misc device, "suspend_blocker", that allows user-space processes
> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> and to block and unblock suspend. To delete the suspend_blocker, close
> >> the device.
> >>
> >> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> > ...
> >> +
> >> +#include <linux/fs.h>
> >> +#include <linux/miscdevice.h>
> >> +#include <linux/module.h>
> >> +#include <linux/uaccess.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/suspend_blocker.h>
> >> +#include <linux/suspend_block_dev.h>
> >> +
> >> +enum {
> >> +     DEBUG_FAILURE   = BIT(0),
> >> +};
> >> +static int debug_mask = DEBUG_FAILURE;
> >
> > What's the exact purpose of this?
> 
> To show errors returned to user space. I can turn it off by default if you want.

Not necessarily, but why is it a mask?  It looks like a 0/1 thing would be
sufficient.

BTW, I'd put parens around (debug_mask & DEBUG_FAILURE) for clarity.

Rafael

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

[Arve Hjønnevåg]

2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> On Thursday 29 April 2010, Arve Hjønnevåg wrote:
>> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
>> >> Add a misc device, "suspend_blocker", that allows user-space processes
>> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> >> and to block and unblock suspend. To delete the suspend_blocker, close
>> >> the device.
>> >>
>> >> Signed-off-by: Arve Hjønnevåg <arve@android.com>
>> > ...
>> >> +
>> >> +#include <linux/fs.h>
>> >> +#include <linux/miscdevice.h>
>> >> +#include <linux/module.h>
>> >> +#include <linux/uaccess.h>
>> >> +#include <linux/slab.h>
>> >> +#include <linux/suspend_blocker.h>
>> >> +#include <linux/suspend_block_dev.h>
>> >> +
>> >> +enum {
>> >> +     DEBUG_FAILURE   = BIT(0),
>> >> +};
>> >> +static int debug_mask = DEBUG_FAILURE;
>> >
>> > What's the exact purpose of this?
>>
>> To show errors returned to user space. I can turn it off by default if you want.
>
> Not necessarily, but why is it a mask?  It looks like a 0/1 thing would be
> sufficient.

I may want to add a bit to print all user-space block and unblock calls.

>
> BTW, I'd put parens around (debug_mask & DEBUG_FAILURE) for clarity.

OK.

-- 
Arve Hjønnevåg

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

[Arve Hjønnevåg]

2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
...
>>
>> +/**
>> + *   policy - set policy for state
>> + */
>> +
>> +static ssize_t policy_show(struct kobject *kobj,
>> +                        struct kobj_attribute *attr, char *buf)
>> +{
>> +     char *s = buf;
>> +     int i;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(policies); i++) {
>> +             if (i == policy)
>> +                     s += sprintf(s, "[%s] ", policies[i].name);
>> +             else
>> +                     s += sprintf(s, "%s ", policies[i].name);
>> +     }
>> +     if (s != buf)
>> +             /* convert the last space to a newline */
>> +             *(s-1) = '\n';
>> +     return (s - buf);
>> +}
>> +
>> +static ssize_t policy_store(struct kobject *kobj,
>> +                         struct kobj_attribute *attr,
>> +                         const char *buf, size_t n)
>> +{
>> +     const char *s;
>> +     char *p;
>> +     int len;
>> +     int i;
>> +
>> +     p = memchr(buf, '\n', n);
>> +     len = p ? p - buf : n;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(policies); i++) {
>> +             s = policies[i].name;
>> +             if (s && len == strlen(s) && !strncmp(buf, s, len)) {
>> +                     mutex_lock(&pm_mutex);
>> +                     policies[policy].set_state(PM_SUSPEND_ON);
>> +                     policy = i;
>> +                     mutex_unlock(&pm_mutex);
>> +                     return n;
>> +             }
>> +     }
>> +     return -EINVAL;
>> +}
>> +
>> +power_attr(policy);
>> +
>>  #ifdef CONFIG_PM_TRACE
>>  int pm_trace_enabled;
>>
>
> Would you mind if I changed the above so that "policy" doesn't even show up
> if CONFIG_OPPORTUNISTIC_SUSPEND is unset?
>
I don't mind, but It did not seem worth the trouble to hide it. It
will only list the supported policies, and it is easy to add or remove
policies this way.

> ...
>> +static void suspend_worker(struct work_struct *work)
>> +{
>> +     int ret;
>> +     int entry_event_num;
>> +
>> +     enable_suspend_blockers = true;
>> +     while (!suspend_is_blocked()) {
>> +             entry_event_num = current_event_num;
>> +
>> +             if (debug_mask & DEBUG_SUSPEND)
>> +                     pr_info("suspend: enter suspend\n");
>> +
>> +             ret = pm_suspend(requested_suspend_state);
>> +
>> +             if (debug_mask & DEBUG_EXIT_SUSPEND)
>> +                     pr_info_time("suspend: exit suspend, ret = %d ", ret);
>> +
>> +             if (current_event_num == entry_event_num)
>> +                     pr_info("suspend: pm_suspend returned with no event\n");
>
> Hmm, what exactly is this for?  It looks like a debug thing to me.  I'd use
> pr_debug() here and in both debug printk()s above.  Would you agree?
>

If the driver that caused the wakeup does not use suspend blockers, we
the only choice is to try to suspend again. I want to know if this
happened. The stats patch disable this printk by default since it will
show up in the stats, and the timeout patch (not included here) delays
the retry.

...
>> +EXPORT_SYMBOL(suspend_blocker_init);
>
> Is there a strong objection to changing that (and the other instances below) to
> EXPORT_SYMBOL_GPL?
>

I don't know if it is a strong objection, but I prefer that this api
is available to all drivers. I don't want to prevent a user from using
opportunistic suspend because a non-gpl driver could not use suspend
blockers. I changed the suspend blocking work functions to be gpl only
though, since they are not required, and the workqueue api is
available to gpl code anyway.

...
>> +bool request_suspend_valid_state(suspend_state_t state)
>> +{
>> +     return (state == PM_SUSPEND_ON) || valid_state(state);
>> +}
>> +
>> +int request_suspend_state(suspend_state_t state)
>> +{
>> +     unsigned long irqflags;
>> +
>> +     if (!request_suspend_valid_state(state))
>> +             return -ENODEV;
>> +
>> +     spin_lock_irqsave(&state_lock, irqflags);
>> +
>> +     if (debug_mask & DEBUG_USER_STATE)
>> +             pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
>> +                          state != PM_SUSPEND_ON ? "sleep" : "wakeup",
>> +                          requested_suspend_state, state,
>> +                          ktime_to_ns(ktime_get()));
>> +
>> +     requested_suspend_state = state;
>> +     if (state == PM_SUSPEND_ON)
>> +             suspend_block(&main_suspend_blocker);
>> +     else
>> +             suspend_unblock(&main_suspend_blocker);
>> +     spin_unlock_irqrestore(&state_lock, irqflags);
>> +     return 0;
>> +}
>
> I think the two functions above should be static, shouldn't they?

No, they are used from main.c.

>
>> +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);
>
> Hmm.  Why don't you want to put that initialization into pm_init() (in
> kernel/power/main.c)?

It was not needed before, but I changed pm_init to call
suspend_block_init after creating pm_wq.

>
> Also, we already have one PM workqueue.  It is used for runtime PM, but I guess
> it may be used just as well for the opportunistic suspend.  It is freezable,
> but would it hurt?

No, it works, the freezable flag is just ignored when I call
pm_suspend and I don't run anything else on the workqueue while
threads are frozen. It does need to be a single threaded workqueue
though, so make sure you don't just change that.

-- 
Arve Hjønnevåg

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Arve Hjønnevåg]

2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> On Thursday 29 April 2010, Arve Hjønnevåg wrote:
>> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Wednesday 28 April 2010, Oleg Nesterov wrote:
>> >> On 04/27, Arve Hjønnevåg wrote:
>> >> >
>> >> > Allow work to be queued that will block suspend while it is pending
>> >> > or executing. To get the same functionality in the calling code often
>> >> > requires a separate suspend_blocker for pending and executing work, or
>> >> > additional state and locking. This implementation does add additional
>> >> > state and locking, but this can be removed later if we add support for
>> >> > suspend blocking work to the core workqueue code.
>> >>
>> >> I think this patch is fine.
>> >>
>> >> Just one silly question,
>> >>
>> >> > +int queue_suspend_blocking_work(struct workqueue_struct *wq,
>> >> > +                           struct suspend_blocking_work *work)
>> >> > +{
>> >> > +   int ret;
>> >> > +   unsigned long flags;
>> >> > +
>> >> > +   spin_lock_irqsave(&work->lock, flags);
>> >> > +   suspend_block(&work->suspend_blocker);
>> >> > +   ret = queue_work(wq, &work->work);
>> >> > +   if (ret)
>> >> > +           work->active++;
>> >>
>> >> why not
>> >>
>> >>       ret = queue_work(wq, &work->work);
>> >>       if (ret) {
>> >>               suspend_block(&work->suspend_blocker);
>> >>               work->active++;
>> >>       }
>> >>
>> >> ?
>> >>
>> >> Afaics, we can't race with work->func() doing unblock, we hold work-lock.
>> >> And this way the code looks more clear.
>> >
>> > Agreed.  Arve, any objections to doing that?
>> >
>>
>> I need to fix the race, but I can easily fix it in
>> cancel_suspend_blocking_work_sync instead. If the suspend blocker is
>> active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can
>> tell if the work is constantly re-queued or if the workqueue is stuck.
>
> Well, perhaps that's worth adding a comment to the code.  The debug part is not
> immediately visible from the code itself.

On second thought, this only makes a difference if both conditions are
true. If we are constantly re-queuing the work but it is not stuck,
either method will show the debug message, so I used Oleg's
suggestion.

-- 
Arve Hjønnevåg

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Oleg Nesterov]

On 04/28, Rafael J. Wysocki wrote:
>
> On Wednesday 28 April 2010, Tejun Heo wrote:
> > On 04/28/2010 09:40 PM, Oleg Nesterov wrote:
> > >> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
> > >> +{
> > >> ...
> > >> +	ret = schedule_work(&work->work);
> > >
> > > Off-topic. We should probably export keventd_wq to avoid the duplications
> > > like this.
> >
> > Yeah, had about the same thought.  cmwq exports it so I didn't suggest
> > it at this point but then again we don't really know whether or when
> > that series is going in
>
> As soon as there are no major objections.  At least to my tree.
>
> > so it might be a good idea to make that change now.  Hmm...
>
> I'd rather like a follow-up patch changing that, if poss.

Confused. Rafael, do you mean you dislike this change now?

I don't really care, this change is trivial. But to me it makes more
sense to push the trivial/simple changes ahead. Unless they really
complicate the maintainging of the pending cmwq changes.

Hmm... Speaking about keventd_wq, I just noticed flush_delayed_work()
needs the fix:

	--- a/kernel/workqueue.c
	+++ b/kernel/workqueue.c
	@@ -774,7 +774,7 @@ void flush_delayed_work(struct delayed_w
	 {
		if (del_timer_sync(&dwork->timer)) {
			struct cpu_workqueue_struct *cwq;
	-		cwq = wq_per_cpu(keventd_wq, get_cpu());
	+		cwq = wq_per_cpu(get_wq_data(&dwork->work)->wq, get_cpu());
			__queue_work(cwq, &dwork->work);
			put_cpu();
		}

Oleg.

[PATCH 0/2] workqueue fixlets (Was: PM: Add suspend blocking work.)

[Oleg Nesterov]

On 04/29, Oleg Nesterov wrote:
>
> I don't really care, this change is trivial. But to me it makes more
> sense to push the trivial/simple changes ahead. Unless they really
> complicate the maintainging of the pending cmwq changes.
>
> Hmm... Speaking about keventd_wq, I just noticed flush_delayed_work()
> needs the fix:

So. I'd suggestd these 2 simple patches for now.

Please note that 2/2 is really trivial, it only adds EXPORT_SYMBOL but
avoids the possible cleanups to minimize the conflicts with the pending
cmwq changes. However, please feel free to ignore this patch.

As for 1/2, imho it is always better to fix the bug asap, even if it is
minor.

Oleg.

[PATCH 1/2] workqueues: flush_delayed_work: keep the original workqueue for re-queueing

[Oleg Nesterov]

flush_delayed_work() always uses keventd_wq for re-queueing,
but it should use the workqueue this dwork was queued on.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/workqueue.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 34-rc1/kernel/workqueue.c~FDW_DONT_USE_KEVENT_WQ	2009-12-18 19:05:38.000000000 +0100
+++ 34-rc1/kernel/workqueue.c	2010-04-29 21:08:32.000000000 +0200
@@ -774,7 +774,7 @@ void flush_delayed_work(struct delayed_w
 {
 	if (del_timer_sync(&dwork->timer)) {
 		struct cpu_workqueue_struct *cwq;
-		cwq = wq_per_cpu(keventd_wq, get_cpu());
+		cwq = wq_per_cpu(get_wq_data(&dwork->work)->wq, get_cpu());
 		__queue_work(cwq, &dwork->work);
 		put_cpu();
 	}

[PATCH 2/2] workqueues: export keventd_wq

[Oleg Nesterov]

Export keventd_wq. Otherwise, any helper on top of queue_work() has
to be copy-and-pasted to create the version which uses keventd_wq.

Note: we can do more cleanups with this change and kill EXPORT_SYMBOLs,
almost any function which currently uses keventd_wq can become the
trivial inline.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 include/linux/workqueue.h |    1 +
 kernel/workqueue.c        |    3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

--- 34-rc1/include/linux/workqueue.h~EXPORT_KEVENT_WQ	2009-12-18 19:05:38.000000000 +0100
+++ 34-rc1/include/linux/workqueue.h	2010-04-29 21:25:12.000000000 +0200
@@ -12,6 +12,7 @@
 #include <asm/atomic.h>
 
 struct workqueue_struct;
+extern struct workqueue_struct *keventd_wq;
 
 struct work_struct;
 typedef void (*work_func_t)(struct work_struct *work);
--- 34-rc1/kernel/workqueue.c~EXPORT_KEVENT_WQ	2010-04-29 21:08:32.000000000 +0200
+++ 34-rc1/kernel/workqueue.c	2010-04-29 21:23:46.000000000 +0200
@@ -717,7 +717,8 @@ int cancel_delayed_work_sync(struct dela
 }
 EXPORT_SYMBOL(cancel_delayed_work_sync);
 
-static struct workqueue_struct *keventd_wq __read_mostly;
+struct workqueue_struct *keventd_wq __read_mostly;
+EXPORT_SYMBOL(keventd_wq);
 
 /**
  * schedule_work - put work task in global workqueue

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Rafael J. Wysocki]

On Thursday 29 April 2010, Oleg Nesterov wrote:
> On 04/28, Rafael J. Wysocki wrote:
> >
> > On Wednesday 28 April 2010, Tejun Heo wrote:
> > > On 04/28/2010 09:40 PM, Oleg Nesterov wrote:
> > > >> +int schedule_suspend_blocking_work(struct suspend_blocking_work *work)
> > > >> +{
> > > >> ...
> > > >> +	ret = schedule_work(&work->work);
> > > >
> > > > Off-topic. We should probably export keventd_wq to avoid the duplications
> > > > like this.
> > >
> > > Yeah, had about the same thought.  cmwq exports it so I didn't suggest
> > > it at this point but then again we don't really know whether or when
> > > that series is going in
> >
> > As soon as there are no major objections.  At least to my tree.
> >
> > > so it might be a good idea to make that change now.  Hmm...
> >
> > I'd rather like a follow-up patch changing that, if poss.
> 
> Confused. Rafael, do you mean you dislike this change now?

No, I'm fine with the change itself, but I wouldn't like to make the suspend
blockers patchset depend on something in a different tree.  If it's not the
case, I have no objections.

Rafael

Re: [PATCH 6/8] PM: Add suspend blocking work.

[Rafael J. Wysocki]

On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> >> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Wednesday 28 April 2010, Oleg Nesterov wrote:
> >> >> On 04/27, Arve Hjønnevåg wrote:
> >> >> >
> >> >> > Allow work to be queued that will block suspend while it is pending
> >> >> > or executing. To get the same functionality in the calling code often
> >> >> > requires a separate suspend_blocker for pending and executing work, or
> >> >> > additional state and locking. This implementation does add additional
> >> >> > state and locking, but this can be removed later if we add support for
> >> >> > suspend blocking work to the core workqueue code.
> >> >>
> >> >> I think this patch is fine.
> >> >>
> >> >> Just one silly question,
> >> >>
> >> >> > +int queue_suspend_blocking_work(struct workqueue_struct *wq,
> >> >> > +                           struct suspend_blocking_work *work)
> >> >> > +{
> >> >> > +   int ret;
> >> >> > +   unsigned long flags;
> >> >> > +
> >> >> > +   spin_lock_irqsave(&work->lock, flags);
> >> >> > +   suspend_block(&work->suspend_blocker);
> >> >> > +   ret = queue_work(wq, &work->work);
> >> >> > +   if (ret)
> >> >> > +           work->active++;
> >> >>
> >> >> why not
> >> >>
> >> >>       ret = queue_work(wq, &work->work);
> >> >>       if (ret) {
> >> >>               suspend_block(&work->suspend_blocker);
> >> >>               work->active++;
> >> >>       }
> >> >>
> >> >> ?
> >> >>
> >> >> Afaics, we can't race with work->func() doing unblock, we hold work-lock.
> >> >> And this way the code looks more clear.
> >> >
> >> > Agreed.  Arve, any objections to doing that?
> >> >
> >>
> >> I need to fix the race, but I can easily fix it in
> >> cancel_suspend_blocking_work_sync instead. If the suspend blocker is
> >> active for a long time, and DEBUG_SUSPEND_BLOCKER is enabled, we can
> >> tell if the work is constantly re-queued or if the workqueue is stuck.
> >
> > Well, perhaps that's worth adding a comment to the code.  The debug part is not
> > immediately visible from the code itself.
> 
> On second thought, this only makes a difference if both conditions are
> true. If we are constantly re-queuing the work but it is not stuck,
> either method will show the debug message, so I used Oleg's
> suggestion.

OK, great.

Rafael

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

[Rafael J. Wysocki]

On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> >> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> >> > On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
> >> >> Add a misc device, "suspend_blocker", that allows user-space processes
> >> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
> >> >> and to block and unblock suspend. To delete the suspend_blocker, close
> >> >> the device.
> >> >>
> >> >> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> >> > ...
> >> >> +
> >> >> +#include <linux/fs.h>
> >> >> +#include <linux/miscdevice.h>
> >> >> +#include <linux/module.h>
> >> >> +#include <linux/uaccess.h>
> >> >> +#include <linux/slab.h>
> >> >> +#include <linux/suspend_blocker.h>
> >> >> +#include <linux/suspend_block_dev.h>
> >> >> +
> >> >> +enum {
> >> >> +     DEBUG_FAILURE   = BIT(0),
> >> >> +};
> >> >> +static int debug_mask = DEBUG_FAILURE;
> >> >
> >> > What's the exact purpose of this?
> >>
> >> To show errors returned to user space. I can turn it off by default if you want.
> >
> > Not necessarily, but why is it a mask?  It looks like a 0/1 thing would be
> > sufficient.
> 
> I may want to add a bit to print all user-space block and unblock calls.

Alternatively, you can add a new parameter for that, which I think I would prefer.

Rafael

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

[Rafael J. Wysocki]

On Thursday 29 April 2010, Arve Hjønnevåg wrote:
> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
> ...
> >>
> >> +/**
> >> + *   policy - set policy for state
> >> + */
> >> +
> >> +static ssize_t policy_show(struct kobject *kobj,
> >> +                        struct kobj_attribute *attr, char *buf)
> >> +{
> >> +     char *s = buf;
> >> +     int i;
> >> +
> >> +     for (i = 0; i < ARRAY_SIZE(policies); i++) {
> >> +             if (i == policy)
> >> +                     s += sprintf(s, "[%s] ", policies[i].name);
> >> +             else
> >> +                     s += sprintf(s, "%s ", policies[i].name);
> >> +     }
> >> +     if (s != buf)
> >> +             /* convert the last space to a newline */
> >> +             *(s-1) = '\n';
> >> +     return (s - buf);
> >> +}
> >> +
> >> +static ssize_t policy_store(struct kobject *kobj,
> >> +                         struct kobj_attribute *attr,
> >> +                         const char *buf, size_t n)
> >> +{
> >> +     const char *s;
> >> +     char *p;
> >> +     int len;
> >> +     int i;
> >> +
> >> +     p = memchr(buf, '\n', n);
> >> +     len = p ? p - buf : n;
> >> +
> >> +     for (i = 0; i < ARRAY_SIZE(policies); i++) {
> >> +             s = policies[i].name;
> >> +             if (s && len == strlen(s) && !strncmp(buf, s, len)) {
> >> +                     mutex_lock(&pm_mutex);
> >> +                     policies[policy].set_state(PM_SUSPEND_ON);
> >> +                     policy = i;
> >> +                     mutex_unlock(&pm_mutex);
> >> +                     return n;
> >> +             }
> >> +     }
> >> +     return -EINVAL;
> >> +}
> >> +
> >> +power_attr(policy);
> >> +
> >>  #ifdef CONFIG_PM_TRACE
> >>  int pm_trace_enabled;
> >>
> >
> > Would you mind if I changed the above so that "policy" doesn't even show up
> > if CONFIG_OPPORTUNISTIC_SUSPEND is unset?
> >
> I don't mind, but It did not seem worth the trouble to hide it. It
> will only list the supported policies, and it is easy to add or remove
> policies this way.
> 
> > ...
> >> +static void suspend_worker(struct work_struct *work)
> >> +{
> >> +     int ret;
> >> +     int entry_event_num;
> >> +
> >> +     enable_suspend_blockers = true;
> >> +     while (!suspend_is_blocked()) {
> >> +             entry_event_num = current_event_num;
> >> +
> >> +             if (debug_mask & DEBUG_SUSPEND)
> >> +                     pr_info("suspend: enter suspend\n");
> >> +
> >> +             ret = pm_suspend(requested_suspend_state);
> >> +
> >> +             if (debug_mask & DEBUG_EXIT_SUSPEND)
> >> +                     pr_info_time("suspend: exit suspend, ret = %d ", ret);
> >> +
> >> +             if (current_event_num == entry_event_num)
> >> +                     pr_info("suspend: pm_suspend returned with no event\n");
> >
> > Hmm, what exactly is this for?  It looks like a debug thing to me.  I'd use
> > pr_debug() here and in both debug printk()s above.  Would you agree?
> >
> 
> If the driver that caused the wakeup does not use suspend blockers, we
> the only choice is to try to suspend again. I want to know if this
> happened. The stats patch disable this printk by default since it will
> show up in the stats, and the timeout patch (not included here) delays
> the retry.
> 
> ...
> >> +EXPORT_SYMBOL(suspend_blocker_init);
> >
> > Is there a strong objection to changing that (and the other instances below) to
> > EXPORT_SYMBOL_GPL?
> >
> 
> I don't know if it is a strong objection, but I prefer that this api
> is available to all drivers. I don't want to prevent a user from using
> opportunistic suspend because a non-gpl driver could not use suspend
> blockers. I changed the suspend blocking work functions to be gpl only
> though, since they are not required, and the workqueue api is
> available to gpl code anyway.
> 
> ...
> >> +bool request_suspend_valid_state(suspend_state_t state)
> >> +{
> >> +     return (state == PM_SUSPEND_ON) || valid_state(state);
> >> +}
> >> +
> >> +int request_suspend_state(suspend_state_t state)
> >> +{
> >> +     unsigned long irqflags;
> >> +
> >> +     if (!request_suspend_valid_state(state))
> >> +             return -ENODEV;
> >> +
> >> +     spin_lock_irqsave(&state_lock, irqflags);
> >> +
> >> +     if (debug_mask & DEBUG_USER_STATE)
> >> +             pr_info_time("request_suspend_state: %s (%d->%d) at %lld ",
> >> +                          state != PM_SUSPEND_ON ? "sleep" : "wakeup",
> >> +                          requested_suspend_state, state,
> >> +                          ktime_to_ns(ktime_get()));
> >> +
> >> +     requested_suspend_state = state;
> >> +     if (state == PM_SUSPEND_ON)
> >> +             suspend_block(&main_suspend_blocker);
> >> +     else
> >> +             suspend_unblock(&main_suspend_blocker);
> >> +     spin_unlock_irqrestore(&state_lock, irqflags);
> >> +     return 0;
> >> +}
> >
> > I think the two functions above should be static, shouldn't they?
> 
> No, they are used from main.c.
> 
> >
> >> +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);
> >
> > Hmm.  Why don't you want to put that initialization into pm_init() (in
> > kernel/power/main.c)?
> 
> It was not needed before, but I changed pm_init to call
> suspend_block_init after creating pm_wq.
> 
> >
> > Also, we already have one PM workqueue.  It is used for runtime PM, but I guess
> > it may be used just as well for the opportunistic suspend.  It is freezable,
> > but would it hurt?
> 
> No, it works, the freezable flag is just ignored when I call
> pm_suspend and I don't run anything else on the workqueue while
> threads are frozen. It does need to be a single threaded workqueue
> though, so make sure you don't just change that.

Freezable workqueues have to be singlethread or else there will be unfixable
races, so you can safely assume things will stay as they are in this respect.

Rafael


> 
> 

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

[Arve Hjønnevåg]

2010/4/29 Rafael J. Wysocki <rjw@sisk.pl>:
> On Thursday 29 April 2010, Arve Hjønnevåg wrote:
>> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
>> > On Thursday 29 April 2010, Arve Hjønnevåg wrote:
>> >> 2010/4/28 Rafael J. Wysocki <rjw@sisk.pl>:
>> >> > On Wednesday 28 April 2010, Arve Hjønnevåg wrote:
>> >> >> Add a misc device, "suspend_blocker", that allows user-space processes
>> >> >> to block auto suspend. The device has ioctls to create a suspend_blocker,
>> >> >> and to block and unblock suspend. To delete the suspend_blocker, close
>> >> >> the device.
>> >> >>
>> >> >> Signed-off-by: Arve Hjønnevåg <arve@android.com>
>> >> > ...
>> >> >> +
>> >> >> +#include <linux/fs.h>
>> >> >> +#include <linux/miscdevice.h>
>> >> >> +#include <linux/module.h>
>> >> >> +#include <linux/uaccess.h>
>> >> >> +#include <linux/slab.h>
>> >> >> +#include <linux/suspend_blocker.h>
>> >> >> +#include <linux/suspend_block_dev.h>
>> >> >> +
>> >> >> +enum {
>> >> >> +     DEBUG_FAILURE   = BIT(0),
>> >> >> +};
>> >> >> +static int debug_mask = DEBUG_FAILURE;
>> >> >
>> >> > What's the exact purpose of this?
>> >>
>> >> To show errors returned to user space. I can turn it off by default if you want.
>> >
>> > Not necessarily, but why is it a mask?  It looks like a 0/1 thing would be
>> > sufficient.
>>
>> I may want to add a bit to print all user-space block and unblock calls.
>
> Alternatively, you can add a new parameter for that, which I think I would prefer.
>

I use a bit-mask in the main suspend blocker code. This makes it easy
to turn on or off all the messages.

-- 
Arve Hjønnevåg

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

[Tejun Heo]

Hello,

On 04/29/2010 11:16 PM, Rafael J. Wysocki wrote:
>>> Also, we already have one PM workqueue.  It is used for runtime PM, but I guess
>>> it may be used just as well for the opportunistic suspend.  It is freezable,
>>> but would it hurt?
>>
>> No, it works, the freezable flag is just ignored when I call
>> pm_suspend and I don't run anything else on the workqueue while
>> threads are frozen. It does need to be a single threaded workqueue
>> though, so make sure you don't just change that.
> 
> Freezable workqueues have to be singlethread or else there will be unfixable
> races, so you can safely assume things will stay as they are in this respect.

Rafael, can you elaborate a bit more on this?  Just in case I missed
something while doing cmwq as it currently doesn't have such limit.

Thanks.

-- 
tejun

Re: [PATCH 1/2] workqueues: flush_delayed_work: keep the original workqueue for re-queueing

[Tejun Heo]

Hello,

On 04/29/2010 09:45 PM, Oleg Nesterov wrote:
> flush_delayed_work() always uses keventd_wq for re-queueing,
> but it should use the workqueue this dwork was queued on.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

I'll queue it through the wq tree I have.

Thanks.

-- 
tejun

Re: [PATCH 2/2] workqueues: export keventd_wq

[Tejun Heo]

Hello,

On 04/29/2010 09:45 PM, Oleg Nesterov wrote:
> -static struct workqueue_struct *keventd_wq __read_mostly;
> +struct workqueue_struct *keventd_wq __read_mostly;
> +EXPORT_SYMBOL(keventd_wq);

Umm... does it have to be EXPORTed?  Suspend block API can't be built
as a module, right?

-- 
tejun

Re: [PATCH 2/2] workqueues: export keventd_wq

[Arve Hjønnevåg]

On Thu, Apr 29, 2010 at 10:16 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On 04/29/2010 09:45 PM, Oleg Nesterov wrote:
>> -static struct workqueue_struct *keventd_wq __read_mostly;
>> +struct workqueue_struct *keventd_wq __read_mostly;
>> +EXPORT_SYMBOL(keventd_wq);
>
> Umm... does it have to be EXPORTed?  Suspend block API can't be built
> as a module, right?

The suspend block api cannot be built as a module, but if
schedule_suspend_blocking_work will just call
queue_suspend_blocking_work(keventd_wq, work) it may as well be an
inline function which would require the export.

-- 
Arve Hjønnevåg

Re: [PATCH 2/2] workqueues: export keventd_wq

[Tejun Heo]

On 04/30/2010 07:39 AM, Arve Hjønnevåg wrote:
> On Thu, Apr 29, 2010 at 10:16 PM, Tejun Heo <tj@kernel.org> wrote:
>> Hello,
>>
>> On 04/29/2010 09:45 PM, Oleg Nesterov wrote:
>>> -static struct workqueue_struct *keventd_wq __read_mostly;
>>> +struct workqueue_struct *keventd_wq __read_mostly;
>>> +EXPORT_SYMBOL(keventd_wq);
>>
>> Umm... does it have to be EXPORTed?  Suspend block API can't be built
>> as a module, right?
> 
> The suspend block api cannot be built as a module, but if
> schedule_suspend_blocking_work will just call
> queue_suspend_blocking_work(keventd_wq, work) it may as well be an
> inline function which would require the export.

I think it would be better to keep the thing inside the kernel, at
least for now.  It's not like we need to save cpu cycles spent on a
function call here (in block suspend API and in workqueue general).

Thanks.

-- 
tejun

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

[Oleg Nesterov]

On 04/30, Tejun Heo wrote:
>
> On 04/29/2010 11:16 PM, Rafael J. Wysocki wrote:
> >>> Also, we already have one PM workqueue.  It is used for runtime PM, but I guess
> >>> it may be used just as well for the opportunistic suspend.  It is freezable,
> >>> but would it hurt?
> >>
> > Freezable workqueues have to be singlethread or else there will be unfixable
> > races, so you can safely assume things will stay as they are in this respect.
>
> Rafael, can you elaborate a bit more on this?  Just in case I missed
> something while doing cmwq as it currently doesn't have such limit.

Currently _cpu_down() can't flush and/or stop the frozen cwq->thread.

IIRC this is fixable, but needs the nasty complications. We should
thaw + stop the frozen cwq->thread, then move the pending works to
another CPU.

Oleg.

Re: [PATCH 2/2] workqueues: export keventd_wq

[Oleg Nesterov]

On 04/30, Tejun Heo wrote:
>
> On 04/30/2010 07:39 AM, Arve Hjønnevåg wrote:
> > On Thu, Apr 29, 2010 at 10:16 PM, Tejun Heo <tj@kernel.org> wrote:
> >> Hello,
> >>
> >> On 04/29/2010 09:45 PM, Oleg Nesterov wrote:
> >>> -static struct workqueue_struct *keventd_wq __read_mostly;
> >>> +struct workqueue_struct *keventd_wq __read_mostly;
> >>> +EXPORT_SYMBOL(keventd_wq);
> >>
> >> Umm... does it have to be EXPORTed?  Suspend block API can't be built
> >> as a module, right?

Right, but this allows to make schedule_xxx/flush_scheduled_work
inline and kill more EXPORT_SYMBOL's, and cmwq exports it anyway
(iirc it also renames it).

> > The suspend block api cannot be built as a module, but if
> > schedule_suspend_blocking_work will just call
> > queue_suspend_blocking_work(keventd_wq, work) it may as well be an
> > inline function which would require the export.
>
> I think it would be better to keep the thing inside the kernel, at
> least for now.

But then schedule_suspend_blocking_work() in turn needs EXPORT_SYMBOL().


OK. Let's forget this patch. We can unify schedule_suspend_blocking_work
and queue_suspend_blocking_work later, or Arve can add this export into
his patch (without EXPORT_SYMBOL) - either way I agree. This is very
minor issue, I regret I originated this almost offtopic noise ;)

Oleg.

Re: [PATCH 2/2] workqueues: export keventd_wq

[Tejun Heo]

Hello,

On 04/30/2010 08:05 PM, Oleg Nesterov wrote:
> Right, but this allows to make schedule_xxx/flush_scheduled_work
> inline and kill more EXPORT_SYMBOL's, and cmwq exports it anyway
> (iirc it also renames it).

Yeah, the 'iirc' part was why I didn't want to export it at this point. :-)

> OK. Let's forget this patch. We can unify schedule_suspend_blocking_work
> and queue_suspend_blocking_work later, or Arve can add this export into
> his patch (without EXPORT_SYMBOL) - either way I agree. This is very
> minor issue, I regret I originated this almost offtopic noise ;)

And, again, yeah, let's do it later.

Thanks.

-- 
tejun

[PATCH 2/8] 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/ioctl/ioctl-number.txt          |    3 +-
 Documentation/power/opportunistic-suspend.txt |   17 ++++
 include/linux/suspend_block_dev.h             |   25 +++++
 kernel/power/Kconfig                          |    9 ++
 kernel/power/Makefile                         |    1 +
 kernel/power/user_suspend_blocker.c           |  128 +++++++++++++++++++++++++
 6 files changed, 182 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/suspend_block_dev.h
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index dd5806f..e2458f7 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
 'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
 		linux/ixjuser.h		<http://www.quicknet.net>
 'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
-'s'	all	linux/cdk.h
+'s'	all	linux/cdk.h	conflict!
+'s'	all	linux/suspend_block_dev.h	conflict!
 't'	00-7F	linux/if_ppp.h
 't'	80-8F	linux/isdn_ppp.h
 't'	90	linux/toshiba.h
diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
index 3d060e8..f2b145e 100644
--- a/Documentation/power/opportunistic-suspend.txt
+++ b/Documentation/power/opportunistic-suspend.txt
@@ -117,3 +117,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..24bc5c7
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
+#define _LINUX_SUSPEND_BLOCK_DEV_H
+
+#include <linux/ioctl.h>
+
+#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
+
+#endif
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 55a06a1..fe5a2f2 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -146,6 +146,15 @@ config OPPORTUNISTIC_SUSPEND
 	  determines the sleep state the system will be put into when there are
 	  no active suspend blockers.
 
+config USER_SUSPEND_BLOCKERS
+	bool "Userspace suspend blockers"
+	depends on OPPORTUNISTIC_SUSPEND
+	default y
+	---help---
+	  User-space suspend block api. Creates a misc device with ioctls
+	  to create, block and unblock a suspend_blocker. The suspend_blocker
+	  will be deleted when the device is closed.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index ee5276d..78f703b 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..dc1d06f
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,128 @@
+/* kernel/power/user_suspend_block.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/suspend_blocker.h>
+#include <linux/suspend_block_dev.h>
+
+enum {
+	DEBUG_FAILURE	= BIT(0),
+};
+static int debug_mask = DEBUG_FAILURE;
+module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
+
+static DEFINE_MUTEX(ioctl_lock);
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[0];
+};
+
+static int create_user_suspend_blocker(struct file *file, void __user *name,
+				 size_t name_len)
+{
+	struct user_suspend_blocker *bl;
+	if (file->private_data)
+		return -EBUSY;
+	if (name_len > NAME_MAX)
+		return -ENAMETOOLONG;
+	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
+	if (!bl)
+		return -ENOMEM;
+	if (copy_from_user(bl->name, name, name_len))
+		goto err_fault;
+	suspend_blocker_init(&bl->blocker, bl->name);
+	file->private_data = bl;
+	return 0;
+
+err_fault:
+	kfree(bl);
+	return -EFAULT;
+}
+
+static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
+				unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *bl;
+	long ret;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
+		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	bl = file->private_data;
+	if (!bl) {
+		ret = -ENOENT;
+		goto done;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&bl->blocker);
+		ret = 0;
+		break;
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&bl->blocker);
+		ret = 0;
+		break;
+	default:
+		ret = -ENOTSUPP;
+	}
+done:
+	if (ret && (debug_mask & DEBUG_FAILURE))
+		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
+			cmd, ret);
+	mutex_unlock(&ioctl_lock);
+	return ret;
+}
+
+static int user_suspend_blocker_release(struct inode *inode, struct file *file)
+{
+	struct user_suspend_blocker *bl = file->private_data;
+	if (!bl)
+		return 0;
+	suspend_blocker_destroy(&bl->blocker);
+	kfree(bl);
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.5.1

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

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

[Pavel Machek]

On Fri 2010-04-30 15:36:55, 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.

Yeah, this one is overly complex; instead of having 'open blocks
suspend' semantics and lsof listing active blockers, it adds strange
ioctl based interface passing names, and then adds debugfs
infrastructure listing those back.

I guess this is why you are getying 'it should be in /proc, no in
/sys, no in debugfs, no in /proc' kind of feedback. This should simply
not exist in the first place...

> Signed-off-by: Arve Hj??nnev??g <arve@android.com>

NAK.

> ---
>  Documentation/ioctl/ioctl-number.txt          |    3 +-
>  Documentation/power/opportunistic-suspend.txt |   17 ++++
>  include/linux/suspend_block_dev.h             |   25 +++++
>  kernel/power/Kconfig                          |    9 ++
>  kernel/power/Makefile                         |    1 +
>  kernel/power/user_suspend_blocker.c           |  128 +++++++++++++++++++++++++
>  6 files changed, 182 insertions(+), 1 deletions(-)
>  create mode 100644 include/linux/suspend_block_dev.h
>  create mode 100644 kernel/power/user_suspend_blocker.c
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index dd5806f..e2458f7 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
>  'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
>  		linux/ixjuser.h		<http://www.quicknet.net>
>  'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
> -'s'	all	linux/cdk.h
> +'s'	all	linux/cdk.h	conflict!
> +'s'	all	linux/suspend_block_dev.h	conflict!
>  't'	00-7F	linux/if_ppp.h
>  't'	80-8F	linux/isdn_ppp.h
>  't'	90	linux/toshiba.h
> diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
> index 3d060e8..f2b145e 100644
> --- a/Documentation/power/opportunistic-suspend.txt
> +++ b/Documentation/power/opportunistic-suspend.txt
> @@ -117,3 +117,20 @@ if (list_empty(&state->pending_work))
>  else
>  	suspend_block(&state->suspend_blocker);
>  
> +User-space API
> +==============
> +
> +To create a suspend_blocker from user-space, open the suspend_blocker device:
> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> +then call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> +
> +To activate a suspend_blocker call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> +
> +To unblock call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> +
> +To destroy the suspend_blocker, close the device:
> +    close(fd);
> +
> diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
> new file mode 100644
> index 0000000..24bc5c7
> --- /dev/null
> +++ b/include/linux/suspend_block_dev.h
> @@ -0,0 +1,25 @@
> +/* include/linux/suspend_block_dev.h
> + *
> + * Copyright (C) 2009 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
> +#define _LINUX_SUSPEND_BLOCK_DEV_H
> +
> +#include <linux/ioctl.h>
> +
> +#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
> +#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
> +#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
> +
> +#endif
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 55a06a1..fe5a2f2 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -146,6 +146,15 @@ config OPPORTUNISTIC_SUSPEND
>  	  determines the sleep state the system will be put into when there are
>  	  no active suspend blockers.
>  
> +config USER_SUSPEND_BLOCKERS
> +	bool "Userspace suspend blockers"
> +	depends on OPPORTUNISTIC_SUSPEND
> +	default y
> +	---help---
> +	  User-space suspend block api. Creates a misc device with ioctls
> +	  to create, block and unblock a suspend_blocker. The suspend_blocker
> +	  will be deleted when the device is closed.
> +
>  config HIBERNATION_NVS
>  	bool
>  
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index ee5276d..78f703b 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
>  obj-$(CONFIG_FREEZER)		+= process.o
>  obj-$(CONFIG_SUSPEND)		+= suspend.o
>  obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
> +obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
>  obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
> new file mode 100644
> index 0000000..dc1d06f
> --- /dev/null
> +++ b/kernel/power/user_suspend_blocker.c
> @@ -0,0 +1,128 @@
> +/* kernel/power/user_suspend_block.c
> + *
> + * Copyright (C) 2009-2010 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/suspend_blocker.h>
> +#include <linux/suspend_block_dev.h>
> +
> +enum {
> +	DEBUG_FAILURE	= BIT(0),
> +};
> +static int debug_mask = DEBUG_FAILURE;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +static DEFINE_MUTEX(ioctl_lock);
> +
> +struct user_suspend_blocker {
> +	struct suspend_blocker	blocker;
> +	char			name[0];
> +};
> +
> +static int create_user_suspend_blocker(struct file *file, void __user *name,
> +				 size_t name_len)
> +{
> +	struct user_suspend_blocker *bl;
> +	if (file->private_data)
> +		return -EBUSY;
> +	if (name_len > NAME_MAX)
> +		return -ENAMETOOLONG;
> +	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
> +	if (!bl)
> +		return -ENOMEM;
> +	if (copy_from_user(bl->name, name, name_len))
> +		goto err_fault;
> +	suspend_blocker_init(&bl->blocker, bl->name);
> +	file->private_data = bl;
> +	return 0;
> +
> +err_fault:
> +	kfree(bl);
> +	return -EFAULT;
> +}
> +
> +static long user_suspend_blocker_ioctl(struct file *file, unsigned int cmd,
> +				unsigned long _arg)
> +{
> +	void __user *arg = (void __user *)_arg;
> +	struct user_suspend_blocker *bl;
> +	long ret;
> +
> +	mutex_lock(&ioctl_lock);
> +	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_INIT(0)) {
> +		ret = create_user_suspend_blocker(file, arg, _IOC_SIZE(cmd));
> +		goto done;
> +	}
> +	bl = file->private_data;
> +	if (!bl) {
> +		ret = -ENOENT;
> +		goto done;
> +	}
> +	switch (cmd) {
> +	case SUSPEND_BLOCKER_IOCTL_BLOCK:
> +		suspend_block(&bl->blocker);
> +		ret = 0;
> +		break;
> +	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
> +		suspend_unblock(&bl->blocker);
> +		ret = 0;
> +		break;
> +	default:
> +		ret = -ENOTSUPP;
> +	}
> +done:
> +	if (ret && (debug_mask & DEBUG_FAILURE))
> +		pr_err("user_suspend_blocker_ioctl: cmd %x failed, %ld\n",
> +			cmd, ret);
> +	mutex_unlock(&ioctl_lock);
> +	return ret;
> +}
> +
> +static int user_suspend_blocker_release(struct inode *inode, struct file *file)
> +{
> +	struct user_suspend_blocker *bl = file->private_data;
> +	if (!bl)
> +		return 0;
> +	suspend_blocker_destroy(&bl->blocker);
> +	kfree(bl);
> +	return 0;
> +}
> +
> +const struct file_operations user_suspend_blocker_fops = {
> +	.release = user_suspend_blocker_release,
> +	.unlocked_ioctl = user_suspend_blocker_ioctl,
> +};
> +
> +struct miscdevice user_suspend_blocker_device = {
> +	.minor = MISC_DYNAMIC_MINOR,
> +	.name = "suspend_blocker",
> +	.fops = &user_suspend_blocker_fops,
> +};
> +
> +static int __init user_suspend_blocker_init(void)
> +{
> +	return misc_register(&user_suspend_blocker_device);
> +}
> +
> +static void __exit user_suspend_blocker_exit(void)
> +{
> +	misc_deregister(&user_suspend_blocker_device);
> +}
> +
> +module_init(user_suspend_blocker_init);
> +module_exit(user_suspend_blocker_exit);

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

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

[Rafael J. Wysocki]

On Saturday 01 May 2010, Arve Hjønnevåg wrote:
> Add a misc device, "suspend_blocker", that allows user-space processes
> to block auto suspend. The device has ioctls to create a suspend_blocker,
> and to block and unblock suspend. To delete the suspend_blocker, close
> the device.
> 
> Signed-off-by: Arve Hjønnevåg <arve@android.com>
> ---
>  Documentation/ioctl/ioctl-number.txt          |    3 +-
>  Documentation/power/opportunistic-suspend.txt |   17 ++++
>  include/linux/suspend_block_dev.h             |   25 +++++
>  kernel/power/Kconfig                          |    9 ++
>  kernel/power/Makefile                         |    1 +
>  kernel/power/user_suspend_blocker.c           |  128 +++++++++++++++++++++++++
>  6 files changed, 182 insertions(+), 1 deletions(-)
>  create mode 100644 include/linux/suspend_block_dev.h
>  create mode 100644 kernel/power/user_suspend_blocker.c
> 
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index dd5806f..e2458f7 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
>  'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
>  		linux/ixjuser.h		<http://www.quicknet.net>
>  'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
> -'s'	all	linux/cdk.h
> +'s'	all	linux/cdk.h	conflict!
> +'s'	all	linux/suspend_block_dev.h	conflict!
>  't'	00-7F	linux/if_ppp.h
>  't'	80-8F	linux/isdn_ppp.h
>  't'	90	linux/toshiba.h
> diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
> index 3d060e8..f2b145e 100644
> --- a/Documentation/power/opportunistic-suspend.txt
> +++ b/Documentation/power/opportunistic-suspend.txt
> @@ -117,3 +117,20 @@ if (list_empty(&state->pending_work))
>  else
>  	suspend_block(&state->suspend_blocker);
>  
> +User-space API
> +==============
> +
> +To create a suspend_blocker from user-space, open the suspend_blocker device:
> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> +then call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
> +
> +To activate a suspend_blocker call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> +
> +To unblock call:
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> +
> +To destroy the suspend_blocker, close the device:
> +    close(fd);
> +
> diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
> new file mode 100644
> index 0000000..24bc5c7
> --- /dev/null
> +++ b/include/linux/suspend_block_dev.h
> @@ -0,0 +1,25 @@
> +/* include/linux/suspend_block_dev.h
> + *
> + * Copyright (C) 2009 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef _LINUX_SUSPEND_BLOCK_DEV_H
> +#define _LINUX_SUSPEND_BLOCK_DEV_H
> +
> +#include <linux/ioctl.h>
> +
> +#define SUSPEND_BLOCKER_IOCTL_INIT(len)		_IOC(_IOC_WRITE, 's', 0, len)
> +#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
> +#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
> +
> +#endif
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 55a06a1..fe5a2f2 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -146,6 +146,15 @@ config OPPORTUNISTIC_SUSPEND
>  	  determines the sleep state the system will be put into when there are
>  	  no active suspend blockers.
>  
> +config USER_SUSPEND_BLOCKERS
> +	bool "Userspace suspend blockers"
> +	depends on OPPORTUNISTIC_SUSPEND
> +	default y
> +	---help---
> +	  User-space suspend block api. Creates a misc device with ioctls
> +	  to create, block and unblock a suspend_blocker. The suspend_blocker
> +	  will be deleted when the device is closed.
> +
>  config HIBERNATION_NVS
>  	bool
>  
> diff --git a/kernel/power/Makefile b/kernel/power/Makefile
> index ee5276d..78f703b 100644
> --- a/kernel/power/Makefile
> +++ b/kernel/power/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
>  obj-$(CONFIG_FREEZER)		+= process.o
>  obj-$(CONFIG_SUSPEND)		+= suspend.o
>  obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= suspend_blocker.o
> +obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
>  obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
>  obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
>  obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
> diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
> new file mode 100644
> index 0000000..dc1d06f
> --- /dev/null
> +++ b/kernel/power/user_suspend_blocker.c
> @@ -0,0 +1,128 @@
> +/* kernel/power/user_suspend_block.c
> + *
> + * Copyright (C) 2009-2010 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/miscdevice.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/suspend_blocker.h>
> +#include <linux/suspend_block_dev.h>
> +
> +enum {
> +	DEBUG_FAILURE	= BIT(0),
> +};
> +static int debug_mask = DEBUG_FAILURE;
> +module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
> +
> +static DEFINE_MUTEX(ioctl_lock);
> +
> +struct user_suspend_blocker {
> +	struct suspend_blocker	blocker;
> +	char			name[0];
> +};
> +
> +static int create_user_suspend_blocker(struct file *file, void __user *name,
> +				 size_t name_len)
> +{
> +	struct user_suspend_blocker *bl;
> +	if (file->private_data)
> +		return -EBUSY;
> +	if (name_len > NAME_MAX)
> +		return -ENAMETOOLONG;
> +	bl = kzalloc(sizeof(*bl) + name_len + 1, GFP_KERNEL);
> +	if (!bl)
> +		return -ENOMEM;
> +	if (copy_from_user(bl->name, name, name_len))
> +		goto err_fault;
> +	suspend_blocker_init(&bl->blocker, bl->name);
> +	file->private_data = bl;

Hmm.  It doesn't seem to be possible to create two different suspend blockers
using the same file handle.  So, what exactly is a process supposed to do to
use two suspend blockers at the same time?

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

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

[Alan Stern]

On Sun, 2 May 2010, Rafael J. Wysocki wrote:

> Hmm.  It doesn't seem to be possible to create two different suspend blockers
> using the same file handle.  So, what exactly is a process supposed to do to
> use two suspend blockers at the same time?

Open the file twice, thereby obtaining two file handles.

Alan Stern

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

[Rafael J. Wysocki]

On Sunday 02 May 2010, Alan Stern wrote:
> On Sun, 2 May 2010, Rafael J. Wysocki wrote:
> 
> > Hmm.  It doesn't seem to be possible to create two different suspend blockers
> > using the same file handle.  So, what exactly is a process supposed to do to
> > use two suspend blockers at the same time?
> 
> Open the file twice, thereby obtaining two file handles.

Well, that's what I thought.

I must admit I'm not really comfortable with this interface.  IMO it would
be better if _open() created the suspend blocker giving it a name based on
the name of the process that called it.  Something like
"<process name>_<timestamp>" might work at first sight.

Alternatively, "<process_name><number>", where <number> is 0 for the first
suspend blocker created by the given process, 1 for the second one etc., also
seems to be doable.

Rafael

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

[Arve Hjønnevåg]

On Mon, May 3, 2010 at 8:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Sunday 02 May 2010, Alan Stern wrote:
>> On Sun, 2 May 2010, Rafael J. Wysocki wrote:
>>
>> > Hmm.  It doesn't seem to be possible to create two different suspend blockers
>> > using the same file handle.  So, what exactly is a process supposed to do to
>> > use two suspend blockers at the same time?
>>
>> Open the file twice, thereby obtaining two file handles.
>
> Well, that's what I thought.
>
> I must admit I'm not really comfortable with this interface.  IMO it would
> be better if _open() created the suspend blocker giving it a name based on
> the name of the process that called it.  Something like
> "<process name>_<timestamp>" might work at first sight.
>
> Alternatively, "<process_name><number>", where <number> is 0 for the first
> suspend blocker created by the given process, 1 for the second one etc., also
> seems to be doable.

I think it is important to let user-space specify the name. If a
process uses multiple suspend blockers, it is impossible to tell what
each one is used for if they are automatically named.

-- 
Arve Hjønnevåg

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

[Rafael J. Wysocki]

On Monday 03 May 2010, Arve Hjønnevåg wrote:
> On Mon, May 3, 2010 at 8:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Sunday 02 May 2010, Alan Stern wrote:
> >> On Sun, 2 May 2010, Rafael J. Wysocki wrote:
> >>
> >> > Hmm.  It doesn't seem to be possible to create two different suspend blockers
> >> > using the same file handle.  So, what exactly is a process supposed to do to
> >> > use two suspend blockers at the same time?
> >>
> >> Open the file twice, thereby obtaining two file handles.
> >
> > Well, that's what I thought.
> >
> > I must admit I'm not really comfortable with this interface.  IMO it would
> > be better if _open() created the suspend blocker giving it a name based on
> > the name of the process that called it.  Something like
> > "<process name>_<timestamp>" might work at first sight.
> >
> > Alternatively, "<process_name><number>", where <number> is 0 for the first
> > suspend blocker created by the given process, 1 for the second one etc., also
> > seems to be doable.
> 
> I think it is important to let user-space specify the name. If a
> process uses multiple suspend blockers, it is impossible to tell what
> each one is used for if they are automatically named.

Well, the problem is the only purpose of this is user space debugging, isn't it?

Now, while I don't think it's generally bad to provide kernel interfaces
helping to debug user space, I'm not quite sure if that should be done at the
expense of the clarity of kernel-user space interfaces.

I wonder how many cases there are in which distinguishing between suspend
blockers used by the same user space task is practically relevant.

Thanks,
Rafael

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

[Arve Hjønnevåg]

2010/5/3 Rafael J. Wysocki <rjw@sisk.pl>:
> On Monday 03 May 2010, Arve Hjønnevåg wrote:
>> On Mon, May 3, 2010 at 8:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> > On Sunday 02 May 2010, Alan Stern wrote:
>> >> On Sun, 2 May 2010, Rafael J. Wysocki wrote:
>> >>
>> >> > Hmm.  It doesn't seem to be possible to create two different suspend blockers
>> >> > using the same file handle.  So, what exactly is a process supposed to do to
>> >> > use two suspend blockers at the same time?
>> >>
>> >> Open the file twice, thereby obtaining two file handles.
>> >
>> > Well, that's what I thought.
>> >
>> > I must admit I'm not really comfortable with this interface.  IMO it would
>> > be better if _open() created the suspend blocker giving it a name based on
>> > the name of the process that called it.  Something like
>> > "<process name>_<timestamp>" might work at first sight.
>> >
>> > Alternatively, "<process_name><number>", where <number> is 0 for the first
>> > suspend blocker created by the given process, 1 for the second one etc., also
>> > seems to be doable.
>>
>> I think it is important to let user-space specify the name. If a
>> process uses multiple suspend blockers, it is impossible to tell what
>> each one is used for if they are automatically named.
>
> Well, the problem is the only purpose of this is user space debugging, isn't it?
>
At the moment, yes. But having an explicit init call also simplifies
future expansion. If we ever need to add back multiple types of
suspend blockers, we can add an init call with a type argument and
have the current init call use the default type. If the past we have
had types that keeps the screen on and prevented idle sleep modes. In
the future we could for instance want to specify if a suspend blocker
should prevent suspend when the battery is low or when the lid is
closed.

> Now, while I don't think it's generally bad to provide kernel interfaces
> helping to debug user space, I'm not quite sure if that should be done at the
> expense of the clarity of kernel-user space interfaces.
>
> I wonder how many cases there are in which distinguishing between suspend
> blockers used by the same user space task is practically relevant.
>

I think all of them (when more than one suspend blocker is used).

-- 
Arve Hjønnevåg

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

[Pavel Machek]

On Mon 2010-05-03 17:03:11, Rafael J. Wysocki wrote:
> On Sunday 02 May 2010, Alan Stern wrote:
> > On Sun, 2 May 2010, Rafael J. Wysocki wrote:
> > 
> > > Hmm.  It doesn't seem to be possible to create two different suspend blockers
> > > using the same file handle.  So, what exactly is a process supposed to do to
> > > use two suspend blockers at the same time?
> > 
> > Open the file twice, thereby obtaining two file handles.
> 
> Well, that's what I thought.
> 
> I must admit I'm not really comfortable with this interface.  IMO it would
> be better if _open() created the suspend blocker giving it a name based on
> the name of the process that called it.  Something like
> "<process name>_<timestamp>" might work at first sight.
> 
> Alternatively, "<process_name><number>", where <number> is 0 for the first
> suspend blocker created by the given process, 1 for the second one etc., also
> seems to be doable.

Yes please.

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

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

[Rafael J. Wysocki]

On Tuesday 04 May 2010, Arve Hjønnevåg wrote:
> 2010/5/3 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Monday 03 May 2010, Arve Hjønnevåg wrote:
> >> On Mon, May 3, 2010 at 8:03 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> > On Sunday 02 May 2010, Alan Stern wrote:
> >> >> On Sun, 2 May 2010, Rafael J. Wysocki wrote:
> >> >>
> >> >> > Hmm.  It doesn't seem to be possible to create two different suspend blockers
> >> >> > using the same file handle.  So, what exactly is a process supposed to do to
> >> >> > use two suspend blockers at the same time?
> >> >>
> >> >> Open the file twice, thereby obtaining two file handles.
> >> >
> >> > Well, that's what I thought.
> >> >
> >> > I must admit I'm not really comfortable with this interface.  IMO it would
> >> > be better if _open() created the suspend blocker giving it a name based on
> >> > the name of the process that called it.  Something like
> >> > "<process name>_<timestamp>" might work at first sight.
> >> >
> >> > Alternatively, "<process_name><number>", where <number> is 0 for the first
> >> > suspend blocker created by the given process, 1 for the second one etc., also
> >> > seems to be doable.
> >>
> >> I think it is important to let user-space specify the name. If a
> >> process uses multiple suspend blockers, it is impossible to tell what
> >> each one is used for if they are automatically named.
> >
> > Well, the problem is the only purpose of this is user space debugging, isn't it?
> >
> At the moment, yes.

OK

> But having an explicit init call also simplifies future expansion.

That's irrelevant.  Let's focus _only_ on the "at the moment" part, please.

And BTW, a "future expansion" is not relevant at all to the patchset at hand
and let's work with this assumption from now on.  OK?

> If we ever need to add back multiple types of
> suspend blockers, we can add an init call with a type argument and
> have the current init call use the default type. If the past we have
> had types that keeps the screen on and prevented idle sleep modes. In
> the future we could for instance want to specify if a suspend blocker
> should prevent suspend when the battery is low or when the lid is
> closed.

_If_ we ever do anything like thie, _then_ we will extend the interface.

> > Now, while I don't think it's generally bad to provide kernel interfaces
> > helping to debug user space, I'm not quite sure if that should be done at the
> > expense of the clarity of kernel-user space interfaces.
> >
> > I wonder how many cases there are in which distinguishing between suspend
> > blockers used by the same user space task is practically relevant.
> >
> 
> I think all of them (when more than one suspend blocker is used).

So how many user space processes use more than one suspend blocker right
now on Android?  I'm interested only in the order of magnitude.

Rafael

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

[Alan Stern]

On Tue, 27 Apr 2010, [UTF-8] Arve Hjønnevåg wrote:

> +static void suspend_worker(struct work_struct *work)
> +{
> +	int ret;
> +	int entry_event_num;
> +
> +	enable_suspend_blockers = true;
> +	while (!suspend_is_blocked()) {
> +		entry_event_num = current_event_num;
> +
> +		if (debug_mask & DEBUG_SUSPEND)
> +			pr_info("suspend: enter suspend\n");
> +
> +		ret = pm_suspend(requested_suspend_state);
> +
> +		if (debug_mask & DEBUG_EXIT_SUSPEND)
> +			pr_info_time("suspend: exit suspend, ret = %d ", ret);
> +
> +		if (current_event_num == entry_event_num)
> +			pr_info("suspend: pm_suspend returned with no event\n");
> +	}
> +	enable_suspend_blockers = false;
> +}

Here's a completely new issue.  When using opportunistic suspends on an
SMP system, it could happen that the system gets a wakeup event and
this routine starts running again before the event's IRQ handler has
finished (or has enabled a suspend blocker).  The system would 
re-suspend too soon.

Is there anything in the PM core that waits until all outstanding IRQs 
have been handled before unfreezing processes?  If there is, I can't 
find it.  Should such a thing be added?

Alan Stern

[PATCH 2/8] 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/ioctl/ioctl-number.txt          |    3 +-
 Documentation/power/opportunistic-suspend.txt |   27 +++++
 include/linux/suspend_ioctls.h                |    4 +
 kernel/power/Kconfig                          |    7 ++
 kernel/power/Makefile                         |    1 +
 kernel/power/user_suspend_blocker.c           |  143 +++++++++++++++++++++++++
 6 files changed, 184 insertions(+), 1 deletions(-)
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index dd5806f..e2458f7 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
 'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
 		linux/ixjuser.h		<http://www.quicknet.net>
 'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
-'s'	all	linux/cdk.h
+'s'	all	linux/cdk.h	conflict!
+'s'	all	linux/suspend_block_dev.h	conflict!
 't'	00-7F	linux/if_ppp.h
 't'	80-8F	linux/isdn_ppp.h
 't'	90	linux/toshiba.h
diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
index 4bee7bc..93f4c24 100644
--- a/Documentation/power/opportunistic-suspend.txt
+++ b/Documentation/power/opportunistic-suspend.txt
@@ -127,3 +127,30 @@ if (list_empty(&state->pending_work))
 	suspend_unblock(&state->suspend_blocker);
 else
 	suspend_block(&state->suspend_blocker);
+
+User space API
+==============
+
+To create a suspend blocker from user space, open the suspend_blocker special
+device file:
+
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+
+then optionally call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
+
+To activate the suspend blocker call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To deactivate it call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend blocker, close the device:
+
+    close(fd);
+
+If the first ioctl called is not SUSPEND_BLOCKER_IOCTL_SET_NAME the suspend
+blocker will get the default name "(userspace)".
diff --git a/include/linux/suspend_ioctls.h b/include/linux/suspend_ioctls.h
index 0b30382..b95a6b2 100644
--- a/include/linux/suspend_ioctls.h
+++ b/include/linux/suspend_ioctls.h
@@ -30,4 +30,8 @@ struct resume_swap_area {
 #define SNAPSHOT_ALLOC_SWAP_PAGE	_IOR(SNAPSHOT_IOC_MAGIC, 20, __kernel_loff_t)
 #define SNAPSHOT_IOC_MAXNR	20
 
+#define SUSPEND_BLOCKER_IOCTL_SET_NAME(len)	_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
+
 #endif /* _LINUX_SUSPEND_IOCTLS_H */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 6d11a45..2e665cd 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -146,6 +146,13 @@ config OPPORTUNISTIC_SUSPEND
 	  determines the sleep state the system will be put into when there are
 	  no active suspend blockers.
 
+config USER_SUSPEND_BLOCKERS
+	bool "User space suspend blockers"
+	depends on OPPORTUNISTIC_SUSPEND
+	---help---
+	  User space suspend blockers API.  Creates a misc device allowing user
+	  space to create, use and destroy suspend blockers.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 95d8e6d..2015594 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= opportunistic_suspend.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..d53f939
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,143 @@
+/*
+ * kernel/power/user_suspend_blocker.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include <linux/suspend_ioctls.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);
+
+#define USER_SUSPEND_BLOCKER_NAME_LEN 31
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[USER_SUSPEND_BLOCKER_NAME_LEN + 1];
+	bool			registered;
+};
+
+static int user_suspend_blocker_open(struct inode *inode, struct file *filp)
+{
+	struct user_suspend_blocker *blocker;
+
+	blocker = kzalloc(sizeof(*blocker), GFP_KERNEL);
+	if (!blocker)
+		return -ENOMEM;
+
+	nonseekable_open(inode, filp);
+	strcpy(blocker->name, "(userspace)");
+	blocker->blocker.name = blocker->name;
+	filp->private_data = blocker;
+
+	return 0;
+}
+
+static int suspend_blocker_set_name(struct user_suspend_blocker *blocker,
+				    void __user *name, size_t name_len)
+{
+	if (blocker->registered)
+		return -EBUSY;
+
+	if (name_len > USER_SUSPEND_BLOCKER_NAME_LEN)
+		name_len = USER_SUSPEND_BLOCKER_NAME_LEN;
+
+	if (copy_from_user(blocker->name, name, name_len))
+		return -EFAULT;
+	blocker->name[name_len] = '\0';
+
+	return 0;
+}
+
+static long user_suspend_blocker_ioctl(struct file *filp, unsigned int cmd,
+					unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *blocker = filp->private_data;
+	long ret = 0;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_SET_NAME(0)) {
+		ret = suspend_blocker_set_name(blocker, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	if (!blocker->registered) {
+		suspend_blocker_register(&blocker->blocker);
+		blocker->registered = true;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&blocker->blocker);
+		break;
+
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&blocker->blocker);
+		break;
+
+	default:
+		ret = -ENOTTY;
+	}
+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 *filp)
+{
+	struct user_suspend_blocker *blocker = filp->private_data;
+
+	if (blocker->registered)
+		suspend_blocker_unregister(&blocker->blocker);
+	kfree(blocker);
+
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.open = user_suspend_blocker_open,
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.5.1

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

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

[Tejun Heo]

Hello,

(sorry about late reply)

On 04/30/2010 07:26 PM, Oleg Nesterov wrote:
> Currently _cpu_down() can't flush and/or stop the frozen cwq->thread.
> 
> IIRC this is fixable, but needs the nasty complications. We should
> thaw + stop the frozen cwq->thread, then move the pending works to
> another CPU.

Oh, this isn't an issue w/ cmwq.  While frozen all new works are
collected into per-cpu delayed worklist and while frozen trustee in
charge of the cpu will keep waiting.  Once thawed, trustee will
execute all works including the delayed ones unbound to any cpu.

Thanks.

-- 
tejun

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

[Rafael J. Wysocki]

On Thursday 20 May 2010, Tejun Heo wrote:
> Hello,
> 
> (sorry about late reply)
> 
> On 04/30/2010 07:26 PM, Oleg Nesterov wrote:
> > Currently _cpu_down() can't flush and/or stop the frozen cwq->thread.
> > 
> > IIRC this is fixable, but needs the nasty complications. We should
> > thaw + stop the frozen cwq->thread, then move the pending works to
> > another CPU.
> 
> Oh, this isn't an issue w/ cmwq.  While frozen all new works are
> collected into per-cpu delayed worklist and while frozen trustee in
> charge of the cpu will keep waiting.  Once thawed, trustee will
> execute all works including the delayed ones unbound to any cpu.

So, does it require any intrusive changes to make it possible to create
multithread freezable workqueues?

Rafael

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

[Tejun Heo]

Hello,

On 05/21/2010 12:27 AM, Rafael J. Wysocki wrote:
>> Oh, this isn't an issue w/ cmwq.  While frozen all new works are
>> collected into per-cpu delayed worklist and while frozen trustee in
>> charge of the cpu will keep waiting.  Once thawed, trustee will
>> execute all works including the delayed ones unbound to any cpu.
> 
> So, does it require any intrusive changes to make it possible to create
> multithread freezable workqueues?

cmwq itself is quite intrusive changes but w/ cmwq it just takes
flipping a flag when creating the queue.

Thanks.

-- 
tejun

[PATCH 2/8] 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 automatic suspend.

Opening this device creates a suspend blocker that can be used by the
opener to prevent automatic suspend from occurring.  There are ioctls
provided for blocking and unblocking suspend and for giving the
suspend blocker a meaningful name.  Closing the device special file
causes the suspend blocker to be destroyed.

For example, when select or poll indicates that input event are
available, this interface can be used by user space to block suspend
before it reads those events. This allows the input driver to release
its suspend blocker as soon as the event queue is empty. If user space
could not use a suspend blocker here the input driver would need to
delay the release of its suspend blocker until it knows (or assumes)
that user space has finished processing the events.

By careful use of suspend blockers in drivers and user space system
code, one can arrange for the system to stay awake for extremely short
periods of time in reaction to events, rapidly returning to a fully
suspended state.

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/ioctl/ioctl-number.txt          |    3 +-
 Documentation/power/opportunistic-suspend.txt |   27 +++++
 include/linux/suspend_ioctls.h                |    4 +
 kernel/power/Kconfig                          |    7 ++
 kernel/power/Makefile                         |    1 +
 kernel/power/user_suspend_blocker.c           |  143 +++++++++++++++++++++++++
 6 files changed, 184 insertions(+), 1 deletions(-)
 create mode 100644 kernel/power/user_suspend_blocker.c

diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
index dd5806f..e2458f7 100644
--- a/Documentation/ioctl/ioctl-number.txt
+++ b/Documentation/ioctl/ioctl-number.txt
@@ -254,7 +254,8 @@ Code  Seq#(hex)	Include File		Comments
 'q'	80-FF	linux/telephony.h	Internet PhoneJACK, Internet LineJACK
 		linux/ixjuser.h		<http://www.quicknet.net>
 'r'	00-1F	linux/msdos_fs.h and fs/fat/dir.c
-'s'	all	linux/cdk.h
+'s'	all	linux/cdk.h	conflict!
+'s'	all	linux/suspend_block_dev.h	conflict!
 't'	00-7F	linux/if_ppp.h
 't'	80-8F	linux/isdn_ppp.h
 't'	90	linux/toshiba.h
diff --git a/Documentation/power/opportunistic-suspend.txt b/Documentation/power/opportunistic-suspend.txt
index 4bee7bc..93f4c24 100644
--- a/Documentation/power/opportunistic-suspend.txt
+++ b/Documentation/power/opportunistic-suspend.txt
@@ -127,3 +127,30 @@ if (list_empty(&state->pending_work))
 	suspend_unblock(&state->suspend_blocker);
 else
 	suspend_block(&state->suspend_blocker);
+
+User space API
+==============
+
+To create a suspend blocker from user space, open the suspend_blocker special
+device file:
+
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+
+then optionally call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
+
+To activate the suspend blocker call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To deactivate it call:
+
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend blocker, close the device:
+
+    close(fd);
+
+If the first ioctl called is not SUSPEND_BLOCKER_IOCTL_SET_NAME the suspend
+blocker will get the default name "(userspace)".
diff --git a/include/linux/suspend_ioctls.h b/include/linux/suspend_ioctls.h
index 0b30382..b95a6b2 100644
--- a/include/linux/suspend_ioctls.h
+++ b/include/linux/suspend_ioctls.h
@@ -30,4 +30,8 @@ struct resume_swap_area {
 #define SNAPSHOT_ALLOC_SWAP_PAGE	_IOR(SNAPSHOT_IOC_MAGIC, 20, __kernel_loff_t)
 #define SNAPSHOT_IOC_MAXNR	20
 
+#define SUSPEND_BLOCKER_IOCTL_SET_NAME(len)	_IOC(_IOC_WRITE, 's', 0, len)
+#define SUSPEND_BLOCKER_IOCTL_BLOCK		_IO('s', 1)
+#define SUSPEND_BLOCKER_IOCTL_UNBLOCK		_IO('s', 2)
+
 #endif /* _LINUX_SUSPEND_IOCTLS_H */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 6d11a45..2e665cd 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -146,6 +146,13 @@ config OPPORTUNISTIC_SUSPEND
 	  determines the sleep state the system will be put into when there are
 	  no active suspend blockers.
 
+config USER_SUSPEND_BLOCKERS
+	bool "User space suspend blockers"
+	depends on OPPORTUNISTIC_SUSPEND
+	---help---
+	  User space suspend blockers API.  Creates a misc device allowing user
+	  space to create, use and destroy suspend blockers.
+
 config HIBERNATION_NVS
 	bool
 
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
index 95d8e6d..2015594 100644
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PM_SLEEP)		+= console.o
 obj-$(CONFIG_FREEZER)		+= process.o
 obj-$(CONFIG_SUSPEND)		+= suspend.o
 obj-$(CONFIG_OPPORTUNISTIC_SUSPEND)	+= opportunistic_suspend.o
+obj-$(CONFIG_USER_SUSPEND_BLOCKERS)	+= user_suspend_blocker.o
 obj-$(CONFIG_PM_TEST_SUSPEND)	+= suspend_test.o
 obj-$(CONFIG_HIBERNATION)	+= hibernate.o snapshot.o swap.o user.o
 obj-$(CONFIG_HIBERNATION_NVS)	+= hibernate_nvs.o
diff --git a/kernel/power/user_suspend_blocker.c b/kernel/power/user_suspend_blocker.c
new file mode 100644
index 0000000..d53f939
--- /dev/null
+++ b/kernel/power/user_suspend_blocker.c
@@ -0,0 +1,143 @@
+/*
+ * kernel/power/user_suspend_blocker.c
+ *
+ * Copyright (C) 2009-2010 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/suspend.h>
+#include <linux/suspend_ioctls.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);
+
+#define USER_SUSPEND_BLOCKER_NAME_LEN 31
+
+struct user_suspend_blocker {
+	struct suspend_blocker	blocker;
+	char			name[USER_SUSPEND_BLOCKER_NAME_LEN + 1];
+	bool			registered;
+};
+
+static int user_suspend_blocker_open(struct inode *inode, struct file *filp)
+{
+	struct user_suspend_blocker *blocker;
+
+	blocker = kzalloc(sizeof(*blocker), GFP_KERNEL);
+	if (!blocker)
+		return -ENOMEM;
+
+	nonseekable_open(inode, filp);
+	strcpy(blocker->name, "(userspace)");
+	blocker->blocker.name = blocker->name;
+	filp->private_data = blocker;
+
+	return 0;
+}
+
+static int suspend_blocker_set_name(struct user_suspend_blocker *blocker,
+				    void __user *name, size_t name_len)
+{
+	if (blocker->registered)
+		return -EBUSY;
+
+	if (name_len > USER_SUSPEND_BLOCKER_NAME_LEN)
+		name_len = USER_SUSPEND_BLOCKER_NAME_LEN;
+
+	if (copy_from_user(blocker->name, name, name_len))
+		return -EFAULT;
+	blocker->name[name_len] = '\0';
+
+	return 0;
+}
+
+static long user_suspend_blocker_ioctl(struct file *filp, unsigned int cmd,
+					unsigned long _arg)
+{
+	void __user *arg = (void __user *)_arg;
+	struct user_suspend_blocker *blocker = filp->private_data;
+	long ret = 0;
+
+	mutex_lock(&ioctl_lock);
+	if ((cmd & ~IOCSIZE_MASK) == SUSPEND_BLOCKER_IOCTL_SET_NAME(0)) {
+		ret = suspend_blocker_set_name(blocker, arg, _IOC_SIZE(cmd));
+		goto done;
+	}
+	if (!blocker->registered) {
+		suspend_blocker_register(&blocker->blocker);
+		blocker->registered = true;
+	}
+	switch (cmd) {
+	case SUSPEND_BLOCKER_IOCTL_BLOCK:
+		suspend_block(&blocker->blocker);
+		break;
+
+	case SUSPEND_BLOCKER_IOCTL_UNBLOCK:
+		suspend_unblock(&blocker->blocker);
+		break;
+
+	default:
+		ret = -ENOTTY;
+	}
+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 *filp)
+{
+	struct user_suspend_blocker *blocker = filp->private_data;
+
+	if (blocker->registered)
+		suspend_blocker_unregister(&blocker->blocker);
+	kfree(blocker);
+
+	return 0;
+}
+
+const struct file_operations user_suspend_blocker_fops = {
+	.open = user_suspend_blocker_open,
+	.release = user_suspend_blocker_release,
+	.unlocked_ioctl = user_suspend_blocker_ioctl,
+};
+
+struct miscdevice user_suspend_blocker_device = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "suspend_blocker",
+	.fops = &user_suspend_blocker_fops,
+};
+
+static int __init user_suspend_blocker_init(void)
+{
+	return misc_register(&user_suspend_blocker_device);
+}
+
+static void __exit user_suspend_blocker_exit(void)
+{
+	misc_deregister(&user_suspend_blocker_device);
+}
+
+module_init(user_suspend_blocker_init);
+module_exit(user_suspend_blocker_exit);
-- 
1.6.5.1

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

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

[Peter Zijlstra]

On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> +To create a suspend blocker from user space, open the suspend_blocker
> special
> +device file:
> +
> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> +
> +then optionally call:
> +
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> +
> +To activate the suspend blocker call:
> +
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> +
> +To deactivate it call:
> +
> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> +
> +To destroy the suspend blocker, close the device:
> +
> +    close(fd);

Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
the SET_NAME thing if you really care.

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

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

[Arve Hjønnevåg]

2010/5/26 Peter Zijlstra <peterz@infradead.org>:
> On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
>> +To create a suspend blocker from user space, open the suspend_blocker
>> special
>> +device file:
>> +
>> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> +
>> +then optionally call:
>> +
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
>> +
>> +To activate the suspend blocker call:
>> +
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
>> +
>> +To deactivate it call:
>> +
>> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
>> +
>> +To destroy the suspend blocker, close the device:
>> +
>> +    close(fd);
>
> Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> the SET_NAME thing if you really care.
>

That would be very inefficient.

-- 
Arve Hjønnevåg

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

[Peter Zijlstra]

On Wed, 2010-05-26 at 03:47 -0700, Arve Hjønnevåg wrote:
> 2010/5/26 Peter Zijlstra <peterz@infradead.org>:
> > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> >> +To create a suspend blocker from user space, open the suspend_blocker
> >> special
> >> +device file:
> >> +
> >> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> >> +
> >> +then optionally call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> >> +
> >> +To activate the suspend blocker call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> >> +
> >> +To deactivate it call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> >> +
> >> +To destroy the suspend blocker, close the device:
> >> +
> >> +    close(fd);
> >
> > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > the SET_NAME thing if you really care.
> >
> 
> That would be very inefficient.

How so? Anyway, since you admitted this thing isn't needed at all, I say
we remove it altogether.
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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

[Florian Mickler]

On Wed, 26 May 2010 03:47:27 -0700
Arve Hjønnevåg <arve@android.com> wrote:

> 2010/5/26 Peter Zijlstra <peterz@infradead.org>:
> > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> >> +To create a suspend blocker from user space, open the suspend_blocker
> >> special
> >> +device file:
> >> +
> >> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> >> +
> >> +then optionally call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> >> +
> >> +To activate the suspend blocker call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> >> +
> >> +To deactivate it call:
> >> +
> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> >> +
> >> +To destroy the suspend blocker, close the device:
> >> +
> >> +    close(fd);
> >
> > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > the SET_NAME thing if you really care.
> >
> 
> That would be very inefficient.
> 

Also I think it is intended to enforce named suspend blockers. (For
debugging/accounting purposes).

Cheers,
Flo

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

[Peter Zijlstra]

On Wed, 2010-05-26 at 12:51 +0200, Florian Mickler wrote:
> On Wed, 26 May 2010 03:47:27 -0700
> Arve Hjønnevåg <arve@android.com> wrote:
> 
> > 2010/5/26 Peter Zijlstra <peterz@infradead.org>:
> > > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> > >> +To create a suspend blocker from user space, open the suspend_blocker
> > >> special
> > >> +device file:
> > >> +
> > >> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > >> +
> > >> +then optionally call:
> > >> +
> > >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> > >> +
> > >> +To activate the suspend blocker call:
> > >> +
> > >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> > >> +
> > >> +To deactivate it call:
> > >> +
> > >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> > >> +
> > >> +To destroy the suspend blocker, close the device:
> > >> +
> > >> +    close(fd);
> > >
> > > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > > the SET_NAME thing if you really care.
> > >
> > 
> > That would be very inefficient.
> > 
> 
> Also I think it is intended to enforce named suspend blockers. (For
> debugging/accounting purposes).

I don't think the code as proposed mandates you SET_NAME, and I didn't
propose killing that off, you can still SET_NAME after you open() and
acquire the thing.

Anyway, the whole point is moot since its simply not needed at all.
_______________________________________________
linux-pm mailing list
linux-pm@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/linux-pm

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

[Rafael J. Wysocki]

On Wednesday 26 May 2010, Peter Zijlstra wrote:
> On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> > +To create a suspend blocker from user space, open the suspend_blocker
> > special
> > +device file:
> > +
> > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > +
> > +then optionally call:
> > +
> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> > +
> > +To activate the suspend blocker call:
> > +
> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> > +
> > +To deactivate it call:
> > +
> > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> > +
> > +To destroy the suspend blocker, close the device:
> > +
> > +    close(fd);
> 
> Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> the SET_NAME thing if you really care.

SET_NAME wouldn't serve any purpose in that case.

This whole thing is related to the statistics part, which Arve says is
essential to him.  He wants to collect statistics for each suspend blocker
activated and deactivated so that he can tell who's causing problems by
blocking suspend too often.  The name also is a part of this.

In fact, without the statistics part the whole thing might be implemented
as a single reference counter such that suspend would happen when it went down
to zero.

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

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

[Alan Stern]

On Wed, 26 May 2010, Rafael J. Wysocki wrote:

> On Wednesday 26 May 2010, Peter Zijlstra wrote:
> > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> > > +To create a suspend blocker from user space, open the suspend_blocker
> > > special
> > > +device file:
> > > +
> > > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > > +
> > > +then optionally call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> > > +
> > > +To activate the suspend blocker call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> > > +
> > > +To deactivate it call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> > > +
> > > +To destroy the suspend blocker, close the device:
> > > +
> > > +    close(fd);
> > 
> > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > the SET_NAME thing if you really care.
> 
> SET_NAME wouldn't serve any purpose in that case.
> 
> This whole thing is related to the statistics part, which Arve says is
> essential to him.  He wants to collect statistics for each suspend blocker
> activated and deactivated so that he can tell who's causing problems by
> blocking suspend too often.  The name also is a part of this.
> 
> In fact, without the statistics part the whole thing might be implemented
> as a single reference counter such that suspend would happen when it went down
> to zero.

There's also Arve's other main point: These suspend blockers tend to
get used quite a lot.  Opening and closing a file each time has much
higher overhead than a simple ioctl.

All these topics have been covered earlier in this discussion.  Peter 
should go back and read the emails in the linux-pm archive.

Alan Stern

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

[Alan Stern]

On Wed, 26 May 2010, Rafael J. Wysocki wrote:

> On Wednesday 26 May 2010, Peter Zijlstra wrote:
> > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> > > +To create a suspend blocker from user space, open the suspend_blocker
> > > special
> > > +device file:
> > > +
> > > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > > +
> > > +then optionally call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> > > +
> > > +To activate the suspend blocker call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> > > +
> > > +To deactivate it call:
> > > +
> > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> > > +
> > > +To destroy the suspend blocker, close the device:
> > > +
> > > +    close(fd);
> > 
> > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > the SET_NAME thing if you really care.
> 
> SET_NAME wouldn't serve any purpose in that case.
> 
> This whole thing is related to the statistics part, which Arve says is
> essential to him.  He wants to collect statistics for each suspend blocker
> activated and deactivated so that he can tell who's causing problems by
> blocking suspend too often.  The name also is a part of this.
> 
> In fact, without the statistics part the whole thing might be implemented
> as a single reference counter such that suspend would happen when it went down
> to zero.

There's also Arve's other main point: These suspend blockers tend to
get used quite a lot.  Opening and closing a file each time has much
higher overhead than a simple ioctl.

All these topics have been covered earlier in this discussion.  Peter 
should go back and read the emails in the linux-pm archive.

Alan Stern

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

[Rafael J. Wysocki]

On Thursday 27 May 2010, Alan Stern wrote:
> On Wed, 26 May 2010, Rafael J. Wysocki wrote:
> 
> > On Wednesday 26 May 2010, Peter Zijlstra wrote:
> > > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
> > > > +To create a suspend blocker from user space, open the suspend_blocker
> > > > special
> > > > +device file:
> > > > +
> > > > +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
> > > > +
> > > > +then optionally call:
> > > > +
> > > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
> > > > +
> > > > +To activate the suspend blocker call:
> > > > +
> > > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
> > > > +
> > > > +To deactivate it call:
> > > > +
> > > > +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
> > > > +
> > > > +To destroy the suspend blocker, close the device:
> > > > +
> > > > +    close(fd);
> > > 
> > > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
> > > the SET_NAME thing if you really care.
> > 
> > SET_NAME wouldn't serve any purpose in that case.
> > 
> > This whole thing is related to the statistics part, which Arve says is
> > essential to him.  He wants to collect statistics for each suspend blocker
> > activated and deactivated so that he can tell who's causing problems by
> > blocking suspend too often.  The name also is a part of this.
> > 
> > In fact, without the statistics part the whole thing might be implemented
> > as a single reference counter such that suspend would happen when it went down
> > to zero.
> 
> There's also Arve's other main point: These suspend blockers tend to
> get used quite a lot.  Opening and closing a file each time has much
> higher overhead than a simple ioctl.
> 
> All these topics have been covered earlier in this discussion.  Peter 
> should go back and read the emails in the linux-pm archive.

Yeah.

Although with a reference counter the special device file won't be necessary.

Rafael

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

[Arve Hjønnevåg]

2010/5/26 Peter Zijlstra <peterz@infradead.org>:
> On Wed, 2010-05-26 at 03:47 -0700, Arve Hjønnevåg wrote:
>> 2010/5/26 Peter Zijlstra <peterz@infradead.org>:
>> > On Fri, 2010-05-21 at 15:46 -0700, Arve Hjønnevåg wrote:
>> >> +To create a suspend blocker from user space, open the suspend_blocker
>> >> special
>> >> +device file:
>> >> +
>> >> +    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
>> >> +
>> >> +then optionally call:
>> >> +
>> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_SET_NAME(strlen(name)), name);
>> >> +
>> >> +To activate the suspend blocker call:
>> >> +
>> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
>> >> +
>> >> +To deactivate it call:
>> >> +
>> >> +    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
>> >> +
>> >> +To destroy the suspend blocker, close the device:
>> >> +
>> >> +    close(fd);
>> >
>> > Urgh, please let the open() be BLOCK, the close() be UNBLOCK, and keep
>> > the SET_NAME thing if you really care.
>> >
>>
>> That would be very inefficient.
>
> How so? Anyway, since you admitted this thing isn't needed at all, I say
> we remove it altogether.
>

I also said it was useful. I don't think we should drop it just
because we can work around its absence.

-- 
Arve Hjønnevåg