[PATCH v2 1/3] PM / sleep: Check legacy pm callbacks for direct complete

[PATCH v2 1/3] PM / sleep: Check legacy pm callbacks for direct complete

[Derek Basehore]

This adds checks for legacy pm callbacks when setting no_pm_callbacks.
This fixes an issue where these suspend/resume callbacks were
incorrectly ignored during suspend/resume with direct complete.

Fixes: aa8e54b55947 "PM / sleep: Go direct_complete if driver has..."
Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/base/power/main.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index 6e7c3cc..e0017d9 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1764,8 +1764,10 @@ void device_pm_check_callbacks(struct device *dev)
 {
 	spin_lock_irq(&dev->power.lock);
 	dev->power.no_pm_callbacks =
-		(!dev->bus || pm_ops_is_empty(dev->bus->pm)) &&
-		(!dev->class || pm_ops_is_empty(dev->class->pm)) &&
+		(!dev->bus || (!dev->bus->resume && !dev->bus->suspend &&
+			       pm_ops_is_empty(dev->bus->pm))) &&
+		(!dev->class || (!dev->class->resume && !dev->class->suspend &&
+				 pm_ops_is_empty(dev->class->pm))) &&
 		(!dev->type || pm_ops_is_empty(dev->type->pm)) &&
 		(!dev->pm_domain || pm_ops_is_empty(&dev->pm_domain->ops)) &&
 		(!dev->driver || pm_ops_is_empty(dev->driver->pm));
-- 
2.7.0.rc3.207.g0ac5344

[PATCH v2 2/3] PM / sleep: try to runtime suspend for direct complete

[Derek Basehore]

This tries to runtime suspend devices that are still active for direct
complete. This is for cases such as autosuspend delays which leaves
devices able to runtime suspend but still active. It's beneficial in
this case to runtime suspend the device to take advantage of direct
complete when possible.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
---
 drivers/base/power/main.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
index e0017d9..9693032 100644
--- a/drivers/base/power/main.c
+++ b/drivers/base/power/main.c
@@ -1380,7 +1380,12 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
 		goto Complete;
 
 	if (dev->power.direct_complete) {
-		if (pm_runtime_status_suspended(dev)) {
+		/*
+		 * Check if we're runtime suspended. If not, try to runtime
+		 * suspend for autosuspend cases.
+		 */
+		if (pm_runtime_status_suspended(dev) ||
+		    !pm_runtime_suspend(dev)) {
 			pm_runtime_disable(dev);
 			if (pm_runtime_status_suspended(dev))
 				goto Complete;
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH v2 2/3] PM / sleep: try to runtime suspend for direct complete

[Rafael J. Wysocki]

On Wednesday, February 24, 2016 04:22:27 PM Derek Basehore wrote:
> This tries to runtime suspend devices that are still active for direct
> complete. This is for cases such as autosuspend delays which leaves
> devices able to runtime suspend but still active. It's beneficial in
> this case to runtime suspend the device to take advantage of direct
> complete when possible.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
> ---
>  drivers/base/power/main.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
> index e0017d9..9693032 100644
> --- a/drivers/base/power/main.c
> +++ b/drivers/base/power/main.c
> @@ -1380,7 +1380,12 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>  		goto Complete;
>  
>  	if (dev->power.direct_complete) {
> -		if (pm_runtime_status_suspended(dev)) {
> +		/*
> +		 * Check if we're runtime suspended. If not, try to runtime
> +		 * suspend for autosuspend cases.
> +		 */
> +		if (pm_runtime_status_suspended(dev) ||
> +		    !pm_runtime_suspend(dev)) {
>  			pm_runtime_disable(dev);
>  			if (pm_runtime_status_suspended(dev))
>  				goto Complete;
> 

This can be done somewhat simpler, because pm_runtime_suspend() will check
if the device has already been suspended:

	ret = pm_runtime_suspend(dev);
	if (ret >= 0) {

and you don't need the extra pm_runtime_status_suspended(dev) check.

Thanks,
Rafael

Re: [PATCH v2 2/3] PM / sleep: try to runtime suspend for direct complete

[dbasehore .]

On Sat, Feb 27, 2016 at 5:31 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, February 24, 2016 04:22:27 PM Derek Basehore wrote:
>> This tries to runtime suspend devices that are still active for direct
>> complete. This is for cases such as autosuspend delays which leaves
>> devices able to runtime suspend but still active. It's beneficial in
>> this case to runtime suspend the device to take advantage of direct
>> complete when possible.
>>
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
>> ---
>>  drivers/base/power/main.c | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/power/main.c b/drivers/base/power/main.c
>> index e0017d9..9693032 100644
>> --- a/drivers/base/power/main.c
>> +++ b/drivers/base/power/main.c
>> @@ -1380,7 +1380,12 @@ static int __device_suspend(struct device *dev, pm_message_t state, bool async)
>>               goto Complete;
>>
>>       if (dev->power.direct_complete) {
>> -             if (pm_runtime_status_suspended(dev)) {
>> +             /*
>> +              * Check if we're runtime suspended. If not, try to runtime
>> +              * suspend for autosuspend cases.
>> +              */
>> +             if (pm_runtime_status_suspended(dev) ||
>> +                 !pm_runtime_suspend(dev)) {
>>                       pm_runtime_disable(dev);
>>                       if (pm_runtime_status_suspended(dev))
>>                               goto Complete;
>>
>
> This can be done somewhat simpler, because pm_runtime_suspend() will check
> if the device has already been suspended:
>
>         ret = pm_runtime_suspend(dev);
>         if (ret >= 0) {
>
> and you don't need the extra pm_runtime_status_suspended(dev) check.
>
> Thanks,
> Rafael
>

I'll look into simplifying it for the next patch set.

[PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[Derek Basehore]

This allows scsi devices to remain runtime suspended for system
suspend. Since runtime suspend is stricter than system suspend
callbacks, this is just returning a positive number for the prepare
callback.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
---
 drivers/scsi/scsi_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index b44c1bb..7af76ad 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -178,7 +178,7 @@ static int scsi_bus_prepare(struct device *dev)
 		/* Wait until async scanning is finished */
 		scsi_complete_async_scans();
 	}
-	return 0;
+	return 1;
 }
 
 static int scsi_bus_suspend(struct device *dev)
-- 
2.7.0.rc3.207.g0ac5344

Re: [PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[Mika Westerberg]

On Wed, Feb 24, 2016 at 04:22:28PM -0800, Derek Basehore wrote:
> This allows scsi devices to remain runtime suspended for system
> suspend. Since runtime suspend is stricter than system suspend
> callbacks, this is just returning a positive number for the prepare
> callback.

AFAICT SCSI layer already leaves devices runtime suspended during system
suspend (see scsi_bus_suspend_common()). What's the benefit using
direct_complete over the current implementation?

> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
> ---
>  drivers/scsi/scsi_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index b44c1bb..7af76ad 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -178,7 +178,7 @@ static int scsi_bus_prepare(struct device *dev)
>  		/* Wait until async scanning is finished */
>  		scsi_complete_async_scans();
>  	}
> -	return 0;
> +	return 1;
>  }
>  
>  static int scsi_bus_suspend(struct device *dev)
> -- 
> 2.7.0.rc3.207.g0ac5344

Re: [PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[dbasehore .]

A device is not able to use direct complete if its children do not
also use direct complete. Even though the SCSI layer leaves devices
runtime suspended, the way it does it still prevents its parent from
using direct complete.

On Fri, Feb 26, 2016 at 11:26 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Feb 24, 2016 at 04:22:28PM -0800, Derek Basehore wrote:
>> This allows scsi devices to remain runtime suspended for system
>> suspend. Since runtime suspend is stricter than system suspend
>> callbacks, this is just returning a positive number for the prepare
>> callback.
>
> AFAICT SCSI layer already leaves devices runtime suspended during system
> suspend (see scsi_bus_suspend_common()). What's the benefit using
> direct_complete over the current implementation?
>
>> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
>> Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
>> ---
>>  drivers/scsi/scsi_pm.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index b44c1bb..7af76ad 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -178,7 +178,7 @@ static int scsi_bus_prepare(struct device *dev)
>>               /* Wait until async scanning is finished */
>>               scsi_complete_async_scans();
>>       }
>> -     return 0;
>> +     return 1;
>>  }
>>
>>  static int scsi_bus_suspend(struct device *dev)
>> --
>> 2.7.0.rc3.207.g0ac5344

Re: [PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[Mika Westerberg]

On Sat, Feb 27, 2016 at 12:10:03AM -0800, dbasehore . wrote:
> A device is not able to use direct complete if its children do not
> also use direct complete. Even though the SCSI layer leaves devices
> runtime suspended, the way it does it still prevents its parent from
> using direct complete.

Okay.

Do you need to provide ->complete() hook that then resumes the device
when system is resumed (since the device resume hook is never called
when direct_complete is set)? Along the lines of
pm_complete_with_resume_check().

Re: [PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[dbasehore .]

On Sat, Feb 27, 2016 at 12:38 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Sat, Feb 27, 2016 at 12:10:03AM -0800, dbasehore . wrote:
>> A device is not able to use direct complete if its children do not
>> also use direct complete. Even though the SCSI layer leaves devices
>> runtime suspended, the way it does it still prevents its parent from
>> using direct complete.
>
> Okay.
>
> Do you need to provide ->complete() hook that then resumes the device
> when system is resumed (since the device resume hook is never called
> when direct_complete is set)? Along the lines of
> pm_complete_with_resume_check().

That's an interesting question. Part of direct complete is to leave
the device runtime suspended even after the system resumes if
possible. The comments in pm_complete_with_resume_check indicate that
the firmware may resume a device. I could see this happening with some
kind of SCSI device.

If this is possible, are we able to put the device back into a
consistent state (runtime suspended) by calling suspend for the scsi
device? If not, we might need to use pm_complete_with_resume_check for
the complete callback.

Re: [PATCH v2 3/3] scsi: allow scsi devices to use direct complete

[Mika Westerberg]

On Sat, Feb 27, 2016 at 01:07:06AM -0800, dbasehore . wrote:
> That's an interesting question. Part of direct complete is to leave
> the device runtime suspended even after the system resumes if
> possible. The comments in pm_complete_with_resume_check indicate that
> the firmware may resume a device. I could see this happening with some
> kind of SCSI device.
> 
> If this is possible, are we able to put the device back into a
> consistent state (runtime suspended) by calling suspend for the scsi
> device? If not, we might need to use pm_complete_with_resume_check for
> the complete callback.

To me the latter option sounds safer.

I tried this series (as I'm interested in AHCI host controller runtime
PM) and noticed possible issue.

I have following two additional patches from linux-next applied to get
system resume working (otherwise resume is never finished):

  d07ab6d11477 block: Add blk_set_runtime_active()
  356fd2663cff scsi: Set request queue runtime PM status back to active on resume

Then I do this:

  # echo auto > /sys/block/sda/device/power/control
  # echo 5000 > /sys/block/sda/device/power/autosuspend_delay_ms

  # echo auto > /sys/bus/pci/devices/0000\:00\:12.0/ata1/power/control

The last line unblocks runtime PM from the parent device.

After 5 seconds of idle I see that the disk gets runtime suspended:

  [  566.858971] sd 0:0:0:0: [sda] Synchronizing SCSI cache
  [  567.049288] sd 0:0:0:0: [sda] Stopping disk

Now suspend the machine:

  # rtcwake -s10 -mmem

Once the system resumes I get this:

  [  668.879149] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
  [  668.927319] ata1.00: configured for UDMA/133
  [  669.392246] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
  [  669.440427] ata1.00: configured for UDMA/133
  [  669.445352] sd 0:0:0:0: [sda] Starting disk
  [  669.464647] sda: detected capacity change from 0 to 240057409536
  [  669.482596] sda: detected capacity change from 0 to 240057409536

It looks like we get resumed two times. If I have this patch reverted I
can see it happening only once. Also with the patch applied and if the
parent device has runtime PM blocked, it happens only once.