pm_runtime_enable() in ->probe() (was: Re: [PATCH v3 0/9] PM / Domains: Fix race conditions during boot)

[Rafael J. Wysocki]

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:
> > On 24 October 2014 18:12, Kevin Hilman <khilman at kernel.org> wrote:

[cut]

> > 
> > 1)
> > It's bad practice to use pm_runtime_get_sync() in the ->probe() path,
> 
> Honestly, I'm no longer amused.
> 
> > 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.
> 
> > 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