diff mbox

[v2] PM / Runtime: Introduce pm_runtime_get_noidle

Message ID 1824809.48ZhrgqXQF@vostro.rjw.lan (mailing list archive)
State New, archived
Headers show

Commit Message

Rafael J. Wysocki Dec. 12, 2015, 1:51 a.m. UTC
On Saturday, December 12, 2015 12:41:06 AM Rafael J. Wysocki wrote:
> On Saturday, December 12, 2015 12:21:43 AM Rafael J. Wysocki wrote:
> > On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> > > On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > > > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak wrote:
> > > > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki wrote:
> > > > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J. Wysocki
> 
> [cut]
> 
> > 
> > Yes, my suggested function can be written like this:
> > 
> > bool pm_runtime_get_if_active(struct device *dev)
> > {
> >         unsigned log flags;
> >         bool ret = false;
> > 
> >         spin_lock_irqsave(&dev->power.lock, flags);
> > 
> >         if (dev->power.runtime_status == RPM_ACTIVE) {
> >                 if (atomic_inc_return(&dev->power.usage_count) > 1)
> >                 	ret = true;
> > 		else
> > 			atomic_dec(&dev->power.usage_count);
> >         }
> > 
> >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > 	return ret;
> > }
> > 
> > but this is obviously racy with respect to anyone concurrently changing the
> > usage counter.
> 
> Somethng like this would be slightly more efficient:

And below is a patch to implement the thing, compile-tested only.

Please let me know if that's what you need.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: PM / runtime: Add new helper for conditional usage count incrementation

Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
that will increment the device's runtime PM usage counter and
return 'true' if its status is RPM_ACTIVE and its usage counter
is greater than 0 at the same time ('false' will be returned
otherwise).

This is useful for things that should only be done if the device
is active (from the runtime PM perspective) and used by somebody
(as indicated by the usage counter) already and engaging the device
otherwise would be overkill.

Requested-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 Documentation/power/runtime_pm.txt |    5 +++++
 drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
 include/linux/pm_runtime.h         |    5 +++++
 3 files changed, 31 insertions(+)

Comments

Imre Deak Dec. 12, 2015, 7:40 p.m. UTC | #1
On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> On Saturday, December 12, 2015 12:41:06 AM Rafael J. Wysocki wrote:
> > On Saturday, December 12, 2015 12:21:43 AM Rafael J. Wysocki wrote:
> > > On Friday, December 11, 2015 05:47:08 PM Imre Deak wrote:
> > > > On pe, 2015-12-11 at 16:40 +0100, Rafael J. Wysocki wrote:
> > > > > On Friday, December 11, 2015 02:54:45 PM Imre Deak wrote:
> > > > > > On to, 2015-12-10 at 23:14 +0100, Rafael J. Wysocki wrote:
> > > > > > > On Thursday, December 10, 2015 11:20:40 PM Imre Deak
> > > > > > > wrote:
> > > > > > > > On Thu, 2015-12-10 at 22:42 +0100, Rafael J. Wysocki
> > > > > > > > wrote:
> > > > > > > > > On Thursday, December 10, 2015 10:36:37 PM Rafael J.
> > > > > > > > > Wysocki
> > 
> > [cut]
> > 
> > > 
> > > Yes, my suggested function can be written like this:
> > > 
> > > bool pm_runtime_get_if_active(struct device *dev)
> > > {
> > >         unsigned log flags;
> > >         bool ret = false;
> > > 
> > >         spin_lock_irqsave(&dev->power.lock, flags);
> > > 
> > >         if (dev->power.runtime_status == RPM_ACTIVE) {
> > >                 if (atomic_inc_return(&dev->power.usage_count) >
> > > 1)
> > >                 	ret = true;
> > > 		else
> > > 			atomic_dec(&dev->power.usage_count);
> > >         }
> > > 
> > >         spin_unlock_irqrestore(&dev->power.lock, flags);
> > > 	return ret;
> > > }
> > > 
> > > but this is obviously racy with respect to anyone concurrently
> > > changing the
> > > usage counter.
> > 
> > Somethng like this would be slightly more efficient:
> 
> And below is a patch to implement the thing, compile-tested only.
> 
> Please let me know if that's what you need.
> 
> Thanks,
> Rafael
> 
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: PM / runtime: Add new helper for conditional usage count
> incrementation
> 
> Introduce a new runtime PM function, pm_runtime_get_if_in_use(),
> that will increment the device's runtime PM usage counter and
> return 'true' if its status is RPM_ACTIVE and its usage counter
> is greater than 0 at the same time ('false' will be returned
> otherwise).
> 
> This is useful for things that should only be done if the device
> is active (from the runtime PM perspective) and used by somebody
> (as indicated by the usage counter) already and engaging the device
> otherwise would be overkill.
> 
> Requested-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  Documentation/power/runtime_pm.txt |    5 +++++
>  drivers/base/power/runtime.c       |   21 +++++++++++++++++++++
>  include/linux/pm_runtime.h         |    5 +++++
>  3 files changed, 31 insertions(+)
> 
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -966,6 +966,27 @@ int __pm_runtime_resume(struct device *d
>  EXPORT_SYMBOL_GPL(__pm_runtime_resume);
>  
>  /**
> + * pm_runtime_get_if_in_use - Conditionally bump up the device's
> usage counter.
> + * @dev: Device to handle.
> + *
> + * Increment the device's runtime PM usage counter and return 'true'
> if its
> + * runtime PM status is RPM_ACTIVE and its usage counter is already
> different
> + * from zero at the same time.  Otherwise, return 'false'.
> + */
> +bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	unsigned long flags;
> +	bool retval;
> +
> +	spin_lock_irqsave(&dev->power.lock, flags);
> +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> false;
> +	spin_unlock_irqrestore(&dev->power.lock, flags);
> +	return retval;
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> +

To me this looks ok:
Acked-by: Imre Deak <imre.deak@intel.com>

The original idea for this logic came from Chris so he may have some
comments.

Thanks,
Imre

> +/**
>   * __pm_runtime_set_status - Set runtime PM status of a device.
>   * @dev: Device to handle.
>   * @status: New runtime PM status of the device.
> Index: linux-pm/include/linux/pm_runtime.h
> ===================================================================
> --- linux-pm.orig/include/linux/pm_runtime.h
> +++ linux-pm/include/linux/pm_runtime.h
> @@ -39,6 +39,7 @@ extern int pm_runtime_force_resume(struc
>  extern int __pm_runtime_idle(struct device *dev, int rpmflags);
>  extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
>  extern int __pm_runtime_resume(struct device *dev, int rpmflags);
> +extern bool pm_runtime_get_if_in_use(struct device *dev);
>  extern int pm_schedule_suspend(struct device *dev, unsigned int
> delay);
>  extern int __pm_runtime_set_status(struct device *dev, unsigned int
> status);
>  extern int pm_runtime_barrier(struct device *dev);
> @@ -143,6 +144,10 @@ static inline int pm_schedule_suspend(st
>  {
>  	return -ENOSYS;
>  }
> +static inline bool pm_runtime_get_if_in_use(struct device *dev)
> +{
> +	return true;
> +}
>  static inline int __pm_runtime_set_status(struct device *dev,
>  					    unsigned int status) {
> return 0; }
>  static inline int pm_runtime_barrier(struct device *dev) { return 0;
> }
> Index: linux-pm/Documentation/power/runtime_pm.txt
> ===================================================================
> --- linux-pm.orig/Documentation/power/runtime_pm.txt
> +++ linux-pm/Documentation/power/runtime_pm.txt
> @@ -371,6 +371,11 @@ drivers/base/power/runtime.c and include
>      - increment the device's usage counter, run
> pm_runtime_resume(dev) and
>        return its result
>  
> +  bool pm_runtime_get_if_in_use(struct device *dev);
> +    - increment the device's usage counter and return 'true' if its
> runtime PM
> +      status is 'active' and its usage counter is greater than 0 at
> the same
> +      time; return 'false' otherwise
> +
>    void pm_runtime_put_noidle(struct device *dev);
>      - decrement the device's usage counter
>  
>
Chris Wilson Dec. 12, 2015, 7:49 p.m. UTC | #2
On Sat, Dec 12, 2015 at 09:40:45PM +0200, Imre Deak wrote:
> On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> > +bool pm_runtime_get_if_in_use(struct device *dev)
> > +{
> > +	unsigned long flags;
> > +	bool retval;
> > +
> > +	spin_lock_irqsave(&dev->power.lock, flags);
> > +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> > +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> > false;
> > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > +	return retval;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> > +
> 
> To me this looks ok:
> Acked-by: Imre Deak <imre.deak@intel.com>

Pendant says
retval = (dev->power.runtime_status == RPM_ACTIVE &&
	  atomic_inc_not_zero(&dev->power.usage_count);
-Chris
Rafael J. Wysocki Dec. 14, 2015, 2:04 a.m. UTC | #3
On Saturday, December 12, 2015 07:49:56 PM Chris Wilson wrote:
> On Sat, Dec 12, 2015 at 09:40:45PM +0200, Imre Deak wrote:
> > On Sat, 2015-12-12 at 02:51 +0100, Rafael J. Wysocki wrote:
> > > +bool pm_runtime_get_if_in_use(struct device *dev)
> > > +{
> > > +	unsigned long flags;
> > > +	bool retval;
> > > +
> > > +	spin_lock_irqsave(&dev->power.lock, flags);
> > > +	retval = dev->power.runtime_status == RPM_ACTIVE ?
> > > +		!!atomic_inc_not_zero(&dev->power.usage_count) :
> > > false;
> > > +	spin_unlock_irqrestore(&dev->power.lock, flags);
> > > +	return retval;
> > > +}
> > > +EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
> > > +
> > 
> > To me this looks ok:
> > Acked-by: Imre Deak <imre.deak@intel.com>
> 
> Pendant says
> retval = (dev->power.runtime_status == RPM_ACTIVE &&
> 	  atomic_inc_not_zero(&dev->power.usage_count);

Well, this wouldn't build AFAICS.

retval = dev->power.runtime_status == RPM_ACTIVE &&
	atomic_inc_not_zero(&dev->power.usage_count);

Thanks,
Rafael
diff mbox

Patch

Index: linux-pm/drivers/base/power/runtime.c
===================================================================
--- linux-pm.orig/drivers/base/power/runtime.c
+++ linux-pm/drivers/base/power/runtime.c
@@ -966,6 +966,27 @@  int __pm_runtime_resume(struct device *d
 EXPORT_SYMBOL_GPL(__pm_runtime_resume);
 
 /**
+ * pm_runtime_get_if_in_use - Conditionally bump up the device's usage counter.
+ * @dev: Device to handle.
+ *
+ * Increment the device's runtime PM usage counter and return 'true' if its
+ * runtime PM status is RPM_ACTIVE and its usage counter is already different
+ * from zero at the same time.  Otherwise, return 'false'.
+ */
+bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	unsigned long flags;
+	bool retval;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	retval = dev->power.runtime_status == RPM_ACTIVE ?
+		!!atomic_inc_not_zero(&dev->power.usage_count) : false;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_runtime_get_if_in_use);
+
+/**
  * __pm_runtime_set_status - Set runtime PM status of a device.
  * @dev: Device to handle.
  * @status: New runtime PM status of the device.
Index: linux-pm/include/linux/pm_runtime.h
===================================================================
--- linux-pm.orig/include/linux/pm_runtime.h
+++ linux-pm/include/linux/pm_runtime.h
@@ -39,6 +39,7 @@  extern int pm_runtime_force_resume(struc
 extern int __pm_runtime_idle(struct device *dev, int rpmflags);
 extern int __pm_runtime_suspend(struct device *dev, int rpmflags);
 extern int __pm_runtime_resume(struct device *dev, int rpmflags);
+extern bool pm_runtime_get_if_in_use(struct device *dev);
 extern int pm_schedule_suspend(struct device *dev, unsigned int delay);
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
@@ -143,6 +144,10 @@  static inline int pm_schedule_suspend(st
 {
 	return -ENOSYS;
 }
+static inline bool pm_runtime_get_if_in_use(struct device *dev)
+{
+	return true;
+}
 static inline int __pm_runtime_set_status(struct device *dev,
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
Index: linux-pm/Documentation/power/runtime_pm.txt
===================================================================
--- linux-pm.orig/Documentation/power/runtime_pm.txt
+++ linux-pm/Documentation/power/runtime_pm.txt
@@ -371,6 +371,11 @@  drivers/base/power/runtime.c and include
     - increment the device's usage counter, run pm_runtime_resume(dev) and
       return its result
 
+  bool pm_runtime_get_if_in_use(struct device *dev);
+    - increment the device's usage counter and return 'true' if its runtime PM
+      status is 'active' and its usage counter is greater than 0 at the same
+      time; return 'false' otherwise
+
   void pm_runtime_put_noidle(struct device *dev);
     - decrement the device's usage counter