Re: [PATCH ver. 2] PM: allow for usage_count > 0 in pm_runtime_get()

["Rafael J. Wysocki" <rjw@sisk.pl>]

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