| Message ID | 1381399307-21534-3-git-send-email-ulf.hansson@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 10 Oct 2013, Ulf Hansson wrote: > Typically the runtime idle function is triggered after resume and > probe. Instead of immediately requesting the device to go into > in-active state we make use of the autosuspend, if we have enabled > it earlier. > > Cc: Len Brown <len.brown@intel.com> > Cc: Pavel Machek <pavel@ucw.cz> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Kevin Hilman <khilman@linaro.org> > Cc: Alan Stern <stern@rowland.harvard.edu> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > --- > drivers/mmc/core/bus.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c > index cdca8a7..7f0e900 100644 > --- a/drivers/mmc/core/bus.c > +++ b/drivers/mmc/core/bus.c > @@ -205,6 +205,12 @@ static int mmc_runtime_resume(struct device *dev) > > static int mmc_runtime_idle(struct device *dev) > { > + if (pm_runtime_autosuspend_used(dev)) { > + pm_runtime_mark_last_busy(dev); > + pm_runtime_autosuspend(dev); > + return -EBUSY; > + } > + > return 0; > } I would greatly prefer to see the core rpm_idle() routine changed instead. Currently the last line says: return retval ? retval : rpm_suspend(dev, rpmflags); The second argument to rpm_suspend should be rpmflags | RPM_AUTO. That way, if the runtime-idle callback returns 0, we will automatically do either a normal suspend or an autosuspend, whichever is appropriate. As an added benefit, with this change there's no need to add the pm_runtime_autosuspend_used() function. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10 October 2013 16:45, Alan Stern <stern@rowland.harvard.edu> wrote: > On Thu, 10 Oct 2013, Ulf Hansson wrote: > >> Typically the runtime idle function is triggered after resume and >> probe. Instead of immediately requesting the device to go into >> in-active state we make use of the autosuspend, if we have enabled >> it earlier. >> >> Cc: Len Brown <len.brown@intel.com> >> Cc: Pavel Machek <pavel@ucw.cz> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Kevin Hilman <khilman@linaro.org> >> Cc: Alan Stern <stern@rowland.harvard.edu> >> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> >> --- >> drivers/mmc/core/bus.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c >> index cdca8a7..7f0e900 100644 >> --- a/drivers/mmc/core/bus.c >> +++ b/drivers/mmc/core/bus.c >> @@ -205,6 +205,12 @@ static int mmc_runtime_resume(struct device *dev) >> >> static int mmc_runtime_idle(struct device *dev) >> { >> + if (pm_runtime_autosuspend_used(dev)) { >> + pm_runtime_mark_last_busy(dev); >> + pm_runtime_autosuspend(dev); >> + return -EBUSY; >> + } >> + >> return 0; >> } > Hi Alan, Thanks for response! > I would greatly prefer to see the core rpm_idle() routine changed > instead. Currently the last line says: > > return retval ? retval : rpm_suspend(dev, rpmflags); > > The second argument to rpm_suspend should be rpmflags | RPM_AUTO. That > way, if the runtime-idle callback returns 0, we will automatically do > either a normal suspend or an autosuspend, whichever is appropriate. I know you guys, Mika and Rafael had a discussion around this feature a few month ago. Were thinking of you sending this response. :-) At that time, Rafael suggested to go ahead using similar method as I implemented here, which is also what Mika proposed for drivers/i2c/busses/i2c-designware-platdrv.c, not sure he finally sent a formal patch though. Anyway, just wanted us all to be aligned on the best way forward. I am also in favour of fixing this in the runtime PM core. How do you think we shall handle drivers that not have the runtime_idle callbacks? I guess we want to use the RPM_AUTO flag here as well? > > As an added benefit, with this change there's no need to add the > pm_runtime_autosuspend_used() function. In my patch I am also checking if I should update the last busy mark, do you mean it should be okay to do this even if not needed? > > Alan Stern > Kind regards Ulf Hansson -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 10 Oct 2013, Ulf Hansson wrote: > Hi Alan, > > Thanks for response! You're welcome. > > I would greatly prefer to see the core rpm_idle() routine changed > > instead. Currently the last line says: > > > > return retval ? retval : rpm_suspend(dev, rpmflags); > > > > The second argument to rpm_suspend should be rpmflags | RPM_AUTO. That > > way, if the runtime-idle callback returns 0, we will automatically do > > either a normal suspend or an autosuspend, whichever is appropriate. > > I know you guys, Mika and Rafael had a discussion around this feature > a few month ago. Were thinking of you sending this response. :-) > > At that time, Rafael suggested to go ahead using similar method as I > implemented here, which is also what Mika proposed for > drivers/i2c/busses/i2c-designware-platdrv.c, not sure he finally sent > a formal patch though. Maybe Rafael wanted to avoid changing a core routine too close to the start of an upcoming merge window. At any rate, IMO it should be changed and now seems like a good time to try it. > Anyway, just wanted us all to be aligned on the best way forward. I am > also in favour of fixing this in the runtime PM core. How do you > think we shall handle drivers that not have the runtime_idle > callbacks? I guess we want to use the RPM_AUTO flag here as well? Currently, retval gets set to 0 before the callback is invoked. If there is no callback routine then retval will remain 0. I agree we want to treat this case the same as if the callback had returned 0. > > As an added benefit, with this change there's no need to add the > > pm_runtime_autosuspend_used() function. > > In my patch I am also checking if I should update the last busy mark, > do you mean it should be okay to do this even if not needed? If the use_autosuspend flag isn't set then the last_busy flag isn't used for anything, so updating it won't hurt. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, October 10, 2013 01:21:53 PM Alan Stern wrote: > On Thu, 10 Oct 2013, Ulf Hansson wrote: > > > Hi Alan, > > > > Thanks for response! > > You're welcome. > > > > I would greatly prefer to see the core rpm_idle() routine changed > > > instead. Currently the last line says: > > > > > > return retval ? retval : rpm_suspend(dev, rpmflags); > > > > > > The second argument to rpm_suspend should be rpmflags | RPM_AUTO. That > > > way, if the runtime-idle callback returns 0, we will automatically do > > > either a normal suspend or an autosuspend, whichever is appropriate. > > > > I know you guys, Mika and Rafael had a discussion around this feature > > a few month ago. Were thinking of you sending this response. :-) > > > > At that time, Rafael suggested to go ahead using similar method as I > > implemented here, which is also what Mika proposed for > > drivers/i2c/busses/i2c-designware-platdrv.c, not sure he finally sent > > a formal patch though. > > Maybe Rafael wanted to avoid changing a core routine too close to the > start of an upcoming merge window. At any rate, IMO it should be > changed and now seems like a good time to try it. I agree, so I'd say go for it (but please remember to update the docs). > > Anyway, just wanted us all to be aligned on the best way forward. I am > > also in favour of fixing this in the runtime PM core. How do you > > think we shall handle drivers that not have the runtime_idle > > callbacks? I guess we want to use the RPM_AUTO flag here as well? > > Currently, retval gets set to 0 before the callback is invoked. If > there is no callback routine then retval will remain 0. I agree we > want to treat this case the same as if the callback had returned 0. Yup. > > > As an added benefit, with this change there's no need to add the > > > pm_runtime_autosuspend_used() function. > > > > In my patch I am also checking if I should update the last busy mark, > > do you mean it should be okay to do this even if not needed? > > If the use_autosuspend flag isn't set then the last_busy flag isn't > used for anything, so updating it won't hurt. Agreed. Thanks!
diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c index cdca8a7..7f0e900 100644 --- a/drivers/mmc/core/bus.c +++ b/drivers/mmc/core/bus.c @@ -205,6 +205,12 @@ static int mmc_runtime_resume(struct device *dev) static int mmc_runtime_idle(struct device *dev) { + if (pm_runtime_autosuspend_used(dev)) { + pm_runtime_mark_last_busy(dev); + pm_runtime_autosuspend(dev); + return -EBUSY; + } + return 0; }
Typically the runtime idle function is triggered after resume and probe. Instead of immediately requesting the device to go into in-active state we make use of the autosuspend, if we have enabled it earlier. Cc: Len Brown <len.brown@intel.com> Cc: Pavel Machek <pavel@ucw.cz> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Kevin Hilman <khilman@linaro.org> Cc: Alan Stern <stern@rowland.harvard.edu> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: linux-pm@vger.kernel.org Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> --- drivers/mmc/core/bus.c | 6 ++++++ 1 file changed, 6 insertions(+)