Re: PM regression with commit 5de85b9d57ab PM runtime re-init in v4.5-rc1

[Tony Lindgren <tony@atomide.com>]

* Alan Stern <stern@rowland.harvard.edu> [160203 07:49]:
> On Tue, 2 Feb 2016, Tony Lindgren wrote:
> 
> > * Alan Stern <stern@rowland.harvard.edu> [160202 13:46]:
> > > On Tue, 2 Feb 2016, Tony Lindgren wrote:
> > > 
> > > > > Also, what is autosuspend_delay set to for your device?  And is 
> > > > > runtime_auto set?
> > > > 
> > > > It's 100 at that point, see the commented snippet below from
> > > > omap_hsmmc_probe():
> > > > 
> > > > 	pm_runtime_enable(host->dev);
> > > > 	pm_runtime_get_sync(host->dev);
> > > > 	pm_runtime_set_autosuspend_delay(host->dev, MMC_AUTOSUSPEND_DELAY);
> > > > 	/* NOTE: pm_runtime_dont_use_autosuspend(host->dev) needed here? */
> > > > 	pm_runtime_use_autosuspend(host->dev);
> > > > 	...
> > > > 	/* gets -EPROBE_DEFER */
> > > > err_irq:
> > > > 	...
> > > > 	pm_runtime_put_sync(host->dev);
> > > 
> > > You could try changing this to pm_runtime_put_sync_suspend().  But
> > > putting pm_runtime_dont_use_autosuspend() before the put_sync seems
> > > like a perfectly reasonable thing to do, especially if you feel you
> > > should reverse all the changes you made at the start.
> > 
> > They both seem to fix the problem.
> 
> So you could use either one.  In my opinion, the 
> pm_runtime_dont_use_autosuspend() solution is a little cleaner.
> 
> > > The reinit function gets called too late to do what you want -- namely, 
> > > put the hardware in a low-power state.
> > 
> > Right, the problem is the lack of suspend on the first probe. But
> > for having autosuspend timeout long enough for the next probe
> > would mean that we can't reset the PM runtime state in between.
> 
> That's one way to look at it.  But in principle you don't need to
> suspend the device after an unsuccessful probe.  You can just leave it
> at high power.  If this causes problems for a second probe, it's the 
> second probe's own fault for assuming the actual state matches the PM 
> status.

Yes. And I can improve the warning for omap_device so the authors
for new drivers can easily fix the issue.

> > > pm_runtime_put_sync() is supposed to follow the driver's wishes.  It
> > > invokes the driver's runtime_idle callback if there is one, and the
> > > callback routine can start a suspend or an autosuspend.  If there is no
> > > callback, it will use whatever autosuspend setting the driver has set
> > > up.  If you want to override the autosuspend setting, use
> > > pm_runtime_put_sync_suspend() instead.
> > 
> > Yes.. That works too. I guess the thing to consider is if we should
> > make pm_runtime_put_sync() always sync along the lines of a patch
> > I posted earlier today. That could avoid quite a bit of confusion
> > as already seen in this thread :)
> 
> As Rafael pointed out, pm_runtime_put_sync() has well-documented 
> behavior.  It shouldn't be changed.  I don't see how changing the 
> behavior would reduce anybody's confusion.  At least, anybody who reads 
> the documentation carefully.

Right :)

Tony