Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210505110915.6861-1-tony@atomide.com>
In-Reply-To: <20210505110915.6861-1-tony@atomide.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 7 May 2021 14:03:57 +0200
Message-ID: <CAPDyKFp36huF2Gu19T+KvUm90xSsd97VDCxst1KT+Qf0F5Vm5g@mail.gmail.com>
Subject: Re: [PATCH] PM: runtime: Fix unpaired parent child_count for force_resume
To:     Tony Lindgren <tony@atomide.com>
Cc:     "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
        Linux PM <linux-pm@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Sebastian Reichel <sebastian.reichel@collabora.com>,
        Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, 5 May 2021 at 13:09, Tony Lindgren <tony@atomide.com> wrote:
>
> As pm_runtime_need_not_resume() relies also on usage_count, it can return
> a different value in pm_runtime_force_suspend() compared to when called in
> pm_runtime_force_resume(). Different return values can happen if anything
> calls PM runtime functions in between, and causes the parent child_count
> to increase on every resume.
>
> So far I've seen the issue only for omapdrm that does complicated things
> with PM runtime calls during system suspend for legacy reasons:
>
> omap_atomic_commit_tail() for omapdrm.0
>  dispc_runtime_get()
>   wakes up 58000000.dss as it's the dispc parent
>    dispc_runtime_resume()
>     rpm_resume() increases parent child_count
>  dispc_runtime_put() won't idle, PM runtime suspend blocked
> pm_runtime_force_suspend() for 58000000.dss, !pm_runtime_need_not_resume()
>  __update_runtime_status()
> system suspended
> pm_runtime_force_resume() for 58000000.dss, pm_runtime_need_not_resume()
>  pm_runtime_enable() only called because of pm_runtime_need_not_resume()
> omap_atomic_commit_tail() for omapdrm.0
>  dispc_runtime_get()
>   wakes up 58000000.dss as it's the dispc parent
>    dispc_runtime_resume()
>     rpm_resume() increases parent child_count
>  dispc_runtime_put() won't idle, PM runtime suspend blocked
> ...
> rpm_suspend for 58000000.dss but parent child_count is now unbalanced
>
> Let's fix the issue by adding a flag for needs_force_resume and use it in
> pm_runtime_force_resume() instead of pm_runtime_need_not_resume().

Thanks for sharing the details, much appreciated.

>
> Additionally omapdrm system suspend could be simplified later on to avoid
> lots of unnecessary PM runtime calls and the complexity it adds. The
> driver can just use internal functions that are shared between the PM
> runtime and system suspend related functions.
>
> Fixes: 4918e1f87c5f ("PM / runtime: Rework pm_runtime_force_suspend/resume()")

Actually, I think the problem has been there from the beginning
(unless I am mistaken), when we introduced the functions. So maybe the
fixes tag isn't entirely correct.

Although, I certainly think we should tag this for stable kernels.

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
>  drivers/base/power/runtime.c | 10 +++++++---
>  include/linux/pm.h           |  1 +
>  2 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1637,6 +1637,7 @@ void pm_runtime_init(struct device *dev)
>         dev->power.request_pending = false;
>         dev->power.request = RPM_REQ_NONE;
>         dev->power.deferred_resume = false;
> +       dev->power.needs_force_resume = 0;
>         INIT_WORK(&dev->power.work, pm_runtime_work);
>
>         dev->power.timer_expires = 0;
> @@ -1804,10 +1805,12 @@ int pm_runtime_force_suspend(struct device *dev)
>          * its parent, but set its status to RPM_SUSPENDED anyway in case this
>          * function will be called again for it in the meantime.
>          */
> -       if (pm_runtime_need_not_resume(dev))
> +       if (pm_runtime_need_not_resume(dev)) {
>                 pm_runtime_set_suspended(dev);
> -       else
> +       } else {
>                 __update_runtime_status(dev, RPM_SUSPENDED);
> +               dev->power.needs_force_resume = 1;
> +       }
>
>         return 0;
>
> @@ -1834,7 +1837,7 @@ int pm_runtime_force_resume(struct device *dev)
>         int (*callback)(struct device *);
>         int ret = 0;
>
> -       if (!pm_runtime_status_suspended(dev) || pm_runtime_need_not_resume(dev))
> +       if (!pm_runtime_status_suspended(dev) || !dev->power.needs_force_resume)
>                 goto out;
>
>         /*
> @@ -1853,6 +1856,7 @@ int pm_runtime_force_resume(struct device *dev)
>
>         pm_runtime_mark_last_busy(dev);
>  out:
> +       dev->power.needs_force_resume = 0;
>         pm_runtime_enable(dev);
>         return ret;
>  }
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -602,6 +602,7 @@ struct dev_pm_info {
>         unsigned int            idle_notification:1;
>         unsigned int            request_pending:1;
>         unsigned int            deferred_resume:1;
> +       unsigned int            needs_force_resume:1;
>         unsigned int            runtime_auto:1;
>         bool                    ignore_children:1;
>         unsigned int            no_callbacks:1;
> --
> 2.31.1