[PATCH 0/3] PM / Runtime: rpm_suspend() and rpm_resume() fixes

[PATCH 0/3] PM / Runtime: rpm_suspend() and rpm_resume() fixes

[Rafael J. Wysocki]

Hi,

The following three patches fix minor issues in rpm_suspend() and rpm_resume().
They are on top of the current linux-next branch of the linux-pm.git tree, but
they should apply to the mainline too.

[1/3] - Fix rpm_resume() return value for power.no_callbacks set and active
        parent.
[2/3] - Move the clearing of power.deferred_resume in rpm_suspend() to a better
        place.
[3/3] - Check the PM QoS setting before doing the first power.no_callbacks check.

Thanks,
Rafael

[PATCH 1/3] PM / Runtime: Fix rpm_resume() return value for power.no_callbacks set

[Rafael J. Wysocki]

For devices whose power.no_callbacks flag is set, rpm_resume()
should return 1 if the device's parent is already active, so that
the callers of pm_runtime_get() don't think that they have to wait
for the device to resume (asynchronously) in that case (the core
won't queue up an asynchronous resume in that case, so there's
nothing to wait for anyway).

Modify the code accordingly (and make sure that an idle notification
will be queued up on success, even if 1 is to be returned).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Index: linux/drivers/base/power/runtime.c
===================================================================
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -584,6 +584,7 @@ static int rpm_resume(struct device *dev
 		    || dev->parent->power.runtime_status == RPM_ACTIVE) {
 			atomic_inc(&dev->parent->power.child_count);
 			spin_unlock(&dev->parent->power.lock);
+			retval = 1;
 			goto no_callback;	/* Assume success. */
 		}
 		spin_unlock(&dev->parent->power.lock);
@@ -664,7 +665,7 @@ static int rpm_resume(struct device *dev
 	}
 	wake_up_all(&dev->power.wait_queue);
 
-	if (!retval)
+	if (retval >= 0)
 		rpm_idle(dev, RPM_ASYNC);
 
  out:

[PATCH 2/3] PM / Runtime: Clear power.deferred_resume on success in rpm_suspend()

[Rafael J. Wysocki]

The power.deferred_resume can only be set if the runtime PM status
of device is RPM_SUSPENDING and it should be cleared after its
status has been changed, regardless of whether or not the runtime
suspend has been successful.  However, it only is cleared on suspend
failure, while it may remain set after successful suspend and is
happily leaked to rpm_resume() executed in that case.

That shouldn't happen, so if power.deferred_resume is set in
rpm_suspend() after the status has been changed to RPM_SUSPENDED,
clear it before calling rpm_resume().  Then, it doesn't need to be
cleared before changing the status to RPM_SUSPENDING any more,
because it's always cleared after the status has been changed to
either RPM_SUSPENDED (on success) or RPM_ACTIVE (on failure).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux/drivers/base/power/runtime.c
===================================================================
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -388,7 +388,6 @@ static int rpm_suspend(struct device *de
 		goto repeat;
 	}
 
-	dev->power.deferred_resume = false;
 	if (dev->power.no_callbacks)
 		goto no_callback;	/* Assume success. */
 
@@ -440,6 +439,7 @@ static int rpm_suspend(struct device *de
 	wake_up_all(&dev->power.wait_queue);
 
 	if (dev->power.deferred_resume) {
+		dev->power.deferred_resume = false;
 		rpm_resume(dev, 0);
 		retval = -EAGAIN;
 		goto out;

[PATCH 3/3] PM / Runtime: Check device PM QoS setting before "no callbacks" check

[Rafael J. Wysocki]

If __dev_pm_qos_read_value(dev) returns a negative value,
rpm_suspend() should return -EPERM for dev even if its
power.no_callbacks flag is set.  For this to happen, the device's
power.no_callbacks flag has to be checked after the PM QoS check,
so modify the code accordingly.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux/drivers/base/power/runtime.c
===================================================================
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -388,6 +388,12 @@ static int rpm_suspend(struct device *de
 		goto repeat;
 	}
 
+	if (__dev_pm_qos_read_value(dev) < 0) {
+		/* Negative PM QoS constraint means "never suspend". */
+		retval = -EPERM;
+		goto out;
+	}
+
 	if (dev->power.no_callbacks)
 		goto no_callback;	/* Assume success. */
 
@@ -402,12 +408,6 @@ static int rpm_suspend(struct device *de
 		goto out;
 	}
 
-	if (__dev_pm_qos_read_value(dev) < 0) {
-		/* Negative PM QoS constraint means "never suspend". */
-		retval = -EPERM;
-		goto out;
-	}
-
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
 	if (dev->pm_domain)

Re: [PATCH 1/3] PM / Runtime: Fix rpm_resume() return value for power.no_callbacks set

[Alan Stern]

On Tue, 14 Aug 2012, Rafael J. Wysocki wrote:

> 
> For devices whose power.no_callbacks flag is set, rpm_resume()
> should return 1 if the device's parent is already active, so that
> the callers of pm_runtime_get() don't think that they have to wait
> for the device to resume (asynchronously) in that case (the core
> won't queue up an asynchronous resume in that case, so there's
> nothing to wait for anyway).
> 
> Modify the code accordingly (and make sure that an idle notification
> will be queued up on success, even if 1 is to be returned).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/runtime.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Index: linux/drivers/base/power/runtime.c
> ===================================================================
> --- linux.orig/drivers/base/power/runtime.c
> +++ linux/drivers/base/power/runtime.c
> @@ -584,6 +584,7 @@ static int rpm_resume(struct device *dev
>  		    || dev->parent->power.runtime_status == RPM_ACTIVE) {
>  			atomic_inc(&dev->parent->power.child_count);
>  			spin_unlock(&dev->parent->power.lock);
> +			retval = 1;
>  			goto no_callback;	/* Assume success. */
>  		}
>  		spin_unlock(&dev->parent->power.lock);
> @@ -664,7 +665,7 @@ static int rpm_resume(struct device *dev
>  	}
>  	wake_up_all(&dev->power.wait_queue);
>  
> -	if (!retval)
> +	if (retval >= 0)
>  		rpm_idle(dev, RPM_ASYNC);
>  
>   out:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 2/3] PM / Runtime: Clear power.deferred_resume on success in rpm_suspend()

[Alan Stern]

On Tue, 14 Aug 2012, Rafael J. Wysocki wrote:

> 
> The power.deferred_resume can only be set if the runtime PM status
> of device is RPM_SUSPENDING and it should be cleared after its
> status has been changed, regardless of whether or not the runtime
> suspend has been successful.  However, it only is cleared on suspend
> failure, while it may remain set after successful suspend and is
> happily leaked to rpm_resume() executed in that case.
> 
> That shouldn't happen, so if power.deferred_resume is set in
> rpm_suspend() after the status has been changed to RPM_SUSPENDED,
> clear it before calling rpm_resume().  Then, it doesn't need to be
> cleared before changing the status to RPM_SUSPENDING any more,
> because it's always cleared after the status has been changed to
> either RPM_SUSPENDED (on success) or RPM_ACTIVE (on failure).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/runtime.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux/drivers/base/power/runtime.c
> ===================================================================
> --- linux.orig/drivers/base/power/runtime.c
> +++ linux/drivers/base/power/runtime.c
> @@ -388,7 +388,6 @@ static int rpm_suspend(struct device *de
>  		goto repeat;
>  	}
>  
> -	dev->power.deferred_resume = false;
>  	if (dev->power.no_callbacks)
>  		goto no_callback;	/* Assume success. */
>  
> @@ -440,6 +439,7 @@ static int rpm_suspend(struct device *de
>  	wake_up_all(&dev->power.wait_queue);
>  
>  	if (dev->power.deferred_resume) {
> +		dev->power.deferred_resume = false;
>  		rpm_resume(dev, 0);
>  		retval = -EAGAIN;
>  		goto out;

Basically this comes down to a matter of taste.  I tend to use a "lazy" 
approach, not initializing values until they need to contain a proper 
value (and not worrying if they contain garbage or wrong values at 
times when they won't be used).

But of course, this is fine too.  With this change, you can also
simplify one of the tests in rpm_check_suspend_allowed().

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH 3/3] PM / Runtime: Check device PM QoS setting before "no callbacks" check

[Alan Stern]

On Tue, 14 Aug 2012, Rafael J. Wysocki wrote:

> 
> If __dev_pm_qos_read_value(dev) returns a negative value,
> rpm_suspend() should return -EPERM for dev even if its
> power.no_callbacks flag is set.  For this to happen, the device's
> power.no_callbacks flag has to be checked after the PM QoS check,
> so modify the code accordingly.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/runtime.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Index: linux/drivers/base/power/runtime.c
> ===================================================================
> --- linux.orig/drivers/base/power/runtime.c
> +++ linux/drivers/base/power/runtime.c
> @@ -388,6 +388,12 @@ static int rpm_suspend(struct device *de
>  		goto repeat;
>  	}
>  
> +	if (__dev_pm_qos_read_value(dev) < 0) {
> +		/* Negative PM QoS constraint means "never suspend". */
> +		retval = -EPERM;
> +		goto out;
> +	}
> +
>  	if (dev->power.no_callbacks)
>  		goto no_callback;	/* Assume success. */
>  
> @@ -402,12 +408,6 @@ static int rpm_suspend(struct device *de
>  		goto out;
>  	}
>  
> -	if (__dev_pm_qos_read_value(dev) < 0) {
> -		/* Negative PM QoS constraint means "never suspend". */
> -		retval = -EPERM;
> -		goto out;
> -	}
> -
>  	__update_runtime_status(dev, RPM_SUSPENDING);
>  
>  	if (dev->pm_domain)

Wouldn't it be better to move the test into 
rpm_check_suspend_allowed()?  Then it would apply to idle notifications 
and pm_schedule_suspend().

Alan Stern

[Update][PATCH 3/3] PM / Runtime: Check device PM QoS setting before "no callbacks" check

[Rafael J. Wysocki]

On Wednesday, August 15, 2012, Alan Stern wrote:
> On Tue, 14 Aug 2012, Rafael J. Wysocki wrote:
> 
> > 
> > If __dev_pm_qos_read_value(dev) returns a negative value,
> > rpm_suspend() should return -EPERM for dev even if its
> > power.no_callbacks flag is set.  For this to happen, the device's
> > power.no_callbacks flag has to be checked after the PM QoS check,
> > so modify the code accordingly.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> > ---
> >  drivers/base/power/runtime.c |   12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > Index: linux/drivers/base/power/runtime.c
> > ===================================================================
> > --- linux.orig/drivers/base/power/runtime.c
> > +++ linux/drivers/base/power/runtime.c
> > @@ -388,6 +388,12 @@ static int rpm_suspend(struct device *de
> >  		goto repeat;
> >  	}
> >  
> > +	if (__dev_pm_qos_read_value(dev) < 0) {
> > +		/* Negative PM QoS constraint means "never suspend". */
> > +		retval = -EPERM;
> > +		goto out;
> > +	}
> > +
> >  	if (dev->power.no_callbacks)
> >  		goto no_callback;	/* Assume success. */
> >  
> > @@ -402,12 +408,6 @@ static int rpm_suspend(struct device *de
> >  		goto out;
> >  	}
> >  
> > -	if (__dev_pm_qos_read_value(dev) < 0) {
> > -		/* Negative PM QoS constraint means "never suspend". */
> > -		retval = -EPERM;
> > -		goto out;
> > -	}
> > -
> >  	__update_runtime_status(dev, RPM_SUSPENDING);
> >  
> >  	if (dev->pm_domain)
> 
> Wouldn't it be better to move the test into 
> rpm_check_suspend_allowed()?  Then it would apply to idle notifications 
> and pm_schedule_suspend().

Yes, that makes sense.

Updated patch is appended.

Thanks,
Rafael


---
From: Rafael J. Wysocki <rjw@sisk.pl>
Subject: PM / Runtime: Check device PM QoS setting before "no callbacks"

If __dev_pm_qos_read_value(dev) returns a negative value,
rpm_suspend() should return -EPERM for dev even if its
power.no_callbacks flag is set.  For this to happen, the device's
power.no_callbacks flag has to be checked after the PM QoS check,
so move the PM QoS check to rpm_check_suspend_allowed() (this will
make it cover idle notifications as well as runtime suspend too).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
---
 drivers/base/power/runtime.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Index: linux/drivers/base/power/runtime.c
===================================================================
--- linux.orig/drivers/base/power/runtime.c
+++ linux/drivers/base/power/runtime.c
@@ -147,6 +147,8 @@ static int rpm_check_suspend_allowed(str
 	    || (dev->power.request_pending
 			&& dev->power.request == RPM_REQ_RESUME))
 		retval = -EAGAIN;
+	else if (__dev_pm_qos_read_value(dev) < 0)
+		retval = -EPERM;
 	else if (dev->power.runtime_status == RPM_SUSPENDED)
 		retval = 1;
 
@@ -402,12 +404,6 @@ static int rpm_suspend(struct device *de
 		goto out;
 	}
 
-	if (__dev_pm_qos_read_value(dev) < 0) {
-		/* Negative PM QoS constraint means "never suspend". */
-		retval = -EPERM;
-		goto out;
-	}
-
 	__update_runtime_status(dev, RPM_SUSPENDING);
 
 	if (dev->pm_domain)

Re: [Update][PATCH 3/3] PM / Runtime: Check device PM QoS setting before "no callbacks" check

[Alan Stern]

On Wed, 15 Aug 2012, Rafael J. Wysocki wrote:

> From: Rafael J. Wysocki <rjw@sisk.pl>
> Subject: PM / Runtime: Check device PM QoS setting before "no callbacks"
> 
> If __dev_pm_qos_read_value(dev) returns a negative value,
> rpm_suspend() should return -EPERM for dev even if its
> power.no_callbacks flag is set.  For this to happen, the device's
> power.no_callbacks flag has to be checked after the PM QoS check,
> so move the PM QoS check to rpm_check_suspend_allowed() (this will
> make it cover idle notifications as well as runtime suspend too).
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> ---
>  drivers/base/power/runtime.c |    8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> Index: linux/drivers/base/power/runtime.c
> ===================================================================
> --- linux.orig/drivers/base/power/runtime.c
> +++ linux/drivers/base/power/runtime.c
> @@ -147,6 +147,8 @@ static int rpm_check_suspend_allowed(str
>  	    || (dev->power.request_pending
>  			&& dev->power.request == RPM_REQ_RESUME))
>  		retval = -EAGAIN;
> +	else if (__dev_pm_qos_read_value(dev) < 0)
> +		retval = -EPERM;
>  	else if (dev->power.runtime_status == RPM_SUSPENDED)
>  		retval = 1;
>  
> @@ -402,12 +404,6 @@ static int rpm_suspend(struct device *de
>  		goto out;
>  	}
>  
> -	if (__dev_pm_qos_read_value(dev) < 0) {
> -		/* Negative PM QoS constraint means "never suspend". */
> -		retval = -EPERM;
> -		goto out;
> -	}
> -
>  	__update_runtime_status(dev, RPM_SUSPENDING);
>  
>  	if (dev->pm_domain)

Acked-by: Alan Stern <stern@rowland.harvard.edu>