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. (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.
> > 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).
> > [Perhaps
> > pm_request_suspend() can return error code if use_suspend_delay is not set,
> > so that it's clear the caller knows what he's doing?]
That's not necessary; pm_request_suspend can be useful all by itself.
> > I'd also rename the above function to something like
> > pm_runtime_put_suspend() (frankly, I can't invent any good name right now).
>
> And this one might be called pm_runtime_put_autosuspend().
Sure.
Alan Stern
--
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
