On Tue, May 25, 2010 at 2:41 AM, Paul Walmsley <paul@xxxxxxxxx> wrote: > 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). > That means modifying all kernel code that would use suspend blockers to abort suspend by returning an error from suspend instead. How is this better than using suspend blockers? >> 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). > That is not how the system wakes up from suspend now. The monotonic clock stops, and timers are ignored. >> > 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 Yes a driver can abort, but the user space code that initiated suspend cannot. > 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. > Do you think this is simpler than using a suspend blocker? > ... > > 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. > Opportunistic suspend does not change the default behaviour either. Your proposal requires that all the drivers that would block suspend using a suspend blocker, now block suspend without the help of the suspend blocker api. > 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 Your proposal to "pause"processes by putting them in TASK_INTERRUPTIBLE state and stop their timers does not work. It is effectivly the same ase freezing them, since if you "paused" it while it was busy, a wakeup event cannot unpause it. > 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 -- Arve Hjønnevåg _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
