On Saturday, November 01, 2014 02:08:57 AM Rafael J. Wysocki wrote: > 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@xxxxxxxxxx> 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. So we actually had pm_runtime_get_noresume() before the barrier, but then we dropped it due to complaints. > 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. I wonder, then, if we just need to do pm_runtime_get_sync() instead of the barrier and then pm_runtime_put() instead of pm_request_idle() in driver_probe_device()? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |