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