This patch series adds a suspend-block api that provides the same
functionality as the android wakelock api. The code is mostly the
same as version 3 posted last year, but the main entry point has
changed back to /sys/power/state with a new /sys/power/policy to
alter its behavior. Timeout support is not included in this initial
patchset.

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


--

[ message continues ]

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

Signed-off-by: Arve Hjønnevåg <arve@android.com>
---
 Documentation/power/suspend-blockers.txt |   97 +++++++++++++
 include/linux/suspend_blocker.h          |   60 ++++++++
 kernel/power/Kconfig                     |   11 ++
 kernel/power/Makefile                    |    1 +
 kernel/power/main.c                      |   84 +++++++++++-
 kernel/power/power.h                     |    5 +
 kernel/power/suspend.c                   |    4 +-
 kernel/power/suspend_blocker.c           |  219 ++++++++++++++++++++++++++++++
 8 files changed, 477 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/power/suspend-blockers.txt
 create mode 100755 include/linux/suspend_blocker.h
 create mode 100644 kernel/power/suspend_blocker.c

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
new file mode 100644
index 0000000..1c48514
--- /dev/null
+++ b/Documentation/power/suspend-blockers.txt
@@ -0,0 +1,97 @@
+Suspend blockers
+================
+
+Suspend blockers provide a mechanism for device drivers and user-space processes
+to prevent the system from entering suspend. By default writing to
+/sys/power/state will ignore suspend blockers. Writing "opportunistic" to
+/sys/power/policy will change the behaviour of /sys/power/state to repeatedly
+enter the requested state when no suspend blockers are active. Writing "on" to
+/sys/power/state will cancel the automatic sleep request. Suspend blockers do
+not affect sleep states entered from idle.
+
+To use a suspend blocker, a device driver needs a struct suspend_blocker object
+that has to be initialized with suspend_blocker_init. When no longer needed,
+the ...
[ message continues ]

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 > ...
[ message continues ]

This allows active suspend blockers to be listed

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

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index 21689cd..1faa433 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -16,8 +16,11 @@
 #ifndef _LINUX_SUSPEND_BLOCKER_H
 #define _LINUX_SUSPEND_BLOCKER_H
 
+#include <linux/list.h>
+
 /**
  * struct suspend_blocker - the basic suspend_blocker structure
+ * @link:	List entry for active or inactive list.
  * @flags:	Tracks initialized and active state.
  * @name:	Name used for debugging.
  *
@@ -29,7 +32,8 @@
 
 struct suspend_blocker {
 #ifdef CONFIG_SUSPEND_BLOCKERS
-	atomic_t            flags;
+	struct list_head    link;
+	int                 flags;
 	const char         *name;
 #endif
 };
diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 1b345d1..868e8f1 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -32,9 +32,11 @@ module_param_named(debug_mask, debug_mask, int, S_IRUGO | S_IWUSR | S_IWGRP);
 #define SB_INITIALIZED            (1U << 8)
 #define SB_ACTIVE                 (1U << 9)
 
+static DEFINE_SPINLOCK(list_lock);
 static DEFINE_SPINLOCK(state_lock);
-static atomic_t suspend_block_count;
-static atomic_t current_event_num;
+static LIST_HEAD(inactive_blockers);
+static LIST_HEAD(active_blockers);
+static int current_event_num;
 struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
@@ -52,6 +54,14 @@ static bool enable_suspend_blockers;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+static void print_active_blockers_locked(void)
+{
+	struct suspend_blocker ...
[ message continues ]

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

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

diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 9466e86..9c5a078 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -9,6 +9,7 @@
 #include <linux/linkage.h>
 #include <linux/bitops.h>
 #include <linux/lockdep.h>
+#include <linux/suspend_blocker.h>
 #include <asm/atomic.h>
 
 struct workqueue_struct;
@@ -26,7 +27,8 @@ struct work_struct {
 	atomic_long_t data;
 #define WORK_STRUCT_PENDING 0		/* T if work item pending execution */
 #define WORK_STRUCT_STATIC  1		/* static initializer (debugobjects) */
-#define WORK_STRUCT_FLAG_MASK (3UL)
+#define WORK_STRUCT_SUSPEND_BLOCKING  2	/* suspend blocking work */
+#define WORK_STRUCT_FLAG_MASK (7UL)
 #define WORK_STRUCT_WQ_DATA_MASK (~WORK_STRUCT_FLAG_MASK)
 	struct list_head entry;
 	work_func_t func;
@@ -48,6 +50,23 @@ static inline struct delayed_work *to_delayed_work(struct work_struct *work)
 	return container_of(work, struct delayed_work, work);
 }
 
+struct suspend_blocking_work {
+	struct work_struct work;
+	struct suspend_blocker suspend_blocker;
+};
+
+static inline struct suspend_blocking_work *
+to_suspend_blocking_work(struct work_struct *work)
+{
+	return container_of(work, struct suspend_blocking_work, work);
+}
+
+static inline struct suspend_blocker *
+work_to_suspend_blocker(struct work_struct *work)
+{
+	return &to_suspend_blocking_work(work)->suspend_blocker;
+}
+
 struct execute_work {
 	struct work_struct work;
 };
@@ -157,6 +176,30 @@ static inline void ...
[ message continues ]

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

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

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

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);
 	}
 
 ...
[ message continues ]

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

[ message continues ]

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

-- 
Dmitry
--

[ message continues ]

On Fri, Apr 23, 2010 at 2:08 PM, Dmitry Torokhov

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

-- 
Arve Hjønnevåg
--

[ message continues ]

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

-- 
Arve Hjønnevåg
--

[ message continues ]

Hello,


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

Thanks.

-- 
tejun
--

[ message continues ]

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


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

Oleg.

--

[ message continues ]

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

Calling suspend_unblock() twice is not a problem as long as


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

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

-- 
Arve Hjønnevåg
--

[ message continues ]

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

-- 
Arve Hjønnevåg
--

[ message continues ]

Hello,


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

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

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

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

Thanks.

-- 
tejun
--

[ message continues ]

I only touch the work structure after the callback has returned for

Sorry, I didn't see the for_each_cpu(cpu, cpu_map) part of
wait_on_work(). cancel_work_sync() does look safe as long as the work

OK.

-- 
Arve Hjønnevåg
--

[ message continues ]

Hello,


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

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

Thanks.

-- 
tejun
--

[ message continues ]

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

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

Oleg.

--

[ message continues ]

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

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

diff --git a/kernel/power/suspend_blocker.c b/kernel/power/suspend_blocker.c
index 868e8f1..047e910 100644
--- a/kernel/power/suspend_blocker.c
+++ b/kernel/power/suspend_blocker.c
@@ -17,6 +17,7 @@
 #include <linux/rtc.h>
 #include <linux/suspend.h>
 #include <linux/suspend_blocker.h>
+#include <linux/debugfs.h>
 #include "power.h"
 
 enum {
@@ -41,6 +42,7 @@ struct workqueue_struct *suspend_work_queue;
 struct suspend_blocker main_suspend_blocker;
 static suspend_state_t requested_suspend_state = PM_SUSPEND_MEM;
 static bool enable_suspend_blockers;
+static struct dentry *suspend_blocker_stats_dentry;
 
 #define pr_info_time(fmt, args...) \
 	do { \
@@ -54,6 +56,21 @@ static bool enable_suspend_blockers;
 			tm.tm_hour, tm.tm_min, tm.tm_sec, ts.tv_nsec); \
 	} while (0);
 
+static int suspend_blocker_stats_show(struct seq_file *m, void *unused)
+{
+	unsigned long irqflags;
+	struct suspend_blocker *blocker;
+
+	seq_puts(m, "name\tactive\n");
+	spin_lock_irqsave(&list_lock, irqflags);
+	list_for_each_entry(blocker, &inactive_blockers, link)
+		seq_printf(m, "\"%s\"\t0\n", blocker->name);
+	list_for_each_entry(blocker, &active_blockers, link)
+		seq_printf(m, "\"%s\"\t1\n", blocker->name);
+	spin_unlock_irqrestore(&list_lock, irqflags);
+	return 0;
+}
+
 static void print_active_blockers_locked(void)
 {
 	struct suspend_blocker *blocker;
@@ -101,8 +118,8 @@ static DECLARE_WORK(suspend_work, suspend_worker);
 /**
  * suspend_blocker_init() - Initialize a suspend blocker
  * @blocker:	The suspend blocker to initialize.
- * @name:	The name of the suspend blocker to show in debug messages.
- *
+ * @name:	The name of the suspend blocker to show in debug messages and
+ ...
[ message continues ]

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

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

diff --git a/include/linux/suspend_blocker.h b/include/linux/suspend_blocker.h
index 1faa433..3bb8a6a 100755
--- a/include/linux/suspend_blocker.h
+++ b/include/linux/suspend_blocker.h
@@ -17,12 +17,21 @@
 #define _LINUX_SUSPEND_BLOCKER_H
 
 #include <linux/list.h>
+#include <linux/ktime.h>
 
 /**
  * struct suspend_blocker - the basic suspend_blocker structure
  * @link:	List entry for active or inactive list.
- * @flags:	Tracks initialized and active state.
+ * @flags:	Tracks initialized, active and stats state.
  * @name:	Name used for debugging.
+ * @count:	Number of times this blocker has been deacivated
+ * @wakeup_count: Number of times this blocker was the first to block suspend
+ *		after resume.
+ * @total_time:	Total time this suspend blocker has prevented suspend.
+ * @prevent_suspend_time: Time this suspend blocker has prevented suspend while
+ *		user-space requested suspend.
+ * @max_time:	Max time this suspend blocker has been continuously active
+ * @last_time:	Monotonic clock when the active state last changed.
  *
  * When a suspend_blocker is active it prevents the system from entering
  * suspend.
@@ -35,6 +44,16 @@ struct suspend_blocker {
 	struct list_head    link;
 	int                 flags;
 	const char         *name;
+#ifdef CONFIG_SUSPEND_BLOCKER_STATS
+	struct {
+		int             count;
+		int             wakeup_count;
+		ktime_t         total_time;
+		ktime_t         prevent_suspend_time;
+		ktime_t         max_time;
+		ktime_t         last_time;
+	} stat;
+#endif
 #endif
 };
 
diff --git ...
[ message continues ]

This looks more like user interface information than it does debug information.


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

[ message continues ]

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

-- 
Arve Hjønnevåg
--

[ message continues ]

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

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

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

[ message continues ]

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

-- 
Arve Hjønnevåg
--

[ message continues ]

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

thanks,

greg k-h
--

[ message continues ]

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

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

[ message continues ]

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

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

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

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

Regards,

						- Ted
--

[ message continues ]

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

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

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

[ message continues ]

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

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

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

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

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



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

[ message continues ]

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

Brian
--

[ message continues ]

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

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

[ message continues ]

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

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

diff --git a/Documentation/power/suspend-blockers.txt b/Documentation/power/suspend-blockers.txt
index 1c48514..877bd8c 100644
--- a/Documentation/power/suspend-blockers.txt
+++ b/Documentation/power/suspend-blockers.txt
@@ -95,3 +95,20 @@ if (list_empty(&state->pending_work))
 else
 	suspend_block(&state->suspend_blocker);
 
+User-space API
+==============
+
+To create a suspend_blocker from user-space, open the suspend_blocker device:
+    fd = open("/dev/suspend_blocker", O_RDWR | O_CLOEXEC);
+then call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_INIT(strlen(name)), name);
+
+To activate a suspend_blocker call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_BLOCK);
+
+To unblock call:
+    ioctl(fd, SUSPEND_BLOCKER_IOCTL_UNBLOCK);
+
+To destroy the suspend_blocker, close the device:
+    close(fd);
+
diff --git a/include/linux/suspend_block_dev.h b/include/linux/suspend_block_dev.h
new file mode 100644
index 0000000..24bc5c7
--- /dev/null
+++ b/include/linux/suspend_block_dev.h
@@ -0,0 +1,25 @@
+/* include/linux/suspend_block_dev.h
+ *
+ * Copyright (C) 2009 Google, Inc.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, ...
[ message continues ]

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

Should the kernel reject empty strings or strings composed only of

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

Do you block/unblock often enough that frequent open/close of the device are


nit: Usually locks protect data -- not functions.

Couldn't this be part of the user_suspend_blocker struct? That would allow

<snip>

Cheers,
	-Matt Helsley
--

[ message continues ]

2010/4/22 Matt Helsley <matthltc@us.ibm.com>:

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

Is there an existing function that check if a sting is "unsafe". If


Yes.


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



-- 
Arve Hjønnevåg
--

[ message continues ]

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

thanks,

greg k-h
--

[ message continues ]

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

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

[ message continues ]

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

						- Ted
--

[ message continues ]

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

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

[ message continues ]

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

thanks again,

greg k-h
--

[ message continues ]