| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAJZ5v0iGZ+zk2MEzejGFz2sO07ywwZouP1W8O_C7nfiSTbGXuw@mail.gmail.com>
Date: Mon, 10 May 2021 19:16:33 +0200
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Ulf Hansson <ulf.hansson@...aro.org>,
Tony Lindgren <tony@...mide.com>
Cc: "Rafael J . Wysocki" <rafael.j.wysocki@...el.com>,
Linux PM <linux-pm@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Sebastian Reichel <sebastian.reichel@...labora.com>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
Subject: Re: [PATCH] PM: runtime: Fix unpaired parent child_count for force_resume
On Fri, May 7, 2021 at 2:04 PM Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Wed, 5 May 2021 at 13:09, Tony Lindgren <tony@...mide.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@...asonboard.com>
> > Cc: Sebastian Reichel <sebastian.reichel@...labora.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
> > Signed-off-by: Tony Lindgren <tony@...mide.com>
>
> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.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
Powered by blists - more mailing lists