Re: [RFC] PM / Runtime: Make autosuspend_delay == 0 work as intended

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

Alan has to see this in the first place.

On Monday, June 02, 2014 12:29:12 PM Bjørn Mork wrote:
> OK, I don't like the result I came up with here.  So I am sending this
> as an RFC in the hope that smarter people than me can come up with
> something nicer. I hope the patch illustrates my problem:  While
> trying do debug some runtime suspend issues in a driver, I temporarily
> set autosuspend to 0 to deliberately make it aggressive enough for my
> driver to fail. I was surprised to learn than this effectively
> *disabled* autosuspend if the driver ever return -EBUSY from the
> runtime suspend callback.

That results from pm_runtime_autosuspend_expiration() always returning 0 in
that case.

> I believe that is too unexpected to be
> acceptable. Either we should not allow 0, or we should make it work
> at least as well as 1.
> 
> 
> Bjørn
> 8<--------------
> We should always reschedule if the driver runtime suspend callback
> returns -EBUSY and the driver updated the last_busy time.  But if
> autosuspend_delay is 0, then this cannot be detected because
> pm_runtime_autosuspend_expiration() always returns 0.
> 
> Fix by rescheduling if autosuspend_delay is 0.
> 
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  drivers/base/power/runtime.c |    8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 67c7938e430b..c52e630a2e54 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -421,6 +421,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  {
>  	int (*callback)(struct device *);
>  	struct device *parent = NULL;
> +	unsigned long retry_delay = 0;
>  	int retval;
>  
>  	trace_rpm_suspend(dev, rpmflags);
> @@ -441,7 +442,7 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  	/* If the autosuspend_delay time hasn't expired yet, reschedule. */
>  	if ((rpmflags & RPM_AUTO)
>  	    && dev->power.runtime_status != RPM_SUSPENDING) {
> -		unsigned long expires = pm_runtime_autosuspend_expiration(dev);
> +		unsigned long expires = pm_runtime_autosuspend_expiration(dev) + retry_delay;
>  
>  		if (expires != 0) {
>  			/* Pending requests need to be canceled. */
> @@ -571,8 +572,11 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  		 * reschedule another autosuspend.
>  		 */
>  		if ((rpmflags & RPM_AUTO) &&
> -		    pm_runtime_autosuspend_expiration(dev) != 0)
> +		    (pm_runtime_autosuspend_expiration(dev) != 0 ||
> +		     ACCESS_ONCE(dev->power.autosuspend_delay) == 0)) {
> +			retry_delay++;
>  			goto repeat;
> +		}
>  	} else {
>  		pm_runtime_cancel_pending(dev);
>  	}

Well, as I said.

Perhaps pm_runtime_autosuspend_expiration() could simply treat
power.autosuspend_delay equal to 0 in a special way, but then what about
power.autosuspend_delay equal to 1?

Rafael