On Thursday 03 December 2009, Alan Stern wrote:
> On Wed, 2 Dec 2009, Alan Stern wrote:
>
> > This patch (as1308b) fixes __pm_runtime_get(). Currently the routine
> > will resume a device if the prior usage count was 0. But this isn't
> > right; thanks to pm_runtime_get_noresume() the usage count can be
> > positive even while the device is suspended.
> >
> > Now the routine always tries to carry out a resume when called
> > synchronously. When called asynchronously, it avoids the overhead of
> > an unnecessary spinlock acquisition by doing the resume only if the
> > device's state was SUSPENDING or SUSPENDED. Since the access to the
> > state is unprotected, be careful to read the value only once.
>
> ...
>
> > int __pm_runtime_get(struct device *dev, bool sync)
> > {
> > - int retval = 1;
> > + int retval = 0;
> >
> > - if (atomic_add_return(1, &dev->power.usage_count) == 1)
> > - retval = sync ? pm_runtime_resume(dev) : pm_request_resume(dev);
> > + atomic_inc(&dev->power.usage_count);
> > + if (sync) {
> > + retval = pm_runtime_resume(dev);
> > + } else {
> > + enum rpm_status s = ACCESS_ONCE(dev->power.runtime_status);
> >
> > + if (s == RPM_SUSPENDING || s == RPM_SUSPENDED)
> > + retval = pm_request_resume(dev);
> > + }
> > return retval;
> > }
>
> I wonder whether this is really a good thing to do. It changes the
> semantics in the async case where the device is already active. The
> old code would cancel a pending or scheduled suspend request, whereas
> the new code will leave it alone.
I prefer the old behavior in that respect.
> My feeling was that an atomic routine would most likely do its work and
> then schedule a new suspend request before the old one expired, so it
> wouldn't matter if the old request wasn't cancelled. Still, some
> drivers might have their own preferences.
>
> Of course, this is just a convenient utility routine. Anybody can
> simply do
>
> pm_runtime_get_noresume(dev);
> switch (ACCESS_ONCE(dev->power.runtime_status)) {
> case RPM_SUSPENDING:
> case RPM_SUSPENDED:
> pm_request_resume(dev);
> default:
> }
>
> and obtain the same effect. So I don't know... Should
> pm_runtime_get() call pm_request_resume() always, or only when the
> state is SUSPENDING or SUSPENDED? Should we offer two routines and let
> people choose which they want?
I'd prefer to keep the current semantics, ie. drop the patch, at least for now.
I think it's reasonable to expect the users of pm_runtime_get_noresume() to
pay attention. ;-)
Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
