Re: [PATCH] PM / core: Fix extra pm_runtime_enable on resume

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 3677 bytes --]

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]