On Tuesday, May 28, 2013 10:13:35 AM Alan Stern wrote: > On Tue, 28 May 2013, Rafael J. Wysocki wrote: > > > On Monday, May 13, 2013 01:50:18 PM Rafael J. Wysocki wrote: > > > On Monday, May 13, 2013 02:05:27 PM Mika Westerberg wrote: > > > > If the device is using autosuspend we should honor that and call > > > > pm_runtime_autosuspend() instead of pm_runtime_suspend(). Failing to do > > > > so > > > > causes the device to be suspended immediately even though it expects to > > > > be > > > > suspended only when the autosuspend delay is expired. > > > > > > > > Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com> > > > > --- > > > > drivers/base/power/generic_ops.c | 5 ++++- > > > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/base/power/generic_ops.c > > > > b/drivers/base/power/generic_ops.c > > > > index bfd898b..19786ca 100644 > > > > --- a/drivers/base/power/generic_ops.c > > > > +++ b/drivers/base/power/generic_ops.c > > > > @@ -29,7 +29,10 @@ int pm_generic_runtime_idle(struct device *dev) > > > > return ret; > > > > } > > > > > > > > - pm_runtime_suspend(dev); > > > > + if (dev->power.use_autosuspend) > > > > + pm_runtime_autosuspend(dev); > > > > + else > > > > + pm_runtime_suspend(dev); > > > > return 0; > > > > > > First of all, this is racy (power.use_autosuspend shoud be accessed under > > > power.lock). > > > > > > Second, this is not the only place we'd need to make this change (the > > > analogous > > > function for PCI is one example, but there may be others). > > > > > > Finally, I'm not sure how to address this problem in general. It may be > > > better > > > to simply modify rpm_idle() and remove pm_generic_runtime_idle() etc. > > > entirely. > > > > > > I'll have a look at that, thanks for pointing out the problem. > > > > I'm not sure if the core is the right place to address this, because it's > > not entirely clear if all drivers using autosuspend will have the same > > policy > > with respect to pm_runtime_idle() (i.e. to avoid suspending immediately if > > the suspend delay timer is active). > > > > In my opinion it'd be better to address that in the driver by adding a > > .runtime_idle() callback executing pm_runtime_autosuspend(dev) and returning > > -EBUSY. > > Remember that the return value from the runtime_idle callback is ignored.
It is ignored by the core, but some subsystems (those using pm_generic_runtime_idle() in particular) take it into account. > Are you suggesting that the PM core should start paying attention to it? In fact, I was pondering posting a patch making that change. :-) Perhaps I'll just post it for discussion later today ... Thanks, Rafael -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
