On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@xxxxxxxxxxx> wrote: > * Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> [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. > >> > pm_runtime_disable(host->dev); >> > /* NOTE: suspend callback never gets called unless >> > * pm_runtime_dont_use_autosuspend() is called >> > * before pm_runtime_put_sync() above. >> > */ >> > ... >> > >> > > > > Does pm_runtime_use_autosuspend() get called by the probe routine? If >> > > > > it does, then perhaps you can get what you want by having the probe >> > > > > routine call pm_runtime_dont_use_autosuspend() whenever it's about to >> > > > > return an error -- particularly -EDEFER. >> > > > >> > > > Yes so far that's the only fix that seems to work like I posted >> > > > earlier. But is that the right fix though? >> > > >> > > No, not really. Ideally you would leave autosuspend turned on. The >> > > delay would be long enough to that after -EDEFER, another probe would >> > > start before the delay expired. But shortly after the last probe >> > > attempt, the delay would expire and the device would then be put in low >> > > power. >> > >> > But then what about the new reinit function? To me it seems that >> > we should not attempt to maintain a state from the earlier failed >> > probe. Or are you thinking we just skip the reinit if autosuspend >> > is set? >> >> 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 _is_ what you want, isn't it? The alternative is to leave >> dev->power.rpm_status set to RPM_ACTIVE, to match the hardware's actual >> state. > > As far as I can tell things work just fine if the failed probe > suspend the device at the end of the failed probe. > >> Given that the reinit function is supposed to restore the initial >> settings, it probably ought to call pm_runtime_dont_use_autosuspend(). >> But that won't be enough to fix your problem. >> >> > > > If we wanted to have some generic fix, it seems we would have to pass >> > > > a new flag in pm_runtime_put_sync() to ignore any autosuspend >> > > > configuration. But I don't know if that's what we want to or should >> > > > do though? >> > > >> > > I don't think so. >> > >> > So should we just establish a policy that pm_runtime_use_autosuspend() >> > needs to be paired with pm_runtime_dont_use_autosuspend() for >> > pm_runtime_put_sync() to work? >> >> Your understanding is slightly wrong. pm_runtime_put_sync() _does_ >> work -- that is, it does what it's supposed to do. The difficulty is >> that what it's supposed to do doesn't match what you think. >> >> 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 :) No, we shouldn't. As I said, the current behavior is actually well documented (in kerneldoc comments and elsewhere). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
