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

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

[tom.leiming]

From: Ming Lei <ming.lei@canonical.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 <ming.lei@canonical.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

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

[tom.leiming]

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 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 <ming.lei@canonical.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

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

[Rafael J. Wysocki]

On Sunday, October 09, 2011, tom.leiming@gmail.com wrote:
> 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 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?

> - idle notification for suspend failure has been removed, so update
> comments for it
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.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:

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.

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

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Sunday, October 09, 2011, tom.leiming@gmail.com wrote:
> From: Ming Lei <ming.lei@canonical.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 <ming.lei@canonical.com>

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

Thanks,
Rafael


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

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

[Ming Lei]

Hi

2011/10/11 Rafael J. Wysocki <rjw@sisk.pl>:
> On Sunday, October 09, 2011, tom.leiming@gmail.com wrote:
>> 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 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?

OK, I will split it into two.

>
>> - idle notification for suspend failure has been removed, so update
>> comments for it
>>
>> Signed-off-by: Ming Lei <ming.lei@canonical.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:
>
> 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?

> 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.

> do parent = NULL along with resetting power.runtime_error.
>
>> -             __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);
>
> Thanks,
> Rafael
>

thanks,
-- 
Ming Lei

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

[Rafael J. Wysocki]

On Tuesday, October 11, 2011, Ming Lei wrote:
> Hi
> 
> 2011/10/11 Rafael J. Wysocki <rjw@sisk.pl>:
> > On Sunday, October 09, 2011, tom.leiming@gmail.com wrote:
> >> 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 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?
> 
> OK, I will split it into two.
> 
> >
> >> - idle notification for suspend failure has been removed, so update
> >> comments for it
> >>
> >> Signed-off-by: Ming Lei <ming.lei@canonical.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:
> >
> > 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?

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