[PATCH 0/2] PM / Runtime: Clean-up and documentation

[PATCH 0/2] PM / Runtime: Clean-up and documentation

[Rafael J. Wysocki]

Hi,

The following two patches are on top of the linux-next branch of my suspend-2.6
tree:

[1/2] - Clean up pm_runtime_disable()

[2/2] - Document new runtime PM helper functions as appropriate.

Thanks,
Rafael

[PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

Remove the second argument of __pm_runtime_disable(), which is not
necessary any more, and rename this funtion to pm_runtime_disable().
Accordingly, drop the static inline definition of
pm_runtime_disable() using __pm_runtime_disable() and export
pm_runtime_disable() as appropriate.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |   31 ++++---------------------------
 include/linux/pm_runtime.h   |    9 ++-------
 2 files changed, 6 insertions(+), 34 deletions(-)

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
@@ -943,20 +943,15 @@ int pm_runtime_barrier(struct device *de
 EXPORT_SYMBOL_GPL(pm_runtime_barrier);
 
 /**
- * __pm_runtime_disable - Disable run-time PM of a device.
+ * pm_runtime_disable - Disable run-time PM of a device.
  * @dev: Device to handle.
- * @check_resume: If set, check if there's a resume request for the device.
  *
  * Increment power.disable_depth for the device and if was zero previously,
  * cancel all pending run-time PM requests for the device and wait for all
  * operations in progress to complete.  The device can be either active or
  * suspended after its run-time PM has been disabled.
- *
- * If @check_resume is set and there's a resume request pending when
- * __pm_runtime_disable() is called and power.disable_depth is zero, the
- * function will wake up the device before disabling its run-time PM.
  */
-void __pm_runtime_disable(struct device *dev, bool check_resume)
+void pm_runtime_disable(struct device *dev)
 {
 	spin_lock_irq(&dev->power.lock);
 
@@ -965,31 +960,13 @@ void __pm_runtime_disable(struct device 
 		goto out;
 	}
 
-	/*
-	 * Wake up the device if there's a resume request pending, because that
-	 * means there probably is some I/O to process and disabling run-time PM
-	 * shouldn't prevent the device from processing the I/O.
-	 */
-	if (check_resume && dev->power.request_pending
-	    && dev->power.request == RPM_REQ_RESUME) {
-		/*
-		 * Prevent suspends and idle notifications from being carried
-		 * out after we have woken up the device.
-		 */
-		pm_runtime_get_noresume(dev);
-
-		__pm_runtime_resume(dev, false);
-
-		pm_runtime_put_noidle(dev);
-	}
-
 	if (!dev->power.disable_depth++)
 		__pm_runtime_barrier(dev);
 
  out:
 	spin_unlock_irq(&dev->power.lock);
 }
-EXPORT_SYMBOL_GPL(__pm_runtime_disable);
+EXPORT_SYMBOL_GPL(pm_runtime_disable);
 
 /**
  * pm_runtime_enable - Enable run-time PM of a device.
@@ -1092,7 +1069,7 @@ void pm_runtime_init(struct device *dev)
  */
 void pm_runtime_remove(struct device *dev)
 {
-	__pm_runtime_disable(dev, false);
+	pm_runtime_disable(dev);
 
 	/* Change the status back to 'suspended' to match the initial status. */
 	if (dev->power.runtime_status == RPM_ACTIVE)
Index: linux-2.6/include/linux/pm_runtime.h
===================================================================
--- linux-2.6.orig/include/linux/pm_runtime.h
+++ linux-2.6/include/linux/pm_runtime.h
@@ -27,7 +27,7 @@ extern int __pm_runtime_put(struct devic
 extern int __pm_runtime_set_status(struct device *dev, unsigned int status);
 extern int pm_runtime_barrier(struct device *dev);
 extern void pm_runtime_enable(struct device *dev);
-extern void __pm_runtime_disable(struct device *dev, bool check_resume);
+extern void pm_runtime_disable(struct device *dev);
 extern void pm_runtime_allow(struct device *dev);
 extern void pm_runtime_forbid(struct device *dev);
 
@@ -79,7 +79,7 @@ static inline int __pm_runtime_set_statu
 					    unsigned int status) { return 0; }
 static inline int pm_runtime_barrier(struct device *dev) { return 0; }
 static inline void pm_runtime_enable(struct device *dev) {}
-static inline void __pm_runtime_disable(struct device *dev, bool c) {}
+static inline void pm_runtime_disable(struct device *dev) {}
 static inline void pm_runtime_allow(struct device *dev) {}
 static inline void pm_runtime_forbid(struct device *dev) {}
 
@@ -122,9 +122,4 @@ static inline void pm_runtime_set_suspen
 	__pm_runtime_set_status(dev, RPM_SUSPENDED);
 }
 
-static inline void pm_runtime_disable(struct device *dev)
-{
-	__pm_runtime_disable(dev, true);
-}
-
 #endif

[PATCH 2/2] PM / Runtime: Document power.runtime_auto and related functions

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rjw@sisk.pl>

The power.runtime_auto device flag and the helper functions
pm_runtime_allow() and pm_runtime_forbid() used to modify it are a
part of the run-time power management framework and therefore they
should be described in Documentation/power/runtime_pm.txt.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 Documentation/power/runtime_pm.txt |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Index: linux-2.6/Documentation/power/runtime_pm.txt
===================================================================
--- linux-2.6.orig/Documentation/power/runtime_pm.txt
+++ linux-2.6/Documentation/power/runtime_pm.txt
@@ -224,6 +224,12 @@ defined in include/linux/pm.h:
       RPM_SUSPENDED, which means that each device is initially regarded by the
       PM core as 'suspended', regardless of its real hardware status
 
+  unsigned int runtime_auto;
+    - if set, indicates that the user space has allowed the device driver to
+      power manage the device at run time via the /sys/devices/.../power/control
+      interface; it may only be modified with the help of the pm_runtime_allow()
+      and pm_runtime_forbid() helper functions
+
 All of the above fields are members of the 'power' member of 'struct device'.
 
 4. Run-time PM Device Helper Functions
@@ -329,6 +335,16 @@ drivers/base/power/runtime.c and include
       'power.runtime_error' is set or 'power.disable_depth' is greater than
       zero)
 
+  void pm_runtime_allow(struct device *dev);
+    - set the power.runtime_auto flag for the device and decrease its usage
+      counter (used by the /sys/devices/.../power/control interface to
+      effectively allow the device to be power managed at run time)
+
+  void pm_runtime_forbid(struct device *dev);
+    - unset the power.runtime_auto flag for the device and increase its usage
+      counter (used by the /sys/devices/.../power/control interface to
+      effectively prevent the device from being power managed at run time)
+
 It is safe to execute the following helper functions from interrupt context:
 
 pm_request_idle()
@@ -382,6 +398,18 @@ may be desirable to suspend the device a
 finished, so the PM core uses pm_runtime_idle_sync() to invoke the
 subsystem-level idle callback for the device at that time.
 
+The user space can effectively disallow the driver of the device to power manage
+it at run time by changing the value of its /sys/devices/.../power/control
+attribute to "on", which causes pm_runtime_forbid() to be called.  In principle,
+this mechanism may also be used by the driver to effectively turn off the
+run-time power management of the device until the user space turns it on.
+Namely, during the initialization the driver can make sure that the run-time PM
+status of the device is 'active' and call pm_runtime_forbid().  It should be
+noted, however, that if the user space has already intentionally changed the
+value of /sys/devices/.../power/control to "auto" to allow the driver to power
+manage the device at run time, the driver may confuse it by using
+pm_runtime_forbid() this way.
+
 6. Run-time PM and System Sleep
 
 Run-time PM and system sleep (i.e., system suspend and hibernation, also known

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Alan Stern]

On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Remove the second argument of __pm_runtime_disable(), which is not
> necessary any more, and rename this funtion to pm_runtime_disable().
> Accordingly, drop the static inline definition of
> pm_runtime_disable() using __pm_runtime_disable() and export
> pm_runtime_disable() as appropriate.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/runtime.c |   31 ++++---------------------------
>  include/linux/pm_runtime.h   |    9 ++-------
>  2 files changed, 6 insertions(+), 34 deletions(-)
> 
> 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
> @@ -943,20 +943,15 @@ int pm_runtime_barrier(struct device *de
>  EXPORT_SYMBOL_GPL(pm_runtime_barrier);
>  
>  /**
> - * __pm_runtime_disable - Disable run-time PM of a device.
> + * pm_runtime_disable - Disable run-time PM of a device.
>   * @dev: Device to handle.
> - * @check_resume: If set, check if there's a resume request for the device.
>   *
>   * Increment power.disable_depth for the device and if was zero previously,
>   * cancel all pending run-time PM requests for the device and wait for all
>   * operations in progress to complete.  The device can be either active or
>   * suspended after its run-time PM has been disabled.
> - *
> - * If @check_resume is set and there's a resume request pending when
> - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> - * function will wake up the device before disabling its run-time PM.
>   */
> -void __pm_runtime_disable(struct device *dev, bool check_resume)
> +void pm_runtime_disable(struct device *dev)
>  {

Why did you decide to remove the check_resume argument?  That decision 
should be explained in the patch description.

Alan Stern

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Rafael J. Wysocki]

On Thursday 28 January 2010, Alan Stern wrote:
> On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:
> 
> > From: Rafael J. Wysocki <rjw@sisk.pl>
> > 
> > Remove the second argument of __pm_runtime_disable(), which is not
> > necessary any more, and rename this funtion to pm_runtime_disable().
> > Accordingly, drop the static inline definition of
> > pm_runtime_disable() using __pm_runtime_disable() and export
> > pm_runtime_disable() as appropriate.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/runtime.c |   31 ++++---------------------------
> >  include/linux/pm_runtime.h   |    9 ++-------
> >  2 files changed, 6 insertions(+), 34 deletions(-)
> > 
> > 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
> > @@ -943,20 +943,15 @@ int pm_runtime_barrier(struct device *de
> >  EXPORT_SYMBOL_GPL(pm_runtime_barrier);
> >  
> >  /**
> > - * __pm_runtime_disable - Disable run-time PM of a device.
> > + * pm_runtime_disable - Disable run-time PM of a device.
> >   * @dev: Device to handle.
> > - * @check_resume: If set, check if there's a resume request for the device.
> >   *
> >   * Increment power.disable_depth for the device and if was zero previously,
> >   * cancel all pending run-time PM requests for the device and wait for all
> >   * operations in progress to complete.  The device can be either active or
> >   * suspended after its run-time PM has been disabled.
> > - *
> > - * If @check_resume is set and there's a resume request pending when
> > - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> > - * function will wake up the device before disabling its run-time PM.
> >   */
> > -void __pm_runtime_disable(struct device *dev, bool check_resume)
> > +void pm_runtime_disable(struct device *dev)
> >  {
> 
> Why did you decide to remove the check_resume argument?  That decision 
> should be explained in the patch description.

Well, I thought the "which is not necessary any more" would be a sufficient
explanation ...

Rafael

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Alan Stern]

On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:

> > > - * If @check_resume is set and there's a resume request pending when
> > > - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> > > - * function will wake up the device before disabling its run-time PM.
> > >   */
> > > -void __pm_runtime_disable(struct device *dev, bool check_resume)
> > > +void pm_runtime_disable(struct device *dev)
> > >  {
> > 
> > Why did you decide to remove the check_resume argument?  That decision 
> > should be explained in the patch description.
> 
> Well, I thought the "which is not necessary any more" would be a sufficient
> explanation ...

But why is it not necessary now, given that apparently it was necessary 
before?  What has changed to make it unnecessary?

Alan Stern

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Rafael J. Wysocki]

On Friday 29 January 2010, Alan Stern wrote:
> On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:
> 
> > > > - * If @check_resume is set and there's a resume request pending when
> > > > - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> > > > - * function will wake up the device before disabling its run-time PM.
> > > >   */
> > > > -void __pm_runtime_disable(struct device *dev, bool check_resume)
> > > > +void pm_runtime_disable(struct device *dev)
> > > >  {
> > > 
> > > Why did you decide to remove the check_resume argument?  That decision 
> > > should be explained in the patch description.
> > 
> > Well, I thought the "which is not necessary any more" would be a sufficient
> > explanation ...
> 
> But why is it not necessary now,

Well, all of the existing callers use only one value of it, which is 'false'
(perhaps I should write that in the changelog).

> given that apparently it was necessary before?  What has changed to make
> it unnecessary?

It was used in the system suspend code path in main.c (in dpm_prepare()) IIRC,
but it was replaced by the current code.

I don't really think it's useful to try to recall why it was used at one point.
It's not used now and I don't have a usage case for it.  If no one else knows
it will be necessary, removing it is the right thing to do.

Rafael

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Alan Stern]

On Fri, 29 Jan 2010, Rafael J. Wysocki wrote:

> On Friday 29 January 2010, Alan Stern wrote:
> > On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:
> > 
> > > > > - * If @check_resume is set and there's a resume request pending when
> > > > > - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> > > > > - * function will wake up the device before disabling its run-time PM.
> > > > >   */
> > > > > -void __pm_runtime_disable(struct device *dev, bool check_resume)
> > > > > +void pm_runtime_disable(struct device *dev)
> > > > >  {
> > > > 
> > > > Why did you decide to remove the check_resume argument?  That decision 
> > > > should be explained in the patch description.
> > > 
> > > Well, I thought the "which is not necessary any more" would be a sufficient
> > > explanation ...
> > 
> > But why is it not necessary now,
> 
> Well, all of the existing callers use only one value of it, which is 'false'
> (perhaps I should write that in the changelog).

I don't understand.  Isn't the existing version of pm_runtime_disable()
a caller which sets check_resume to 'true'?  There certainly are places 
that call pm_runtime_disable().

> > given that apparently it was necessary before?  What has changed to make
> > it unnecessary?
> 
> It was used in the system suspend code path in main.c (in dpm_prepare()) IIRC,
> but it was replaced by the current code.
> 
> I don't really think it's useful to try to recall why it was used at one point.
> It's not used now and I don't have a usage case for it.  If no one else knows
> it will be necessary, removing it is the right thing to do.

A particularly important usage case for pm_runtime_disable() is when a
bus subsystem or driver does when resuming from system sleep and the
device was already runtime-suspended when the sleep began.  However in 
this case (and all the other cases I'm aware of) there's no need to 
check whether a resume request is pending before doing the 
runtime_disable.

I wasn't saying that the patch is wrong, just asking the reason for it.

Alan Stern

Re: [PATCH 1/2] PM / Runtime: Clean up pm_runtime_disable()

[Rafael J. Wysocki]

On Friday 29 January 2010, Alan Stern wrote:
> On Fri, 29 Jan 2010, Rafael J. Wysocki wrote:
> 
> > On Friday 29 January 2010, Alan Stern wrote:
> > > On Thu, 28 Jan 2010, Rafael J. Wysocki wrote:
> > > 
> > > > > > - * If @check_resume is set and there's a resume request pending when
> > > > > > - * __pm_runtime_disable() is called and power.disable_depth is zero, the
> > > > > > - * function will wake up the device before disabling its run-time PM.
> > > > > >   */
> > > > > > -void __pm_runtime_disable(struct device *dev, bool check_resume)
> > > > > > +void pm_runtime_disable(struct device *dev)
> > > > > >  {
> > > > > 
> > > > > Why did you decide to remove the check_resume argument?  That decision 
> > > > > should be explained in the patch description.
> > > > 
> > > > Well, I thought the "which is not necessary any more" would be a sufficient
> > > > explanation ...
> > > 
> > > But why is it not necessary now,
> > 
> > Well, all of the existing callers use only one value of it, which is 'false'
> > (perhaps I should write that in the changelog).
> 
> I don't understand.  Isn't the existing version of pm_runtime_disable()
> a caller which sets check_resume to 'true'?  There certainly are places 
> that call pm_runtime_disable().

Sorry, you're absolutely right, so the patch is wrong.

Rafael