[RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

Low Power Events is a possible alternative to suspend blocker / wake
lock API used by Android.

It provides comparable power state notification and kernel mode critical
section definition.  It differs from suspend blocker in that:
1) it is a platform and device independent implementation.  Device
specific code is register as lp_ops, similar to pm_ops.  Drivers use
the platform independent functions.

2) it forces a user mode transition coming out of a LP state.
Notification of wake up sources go to the user mode process managing the
lp states.  Notification of blocked LP state entry is through an
error return, and notification of un-blocking is through a file node as
well.

I think the change need to the Google Android user mode power handling
code can be limited to changes to _only_
"hardware/libhardware_legacy/power/power.c"

This code is still just a prototype of the platform independent code,
and I'm implementing it on Eclair for the Intel Moorestown platform this
weekend.  I'll have patches to eclair and updated kernel patches that
enable this sometime Monday, after I bring it up on my target device.
Hopefully by the end of next week I think I'll have it working well with
Android.

At this time the following patch is only known to compile.  I send it to
help with the discussion.

FWTW I do work on Android at Intel and I think I can make this work.
(well, today I do.)

--mgross

Draft kernel patch :

--Signed-off-by: Mark Gross <markgross@thegnar.org>


>From 5061a182e18520da792760e2008c3f051f032426 Mon Sep 17 00:00:00 2001
From: markgross <markgross@thegnar.org>
Date: Sun, 30 May 2010 11:58:33 -0700
Subject: [PATCH] New power management event mechanism for implementing Android power
 management by passing event data up to the user mode power manager.

---
 Documentation/power/low_power_events_interface.txt |   65 +++
 include/linux/low_power_events.h                   |   39 ++
 kernel/Makefile                                    |    2 +-
 kernel/low_power_events.c                          |  428 ++++++++++++++++++++
 4 files changed, 533 insertions(+), 1 deletions(-)
 create mode 100644 Documentation/power/low_power_events_interface.txt
 create mode 100644 include/linux/low_power_events.h
 create mode 100644 kernel/low_power_events.c

diff --git a/Documentation/power/low_power_events_interface.txt b/Documentation/power/low_power_events_interface.txt
new file mode 100644
index 0000000..f8b5c0d
--- /dev/null
+++ b/Documentation/power/low_power_events_interface.txt
@@ -0,0 +1,65 @@
+Low Power Events is a power state / event framework that can be used by
+Google Android and other power screams.
+
+It provides comparable power state notification and kernel mode critical
+section definition.  It differs from suspend blocker in that:
+
+1) it is a platform and device independent implementation.  Device specific
+code is register as lp_ops, similar to pm_ops.  Drivers use the platform
+independent functions.
+
+2) it forces a user mode transition coming out of a LP state.  Notification
+of wake up sources go to the user mode process managing the lp states.
+Notification of blocked LP state entry is through an error return, and
+notification of un-blocking is through a file node as well.
+
+The user mode ABI is defined by the following files implemented as misc
+devices in my current implementation:
+
+/dev/lpe_enter:
+writes of an s32 integer or 0x00000000 formatted string implements a
+blocking state change.  -EBUSY is returned if LP mode is blocked or a wake
+even is unacknowledged.  Re-entry to a LP mode is blocked until the
+low_power_wake_event file is read.
+
+/dev/lpe_blocked:
+blocking read or selects while requested state is blocked at the kernel
+level, by some driver holding a blocker-critical section.
+
+/dev/lpe_wake_event:
+reads from this returns a list of wake events that happened between entry
+into the LP state and the return from the read of the low_power_enter device
+node.  User mode is expected to do something with these events before
+re-trying to enter low power mode.
+
+The kernel mode API is defined by the following:
+
+new_lpe_block :  
+delete_lpe_block :
+Allocate / free a low power blocker.
+
+lpe_block :
+Block entry into any low power level > level.
+
+lpe_unblock :
+Remove block
+
+register_wake_event : 
+unregister_wake_event :
+register / un-register wake events that need to be handled by user mode
+before re-entry into a low power state.
+
+set_wake_event :
+Wake up sources call this to pass the event to user mode after wake up
+processing.
+
+lpe_ponr :
+Helper function that needs to be called by the platform specific lpe_ops or
+pm_ops function at the "point of no return" on the way into the low power
+state.
+
+set_lpe_ops :
+Platform code registers its specific entry code to the low power state.
+
+
+
diff --git a/include/linux/low_power_events.h b/include/linux/low_power_events.h
new file mode 100644
index 0000000..335602f
--- /dev/null
+++ b/include/linux/low_power_events.h
@@ -0,0 +1,39 @@
+/*
+ * Low power event framework for implementing user guided
+ * power state control approximately equivalent to to 
+ * Android wake lock or suspend blocking without the tight 
+ * coupling with the platform 
+ *
+ * Mark Gross <markgross@thegnar.org>
+ */
+
+#include <linux/list.h>
+
+struct lpe_ops {
+	int (*enter)(int);
+};
+
+struct lpe_block_list {
+	struct list_head list;
+	char *name;
+	int level; /*block entry into lp modes > level */
+};
+
+struct lpe_wake_event_list{
+	struct list_head list;
+	char *name;
+	int event;
+};
+
+
+struct lpe_block_list *new_lpe_block(char *name);
+void delete_lpe_block(struct lpe_block_list *blocker);
+int lpe_block(struct lpe_block_list *blocker, int level);
+int lpe_unblock(struct lpe_block_list *blocker);
+
+struct lpe_wake_event_list *register_wake_event(char *name);
+int unregister_wake_event(struct lpe_wake_event_list *waker);
+void set_wake_event(struct lpe_wake_event_list *waker);
+void lpe_ponr(void);
+void set_lpe_ops(struct lpe_ops *new_ops);
+
diff --git a/kernel/Makefile b/kernel/Makefile
index 057472f..d311462 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -10,7 +10,7 @@ obj-y     = sched.o fork.o exec_domain.o panic.o printk.o \
 	    kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
 	    hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
 	    notifier.o ksysfs.o pm_qos_params.o sched_clock.o cred.o \
-	    async.o range.o
+	    async.o range.o low_power_events.o 
 obj-$(CONFIG_HAVE_EARLY_RES) += early_res.o
 obj-y += groups.o
 
diff --git a/kernel/low_power_events.c b/kernel/low_power_events.c
new file mode 100644
index 0000000..1f3c591
--- /dev/null
+++ b/kernel/low_power_events.c
@@ -0,0 +1,428 @@
+/*
+ * Low power event framework for implementing user guided
+ * power state control approximately equivalent to to 
+ * Android wake lock or suspend blocking without the tight 
+ * coupling with the platform
+ *
+ * Mark Gross <markgross@thegnar.org>
+ */
+
+#include <linux/low_power_events.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/fs.h>
+#include <linux/miscdevice.h>
+#include <linux/string.h>
+#include <linux/init.h>
+#include <linux/uaccess.h>
+
+/*
+ * low_power_events core data structures
+ */
+#define ACTIVE 0
+
+static int current_lp_level;
+static int requested_lp_level;
+static int lp_blocked;
+
+struct lpe_blockers{
+	int level;
+	struct lpe_block_list blockers;
+};
+
+static struct lpe_blockers lpe_critical_sections = {
+	.blockers.list = LIST_HEAD_INIT(lpe_critical_sections.blockers.list)
+};
+
+static struct lpe_wake_event_list lpe_wakers = {
+	.list = LIST_HEAD_INIT(lpe_wakers.list)
+};
+static struct lpe_ops ops;
+static wait_queue_head_t lpe_blocked_wq;
+
+/*
+ * lp event lock.  One lock protecting the above data.
+ */
+static DEFINE_SPINLOCK(lpe_lock);
+
+
+/*
+ * accessors and static functions
+ */
+
+void set_lpe_ops(struct lpe_ops *new_ops)
+{
+	if (new_ops)
+		ops.enter = new_ops->enter;
+}
+EXPORT_SYMBOL_GPL(set_lpe_ops);
+
+/*
+ * assumes caller holds lpe_lock
+ */
+void update_block_level(void)
+{
+	int level;
+	struct lpe_block_list *node;
+	
+	level =0;
+	list_for_each_entry(node,
+			&lpe_critical_sections.blockers.list, list) {
+		if (level < node->level)
+			level = node->level;
+	}
+	lpe_critical_sections.level = level;
+	if (lp_blocked && (requested_lp_level < level)) {
+		lp_blocked = 0;
+		wake_up_all(&lpe_blocked_wq);
+	}
+}
+
+/*
+ * assumes caller handles name buffer, will when blocker is 
+ * freed it will not free the name pointer
+ */
+struct lpe_block_list *new_lpe_block(char *name)
+{
+	struct lpe_block_list *dep;
+	unsigned long flags;
+
+	dep = kzalloc(sizeof(struct lpe_block_list), GFP_KERNEL);
+	if (dep) {
+		spin_lock_irqsave(&lpe_lock, flags);
+		dep->name = name;
+		list_add(&dep->list, &lpe_critical_sections.blockers.list);
+		spin_unlock_irqrestore(&lpe_lock, flags);
+	}
+
+	return dep;
+}
+EXPORT_SYMBOL_GPL(new_lpe_block);
+
+
+
+void delete_lpe_block(struct lpe_block_list *blocker)
+{
+	unsigned long flags;
+
+	if (blocker == NULL)
+		return;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	list_del(&blocker->list);
+	kfree(blocker);
+	update_block_level();
+	spin_unlock_irqrestore(&lpe_lock, flags);
+}
+EXPORT_SYMBOL_GPL(delete_lpe_block);
+
+
+int lpe_block(struct lpe_block_list *blocker, int level)
+{
+	unsigned long flags;
+
+	if (blocker) {
+		spin_lock_irqsave(&lpe_lock, flags);
+		blocker->level = level;
+		update_block_level();
+		spin_unlock_irqrestore(&lpe_lock, flags);
+		return 1;
+	} else {
+		WARN(true, "lpe_block abuse!\n");
+		dump_stack();
+	}
+	return -1;
+}
+EXPORT_SYMBOL_GPL(lpe_block);
+
+
+int lpe_unblock(struct lpe_block_list *blocker)
+{
+	unsigned long flags;
+
+	if (blocker) {
+		spin_lock_irqsave(&lpe_lock, flags);
+		blocker->level = 0;
+		update_block_level();
+		spin_unlock_irqrestore(&lpe_lock, flags);
+		return 1;
+	} else {
+		WARN(true, "lpe_unblock abuse!\n");
+		dump_stack();
+	}
+	return -1;
+}
+EXPORT_SYMBOL_GPL(lpe_unblock);
+
+/*
+ * Drivers that think they are wake up events should
+ * register their event name.  event string is assumed
+ * to be handled by caller, unregistering the returned
+ * event will not free the name string.
+ */
+struct lpe_wake_event_list *register_wake_event(char *name)
+{
+	struct lpe_wake_event_list *waker;
+	unsigned long flags;
+
+	waker = kzalloc(sizeof(struct lpe_wake_event_list), GFP_KERNEL);
+	if (waker) {
+		waker->name = name;
+
+		spin_lock_irqsave(&lpe_lock, flags);
+		list_add(&waker->list,
+			&lpe_wakers.list);
+		spin_unlock_irqrestore(&lpe_lock, flags);
+	}
+
+	return waker;
+}
+EXPORT_SYMBOL_GPL(register_wake_event);
+
+int unregister_wake_event(struct lpe_wake_event_list *waker)
+{
+	unsigned long flags;
+
+	if (waker == NULL)
+		return -1;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	list_del(&waker->list);
+	kfree(waker);
+	spin_unlock_irqrestore(&lpe_lock, flags);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(unregister_wake_event);
+
+
+/*
+ * only set event if between point of no return of entry
+ * into low power state and exiting the lpe_ops->enter
+ * function
+ */
+void set_wake_event(struct lpe_wake_event_list *waker)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	if ((requested_lp_level < ACTIVE) &&
+		(current_lp_level == requested_lp_level)) {
+		waker->event = 1;	
+	}
+	spin_unlock_irqrestore(&lpe_lock, flags);
+}
+EXPORT_SYMBOL_GPL(set_wake_event);
+
+
+static void acknowledge_wake_events(void)
+{
+	struct lpe_wake_event_list *node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	list_for_each_entry(node,
+		&lpe_wakers.list, list) {
+			node->event = 0;
+	}
+	spin_unlock_irqrestore(&lpe_lock, flags);
+}
+
+
+static int unacknowledged_wake_event(void)
+{
+	struct lpe_wake_event_list *node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	list_for_each_entry(node,
+		&lpe_wakers.list, list) {
+			if (node->event) {
+				spin_unlock_irqrestore(&lpe_lock, flags);
+				return 1;
+			}
+	}
+	spin_unlock_irqrestore(&lpe_lock, flags);
+	return 0;
+}
+
+
+/*
+ * Call this from by pm_ops->enter or lp_ops->enter when entry
+ * into the requested lp mode is at the point of no return.
+ * This allows the wake event processing to work.
+ */
+void lpe_ponr(void)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	current_lp_level = requested_lp_level;
+	spin_unlock_irqrestore(&lpe_lock, flags);
+}
+EXPORT_SYMBOL_GPL(lpe_ponr);
+
+
+/*
+ * This allows the wake event processing to work.
+ */
+static void lpe_exit_lp_mode(void)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	current_lp_level = ACTIVE;
+	requested_lp_level = ACTIVE;
+	spin_unlock_irqrestore(&lpe_lock, flags);
+}
+
+
+/*
+ * User Mode ABI interface implementation
+ */
+
+static ssize_t lpe_enter_write(struct file *filp, const char __user *buf,
+		size_t count, loff_t *f_pos)
+{
+	s32 value;
+	int x;
+	char ascii_value[11];
+
+	if (count == sizeof(s32)) {
+		if (copy_from_user(&value, buf, sizeof(s32)))
+			return -EFAULT;
+	} else if (count == 11) { /* len('0x12345678/0') */
+		if (copy_from_user(ascii_value, buf, 11))
+			return -EFAULT;
+		x = sscanf(ascii_value, "%x", &value);
+		if (x != 1)
+			return -EINVAL;
+		pr_debug(KERN_ERR "%s, %d, 0x%x\n", ascii_value, x, value);
+	} else
+		return -EINVAL;
+
+	if (unacknowledged_wake_event())
+		return -EBUSY; /*blocked*/
+	
+	requested_lp_level = value;
+	if (requested_lp_level > lpe_critical_sections.level) {
+		lp_blocked = 1;
+		return -EBUSY; /*blocked*/
+	}
+	
+	/*
+	 * attempt state change
+	 */
+	if (ops.enter(value) < 0)
+	{
+		/*TODO: some more sensible error reporting;*/
+		return -1;
+	}
+	lpe_exit_lp_mode();
+
+	return count;
+}
+
+
+static ssize_t lpe_blocked_read(struct file *filp, char __user *buf,
+		size_t count, loff_t *f_pos)
+{
+	int err=0;
+	int len=count;
+
+	wait_event_interruptible(lpe_blocked_wq, lp_blocked!=0);
+	lp_blocked = 0;
+	
+	if (count > strlen("unblocked")) {
+		len = strlen("unblocked");
+		err = copy_to_user(buf,"unblocked", len);
+	}
+
+	return err ? -EFAULT : len;
+}
+
+
+static ssize_t lpe_wake_event_read(struct file *filp, char __user *buf,
+		size_t count, loff_t *f_pos)
+{
+	struct lpe_wake_event_list *node;
+	unsigned long flags;
+	int err;
+	size_t len=count;
+
+	spin_lock_irqsave(&lpe_lock, flags);
+	list_for_each_entry(node,
+		&lpe_wakers.list, list) {
+			if (node->event) {
+				err = copy_to_user(buf,node->name,
+					min(len,strlen(node->name)));
+				if (err) goto error;
+				buf += min(len,strlen(node->name)); 
+				len -= min(len,strlen(node->name)); 
+			}
+	}
+
+error:
+	spin_unlock_irqrestore(&lpe_lock, flags);
+	acknowledge_wake_events();
+	return err ? -EFAULT : count;
+}
+
+
+
+static const struct file_operations lpe_blocked_fops = {
+	.read = lpe_blocked_read,
+};
+
+struct miscdevice lpe_blocked = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "lpe_blocked",
+	.fops = &lpe_blocked_fops,
+};
+
+
+static const struct file_operations lpe_enter_fops = {
+	.write = lpe_enter_write,
+};
+
+struct miscdevice lpe_enter = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "lpe_enter",
+	.fops = &lpe_enter_fops,
+};
+
+static const struct file_operations lpe_wake_events_fops = {
+	.read = lpe_wake_event_read,
+};
+
+struct miscdevice lpe_wake_event = {
+	.minor = MISC_DYNAMIC_MINOR,
+	.name = "lpe_wake_event",
+	.fops = &lpe_wake_events_fops,
+};
+
+
+static int __init low_power_events_init(void)
+{
+	int ret = 0;
+
+	ret = misc_register(&lpe_wake_event);
+	if (ret < 0) {
+		printk(KERN_ERR "low_power_events: lpe_wake_event setup failed\n");
+		return ret;
+	}
+	ret = misc_register(&lpe_enter);
+	if (ret < 0) {
+		printk(KERN_ERR "low_power_events: lpe_enter setup failed\n");
+		return ret;
+	}
+	ret = misc_register(&lpe_blocked);
+	if (ret < 0) {
+		printk(KERN_ERR "low_power_events: lpe_blocked setup failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+late_initcall(low_power_events_init);
-- 
1.6.3.3

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Neil Brown]

On Sun, 30 May 2010 13:04:10 -0700
mark gross <640e9920@gmail.com> wrote:

> Low Power Events is a possible alternative to suspend blocker / wake
> lock API used by Android.

Here is how I see your proposal.  It is of course possible that I
misunderstood bits, so please correct me where I'm wrong.

1/ You have introduced a new mechanism for requesting a transition
  to a low power state.  This involves writing a number to /dev/lpe_enter.
  It is not clear to me from your text what the magic number really means.
  I think this parallels writing to /sys/power/state, but achieves the same
  result though a different mechanism and adds some extra checking.
  So: I don't understand the numbers, and I don't see why we need a
   second way to request a low power state.  Probably I missed something
   important.

2/ Rather than tracking wake-events from the hardware up through possibly
   several kernel modules, you go directly from hardware to user-space so each
   event is potentially presented to user-space twice: once as a "wake up
   from low power state" event and once following the normal path (maybe a
   key-press event, maybe a serial-port event, maybe a network receive event).
   I can see that this is a very tempting approach.  It allows all those
   intermediate modules to remain unchanged and that is good.
   However it isn't clear to me that this would be easy for user-space to use
   correctly.
   When an lpe event arrived it would need to wait around for the real event
   to arrive and then process that.  I probably wouldn't wait long, but it
   would be an indeterminate wait, and it might not be  trivial to determine
   if all events that would cause a wake-up have been consumed as a direct
   mapping from lpe event to normal event may not always be possible.
   Maybe this is more of a theoretical problem and in practice it would be
   easy to get it right - I don't have enough concrete experience to be sure.

   So: I like the idea of leaving the intermediate layers unchanged, but I'm
   not convinced it would work.

3/ You have created a new mechanism for passing events to user-space.  That
   seems unnecessary.  We already have the input subsystem which is pretty
   good at communication arbitrary events, and the events you are dealing
   with a very much like input events.

So I would suggest modifying your proposal to simply create a new 'input'
device.  Any driver that supports wake-from-suspend queues an event to that
device when a wakeup event occurs.  If the device is open and has any queued
events, then a suspend request such as 'echo mem > /sys/power/state' completes
without going into full suspend.

Then you just need to convince us that this mechanism can be used without any
race problems.  If it can, then it would certainly be a simple and
unobtrusive approach.

Thanks,
NeilBrown

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Florian Mickler]

On Mon, 31 May 2010 09:57:53 +1000
Neil Brown <neilb@suse.de> wrote:

> On Sun, 30 May 2010 13:04:10 -0700
> mark gross <640e9920@gmail.com> wrote:
> 
> > Low Power Events is a possible alternative to suspend blocker / wake
> > lock API used by Android.
> 
> Here is how I see your proposal.  It is of course possible that I
> misunderstood bits, so please correct me where I'm wrong.
> 
> 1/ You have introduced a new mechanism for requesting a transition
>   to a low power state.  This involves writing a number to /dev/lpe_enter.
>   It is not clear to me from your text what the magic number really means.
>   I think this parallels writing to /sys/power/state, but achieves the same
>   result though a different mechanism and adds some extra checking.
>   So: I don't understand the numbers, and I don't see why we need a
>    second way to request a low power state.  Probably I missed something
>    important.

I can only think for lpe to provide the levels and have userspace and
platform code hook into there. Else you would have a dependency from
userspace to platform code.

> 
> 2/ Rather than tracking wake-events from the hardware up through possibly
>    several kernel modules, you go directly from hardware to user-space so each
>    event is potentially presented to user-space twice: once as a "wake up
>    from low power state" event and once following the normal path (maybe a
>    key-press event, maybe a serial-port event, maybe a network receive event).
>    I can see that this is a very tempting approach.  It allows all those
>    intermediate modules to remain unchanged and that is good.
>    However it isn't clear to me that this would be easy for user-space to use
>    correctly.
>    When an lpe event arrived it would need to wait around for the real event
>    to arrive and then process that.  I probably wouldn't wait long, but it
>    would be an indeterminate wait, and it might not be  trivial to determine
>    if all events that would cause a wake-up have been consumed as a direct
>    mapping from lpe event to normal event may not always be possible.
>    Maybe this is more of a theoretical problem and in practice it would be
>    easy to get it right - I don't have enough concrete experience to be sure.
> 
>    So: I like the idea of leaving the intermediate layers unchanged, but I'm
>    not convinced it would work.

To add to this: Is it a correct assumption
that all wake-up events that leave a driver trickle eventually up to
userspace?

I think splitting the actual driver product (i.e. keypress or whatever)
of a wake-up-event and it's corresponding wake-lock is not possible.
Because you would have to _somehow_ map the block back to the product
when you consume the product. 

If you want to abstract the blocking from the kernel-code you probably
have to introduce an abstract "driver-product" entity where you can do
all your blocking associated with the product but hidden from the code
that uses the product. (Which I don't think is feasible, because it
increases overhead)

Or am I on the wrong track here? 

cheers,
Flo

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Florian Mickler]

On Mon, 31 May 2010 08:43:56 +0200
Florian Mickler <florian@mickler.org> wrote:

> On Mon, 31 May 2010 09:57:53 +1000
> Neil Brown <neilb@suse.de> wrote:
> 
> > 2/ Rather than tracking wake-events from the hardware up through possibly
> >    several kernel modules, you go directly from hardware to user-space so each
> >    event is potentially presented to user-space twice: once as a "wake up
> >    from low power state" event and once following the normal path (maybe a
> >    key-press event, maybe a serial-port event, maybe a network receive event).
> >    I can see that this is a very tempting approach.  It allows all those
> >    intermediate modules to remain unchanged and that is good.
> >    However it isn't clear to me that this would be easy for user-space to use
> >    correctly.
> >    When an lpe event arrived it would need to wait around for the real event
> >    to arrive and then process that.  I probably wouldn't wait long, but it
> >    would be an indeterminate wait, and it might not be  trivial to determine
> >    if all events that would cause a wake-up have been consumed as a direct
> >    mapping from lpe event to normal event may not always be possible.
> >    Maybe this is more of a theoretical problem and in practice it would be
> >    easy to get it right - I don't have enough concrete experience to be sure.
> > 
> >    So: I like the idea of leaving the intermediate layers unchanged, but I'm
> >    not convinced it would work.
> 
> To add to this: Is it a correct assumption
> that all wake-up events that leave a driver trickle eventually up to
> userspace?
> 
> I think splitting the actual driver product (i.e. keypress or whatever)
> of a wake-up-event and it's corresponding wake-lock is not possible.
> Because you would have to _somehow_ map the block back to the product
> when you consume the product. 
> 
> If you want to abstract the blocking from the kernel-code you probably
> have to introduce an abstract "driver-product" entity where you can do
> all your blocking associated with the product but hidden from the code
> that uses the product. (Which I don't think is feasible, because it
> increases overhead)
> 
> Or am I on the wrong track here? 

I just realized, that you can cancel lpe_blocks via delete_lpe_block(),
so this is not an issue at all.
They can be used just like suspend blockers. 

Also the mapping of lpe_block to "wake event" is the same problem as
with the suspend_blockers... 

So I don't think this is a bad idea after all. It decouples the
suspend_blockers from "suspend" quite nicely. 
Although it still is only "block" or "no block" and not, as was
suggested some sort of more fine grained requirement definition.

Cheers,
Flo

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Mon, May 31, 2010 at 10:05:45AM +0200, Florian Mickler wrote:
> On Mon, 31 May 2010 08:43:56 +0200
> Florian Mickler <florian@mickler.org> wrote:
> 
> > On Mon, 31 May 2010 09:57:53 +1000
> > Neil Brown <neilb@suse.de> wrote:
> > 
> > > 2/ Rather than tracking wake-events from the hardware up through possibly
> > >    several kernel modules, you go directly from hardware to user-space so each
> > >    event is potentially presented to user-space twice: once as a "wake up
> > >    from low power state" event and once following the normal path (maybe a
> > >    key-press event, maybe a serial-port event, maybe a network receive event).
> > >    I can see that this is a very tempting approach.  It allows all those
> > >    intermediate modules to remain unchanged and that is good.
> > >    However it isn't clear to me that this would be easy for user-space to use
> > >    correctly.
> > >    When an lpe event arrived it would need to wait around for the real event
> > >    to arrive and then process that.  I probably wouldn't wait long, but it
> > >    would be an indeterminate wait, and it might not be  trivial to determine
> > >    if all events that would cause a wake-up have been consumed as a direct
> > >    mapping from lpe event to normal event may not always be possible.
> > >    Maybe this is more of a theoretical problem and in practice it would be
> > >    easy to get it right - I don't have enough concrete experience to be sure.
> > > 
> > >    So: I like the idea of leaving the intermediate layers unchanged, but I'm
> > >    not convinced it would work.
> > 
> > To add to this: Is it a correct assumption
> > that all wake-up events that leave a driver trickle eventually up to
> > userspace?
> > 
> > I think splitting the actual driver product (i.e. keypress or whatever)
> > of a wake-up-event and it's corresponding wake-lock is not possible.
> > Because you would have to _somehow_ map the block back to the product
> > when you consume the product. 
> > 
> > If you want to abstract the blocking from the kernel-code you probably
> > have to introduce an abstract "driver-product" entity where you can do
> > all your blocking associated with the product but hidden from the code
> > that uses the product. (Which I don't think is feasible, because it
> > increases overhead)
> > 
> > Or am I on the wrong track here? 
> 
> I just realized, that you can cancel lpe_blocks via delete_lpe_block(),
> so this is not an issue at all.
> They can be used just like suspend blockers. 
> 
> Also the mapping of lpe_block to "wake event" is the same problem as
> with the suspend_blockers... 
> 
> So I don't think this is a bad idea after all. It decouples the
> suspend_blockers from "suspend" quite nicely. 
> Although it still is only "block" or "no block" and not, as was
> suggested some sort of more fine grained requirement definition.

I attempted to add level's of blocking but I honestly don't see a use
for the levels at the moment.  I'm sure there is some better
genralization possible.

--mgross

 
> Cheers,
> Flo

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Mon, May 31, 2010 at 08:43:56AM +0200, Florian Mickler wrote:
> On Mon, 31 May 2010 09:57:53 +1000
> Neil Brown <neilb@suse.de> wrote:
> 
> > On Sun, 30 May 2010 13:04:10 -0700
> > mark gross <640e9920@gmail.com> wrote:
> > 
> > > Low Power Events is a possible alternative to suspend blocker / wake
> > > lock API used by Android.
> > 
> > Here is how I see your proposal.  It is of course possible that I
> > misunderstood bits, so please correct me where I'm wrong.
> > 
> > 1/ You have introduced a new mechanism for requesting a transition
> >   to a low power state.  This involves writing a number to /dev/lpe_enter.
> >   It is not clear to me from your text what the magic number really means.
> >   I think this parallels writing to /sys/power/state, but achieves the same
> >   result though a different mechanism and adds some extra checking.
> >   So: I don't understand the numbers, and I don't see why we need a
> >    second way to request a low power state.  Probably I missed something
> >    important.
> 
> I can only think for lpe to provide the levels and have userspace and
> platform code hook into there. Else you would have a dependency from
> userspace to platform code.
> 
> > 
> > 2/ Rather than tracking wake-events from the hardware up through possibly
> >    several kernel modules, you go directly from hardware to user-space so each
> >    event is potentially presented to user-space twice: once as a "wake up
> >    from low power state" event and once following the normal path (maybe a
> >    key-press event, maybe a serial-port event, maybe a network receive event).
> >    I can see that this is a very tempting approach.  It allows all those
> >    intermediate modules to remain unchanged and that is good.
> >    However it isn't clear to me that this would be easy for user-space to use
> >    correctly.
> >    When an lpe event arrived it would need to wait around for the real event
> >    to arrive and then process that.  I probably wouldn't wait long, but it
> >    would be an indeterminate wait, and it might not be  trivial to determine
> >    if all events that would cause a wake-up have been consumed as a direct
> >    mapping from lpe event to normal event may not always be possible.
> >    Maybe this is more of a theoretical problem and in practice it would be
> >    easy to get it right - I don't have enough concrete experience to be sure.
> > 
> >    So: I like the idea of leaving the intermediate layers unchanged, but I'm
> >    not convinced it would work.
> 
> To add to this: Is it a correct assumption
> that all wake-up events that leave a driver trickle eventually up to
> userspace?
> 
> I think splitting the actual driver product (i.e. keypress or whatever)
> of a wake-up-event and it's corresponding wake-lock is not possible.
> Because you would have to _somehow_ map the block back to the product
> when you consume the product. 

its only possible if you require a user mode transition after a wake up
before re-entry into the suspend / low power state.

I think kernel mode initiated suspends are problematic.
 
> If you want to abstract the blocking from the kernel-code you probably
> have to introduce an abstract "driver-product" entity where you can do
> all your blocking associated with the product but hidden from the code
> that uses the product. (Which I don't think is feasible, because it
> increases overhead)
> 
> Or am I on the wrong track here? 
maybe a little. 

blocking low power entry and the handling of the unblocking as an event
is orthogonal to wake event notification and handling and the coupling
of the next entry into the low power state..  Much of these
discussions are conflating these two notions.

first blocking from the kernel mode *is* needed.  My poster child for
this is the moorestown OTG hardware.  If we attempt a suspend while
attached to a host the device hangs.  there is a real need for blocking
suspend attempts from kernel mode.  How to handle the event of
unblocking is one of the two main issues.  (the other being wake event
handling.)


> cheers,
> Flo

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
> On Sun, 30 May 2010 13:04:10 -0700
> mark gross <640e9920@gmail.com> wrote:
> 
> > Low Power Events is a possible alternative to suspend blocker / wake
> > lock API used by Android.
> 
> Here is how I see your proposal.  It is of course possible that I
> misunderstood bits, so please correct me where I'm wrong.
> 
> 1/ You have introduced a new mechanism for requesting a transition
>   to a low power state.  This involves writing a number to /dev/lpe_enter.
>   It is not clear to me from your text what the magic number really means.
>   I think this parallels writing to /sys/power/state, but achieves the same
>   result though a different mechanism and adds some extra checking.
>   So: I don't understand the numbers, and I don't see why we need a
>    second way to request a low power state.  Probably I missed something
>    important.

You are not missing much.  Those numbers where an attempt at
generalization for the level of suspend that could be requested by user
mode.  The semantics of what I'm talking about are different form
/sys/power/state even though they both enter low power states.

> 
> 2/ Rather than tracking wake-events from the hardware up through possibly
>    several kernel modules, you go directly from hardware to user-space so each
>    event is potentially presented to user-space twice: once as a "wake up
>    from low power state" event and once following the normal path (maybe a
>    key-press event, maybe a serial-port event, maybe a network receive event).
>    I can see that this is a very tempting approach.  It allows all those
>    intermediate modules to remain unchanged and that is good.
>    However it isn't clear to me that this would be easy for user-space to use
>    correctly.

yeah, I think it needs some prototyping too.

>    When an lpe event arrived it would need to wait around for the real event
>    to arrive and then process that.  I probably wouldn't wait long, but it
>    would be an indeterminate wait, and it might not be  trivial to determine
>    if all events that would cause a wake-up have been consumed as a direct
>    mapping from lpe event to normal event may not always be possible.
>    Maybe this is more of a theoretical problem and in practice it would be
>    easy to get it right - I don't have enough concrete experience to be sure.

You suggest below adding a input class for wakeups.  I think using that
would make sense.
 
>    So: I like the idea of leaving the intermediate layers unchanged, but I'm
>    not convinced it would work.
> 

I'm not fully convinced yet either, but I still think it could work.

> 3/ You have created a new mechanism for passing events to user-space.  That
>    seems unnecessary.  We already have the input subsystem which is pretty
>    good at communication arbitrary events, and the events you are dealing
>    with a very much like input events.

really good idea!
 
> So I would suggest modifying your proposal to simply create a new 'input'
> device.  Any driver that supports wake-from-suspend queues an event to that
> device when a wakeup event occurs.  If the device is open and has any queued
> events, then a suspend request such as 'echo mem > /sys/power/state' completes
> without going into full suspend.

/me likes.
 
> Then you just need to convince us that this mechanism can be used without any
> race problems.  If it can, then it would certainly be a simple and
> unobtrusive approach.

Lets find out.

--mgross

> 
> Thanks,
> NeilBrown

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Rafael J. Wysocki]

On Tuesday 01 June 2010, mark gross wrote:
> On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
...
> > So I would suggest modifying your proposal to simply create a new 'input'
> > device.  Any driver that supports wake-from-suspend queues an event to that
> > device when a wakeup event occurs.  If the device is open and has any queued
> > events, then a suspend request such as 'echo mem > /sys/power/state' completes
> > without going into full suspend.
> 
> /me likes.
>  
> > Then you just need to convince us that this mechanism can be used without any
> > race problems.  If it can, then it would certainly be a simple and
> > unobtrusive approach.
> 
> Lets find out.

Simple question: how is that better than the Alan Stern's proposed approach?

Rafael

Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
> On Tuesday 01 June 2010, mark gross wrote:
> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
> ...
> > > So I would suggest modifying your proposal to simply create a new 'input'
> > > device.  Any driver that supports wake-from-suspend queues an event to that
> > > device when a wakeup event occurs.  If the device is open and has any queued
> > > events, then a suspend request such as 'echo mem > /sys/power/state' completes
> > > without going into full suspend.
> > 
> > /me likes.
> >  
> > > Then you just need to convince us that this mechanism can be used without any
> > > race problems.  If it can, then it would certainly be a simple and
> > > unobtrusive approach.
> > 
> > Lets find out.
> 
> Simple question: how is that better than the Alan Stern's proposed approach?
>
I just saw Alan Stern's proposal, and have gotten some input form some
others.  I can't say my patch represents a better Idea than what Alan
proposed.  However; what Alan (and Thomas) are talking about is
effectively the same as the kenrel mode wakelock/suspend blocker thing,
and although it reuses existing infrastructure, it doesn't solve the
problem of needing overlapping blocking sections of code from ISR to
user mode.

(which to be fair, I seem to be the only one that feels thats a problem.
so perhaps I'm making that a bigger issue than it is...)

Also my current patch may be better in that it makes it explicit that we
need wake event notification up to user mode, where this Alan's leaves
it as an exercise for the integrator.

(wake events are not just key presses or lid events,  there is modem,
UI, network, usb, block device insertion, RTC and likely a few other
wake events that need to be handled effectively before re-suspending.

so to answer your question: I'm not sure.  I need to stub out what Alan
is proposing and hash it out with some folks.  I'm sorry I can't be
decisive on this yet.

I'll stub out a new pm_qos class for "qos_eventually" (perhaps with a
better name.) some modes to the input event mechanism to see how that
feels.


--mgross

Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Arve Hjønnevåg]

On Mon, May 31, 2010 at 10:09 PM, mark gross <640e9920@gmail.com> wrote:
> On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
>> On Tuesday 01 June 2010, mark gross wrote:
>> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
>> ...
>> > > So I would suggest modifying your proposal to simply create a new 'input'
>> > > device.  Any driver that supports wake-from-suspend queues an event to that
>> > > device when a wakeup event occurs.  If the device is open and has any queued
>> > > events, then a suspend request such as 'echo mem > /sys/power/state' completes
>> > > without going into full suspend.
>> >
>> > /me likes.
>> >
>> > > Then you just need to convince us that this mechanism can be used without any
>> > > race problems.  If it can, then it would certainly be a simple and
>> > > unobtrusive approach.
>> >
>> > Lets find out.
>>
>> Simple question: how is that better than the Alan Stern's proposed approach?
>>
> I just saw Alan Stern's proposal, and have gotten some input form some
> others.  I can't say my patch represents a better Idea than what Alan
> proposed.  However; what Alan (and Thomas) are talking about is
> effectively the same as the kenrel mode wakelock/suspend blocker thing,
> and although it reuses existing infrastructure, it doesn't solve the
> problem of needing overlapping blocking sections of code from ISR to
> user mode.
>

I don't think your solution solves this either.

-- 
Arve Hjønnevåg

Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Mon, May 31, 2010 at 10:24:30PM -0700, Arve Hjønnevåg wrote:
> On Mon, May 31, 2010 at 10:09 PM, mark gross <640e9920@gmail.com> wrote:
> > On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
> >> On Tuesday 01 June 2010, mark gross wrote:
> >> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
> >> ...
> >> > > So I would suggest modifying your proposal to simply create a new 'input'
> >> > > device.  Any driver that supports wake-from-suspend queues an event to that
> >> > > device when a wakeup event occurs.  If the device is open and has any queued
> >> > > events, then a suspend request such as 'echo mem > /sys/power/state' completes
> >> > > without going into full suspend.
> >> >
> >> > /me likes.
> >> >
> >> > > Then you just need to convince us that this mechanism can be used without any
> >> > > race problems.  If it can, then it would certainly be a simple and
> >> > > unobtrusive approach.
> >> >
> >> > Lets find out.
> >>
> >> Simple question: how is that better than the Alan Stern's proposed approach?
> >>
> > I just saw Alan Stern's proposal, and have gotten some input form some
> > others.  I can't say my patch represents a better Idea than what Alan
> > proposed.  However; what Alan (and Thomas) are talking about is
> > effectively the same as the kenrel mode wakelock/suspend blocker thing,
> > and although it reuses existing infrastructure, it doesn't solve the
> > problem of needing overlapping blocking sections of code from ISR to
> > user mode.
> >
> 
> I don't think your solution solves this either.

Why?  my proposal effectively removes the overlapping kernel blocking
sections uppon wake up by forcing the user mode to ack the wake event
and re-issue the suspend request explicitly.  That pretty much solves
that problem.

We can talk about whether or not it can be used effectively with Android
user mode PM or not, which I still think it can, but I need to try the
mods to power.c.

--mgross

 
> -- 
> Arve Hjønnevåg

Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Arve Hjønnevåg]

2010/6/1 mark gross <640e9920@gmail.com>:
> On Mon, May 31, 2010 at 10:24:30PM -0700, Arve Hjønnevåg wrote:
>> On Mon, May 31, 2010 at 10:09 PM, mark gross <640e9920@gmail.com> wrote:
>> > On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
>> >> On Tuesday 01 June 2010, mark gross wrote:
>> >> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
>> >> ...
>> >> > > So I would suggest modifying your proposal to simply create a new 'input'
>> >> > > device.  Any driver that supports wake-from-suspend queues an event to that
>> >> > > device when a wakeup event occurs.  If the device is open and has any queued
>> >> > > events, then a suspend request such as 'echo mem > /sys/power/state' completes
>> >> > > without going into full suspend.
>> >> >
>> >> > /me likes.
>> >> >
>> >> > > Then you just need to convince us that this mechanism can be used without any
>> >> > > race problems.  If it can, then it would certainly be a simple and
>> >> > > unobtrusive approach.
>> >> >
>> >> > Lets find out.
>> >>
>> >> Simple question: how is that better than the Alan Stern's proposed approach?
>> >>
>> > I just saw Alan Stern's proposal, and have gotten some input form some
>> > others.  I can't say my patch represents a better Idea than what Alan
>> > proposed.  However; what Alan (and Thomas) are talking about is
>> > effectively the same as the kenrel mode wakelock/suspend blocker thing,
>> > and although it reuses existing infrastructure, it doesn't solve the
>> > problem of needing overlapping blocking sections of code from ISR to
>> > user mode.
>> >
>>
>> I don't think your solution solves this either.
>
> Why?  my proposal effectively removes the overlapping kernel blocking
> sections uppon wake up by forcing the user mode to ack the wake event
> and re-issue the suspend request explicitly.  That pretty much solves
> that problem.
>

How to you ack the wakeup event in a safe way. Another wakeup event
can come in after you decided to ack the last event. Also when the
user-space power manager reads that there was a wakeup event, how does
it know if the real event has been delivered to user-space, and if the
user-space code that consumed this event has had a chance to block
suspend?

> We can talk about whether or not it can be used effectively with Android
> user mode PM or not, which I still think it can, but I need to try the
> mods to power.c.
>


-- 
Arve Hjønnevåg

RE: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Gross, Mark]

>-----Original Message-----
>From: Arve Hjønnevåg [mailto:arve@android.com]
>Sent: Tuesday, June 01, 2010 8:10 PM
>To: markgross@thegnar.org
>Cc: Rafael J. Wysocki; Gross, Mark; Neil Brown; Brian Swetland; linux-
>kernel@vger.kernel.org; Arve@smtp1.linux-foundation.org; Thomas Gleixner;
>linux-pm@lists.linux-foundation.org; Alan Cox
>Subject: Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker
>user mode and kernel API
>
>2010/6/1 mark gross <640e9920@gmail.com>:
>> On Mon, May 31, 2010 at 10:24:30PM -0700, Arve Hjønnevåg wrote:
>>> On Mon, May 31, 2010 at 10:09 PM, mark gross <640e9920@gmail.com> wrote:
>>> > On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
>>> >> On Tuesday 01 June 2010, mark gross wrote:
>>> >> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
>>> >> ...
[mtg: ] snipping outlook bollixed formatting of history.

>approach?
>>> >>
>>> > I just saw Alan Stern's proposal, and have gotten some input form some
>>> > others.  I can't say my patch represents a better Idea than what Alan
>>> > proposed.  However; what Alan (and Thomas) are talking about is
>>> > effectively the same as the kenrel mode wakelock/suspend blocker thing,
>>> > and although it reuses existing infrastructure, it doesn't solve the
>>> > problem of needing overlapping blocking sections of code from ISR to
>>> > user mode.
>>> >
>>>
>>> I don't think your solution solves this either.
>>
>> Why?  my proposal effectively removes the overlapping kernel blocking
>> sections uppon wake up by forcing the user mode to ack the wake event
>> and re-issue the suspend request explicitly.  That pretty much solves
>> that problem.
>>
>
>How to you ack the wakeup event in a safe way. Another wakeup event
>can come in after you decided to ack the last event. Also when the
[mtg: ] um, by blocking?  Ok, I see your point here.  I need more care here.

How real is this race?  At some point you turn off interrupts before programming the wake up sources anyway, and between turning off interrupts and the programming of the wake up events on the way into the platform suspend you have the same race.

>user-space power manager reads that there was a wakeup event, how does
>it know if the real event has been delivered to user-space, and if the
>user-space code that consumed this event has had a chance to block
>suspend?
[mtg: ] I suppose the same way the uevent.c wake_lock handling does.  (that code grabs a 5 second timeout wake lock on key events, not pretty either...)  I think the idea of getting the wake events passing through the input layer could help with this.

--mgross

>
>> We can talk about whether or not it can be used effectively with Android
>> user mode PM or not, which I still think it can, but I need to try the
>> mods to power.c.
>>
>
>
>--
>Arve Hjønnevåg

Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Arve Hjønnevåg]

On Tue, Jun 1, 2010 at 8:24 PM, Gross, Mark <mark.gross@intel.com> wrote:
>
>
>>-----Original Message-----
>>From: Arve Hjønnevåg [mailto:arve@android.com]
>>Sent: Tuesday, June 01, 2010 8:10 PM
>>To: markgross@thegnar.org
>>Cc: Rafael J. Wysocki; Gross, Mark; Neil Brown; Brian Swetland; linux-
>>kernel@vger.kernel.org; Arve@smtp1.linux-foundation.org; Thomas Gleixner;
>>linux-pm@lists.linux-foundation.org; Alan Cox
>>Subject: Re: [linux-pm] [RFC] lp_events: an lternitive to suspend blocker
>>user mode and kernel API
>>
>>2010/6/1 mark gross <640e9920@gmail.com>:
>>> On Mon, May 31, 2010 at 10:24:30PM -0700, Arve Hjønnevåg wrote:
>>>> On Mon, May 31, 2010 at 10:09 PM, mark gross <640e9920@gmail.com> wrote:
>>>> > On Tue, Jun 01, 2010 at 12:45:21AM +0200, Rafael J. Wysocki wrote:
>>>> >> On Tuesday 01 June 2010, mark gross wrote:
>>>> >> > On Mon, May 31, 2010 at 09:57:53AM +1000, Neil Brown wrote:
>>>> >> ...
> [mtg: ] snipping outlook bollixed formatting of history.
>
>>approach?
>>>> >>
>>>> > I just saw Alan Stern's proposal, and have gotten some input form some
>>>> > others.  I can't say my patch represents a better Idea than what Alan
>>>> > proposed.  However; what Alan (and Thomas) are talking about is
>>>> > effectively the same as the kenrel mode wakelock/suspend blocker thing,
>>>> > and although it reuses existing infrastructure, it doesn't solve the
>>>> > problem of needing overlapping blocking sections of code from ISR to
>>>> > user mode.
>>>> >
>>>>
>>>> I don't think your solution solves this either.
>>>
>>> Why?  my proposal effectively removes the overlapping kernel blocking
>>> sections uppon wake up by forcing the user mode to ack the wake event
>>> and re-issue the suspend request explicitly.  That pretty much solves
>>> that problem.
>>>
>>
>>How to you ack the wakeup event in a safe way. Another wakeup event
>>can come in after you decided to ack the last event. Also when the
> [mtg: ] um, by blocking?  Ok, I see your point here.  I need more care here.
>
> How real is this race? At some point you turn off interrupts before programming the wake up sources anyway, and between turning off interrupts and the programming of the wake up events on the way into the platform suspend you have the same race.

This depends on the hardware but if you can enable the wakeup event
before you turn the interrupt off you don't have a race (msm works
this way). On other hardware the interrupt controller is the wakeup
source so by masking the interrupt during suspend instead or turning
it off altogether you also don't have a race.

>
>>user-space power manager reads that there was a wakeup event, how does
>>it know if the real event has been delivered to user-space, and if the
>>user-space code that consumed this event has had a chance to block
>>suspend?
> [mtg: ] I suppose the same way the uevent.c wake_lock handling does.  (that code grabs a 5 second timeout wake lock on key events, not pretty either...)  I think the idea of getting the wake events passing through the input layer could help with this.

The 5 second timeout on keyevents are there to avoid blocking suspend
forever if someone opens an input device and don't read from it. The
suspend blocker patchset uses an ioctl to enable the suspend blocker
so it does not have this timeout. Either way, the 5 second timeout is
not how the system normally goes back to sleep after a key event. I do
think timeouts are useful so a driver can keep the system awake for a
while after passing an event to a subsystem that has not been modified
to block suspend (tty, network etc) but I don't think we should force
subsystems that can easily use overlapping constraints (like input
events) to keep the system awake for longer than needed.

>
> --mgross
>
>>
>>> We can talk about whether or not it can be used effectively with Android
>>> user mode PM or not, which I still think it can, but I need to try the
>>> mods to power.c.
>>>
>>
>>
>>--
>>Arve Hjønnevåg
>



-- 
Arve Hjønnevåg

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[Arve Hjønnevåg]

On Sun, May 30, 2010 at 1:04 PM, mark gross <640e9920@gmail.com> wrote:
> Low Power Events is a possible alternative to suspend blocker / wake
> lock API used by Android.
>
> It provides comparable power state notification and kernel mode critical
> section definition.  It differs from suspend blocker in that:
> 1) it is a platform and device independent implementation.  Device
> specific code is register as lp_ops, similar to pm_ops.  Drivers use
> the platform independent functions.
>
How is this more platform independent than suspend blockers?

> 2) it forces a user mode transition coming out of a LP state.
> Notification of wake up sources go to the user mode process managing the
> lp states.  Notification of blocked LP state entry is through an
> error return, and notification of un-blocking is through a file node as
> well.
>
If you want a user mode transition for every resume, you can easily
get this with the opportunistic suspend patchset by sending an input
event (or other a custom event with that blocks suspend) from a
driver's resume hook.

> I think the change need to the Google Android user mode power handling
> code can be limited to changes to _only_
> "hardware/libhardware_legacy/power/power.c"
>
> This code is still just a prototype of the platform independent code,
> and I'm implementing it on Eclair for the Intel Moorestown platform this
> weekend.  I'll have patches to eclair and updated kernel patches that
> enable this sometime Monday, after I bring it up on my target device.
> Hopefully by the end of next week I think I'll have it working well with
> Android.
>
> At this time the following patch is only known to compile.  I send it to
> help with the discussion.
>
> FWTW I do work on Android at Intel and I think I can make this work.
> (well, today I do.)
>
> --mgross
>
> Draft kernel patch :
>
...
> +
> +/dev/lpe_wake_event:
> +reads from this returns a list of wake events that happened between entry
> +into the LP state and the return from the read of the low_power_enter device
> +node.  User mode is expected to do something with these events before
> +re-trying to enter low power mode.
> +

I don't think this will work very well. Multiple input events can be
pending at the same time. If you register a single wake-event for an
input device, you don't know if all the events have been read when you
try to clear this wake-event.


-- 
Arve Hjønnevåg

Re: [RFC] lp_events: an lternitive to suspend blocker user mode and kernel API

[mark gross]

On Mon, May 31, 2010 at 02:55:16AM -0700, Arve Hjønnevåg wrote:
> On Sun, May 30, 2010 at 1:04 PM, mark gross <640e9920@gmail.com> wrote:
> > Low Power Events is a possible alternative to suspend blocker / wake
> > lock API used by Android.
> >
> > It provides comparable power state notification and kernel mode critical
> > section definition.  It differs from suspend blocker in that:
> > 1) it is a platform and device independent implementation.  Device
> > specific code is register as lp_ops, similar to pm_ops.  Drivers use
> > the platform independent functions.
> >
> How is this more platform independent than suspend blockers?

You don't have to do "suspend to ram" for the low power mode and one
could could entertain something different form S2R.
 
> > 2) it forces a user mode transition coming out of a LP state.
> > Notification of wake up sources go to the user mode process managing the
> > lp states.  Notification of blocked LP state entry is through an
> > error return, and notification of un-blocking is through a file node as
> > well.
> >
> If you want a user mode transition for every resume, you can easily
> get this with the opportunistic suspend patchset by sending an input
> event (or other a custom event with that blocks suspend) from a
> driver's resume hook.

the input event thing looks like a way to go.
 
> > I think the change need to the Google Android user mode power handling
> > code can be limited to changes to _only_
> > "hardware/libhardware_legacy/power/power.c"
> >
> > This code is still just a prototype of the platform independent code,
> > and I'm implementing it on Eclair for the Intel Moorestown platform this
> > weekend.  I'll have patches to eclair and updated kernel patches that
> > enable this sometime Monday, after I bring it up on my target device.
> > Hopefully by the end of next week I think I'll have it working well with
> > Android.
> >
> > At this time the following patch is only known to compile.  I send it to
> > help with the discussion.
> >
> > FWTW I do work on Android at Intel and I think I can make this work.
> > (well, today I do.)
> >
> > --mgross
> >
> > Draft kernel patch :
> >
> ...
> > +
> > +/dev/lpe_wake_event:
> > +reads from this returns a list of wake events that happened between entry
> > +into the LP state and the return from the read of the low_power_enter device
> > +node.  User mode is expected to do something with these events before
> > +re-trying to enter low power mode.
> > +
> 
> I don't think this will work very well. Multiple input events can be
> pending at the same time. If you register a single wake-event for an
> input device, you don't know if all the events have been read when you
> try to clear this wake-event.
>
whats important, to me at least, is that uppon wakeup some entity in
user mode makes a choice on when to re-enter suspend.  

--mgross