Messages in this thread | | | From | Doug Anderson <> | Date | Thu, 22 Sep 2022 12:46:08 -0700 | Subject | Re: [RFC PATCH 1/2] PM: runtime: Add pm_runtime_try_put_sync() and pm_runtime_try_get_sync() |
| |
Hi,
On Thu, Sep 22, 2022 at 10:38 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Sep 22, 2022 at 5:44 PM Douglas Anderson <dianders@chromium.org> wrote: > > > > In some cases, a caller may wish to synchronously get or put the PM > > Runtime state of a device but the caller may also be holding a > > resource that the runtime suspend or runtime resume of the device > > needs. Obviously this can lead to deadlock. > > > > A case in point is the clock framework. While running > > clk_disable_unused() the clock framework holds the global clock > > "prepare" lock. The clock framework then goes through and does PM > > Runtime actions. It should be no surprise to anyone that some devices > > need to prepare or unprepare clocks are part of their PM Runtime > > actions. Things generally work OK because of the "recursive" nature of > > the global clock "prepare" lock, but if we get unlucky and the PM > > Runtime action is happening in another task then we can end up > > deadlocking. > > > > Let's add a "try" version of the synchronous PM Runtime routines. > > This version will return -EINPROGRESS rather than waiting. To > > implement this we'll add a new flag: RPM_TRY. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > drivers/base/power/runtime.c | 7 +++++-- > > include/linux/pm_runtime.h | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 33 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c > > index 997be3ac20a7..67cc6a620b12 100644 > > --- a/drivers/base/power/runtime.c > > +++ b/drivers/base/power/runtime.c > > @@ -609,7 +609,7 @@ static int rpm_suspend(struct device *dev, int rpmflags) > > if (dev->power.runtime_status == RPM_SUSPENDING) { > > DEFINE_WAIT(wait); > > > > - if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) { > > + if (rpmflags & (RPM_ASYNC | RPM_NOWAIT | RPM_TRY)) { > > retval = -EINPROGRESS; > > goto out; > > } > > @@ -791,7 +791,10 @@ static int rpm_resume(struct device *dev, int rpmflags) > > || dev->power.runtime_status == RPM_SUSPENDING) { > > DEFINE_WAIT(wait); > > > > - if (rpmflags & (RPM_ASYNC | RPM_NOWAIT)) { > > + if (rpmflags & RPM_TRY) { > > + retval = -EINPROGRESS; > > Returning -EINPROGRESS from here may be misleading, because the device > may not be resuming. > > Besides, I'm not sure why a new flag is needed. What about using > RPM_NOWAIT instead?
Yeah, we can use RPM_NOWAIT if we land your patch [1]. I'll spin with that if folks agree that the overall approach taken in this series makes sense.
[1] https://lore.kernel.org/r/12079576.O9o76ZdvQC@kreacher
-Doug
| |