Re: [PATCH] PM / Domains: Don't skip driver's ->suspend|resume_noirq() callbacks

["Rafael J. Wysocki" <rafael@kernel.org>]

On Wed, Jan 10, 2018 at 9:31 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> The commit 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")
> started to respect driver's noirq callbacks, but while doing that it also
> introduced a few potential problems.
>
> More precisely, in genpd_finish_suspend() and genpd_resume_noirq() the
> noirq callbacks at the driver level should be invoked, no matter of whether
> dev->power.wakeup_path is set or not.
>
> Additionally, the commit in question also made genpd_resume_noirq() to
> ignore the return value from pm_runtime_force_resume().
>
> Let's fix both these issues!
>
> Fixes: 10da65423fdb ("PM / Domains: Call driver's noirq callbacks")
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 30 +++++++++++++++++-------------
>  1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index f9dcc98..48255ce 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1032,15 +1032,12 @@ static int genpd_prepare(struct device *dev)
>  static int genpd_finish_suspend(struct device *dev, bool poweroff)
>  {
>         struct generic_pm_domain *genpd;
> -       int ret;
> +       int ret = 0;
>
>         genpd = dev_to_genpd(dev);
>         if (IS_ERR(genpd))
>                 return -EINVAL;
>
> -       if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
> -               return 0;
> -
>         if (poweroff)
>                 ret = pm_generic_poweroff_noirq(dev);
>         else
> @@ -1048,10 +1045,18 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
>         if (ret)
>                 return ret;
>
> +       if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
> +               return 0;
> +
>         if (genpd->dev_ops.stop && genpd->dev_ops.start) {
>                 ret = pm_runtime_force_suspend(dev);

Good catch!

> -               if (ret)
> +               if (ret) {
> +                       if (poweroff)
> +                               pm_generic_restore_noirq(dev);
> +                       else
> +                               pm_generic_resume_noirq(dev);
>                         return ret;
> +               }
>         }
>
>         genpd_lock(genpd);
> @@ -1085,7 +1090,7 @@ static int genpd_suspend_noirq(struct device *dev)
>  static int genpd_resume_noirq(struct device *dev)
>  {
>         struct generic_pm_domain *genpd;
> -       int ret = 0;
> +       int ret;
>
>         dev_dbg(dev, "%s()\n", __func__);
>
> @@ -1094,21 +1099,20 @@ static int genpd_resume_noirq(struct device *dev)
>                 return -EINVAL;
>
>         if (dev->power.wakeup_path && genpd_is_active_wakeup(genpd))
> -               return 0;
> +               return pm_generic_resume_noirq(dev);
>
>         genpd_lock(genpd);
>         genpd_sync_power_on(genpd, true, 0);
>         genpd->suspended_count--;
>         genpd_unlock(genpd);
>
> -       if (genpd->dev_ops.stop && genpd->dev_ops.start)
> +       if (genpd->dev_ops.stop && genpd->dev_ops.start) {
>                 ret = pm_runtime_force_resume(dev);
> +               if (ret)
> +                       return ret;
> +       }
>
> -       ret = pm_generic_resume_noirq(dev);
> -       if (ret)
> -               return ret;
> -
> -       return ret;
> +       return pm_generic_resume_noirq(dev);
>  }
>
>  /**
> --

Looks OK to me overall.