Re: [PATCH 0/8] Suspend block api (version 6)

["Arve Hjønnevåg" <arve@android.com>]

On Tue, May 25, 2010 at 2:41 AM, Paul Walmsley <paul@pwsan.com> 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@pwsan.com> wrote:
>> > On Fri, 14 May 2010, Arve Hjønnevåg wrote:
>> >> On Fri, May 14, 2010 at 1:27 PM, Paul Walmsley <paul@pwsan.com> 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