On Friday, September 03, 2010, Alan Stern wrote:
> On Fri, 3 Sep 2010, Rafael J. Wysocki wrote:
>
> > > OK, so my opinion is that the way it modifies pm_schedule_suspend() is
> > > confusing. Especially this function looks awkward:
> > >
> > > +int pm_runtime_put_delay(struct device *dev)
> > > +{
> > > + int retval = 0;
> > > +
> > > + if (atomic_dec_and_test(&dev->power.usage_count))
> > > + retval = pm_schedule_suspend(dev, 0);
> > > + return retval;
> > > +}
> > >
> > > because the "delay" doesn't even appear here and there's the 0 passed
>
> Maybe adding a comment would make the reason for the "0" clearer.
>
> > > to pm_schedule_suspend() (besides, the pm_runtime_put_* family of functions
> > > calls _idle() rather than _suspend().
>
> The idle callbacks don't mix very well with delayed autosuspend.
> It's silly to have to go to process context in order to run an idle
> callback, just to find out whether or not an autosuspend timer should
> be started. Basically I assumed that a subsystem would use either the
> idle calls or the autosuspend calls but not both. (They _can_ both be
> used, but the result won't be as efficient -- there will be unnecessary
> workqueue activity.)
>
> That's why the _idle routines don't bother to check the autosuspend
> delay, and it's why this routine calls pm_schedule_suspend rather than
> pm_request_idle. (I would have called pm_request_suspend, but there is
> no such function, as you said. And if there were, it would be
> equivalent to pm_schedule_suspend(dev, 0) anyway. I suppose it could
> be added as an inline...)
>
> > > So, instead, I'd introduce pm_request_suspend() (we don't have that yet!) that
> >
> > Hmm. Perhaps it's better to call it pm_autosuspend() (the name would suggest
> > its purpose) and give it a second argument specifying the minimum delay with
> > respect to power.last_busy.
> >
> > In analogy, use_suspend_delay may be called use_autosuspend, because
> > it also implies last_busy will be taken into account.
>
> That's fine; I don't mind renaming things. But it's currently not
> possible to add a second argument specifying the minimum delay, because
> the delay has to be stored in power.suspend_delay for later use by
> pm_runtime_suspend_delay_expiration.
Hmm, does it change the value set by user space? It shouldn't do that.
> (I think that will be renamed to "pm_autosuspend_expiration".) If the value
> given by the driver were stored there, it would overwrite the value given by
> the user in sysfs.
>
> The alternative, as I suggested earlier, is to store _two_ delay values
> in dev->power: the value from sysfs and the value to actually use. I'm
> not in favor of this but I'll consider it if you want.
I think suspend_delay shouldn't be modified in any case, because it's what
user space provided us with and it has the right to expect that we won't change
that value. There may be an additional "current suspend delay" field used for
temporary storage.
Then, pm_autosuspend(dev, delay) may simply compare its delay argument with
suspend_delay (that can only be changed by user space) and use the greater
value to initialize the "current suspend delay" field.
Would that make sense?
> > > would take the user space provided delay into account _if_ use_suspend_delay
> > > is set. Then, the driver might either use the new pm_request_suspend(),
> > > or override suspend_delay using pm_schedule_suspend().
>
> In the current patch, suspend_delay can't be overridden. It can only
> be ignored (when use_suspend_delay isn't set).
I used wrong words, sorry. I wanted to say that pm_schedule_suspend() in its
current form may be useful too, even if one's going to use pm_autosuspend()
in general. Namely, pm_schedule_suspend() means "suspend 'delay' ms from
now and ignore last_busy", which may be useful in some cases.
Thanks,
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
