Message-ID: <CAPDyKFpC0xLfGLqyWUZwnSVdeY9Ojs-JUtPzEkRr=KAuSXDfmQ@mail.gmail.com>
Date:   Tue, 19 Dec 2017 08:38:18 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Kevin Hilman <khilman@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Mika Westerberg <mika.westerberg@...ux.intel.com>
Subject: Re: [PATCH 3/4] PM / core: Direct DPM_FLAG_SMART_SUSPEND optimization

On 10 December 2017 at 01:00, Rafael J. Wysocki <rjw@...ysocki.net> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
>
> Make the PM core avoid invoking the "late" and "noirq" system-wide
> suspend (or analogous) callbacks provided by device drivers directly
> for devices with DPM_FLAG_SMART_SUSPEND set that are in runtime
> suspend during the "late" and "noirq" phases of system-wide suspend
> (or analogous) transitions.  That is only done for devices without
> any middle-layer "late" and "noirq" suspend callbacks (to avoid
> confusing the middle layer if there is one).
>
> The underlying observation is that runtime PM is disabled for devices
> during the "late" and "noirq" system-wide suspend phases, so if they
> remain in runtime suspend from the "late" phase forward, it doesn't
> make sense to invoke the "late" and "noirq" callbacks provided by
> the drivers for them (arguably, the device is already suspended and
> in the right state).  Thus, if the remaining driver suspend callbacks
> are to be invoked directly by the core, they can be skipped.
>

As I have stated earlier, this isn't going to solve the general case,
as the above change log seems to state. I think we really need to do
that, before adding yet another system suspend/resume optimization
path in the PM core.

The main reason is that lots of drivers have additional operations to
perform, besides making sure that its device is put into a "runtime
suspended state" during system suspend. In other words, skipping
system suspend callbacks (and likewise for system resume) is to me the
wrong solution to mitigate these problems.

> This change really makes it possible for, say, platform device
> drivers to re-use runtime PM suspend and resume callbacks by
> pointing ->suspend_late and ->resume_early, respectively (and
> possibly the analogous hibernation-related callback pointers too),
> to them without adding any extra "is the device already suspended?"
> type of checks to the callback routines, as long as they will be
> invoked directly by the core.

Certainly there are drivers that could start to deploying this
solution, because at the moment they don't have any additional
operations to perform at system suspend. But what about the rest,
don't we care about them?

Moreover, what happens when/if a driver that has deployed this
solution, starts being used on a new SoC and then additional
operations during system suspend becomes required (for example
pinctrls that needs to be put in a system sleep state)? Then
everything falls apart, doesn't it?

Kind regards
Uffe

>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> ---
>  Documentation/driver-api/pm/devices.rst |   18 +++---
>  drivers/base/power/main.c               |   85 +++++++++++++++++++++++++++++---
>  2 files changed, 88 insertions(+), 15 deletions(-)
>
> Index: linux-pm/Documentation/driver-api/pm/devices.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/pm/devices.rst
> +++ linux-pm/Documentation/driver-api/pm/devices.rst
> @@ -777,14 +777,16 @@ The driver can indicate that by setting
>  runtime suspend at the beginning of the ``suspend_late`` phase of system-wide
>  suspend (or in the ``poweroff_late`` phase of hibernation), when runtime PM
>  has been disabled for it, under the assumption that its state should not change
> -after that point until the system-wide transition is over.  If that happens, the
> -driver's system-wide resume callbacks, if present, may still be invoked during
> -the subsequent system-wide resume transition and the device's runtime power
> -management status may be set to "active" before enabling runtime PM for it,
> -so the driver must be prepared to cope with the invocation of its system-wide
> -resume callbacks back-to-back with its ``->runtime_suspend`` one (without the
> -intervening ``->runtime_resume`` and so on) and the final state of the device
> -must reflect the "active" status for runtime PM in that case.
> +after that point until the system-wide transition is over (the PM core itself
> +does that for devices whose "noirq", "late" and "early" system-wide PM callbacks
> +are executed directly by it).  If that happens, the driver's system-wide resume
> +callbacks, if present, may still be invoked during the subsequent system-wide
> +resume transition and the device's runtime power management status may be set
> +to "active" before enabling runtime PM for it, so the driver must be prepared to
> +cope with the invocation of its system-wide resume callbacks back-to-back with
> +its ``->runtime_suspend`` one (without the intervening ``->runtime_resume`` and
> +so on) and the final state of the device must reflect the "active" runtime PM
> +status in that case.
>
>  During system-wide resume from a sleep state it's easiest to put devices into
>  the full-power state, as explained in :file:`Documentation/power/runtime_pm.txt`.
> Index: linux-pm/drivers/base/power/main.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/main.c
> +++ linux-pm/drivers/base/power/main.c
> @@ -541,6 +541,24 @@ void dev_pm_skip_next_resume_phases(stru
>  }
>
>  /**
> + * suspend_event - Return a "suspend" message for given "resume" one.
> + * @resume_msg: PM message representing a system-wide resume transition.
> + */
> +static pm_message_t suspend_event(pm_message_t resume_msg)
> +{
> +       switch (resume_msg.event) {
> +       case PM_EVENT_RESUME:
> +               return PMSG_SUSPEND;
> +       case PM_EVENT_THAW:
> +       case PM_EVENT_RESTORE:
> +               return PMSG_FREEZE;
> +       case PM_EVENT_RECOVER:
> +               return PMSG_HIBERNATE;
> +       }
> +       return PMSG_ON;
> +}
> +
> +/**
>   * dev_pm_may_skip_resume - System-wide device resume optimization check.
>   * @dev: Target device.
>   *
> @@ -581,6 +599,14 @@ static pm_callback_t dpm_subsys_resume_n
>         return callback;
>  }
>
> +static pm_callback_t dpm_subsys_suspend_noirq_cb(struct device *dev,
> +                                                pm_message_t state,
> +                                                const char **info_p);
> +
> +static pm_callback_t dpm_subsys_suspend_late_cb(struct device *dev,
> +                                               pm_message_t state,
> +                                               const char **info_p);
> +
>  /**
>   * device_resume_noirq - Execute a "noirq resume" callback for given device.
>   * @dev: Device to handle.
> @@ -608,13 +634,40 @@ static int device_resume_noirq(struct de
>         dpm_wait_for_superior(dev, async);
>
>         callback = dpm_subsys_resume_noirq_cb(dev, state, &info);
> +       if (callback)
> +               goto Run;
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       if (dev_pm_smart_suspend_and_suspended(dev)) {
> +               pm_message_t suspend_msg = suspend_event(state);
> +
> +               /*
> +                * If "freeze" callbacks have been skipped during a transition
> +                * related to hibernation, the subsequent "thaw" callbacks must
> +                * be skipped too or bad things may happen.  Otherwise, resume
> +                * callbacks are going to be run for the device, so its runtime
> +                * PM status must be changed to reflect the new state after the
> +                * transition under way.
> +                */
> +               if (!dpm_subsys_suspend_late_cb(dev, suspend_msg, NULL) &&
> +                   !dpm_subsys_suspend_noirq_cb(dev, suspend_msg, NULL)) {
> +                       if (state.event == PM_EVENT_THAW) {
> +                               dev_pm_skip_next_resume_phases(dev);
> +                               goto Skip;
> +                       } else {
> +                               pm_runtime_set_active(dev);
> +                       }
> +               }
> +       }
> +
> +       if (dev->driver && dev->driver->pm) {
>                 info = "noirq driver ";
>                 callback = pm_noirq_op(dev->driver->pm, state);
>         }
>
> +Run:
>         error = dpm_run_callback(callback, dev, state, info);
> +
> +Skip:
>         dev->power.is_noirq_suspended = false;
>
>         if (dev_pm_may_skip_resume(dev)) {
> @@ -629,7 +682,7 @@ static int device_resume_noirq(struct de
>                 dev_pm_skip_next_resume_phases(dev);
>         }
>
> - Out:
> +Out:
>         complete_all(&dev->power.completion);
>         TRACE_RESUME(error);
>         return error;
> @@ -1224,18 +1277,26 @@ static int __device_suspend_noirq(struct
>                 goto Complete;
>
>         callback = dpm_subsys_suspend_noirq_cb(dev, state, &info);
> +       if (callback)
> +               goto Run;
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       if (dev_pm_smart_suspend_and_suspended(dev) &&
> +           !dpm_subsys_suspend_late_cb(dev, state, NULL))
> +               goto Skip;
> +
> +       if (dev->driver && dev->driver->pm) {
>                 info = "noirq driver ";
>                 callback = pm_noirq_op(dev->driver->pm, state);
>         }
>
> +Run:
>         error = dpm_run_callback(callback, dev, state, info);
>         if (error) {
>                 async_error = error;
>                 goto Complete;
>         }
>
> +Skip:
>         dev->power.is_noirq_suspended = true;
>
>         if (dev_pm_test_driver_flags(dev, DPM_FLAG_LEAVE_SUSPENDED)) {
> @@ -1420,17 +1481,27 @@ static int __device_suspend_late(struct
>                 goto Complete;
>
>         callback = dpm_subsys_suspend_late_cb(dev, state, &info);
> +       if (callback)
> +               goto Run;
>
> -       if (!callback && dev->driver && dev->driver->pm) {
> +       if (dev_pm_smart_suspend_and_suspended(dev) &&
> +           !dpm_subsys_suspend_noirq_cb(dev, state, NULL))
> +               goto Skip;
> +
> +       if (dev->driver && dev->driver->pm) {
>                 info = "late driver ";
>                 callback = pm_late_early_op(dev->driver->pm, state);
>         }
>
> +Run:
>         error = dpm_run_callback(callback, dev, state, info);
> -       if (!error)
> -               dev->power.is_late_suspended = true;
> -       else
> +       if (error) {
>                 async_error = error;
> +               goto Complete;
> +       }
> +
> +Skip:
> +       dev->power.is_late_suspended = true;
>
>  Complete:
>         TRACE_SUSPEND(error);
>

Powered by blists - more mailing lists