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

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

[Alan Stern]

This patch (as1308) 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.

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

---

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,22 @@ 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;
-
-	if (atomic_add_return(1, &dev->power.usage_count) == 1)
-		retval = sync ? pm_runtime_resume(dev) : pm_request_resume(dev);
+	int retval = 0;
 
+	atomic_inc(&dev->power.usage_count);
+	if (sync)
+		retval = pm_runtime_resume(dev);
+	else if (dev->power.runtime_status == RPM_SUSPENDING ||
+	    dev->power.runtime_status == 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] PM: allow for usage_count > 0 in pm_runtime_get()

[Rafael J. Wysocki]

On Monday 30 November 2009, Alan Stern wrote:
> This patch (as1308) 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.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

All three patches applied to suspend-2.6/linux-next.  Do you want me to push
them to Linus before 2.6.32, or perhaps the $subject one only?

Rafael


> ---
> 
> 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,22 @@ 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;
> -
> -	if (atomic_add_return(1, &dev->power.usage_count) == 1)
> -		retval = sync ? pm_runtime_resume(dev) : pm_request_resume(dev);
> +	int retval = 0;
>  
> +	atomic_inc(&dev->power.usage_count);
> +	if (sync)
> +		retval = pm_runtime_resume(dev);
> +	else if (dev->power.runtime_status == RPM_SUSPENDING ||
> +	    dev->power.runtime_status == 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] PM: allow for usage_count > 0 in pm_runtime_get()

[Alan Stern]

On Mon, 30 Nov 2009, Rafael J. Wysocki wrote:

> On Monday 30 November 2009, Alan Stern wrote:
> > This patch (as1308) 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.
> > 
> > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> 
> All three patches applied to suspend-2.6/linux-next.  Do you want me to push
> them to Linus before 2.6.32, or perhaps the $subject one only?

Thanks.  The patches are not urgently needed; they can wait until the 
merge window.

BTW, I just noticed that the runtime_idle method is documented as 
returning void but defined in pm.h as returning int.  Obviously the 
definition is wrong, since no return value is ever used.  Would you 
like to fix it?

Alan Stern

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

[Rafael J. Wysocki]

On Monday 30 November 2009, Alan Stern wrote:
> On Mon, 30 Nov 2009, Rafael J. Wysocki wrote:
> 
> > On Monday 30 November 2009, Alan Stern wrote:
> > > This patch (as1308) 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.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > All three patches applied to suspend-2.6/linux-next.  Do you want me to push
> > them to Linus before 2.6.32, or perhaps the $subject one only?
> 
> Thanks.  The patches are not urgently needed; they can wait until the 
> merge window.
> 
> BTW, I just noticed that the runtime_idle method is documented as 
> returning void but defined in pm.h as returning int.  Obviously the 
> definition is wrong, since no return value is ever used.  Would you 
> like to fix it?

Yes, I'll fix it, thanks for chatching this!

Best,
Rafael

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

[Alan Stern]

Rafael:

One other thing I just noticed.  The power.timer_expires field is 
supposed to be nonzero when the timer is scheduled.  But the code at 
the end of pm_schedule_suspend() doesn't check for the possibility that 
the expiration time might just happen to be 0.  If it is, the 
timer_expires value should be bumped up to 1 before the mod_timer() 
call.

Alan Stern

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

[Rafael J. Wysocki]

On Tuesday 01 December 2009, Alan Stern wrote:
> Rafael:

Hi,

> One other thing I just noticed.  The power.timer_expires field is 
> supposed to be nonzero when the timer is scheduled.  But the code at 
> the end of pm_schedule_suspend() doesn't check for the possibility that 
> the expiration time might just happen to be 0.  If it is, the 
> timer_expires value should be bumped up to 1 before the mod_timer() 
> call.

Do you mean something like the patch below?

Rafael

---
 drivers/base/power/runtime.c |    2 ++
 1 file changed, 2 insertions(+)

Index: linux-2.6/drivers/base/power/runtime.c
===================================================================
--- linux-2.6.orig/drivers/base/power/runtime.c
+++ linux-2.6/drivers/base/power/runtime.c
@@ -625,6 +625,8 @@ int pm_schedule_suspend(struct device *d
 		goto out;
 
 	dev->power.timer_expires = jiffies + msecs_to_jiffies(delay);
+	if (!dev->power.timer_expires)
+		dev->power.timer_expires = 1;
 	mod_timer(&dev->power.suspend_timer, dev->power.timer_expires);
 
  out:

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

[Rafael J. Wysocki]

On Monday 30 November 2009, Alan Stern wrote:
> On Mon, 30 Nov 2009, Rafael J. Wysocki wrote:
> 
> > On Monday 30 November 2009, Alan Stern wrote:
> > > This patch (as1308) 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.
> > > 
> > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> > 
> > All three patches applied to suspend-2.6/linux-next.  Do you want me to push
> > them to Linus before 2.6.32, or perhaps the $subject one only?
> 
> Thanks.  The patches are not urgently needed; they can wait until the 
> merge window.
> 
> BTW, I just noticed that the runtime_idle method is documented as 
> returning void but defined in pm.h as returning int.  Obviously the 
> definition is wrong, since no return value is ever used.  Would you 
> like to fix it?

In fact it's been changed to return int at the Matthew's request, because
bus type code might use the return value from a driver's callback.  In fact
the PCI runtime PM framework being worked on at the moment uses
the return value of a driver's runtime_idle(), if implemented.

So, the documentation has to be changed rather than the definition.

Thanks,
Rafael

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

[Alan Stern]

On Wed, 2 Dec 2009, Rafael J. Wysocki wrote:

> On Tuesday 01 December 2009, Alan Stern wrote:
> > Rafael:
> 
> Hi,
> 
> > One other thing I just noticed.  The power.timer_expires field is 
> > supposed to be nonzero when the timer is scheduled.  But the code at 
> > the end of pm_schedule_suspend() doesn't check for the possibility that 
> > the expiration time might just happen to be 0.  If it is, the 
> > timer_expires value should be bumped up to 1 before the mod_timer() 
> > call.
> 
> Do you mean something like the patch below?
> 
> Rafael
> 
> ---
>  drivers/base/power/runtime.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> Index: linux-2.6/drivers/base/power/runtime.c
> ===================================================================
> --- linux-2.6.orig/drivers/base/power/runtime.c
> +++ linux-2.6/drivers/base/power/runtime.c
> @@ -625,6 +625,8 @@ int pm_schedule_suspend(struct device *d
>  		goto out;
>  
>  	dev->power.timer_expires = jiffies + msecs_to_jiffies(delay);
> +	if (!dev->power.timer_expires)
> +		dev->power.timer_expires = 1;
>  	mod_timer(&dev->power.suspend_timer, dev->power.timer_expires);

Yes, that's exactly what I meant.

Alan Stern

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

[Alan Stern]

On Wed, 2 Dec 2009, Rafael J. Wysocki wrote:

> > BTW, I just noticed that the runtime_idle method is documented as 
> > returning void but defined in pm.h as returning int.  Obviously the 
> > definition is wrong, since no return value is ever used.  Would you 
> > like to fix it?
> 
> In fact it's been changed to return int at the Matthew's request, because
> bus type code might use the return value from a driver's callback.  In fact
> the PCI runtime PM framework being worked on at the moment uses
> the return value of a driver's runtime_idle(), if implemented.
> 
> So, the documentation has to be changed rather than the definition.

Understood.  When you change it, mention that the PM core doesn't use
the return value from bus subsystems, but the bus might want to use the 
return value from drivers.

Alan Stern