Re: use of pm_runtime_disable() from driver probe?

[Kevin Hilman <khilman@ti.com>]

Alan Stern <stern@rowland.harvard.edu> writes:

> On Tue, 10 Jul 2012, Rafael J. Wysocki wrote:
>
>> > If .probe() enabled runtime PM and called pm_runtime_get_sync() (or _resume),
>> > it can't clean up properly in case of an error, because its
>> > pm_runtime_put_sync() (or _suspend) won't be effective and you're right that
>> > it has to call pm_runtime_disable().
>> > 
>> > So, we don't handle this particular case correctly.

Right, this is exactly the case I'm working on now.  The driver is
calling pm_runtime_enable() and _disable() in .probe().

>> > I'm not sure what the solution should be, though.  We could remove the
>> > runtime PM operations around really_probe(), but then there may be drivers
>> > assuming that the core will call pm_runtime_put_sync() after .probe()
>> > has returned.
>> 
>> I have an idea.
>> 
>> What about the following patch?  It shouldn't matter except for the cases when
>> .probe() attempts to suspend the device by itself.
>> 
>> ---
>>  drivers/base/dd.c |    7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>> 
>> Index: linux/drivers/base/dd.c
>> ===================================================================
>> --- linux.orig/drivers/base/dd.c
>> +++ linux/drivers/base/dd.c
>> @@ -356,10 +356,8 @@ int driver_probe_device(struct device_dr
>>  	pr_debug("bus: '%s': %s: matched device %s with driver %s\n",
>>  		 drv->bus->name, __func__, dev_name(dev), drv->name);
>>  
>> -	pm_runtime_get_noresume(dev);
>> -	pm_runtime_barrier(dev);
>>  	ret = really_probe(dev, drv);
>> -	pm_runtime_put_sync(dev);
>> +	pm_runtime_idle(dev);
>>  
>>  	return ret;
>>  }
>> @@ -406,9 +404,8 @@ int device_attach(struct device *dev)
>>  			ret = 0;
>>  		}
>>  	} else {
>> -		pm_runtime_get_noresume(dev);
>>  		ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
>> -		pm_runtime_put_sync(dev);
>> +		pm_runtime_idle(dev);
>>  	}
>>  out_unlock:
>>  	device_unlock(dev);
>
>
> This opens up the possibility of calling probe while a runtime resume
> or suspend is in progress.  (On the other hand, the existing code
> doesn't prevent a concurrent runtime resume.)  Maybe it would be best
> to leave the pm_runtime_barrier().
>
> Apart from that, I think it would solve the problem.

I just tested it and it definitely solves the problem.  I forgot exactly
why the get/put was added around probe in the first place, so am not
sure if this opens up other potential problems like Alan mentioned.

In any case, if you go forward with this patch, feel free to add:

Reviewed-by: Kevin Hilman <khilman@ti.com>
Tested-by: Kevin Hilman <khilman@ti.com>

Kevin