From: "Rafael J. Wysocki" <rafael@kernel.org>
To: Ulf Hansson <ulf.hansson@linaro.org>, 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>
Subject: Re: [PATCH] PM: runtime: Fix unpaired parent child_count for force_resume
Date: Mon, 10 May 2021 19:16:33 +0200 [thread overview]
Message-ID: <CAJZ5v0iGZ+zk2MEzejGFz2sO07ywwZouP1W8O_C7nfiSTbGXuw@mail.gmail.com> (raw)
In-Reply-To: <CAPDyKFp36huF2Gu19T+KvUm90xSsd97VDCxst1KT+Qf0F5Vm5g@mail.gmail.com>
On Fri, May 7, 2021 at 2:04 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> 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.
It kind of make sense to point to the last commit that touched this
code and didn't address the issue.
> 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>
Applied as 5.13-rc material, thanks!
> > ---
> > 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
next prev parent reply other threads:[~2021-05-10 17:16 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-05 11:09 [PATCH] PM: runtime: Fix unpaired parent child_count for force_resume Tony Lindgren
2021-05-07 12:03 ` Ulf Hansson
2021-05-10 17:16 ` Rafael J. Wysocki [this message]
2021-05-07 13:23 ` Tomi Valkeinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJZ5v0iGZ+zk2MEzejGFz2sO07ywwZouP1W8O_C7nfiSTbGXuw@mail.gmail.com \
--to=rafael@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=sebastian.reichel@collabora.com \
--cc=tomi.valkeinen@ideasonboard.com \
--cc=tony@atomide.com \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).