[PATCH 1/2] PM/runtime: update document about callbacks

[PATCH 1/2] PM/runtime: update document about callbacks

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

Support for device power domains has been introduced in
commit 9659cc0678b954f187290c6e8b247a673c5d37e1 (PM: Make
system-wide PM and runtime PM treat subsystems consistently),
also power domain callbacks will take precedence over subsystem ones
from commit 4d27e9dcff00a6425d779b065ec8892e4f391661(PM: Make
power domain callbacks take precedence over subsystem ones).

So update part of "Device Runtime PM Callbacks" in
Documentation/power/runtime_pm.txt.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 Documentation/power/runtime_pm.txt |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/power/runtime_pm.txt b/Documentation/power/runtime_pm.txt
index 1f05404..0e85608 100644
--- a/Documentation/power/runtime_pm.txt
+++ b/Documentation/power/runtime_pm.txt
@@ -43,13 +43,18 @@ struct dev_pm_ops {
 	...
 };
 
-The ->runtime_suspend(), ->runtime_resume() and ->runtime_idle() callbacks are
-executed by the PM core for either the device type, or the class (if the device
-type's struct dev_pm_ops object does not exist), or the bus type (if the
-device type's and class' struct dev_pm_ops objects do not exist) of the given
-device (this allows device types to override callbacks provided by bus types or
-classes if necessary).  The bus type, device type and class callbacks are
-referred to as subsystem-level callbacks in what follows.
+The ->runtime_suspend(), ->runtime_resume() and ->runtime_idle() callbacks
+are executed by the PM core for either the power domain, or the device type
+(if the device power domain's struct dev_pm_ops does not exist), or the class
+(if the device power domain's and type's struct dev_pm_ops object does not
+exist), or the bus type (if the device power domain's, type's and class'
+struct dev_pm_ops objects do not exist) of the given device, so the priority
+order of callbacks from high to low is that power domain callbacks, device
+type callbacks, class callbacks and bus type callbacks, and the high priority
+one will take precedence over low priority one. The bus type, device type and
+class callbacks are referred to as subsystem-level callbacks in what follows,
+and generally speaking, the power domain callbacks are used for representing
+power domains within a SoC.
 
 By default, the callbacks are always invoked in process context with interrupts
 enabled.  However, subsystems can use the pm_runtime_irq_safe() helper function
-- 
1.7.4.1

[PATCH 2/2] PM/runtime: handle ->runtime_suspend failure correctly

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

If ->runtime_suspend returns -EAGAIN or -EBUSY, the device should
still be in ACTIVE state, so it is not needed to handle defered
resume and idle notification to its parent; if ->runtime_suspend
returns other fatal failure, it doesn't make sense to process defered
resume and send idle notification to its parent.

So skip these when failure is returned from ->runtime_suspend, also add
comments for this handling in rpm_suspend.

This patch also updates comments for rpm_suspend:

- 'Cancel a pending idle notification' should be put before, also
should be changed as 'Cancel a pending idle notification or
autosuspend/suspend'

- idle notification for suspend failure has been removed, so update
comments for it

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/base/power/runtime.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 441b5a3..6fa3241 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -284,14 +284,17 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
  * @dev: Device to suspend.
  * @rpmflags: Flag bits.
  *
- * Check if the device's runtime PM status allows it to be suspended.  If
- * another suspend has been started earlier, either return immediately or wait
- * for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC flags.  Cancel a
- * pending idle notification.  If the RPM_ASYNC flag is set then queue a
- * suspend request; otherwise run the ->runtime_suspend() callback directly.
- * If a deferred resume was requested while the callback was running then carry
- * it out; otherwise send an idle notification for the device (if the suspend
- * failed) or for its parent (if the suspend succeeded).
+ * Check if the device's runtime PM status allows it to be suspended. Cancel
+ * a pending idle notification or autosuspend/suspend. If another suspend has
+ * been started earlier, either return immediately or wait for it to finish,
+ * depending on the RPM_NOWAIT and RPM_ASYNC flags. If the RPM_ASYNC flag is
+ * set then queue a suspend request; otherwise run the ->runtime_suspend()
+ * callback directly. If ->runtime_suspend returns failure, just cancel
+ * pending request and wake up waited tasks, then return immediatelly.
+ * After ->runtime_suspend succeeded, if a deferred resume was requested
+ * while the callback was running then carry it out; otherwise send an idle
+ * notification for its parent (if both ignore_children and irq_safe
+ * are not set).
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
@@ -422,6 +425,9 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 	}
 	wake_up_all(&dev->power.wait_queue);
 
+	if (retval)
+		goto out;
+
 	if (dev->power.deferred_resume) {
 		rpm_resume(dev, 0);
 		retval = -EAGAIN;
-- 
1.7.4.1

Re: [PATCH 2/2] PM/runtime: handle ->runtime_suspend failure correctly

[Alan Stern]

On Sat, 10 Sep 2011 tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>
> 
> If ->runtime_suspend returns -EAGAIN or -EBUSY, the device should
> still be in ACTIVE state, so it is not needed to handle defered
> resume and idle notification to its parent; if ->runtime_suspend
> returns other fatal failure, it doesn't make sense to process defered
> resume and send idle notification to its parent.

> @@ -422,6 +425,9 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>  	}
>  	wake_up_all(&dev->power.wait_queue);
>  
> +	if (retval)
> +		goto out;
> +
>  	if (dev->power.deferred_resume) {
>  		rpm_resume(dev, 0);
>  		retval = -EAGAIN;

If there's a suspend failure, the deferred_resume flag gets turned off 
anyway.  But skipping this test won't hurt, and skipping the parent 
notification is a good idea.

In fact, it might be even better to put a copy of the wake_up_all() in
the "if (retval)" branch after the suspend callback and then go
directly to out.  The "else" branch could then become part of the
straight-through code, not indented.

Alan Stern

Re: [PATCH 2/2] PM/runtime: handle ->runtime_suspend failure correctly

[Ming Lei]

Hi,

On Mon, Sep 12, 2011 at 4:44 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 10 Sep 2011 tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> If ->runtime_suspend returns -EAGAIN or -EBUSY, the device should
>> still be in ACTIVE state, so it is not needed to handle defered
>> resume and idle notification to its parent; if ->runtime_suspend
>> returns other fatal failure, it doesn't make sense to process defered
>> resume and send idle notification to its parent.
>
>> @@ -422,6 +425,9 @@ static int rpm_suspend(struct device *dev, int rpmflags)
>>       }
>>       wake_up_all(&dev->power.wait_queue);
>>
>> +     if (retval)
>> +             goto out;
>> +
>>       if (dev->power.deferred_resume) {
>>               rpm_resume(dev, 0);
>>               retval = -EAGAIN;
>
> If there's a suspend failure, the deferred_resume flag gets turned off
> anyway.  But skipping this test won't hurt, and skipping the parent
> notification is a good idea.

Yes, I will update the commit log.

>
> In fact, it might be even better to put a copy of the wake_up_all() in
> the "if (retval)" branch after the suspend callback and then go
> directly to out.  The "else" branch could then become part of the
> straight-through code, not indented.

Good idea, will update this in -v1.

thanks,
-- 
Ming Lei

[PATCH 2/2] PM/runtime: handle ->runtime_suspend failure correctly

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

If ->runtime_suspend returns -EAGAIN or -EBUSY, the device should
still be in ACTIVE state, so it is not needed to send idle notification
to its parent; if ->runtime_suspend returns other fatal failure, it
doesn't make sense to send idle notification to its parent.

So skip these when failure is returned from ->runtime_suspend, also add
comments for this handling in rpm_suspend.

This patch also updates comments for rpm_suspend:

- 'Cancel a pending idle notification' should be put before, also
should be changed as 'Cancel a pending idle notification or
autosuspend/suspend'

- idle notification for suspend failure has been removed, so update
comments for it

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
v1: some minor change on Alan's suggestion
---
 drivers/base/power/runtime.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 441b5a3..e3c6a8f 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -284,14 +284,17 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
  * @dev: Device to suspend.
  * @rpmflags: Flag bits.
  *
- * Check if the device's runtime PM status allows it to be suspended.  If
- * another suspend has been started earlier, either return immediately or wait
- * for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC flags.  Cancel a
- * pending idle notification.  If the RPM_ASYNC flag is set then queue a
- * suspend request; otherwise run the ->runtime_suspend() callback directly.
- * If a deferred resume was requested while the callback was running then carry
- * it out; otherwise send an idle notification for the device (if the suspend
- * failed) or for its parent (if the suspend succeeded).
+ * Check if the device's runtime PM status allows it to be suspended. Cancel
+ * a pending idle notification or autosuspend/suspend. If another suspend has
+ * been started earlier, either return immediately or wait for it to finish,
+ * depending on the RPM_NOWAIT and RPM_ASYNC flags. If the RPM_ASYNC flag is
+ * set then queue a suspend request; otherwise run the ->runtime_suspend()
+ * callback directly. If ->runtime_suspend returns failure, just cancel
+ * pending request and wake up waited tasks, then return immediatelly.
+ * After ->runtime_suspend succeeded, if a deferred resume was requested
+ * while the callback was running then carry it out; otherwise send an idle
+ * notification for its parent (if both ignore_children and irq_safe
+ * are not set).
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
@@ -410,15 +413,16 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 			dev->power.runtime_error = 0;
 		else
 			pm_runtime_cancel_pending(dev);
-	} else {
+		wake_up_all(&dev->power.wait_queue);
+		goto out;
+	}
  no_callback:
-		__update_runtime_status(dev, RPM_SUSPENDED);
-		pm_runtime_deactivate_timer(dev);
+	__update_runtime_status(dev, RPM_SUSPENDED);
+	pm_runtime_deactivate_timer(dev);
 
-		if (dev->parent) {
-			parent = dev->parent;
-			atomic_add_unless(&parent->power.child_count, -1, 0);
-		}
+	if (dev->parent) {
+		parent = dev->parent;
+		atomic_add_unless(&parent->power.child_count, -1, 0);
 	}
 	wake_up_all(&dev->power.wait_queue);
 
-- 
1.7.4.1

[PATCH 1/2] PM/runtime: fix comment for rpm_suspend

[ming.lei]

From: Ming Lei <ming.lei@canonical.com>

This patch fix comments for rpm_suspend:

- 'Cancel a pending idle notification' should be put before, also
should be changed as 'Cancel a pending idle notification,
autosuspend or suspend'

- after suspend succeeded, idle notification for the device has
been removed, so update comment for it

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/power/runtime.c |   18 ++++++++++--------
 1 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index 7a6fb5e..aa23a64 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -286,14 +286,16 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
  * @dev: Device to suspend.
  * @rpmflags: Flag bits.
  *
- * Check if the device's runtime PM status allows it to be suspended.  If
- * another suspend has been started earlier, either return immediately or wait
- * for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC flags.  Cancel a
- * pending idle notification.  If the RPM_ASYNC flag is set then queue a
- * suspend request; otherwise run the ->runtime_suspend() callback directly.
- * If a deferred resume was requested while the callback was running then carry
- * it out; otherwise send an idle notification for the device (if the suspend
- * failed) or for its parent (if the suspend succeeded).
+ * Check if the device's runtime PM status allows it to be suspended.
+ * Cancel a pending idle notification, autosuspend or suspend. If
+ * another suspend has been started earlier, either return immediately
+ * or wait for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC
+ * flags. If the RPM_ASYNC flag is set then queue a suspend request;
+ * otherwise run the ->runtime_suspend() callback directly. If a deferred
+ * resume was requested while the callback was running then carry it out;
+ * otherwise send an idle notification for its parent (if the suspend
+ * succeeded and both ignore_children of parent->power and irq_safe of
+ * dev->power are not set).
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
-- 
1.7.5.4

[PATCH 2/2] PM/runtime: handle ->runtime_suspend failure correctly

[ming.lei]

From: Ming Lei <ming.lei@canonical.com>

If ->runtime_suspend returns -EAGAIN or -EBUSY, the device should
still be in ACTIVE state, so it is not needed to send idle notification
to its parent; if ->runtime_suspend returns other fatal failure, it
doesn't make sense to send idle notification to its parent.

So skip these when failure is returned from ->runtime_suspend, also
update comments for this handling in rpm_suspend.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/base/power/runtime.c |   25 +++++++++++++------------
 1 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
index aa23a64..6bb3aaf 100644
--- a/drivers/base/power/runtime.c
+++ b/drivers/base/power/runtime.c
@@ -291,11 +291,11 @@ static int rpm_callback(int (*cb)(struct device *), struct device *dev)
  * another suspend has been started earlier, either return immediately
  * or wait for it to finish, depending on the RPM_NOWAIT and RPM_ASYNC
  * flags. If the RPM_ASYNC flag is set then queue a suspend request;
- * otherwise run the ->runtime_suspend() callback directly. If a deferred
- * resume was requested while the callback was running then carry it out;
- * otherwise send an idle notification for its parent (if the suspend
- * succeeded and both ignore_children of parent->power and irq_safe of
- * dev->power are not set).
+ * otherwise run the ->runtime_suspend() callback directly. When
+ * ->runtime_suspend succeeded, if a deferred resume was requested while
+ * the callback was running then carry it out, otherwise send an idle
+ * notification for its parent (if the suspend succeeded and both
+ * ignore_children of parent->power and irq_safe of dev->power are not set).
  *
  * This function must be called under dev->power.lock with interrupts disabled.
  */
@@ -420,15 +420,16 @@ static int rpm_suspend(struct device *dev, int rpmflags)
 			dev->power.runtime_error = 0;
 		else
 			pm_runtime_cancel_pending(dev);
-	} else {
+		wake_up_all(&dev->power.wait_queue);
+		goto out;
+	}
  no_callback:
-		__update_runtime_status(dev, RPM_SUSPENDED);
-		pm_runtime_deactivate_timer(dev);
+	__update_runtime_status(dev, RPM_SUSPENDED);
+	pm_runtime_deactivate_timer(dev);
 
-		if (dev->parent) {
-			parent = dev->parent;
-			atomic_add_unless(&parent->power.child_count, -1, 0);
-		}
+	if (dev->parent) {
+		parent = dev->parent;
+		atomic_add_unless(&parent->power.child_count, -1, 0);
 	}
 	wake_up_all(&dev->power.wait_queue);
 
-- 
1.7.5.4