Messages in this thread | | | From | Ulf Hansson <> | Date | Mon, 5 Dec 2022 13:07:59 +0100 | Subject | Re: [PATCH v1 1/2] PM: runtime: Do not call __rpm_callback() from rpm_idle() |
| |
On Fri, 2 Dec 2022 at 15:32, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Calling __rpm_callback() from rpm_idle() after adding device links > support to the former is a clear mistake. > > Not only it causes rpm_idle() to carry out unnecessary actions, but it > is also against the assumption regarding the stability of PM-runtime > status accross __rpm_callback() invocations, because rpm_suspend() and > rpm_resume() may run in parallel with __rpm_callback() when it is called > by rpm_idle() and the device's PM-runtime status can be updated by any > of them.
Urgh, that's a nasty bug you are fixing here. Is there perhaps some links to some error reports that can make sense to include here?
> > Fixes: 21d5c57b3726 ("PM / runtime: Use device links") > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/base/power/runtime.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/base/power/runtime.c > =================================================================== > --- linux-pm.orig/drivers/base/power/runtime.c > +++ linux-pm/drivers/base/power/runtime.c > @@ -484,7 +484,17 @@ static int rpm_idle(struct device *dev, > > dev->power.idle_notification = true; > > - retval = __rpm_callback(callback, dev);
Couldn't we just extend __rpm_callback() to take another in-parameter, rather than open-coding the below?
Note that, __rpm_callback() already uses a "bool use_links" internal variable, that indicates whether the device links should be used or not.
> + if (dev->power.irq_safe) > + spin_unlock(&dev->power.lock); > + else > + spin_unlock_irq(&dev->power.lock); > + > + retval = callback(dev); > + > + if (dev->power.irq_safe) > + spin_lock(&dev->power.lock); > + else > + spin_lock_irq(&dev->power.lock); > > dev->power.idle_notification = false; > wake_up_all(&dev->power.wait_queue); > > >
Kind regards Uffe
| |