Messages in this thread Patch in this message |  | | Date | Thu, 27 Sep 2018 23:46:54 +0200 | From | Pavel Machek <> | Subject | Re: [PATCH] PM / core: Fix extra pm_runtime_enable on resume |
| |
On Tue 2018-09-25 18:10:55, Al Cooper wrote: > Matching pm_runtime_disable/pm_runtime_enable routines should be > called for "direct_complete" devices during suspend/resume and there > are cases where the pm_runtime_disable is skipped during suspend but > pm_runtime_enable is still called during resume. This is a problem > because the runtime enable state is really a counter and this can > incorrectly enable pm_runtime when it should not be enabled. This > happens for any direct_complete device doing an async suspend after > the global variable "async_error" is set (which is set by any sync > or async device's suspend error or early wake condition). > > This failure is very timing dependent but for testing and debug > the following changes will make it happen more frequently. > - Add an msleep(500) as the first line in async_suspend() in > drivers/base/power/main.c > - Modify alarmtimer_suspend in kernel/time/alarmtimer.c to just > return -EBUSY > > To see the failure condition that's been fixed with this patch, > enable dynamic debug for drivers/power/main.c and then run > "rtcwake -s 2 -m standby" and grep for > "skipping runtime enable during resume" messages.
Thanks for the patch...
Could / should we add some WARN_ONs to pm_runtime_{disable|enable} to catch stuff like this? Pavel
> Signed-off-by: Al Cooper <alcooperx@gmail.com> > --- > drivers/base/power/main.c | 21 +++++++++++++++++++-- > include/linux/pm.h | 1 + > 2 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c > index 3f68e2919dc5..2dc40662aae0 100644 > --- a/drivers/base/power/main.c > +++ b/drivers/base/power/main.c > @@ -945,7 +945,13 @@ static int device_resume(struct device *dev, pm_message_t state, bool async) > > if (dev->power.direct_complete) { > /* Match the pm_runtime_disable() in __device_suspend(). */ > - pm_runtime_enable(dev); > + if (dev->power.pm_runtime_disabled) { > + pm_runtime_enable(dev); > + dev->power.pm_runtime_disabled = false; > + } else { > + pm_dev_dbg(dev, state, > + "skipping runtime enable during "); > + } > goto Complete; > } > > @@ -1736,8 +1742,19 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async) > if (dev->power.direct_complete) { > if (pm_runtime_status_suspended(dev)) { > pm_runtime_disable(dev); > - if (pm_runtime_status_suspended(dev)) > + if (pm_runtime_status_suspended(dev)) { > + /* > + * If any device's sync or async suspend fails > + * and sets async_error, any async suspend for > + * direct_complete devices after the failure > + * will not execute the pm_runtime_disable > + * above. This flag lets the async device's > + * resume function (which is always run) know > + * if a matching pm_runtime_enable is needed. > + */ > + dev->power.pm_runtime_disabled = true; > goto Complete; > + } > > pm_runtime_enable(dev); > } > diff --git a/include/linux/pm.h b/include/linux/pm.h > index e723b78d8357..45738ad977fd 100644 > --- a/include/linux/pm.h > +++ b/include/linux/pm.h > @@ -593,6 +593,7 @@ struct dev_pm_info { > bool is_late_suspended:1; > bool early_init:1; /* Owned by the PM core */ > bool direct_complete:1; /* Owned by the PM core */ > + unsigned int pm_runtime_disabled:1; > u32 driver_flags; > spinlock_t lock; > #ifdef CONFIG_PM_SLEEP
-- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html [unhandled content-type:application/pgp-signature] |  |