PM/runtime: update document about callbacks

PATCH RESEND Applied [email protected] 2 participants 6 messages Oct 9, 2011 – Oct 11, 2011

Subsystems (4)

Driver core Suspend to RAM Hibernation Power management core

lore

Share & tools

lore

[email protected]

author

2011-10-09 03:40 UTC #0 lore raw

PATCH RESEND 1/2 PM/runtime: update document about callbacks

From: Ming Lei <[email protected]>

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 <[email protected]>

---
 Documentation/power/runtime_pm.txt |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

Documentation/power/runtime_pm.txt +12 -7

index 1f05404..0e85608 100644
@@ -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

Signature · 1 line

1.7.4.1

[email protected]

author

2011-10-09 03:41 UTC #1 lore raw

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

From: Ming Lei <[email protected]>

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 <[email protected]>

---
v1: some minor change on Alan's suggestion

---
 drivers/base/power/runtime.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

drivers/base/power/runtime.c +19 ~~-15~~

index 441b5a3..e3c6a8f 100644
@@ -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);

Signature · 1 line

1.7.4.1

Rafael J. Wysocki

2011-10-10 21:38 UTC #2 lore raw

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

Quoted 7 lines · On Sunday, October 09, 2011, [email protected] wrote:

From: Ming Lei <[email protected]>
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.

Quoted 8 lines

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'

That should be a different patch I think?

Quoted 41 lines

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

Signed-off-by Ming Lei <[email protected]>

---
v1: some minor change on Alan's suggestion

---
 drivers/base/power/runtime.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)

drivers/base/power/runtime.c +11 -8

index 441b5a3..e3c6a8f 100644
@@ -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.
  */

drivers/base/power/runtime.c static int rpm_suspend(struct device *dev, int rpmflags) · line 419

Quoted 8 lines

drivers/base/power/runtime.c +3 -1

@@ -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:

I don't think the change above is correct. The code below no_callback only should be executed if retval is zero.

To achieve the goal (i.e. avoid notifying the parent if -EAGAIN or -EBUSY is returned by the callbacks) it would be sufficient to do parent = NULL along with resetting power.runtime_error.

Quoted 4 lines

drivers/base/power/runtime.c +2 -2

-		__update_runtime_status(dev, RPM_SUSPENDED);
-		pm_runtime_deactivate_timer(dev);
+	__update_runtime_status(dev, RPM_SUSPENDED);
+	pm_runtime_deactivate_timer(dev);

drivers/base/power/runtime.c static int rpm_suspend(struct device *dev, int rpmflags) · line 427

Quoted 10 lines

drivers/base/power/runtime.c +3 -4

 
-		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);

Thanks,
Rafael

Ming Lei

author

2011-10-11 07:06 UTC #3 lore raw

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

2011/10/11 Rafael J. Wysocki <[email protected]>:

Quoted 11 lines · On Sunday, October 09, 2011, [email protected] wrote:

Quoted 10 lines
From: Ming Lei <[email protected]>
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.

Quoted 7 lines

Quoted 5 lines
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'
That should be a different patch I think?

OK, I will split it into two.

Quoted 45 lines

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

Signed-off-by Ming Lei <[email protected]>

---
v1: some minor change on Alan's suggestion
---
drivers/base/power/runtime.c | 34 +++++++++++++++++++---------------

 1 files changed, 19 insertions(+), 15 deletions(-)

drivers/base/power/runtime.c +11 -8

index 441b5a3..e3c6a8f 100644
@@ -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);

Quoted 8 lines

Quoted 5 lines
- } else {
+ wake_up_all(&dev->power.wait_queue);
+ goto out;
+ }
no_callback:
I don't think the change above is correct. The code below no_callback only should be executed if retval is zero.

The 'goto out' above no_callback will bypass this, won't it?

Quoted 2 lines

To achieve the goal (i.e. avoid notifying the parent if -EAGAIN or -EBUSY is returned by the callbacks) it would be sufficient to

For all non zero value return from callbacks, the idle notification to its parent should be avoided too.

Quoted 17 lines

do parent = NULL along with resetting power.runtime_error.
Quoted 5 lines
- __update_runtime_status(dev, RPM_SUSPENDED);
- pm_runtime_deactivate_timer(dev);
+ __update_runtime_status(dev, RPM_SUSPENDED);
+ pm_runtime_deactivate_timer(dev);
Quoted 9 lines
- 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);

Quoted 3 lines

Thanks,
Rafael

thanks,

Signature · 1 line

Ming Lei

Rafael J. Wysocki

2011-10-11 19:10 UTC #4 lore raw

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

Quoted 78 lines · Ming Lei

Hi
2011/10/11 Rafael J. Wysocki <[email protected]>:
Quoted 11 lines · On Sunday, October 09, 2011, [email protected] wrote:
Quoted 10 lines
From: Ming Lei <[email protected]>
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.
Quoted 7 lines
Quoted 5 lines
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'
That should be a different patch I think?
OK, I will split it into two.
Quoted 45 lines
Quoted 45 lines
- idle notification for suspend failure has been removed, so update comments for it
Signed-off-by Ming Lei <[email protected]>
---
v1: some minor change on Alan's suggestion
---
 drivers/base/power/runtime.c |   34 +++++++++++++++++++---------------
 1 files changed, 19 insertions(+), 15 deletions(-)
drivers/base/power/runtime.c +11 -8
index 441b5a3..e3c6a8f 100644
@@ -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);
Quoted 8 lines
drivers/base/power/runtime.c static int rpm_suspend(struct device *dev, int rpmflags) · line 419
Quoted 5 lines
drivers/base/power/runtime.c +3 -1
-     } else {
+             wake_up_all(&dev->power.wait_queue);
+             goto out;
+     }
  no_callback:
I don't think the change above is correct. The code below no_callback only should be executed if retval is zero.

Quoted 1 line

The 'goto out' above no_callback will bypass this, won't it?

Yes, it will, sorry. It seems I was confused by the removal of "} else {".

OK, so this is technically correct.

Please resubmit with the unrelated comment changes splitted out.

Thanks,
Rafael

Rafael J. Wysocki

Applied

2011-10-10 21:44 UTC #5 lore raw

In reply to [email protected]

Quoted 9 lines · On Sunday, October 09, 2011, [email protected] wrote:

From: Ming Lei <[email protected]>
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).

Quoted 4 lines

So update part of "Device Runtime PM Callbacks" in Documentation/power/runtime_pm.txt.
Signed-off-by Ming Lei <[email protected]>

Applied to linux-pm/pm-runtime (and merged into linux-pm/linux-next).

Thanks,
Rafael

Quoted 35 lines

---
 Documentation/power/runtime_pm.txt |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

Documentation/power/runtime_pm.txt +12 -7

index 1f05404..0e85608 100644
@@ -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