| Message ID | 20160202180555.GW19432@atomide.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2 Feb 2016, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [160202 08:50]: > > * Alan Stern <stern@rowland.harvard.edu> [160202 08:17]: > > > > > ? pm_runtime_put_sync() _already_ does not respect the autosuspend > > > mode. If you want to respect it, you have to call > > > pm_runtime_put_sync_autosuspend() instead. > > > > I think you found the real bug there. So the right fix is to > > call pm_runtime_put_sync_autosuspend() at the end of failed > > probe in omap_hsmmc. Let me give that a try here. > > Nope that's not it but getting closer. > > The following seems to make things behave for me. Now the > question is.. Does it have some undesired side effects? Yes, it does. I'm still not clear on what you want to accomplish. It sounds like you want to perform a runtime suspend following the last probe (if the probe fails), and in between probes you don't really care (although it would be preferable to avoid suspending). 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. > > Can we add some warning to pm_runtime_put_sync() about that? > > Probably no need for that, I misunderstood the meaning of > pm_runtime_put_sync_autosuspend(). > > Regards, > > Tony > > 8< -------------- > --- a/drivers/base/power/runtime.c > +++ b/drivers/base/power/runtime.c > @@ -435,7 +435,7 @@ static int rpm_suspend(struct device *dev, int rpmflags) > goto out; > > /* If the autosuspend_delay time hasn't expired yet, reschedule. */ > - if ((rpmflags & RPM_AUTO) > + if (((rpmflags & (RPM_ASYNC | RPM_AUTO)) == ((RPM_ASYNC | RPM_AUTO))) > && dev->power.runtime_status != RPM_SUSPENDING) { > unsigned long expires = pm_runtime_autosuspend_expiration(dev); This would prevent pm_runtime_autosuspend() from working correctly. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Tony Lindgren <tony@atomide.com> [160202 10:07]: > > The following seems to make things behave for me. Now the > question is.. Does it have some undesired side effects? The side effect is that it breaks PM runtime for devices during runtime.. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alan Stern <stern@rowland.harvard.edu> [160202 10:44]: > On Tue, 2 Feb 2016, Tony Lindgren wrote: > > > * Tony Lindgren <tony@atomide.com> [160202 08:50]: > > > * Alan Stern <stern@rowland.harvard.edu> [160202 08:17]: > > > > > > > ? pm_runtime_put_sync() _already_ does not respect the autosuspend > > > > mode. If you want to respect it, you have to call > > > > pm_runtime_put_sync_autosuspend() instead. > > > > > > I think you found the real bug there. So the right fix is to > > > call pm_runtime_put_sync_autosuspend() at the end of failed > > > probe in omap_hsmmc. Let me give that a try here. > > > > Nope that's not it but getting closer. > > > > The following seems to make things behave for me. Now the > > question is.. Does it have some undesired side effects? > > Yes, it does. Yeah noticed.. > I'm still not clear on what you want to accomplish. It sounds like you > want to perform a runtime suspend following the last probe (if the > probe fails), and in between probes you don't really care (although it > would be preferable to avoid suspending). I'd like to have pm_runtime_put_sync() disable the hardware after the initial failed probe. Currently that does not happen unless pm_runtime_dont_use_autosuspend() is called before pm_runtime_put_sync(). > 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? 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? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2 Feb 2016, Tony Lindgren wrote: > > I'm still not clear on what you want to accomplish. It sounds like you > > want to perform a runtime suspend following the last probe (if the > > probe fails), and in between probes you don't really care (although it > > would be preferable to avoid suspending). > > I'd like to have pm_runtime_put_sync() disable the hardware after > the initial failed probe. Currently that does not happen unless > pm_runtime_dont_use_autosuspend() is called before pm_runtime_put_sync(). pm_runtime_put_sync() doesn't do anything to the hardware if the usage count was > 1, because after the decrement it's still nonzero. Where is the particular call of pm_runtime_put_sync() that you're interested in, and what is the usage count when it runs? It's not at all unusual for the usage count to be > 1 during a probe. Also, what is autosuspend_delay set to for your device? And is runtime_auto set? > > 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. > 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alan Stern <stern@rowland.harvard.edu> [160202 11:17]: > On Tue, 2 Feb 2016, Tony Lindgren wrote: > > I'd like to have pm_runtime_put_sync() disable the hardware after > > the initial failed probe. Currently that does not happen unless > > pm_runtime_dont_use_autosuspend() is called before pm_runtime_put_sync(). > > pm_runtime_put_sync() doesn't do anything to the hardware if the usage > count was > 1, because after the decrement it's still nonzero. Where > is the particular call of pm_runtime_put_sync() that you're interested > in, and what is the usage count when it runs? It's not at all unusual > for the usage count to be > 1 during a probe. The usage count is 0 at that point, it seems the be the RPM_AUTO causing the issues that we set at the end of rpm_idle(). > 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); 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? > > 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? Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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. 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. 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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. > > 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 :) Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> 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. > >> > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > 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. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Rafael J. Wysocki <rafael@kernel.org> [160203 05:07]: > On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> wrote: > > > > 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). OK. I'll do a series of fixes to the drivers using omap_device. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> 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. FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync(). After all, the driver doesn't want to use autosuspend going forward, so stating that explicitly looks like the right thing to do. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Rafael J. Wysocki <rafael@kernel.org> [160203 09:19]: > On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> 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. > > FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync(). > > After all, the driver doesn't want to use autosuspend going forward, > so stating that explicitly looks like the right thing to do. Yeah agreed. FYI, this is what I typed up here into a commit message: 1. For sections of code that needs the device disabled, use pm_runtime_put_sync_suspend() if pm_runtime_set_autosuspend() has been set. 2. For driver exit code, use pm_runtime_dont_use_autosuspend() before pm_runtime_put_sync() if pm_runtime_use_autosuspend() has been set. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Feb 3, 2016 at 6:22 PM, Tony Lindgren <tony@atomide.com> wrote: > * Rafael J. Wysocki <rafael@kernel.org> [160203 09:19]: >> On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> 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. >> >> FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync(). >> >> After all, the driver doesn't want to use autosuspend going forward, >> so stating that explicitly looks like the right thing to do. > > Yeah agreed. FYI, this is what I typed up here into a commit message: > > 1. For sections of code that needs the device disabled, use > pm_runtime_put_sync_suspend() if pm_runtime_set_autosuspend() has > been set. > > 2. For driver exit code, use pm_runtime_dont_use_autosuspend() before > pm_runtime_put_sync() if pm_runtime_use_autosuspend() has been > set. Sounds reasonable to me. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 February 2016 at 18:18, Rafael J. Wysocki <rafael@kernel.org> wrote: > On Wed, Feb 3, 2016 at 12:46 AM, Tony Lindgren <tony@atomide.com> 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. > > FWIW, I'd call pm_runtime_dont_use_autosuspend() before put_sync(). > > After all, the driver doesn't want to use autosuspend going forward, > so stating that explicitly looks like the right thing to do. Just wanted to add yet some new findings while I was looking into this regression. So, the reason why pm_runtime_put_sync() doesn't idle the device in these cases, is because autosuspend is respected and for some reason the autosuspend timer hasn't expired. I was wondering *why* the timer isn't considered as expired, because the driver don't call pm_runtime_mark_last_busy() in the failing probe case... Then I realized the following commit was merged a few releases ago, which makes the runtime PM core to invoke pm_runtime_mark_last_busy() for us. commit 56f487c78015936097474fd89b2ccb229d500d0f Author: Tony Lindgren <tony@atomide.com> Date: Wed May 13 16:36:32 2015 -0700 PM / Runtime: Update last_busy in rpm_resume So, this commit actually causes the devices to stay active after a failed probe attempt. I think it's a good idea to revert this change, but what to you think? Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 4 Feb 2016, Ulf Hansson wrote: > Just wanted to add yet some new findings while I was looking into this > regression. > > So, the reason why pm_runtime_put_sync() doesn't idle the device in > these cases, is because autosuspend is respected and for some reason > the autosuspend timer hasn't expired. No doubt the autosuspend delay is longer than the time it takes for a probe to fail. > I was wondering *why* the timer isn't considered as expired, because > the driver don't call pm_runtime_mark_last_busy() in the failing probe > case... > > Then I realized the following commit was merged a few releases ago, > which makes the runtime PM core to invoke pm_runtime_mark_last_busy() > for us. > > commit 56f487c78015936097474fd89b2ccb229d500d0f > Author: Tony Lindgren <tony@atomide.com> > Date: Wed May 13 16:36:32 2015 -0700 > PM / Runtime: Update last_busy in rpm_resume > > So, this commit actually causes the devices to stay active after a > failed probe attempt. > > I think it's a good idea to revert this change, but what to you think? I disagree. The whole idea of autosuspend is to prevent the device's power state from bouncing up and down too rapidly. This implies that whenever the device gets resumed, we should wait at least the minimum autosuspend delay before allowing the next autosuspend. Perhaps you think that it's silly to behave that way in this case, because the device wasn't accessed at all during the time it was at full power. That's a valid objection, but the proper solution is not to revert the 56f487c78015 commit. Rather, change the driver to avoid doing a pm_runtime_resume_sync until you _know_ that the device will be accessed soon. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alan Stern <stern@rowland.harvard.edu> [160204 08:05]: > On Thu, 4 Feb 2016, Ulf Hansson wrote: > > > Just wanted to add yet some new findings while I was looking into this > > regression. > > > > So, the reason why pm_runtime_put_sync() doesn't idle the device in > > these cases, is because autosuspend is respected and for some reason > > the autosuspend timer hasn't expired. > > No doubt the autosuspend delay is longer than the time it takes for a > probe to fail. > > > I was wondering *why* the timer isn't considered as expired, because > > the driver don't call pm_runtime_mark_last_busy() in the failing probe > > case... > > > > Then I realized the following commit was merged a few releases ago, > > which makes the runtime PM core to invoke pm_runtime_mark_last_busy() > > for us. > > > > commit 56f487c78015936097474fd89b2ccb229d500d0f > > Author: Tony Lindgren <tony@atomide.com> > > Date: Wed May 13 16:36:32 2015 -0700 > > PM / Runtime: Update last_busy in rpm_resume > > > > So, this commit actually causes the devices to stay active after a > > failed probe attempt. > > > > I think it's a good idea to revert this change, but what to you think? > > I disagree. The whole idea of autosuspend is to prevent the device's > power state from bouncing up and down too rapidly. This implies that > whenever the device gets resumed, we should wait at least the minimum > autosuspend delay before allowing the next autosuspend. Yeah let's not revert 56f487c78015. Without that we have devices falling right back to sleep. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 February 2016 at 17:04, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 4 Feb 2016, Ulf Hansson wrote: > >> Just wanted to add yet some new findings while I was looking into this >> regression. >> >> So, the reason why pm_runtime_put_sync() doesn't idle the device in >> these cases, is because autosuspend is respected and for some reason >> the autosuspend timer hasn't expired. > > No doubt the autosuspend delay is longer than the time it takes for a > probe to fail. > >> I was wondering *why* the timer isn't considered as expired, because >> the driver don't call pm_runtime_mark_last_busy() in the failing probe >> case... >> >> Then I realized the following commit was merged a few releases ago, >> which makes the runtime PM core to invoke pm_runtime_mark_last_busy() >> for us. >> >> commit 56f487c78015936097474fd89b2ccb229d500d0f >> Author: Tony Lindgren <tony@atomide.com> >> Date: Wed May 13 16:36:32 2015 -0700 >> PM / Runtime: Update last_busy in rpm_resume >> >> So, this commit actually causes the devices to stay active after a >> failed probe attempt. >> >> I think it's a good idea to revert this change, but what to you think? > > I disagree. The whole idea of autosuspend is to prevent the device's > power state from bouncing up and down too rapidly. This implies that > whenever the device gets resumed, we should wait at least the minimum > autosuspend delay before allowing the next autosuspend. I am really not questioning the autosuspend feature at all, it's a really great feature! Now, I question the minor benefit we actually gain from having the runtime PM core to update the mark in rpm_resume(). To me, the best decision when to update the mark is know by the driver/subsystem for the device and not the core. In most cases the mark will be updated after a request has been completed, which leads to one unnecessary update at rpm_resume(). In this path (the resume), you really want to keep latencies to a minimum and for sure not do unnecessary things. > > Perhaps you think that it's silly to behave that way in this case, > because the device wasn't accessed at all during the time it was at > full power. That's a valid objection, but the proper solution is not > to revert the 56f487c78015 commit. Rather, change the driver to avoid > doing a pm_runtime_resume_sync until you _know_ that the device will be > accessed soon. That's not always going to work. Sometimes you need to access the device when trying to probe. Failing later in probe, shows just *one* case where it doesn't make sense to update the last busy mark. I suspect there may be other cases as well. Of course one can always use runtime PM APIs which overrides the autosuspend mode, so it's not a big deal. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 4 Feb 2016, Ulf Hansson wrote: > I am really not questioning the autosuspend feature at all, it's a > really great feature! > > Now, I question the minor benefit we actually gain from having the > runtime PM core to update the mark in rpm_resume(). As Tony pointed out, it prevents some devices from going to sleep right away. > To me, the best decision when to update the mark is know by the > driver/subsystem for the device and not the core. > > In most cases the mark will be updated after a request has been > completed, which leads to one unnecessary update at rpm_resume(). Sure, but that update is a simple assignment statement. It's about as cheap as you can get, short of doing nothing at all. > In this path (the resume), you really want to keep latencies to a > minimum and for sure not do unnecessary things. > > > > > Perhaps you think that it's silly to behave that way in this case, > > because the device wasn't accessed at all during the time it was at > > full power. That's a valid objection, but the proper solution is not > > to revert the 56f487c78015 commit. Rather, change the driver to avoid > > doing a pm_runtime_resume_sync until you _know_ that the device will be > > accessed soon. > > That's not always going to work. > > Sometimes you need to access the device when trying to probe. Failing > later in probe, shows just *one* case where it doesn't make sense to > update the last busy mark. I suspect there may be other cases as well. I don't follow your reasoning. If you don't update the last_busy mark then the probe fails, the device goes to sleep immediately, and then wakes up again a fraction of a second later for another probe attempt (if the error was -EDEFER). Thus you get an unnecessary suspend followed by an unnecessary resume. If the error was something other than -EDEFER and there will be no more probes, then yes -- the device remains at full power for longer than necessary. But how often does that happen? In general, people have drivers that _do_ work with their devices. > Of course one can always use runtime PM APIs which overrides the > autosuspend mode, so it's not a big deal. Or turn autosuspend off completely when you know you're not going to want it any more. True. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 February 2016 at 23:09, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 4 Feb 2016, Ulf Hansson wrote: > >> I am really not questioning the autosuspend feature at all, it's a >> really great feature! >> >> Now, I question the minor benefit we actually gain from having the >> runtime PM core to update the mark in rpm_resume(). > > As Tony pointed out, it prevents some devices from going to sleep right > away. Because their drivers don't care to update the last busy mark!? > >> To me, the best decision when to update the mark is know by the >> driver/subsystem for the device and not the core. >> >> In most cases the mark will be updated after a request has been >> completed, which leads to one unnecessary update at rpm_resume(). > > Sure, but that update is a simple assignment statement. It's about as > cheap as you can get, short of doing nothing at all. Valid point. I rest my case. :-) Thanks for the discussion. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ulf Hansson <ulf.hansson@linaro.org> [160204 14:35]: > On 4 February 2016 at 23:09, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Thu, 4 Feb 2016, Ulf Hansson wrote: > > > >> I am really not questioning the autosuspend feature at all, it's a > >> really great feature! > >> > >> Now, I question the minor benefit we actually gain from having the > >> runtime PM core to update the mark in rpm_resume(). > > > > As Tony pointed out, it prevents some devices from going to sleep right > > away. > > Because their drivers don't care to update the last busy mark!? Nope. Without that devices may never resume at all so the drivers can't do anything about it. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5 February 2016 at 02:08, Tony Lindgren <tony@atomide.com> wrote: > * Ulf Hansson <ulf.hansson@linaro.org> [160204 14:35]: >> On 4 February 2016 at 23:09, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Thu, 4 Feb 2016, Ulf Hansson wrote: >> > >> >> I am really not questioning the autosuspend feature at all, it's a >> >> really great feature! >> >> >> >> Now, I question the minor benefit we actually gain from having the >> >> runtime PM core to update the mark in rpm_resume(). >> > >> > As Tony pointed out, it prevents some devices from going to sleep right >> > away. >> >> Because their drivers don't care to update the last busy mark!? > > Nope. Without that devices may never resume at all so the drivers > can't do anything about it. I don't get it. Why not? Because of another abuse of the runtime PM API? Or we should probably continue to focus on fixing the regression. :-) Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Ulf Hansson <ulf.hansson@linaro.org> [160204 22:55]: > On 5 February 2016 at 02:08, Tony Lindgren <tony@atomide.com> wrote: > > * Ulf Hansson <ulf.hansson@linaro.org> [160204 14:35]: > >> On 4 February 2016 at 23:09, Alan Stern <stern@rowland.harvard.edu> wrote: > >> > On Thu, 4 Feb 2016, Ulf Hansson wrote: > >> > > >> >> I am really not questioning the autosuspend feature at all, it's a > >> >> really great feature! > >> >> > >> >> Now, I question the minor benefit we actually gain from having the > >> >> runtime PM core to update the mark in rpm_resume(). > >> > > >> > As Tony pointed out, it prevents some devices from going to sleep right > >> > away. > >> > >> Because their drivers don't care to update the last busy mark!? > > > > Nope. Without that devices may never resume at all so the drivers > > can't do anything about it. > > I don't get it. Why not? Because of another abuse of the runtime PM API? I think you should be able to test this case in your test driver by calling pm_runtime_resume() for your test driver after your test drive has autosuspended. Probably you need some delayed_work to do this in your test driver unless you have some test bus to go with it. > Or we should probably continue to focus on fixing the regression. :-) Naturally we should fix up things yeah :) Having a test driver that works on any architecture sure makes things easier to verify. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/drivers/base/power/runtime.c +++ b/drivers/base/power/runtime.c @@ -435,7 +435,7 @@ static int rpm_suspend(struct device *dev, int rpmflags) goto out; /* If the autosuspend_delay time hasn't expired yet, reschedule. */ - if ((rpmflags & RPM_AUTO) + if (((rpmflags & (RPM_ASYNC | RPM_AUTO)) == ((RPM_ASYNC | RPM_AUTO))) && dev->power.runtime_status != RPM_SUSPENDING) { unsigned long expires = pm_runtime_autosuspend_expiration(dev);