On Saturday, November 01, 2014 01:20:38 AM Rafael J. Wysocki wrote:

On Friday, October 31, 2014 10:16:14 AM Ulf Hansson wrote:

quoted

On 24 October 2014 18:12, Kevin Hilman [off-list ref] wrote:

[cut]

quoted

1)
It's bad practice to use pm_runtime_get_sync() in the ->probe() path,

Honestly, I'm no longer amused.

quoted

to bring your resources to full power. The consequence would be a
driver that requires CONFIG_PM_RUNTIME to be even functional, which
just isn't acceptable.

Sorry, but this is utter nonsense.

CONFIG_PM_RUNTIME unset means "no runtime PM at all", so all drivers can expect
everything they need to be always on.  If that is not the case, then someone is
doing runtime PM behind the scenes and therefore cheating.  Or in different
words, for CONFIG_PM_RUNTIME unset bus types, platforms etc must ensure that
everything is on from the drivers' perspective.

If that is the case, then calling pm_runtime_get_sync() from ->probe
for CONFIG_PM_RUNTIME unset simply doesn't matter.

Now, for CONFIG_PM_RUNTIME enabled, if power domains are in use, doing
pm_runtime_get_sync() from ->probe is the only way the driver can ensure
in a non-racy way that the device will be accessible going forward.

Why?  Simply because the probing need not happen during system initialization.
It very well may take places when the other devices in the same domain have
beein in use for quite a while and have been using runtime PM (in which
case the domain may go off at any time unless it is explicityly prevented from
doing that).

One thing that you may be missing is that, for CONFIG_PM_RUNTIME set,
runtime PM has to be either enabled or disabled for all devices in one
domain (and if it is disabled, then the domain needs to be always on for
all practical purposes).  Otherwise you can't just make all of them happy
at the same time.  The documentation doesn't cover this, because it had been
written before we even started to consider power domains.

quoted

Drivers that behaves well within this context, follows the runtime PM
documentation/recommendation.

So please go to Documentation/power/runtime_pm.txt and read it again.  Quote:

"If the default initial runtime PM status of the device (i.e. 'suspended')
reflects the actual state of the device, its bus type's or its driver's
->probe() callback will likely need to wake it up using one of the PM core's
helper functions described in Section 4.  In that case, pm_runtime_resume()
should be used.  Of course, for this purpose the device's runtime PM has to be
enabled earlier by calling pm_runtime_enable()."

All that said there is a logical error related to calling pm_runtime_enable()
and its derivatives from ->probe() that I've been overlooking pretty much
from the start.

Namely, really_probe() sets dev->driver to drv before calling ->probe()
for either the bus type or the driver itself, so if the driver's probe
calls pm_runtime_enable(), it will execute the driver's own runtime PM
resume callback before the driver can check whether or not it wants to handle
the device in the first place.  That doesn't sound quite right to me.

This means we need a different mechanism to ensure that the device will
be accessible to the driver and/or the bus type in ->probe().

At one point we had pm_runtime_get_sync() before really_probe() in
driver_proble_device() IIRC, but people complained about it, so we dropped it
and put a barrier in there instead, but that's not sufficient.  We need to
explicitly ensure that the device won't be powered off while ->probe() is
in progress *but* we need to avoid calling the driver's runtime PM resume
callback before ->probe() returns.

Alan, Kevin, ideas?

Rafael