Hello Arve,
I regret the delay in responding.
On Mon, 17 May 2010, Arve Hjønnevåg wrote:
> On Mon, May 17, 2010 at 7:26 PM, Paul Walmsley <paul@xxxxxxxxx> wrote:
> > On Fri, 14 May 2010, Arve Hjønnevåg wrote:
> >> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@xxxxxxxxx> wrote:
> >> > On Mon, 3 May 2010, Arve Hjønnevåg wrote:
> >> >
> >> >> No, suspend blockers are mostly used to ensure wakeup events are not
> >> >> ignored, and to ensure tasks triggered by these wakeup events
> >> >> complete.
> >> >
> >> > Standard Linux systems don't need these,
> >>
> >> If you don't want to lose wakeup events they do. Standard Linux systems
> >> support suspend, but since they usually don't have a lot of wakeup
> >> events you don't run into a lot of problems.
> >
> > Sorry, I don't follow. What causes wakeup events to be lost? Is it the
> > current opportunistic suspend governor? On OMAP Linux systems, as far as
> > I know, we don't lose any wakeup events.
> >
> Have you used suspend?
I see. You are referring to wakeup event loss/delay caused by the full
system suspend process (e.g., kernel/power/suspend.c:enter_state()
onwards[1]), correct? This message addresses this at the end of the mail.
> >> > because the scheduler just keeps the system running as long as there
> >> > is work to be done.
> >>
> >> That is only true if you never use suspend.
> >
> > If, instead of the current Android opportunistic suspend governor, the
> > system entered suspend from pm_idle(), wouldn't that keep the system
> > running as long as there is work to done?
> >
> How do you know if the work being done while suspending is work that
> is needed to suspend or work that should abort suspend?
Consider a suspending system in which all "untrusted" processes have been
placed into TASK_INTERRUPTIBLE state and have had their timers
disabled.[2] The only remaining work to do on the system is work that
needs to be done as part of the suspend process, or work that is part of a
"trusted" process or the kernel. If the suspend needs to be aborted due
to the arrival of an event, this can be handled as part of the standard
suspend process (described below).
> When should the system wake up?
I suspect that I am misunderstanding your question, but the system should
wake up the same way it does now: when a wakeup interrupt arrives (either
from the next scheduled timer expiration, or from some external device).
> > As far as I can see, it's the current Android opportunistic suspend
> > governor design in patch 1 that causes the system to enter suspend even
> > when there is work to be done, since it will try to suspend even when the
> > system is out of the idle loop.
>
> It does not matter how you enter suspend. Without opportunistic suspend,
> once you tell the kernel that you want to suspend, you cannot abort.
Any driver should be able to abort suspend by returning an error from its
struct device_driver/struct dev_pm_ops suspend function. Consider this
partial callgraph for full system suspend, from [3], [4]:
kernel/power/suspend.c: enter_state() ->
kernel/power/suspend.c: suspend_devices_and_enter() ->
drivers/base/power/main.c: dpm_suspend_start() ->
drivers/base/power/main.c: dpm_suspend() ->
drivers/base/power/main.c: device_suspend() ->
drivers/base/power/main.c: __device_suspend() ->
drivers/base/power/main.c: pm_op() ->
drivers/base/power/main.c: ops->suspend()
If ops->suspend() returns an error, it's ultimately passed back to
suspend_devices_and_enter(), which aborts the suspend[5].
In this context, the main change that should be necessary is for
driver/subsystem suspend functions to return an error when there are
events pending. For example, as an alternative to the Android series'
evdev.c patch[6], the evdev.c suspend function could return an error such
as -EBUSY if events are currently queued (example below).
Aborting suspend in the event of driver activity seems like the right
thing to do in a system that attempts to enter full system suspend while
idle. But it is not the current expected Linux behavior, which also seems
useful. Any abort-suspend-if-busy behavior should be configurable. One
way would be to add a sysfs file for each driver/subsystem: perhaps
something like 'abort_suspend_if_busy'. The default value would be zero
to preserve the current behavior. Systems that wish to use opportunistic
suspend could write a one to some or all of those files.
An untested, purely illustrative example, based on evdev.c, is at the end
of this message. It's not meant to be functional; it's just meant to
demonstrate the basic idea in code form.
...
Like suspend-blockers, adding abort_suspend_if_busy support would require
changes throughout many drivers and subsystems. However, unlike
suspend-blockers, the above abort_suspend_if_busy approach would not
introduce any new APIs[7], nor would it change the default suspend
behavior of the system.
As an aside, it seems that Android shouldn't currently need this if it's
possible to pause "untrusted" processes and timers[8], since current
shipping Android hardware can enter the same system low power state both
with and without full system suspend[9]. But this could be useful for
non-Android systems that still wish to use an idle-loop based,
opportunistic full system suspend.
regards,
- Paul
1. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258
2. Section B4 of Paul Walmsley E-mail to the linux-pm mailing list,
dated Mon May 17 19:39:44 CDT 2010:
https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html
3. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l258
4. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/base/power/main.c;h=941fcb87e52a12765df2aaa2e2ab21267693af9f;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l1062
5. http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=kernel/power/suspend.c;h=56e7dbb8b996db295b4fc7cdf1b5f11a0409cb0c;hb=7e125f7b9cbfce4101191b8076d606c517a73066#l207
6. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Fri May 21
15:46:54 PDT 2010:
https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025747.html
7. Paul Walmsley E-mail to the linux-pm mailing list, dated Thu May 13
23:13:50 PDT 2010:
https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025524.html
8. Paul Walmsley E-mail to the linux-pm mailing list, dated Mon May 17
18:39:44 PDT 2010:
https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025663.html
9. Arve Hjønnevåg E-mail to the linux-pm mailing list, dated Mon, May 17
20:06:33 PDT 2010:
https://lists.linux-foundation.org/pipermail/linux-pm/2010-May/025668.html
--------------------------------------------------------------------------------------
evdev.c abort_suspend_if_idle experiment:
---
drivers/input/evdev.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 63 insertions(+), 1 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 2ee6c7a..23eaad1 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -20,8 +20,16 @@
#include <linux/input.h>
#include <linux/major.h>
#include <linux/device.h>
+#include <linux/pm.h>
#include "input-compat.h"
+/*
+ * abort_suspend_if_busy: set to 1 to prevent suspend as long as there
+ * is an event in the queue; set to 0 to allow suspend at any time.
+ * XXX Intended to be read from sysfs or the input subsystem.
+ */
+static int abort_suspend_if_busy;
+
struct evdev {
int exist;
int open;
@@ -118,6 +126,36 @@ static int evdev_flush(struct file *file, fl_owner_t id)
return retval;
}
+static int evdev_suspend(struct device *dev)
+{
+ struct evdev *evdev = container_of(dev, struct evdev, dev);
+ struct evdev_client *client;
+ int ret = 0;
+
+ if (!abort_suspend_if_busy)
+ return 0;
+
+ rcu_read_lock();
+
+ client = rcu_dereference(evdev->grab);
+ if (client) {
+ if (client->head != client->tail)
+ ret = -EBUSY;
+ } else {
+ list_for_each_entry_rcu(client, &evdev->client_list, node) {
+ if (client->head != client->tail) {
+ ret = -EBUSY;
+ break;
+ }
+ }
+ }
+
+ rcu_read_unlock();
+
+ return ret;
+}
+
+
static void evdev_free(struct device *dev)
{
struct evdev *evdev = container_of(dev, struct evdev, dev);
@@ -787,6 +825,21 @@ static void evdev_cleanup(struct evdev *evdev)
}
}
+static const struct dev_pm_ops evdev_pm_ops = {
+ .suspend = &evdev_suspend,
+};
+
+static char *evdev_devnode(struct device *dev, mode_t *mode)
+{
+ return kasprintf(GFP_KERNEL, "input/%s", dev_name(dev));
+}
+
+static struct class evdev_class = {
+ .name = "evdev",
+ .devnode = evdev_devnode,
+ .pm = &evdev_pm_ops,
+};
+
/*
* Create new evdev device. Note that input core serializes calls
* to connect and disconnect so we don't need to lock evdev_table here.
@@ -826,7 +879,7 @@ static int evdev_connect(struct input_handler *handler, struct input_dev *dev,
evdev->handle.private = evdev;
evdev->dev.devt = MKDEV(INPUT_MAJOR, EVDEV_MINOR_BASE + minor);
- evdev->dev.class = &input_class;
+ evdev->dev.class = &evdev_class;
evdev->dev.parent = &dev->dev;
evdev->dev.release = evdev_free;
device_initialize(&evdev->dev);
@@ -883,12 +936,21 @@ static struct input_handler evdev_handler = {
static int __init evdev_init(void)
{
+ int err;
+
+ err = class_register(&input_class);
+ if (err) {
+ printk(KERN_ERR "evdev: unable to register evdev class\n");
+ return err;
+ }
+
return input_register_handler(&evdev_handler);
}
static void __exit evdev_exit(void)
{
input_unregister_handler(&evdev_handler);
+ class_unregister(&evdev_class);
}
module_init(evdev_init);
--
1.7.1.GIT_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
