Re: REQ_PM vs REQ_TYPE_PM_RESUME

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/06/2014 10:40 PM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/6/2014 4:15 AM, Aaron Lu wrote:
>> My guess why it doesn't work for you is that, when you call 
>> blk_pre_runtime_suspend in sd_resume_work, there are requests left
>> in the queue so that call will simply fail, it's not meant to be
>> used that way.
> 
> There should be no other requests during the system resume callback.

There can be requests left when system resume callback is invoked, but
it's not always the case and depends on what you are doing before system
suspend.

> 
>> It seems you are making use of runtime PM to speed up disk resume,
>> if that is the case, I think we can simply make sure the disk's
>> block queue is put into the same state as runtime suspended and
>> then mark it as runtime suspended during system suspend phase; on
>> system resume, call
> 
> I think I tried that and it didn't work; when it is runtime suspended
> when the system suspends, it's no longer runtime suspended when the
> system resume function was called.  Hence why I'm using the

We can modify the device's system resume callback. To better illustrate
the idea, I just made two patches to do this and I did some quick tests
and didn't find anything wrong.

The two patches are here.

From: Aaron Lu <aaron.lu@intel.com>
Date: Tue, 7 Jan 2014 15:02:13 +0800
Subject: [PATCH 1/2] SCSI: pm: make use of runtime PM for SCSI device

To make system resume fast, modify SCSI PM callbacks so that if
CONFIG_PM_RUNTIME is set, during a system suspend transition, make the
disk device's status exactly the same as runtime suspended, i.e. drain
its request queue and set its request queue's status to RPM_SUSPENDED,
so that during system resume phase, instead of resuming the device
synchronously, we can relay the resume operation to runtime PM framework
by calling pm_request_resume to take advantage of the block layer
runtime PM.

The simplest way to achieve this would be to use the bus' runtime suspend
callback for system suspend callback, but for sr driver, it will refuse
to enter runtime suspend state if there is media inside. This is obviously
not acceptable for system suspend, so instead of using driver's runtime
suspend callback, we keep using driver's system suspend callback(which
is the same for sd and doesn't matter for sr). In addition to drain device's
request queue and set proper runtime status for its request queue, we will
also set the device's runtime status accordingly in its system suspend
callback.

This is part 1, we will also need to do the same thing for the disk's
host, i.e. ATA port on PCs. The next patch will handle just that.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_lib.c  | 11 +++++++++
 drivers/scsi/scsi_pm.c   | 60 +++++++++++++++++++++++++++++++++++++++---------
 drivers/scsi/scsi_priv.h |  3 +++
 3 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d5f050..2b490813d5ed 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2433,6 +2433,17 @@ void scsi_device_resume(struct scsi_device *sdev)
 }
 EXPORT_SYMBOL(scsi_device_resume);
 
+#ifdef CONFIG_PM_RUNTIME
+void scsi_device_drain_queue(struct scsi_device *sdev)
+{
+	scsi_run_queue(sdev->request_queue);
+	while (sdev->request_queue->nr_pending) {
+		msleep_interruptible(200);
+		scsi_run_queue(sdev->request_queue);
+	}
+}
+#endif
+
 static void
 device_quiesce_fn(struct scsi_device *sdev, void *data)
 {
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 001e9ceda4c3..4f3fbd91c396 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -16,6 +16,24 @@
 
 #include "scsi_priv.h"
 
+#ifdef CONFIG_PM_RUNTIME
+static int sdev_runtime_suspend_common(struct device *dev,
+				       int (*cb)(struct device *))
+{
+	struct scsi_device *sdev = to_scsi_device(dev);
+	int err;
+
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	if (cb)
+		err = cb(dev);
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
+	return err;
+}
+#endif
+
 #ifdef CONFIG_PM_SLEEP
 
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
@@ -95,6 +113,35 @@ static int scsi_bus_prepare(struct device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int sdev_system_suspend(struct device *dev, int (*cb)(struct device *))
+{
+	scsi_device_drain_queue(to_scsi_device(dev));
+	return sdev_runtime_suspend_common(dev, cb);
+}
+
+static int scsi_bus_suspend(struct device *dev)
+{
+	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	int err = 0;
+
+	if (scsi_is_sdev_device(dev))
+		err = sdev_system_suspend(dev, pm ? pm->suspend : NULL);
+
+	if (!err) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return err;
+}
+
+static int scsi_bus_resume(struct device *dev)
+{
+	return pm_request_resume(dev);
+}
+#else
 static int scsi_bus_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
@@ -106,6 +153,7 @@ static int scsi_bus_resume(struct device *dev)
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 	return scsi_bus_resume_common(dev, pm ? pm->resume : NULL);
 }
+#endif
 
 static int scsi_bus_freeze(struct device *dev)
 {
@@ -148,17 +196,7 @@ static int scsi_bus_restore(struct device *dev)
 static int sdev_runtime_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	struct scsi_device *sdev = to_scsi_device(dev);
-	int err;
-
-	err = blk_pre_runtime_suspend(sdev->request_queue);
-	if (err)
-		return err;
-	if (pm && pm->runtime_suspend)
-		err = pm->runtime_suspend(dev);
-	blk_post_runtime_suspend(sdev->request_queue, err);
-
-	return err;
+	return sdev_runtime_suspend_common(dev, pm->runtime_suspend);
 }
 
 static int scsi_runtime_suspend(struct device *dev)
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index f079a598bed4..fdd3a3a04eb4 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -90,6 +90,9 @@ extern void scsi_run_host_queues(struct Scsi_Host *shost);
 extern struct request_queue *scsi_alloc_queue(struct scsi_device *sdev);
 extern int scsi_init_queue(void);
 extern void scsi_exit_queue(void);
+#ifdef CONFIG_PM_RUNTIME
+extern void scsi_device_drain_queue(struct scsi_device *sdev);
+#endif
 struct request_queue;
 struct request;
 extern struct kmem_cache *scsi_sdb_cache;
-- 
1.8.4.2


From: Aaron Lu <aaron.lu@intel.com>
Date: Tue, 7 Jan 2014 15:14:09 +0800
Subject: [PATCH 2/2] ata: pm: make use of runtime PM for ata port

To realize fast resume for hard disks, the ata port's device will also
need to make use of runtime PM in its system resume callback.

Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/ata/libata-core.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1393a5890ed5..4f92a7834dd1 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5396,10 +5396,18 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
 
 static int ata_port_suspend(struct device *dev)
 {
+	int err;
+
 	if (pm_runtime_suspended(dev))
 		return 0;
 
-	return ata_port_suspend_common(dev, PMSG_SUSPEND);
+	err = ata_port_suspend_common(dev, PMSG_SUSPEND);
+	if (!err) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+	return err;
 }
 
 static int ata_port_do_freeze(struct device *dev)
@@ -5432,6 +5440,12 @@ static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 	return __ata_port_resume_common(ap, mesg, NULL);
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int ata_port_resume(struct device *dev)
+{
+	return pm_request_resume(dev);
+}
+#else
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
@@ -5445,6 +5459,7 @@ static int ata_port_resume(struct device *dev)
 
 	return rc;
 }
+#endif
 
 /*
  * For ODDs, the upper layer will poll for media change every few seconds,
-- 
1.8.4.2

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 2:49 AM, Aaron Lu wrote:
> We can modify the device's system resume callback. To better
> illustrate the idea, I just made two patches to do this and I did
> some quick tests and didn't find anything wrong.

That misses one key aspect I was trying to capture: it always leaves
the disk runtime suspended after a resume.  If the disk spins up on
its own, as most ata disks do, then the runtime status doesn't
correctly reflect the actual state of the disk.  This means that
applications that delay activities due to the runtime pm status to
avoid waking a disk won't run, and any runtime autosuspend won't kick
in to actually put the disk back to sleep.

To do that, you need to be able to issue a REQUEST SENSE and
conditionally resume the device.  Of course, you can't do that unless
you first get it into a transitional state, but you can't just request
a resume and then conditionally fail it.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzBQiAAoJEI5FoCIzSKrwH9UH/3Z3TfKKKYhNsjr26YsKOge2
EQzbcFCg4H5irnKL/olR5fnRKpZYi1Er2ydV6zsHdcCJLFQoqvzVE4ZInl42STI0
IWQ65n5q+U1OaZY+ttGkOBjixi5VIlkb9izZVbzBTi4n5cLEDwyQsE4Rgd9STwjm
gPkNrmGNGPUoY+4O6bLHu0/WvwTX3L/OgxcSsQd1gsdCX3qZzB+UIzfM31W9/4Yl
3sP/tN8mBYcpqR9jIpw2u7m0XEe9Wc71Nepdv2+6sT5uZJ5knTY6epH5Of4FJsyp
CxslFXf6Jb2FZaey8EJB1ocABnePYluQNHrpdRO4gjY+Au/mtzk15tJjoYx3WXg=
=VlKV
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/07/2014 10:50 PM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/7/2014 2:49 AM, Aaron Lu wrote:
>> We can modify the device's system resume callback. To better
>> illustrate the idea, I just made two patches to do this and I did
>> some quick tests and didn't find anything wrong.
> 
> That misses one key aspect I was trying to capture: it always leaves
> the disk runtime suspended after a resume.  If the disk spins up on

You mean you want to leave the disk runtime suspended after a system
resume and in the meantime make sure the disk is indeed not spun up?

-Aaron

> its own, as most ata disks do, then the runtime status doesn't
> correctly reflect the actual state of the disk.  This means that
> applications that delay activities due to the runtime pm status to
> avoid waking a disk won't run, and any runtime autosuspend won't kick
> in to actually put the disk back to sleep.
> 
> To do that, you need to be able to issue a REQUEST SENSE and
> conditionally resume the device.  Of course, you can't do that unless
> you first get it into a transitional state, but you can't just request
> a resume and then conditionally fail it.
> 
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2.0.17 (MingW32)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQEcBAEBAgAGBQJSzBQiAAoJEI5FoCIzSKrwH9UH/3Z3TfKKKYhNsjr26YsKOge2
> EQzbcFCg4H5irnKL/olR5fnRKpZYi1Er2ydV6zsHdcCJLFQoqvzVE4ZInl42STI0
> IWQ65n5q+U1OaZY+ttGkOBjixi5VIlkb9izZVbzBTi4n5cLEDwyQsE4Rgd9STwjm
> gPkNrmGNGPUoY+4O6bLHu0/WvwTX3L/OgxcSsQd1gsdCX3qZzB+UIzfM31W9/4Yl
> 3sP/tN8mBYcpqR9jIpw2u7m0XEe9Wc71Nepdv2+6sT5uZJ5knTY6epH5Of4FJsyp
> CxslFXf6Jb2FZaey8EJB1ocABnePYluQNHrpdRO4gjY+Au/mtzk15tJjoYx3WXg=
> =VlKV
> -----END PGP SIGNATURE-----
> 

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 08:03 PM, Aaron Lu wrote:
> You mean you want to leave the disk runtime suspended after a
> system resume and in the meantime make sure the disk is indeed not
> spun up?

Yep.  If it is spun up, then the runtime status should be updated to
reflect that, otherwise it tricks user space programs into avoiding
doing IO to the disk for fear of waking it, and prevents the runtime
autosuspend timer from kicking in.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzKbjAAoJEI5FoCIzSKrwhEsH/3WXEJm2+uPR2zIeGVvZ/I2X
yu0KpcMto0Ox/v2htII/SjQsmhAPbyphTcbJeZJTmbA5EZmNixZXrZAS1vy3vK06
n2Azmywuf0mihfZV1ORg1GuwjgqSSW9MNRw+IuJ7NkxtOZWvWFRNvpcVdyfcig6d
26CP0drDH/j9VL5L1UFWensESswAWV9l1Ht45FUemQw5QDQK6hwUpOAfhbnurz06
OCOs4QRC+O/zWyxzMVKQtQnCEd5tRnKQq0K6d2ZmCKe368KNUrKZ+sk3SPabYtjI
3CEv1+ytG8VajzxJ0cj4TuePK2O+lcd0TWddARJqxZg7o/JYWAyN4yMFoPsAUPo=
=oDcD
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/08/2014 09:16 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 08:03 PM, Aaron Lu wrote:
>> You mean you want to leave the disk runtime suspended after a
>> system resume and in the meantime make sure the disk is indeed not
>> spun up?
> 
> Yep.  If it is spun up, then the runtime status should be updated to
> reflect that, otherwise it tricks user space programs into avoiding
> doing IO to the disk for fear of waking it, and prevents the runtime
> autosuspend timer from kicking in.

The ATA and SCSI devices are all resumed in my patches, notice there is
a single pm_request_resume call in both ATA and SCSI's system resume
callback, so the runtime status and the disk's state is synced.
The pm_request_resume call is asynchronous to the system resume, so it
doesn't block system resume.

But I see your point, my patch will not achieve that, it can only speed
up S3 for a typical PC with a traditional disk. I can omit the
pm_request_resume call in the system resume callback, but then if the
disk is spun up by itself, then the runtime status indeed doesn't
reflect the actual state. I suppose for SATA controllers that support
Staggered Spin-up wouldn't do this?

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 08:32 PM, Aaron Lu wrote:
> The ATA and SCSI devices are all resumed in my patches, notice
> there is a single pm_request_resume call in both ATA and SCSI's
> system resume callback, so the runtime status and the disk's state
> is synced. The pm_request_resume call is asynchronous to the system
> resume, so it doesn't block system resume.
> 
> But I see your point, my patch will not achieve that, it can only
> speed up S3 for a typical PC with a traditional disk. I can omit
> the pm_request_resume call in the system resume callback, but then
> if the disk is spun up by itself, then the runtime status indeed
> doesn't reflect the actual state. I suppose for SATA controllers
> that support Staggered Spin-up wouldn't do this?

Ahh, yes, the point of my patches was to avoid waking a disk at all if
possible, and avoid blocking on it otherwise.  Todd Brandt's patches
just backgrounded the resume.

As far as I can tell, the AHCI staggered spinup feature is only a hint
to the libata driver that it should not probe all disks in parallel.
The way to get an ATA disk to not spin itself up is by enabling the
Power on in Standby feature, either through hdparm, or via a jumper,
and it seems WD drives only support the jumper method.  Once enabled,
a drive may chose to automatically spin up when given a command that
requires it to be spinning, or it can opt to require an explicit SET
FEATURES command to spin up.  libata issues an IDENTIFY DEVICE on
resume to find out of the drive requires this command, and issues it
if so.  One of the other patches in my set fixes libata to avoid doing
this in the suspend path, and defer it to the first time a command is
issued.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzK+MAAoJEI5FoCIzSKrwE5gH/3zpkOR1u+W1/kw3GFLO1YHH
kA2g9VlBMoisiUGLltAuvZYN8zALhWvH3QrTIAvAxq/DjlRQ5ZyBSi3g56swsrHg
ILdx3XW9wuPLSxpWLaiZ/sowTvmrWKSYbyUpxdkDJizCXkg5R3J4LuQ3OpLSSLRh
a6IYMas6l74+xq3wp/eHTE7ofAeoN/jJmT4slUFbzgILMKKEZJQ3wLdjM2uy1d2l
ip3anDOKXHqjrTW4QSkj8piMpR4LBsEpWpMPW9fjYhQe54Hpqv4hwn6vuXEg9SKu
TrwjiH2qb4Ro9twQMUrfF2/r4Ov9swPI1r4EL/bvJ7lJSJ+9c5fRIvObg5Hdaa8=
=woDI
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/08/2014 09:53 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 08:32 PM, Aaron Lu wrote:
>> The ATA and SCSI devices are all resumed in my patches, notice
>> there is a single pm_request_resume call in both ATA and SCSI's
>> system resume callback, so the runtime status and the disk's state
>> is synced. The pm_request_resume call is asynchronous to the system
>> resume, so it doesn't block system resume.
>>
>> But I see your point, my patch will not achieve that, it can only
>> speed up S3 for a typical PC with a traditional disk. I can omit
>> the pm_request_resume call in the system resume callback, but then
>> if the disk is spun up by itself, then the runtime status indeed
>> doesn't reflect the actual state. I suppose for SATA controllers
>> that support Staggered Spin-up wouldn't do this?
> 
> Ahh, yes, the point of my patches was to avoid waking a disk at all if
> possible, and avoid blocking on it otherwise.  Todd Brandt's patches
> just backgrounded the resume.
> 
> As far as I can tell, the AHCI staggered spinup feature is only a hint
> to the libata driver that it should not probe all disks in parallel.

I thought that feature is used to control if a disk should be spun up
once powered from the host side.

> The way to get an ATA disk to not spin itself up is by enabling the
> Power on in Standby feature, either through hdparm, or via a jumper,

Too bad for a jumper, that's beyond our control. And about the hdparm,
does the setting survive a power cycle?

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 09:11 PM, Aaron Lu wrote:
> I thought that feature is used to control if a disk should be spun
> up once powered from the host side.

That *is* what it sounds like, only ATA disks either spin up as soon
as power is applied, or wait for an actual command issued by higher
layers.  There doesn't seem to be any low level SATA mechanism to spin
the drive up that the SSS feature could toggle, so it seems to simply
be a hint to libata, and possibly the bios.

> Too bad for a jumper, that's beyond our control. And about the
> hdparm, does the setting survive a power cycle?

Yes, otherwise there wouldn't be much point to a setting that only
matters at power on ;)


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzLXEAAoJEI5FoCIzSKrwhVwH/2amZwCu1WIFiPswU2ydIZhR
lGL0+0C9hKb4O+FEahkLMYsX/bouxtUs3HOcAJOlmmAFZC5yhczIBijcuIKjZyW5
HAv4Sfw1xIjC6N5pMtNnnG8SUORDXy4cbMDS4+poC0aKIWw47GMNin/Uqav4Wn90
3U9nbUm7sBZ6zSLpDkenB5kZKUYmMaziwEb1IwBvxrRdnS3X5B9L3WuuCjheQoF8
YhSFdc/0OGdRRiY/oaZwizRV7bJO2sg99qRoSVIa8ZnL5v9HzT0MCVq9aUBXNLUI
eTyv0O1rkgW5lFk+4aIhJUPSb52F4lNwBmNVINIzLCehpy/oT1DVZN3sL+ykBeI=
=DJOH
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/08/2014 10:19 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 09:11 PM, Aaron Lu wrote:
>> I thought that feature is used to control if a disk should be spun
>> up once powered from the host side.
> 
> That *is* what it sounds like, only ATA disks either spin up as soon
> as power is applied, or wait for an actual command issued by higher
> layers.  There doesn't seem to be any low level SATA mechanism to spin
> the drive up that the SSS feature could toggle, so it seems to simply
> be a hint to libata, and possibly the bios.

I think the host controller can decide not to power all its ports, only
when a specific reg in the port's memory range is set, it will give
power to that port and thus spin up the drive. Makes sense?

> 
>> Too bad for a jumper, that's beyond our control. And about the
>> hdparm, does the setting survive a power cycle?
> 
> Yes, otherwise there wouldn't be much point to a setting that only
> matters at power on ;)

Oh, of course, my stupid :-)
Then I suddenly think my patches can kind of work - let's say we have
done the hdparm setting thing before suspend and the disk will be spun
up in standby mode next time it is powered. Then during system resume
phase, remove the pm_request_resume call in both SCSI and ATA's system
resume callback,
  - if the disk is powered, it will be in standby mode and its runtime
    status is RPM_SUSPENDED, match its real status(sort of);
  - if the disk is not powered due to some host feature or whatever, it
    will be in unpowered mode and its runtime status is RPM_SUSPENDED,
    still match its real status.

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 09:36 PM, Aaron Lu wrote:
> I think the host controller can decide not to power all its ports,
> only when a specific reg in the port's memory range is set, it will
> give power to that port and thus spin up the drive. Makes sense?

The port doesn't supply power to the drive, it gets it straight from
the system PSU.

> Oh, of course, my stupid :-) Then I suddenly think my patches can
> kind of work - let's say we have done the hdparm setting thing
> before suspend and the disk will be spun up in standby mode next
> time it is powered. Then during system resume phase, remove the
> pm_request_resume call in both SCSI and ATA's system resume
> callback, - if the disk is powered, it will be in standby mode and
> its runtime status is RPM_SUSPENDED, match its real status(sort
> of); - if the disk is not powered due to some host feature or
> whatever, it will be in unpowered mode and its runtime status is
> RPM_SUSPENDED, still match its real status.

Right, but if the disk is a run of the mill ATA disk not configured
for power up in standby, then you end up with runtime pm saying that
it is suspended, when in fact, it spun up on its own and is sitting
there waiting for commands.

The PuiS setting isn't something we can or want to twiggle on our own
during suspend, that's an admin decision that they set more or less
permanently either with hdparm or the hardware jumper.  We just need
to detect what the drive has done and update the runtime pm status to
match.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzOEaAAoJEI5FoCIzSKrwIVoH/1sszQsK1buyg9hDvemd84m6
EMkMUsab4qYZlxGcQpnUUlJbQpGKnhDXjxBstjD8zfnC6WQfOCySTqkqBzZqEXzE
QEt5IV7mWn43tGbu4pyYlw4SrEOmOmmYJxl5yh033MAPNsP/rhToXZoEPOTRCro4
GdkZpxx0A9Y/rnzLN29RoFw41T5G4aG0O7FyTuZGPW/uWhhdUqxpUQt7ACCD+fdD
GaHWf2WInU7vSrDcg6daxvarqQ8GJavc1rafM45EkGMCzGwRhvIR+PCBk8E9t1qA
eB/1b9q8DiBJVCiMxcZVOLY8PY0bm1eBRRqhMef0l7Ppvl8N23f84o7tcN57lWY=
=RCPv
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/08/2014 01:24 PM, Phillip Susi wrote:
> On 01/07/2014 09:36 PM, Aaron Lu wrote:
>> Oh, of course, my stupid :-) Then I suddenly think my patches can
>> kind of work - let's say we have done the hdparm setting thing
>> before suspend and the disk will be spun up in standby mode next
>> time it is powered. Then during system resume phase, remove the
>> pm_request_resume call in both SCSI and ATA's system resume
>> callback, - if the disk is powered, it will be in standby mode and
>> its runtime status is RPM_SUSPENDED, match its real status(sort
>> of); - if the disk is not powered due to some host feature or
>> whatever, it will be in unpowered mode and its runtime status is
>> RPM_SUSPENDED, still match its real status.
> 
> Right, but if the disk is a run of the mill ATA disk not configured
> for power up in standby, then you end up with runtime pm saying that
> it is suspended, when in fact, it spun up on its own and is sitting
> there waiting for commands.
> 
> The PuiS setting isn't something we can or want to twiggle on our own
> during suspend, that's an admin decision that they set more or less
> permanently either with hdparm or the hardware jumper.  We just need

OK, I see.

> to detect what the drive has done and update the runtime pm status to
> match.

Then I would say we just keep the pm_request_resume call in their system
resume callback. For drives that have that cool feature turned on, it
will be in runtime active state while it's actually in standby mode, not
a big deal since it should go to runtime suspended state once the
autosuspend timer is due(the runtime PM core will schedule an idle call
for newly resumed device); For drives that don't have that feature turned
on, it will be in runtime active state while it's actually spun up, no
problem here.

For the PuiS set drive, there is a time window the drive's power mode
doesn't agree with its runtime status, the window is the length of the
autosuspend time interval. Not a big deal I suppose, considering that
time interval isn't eternal? And I wouldn't say the runtime status for
the drive is wrong even in this case, since the runtime status is a
reflection of whether the device is in use or not from kernel's view,
instead of whether it is in a low power mode. But I agree we better
align them.

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2014 2:00 AM, Aaron Lu wrote:
> Then I would say we just keep the pm_request_resume call in their
> system resume callback. For drives that have that cool feature
> turned on, it will be in runtime active state while it's actually
> in standby mode, not

No, it won't since the runtime resume starts the drive.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzadoAAoJEI5FoCIzSKrwQWsIAKt/F+eiYYrxxjx58hFCwLxV
3E+CDiuf+pTyUUqPFgcGjyNrfoBO2nWXiCyg5XZ//pjPwbD4GenwSajgUIvKRw/6
0mDw4bb4CyU9/vqVKwHUHfM2jgsAuK9VFn+1fSomODy6B9ZYX6pDPr3jw5S7kDko
nDaYhBX9L2/AE4oPo2qPLpAPMxiYFD9qIi8LcfW/Ha2Av7AneIyTXEvFZzyeDv2K
WoH/KF59snFz9t4wwwfdqu1l1w93b+Tcn2zCpFBaA63TyrJKq4qpx9/W+INgBGol
HKEyv1vh5UkaVSvffiFE/2b1lR4n0hL7LVMe1egazAlNPcsjXdBZv1amYJ7NltI=
=N8WO
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Tue, 7 Jan 2014, Aaron Lu wrote:

> From: Aaron Lu <aaron.lu@intel.com>
> Date: Tue, 7 Jan 2014 15:02:13 +0800
> Subject: [PATCH 1/2] SCSI: pm: make use of runtime PM for SCSI device
> 
> To make system resume fast, modify SCSI PM callbacks so that if
> CONFIG_PM_RUNTIME is set, during a system suspend transition, make the
> disk device's status exactly the same as runtime suspended, i.e. drain
> its request queue and set its request queue's status to RPM_SUSPENDED,
> so that during system resume phase, instead of resuming the device
> synchronously, we can relay the resume operation to runtime PM framework
> by calling pm_request_resume to take advantage of the block layer
> runtime PM.

This doesn't seem like a good idea.  The way to speed up resumes is to
allow sd's resume routine to return while the disk is still spinning up
(i.e., make the spin-up asynchronous).  There already have been patches
submitted to do this; I don't know what happened to them.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 10:25 AM, Alan Stern wrote:
> This doesn't seem like a good idea.  The way to speed up resumes is
> to allow sd's resume routine to return while the disk is still
> spinning up (i.e., make the spin-up asynchronous).  There already
> have been patches submitted to do this; I don't know what happened
> to them.

Sure, if that is your *only* goal.  I also want the disk to not spin
up *at all* if possible.  There's no sense spinning up all of your
disks every time you resume when you very rarely access some of them.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzCCgAAoJEI5FoCIzSKrwkMAH/3Fs/1tdkCUNtj32PjvD/C2O
qgJwkXKtJmThL9+8NK5lNLxWt+buJ9HBozoeKs2AkBIsRpxdXZb6FnutQHhOSIBQ
vqgwKoGvKQUc7bj0sg2WUVckYkZcl2hs54PIZgaAj8VwcJZE7XTNo+BeGlyslyi+
isG+BvGInBkhJ7BsR0nC05Sytrb6F6xYkOkV5hn2PjvPNhxpw9dKlVLGfO9qNFcJ
irazJwVQ2k49y7IGHS+K0NLGv45pBEPRcirwTaxfH0IIVCmDPeQk86SmpmeS+TSO
o3tFgUC7JQjeH2yg0YXkIEGZGGycv57H7Gf+hAOL9mOOD3CD0D4TagbMZP2SOIU=
=gM81
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Tue, 7 Jan 2014, Phillip Susi wrote:

> On 1/7/2014 10:25 AM, Alan Stern wrote:
> > This doesn't seem like a good idea.  The way to speed up resumes is
> > to allow sd's resume routine to return while the disk is still
> > spinning up (i.e., make the spin-up asynchronous).  There already
> > have been patches submitted to do this; I don't know what happened
> > to them.
> 
> Sure, if that is your *only* goal.  I also want the disk to not spin
> up *at all* if possible.  There's no sense spinning up all of your
> disks every time you resume when you very rarely access some of them.

Okay, that's a different matter.  There's a much simpler way to 
accomplish this.  The patch below will avoid spinning up drives that 
were already in runtime suspend when the system sleep started.  
(If a drive wasn't in runtime suspend then presumably it was used 
recently; therefore it's likely to be used again in the near future and 
so it _should_ be spun up.)

Warning: This patch is completely untested.  I didn't even try to
compile it.  Still, it should give you a good idea as to what is really
needed here.

Alan Stern



Index: usb-3.13/drivers/scsi/scsi_pm.c
===================================================================
--- usb-3.13.orig/drivers/scsi/scsi_pm.c
+++ usb-3.13/drivers/scsi/scsi_pm.c
@@ -71,14 +71,11 @@ scsi_bus_resume_common(struct device *de
 {
 	int err = 0;
 
+	if (pm_runtime_status_suspended(dev))
+		return err;
+
 	if (scsi_is_sdev_device(dev))
 		err = scsi_dev_type_resume(dev, cb);
-
-	if (err == 0) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
 	return err;
 }
 

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 11:08 AM, Alan Stern wrote:
> Okay, that's a different matter.  There's a much simpler way to 
> accomplish this.  The patch below will avoid spinning up drives
> that were already in runtime suspend when the system sleep started.
>  (If a drive wasn't in runtime suspend then presumably it was used
>  recently; therefore it's likely to be used again in the near
> future and so it _should_ be spun up.)

This is a poor assumption.  You may not be using autosuspend at all,
or with a rather long timeout, so it is fairly normal to not be
runtime suspended at suspend time, and yet not likely to access the
disk for some time after resume.  It also still suffers from the issue
of claiming the device is runtime suspended while the disk has in
fact, spun up of its own accord.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzC1fAAoJEI5FoCIzSKrwO5oIAJaROYiUSrapbSo40/YLLuPH
Y+5ECV7JW1hVSK7n3MyYls5YS6DiBQpjlhZ0h7XXoRWzUuT8GtU69wufjgln9IkZ
0zTDj7idDBzbBVBzdhLRsposIWWldsnpfYqvOPZsCd/xQe/jhnamKfBCSr4gYHy0
/FklPdPa2HXwApTNEz2fpleNiqdl6lbDEUkqI/sWkbjKrGVb57CAu8MaruPd2Frh
lkE3Dw7OfgQcngU8pU6kj4otRaBMEoEfIN7gMQ2CNont5JICDf4CvrZXawvKD3/1
Vg72E7DEUkNOCorqGywPcbV8838joEefmcsBc73w7Cn0rZC/5OmgrPyIUfgaS9k=
=bu2W
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Tue, 7 Jan 2014, Phillip Susi wrote:

> On 1/7/2014 11:08 AM, Alan Stern wrote:
> > Okay, that's a different matter.  There's a much simpler way to 
> > accomplish this.  The patch below will avoid spinning up drives
> > that were already in runtime suspend when the system sleep started.
> >  (If a drive wasn't in runtime suspend then presumably it was used
> >  recently; therefore it's likely to be used again in the near
> > future and so it _should_ be spun up.)
> 
> This is a poor assumption.  You may not be using autosuspend at all,
> or with a rather long timeout, so it is fairly normal to not be
> runtime suspended at suspend time, and yet not likely to access the
> disk for some time after resume.

I disagree with your argument.  If you aren't using autosuspend at all 
then you don't mind having your disks spin all the time.  Therefore you 
don't mind if the disks spin up during system resume.

If you're using a long timeout and you don't like the way the timer 
gets reset by a system sleep, then you have set the timeout too long.

Now, a more reasonable argument might be that for some disks, the
kernel doesn't need to do an explicit spin-up or spin-down (for runtime
suspend) at all; instead we can rely on the drive's internal power
management.  In fact there already is a "manage_start_stop" attribute
to control this.  (Well, almost -- if the attribute is set to 0 then
the kernel won't issue a spin-down command even for system suspend.)

>  It also still suffers from the issue
> of claiming the device is runtime suspended while the disk has in
> fact, spun up of its own accord.

I don't understand.  Under what circumstances would this happen?  Are 
you saying this would happen during system resume?  Why would the disk 
spin up of its own accord at that time?

And if it does spin up of its own accord, what makes you think the 
kernel can do anything to prevent it from spinning up?

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/7/2014 1:05 PM, Alan Stern wrote:
> I disagree with your argument.  If you aren't using autosuspend at
> all then you don't mind having your disks spin all the time.
> Therefore you don't mind if the disks spin up during system
> resume.

I prefer to manually spin the disk down when I don't plan on using it
for a while instead of letting it auto suspend using a timeout since
the timeout often either is too short and spins the disk down before
I'm done with it, or too long, and doesn't let it sleep as much as it
could.  Yet, I do mind that the disk must spin up whenever I resume,
even though I don't plan on using it any time soon.

> If you're using a long timeout and you don't like the way the timer
>  gets reset by a system sleep, then you have set the timeout too
> long.

It isn't reset, it simply hasn't timed out at the time I suspend.
This isn't very hard to do with even only a 5 minute auto suspend
delay.  If I do something with the disk then suspend the whole system
3 minutes later, it would spin up again when I resume even though I
finished my use of the disk before suspending the system.

> Now, a more reasonable argument might be that for some disks, the 
> kernel doesn't need to do an explicit spin-up or spin-down (for
> runtime suspend) at all; instead we can rely on the drive's
> internal power management.  In fact there already is a
> "manage_start_stop" attribute to control this.  (Well, almost -- if
> the attribute is set to 0 then the kernel won't issue a spin-down
> command even for system suspend.)

This flag is broken and unusable and should be removed.  First, SCSI
disks *require* the start command after a suspend, and secondly, not
stopping the drive before suspend causes an emergency head retract,
which damages the drive.

> I don't understand.  Under what circumstances would this happen?
> Are you saying this would happen during system resume?  Why would
> the disk spin up of its own accord at that time?

By default, ATA disks spin up on their own when they are powered on.

> And if it does spin up of its own accord, what makes you think the
>  kernel can do anything to prevent it from spinning up?

It can't.  What it should do is notice when this has happened and not
claim the device is runtime suspended when it is in fact, spinning.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzErYAAoJEI5FoCIzSKrw8R4H/RZZfPxQYcFEw1oVlkVY1gdM
TCi0MJoSBVAg9Iepplbl3RPZKb4DR9W+k6ICcZyOdw155B8mRzznn9RYoqkDEXX8
u2mqGfqx4TRf1OJo8jGKyJhPW6tJjwHM+G5Dx4WO6XAoVtwAvJHrFiNfuG9wQa1t
DswH0IOm6ZfEH0Z4hjeq6vnyEtmB3ecrMpvplM5+tRchE1SuvP+OkD3J6EXIlL3c
J7+b7pmu3Cx/+2vz92NlWHLpFXeC1fnOAy2+jJhwia1X3tVtf+wN1KIrQBK9Lq+i
Dmgmd++DUr4//CwKftPUx/cmv/RUgSnfd7MLfHZvxJrBOsOHFoI8BYbDHqoGWEU=
=O0Xo
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Tue, 7 Jan 2014, Phillip Susi wrote:

> On 1/7/2014 1:05 PM, Alan Stern wrote:
> > I disagree with your argument.  If you aren't using autosuspend at
> > all then you don't mind having your disks spin all the time.
> > Therefore you don't mind if the disks spin up during system
> > resume.
> 
> I prefer to manually spin the disk down when I don't plan on using it
> for a while instead of letting it auto suspend using a timeout since
> the timeout often either is too short and spins the disk down before
> I'm done with it, or too long, and doesn't let it sleep as much as it
> could.  Yet, I do mind that the disk must spin up whenever I resume,
> even though I don't plan on using it any time soon.

I don't know how you manually spin down your disk, but one reasonable
way to do it is to set the autosuspend timeout to 0.  If you use this
approach and apply my patch, the disk should remain spun down when the
system resumes.

> > If you're using a long timeout and you don't like the way the timer
> >  gets reset by a system sleep, then you have set the timeout too
> > long.
> 
> It isn't reset, it simply hasn't timed out at the time I suspend.

On the contrary, it is reset.  See below.

> This isn't very hard to do with even only a 5 minute auto suspend
> delay.  If I do something with the disk then suspend the whole system
> 3 minutes later, it would spin up again when I resume even though I
> finished my use of the disk before suspending the system.

Right.  If you hadn't put the whole system to sleep, the disk would
have gone into runtime suspend 2 minutes later (assuming you didn't use
it in the meantime).  Whereas since you did put the whole system to
sleep, the disk will go into runtime suspend 5 minutes after the system
wakes up -- not 2 minutes later.  I.e., the timer has been reset from 2 
minutes back to 5.

How is the kernel supposed to know that you finished using the disk
before suspending the system?  After all, by setting the autosuspend
timeout to 5 minutes, you have already told the kernel to keep the disk
spun up if it has been used any time in the last 5 minutes, and you
used it only 3 minutes ago.  As far as the kernel can tell, you still
want the disk to be spun up and ready for use.  That remains true at 
the time of the system resume.

> > Now, a more reasonable argument might be that for some disks, the 
> > kernel doesn't need to do an explicit spin-up or spin-down (for
> > runtime suspend) at all; instead we can rely on the drive's
> > internal power management.  In fact there already is a
> > "manage_start_stop" attribute to control this.  (Well, almost -- if
> > the attribute is set to 0 then the kernel won't issue a spin-down
> > command even for system suspend.)
> 
> This flag is broken and unusable and should be removed.

Not at all.

>  First, SCSI
> disks *require* the start command after a suspend, and secondly, not
> stopping the drive before suspend causes an emergency head retract,
> which damages the drive.

You're forgetting that the same sd driver manages other types of 
devices than disk drives.  Card readers and USB flash drives have no 
heads to retract and no spinning platters.  They don't need START STOP 
commands (in fact, some of them probably would crash if they received 
such a command).

> > I don't understand.  Under what circumstances would this happen?
> > Are you saying this would happen during system resume?  Why would
> > the disk spin up of its own accord at that time?
> 
> By default, ATA disks spin up on their own when they are powered on.

Irrelevant, unless you are also claiming that all ATA disks always get
powered on whenever the system resumes, something which is not at all
evident to me.  Particularly if the disk was in runtime suspend before
the system went to sleep.

> > And if it does spin up of its own accord, what makes you think the
> >  kernel can do anything to prevent it from spinning up?
> 
> It can't.  What it should do is notice when this has happened and not
> claim the device is runtime suspended when it is in fact, spinning.

Now I'm really getting confused.  Here, you are saying that ATA disks
will always spin up, all by themselves, whenever the system resumes,
and there's nothing the kernel can do to prevent it.  But earlier you
wrote:

> Sure, if that is your *only* goal.  I also want the disk to not spin
> up *at all* if possible.  There's no sense spinning up all of your
> disks every time you resume when you very rarely access some of them.

Isn't this a contradiction?  Or are you making a distinction between 
ATA and non-ATA disks?

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/07/2014 02:18 PM, Alan Stern wrote:
> I don't know how you manually spin down your disk, but one
> reasonable way to do it is to set the autosuspend timeout to 0.  If
> you use this approach and apply my patch, the disk should remain
> spun down when the system resumes.

The traditional method before the recently added block pm feature was
with hdparm -y.

> Right.  If you hadn't put the whole system to sleep, the disk
> would have gone into runtime suspend 2 minutes later (assuming you
> didn't use it in the meantime).  Whereas since you did put the
> whole system to sleep, the disk will go into runtime suspend 5
> minutes after the system wakes up -- not 2 minutes later.  I.e.,
> the timer has been reset from 2 minutes back to 5.

What resets the timer?  It should only be reset when IO is done.  And
yes, it is exactly that wakeup and turn around and suspend again that
I'm trying to avoid; it puts unnecessary wear and tear on the disk.

> How is the kernel supposed to know that you finished using the
> disk before suspending the system?  After all, by setting the
> autosuspend timeout to 5 minutes, you have already told the kernel
> to keep the disk spun up if it has been used any time in the last 5
> minutes, and you used it only 3 minutes ago.  As far as the kernel
> can tell, you still want the disk to be spun up and ready for use.
> That remains true at the time of the system resume.

That's the whole point: it doesn't know.  What it does know is that
the disk is spun down on resume, and it has no reason to believe that
you will need it again soon.  This is especially true given that your
typical single ATA disk system will spin up the drive on its own
anyhow, so on systems with multiple scsi or ATA disks that explicitly
have been set to power up in standby, it is at least an even bet that
you won't be touching them all soon.

> You're forgetting that the same sd driver manages other types of 
> devices than disk drives.  Card readers and USB flash drives have
> no heads to retract and no spinning platters.  They don't need
> START STOP commands (in fact, some of them probably would crash if
> they received such a command).

They are irrelevant since they ignore the command anyhow.

> Irrelevant, unless you are also claiming that all ATA disks always
> get powered on whenever the system resumes, something which is not
> at all evident to me.  Particularly if the disk was in runtime
> suspend before the system went to sleep.

I'm saying that most do.  It doesn't require 100% to be relevant.
There will also be plenty of disks at least for some time that are
still using the internal standby timer rather than runtime pm.

> Now I'm really getting confused.  Here, you are saying that ATA
> disks will always spin up, all by themselves, whenever the system
> resumes, and there's nothing the kernel can do to prevent it.  But
> earlier you wrote:

No, I'm saying some ( most in fact ) do, and some do not.  Both cases
need to be handled.  Runtime pm must not indicate the disk is
suspended when it spins up on its own after power on, which ATA disks
do by default.  Oddly, the SCSI standard says that SCSI disks can be
configured behave this way as well rather than requiring an explicit
start command, though it doesn't say how and I've not heard of a way
to do this.  I suppose this also applies to the USB flash drives and
SD card readers you mentioned that are managed by sd.

> Isn't this a contradiction?  Or are you making a distinction
> between ATA and non-ATA disks?

What?  I have some disk drives that I rarely access.  I simply don't
want them starting up and spinning back down every time I resume.  In
my case they are ATA with PuiS enabled, but it applies to SCSI the
same way.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSzJItAAoJEI5FoCIzSKrwhCQIAIclA7u+FJ1PSZRYWcvFQg0B
n/WIupSCAXfiSo4uVZpC9m4W8TR9CawEorIPZGE+G/Hv+zRz3YKqA3OOuOQc1S5i
5IK019yY4EsOHsUK8RlBsgKu1bW0SdFsa73COzqT65bxNqUb5PUK5ed/Z1Kg7cTn
QM7jCMYz0wE7cqAQQ8eV56Y1Nolb6T3WmqKqoGl+qJj+IvebCuJ9vsYXf7MhSd9b
yb2Iq/ThR81vm68c6+KOFQkOkL3zPZ6QWCh5Xj4mRkvXtsd7htNKpxTkM7OC6azF
Z0I5xreArN9SQ8GLHtzB2Lrs+SzlMSvIE1HKQdieBcy//LImk8DajbddupJy7Bk=
=dYPC
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Tue, 7 Jan 2014, Phillip Susi wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 01/07/2014 02:18 PM, Alan Stern wrote:
> > I don't know how you manually spin down your disk, but one
> > reasonable way to do it is to set the autosuspend timeout to 0.  If
> > you use this approach and apply my patch, the disk should remain
> > spun down when the system resumes.
> 
> The traditional method before the recently added block pm feature was
> with hdparm -y.

A real problem here is that hdparm does not interact with the runtime 
PM part of the kernel.  They work independently and can interfere with 
each other.

> > Right.  If you hadn't put the whole system to sleep, the disk
> > would have gone into runtime suspend 2 minutes later (assuming you
> > didn't use it in the meantime).  Whereas since you did put the
> > whole system to sleep, the disk will go into runtime suspend 5
> > minutes after the system wakes up -- not 2 minutes later.  I.e.,
> > the timer has been reset from 2 minutes back to 5.
> 
> What resets the timer?  It should only be reset when IO is done.  And

I/O _was_ done.  To spin up the disk requires sending it a START 
command.  That's I/O.

Okay, you may not think that is a valid argument.  Still, the timer has
to be set to _something_, and during the resume we don't know how much
time was remaining as of the suspend.  (I suppose we could store this 
information when the suspend occurs, but currently we don't.)

> yes, it is exactly that wakeup and turn around and suspend again that
> I'm trying to avoid; it puts unnecessary wear and tear on the disk.

In essence, you want your disk's state to move directly from unpowered
(system asleep) to runtime-suspended with no intermediate active -- but
only if the disk doesn't spin up all by itself.

One problem: How can the kernel tell whether the disk has spun up by
itself?  I don't think it can be done at the SCSI level.  Can it be
done at the ATA level (and without using the SCSI stack, as that would
require the disk to go out of runtime suspend and spin up anyway)?


> > How is the kernel supposed to know that you finished using the
> > disk before suspending the system?  After all, by setting the
> > autosuspend timeout to 5 minutes, you have already told the kernel
> > to keep the disk spun up if it has been used any time in the last 5
> > minutes, and you used it only 3 minutes ago.  As far as the kernel
> > can tell, you still want the disk to be spun up and ready for use.
> > That remains true at the time of the system resume.
> 
> That's the whole point: it doesn't know.  What it does know is that
> the disk is spun down on resume, and it has no reason to believe that
> you will need it again soon.

Actually, it does have a reason.  Namely, you told it earlier to assume 
the disk will be needed again if it was used within the last 5 
minutes.  At the time of the system resume, the disk was last used 3 
minutes ago (not counting the time the system was asleep).

>  This is especially true given that your
> typical single ATA disk system will spin up the drive on its own
> anyhow, so on systems with multiple scsi or ATA disks that explicitly
> have been set to power up in standby, it is at least an even bet that
> you won't be touching them all soon.

Are you assuming these disks were all runtime-active before the system 
sleep?  If so, the kernel is justified in assuming that you will be 
touching them all soon.

> > You're forgetting that the same sd driver manages other types of 
> > devices than disk drives.  Card readers and USB flash drives have
> > no heads to retract and no spinning platters.  They don't need
> > START STOP commands (in fact, some of them probably would crash if
> > they received such a command).
> 
> They are irrelevant since they ignore the command anyhow.

Not the ones that crash when they receive it.  But this point is moot
since manage_start_stop doesn't do what you want anyway.

> > Now I'm really getting confused.  Here, you are saying that ATA
> > disks will always spin up, all by themselves, whenever the system
> > resumes, and there's nothing the kernel can do to prevent it.  But
> > earlier you wrote:
> 
> No, I'm saying some ( most in fact ) do, and some do not.  Both cases
> need to be handled.  Runtime pm must not indicate the disk is
> suspended when it spins up on its own after power on, which ATA disks
> do by default.  Oddly, the SCSI standard says that SCSI disks can be
> configured behave this way as well rather than requiring an explicit
> start command, though it doesn't say how and I've not heard of a way
> to do this.  I suppose this also applies to the USB flash drives and
> SD card readers you mentioned that are managed by sd.

No, it doesn't.  They don't spin, so they can't be configured either to 
spin up or not to spin up after power-on.

> > Isn't this a contradiction?  Or are you making a distinction
> > between ATA and non-ATA disks?
> 
> What?  I have some disk drives that I rarely access.  I simply don't
> want them starting up and spinning back down every time I resume.  In
> my case they are ATA with PuiS enabled, but it applies to SCSI the
> same way.

What is PuiS?


In the end, it sounds like what you want can be done fairly easily.  
But only if the kernel has a reliable way to tell whether or not a disk
is spinning (or is spinning up).

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Wed, 8 Jan 2014, Alan Stern wrote:

> In essence, you want your disk's state to move directly from unpowered
> (system asleep) to runtime-suspended with no intermediate active -- but
> only if the disk doesn't spin up all by itself.

> In the end, it sounds like what you want can be done fairly easily.  

It turns out there is a snag.  The PM core is set up such that during
system sleep transitions, there's no way to tell whether or not
userspace has permitted runtime PM for a device.  (This is because the
core wants to prevent untimely runtime suspends from occurring in the
middle of a system sleep transition, and to do so it uses the same
mechanism as it does when userspace disallows runtime suspend.)

As a result, we must not go directly from unpowered to
runtime-suspended.  Not without some sort of change to the PM core.

Alan Stern

Allow runtime suspend during system resume

[Alan Stern]

On Wed, 8 Jan 2014, Alan Stern wrote:

> On Wed, 8 Jan 2014, Alan Stern wrote:
> 
> > In essence, you want your disk's state to move directly from unpowered
> > (system asleep) to runtime-suspended with no intermediate active -- but
> > only if the disk doesn't spin up all by itself.
> 
> > In the end, it sounds like what you want can be done fairly easily.  
> 
> It turns out there is a snag.  The PM core is set up such that during
> system sleep transitions, there's no way to tell whether or not
> userspace has permitted runtime PM for a device.  (This is because the
> core wants to prevent untimely runtime suspends from occurring in the
> middle of a system sleep transition, and to do so it uses the same
> mechanism as it does when userspace disallows runtime suspend.)
> 
> As a result, we must not go directly from unpowered to
> runtime-suspended.  Not without some sort of change to the PM core.

Rafael:

Right now the PM core does pm_runtime_get_noresume() during the prepare 
phase of system sleep, and pm_runtime_put() during the complete phase.  
Maybe we should cut down the range of coverage.

For example, we could do the pm_runtime_put() during dpm_resume(), or 
even earlier.  That way, the resume routine could do something like:

	if (!pm_runtime_in_use(dev)) {
		pm_runtime_disable(dev);
		pm_runtime_set_suspended(dev);
		pm_runtime_enable(dev);
		return 0;
	}

where pm_runtime_in_use() would check the device's usage counters.  
The device would come out of system sleep in a powered-down state, and
could remain runtime-suspended with no need for an extra
power-up/power-down cycle.  Isn't this the sort of thing the embedded
people want to do?

As it is, drivers have no way to safely put devices directly into 
runtime suspend when coming out of system sleep.

What do you think?

Alan Stern

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2014 3:44 PM, Alan Stern wrote:
> Right now the PM core does pm_runtime_get_noresume() during the
> prepare phase of system sleep, and pm_runtime_put() during the
> complete phase. Maybe we should cut down the range of coverage.
> 
> For example, we could do the pm_runtime_put() during dpm_resume(),
> or even earlier.  That way, the resume routine could do something
> like:
> 
> if (!pm_runtime_in_use(dev)) { pm_runtime_disable(dev); 
> pm_runtime_set_suspended(dev); pm_runtime_enable(dev); return 0; }
> 

I'm a little unclear on the problem you are describing.  Is it just
with pm_runtime_in_use()?  Because in one of my earlier patch
iterations, I had used the disable/set_suspended/enable to force the
device into suspend without checking runtime_in_use.  It worked, but
that allowed the port device to auto suspend since its use counter
dropped to zero, so I couldn't issue the REQUEST SENSE command
disable/set_active/enable to reverse the process ( since the port was
suspended ).


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzcBnAAoJEI5FoCIzSKrw/LQH/A7voCeYxm8fU8k2Uxw2yTNO
dZjgzTVoETQAif3iNtbMaXy8lF4GrqRVtGJIF683eK5jn5zUTSPggaxFXUqFGRAM
sJHfxxNaJKThCRGBrZg6NEIoushNLlC/XdwY2XEt+ee2aVdNbSs2Up7IOdkuqrjc
1uPThOoBbgb3GE/5FlLgfhouQhdIXYhh7r7xu1JaJ0DVhtwPMWdMFBLos2qdsMk4
5uPBLNhrQyHt2mQvEQj4meVwqun+SsbzWtgiP3Nk5MSCCRa4LHVa0nfb2N6+ee8+
Q3/kwoDDyghvJeoi4K7iWLFfFjynTchvxfzInItnYK5TKJiU1rf2En+qIT2g9BQ=
=Fkc4
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Wed, 8 Jan 2014, Phillip Susi wrote:

> On 1/8/2014 3:44 PM, Alan Stern wrote:
> > Right now the PM core does pm_runtime_get_noresume() during the
> > prepare phase of system sleep, and pm_runtime_put() during the
> > complete phase. Maybe we should cut down the range of coverage.
> > 
> > For example, we could do the pm_runtime_put() during dpm_resume(),
> > or even earlier.  That way, the resume routine could do something
> > like:
> > 
> > if (!pm_runtime_in_use(dev)) { pm_runtime_disable(dev); 
> > pm_runtime_set_suspended(dev); pm_runtime_enable(dev); return 0; }
> > 
> 
> I'm a little unclear on the problem you are describing.  Is it just
> with pm_runtime_in_use()?

Note that currently there is no pm_runtime_in_use().  One of the things
I'm proposing is to add it.

If it were already present and you used it properly, you would find
that it always returned non-zero.  Therefore you would never execute
the pm_runtime_set_suspended.  _That_'s the problem I'm trying to
address here.

>  Because in one of my earlier patch
> iterations, I had used the disable/set_suspended/enable to force the
> device into suspend without checking runtime_in_use.

That is not a valid thing to do.  The user may have disabled runtime 
suspend for the disk; you must not override the user's policy.

>  It worked, but
> that allowed the port device to auto suspend since its use counter
> dropped to zero, so I couldn't issue the REQUEST SENSE command
> disable/set_active/enable to reverse the process ( since the port was
> suspended ).

That's a separate problem, to be discussed back in the earlier email 
thread.

Alan Stern

Re: Allow runtime suspend during system resume

[Ulf Hansson]

On 8 January 2014 22:34, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 8 Jan 2014, Phillip Susi wrote:
>
>> On 1/8/2014 3:44 PM, Alan Stern wrote:
>> > Right now the PM core does pm_runtime_get_noresume() during the
>> > prepare phase of system sleep, and pm_runtime_put() during the
>> > complete phase. Maybe we should cut down the range of coverage.
>> >
>> > For example, we could do the pm_runtime_put() during dpm_resume(),
>> > or even earlier.  That way, the resume routine could do something
>> > like:
>> >
>> > if (!pm_runtime_in_use(dev)) { pm_runtime_disable(dev);
>> > pm_runtime_set_suspended(dev); pm_runtime_enable(dev); return 0; }
>> >
>>
>> I'm a little unclear on the problem you are describing.  Is it just
>> with pm_runtime_in_use()?
>
> Note that currently there is no pm_runtime_in_use().  One of the things
> I'm proposing is to add it.
>
> If it were already present and you used it properly, you would find
> that it always returned non-zero.  Therefore you would never execute
> the pm_runtime_set_suspended.  _That_'s the problem I'm trying to
> address here.
>
>>  Because in one of my earlier patch
>> iterations, I had used the disable/set_suspended/enable to force the
>> device into suspend without checking runtime_in_use.
>
> That is not a valid thing to do.  The user may have disabled runtime
> suspend for the disk; you must not override the user's policy.

Allowing userspace to disable/enable runtime PM, does in many cases
add complexity to subsystems and drivers when they handle system
suspend/resume. This seems to be the case for SCSI as well?

To simplify for these subsystems and drivers, what do you think about
inventing something like "pm_runtime_sysfs_forbid", which in principle
will prevent userspace from enable/disable runtime PM? Typically this
function will be called from ->probe.

Kind regards
Ulf Hansson

>
>>  It worked, but
>> that allowed the port device to auto suspend since its use counter
>> dropped to zero, so I couldn't issue the REQUEST SENSE command
>> disable/set_active/enable to reverse the process ( since the port was
>> suspended ).
>
> That's a separate problem, to be discussed back in the earlier email
> thread.
>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Allow runtime suspend during system resume

[Alan Stern]

On Thu, 9 Jan 2014, Ulf Hansson wrote:

> Allowing userspace to disable/enable runtime PM, does in many cases
> add complexity to subsystems and drivers when they handle system
> suspend/resume. This seems to be the case for SCSI as well?

No.

> To simplify for these subsystems and drivers, what do you think about
> inventing something like "pm_runtime_sysfs_forbid", which in principle
> will prevent userspace from enable/disable runtime PM? Typically this
> function will be called from ->probe.

I think it is a very bad idea.

Alan Stern

Re: Allow runtime suspend during system resume

[Rafael J. Wysocki]

On Wednesday, January 08, 2014 03:44:53 PM Alan Stern wrote:
> On Wed, 8 Jan 2014, Alan Stern wrote:
> 
> > On Wed, 8 Jan 2014, Alan Stern wrote:
> > 
> > > In essence, you want your disk's state to move directly from unpowered
> > > (system asleep) to runtime-suspended with no intermediate active -- but
> > > only if the disk doesn't spin up all by itself.
> > 
> > > In the end, it sounds like what you want can be done fairly easily.  
> > 
> > It turns out there is a snag.  The PM core is set up such that during
> > system sleep transitions, there's no way to tell whether or not
> > userspace has permitted runtime PM for a device.  (This is because the
> > core wants to prevent untimely runtime suspends from occurring in the
> > middle of a system sleep transition, and to do so it uses the same
> > mechanism as it does when userspace disallows runtime suspend.)
> > 
> > As a result, we must not go directly from unpowered to
> > runtime-suspended.  Not without some sort of change to the PM core.
> 
> Rafael:
> 
> Right now the PM core does pm_runtime_get_noresume() during the prepare 
> phase of system sleep, and pm_runtime_put() during the complete phase.  
> Maybe we should cut down the range of coverage.
> 
> For example, we could do the pm_runtime_put() during dpm_resume(), or 
> even earlier.  That way, the resume routine could do something like:
> 
> 	if (!pm_runtime_in_use(dev)) {
> 		pm_runtime_disable(dev);
> 		pm_runtime_set_suspended(dev);
> 		pm_runtime_enable(dev);
> 		return 0;
> 	}
> 
> where pm_runtime_in_use() would check the device's usage counters.  
> The device would come out of system sleep in a powered-down state, and
> could remain runtime-suspended with no need for an extra
> power-up/power-down cycle.  Isn't this the sort of thing the embedded
> people want to do?
> 
> As it is, drivers have no way to safely put devices directly into 
> runtime suspend when coming out of system sleep.
> 
> What do you think?

There are two problems here.

First off, for some devices that were runtime-suspended before system suspend
we may not want to touch them at all during system suspend/resume.  I think
I know how to address that, but I need to prepare patches yet.  That's going
to take a few days at least, sorry about that.

Second, we may want some devices to be set to "runtime suspended" during
system resume without touching them, so that runtime PM resumes them when
necessary.  I have a patch for that, which hasn't been published yet and which
is appended.

The way it works is that whoever wants the system resume of certain device
to be skipped, sets the (new) skip_resume flag for that device using
device_pm_skip_resume() and that causes the PM core to basically ignore that
device's PM callbacks during system resume.

Whether or not doing the pm_request_resume(dev) for devices with skip_resume
set in dpm_resume_early() is a good idea is a discussion item in my opinion
(evidently, I thought it was when I was preparing this patch).

Thanks,
Rafael


---
 drivers/base/power/main.c |   98 ++++++++++++++++++++++++++++++++++------------
 include/linux/pm.h        |   13 +++++-
 2 files changed, 85 insertions(+), 26 deletions(-)

Index: linux-pm/include/linux/pm.h
===================================================================
--- linux-pm.orig/include/linux/pm.h
+++ linux-pm/include/linux/pm.h
@@ -532,9 +532,12 @@ struct dev_pm_info {
 	struct wakeup_source	*wakeup;
 	bool			wakeup_path:1;
 	bool			syscore:1;
+#ifdef CONFIG_PM_RUNTIME
+	bool			skip_resume:1;
+#endif
 #else
 	unsigned int		should_wakeup:1;
-#endif
+#endif /* CONFIG_PM_SLEEP */
 #ifdef CONFIG_PM_RUNTIME
 	struct timer_list	suspend_timer;
 	unsigned long		timer_expires;
@@ -561,7 +564,7 @@ struct dev_pm_info {
 	unsigned long		active_jiffies;
 	unsigned long		suspended_jiffies;
 	unsigned long		accounting_timestamp;
-#endif
+#endif /* CONFIG_PM_RUNTIME */
 	struct pm_subsys_data	*subsys_data;  /* Owned by the subsystem. */
 	struct dev_pm_qos	*qos;
 };
@@ -716,4 +719,10 @@ enum dpm_order {
 	DPM_ORDER_DEV_LAST,
 };
 
+#if defined(CONFIG_PM_RUNTIME) && defined(CONFIG_PM_SLEEP)
+void device_pm_skip_resume(struct device *dev, bool enable);
+#else
+static inline void device_pm_skip_resume(struct device *dev, bool enable) {}
+#endif /* CONFIG_PM_RUNTIME && CONFIG_PM_SLEEP */
+
 #endif /* _LINUX_PM_H */
Index: linux-pm/drivers/base/power/main.c
===================================================================
--- linux-pm.orig/drivers/base/power/main.c
+++ linux-pm/drivers/base/power/main.c
@@ -384,6 +384,29 @@ static int dpm_run_callback(pm_callback_
 	return error;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+void device_pm_skip_resume(struct device *dev, bool enable)
+{
+	device_pm_lock();
+
+	if (!dev->power.is_prepared)
+		dev->power.skip_resume = !!enable;
+
+	device_pm_unlock();
+}
+
+static inline bool skip_device_resume(struct device *dev, pm_message_t state)
+{
+	return dev->power.skip_resume && (state.event == PM_EVENT_RESUME
+		|| state.event == PM_EVENT_RESTORE);
+}
+#else
+static inline bool skip_device_resume(struct device *dev, pm_message_t state)
+{
+	return false;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
 /*------------------------- Resume routines -------------------------*/
 
 /**
@@ -446,21 +469,24 @@ static void dpm_resume_noirq(pm_message_
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_noirq_list)) {
 		struct device *dev = to_device(dpm_noirq_list.next);
-		int error;
 
 		get_device(dev);
 		list_move_tail(&dev->power.entry, &dpm_late_early_list);
-		mutex_unlock(&dpm_list_mtx);
+		if (!skip_device_resume(dev, state)) {
+			int error;
 
-		error = device_resume_noirq(dev, state);
-		if (error) {
-			suspend_stats.failed_resume_noirq++;
-			dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
-			dpm_save_failed_dev(dev_name(dev));
-			pm_dev_err(dev, state, " noirq", error);
-		}
+			mutex_unlock(&dpm_list_mtx);
+
+			error = device_resume_noirq(dev, state);
+			if (error) {
+				suspend_stats.failed_resume_noirq++;
+				dpm_save_failed_step(SUSPEND_RESUME_NOIRQ);
+				dpm_save_failed_dev(dev_name(dev));
+				pm_dev_err(dev, state, " noirq", error);
+			}
 
-		mutex_lock(&dpm_list_mtx);
+			mutex_lock(&dpm_list_mtx);
+		}
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
@@ -527,21 +553,39 @@ static void dpm_resume_early(pm_message_
 	mutex_lock(&dpm_list_mtx);
 	while (!list_empty(&dpm_late_early_list)) {
 		struct device *dev = to_device(dpm_late_early_list.next);
-		int error;
 
 		get_device(dev);
 		list_move_tail(&dev->power.entry, &dpm_suspended_list);
-		mutex_unlock(&dpm_list_mtx);
+		if (skip_device_resume(dev, state)) {
+			pm_runtime_set_suspended(dev);
+			pm_runtime_enable(dev);
+			/*
+			 * Balance the pm_runtime_get_noresume() in
+			 * device_prepare().
+			 */
+			pm_runtime_put_noidle(dev);
+			/*
+			 * The device might have been powered up by the platform
+			 * firmware already, so make it resume and then possibly
+			 * suspend again to avoid leaving powered up devices as
+			 * "suspended" for too long.
+			 */
+			pm_request_resume(dev);
+		} else {
+			int error;
 
-		error = device_resume_early(dev, state);
-		if (error) {
-			suspend_stats.failed_resume_early++;
-			dpm_save_failed_step(SUSPEND_RESUME_EARLY);
-			dpm_save_failed_dev(dev_name(dev));
-			pm_dev_err(dev, state, " early", error);
-		}
+			mutex_unlock(&dpm_list_mtx);
+
+			error = device_resume_early(dev, state);
+			if (error) {
+				suspend_stats.failed_resume_early++;
+				dpm_save_failed_step(SUSPEND_RESUME_EARLY);
+				dpm_save_failed_dev(dev_name(dev));
+				pm_dev_err(dev, state, " early", error);
+			}
 
-		mutex_lock(&dpm_list_mtx);
+			mutex_lock(&dpm_list_mtx);
+		}
 		put_device(dev);
 	}
 	mutex_unlock(&dpm_list_mtx);
@@ -682,6 +726,10 @@ void dpm_resume(pm_message_t state)
 
 	list_for_each_entry(dev, &dpm_suspended_list, power.entry) {
 		INIT_COMPLETION(dev->power.completion);
+		if (skip_device_resume(dev, state)) {
+			complete(&dev->power.completion);
+			continue;
+		}
 		if (is_async(dev)) {
 			get_device(dev);
 			async_schedule(async_resume, dev);
@@ -691,7 +739,7 @@ void dpm_resume(pm_message_t state)
 	while (!list_empty(&dpm_suspended_list)) {
 		dev = to_device(dpm_suspended_list.next);
 		get_device(dev);
-		if (!is_async(dev)) {
+		if (!is_async(dev) && !skip_device_resume(dev, state)) {
 			int error;
 
 			mutex_unlock(&dpm_list_mtx);
@@ -780,11 +828,13 @@ void dpm_complete(pm_message_t state)
 		get_device(dev);
 		dev->power.is_prepared = false;
 		list_move(&dev->power.entry, &list);
-		mutex_unlock(&dpm_list_mtx);
+		if (!skip_device_resume(dev, state)) {
+			mutex_unlock(&dpm_list_mtx);
 
-		device_complete(dev, state);
+			device_complete(dev, state);
 
-		mutex_lock(&dpm_list_mtx);
+			mutex_lock(&dpm_list_mtx);
+		}
 		put_device(dev);
 	}
 	list_splice(&list, &dpm_list);

Re: Allow runtime suspend during system resume

[Alan Stern]

On Wed, 8 Jan 2014, Rafael J. Wysocki wrote:

> On Wednesday, January 08, 2014 03:44:53 PM Alan Stern wrote:
> > On Wed, 8 Jan 2014, Alan Stern wrote:
> > 
> > > On Wed, 8 Jan 2014, Alan Stern wrote:
> > > 
> > > > In essence, you want your disk's state to move directly from unpowered
> > > > (system asleep) to runtime-suspended with no intermediate active -- but
> > > > only if the disk doesn't spin up all by itself.
> > > 
> > > > In the end, it sounds like what you want can be done fairly easily.  
> > > 
> > > It turns out there is a snag.  The PM core is set up such that during
> > > system sleep transitions, there's no way to tell whether or not
> > > userspace has permitted runtime PM for a device.  (This is because the
> > > core wants to prevent untimely runtime suspends from occurring in the
> > > middle of a system sleep transition, and to do so it uses the same
> > > mechanism as it does when userspace disallows runtime suspend.)
> > > 
> > > As a result, we must not go directly from unpowered to
> > > runtime-suspended.  Not without some sort of change to the PM core.
> > 
> > Rafael:
> > 
> > Right now the PM core does pm_runtime_get_noresume() during the prepare 
> > phase of system sleep, and pm_runtime_put() during the complete phase.  
> > Maybe we should cut down the range of coverage.
> > 
> > For example, we could do the pm_runtime_put() during dpm_resume(), or 
> > even earlier.  That way, the resume routine could do something like:
> > 
> > 	if (!pm_runtime_in_use(dev)) {
> > 		pm_runtime_disable(dev);
> > 		pm_runtime_set_suspended(dev);
> > 		pm_runtime_enable(dev);
> > 		return 0;
> > 	}
> > 
> > where pm_runtime_in_use() would check the device's usage counters.  
> > The device would come out of system sleep in a powered-down state, and
> > could remain runtime-suspended with no need for an extra
> > power-up/power-down cycle.  Isn't this the sort of thing the embedded
> > people want to do?
> > 
> > As it is, drivers have no way to safely put devices directly into 
> > runtime suspend when coming out of system sleep.
> > 
> > What do you think?
> 
> There are two problems here.
> 
> First off, for some devices that were runtime-suspended before system suspend
> we may not want to touch them at all during system suspend/resume.  I think
> I know how to address that, but I need to prepare patches yet.  That's going
> to take a few days at least, sorry about that.
> 
> Second, we may want some devices to be set to "runtime suspended" during
> system resume without touching them, so that runtime PM resumes them when
> necessary.  I have a patch for that, which hasn't been published yet and which
> is appended.
> 
> The way it works is that whoever wants the system resume of certain device
> to be skipped, sets the (new) skip_resume flag for that device using
> device_pm_skip_resume() and that causes the PM core to basically ignore that
> device's PM callbacks during system resume.
> 
> Whether or not doing the pm_request_resume(dev) for devices with skip_resume
> set in dpm_resume_early() is a good idea is a discussion item in my opinion
> (evidently, I thought it was when I was preparing this patch).

The situation isn't quite so simple.

We're talking specifically about disk drives.  Depending on the drive,
it may or may not spin up all by itself when the system wakes up.  If
it does spin up, the final state should be RPM_ACTIVE (the way it is
now).  If it doesn't, we want the final state to be RPM_SUSPENDED.  
This is true regardless of the runtime status before the system sleep.

This means we can't skip the resume routine.  At the very least, the 
routine needs to ask the disk whether or not it is spinning.  Depending 
on the result, it would then set the RPM status appropriately.

In addition, we only want to end up in RPM_SUSPENDED if the usage
counters are 0 (in particular, power/control = "auto").  Otherwise, we
want the resume routine always to spin up the drive, if it hasn't spun
up by itself.

Alan Stern

Re: Allow runtime suspend during system resume

[Rafael J. Wysocki]

> On Wed, 8 Jan 2014, Rafael J. Wysocki wrote:
> > On Wednesday, January 08, 2014 03:44:53 PM Alan Stern wrote:
> > > On Wed, 8 Jan 2014, Alan Stern wrote:
> > > > On Wed, 8 Jan 2014, Alan Stern wrote:
> > > > > In essence, you want your disk's state to move directly from
> > > > > unpowered
> > > > > (system asleep) to runtime-suspended with no intermediate active --
> > > > > but
> > > > > only if the disk doesn't spin up all by itself.
> > > > > 
> > > > > In the end, it sounds like what you want can be done fairly easily.
> > > > 
> > > > It turns out there is a snag.  The PM core is set up such that during
> > > > system sleep transitions, there's no way to tell whether or not
> > > > userspace has permitted runtime PM for a device.  (This is because the
> > > > core wants to prevent untimely runtime suspends from occurring in the
> > > > middle of a system sleep transition, and to do so it uses the same
> > > > mechanism as it does when userspace disallows runtime suspend.)
> > > > 
> > > > As a result, we must not go directly from unpowered to
> > > > runtime-suspended.  Not without some sort of change to the PM core.
> > > 
> > > Rafael:
> > > 
> > > Right now the PM core does pm_runtime_get_noresume() during the prepare
> > > phase of system sleep, and pm_runtime_put() during the complete phase.
> > > Maybe we should cut down the range of coverage.
> > > 
> > > For example, we could do the pm_runtime_put() during dpm_resume(), or
> > > 
> > > even earlier.  That way, the resume routine could do something like:
> > > 	if (!pm_runtime_in_use(dev)) {
> > > 	
> > > 		pm_runtime_disable(dev);
> > > 		pm_runtime_set_suspended(dev);
> > > 		pm_runtime_enable(dev);
> > > 		return 0;
> > > 	
> > > 	}
> > > 
> > > where pm_runtime_in_use() would check the device's usage counters.
> > > The device would come out of system sleep in a powered-down state, and
> > > could remain runtime-suspended with no need for an extra
> > > power-up/power-down cycle.  Isn't this the sort of thing the embedded
> > > people want to do?
> > > 
> > > As it is, drivers have no way to safely put devices directly into
> > > runtime suspend when coming out of system sleep.
> > > 
> > > What do you think?
> > 
> > There are two problems here.
> > 
> > First off, for some devices that were runtime-suspended before system
> > suspend
> > we may not want to touch them at all during system suspend/resume.  I
> > think
> > I know how to address that, but I need to prepare patches yet.  That's
> > going
> > to take a few days at least, sorry about that.
> > 
> > Second, we may want some devices to be set to "runtime suspended" during
> > system resume without touching them, so that runtime PM resumes them when
> > necessary.  I have a patch for that, which hasn't been published yet and
> > which is appended.
> > 
> > The way it works is that whoever wants the system resume of certain device
> > to be skipped, sets the (new) skip_resume flag for that device using
> > device_pm_skip_resume() and that causes the PM core to basically ignore
> > that
> > device's PM callbacks during system resume.
> > 
> > Whether or not doing the pm_request_resume(dev) for devices with
> > skip_resume set in dpm_resume_early() is a good idea is a discussion item
> > in my opinion (evidently, I thought it was when I was preparing this patch).
> 
> The situation isn't quite so simple.
> 
> We're talking specifically about disk drives.  Depending on the drive,
> it may or may not spin up all by itself when the system wakes up.  If
> it does spin up, the final state should be RPM_ACTIVE (the way it is
> now).  If it doesn't, we want the final state to be RPM_SUSPENDED.
> This is true regardless of the runtime status before the system sleep.

OK

We can do all that in .resume_early() which runs with runtime PM disabled.

> This means we can't skip the resume routine.  At the very least, the
> routine needs to ask the disk whether or not it is spinning.  Depending
> on the result, it would then set the RPM status appropriately.

Yes, .resume_early() can do that.  And it can set a (internal) flag for
a device telling the driver/bus type .resume() and .complete() to
leave the device alone.

> In addition, we only want to end up in RPM_SUSPENDED if the usage
> counters are 0 (in particular, power/control = "auto").  Otherwise, we
> want the resume routine always to spin up the drive, if it hasn't spun
> up by itself.

That still can be done in .resume_early().  Or in .resume_noirq() if device
interrupts are not requisite for that.

Thanks,
Rafael

Re: Allow runtime suspend during system resume

[Alan Stern]

On Thu, 9 Jan 2014, Rafael J. Wysocki wrote:

> > The situation isn't quite so simple.
> > 
> > We're talking specifically about disk drives.  Depending on the drive,
> > it may or may not spin up all by itself when the system wakes up.  If
> > it does spin up, the final state should be RPM_ACTIVE (the way it is
> > now).  If it doesn't, we want the final state to be RPM_SUSPENDED.
> > This is true regardless of the runtime status before the system sleep.
> 
> OK
> 
> We can do all that in .resume_early() which runs with runtime PM disabled.

Either the .resume_early or the .resume routine would be okay, as far
as I'm concerned.  Is there any strong reason to prefer one over the
other, besides the fact that in .resume_early the pm_runtime_disable
and _enable calls wouldn't be needed?

> > This means we can't skip the resume routine.  At the very least, the
> > routine needs to ask the disk whether or not it is spinning.  Depending
> > on the result, it would then set the RPM status appropriately.
> 
> Yes, .resume_early() can do that.  And it can set a (internal) flag for
> a device telling the driver/bus type .resume() and .complete() to
> leave the device alone.

I had in mind something more like this for the driver's .resume (or
.resume_early) routine:

	bool leave_suspended = false;
	int rc = 0;

	if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev))
		leave_suspended = true;
	else
		rc = spin_up_the_disk(dev);

	if (rc == 0) {
		pm_runtime_disable(dev);
		if (leave_suspended)
			pm_runtime_set_suspended(dev);
		else
			pm_runtime_set_active(dev);
		pm_runtime_enable(dev);
	}
	return rc;

That wouldn't need any special support from the PM core, other than the
new pm_runtime_in_use routine.

> > In addition, we only want to end up in RPM_SUSPENDED if the usage
> > counters are 0 (in particular, power/control = "auto").  Otherwise, we
> > want the resume routine always to spin up the drive, if it hasn't spun
> > up by itself.
> 
> That still can be done in .resume_early().  Or in .resume_noirq() if device
> interrupts are not requisite for that.

No, currently it cannot be done.

In both resume_early and resume_noirq, the usage counter is _always_ >
0.  This is because device_prepare calls pm_runtime_get_noresume, and
the corresponding pm_runtime_put doesn't occur until device_complete.  
That's the problem I need to fix.

Alan Stern

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/9/2014 10:32 AM, Alan Stern wrote:
> I had in mind something more like this for the driver's .resume
> (or .resume_early) routine:
> 
> bool leave_suspended = false; int rc = 0;
> 
> if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev)) 
> leave_suspended = true; else rc = spin_up_the_disk(dev);
> 
> if (rc == 0) { pm_runtime_disable(dev); if (leave_suspended) 
> pm_runtime_set_suspended(dev); else pm_runtime_set_active(dev); 
> pm_runtime_enable(dev); } return rc;

We also don't want to block the resume path while the REQUEST SENSE
runs, since it typically takes 10 seconds or so on normal ATA disks.
Thus, the resume or resume early needs to be able to return with the
device left in the transitioning state and complete or abort the
transition later.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzsVVAAoJEI5FoCIzSKrwLGAIAJj3GvPEcwtFvkJ6oAX+4fbf
z75OHkOkKtwOTY1SvqpgyYUMdrsTLRYO5ZPCCzZemow6s24xjEhtHVYONKogZ8Rv
lYzFpsZhWd7pLjHm5PDUSbEJiOvx9Ba+6JbRYGl9rInv2PdJOSscEpN+4a0/bvGU
F72tUfYI58su0RHcee9xXFRBOZkM61DCxNNXAyr+SHa7fIJi5jKrlNSbHPzBXAo0
g/qw8IY86a3SWE4cOGIfsrtL8GpmbwFNkT+BHWhaRlj2wj2VTP5FTZrOHndNL7pU
WC29Qxg2cKhSgm15BhNw3ESjn/3rdFaHsV96ietFKMlavXDqRmmwPsTmNK8Pbuo=
=5AE6
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Thu, 9 Jan 2014, Phillip Susi wrote:

> On 1/9/2014 10:32 AM, Alan Stern wrote:
> > I had in mind something more like this for the driver's .resume
> > (or .resume_early) routine:
> > 
> > bool leave_suspended = false; int rc = 0;
> > 
> > if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev)) 
> > leave_suspended = true; else rc = spin_up_the_disk(dev);
> > 
> > if (rc == 0) { pm_runtime_disable(dev); if (leave_suspended) 
> > pm_runtime_set_suspended(dev); else pm_runtime_set_active(dev); 
> > pm_runtime_enable(dev); } return rc;
> 
> We also don't want to block the resume path while the REQUEST SENSE
> runs, since it typically takes 10 seconds or so on normal ATA disks.
> Thus, the resume or resume early needs to be able to return with the
> device left in the transitioning state and complete or abort the
> transition later.

The code will be restructured as part of the asynchronous spin-up 
change.  The resume routine would then look more like this:

	bool test_spinning = !pm_runtime_in_use(dev);

	pm_runtime_disable(dev);
	if (test_spinning)
		pm_runtime_set_suspended(dev);
	else
		pm_runtime_set_active(dev);
	pm_runtime_enable(dev);

	start_async_spin_up(dev, test_spinning);
	return 0;

The asynchronous routine would skip the initial test if its second 
argument is nonzero, and proceed directly to spin-up the disk.

Alan Stern

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/9/2014 11:08 AM, Alan Stern wrote:
> The code will be restructured as part of the asynchronous spin-up 
> change.  The resume routine would then look more like this:
> 
> bool test_spinning = !pm_runtime_in_use(dev);
> 
> pm_runtime_disable(dev); if (test_spinning) 
> pm_runtime_set_suspended(dev); else pm_runtime_set_active(dev); 
> pm_runtime_enable(dev);
> 
> start_async_spin_up(dev, test_spinning); return 0;
> 
> The asynchronous routine would skip the initial test if its second
>  argument is nonzero, and proceed directly to spin-up the disk.

It isn't enough to background the spin up; the TEST will block, so it
needs to be async as well.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzs66AAoJEI5FoCIzSKrwY40H/1gAfH5KxfXvr9gHGMOHAdiw
VwUNKJTnADrEREOTEgWSPH8/VYyK2aFPjiGlWKvmZXMNC0CYLPRInSbhLx0cH4x1
uvezYnvVno/xME8QlLTqzVuQIrk3TKjyrd0rCBpj3YvAAyDy8TSGrJZfDAUixZkP
zoahqzIthPE52DirixVBYIamsDOZltHOjK2HbfTHZXo9m+dxQirDqL/ClQivt3Gq
e0IhRDEXsEb1b4ICQztuqNAp2BV57IzIHCd83+CTd/hkPalHFxJv6Y26IGYrORSZ
CRtXWMYiGs2/cIElSpIIQDUL60niqfFPaxsGRCT+AISwu/f4TRzAo2t/t+Vaa6c=
=aR9i
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Thu, 9 Jan 2014, Phillip Susi wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/9/2014 11:08 AM, Alan Stern wrote:
> > The code will be restructured as part of the asynchronous spin-up 
> > change.  The resume routine would then look more like this:
> > 
> > bool test_spinning = !pm_runtime_in_use(dev);
> > 
> > pm_runtime_disable(dev); if (test_spinning) 
> > pm_runtime_set_suspended(dev); else pm_runtime_set_active(dev); 
> > pm_runtime_enable(dev);
> > 
> > start_async_spin_up(dev, test_spinning); return 0;
> > 
> > The asynchronous routine would skip the initial test if its second
> >  argument is nonzero, and proceed directly to spin-up the disk.
> 
> It isn't enough to background the spin up; the TEST will block, so it
> needs to be async as well.

That's what I meant.  If the second argument is zero, the asynchronous 
routine will first test whether the disk is spinning.  If it is, the 
status will be set to RPM_ACTIVE.  (The routine may also send a START 
command -- for some disks this may be necessary.)

Alan Stern

Re: Allow runtime suspend during system resume

[Rafael J. Wysocki]

On Thursday, January 09, 2014 10:32:37 AM Alan Stern wrote:
> On Thu, 9 Jan 2014, Rafael J. Wysocki wrote:
> 
> > > The situation isn't quite so simple.
> > > 
> > > We're talking specifically about disk drives.  Depending on the drive,
> > > it may or may not spin up all by itself when the system wakes up.  If
> > > it does spin up, the final state should be RPM_ACTIVE (the way it is
> > > now).  If it doesn't, we want the final state to be RPM_SUSPENDED.
> > > This is true regardless of the runtime status before the system sleep.
> > 
> > OK
> > 
> > We can do all that in .resume_early() which runs with runtime PM disabled.
> 
> Either the .resume_early or the .resume routine would be okay, as far
> as I'm concerned.  Is there any strong reason to prefer one over the
> other, besides the fact that in .resume_early the pm_runtime_disable
> and _enable calls wouldn't be needed?

Not so much, at least I don't have anything like that in mind right now, but
I'm not a big fan of disabling/enabling runtime PM temporarily to have a look
at the "current" status.  That just doesn't seem quite right. :-)

> > > This means we can't skip the resume routine.  At the very least, the
> > > routine needs to ask the disk whether or not it is spinning.  Depending
> > > on the result, it would then set the RPM status appropriately.
> > 
> > Yes, .resume_early() can do that.  And it can set a (internal) flag for
> > a device telling the driver/bus type .resume() and .complete() to
> > leave the device alone.
> 
> I had in mind something more like this for the driver's .resume (or
> .resume_early) routine:
> 
> 	bool leave_suspended = false;
> 	int rc = 0;
> 
> 	if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev))
> 		leave_suspended = true;
> 	else
> 		rc = spin_up_the_disk(dev);
> 
> 	if (rc == 0) {
> 		pm_runtime_disable(dev);
> 		if (leave_suspended)
> 			pm_runtime_set_suspended(dev);
> 		else
> 			pm_runtime_set_active(dev);
> 		pm_runtime_enable(dev);
> 	}
> 	return rc;
> 
> That wouldn't need any special support from the PM core, other than the
> new pm_runtime_in_use routine.

That looks reasonable, but in .resume_early() you can do it like this I think:

	bool disk_spinning = disk_is_spinning(dev);
	int rc = 0;

	if (!pm_runtime_in_use(dev) && !disk_spinning) {
		pm_runtime_set_suspended(dev);
	} else {
		pm_runtime_set_active(dev);
		if (!disk_spinning)
			rc = spin_up_the_disk(dev);
	}

and of course there needs to be a flag to pass to .resume() in case the
device is supposed to be left in the suspended state.

> > > In addition, we only want to end up in RPM_SUSPENDED if the usage
> > > counters are 0 (in particular, power/control = "auto").  Otherwise, we
> > > want the resume routine always to spin up the drive, if it hasn't spun
> > > up by itself.
> > 
> > That still can be done in .resume_early().  Or in .resume_noirq() if device
> > interrupts are not requisite for that.
> 
> No, currently it cannot be done.
> 
> In both resume_early and resume_noirq, the usage counter is _always_ >
> 0.  This is because device_prepare calls pm_runtime_get_noresume, and
> the corresponding pm_runtime_put doesn't occur until device_complete.  
> That's the problem I need to fix.

Hmm.  Why do you have to compare usage_count with 0 and not with 1?

Rafael

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/09/2014 08:25 PM, Rafael J. Wysocki wrote:
> That looks reasonable, but in .resume_early() you can do it like
> this I think:
> 
> bool disk_spinning = disk_is_spinning(dev); int rc = 0;
> 
> if (!pm_runtime_in_use(dev) && !disk_spinning) { 
> pm_runtime_set_suspended(dev); } else { 
> pm_runtime_set_active(dev); if (!disk_spinning) rc =
> spin_up_the_disk(dev); }

You're not allowed to call pm_runtime_set_{suspended,active} without
disabling runtime pm.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSz1L6AAoJEI5FoCIzSKrwNWoIAKXZ6E7Vcrhbf7odxxdp11nq
vmr76+RkywUvJWDeRkq5IrgXfepyc/tEdQURNXcUvquOwbxX6NfH/Sb4DYZ+UUva
nJ9Po/vCyQAZgXodK3In7cteY4NvZLGVt9m3JkaYHKs3vkDriusSLMI1ghVNZIkW
X/VrVWhlnl9XNYWzFZuCs3ccsYpDfQ0akrxqFDoEh/mj5UUVkVXQt+JpkGPuUDD4
8yR3omZ+OFwnjqHLtzC8AOI59wujY2NPjoohQH4Rd+JTJEmAu9Ml5fqkPg90z9b2
C2mnpkEBoDEO4L1pDrD1jmxCwZQApf0qFWfu8qo+OR/VtMnxF5Cpkd+O5H+0OQg=
=Ksl4
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Rafael J. Wysocki]

On Thursday, January 09, 2014 08:55:06 PM Phillip Susi wrote:
> On 01/09/2014 08:25 PM, Rafael J. Wysocki wrote:
> > That looks reasonable, but in .resume_early() you can do it like
> > this I think:
> >
> > bool disk_spinning = disk_is_spinning(dev); int rc = 0;
> >
> > if (!pm_runtime_in_use(dev) && !disk_spinning) {
> > pm_runtime_set_suspended(dev); } else {
> > pm_runtime_set_active(dev); if (!disk_spinning) rc =
> > spin_up_the_disk(dev); }
> 
> You're not allowed to call pm_runtime_set_{suspended,active} without
> disabling runtime pm.

Well, thanks for reminding me how my code works. ;-)

.resume_early() runs with runtime PM disabled.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/10/2014 8:35 AM, Rafael J. Wysocki wrote:
> Well, thanks for reminding me how my code works. ;-)
> 
> .resume_early() runs with runtime PM disabled.

Ahh, then in that case we're back to not wanting to block the resume
path on disk_is_spinning().



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJS0Ae2AAoJEI5FoCIzSKrwiqcH/jaoQ9WtE124xT53OaD7XfZQ
C45ftL7iZG/8iF/oW48RaEa8l87yoisThDjdokuMqo6SEkxsxAegg+l9yz98tSoX
kTj3x7rE8wPAwC4JCq7+6Xu2XTiK9ujHmohwJNljsqIdhbuLo+Y4XWNrKUzag2WX
8rJbX7bPTfg0ijtOkYUHfpJyap1OkG007MIeH4qQ/vZVd8Pem3Oy1jLdyLtpUMB5
D5ebHo6IdJDJpxX2EDSyHsI5UKEDycH6VLkCz06RpeMBkL4jePzL76D//9qfw+Kc
RpOyoOFhYM5q2XeXRSjtPX4vrUe5EqdLfqYriIYiYjNNDqrirhsBDUQ5SdXr7zE=
=HLer
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Fri, 10 Jan 2014, Rafael J. Wysocki wrote:

> On Thursday, January 09, 2014 10:32:37 AM Alan Stern wrote:
> > On Thu, 9 Jan 2014, Rafael J. Wysocki wrote:
> > 
> > > > The situation isn't quite so simple.
> > > > 
> > > > We're talking specifically about disk drives.  Depending on the drive,
> > > > it may or may not spin up all by itself when the system wakes up.  If
> > > > it does spin up, the final state should be RPM_ACTIVE (the way it is
> > > > now).  If it doesn't, we want the final state to be RPM_SUSPENDED.
> > > > This is true regardless of the runtime status before the system sleep.
> > > 
> > > OK
> > > 
> > > We can do all that in .resume_early() which runs with runtime PM disabled.
> > 
> > Either the .resume_early or the .resume routine would be okay, as far
> > as I'm concerned.  Is there any strong reason to prefer one over the
> > other, besides the fact that in .resume_early the pm_runtime_disable
> > and _enable calls wouldn't be needed?
> 
> Not so much, at least I don't have anything like that in mind right now, but
> I'm not a big fan of disabling/enabling runtime PM temporarily to have a look
> at the "current" status.  That just doesn't seem quite right. :-)

The disable/enable is in order to _set_ the status, not to have a look 
at it.

> > I had in mind something more like this for the driver's .resume (or
> > .resume_early) routine:
> > 
> > 	bool leave_suspended = false;
> > 	int rc = 0;
> > 
> > 	if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev))
> > 		leave_suspended = true;
> > 	else
> > 		rc = spin_up_the_disk(dev);
> > 
> > 	if (rc == 0) {
> > 		pm_runtime_disable(dev);
> > 		if (leave_suspended)
> > 			pm_runtime_set_suspended(dev);
> > 		else
> > 			pm_runtime_set_active(dev);
> > 		pm_runtime_enable(dev);
> > 	}
> > 	return rc;
> > 
> > That wouldn't need any special support from the PM core, other than the
> > new pm_runtime_in_use routine.
> 
> That looks reasonable, but in .resume_early() you can do it like this I think:
> 
> 	bool disk_spinning = disk_is_spinning(dev);
> 	int rc = 0;
> 
> 	if (!pm_runtime_in_use(dev) && !disk_spinning) {
> 		pm_runtime_set_suspended(dev);
> 	} else {
> 		pm_runtime_set_active(dev);
> 		if (!disk_spinning)
> 			rc = spin_up_the_disk(dev);
> 	}
> 
> and of course there needs to be a flag to pass to .resume() in case the
> device is supposed to be left in the suspended state.

Agreed, there are various ways to get the same end result.  My question 
here is: How should we implement pm_runtime_in_use()?

> > > > In addition, we only want to end up in RPM_SUSPENDED if the usage
> > > > counters are 0 (in particular, power/control = "auto").  Otherwise, we
> > > > want the resume routine always to spin up the drive, if it hasn't spun
> > > > up by itself.
> > > 
> > > That still can be done in .resume_early().  Or in .resume_noirq() if device
> > > interrupts are not requisite for that.
> > 
> > No, currently it cannot be done.
> > 
> > In both resume_early and resume_noirq, the usage counter is _always_ >
> > 0.  This is because device_prepare calls pm_runtime_get_noresume, and
> > the corresponding pm_runtime_put doesn't occur until device_complete.  
> > That's the problem I need to fix.
> 
> Hmm.  Why do you have to compare usage_count with 0 and not with 1?

It is an awkward problem.  During a system sleep transition,
pm_runtime_in_use() should return 0 if the usage_count is 1 (because of
the pm_runtime_get_noresume() in device_prepare()).

But at other times (i.e., during normal operation), pm_runtime_in_use()  
should return 0 if the usage_count is 0.

I suppose a simple way to solve the problem is to document that
pm_runtime_in_use() should only be invoked from within a system sleep
callback routine.  Then we can compare against 1 and not worry about
the normal-operation case.  Maybe call it
pm_runtime_usage_during_sleep() to make this restriction explicit.

Would that be acceptable?

Alan Stern

Re: Allow runtime suspend during system resume

[Rafael J. Wysocki]

On Friday, January 10, 2014 10:25:32 AM Alan Stern wrote:
> On Fri, 10 Jan 2014, Rafael J. Wysocki wrote:
> 
> > On Thursday, January 09, 2014 10:32:37 AM Alan Stern wrote:
> > > On Thu, 9 Jan 2014, Rafael J. Wysocki wrote:
> > > 
> > > > > The situation isn't quite so simple.
> > > > > 
> > > > > We're talking specifically about disk drives.  Depending on the drive,
> > > > > it may or may not spin up all by itself when the system wakes up.  If
> > > > > it does spin up, the final state should be RPM_ACTIVE (the way it is
> > > > > now).  If it doesn't, we want the final state to be RPM_SUSPENDED.
> > > > > This is true regardless of the runtime status before the system sleep.
> > > > 
> > > > OK
> > > > 
> > > > We can do all that in .resume_early() which runs with runtime PM disabled.
> > > 
> > > Either the .resume_early or the .resume routine would be okay, as far
> > > as I'm concerned.  Is there any strong reason to prefer one over the
> > > other, besides the fact that in .resume_early the pm_runtime_disable
> > > and _enable calls wouldn't be needed?
> > 
> > Not so much, at least I don't have anything like that in mind right now, but
> > I'm not a big fan of disabling/enabling runtime PM temporarily to have a look
> > at the "current" status.  That just doesn't seem quite right. :-)
> 
> The disable/enable is in order to _set_ the status, not to have a look 
> at it.

Right.

I should have said that in my opinion disabling runtime PM in order to do
anything with the fields owned by it and re-enabling it right after that
wasn't quite right in principle.  Re-adjusting those fields *before* we
re-enable runtime PM during system resume is quite different from doing
the on-off trick *after* we've re-enabled runtime PM.

So, my opinion is that the runtime PM internal information should be made
reflect the actual (and/or desirable) state of things before it is re-enabled,
which practically means in the .resume_noirq()/.resume_early() time frame.

Doing it after that might actually work, but is questionable from the
overall consistency perspective (it basically would mean that we ran with
wrong runtime PM data for a while after we'd re-enabled it and before it
got adjusted, so we shouldn't have re-enabled it in the first place).

> > > I had in mind something more like this for the driver's .resume (or
> > > .resume_early) routine:
> > > 
> > > 	bool leave_suspended = false;
> > > 	int rc = 0;
> > > 
> > > 	if (!pm_runtime_in_use(dev) && !disk_is_spinning(dev))
> > > 		leave_suspended = true;
> > > 	else
> > > 		rc = spin_up_the_disk(dev);
> > > 
> > > 	if (rc == 0) {
> > > 		pm_runtime_disable(dev);
> > > 		if (leave_suspended)
> > > 			pm_runtime_set_suspended(dev);
> > > 		else
> > > 			pm_runtime_set_active(dev);
> > > 		pm_runtime_enable(dev);
> > > 	}
> > > 	return rc;
> > > 
> > > That wouldn't need any special support from the PM core, other than the
> > > new pm_runtime_in_use routine.
> > 
> > That looks reasonable, but in .resume_early() you can do it like this I think:
> > 
> > 	bool disk_spinning = disk_is_spinning(dev);
> > 	int rc = 0;
> > 
> > 	if (!pm_runtime_in_use(dev) && !disk_spinning) {
> > 		pm_runtime_set_suspended(dev);
> > 	} else {
> > 		pm_runtime_set_active(dev);
> > 		if (!disk_spinning)
> > 			rc = spin_up_the_disk(dev);
> > 	}
> > 
> > and of course there needs to be a flag to pass to .resume() in case the
> > device is supposed to be left in the suspended state.
> 
> Agreed, there are various ways to get the same end result.  My question 
> here is: How should we implement pm_runtime_in_use()?

In my opinion what really matters is (a) whether or not the pm_runtime_enable()
in device_resume_early() will actually enable runtime PM and (b) whether or not
the pm_runtime_put() in device_complete() will actually make it possible to
suspend the device.  So, these are the conditions that I would check.

> > > > > In addition, we only want to end up in RPM_SUSPENDED if the usage
> > > > > counters are 0 (in particular, power/control = "auto").  Otherwise, we
> > > > > want the resume routine always to spin up the drive, if it hasn't spun
> > > > > up by itself.
> > > > 
> > > > That still can be done in .resume_early().  Or in .resume_noirq() if device
> > > > interrupts are not requisite for that.
> > > 
> > > No, currently it cannot be done.
> > > 
> > > In both resume_early and resume_noirq, the usage counter is _always_ >
> > > 0.  This is because device_prepare calls pm_runtime_get_noresume, and
> > > the corresponding pm_runtime_put doesn't occur until device_complete.  
> > > That's the problem I need to fix.
> > 
> > Hmm.  Why do you have to compare usage_count with 0 and not with 1?
> 
> It is an awkward problem.  During a system sleep transition,
> pm_runtime_in_use() should return 0 if the usage_count is 1 (because of
> the pm_runtime_get_noresume() in device_prepare()).
> 
> But at other times (i.e., during normal operation), pm_runtime_in_use()  
> should return 0 if the usage_count is 0.
> 
> I suppose a simple way to solve the problem is to document that
> pm_runtime_in_use() should only be invoked from within a system sleep
> callback routine.  Then we can compare against 1 and not worry about
> the normal-operation case.  Maybe call it
> pm_runtime_usage_during_sleep() to make this restriction explicit.
> 
> Would that be acceptable?

Yes, it would, as far as I'm concerned.

In my opinion it only is really useful to call it before calling
pm_runtime_enable() in device_resume_early().

Thanks,
Rafael

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/10/2014 06:02 PM, Rafael J. Wysocki wrote:
> So, my opinion is that the runtime PM internal information should
> be made reflect the actual (and/or desirable) state of things
> before it is re-enabled, which practically means in the
> .resume_noirq()/.resume_early() time frame.
> 
> Doing it after that might actually work, but is questionable from
> the overall consistency perspective (it basically would mean that
> we ran with wrong runtime PM data for a while after we'd re-enabled
> it and before it got adjusted, so we shouldn't have re-enabled it
> in the first place).

But we don't want to block the system resume path for too long while
we figure out what the correct state is.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJS0KedAAoJEI5FoCIzSKrw41EH/0phrR8FKj2D8H2YidMluZ0/
0crckeYkz4VMEkxMrzjnFl1sxwaWLfYW1GOpJ6FnGZqArjLr0GktOHBJfVTWqNqI
bNIFW5rKX/JKgns3Nmv1Zx5twxHuIfVtAF2mjgorabyKyRpaVwsiZa9ogLgBnUVr
ipLYX1mBRvmoMPDHYB3/wrefc4c0Eg4n1O24/wVhehxtsBCeN9BKUUEcIRvkiRUj
FwMmWXuZMlTGK2LhjmgCMuH7Ea9r8sAJWUkGA8zJJLuS8y7kK9grUwAW5IhfrJGH
Wg5sBce8sKhoX7h5gze+rVktrM1B4DDzEt4J356lmIjfeQ75rUrHMzEDvzh021M=
=+ubG
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Fri, 10 Jan 2014, Phillip Susi wrote:

> On 01/10/2014 06:02 PM, Rafael J. Wysocki wrote:
> > So, my opinion is that the runtime PM internal information should
> > be made reflect the actual (and/or desirable) state of things
> > before it is re-enabled, which practically means in the
> > .resume_noirq()/.resume_early() time frame.
> > 
> > Doing it after that might actually work, but is questionable from
> > the overall consistency perspective (it basically would mean that
> > we ran with wrong runtime PM data for a while after we'd re-enabled
> > it and before it got adjusted, so we shouldn't have re-enabled it
> > in the first place).
> 
> But we don't want to block the system resume path for too long while
> we figure out what the correct state is.

We won't.  The resume_early routine will set the status to RPM_ACTIVE 
or RPM_SUSPENDED, according to the result from 
pm_runtime_usage_during_sleep.  It will also do a pm_runtime_get_sync 
on the disk's parent, to prevent the parent from going into runtime 
suspend before the async routine can run.

Then the async routine will do the following:

	If the status is RPM_ACTIVE, spin up the disk.

	Else (the status is RPM_SUSPENDED) check whether the disk has
	spun up by itself.  If it has, or if you can't tell, call 
	pm_runtime_resume to set the status correctly and do an 
	explicit spin-up.

	Call pm_runtime_put_sync on the parent.

I think this will give the desired result.  But there is one potential
problem: If the disk has a child device (such as a partition) and the
child's driver needs to do I/O as part of its resume routine, the I/O
is likely to hang if the disk is runtime suspended.  However, I don't
know of any child devices like this.

Alan Stern

Re: Allow runtime suspend during system resume

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

On 01/11/2014 05:50 PM, Alan Stern wrote:
> We won't.  The resume_early routine will set the status to
> RPM_ACTIVE or RPM_SUSPENDED, according to the result from 
> pm_runtime_usage_during_sleep.  It will also do a
> pm_runtime_get_sync on the disk's parent, to prevent the parent
> from going into runtime suspend before the async routine can run.
> 
> Then the async routine will do the following:
> 
> If the status is RPM_ACTIVE, spin up the disk.
> 
> Else (the status is RPM_SUSPENDED) check whether the disk has spun
> up by itself.  If it has, or if you can't tell, call 
> pm_runtime_resume to set the status correctly and do an explicit
> spin-up.
> 
> Call pm_runtime_put_sync on the parent.

I see now... thanks.

> I think this will give the desired result.  But there is one
> potential problem: If the disk has a child device (such as a
> partition) and the child's driver needs to do I/O as part of its
> resume routine, the I/O is likely to hang if the disk is runtime
> suspended.  However, I don't know of any child devices like this.

Probably should check dm and md to see if they do IO in their resume.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJS0fTbAAoJEI5FoCIzSKrwTVoH/3Cl4ZEmE+5aUJX5+mCGmoPD
Ap+UHe517qsfDWwKaDqURTcXaZOqMfbk71jdTCAIIVTSPB95NWFQg3d/XpWAVY29
yoXdUj28WHTlgtfhtqa92K7lImOijxJ/32myEK9/+dQBDXXnG4n+jt1eL+vo9Rhy
vlcAJLKcCsL4SpDD8z/N2lvsmJ5F1vIB5SkM7GiyaR3HHfRIM3hzV4JErtHozgWE
x2zNfD6DXk7CR3GnISq2bznIYvRlfsmvzwCjfJ3AYBJdtFQmuwMDznaVKlCpy/Ya
G07n0HqghB3ZYseZLZt8HyB/men72SZolJCTb/UnkODTsymq9+6J3XrJ6bb29Vc=
=V+EM
-----END PGP SIGNATURE-----

Re: Allow runtime suspend during system resume

[Alan Stern]

On Sat, 11 Jan 2014, Rafael J. Wysocki wrote:

> I should have said that in my opinion disabling runtime PM in order to do
> anything with the fields owned by it and re-enabling it right after that
> wasn't quite right in principle.  Re-adjusting those fields *before* we
> re-enable runtime PM during system resume is quite different from doing
> the on-off trick *after* we've re-enabled runtime PM.
> 
> So, my opinion is that the runtime PM internal information should be made
> reflect the actual (and/or desirable) state of things before it is re-enabled,
> which practically means in the .resume_noirq()/.resume_early() time frame.

That makes sense.

> Doing it after that might actually work, but is questionable from the
> overall consistency perspective (it basically would mean that we ran with
> wrong runtime PM data for a while after we'd re-enabled it and before it
> got adjusted, so we shouldn't have re-enabled it in the first place).

Of course, the middle of a system resume is a rather special time.  
Still, I agree with your basic point.

> > Agreed, there are various ways to get the same end result.  My question 
> > here is: How should we implement pm_runtime_in_use()?
> 
> In my opinion what really matters is (a) whether or not the pm_runtime_enable()
> in device_resume_early() will actually enable runtime PM and (b) whether or not
> the pm_runtime_put() in device_complete() will actually make it possible to
> suspend the device.  So, these are the conditions that I would check.

Yes.  The key being that the check will give the correct result only
during the _noirq and _late/_early phases of system sleep, not at other
times.

> > I suppose a simple way to solve the problem is to document that
> > pm_runtime_in_use() should only be invoked from within a system sleep
> > callback routine.  Then we can compare against 1 and not worry about
> > the normal-operation case.  Maybe call it
> > pm_runtime_usage_during_sleep() to make this restriction explicit.
> > 
> > Would that be acceptable?
> 
> Yes, it would, as far as I'm concerned.
> 
> In my opinion it only is really useful to call it before calling
> pm_runtime_enable() in device_resume_early().

Okay, I'll put something together.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2014 12:46 PM, Alan Stern wrote:
> I/O _was_ done.  To spin up the disk requires sending it a START 
> command.  That's I/O.

Yes, currently, but that's exactly what I'm trying to get rid of.

> In essence, you want your disk's state to move directly from
> unpowered (system asleep) to runtime-suspended with no intermediate
> active -- but only if the disk doesn't spin up all by itself.
> 
> One problem: How can the kernel tell whether the disk has spun up
> by itself?  I don't think it can be done at the SCSI level.  Can it
> be done at the ATA level (and without using the SCSI stack, as that
> would require the disk to go out of runtime suspend and spin up
> anyway)?

You issue a REQUEST SENSE command and that returns status indicating
whether the drive is stopped, or in standby.  See my patches.  One of
them fixes the scsi-ata translation to issue the ATA CHECK power
command to see if the drive is suspended, as SAT-3 calls for.

> Actually, it does have a reason.  Namely, you told it earlier to
> assume the disk will be needed again if it was used within the last
> 5 minutes.  At the time of the system resume, the disk was last
> used 3 minutes ago (not counting the time the system was asleep).

You're looking at it backwards.  You aren't telling it to assume the
disk will be needed again if it was used within the last 5 minutes;
you are telling it that it can assume it *won't* be needed again soon
if not used in the last 5 minutes, and therefore, it is ok to shut it
down.

Also the act of suspending the system is a pretty clear breaking point
in general activity.  You normally stop your work and quiesce the
system before you suspend it.  If I finish my work and copy it to my
backup drive, then suspend the system, and my wife comes by an hour
later to browse the web, she's not going to be touching my backup disk.

> Are you assuming these disks were all runtime-active before the
> system sleep?  If so, the kernel is justified in assuming that you
> will be touching them all soon.

No it isn't.  Absence of evidence is not evidence of absence.  I
listed several use cases where the drive won't be runtime suspended
before the system suspend, yet will not be accessed any time soon
after resume.

> Not the ones that crash when they receive it.  But this point is
> moot since manage_start_stop doesn't do what you want anyway.

Any that crash when they get it either are already unusable or have a
quirk registered to inhibit the command, which would still apply.

> No, it doesn't.  They don't spin, so they can't be configured
> either to spin up or not to spin up after power-on.

Which means we shouldn't be setting the runtime status to suspended
after system resume... that's the second case.

> What is PuiS?

Power up in Standby.

> In the end, it sounds like what you want can be done fairly easily.
>  But only if the kernel has a reliable way to tell whether or not a
> disk is spinning (or is spinning up).

That's why my patches are attempting to do: use REQUEST SENSE to ask
the disk if it is spinning up or not.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzbMqAAoJEI5FoCIzSKrwCYwIAKMwlRovIdfO6rSgZL7pufFG
LOV/9apHzqldPPLJ5BPk0A69Ok/TimrQax06HTPyGJ1/MmOrEcjJzb482mJ4ixpx
b7DP3De+RCEUifrfIlI/U+fWLHpw4DOYToSy5ZjNZbF/cDz43k85arAwRK/Bkb++
C2F+YfeQyLcR0KjToj8MK9evlGY6oKfEYx9QCjBEgiaXRjIHfu0AuLn7y7AATK6n
pFBzgq4cNSalob9I4WdwejzJz/RzqQFujtknB+nAaKhESqLCoclJLSzwNQyuxcRz
qT3DckTyTBF7vGmYayaeERnukqWnDEBAQOc8qtOxx2rbYpivNHiqIc8sfrRoaJc=
=2kXd
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Wed, 8 Jan 2014, Phillip Susi wrote:

> > In essence, you want your disk's state to move directly from
> > unpowered (system asleep) to runtime-suspended with no intermediate
> > active -- but only if the disk doesn't spin up all by itself.
> > 
> > One problem: How can the kernel tell whether the disk has spun up
> > by itself?  I don't think it can be done at the SCSI level.  Can it
> > be done at the ATA level (and without using the SCSI stack, as that
> > would require the disk to go out of runtime suspend and spin up
> > anyway)?
> 
> You issue a REQUEST SENSE command and that returns status indicating
> whether the drive is stopped, or in standby.  See my patches.  One of

I never saw your patches.  Where were they posted?

If you issue the REQUEST SENSE command in the usual way (going through
the SCSI and block layers), and the disk is already in runtime suspend,
it won't do what you want.  The block layer won't deliver requests
until the device leaves the RPM_SUSPENDED state.  In addition, when it
receives the command the block layer will submit a deferred
runtime-resume request, which rather defeats the whole purpose.

(I guess you actually saw some of this happen, and that's what led to
this email thread in the first place.)

It's a knotty situation.  The only way to find out whether the disk is
spinning is to ask it, which requires doing I/O, which requires
spinning up the disk.  Maybe we need to add a mechanism to the block
layer's runtime PM implementation for addressing just this situation.

> them fixes the scsi-ata translation to issue the ATA CHECK power
> command to see if the drive is suspended, as SAT-3 calls for.

What happens with non-ATA disks?


> > Actually, it does have a reason.  Namely, you told it earlier to
> > assume the disk will be needed again if it was used within the last
> > 5 minutes.  At the time of the system resume, the disk was last
> > used 3 minutes ago (not counting the time the system was asleep).
> 
> You're looking at it backwards.  You aren't telling it to assume the
> disk will be needed again if it was used within the last 5 minutes;
> you are telling it that it can assume it *won't* be needed again soon
> if not used in the last 5 minutes, and therefore, it is ok to shut it
> down.

Okay, yes, the autosuspend timeout affects when devices should be
powered down, not when they should be powered back up.

> Also the act of suspending the system is a pretty clear breaking point
> in general activity.  You normally stop your work and quiesce the
> system before you suspend it.  If I finish my work and copy it to my
> backup drive, then suspend the system, and my wife comes by an hour
> later to browse the web, she's not going to be touching my backup disk.

While that's true, it's also true that any userspace processes running 
before the system suspend will continue running, with no perceptible 
break, after the system wakes up.  If they were accessing the disk 
before, they will most likely continue to access it afterward.

But never mind all this; it's a pointless discussion.  The real 
question is how to implement what you want.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2014 4:21 PM, Alan Stern wrote:
> I never saw your patches.  Where were they posted?

Higher up in this thread on linux-ide and linux-scsi.  Subject was Let
sleeping disks lie.

> If you issue the REQUEST SENSE command in the usual way (going
> through the SCSI and block layers), and the disk is already in
> runtime suspend, it won't do what you want.  The block layer won't
> deliver requests until the device leaves the RPM_SUSPENDED state.
> In addition, when it receives the command the block layer will
> submit a deferred runtime-resume request, which rather defeats the
> whole purpose.

Right, which is why I left the device in the active state and used the
pre/post_suspend functions to change the queue into the RPM_SUSPENDING
state, then either complete the transition to RPM_SUSPENDED, or bail
out and go back to RPM_ACTIVE.

> What happens with non-ATA disks?

Same thing: they return a sense status that either says they are
suspended, stopped, or normal.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzcg2AAoJEI5FoCIzSKrw7wcIAJLiliMYnoHLm/dGWHhhfcnb
ELJLLAhhWsOIQmHiV1svdy0F7EuKi3KF9qJ9JwCaAspO0w8q24UGhvQW20uSH9Md
JliKhn6eBY/NFTctaVP2tnOcc4vevCndJfEScBlWxI82MWMC1TV3lA7xPtcJ5ocX
LMBLPXpr0qypisgmu/tAnwTPQVyU4WUkgMmG9us4w3BgCkvI/oXf4oDKFkCNgUye
FX16NTZPR6iaIK+YJMG3uKCSD4CQjTnKJNXGR89XKJG+Z9v04jyQgWs7kgvqCov9
F5j+yueyMVlveGPHmNwhElHEPT2UWSoyQbonuOrYQZ+Db+xJBOxzoY8Jgq9lWVI=
=+vwf
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/09/2014 05:21 AM, Alan Stern wrote:
> On Wed, 8 Jan 2014, Phillip Susi wrote:
>> You issue a REQUEST SENSE command and that returns status indicating
>> whether the drive is stopped, or in standby.  See my patches.  One of
> 
> I never saw your patches.  Where were they posted?
> 
> If you issue the REQUEST SENSE command in the usual way (going through
> the SCSI and block layers), and the disk is already in runtime suspend,
> it won't do what you want.  The block layer won't deliver requests
> until the device leaves the RPM_SUSPENDED state.  In addition, when it
> receives the command the block layer will submit a deferred
> runtime-resume request, which rather defeats the whole purpose.
> 
> (I guess you actually saw some of this happen, and that's what led to
> this email thread in the first place.)
> 
> It's a knotty situation.  The only way to find out whether the disk is
> spinning is to ask it, which requires doing I/O, which requires
> spinning up the disk.  Maybe we need to add a mechanism to the block
> layer's runtime PM implementation for addressing just this situation.

I think it's knotty because the runtime PM status is a view from the
kernel/host side, i.e. it is runtime suspended if it is not being used,
no matter which power state it is in. The trigger for the PM state
transition are all based on this, if we want to do it the other way
around(update device's runtime PM status based on device's actual power
state), we are in a knotty situation.

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Rafael J. Wysocki]

On Thursday, January 09, 2014 09:29:59 AM Aaron Lu wrote:
> On 01/09/2014 05:21 AM, Alan Stern wrote:
> > On Wed, 8 Jan 2014, Phillip Susi wrote:
> >> You issue a REQUEST SENSE command and that returns status indicating
> >> whether the drive is stopped, or in standby.  See my patches.  One of
> > 
> > I never saw your patches.  Where were they posted?
> > 
> > If you issue the REQUEST SENSE command in the usual way (going through
> > the SCSI and block layers), and the disk is already in runtime suspend,
> > it won't do what you want.  The block layer won't deliver requests
> > until the device leaves the RPM_SUSPENDED state.  In addition, when it
> > receives the command the block layer will submit a deferred
> > runtime-resume request, which rather defeats the whole purpose.
> > 
> > (I guess you actually saw some of this happen, and that's what led to
> > this email thread in the first place.)
> > 
> > It's a knotty situation.  The only way to find out whether the disk is
> > spinning is to ask it, which requires doing I/O, which requires
> > spinning up the disk.  Maybe we need to add a mechanism to the block
> > layer's runtime PM implementation for addressing just this situation.
> 
> I think it's knotty because the runtime PM status is a view from the
> kernel/host side, i.e. it is runtime suspended if it is not being used,
> no matter which power state it is in. The trigger for the PM state
> transition are all based on this, if we want to do it the other way
> around(update device's runtime PM status based on device's actual power
> state), we are in a knotty situation.

Agreed.

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Rafael J. Wysocki]

On Thursday, January 09, 2014 01:17:12 PM Rafael J. Wysocki wrote:
> On Thursday, January 09, 2014 09:29:59 AM Aaron Lu wrote:
> > On 01/09/2014 05:21 AM, Alan Stern wrote:
> > > On Wed, 8 Jan 2014, Phillip Susi wrote:
> > >> You issue a REQUEST SENSE command and that returns status indicating
> > >> whether the drive is stopped, or in standby.  See my patches.  One of
> > > 
> > > I never saw your patches.  Where were they posted?
> > > 
> > > If you issue the REQUEST SENSE command in the usual way (going through
> > > the SCSI and block layers), and the disk is already in runtime suspend,
> > > it won't do what you want.  The block layer won't deliver requests
> > > until the device leaves the RPM_SUSPENDED state.  In addition, when it
> > > receives the command the block layer will submit a deferred
> > > runtime-resume request, which rather defeats the whole purpose.
> > > 
> > > (I guess you actually saw some of this happen, and that's what led to
> > > this email thread in the first place.)
> > > 
> > > It's a knotty situation.  The only way to find out whether the disk is
> > > spinning is to ask it, which requires doing I/O, which requires
> > > spinning up the disk.  Maybe we need to add a mechanism to the block
> > > layer's runtime PM implementation for addressing just this situation.
> > 
> > I think it's knotty because the runtime PM status is a view from the
> > kernel/host side, i.e. it is runtime suspended if it is not being used,
> > no matter which power state it is in. The trigger for the PM state
> > transition are all based on this, if we want to do it the other way
> > around(update device's runtime PM status based on device's actual power
> > state), we are in a knotty situation.
> 
> Agreed.

That said during system resume, when we are trying to avoid resuming the
device to save time/energy, it makes sense to check its physical state and
do a pm_request_resume() if that is "on" to avoid a situation in which the
device is physically "on", but its runtime PM status is "suspended" and it
never gets powered down because of that.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Thu, 9 Jan 2014, Aaron Lu wrote:

> > It's a knotty situation.  The only way to find out whether the disk is
> > spinning is to ask it, which requires doing I/O, which requires
> > spinning up the disk.  Maybe we need to add a mechanism to the block
> > layer's runtime PM implementation for addressing just this situation.
> 
> I think it's knotty because the runtime PM status is a view from the
> kernel/host side, i.e. it is runtime suspended if it is not being used,
> no matter which power state it is in. The trigger for the PM state
> transition are all based on this, if we want to do it the other way
> around(update device's runtime PM status based on device's actual power
> state), we are in a knotty situation.

No, that's not the problem.  The problem is that the block layer does
not permit any requests to go through if the queue's rpm_status is
RPM_SUSPENDED.

We should change the code in the block layer.  blk_pm_peek_request 
should allow requests with REQ_PM set to go through, no matter what the 
rpm_status is.  Then the disk driver could query the disk (to see 
whether it is spinning) while the disk remains in runtime suspend.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/9/2014 10:40 AM, Alan Stern wrote:
> We should change the code in the block layer.  blk_pm_peek_request
>  should allow requests with REQ_PM set to go through, no matter
> what the rpm_status is.  Then the disk driver could query the disk
> (to see whether it is spinning) while the disk remains in runtime
> suspend.

No, because if the disk is RPM_SUSPENDED, the port may be
RPM_SUSPENDED, which likely means that you CAN'T send down requests to
the device.

We need to put the device into one of the transitioning states to
block other IO, without allowing the port to suspend.


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzsXvAAoJEI5FoCIzSKrwg30H/A52cy6+GAzE372KVMkP/bI+
LQe4FTTIjo8PDBuYeASzmjdiD8x51++w/QfUQEtSTEhr7qWhyXdMbb1B6eGuDlv/
KqOBTTuhC7eiLArGLTGyLRiLOLEiQb7lHStw4ZCpQpZ88f9pNPdcLCslp8zrz4Of
6pt2lLztdG8zexmKyu0Zq/oSe6ajKqVgrSqjoCO9jQL7NUecZNlBnOnsF4jri0MY
GPk3HIb0gZjtVVfeWAiEE9s4MpRlMYHfx+dg8uxNJiAhJcMQeHJLCTKAKyEMKG68
45Ehqp5XITJG8Mx/Mb6gOzA/mFDShzqRiA0i6MtKBcnr+zNeRnWKRV1UbDKcrxk=
=B5lj
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Thu, 9 Jan 2014, Phillip Susi wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/9/2014 10:40 AM, Alan Stern wrote:
> > We should change the code in the block layer.  blk_pm_peek_request
> >  should allow requests with REQ_PM set to go through, no matter
> > what the rpm_status is.  Then the disk driver could query the disk
> > (to see whether it is spinning) while the disk remains in runtime
> > suspend.
> 
> No, because if the disk is RPM_SUSPENDED, the port may be
> RPM_SUSPENDED, which likely means that you CAN'T send down requests to
> the device.

That's true, but it isn't a problem.  We know that requests with REQ_PM
are sent only at certain, controlled times.  In particular, the only
time such a request would be sent while the disk is RPM_SUSPENDED is
during a system resume.

> We need to put the device into one of the transitioning states to
> block other IO, without allowing the port to suspend.

No, we only need to make sure that the port doesn't go into runtime 
suspend while we carry out the "is the disk spinning" test.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/9/2014 11:14 AM, Alan Stern wrote:
> That's true, but it isn't a problem.  We know that requests with
> REQ_PM are sent only at certain, controlled times.  In particular,
> the only time such a request would be sent while the disk is
> RPM_SUSPENDED is during a system resume.

Yes, but if the disk and port were both already RPM_SUSPENDED when the
system was suspended, and so the port is still RPM_SUSPENDED when the
disk's system resume method is called, then it can't communicate with
the disk.

>> We need to put the device into one of the transitioning states
>> to block other IO, without allowing the port to suspend.
> 
> No, we only need to make sure that the port doesn't go into runtime
>  suspend while we carry out the "is the disk spinning" test.

Well how else do you do that other than with the transition states?


-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJSzs+RAAoJEI5FoCIzSKrw1KoH+gJ4c1iXqOUUqJfF2xoxrF6k
Fv3KyTNMn8Y9RF12o5OlfQHwPkwhplIR7uBd5agnd9cXR2owRVLgAE34/ggEEOOq
NgaRwfNBbD3Q83I/6aEZ7C7A0wbWyrA+w2aaBIe5RzTslH3IYcePZvnKUZuG+/0A
rAR/IVyNL5e6JDPgvSkx7/LmxLVb3M4NkYeCMHvl8Gmg+bvg2S+R3TAcUTvRzRv4
gwtkbYGkyS/NmXc86Vi7Z2pOcIyZT3T0CyzlSqAIjljYSFJbzY+PRP4D4gAOXqyd
vqrg+PtxUYlQ0POdYRo0TtCe2N5nk4RI3jwiyD0U1/GFgcY/uNNJOtPaiQOLxOU=
=cMp1
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Thu, 9 Jan 2014, Phillip Susi wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 1/9/2014 11:14 AM, Alan Stern wrote:
> > That's true, but it isn't a problem.  We know that requests with
> > REQ_PM are sent only at certain, controlled times.  In particular,
> > the only time such a request would be sent while the disk is
> > RPM_SUSPENDED is during a system resume.
> 
> Yes, but if the disk and port were both already RPM_SUSPENDED when the
> system was suspended, and so the port is still RPM_SUSPENDED when the
> disk's system resume method is called, then it can't communicate with
> the disk.

The port should not be runtime suspended during system resume.  It 
should always go to runtime active.
 
> >> We need to put the device into one of the transitioning states
> >> to block other IO, without allowing the port to suspend.
> > 
> > No, we only need to make sure that the port doesn't go into runtime
> >  suspend while we carry out the "is the disk spinning" test.
> 
> Well how else do you do that other than with the transition states?

pm_runtime_get_noresume(dev->parent).

Alan Stern