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

["Bjørn Mork" <bjorn@mork.no>]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> 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.

Yup.

>> 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?

Is that going to cause similar problems? Hmm, I guess it is if the
callback takes so long that the timeout is reached when we get here?

In which cases do you *not* want to retry runtime suspend on -EBUSY from
the driver callback? If you don't retry, then the device will not
runtime suspend at all until the next time the driver updates last_busy.
And if the driver always is busy some time after such updates, then
runtime suspend is effectively disabled.

Should drivers just update last_busy more agressively, and make sure to
update it both when starting some hardware transaction and when
finishing it? That would make the suspend callback run again when the
hardware is not busy anymore, and would probably make stuff work without
any PM core changes.



Bjørn