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

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

[Alan Stern]

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.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Although this probably isn't an important issue, it's a good idea to 
avoid doing a double read of a value that might change while we're 
looking at it.

I trust there is no problem in replacing the previous version of the 
patch with this version.



Index: usb-2.6/drivers/base/power/runtime.c
===================================================================
--- usb-2.6.orig/drivers/base/power/runtime.c
+++ usb-2.6/drivers/base/power/runtime.c
@@ -703,16 +703,25 @@ EXPORT_SYMBOL_GPL(pm_request_resume);
  * @dev: Device to handle.
  * @sync: If set and the device is suspended, resume it synchronously.
  *
- * Increment the usage count of the device and if it was zero previously,
- * resume it or submit a resume request for it, depending on the value of @sync.
+ * Increment the usage count of the device.  If @sync is set, resume the device
+ * and wait for the resume to complete.  Otherwise if the device is currently
+ * suspending or suspended, submit a resume request.
+ *
+ * If @sync is clear, the caller is responsible for synchronization.
  */
 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;
 }
 EXPORT_SYMBOL_GPL(__pm_runtime_get);
Index: usb-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- usb-2.6.orig/Documentation/power/runtime_pm.txt
+++ usb-2.6/Documentation/power/runtime_pm.txt
@@ -276,8 +276,9 @@ drivers/base/power/runtime.c and include
     - increment the device's usage counter
 
   int pm_runtime_get(struct device *dev);
-    - increment the device's usage counter, run pm_request_resume(dev) and
-      return its result
+    - increment the device's usage counter; if the device is currently
+      suspending or suspended then run pm_request_resume(dev) and return its
+      result
 
   int pm_runtime_get_sync(struct device *dev);
     - increment the device's usage counter, run pm_runtime_resume(dev) and

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

[Alan Stern]

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.

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?

Alan Stern

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

[Rafael J. Wysocki]

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

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

[Alan Stern]

On Thu, 3 Dec 2009, Rafael J. Wysocki wrote:

> > 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.

Okay.  I'll rewrite my own code to avoid calling it.

Alan Stern