[PATCH/RESEND v2 0/2] SATA disk resume time optimization

[PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Todd E Brandt]

The essential issue behind hard disks' lengthy resume time is the ata port driver blocking until the ATA port hardware is finished coming online. So the kernel isn't really doing anything during all those seconds that the disks are resuming, it's just blocking until the hardware says it's ready to accept commands. Applying this patch set allows SATA disks to resume asynchronously without holding up system resume, thus allowing the UI to come online much more quickly. There may be a short period after resume where the disks are still spinning up in the background, but the user shouldn't notice since the OS can function with the data left in RAM.

The patch set has two parts which apply to ata_port_resume and sd_resume respectively. Both are required to achieve any real performance benefit, but they will still function independantly without a performance hit.

ata_port_resume patch (1/2):

On resume, the ATA port driver currently waits until the AHCI controller finishes executing the port wakeup command. This patch changes the ata_port_resume callback to issue the wakeup and then return immediately, thus allowing the next device in the pm queue to resume. Any commands issued to the AHCI hardware during the wakeup will be queued up and executed once the port is physically online. Thus no information is lost.

sd_resume patch (2/2):

On resume, the SD driver currently waits until the block driver finishes executing a disk start command with blk_execute_rq. This patch changes the sd_resume callback to use blk_execute_rq_nowait instead, which allows it to return immediately, thus allowing the next device in the pm queue to resume. The return value of blk_execute_rq_nowait is handled in the background by sd_resume_complete. Any commands issued to the scsi disk during the startup will be queued up and executed once the disk is online. Thus no information is lost.

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Phillip Susi]

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

On 10/17/2013 03:33 PM, Todd E Brandt wrote:
> The essential issue behind hard disks' lengthy resume time is the
> ata port driver blocking until the ATA port hardware is finished
> coming online. So the kernel isn't really doing anything during all
> those seconds that the disks are resuming, it's just blocking until
> the hardware says it's ready to accept commands. Applying this
> patch set

I have been working in a similar direction and have patches to follow.
 Instead of backgrounding the start in sd_resume, I have removed it
entirely, and background the resume at the ata port layer, as well as
avoid resuming Power on in Standby disks entirely.


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

iQEcBAEBCgAGBQJSevKEAAoJEJrBOlT6nu75FFkIAL8mwXOuuNvY5GoppHVMcweC
EOBz6t2Xmc/TsGYr9u4kRVXbf3G0trSM9dj/XSJ6p+Dj/su/vJ69yKxp7sUPidgz
zqjfKlG+myMQIaAO+tPGXvMcmT74BXjgjobZw2lUyLyRPLD9elDGmrDA5ZXZ+hiz
hzSx4gOC57j+sK/pmoZ7U/CWeWYEYQ+J9xBXm0hzbBbSTjpzVsR0T+7xU+WVkeX0
Ox++FFBZp2b1xJgaREurRv1mORx/GJ+PfSETb49P5BWaWW+QVwOEhWOX/JQNc/Pp
ANJmttP4WPvlRV0X3hD6FvAYSnu1qbwwGOiP/j5g7mklyfm5alA0jcuahc1w5tI=
=9KhA
-----END PGP SIGNATURE-----

[PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

Don't bother forcing disks to spin up on resume, as they
will do so automatically when accessed, and forcing them
to spin up slows down the resume.  Add a second bit to the
manage_start_stop flag to restore the previous behavior.
---
 drivers/scsi/sd.c          | 6 +++---
 include/scsi/scsi_device.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..3143311 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3083,7 +3083,7 @@ static void sd_shutdown(struct device *dev)
 		sd_sync_cache(sdkp);
 	}
 
-	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
+	if (system_state != SYSTEM_RESTART && (sdkp->device->manage_start_stop & 1)) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		sd_start_stop_device(sdkp, 0);
 	}
@@ -3107,7 +3107,7 @@ static int sd_suspend(struct device *dev)
 			goto done;
 	}
 
-	if (sdkp->device->manage_start_stop) {
+	if (sdkp->device->manage_start_stop & 1) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}
@@ -3122,7 +3122,7 @@ static int sd_resume(struct device *dev)
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
 
-	if (!sdkp->device->manage_start_stop)
+	if (!(sdkp->device->manage_start_stop & 2))
 		goto done;
 
 	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d65fbec..1c46d2d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -152,7 +152,7 @@ struct scsi_device {
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
-	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
+	unsigned manage_start_stop:2;	/* Let HLD (sd) manage start/stop */
 	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
 	unsigned select_no_atn:1;
-- 
1.8.3.2

[PATCH 2/3] libata: resume in the background

[Phillip Susi]

Don't block the resume path waiting for the disk to
spin up.
---
 drivers/ata/libata-core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 573d151..16f45dc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5420,20 +5420,18 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 {
 	struct ata_port *ap = to_ata_port(dev);
+	static int dontcare;
 
-	return __ata_port_resume_common(ap, mesg, NULL);
+	return __ata_port_resume_common(ap, mesg, &dontcare);
 }
 
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
 	rc = ata_port_resume_common(dev, PMSG_RESUME);
-	if (!rc) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
 
 	return rc;
 }
-- 
1.8.3.2

[PATCH 3/3] libata: don't start disks on resume

[Phillip Susi]

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Suppress this until the disk
is actually accessed.
---
 drivers/ata/libata-core.c | 10 +++++++---
 drivers/ata/libata-eh.c   | 11 +++++++++++
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 16f45dc..06b81fc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1993,7 +1993,10 @@ retry:
 			goto err_out;
 	}
 
-	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
+	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
+		return -EAGAIN;
+	if (!(flags & ATA_READID_NOSTART) &&
+	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
 		tried_spinup = 1;
 		/*
 		 * Drive powered-up in standby mode, and requires a specific
@@ -3982,7 +3985,6 @@ int ata_dev_reread_id(struct ata_device *dev, unsigned int readid_flags)
 	if (!ata_dev_same_device(dev, class, id))
 		return -ENODEV;
 
-	memcpy(dev->id, id, sizeof(id[0]) * ATA_ID_WORDS);
 	return 0;
 }
 
@@ -4024,6 +4026,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 
 	/* re-read ID */
 	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc == -EAGAIN)
+		return rc;
 	if (rc)
 		goto fail;
 
@@ -5413,7 +5417,7 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 	int rc;
 
 	rc = ata_port_request_pm(ap, mesg, ATA_EH_RESET,
-		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, async);
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET | ATA_EHI_NOSTART, async);
 	return rc;
 }
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c69fcce..b4fcf62 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3086,6 +3086,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
+		if (ehc->i.flags & ATA_EHI_NOSTART)
+			readid_flags |= ATA_READID_NOSTART;
 
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
@@ -3098,6 +3100,10 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
 			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
 						readid_flags);
+			if (rc == -EAGAIN) {
+				rc = 0; /* start required but suppressed, handle later */
+				continue;
+			}
 			if (rc)
 				goto err;
 
@@ -3830,6 +3836,11 @@ int ata_eh_recover(struct ata_port *ap, ata_prereset_fn_t prereset,
 
 		/* revalidate existing devices and attach new ones */
 		rc = ata_eh_revalidate_and_attach(link, &dev);
+		if (rc == -EAGAIN) {
+			/* spinup required, but suppressed, ignore for now */
+			rc = 0;
+			continue;
+		}
 		if (rc)
 			goto rest_fail;
 
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index eeeb778..146d62d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -42,6 +42,7 @@ struct ata_scsi_args {
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
 
 	/* selector for ata_down_xfermask_limit() */
 	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0e23c26..3929bb8 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -362,6 +362,7 @@ enum {
 
 	/* ata_eh_info->flags */
 	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */
-- 
1.8.3.2

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Douglas Gilbert]

On 13-11-06 08:57 PM, Phillip Susi wrote:
> Don't bother forcing disks to spin up on resume, as they
> will do so automatically when accessed, and forcing them
> to spin up slows down the resume.  Add a second bit to the
> manage_start_stop flag to restore the previous behavior.

SCSI disks when in STOP state do not spin up "automatically
when accessed".

If you haven't looked at the SAT-2 standard (SAT-3 drafts)
then perhaps you should as that gives insights into how SCSI
and ATA disks can play together in the same sandpit (and the
code you are patching is in the SCSI part of the sandpit).

And your choice of bits looks like it will favour fixing
broken SATA behaviour but as a by-product break working
SCSI disk behaviour.

Doug Gilbert

> ---
>   drivers/scsi/sd.c          | 6 +++---
>   include/scsi/scsi_device.h | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..3143311 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3083,7 +3083,7 @@ static void sd_shutdown(struct device *dev)
>   		sd_sync_cache(sdkp);
>   	}
>
> -	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> +	if (system_state != SYSTEM_RESTART && (sdkp->device->manage_start_stop & 1)) {
>   		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>   		sd_start_stop_device(sdkp, 0);
>   	}
> @@ -3107,7 +3107,7 @@ static int sd_suspend(struct device *dev)
>   			goto done;
>   	}
>
> -	if (sdkp->device->manage_start_stop) {
> +	if (sdkp->device->manage_start_stop & 1) {
>   		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
>   		ret = sd_start_stop_device(sdkp, 0);
>   	}
> @@ -3122,7 +3122,7 @@ static int sd_resume(struct device *dev)
>   	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
>   	int ret = 0;
>
> -	if (!sdkp->device->manage_start_stop)
> +	if (!(sdkp->device->manage_start_stop & 2))
>   		goto done;
>
>   	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index d65fbec..1c46d2d 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -152,7 +152,7 @@ struct scsi_device {
>   	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
>   	unsigned no_start_on_add:1;	/* do not issue start on add */
>   	unsigned allow_restart:1; /* issue START_UNIT in error handler */
> -	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
> +	unsigned manage_start_stop:2;	/* Let HLD (sd) manage start/stop */
>   	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
>   	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
>   	unsigned select_no_atn:1;
>

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/7/2013 1:21 PM, Douglas Gilbert wrote:
> On 13-11-06 08:57 PM, Phillip Susi wrote:
>> Don't bother forcing disks to spin up on resume, as they will do
>> so automatically when accessed, and forcing them to spin up slows
>> down the resume.  Add a second bit to the manage_start_stop flag
>> to restore the previous behavior.
> 
> SCSI disks when in STOP state do not spin up "automatically when
> accessed".

The drive does not, but the scsi error handling notices when a command
fails because the drive needs started, starts it, and retries the command.

> And your choice of bits looks like it will favour fixing broken
> SATA behaviour but as a by-product break working SCSI disk
> behaviour.

No, it has nothing to do with sata behavior; it has to do with whether
or not all disk drives should be restarted immediately after a resume,
or only when they are accessed.  I happen to have a few older drives
that I very rarely access and would rather they not start up every
time I resume.


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

iQEcBAEBAgAGBQJSfANJAAoJEJrBOlT6nu75BIAIALHveooj3EuVQ9DmzsCKezT2
ILHsGKAecgojAbxNFp4JXTwmlnNMUQzGaaxfXgoTES4WBiDnba0sedVW+aPqI8Ul
cA8FXeIPtk1U5/xA6iW9XFInOTkvpXSnl9yeX7zldtcjxBm0bQ49JUk4zI68jQwF
YPxEb0bxPpJvUcoYbM6eo+I6GnBqpCU9FDVoy0wnb1Zp6qQEY59kOvQc51Pl10KR
Vwmayc7B9dncfk9+46knt4ocBUdSdX1rzRqN0EfGIZtkWysP6w5HmA0ejocEIyHT
YErI94JvAEOZkzGHZgaMykxTE5SGYX8Fz3w3LVc9uAc40oIf3qFHb5Bob6Ecy6Q=
=SkP3
-----END PGP SIGNATURE-----

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Todd E Brandt]

I tested your patches and they do function. We tried a similar approach
a few months back where instead of waking the scsi disks we just set 
them all to runtime_suspended and skipped the resume. Then we let
them be awakened later by read/write access just as you have. It's a really 
tempting approach, in theory, since you're saving both time and power 
by only waking those disks you know you need. But in practice I've found 
that userspace doesn't play nice.

In my experience the user layer almost always manages to wake up every
mounted disk after resume, even if you didn't deliberately use them
prior to suspend. The accesses can come from the file manager doing a 
scan after resume, or from any number of apps running on the system that 
decide they need to get even the smallest piece of information from the 
disks. A simple space check will wake them up.

Thus when you leave all the disks stopped, user space ends up triggering 
a traffic jam when the OS wakes back up, which makes disk access take even
longer.

My patch works very similarly to yours but just triggers an asynchronous 
wakeup to all the disks in anticipation of userspace's needs. We've 
tested it pretty heavily on ubuntu machines of all types and it's 
done well.

Ultimately I would be happy if either solution were accepted though, 
because this is a serious performance problem that needs to be addressed
in some way.

On Wed, Nov 06, 2013 at 08:53:11PM -0500, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 10/17/2013 03:33 PM, Todd E Brandt wrote:
> > The essential issue behind hard disks' lengthy resume time is the
> > ata port driver blocking until the ATA port hardware is finished
> > coming online. So the kernel isn't really doing anything during all
> > those seconds that the disks are resuming, it's just blocking until
> > the hardware says it's ready to accept commands. Applying this
> > patch set
> 
> I have been working in a similar direction and have patches to follow.
>  Instead of backgrounding the start in sd_resume, I have removed it
> entirely, and background the resume at the ata port layer, as well as
> avoid resuming Power on in Standby disks entirely.
> 
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.14 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQEcBAEBCgAGBQJSevKEAAoJEJrBOlT6nu75FFkIAL8mwXOuuNvY5GoppHVMcweC
> EOBz6t2Xmc/TsGYr9u4kRVXbf3G0trSM9dj/XSJ6p+Dj/su/vJ69yKxp7sUPidgz
> zqjfKlG+myMQIaAO+tPGXvMcmT74BXjgjobZw2lUyLyRPLD9elDGmrDA5ZXZ+hiz
> hzSx4gOC57j+sK/pmoZ7U/CWeWYEYQ+J9xBXm0hzbBbSTjpzVsR0T+7xU+WVkeX0
> Ox++FFBZp2b1xJgaREurRv1mORx/GJ+PfSETb49P5BWaWW+QVwOEhWOX/JQNc/Pp
> ANJmttP4WPvlRV0X3hD6FvAYSnu1qbwwGOiP/j5g7mklyfm5alA0jcuahc1w5tI=
> =9KhA
> -----END PGP SIGNATURE-----

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Phillip Susi]

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

On 11/08/2013 08:20 PM, Todd E Brandt wrote:
> I tested your patches and they do function. We tried a similar
> approach a few months back where instead of waking the scsi disks
> we just set them all to runtime_suspended and skipped the resume.
> Then we let them be awakened later by read/write access just as you
> have. It's a really tempting approach, in theory, since you're
> saving both time and power by only waking those disks you know you
> need. But in practice I've found that userspace doesn't play nice.

I've just about gotten rid of all instances of user space waking the
disks on my system.  The one I have left to do is to fake the IDENTIFY
DEVICE command using the cached identity info to satisfy a script that
tries to set the APM mode of the disk after resume.  If I disable that
script, my extra disks stay asleep and Xwindows fires up nice and fast.

> In my experience the user layer almost always manages to wake up
> every mounted disk after resume, even if you didn't deliberately
> use them prior to suspend. The accesses can come from the file
> manager doing a scan after resume, or from any number of apps
> running on the system that decide they need to get even the
> smallest piece of information from the disks. A simple space check
> will wake them up.

By simple space check I assume you mean df?  The superblocks easily
fit in the cache so that shouldn't generate any IO.

> Thus when you leave all the disks stopped, user space ends up
> triggering a traffic jam when the OS wakes back up, which makes
> disk access take even longer.
> 
> My patch works very similarly to yours but just triggers an
> asynchronous wakeup to all the disks in anticipation of userspace's
> needs. We've tested it pretty heavily on ubuntu machines of all
> types and it's done well.

I don't think the wakeup is needed since ( ata ) disks normally wake
up on their own unless you enable power up in standby and pro-actively
initiating the wakeup only buys you maybe 500ms or less?  What is that
compared to a typical 10 second startup time?

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

iQEcBAEBCgAGBQJSfqIUAAoJEJrBOlT6nu75v3sH/R374d/Sgx3DB0DVGgDch3jA
ZlR4eb5x5umw2CApGE0jbbj91/330Z5uxgr76tn6/nSRftDJ5ZgLc6dBTF1VwX4q
fqxKgNY1euIARiCL4jLxiK9JfX7hB0GtknJaMRvG4JHaSP4d0Cvhr0sbd5mpmJp7
P0TMVslJJHyIFVk0QjvisDBcFgo1onBkbVnX6B5Z6mPZXhAd+WCA3CJfiHnAK7t+
mINmlTBXnZQFXLXY2rDrmZEUCLFfTqtlprkAuGdlfXsMVYBTD31notuZ74Xbv7C7
vBJLiQ6b7dyF8eqcHoc49qqNO1n38nhRmYhIOSYgsyRFhECjVms5/mfEj9UBkiY=
=iDTY
-----END PGP SIGNATURE-----

[PATCH 0/6] Let sleeping disks lie

[Phillip Susi]

I've made a few corrections and extentions to this patch set after
more testing.  Changes include fixing the faked command to return
the correct status, fixing some errors and adding more commands
to be faked.

The one command I have yet to figure out how to fake is
IDENTIFY DEVICE, which I would like to just have return the
cached identify block, but am not sure how to get from the
qc's scattergather list to a memory pointer I can memcpy to.

Phillip Susi (6):
  libata: use sleep instead of standby command
  libata: avoid waking disk to check power
  sd: don't bother spinning up disks on resume
  libata: resume in the background
  libata: don't start disks on resume
  libata: fake some more commands when drive is sleeping

 drivers/ata/libata-core.c  | 34 ++++++++++++++++++++++++++--------
 drivers/ata/libata-eh.c    | 10 ++++++++--
 drivers/ata/libata-scsi.c  |  4 ++--
 drivers/ata/libata.h       |  1 +
 drivers/scsi/sd.c          |  6 +++---
 include/linux/libata.h     |  1 +
 include/scsi/scsi_device.h |  2 +-
 7 files changed, 42 insertions(+), 16 deletions(-)

-- 
1.8.3.2

[PATCH 1/6] libata: use sleep instead of standby command

[Phillip Susi]

The ATA SLEEP mode saves some more power than SUSPEND, and
has basically the same recovery time, so use it instead.
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 97a0cef..79b75fd 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1362,8 +1362,8 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 		     system_entering_hibernation())
 			goto skip;
 
-		/* Issue ATA STANDBY IMMEDIATE command */
-		tf->command = ATA_CMD_STANDBYNOW1;
+		/* Issue ATA SLEEP command */
+		tf->command = ATA_CMD_SLEEP;
 	}
 
 	/*
-- 
1.8.3.2

[PATCH 2/6] libata: avoid waking disk to check power

[Phillip Susi]

When a disk is in SLEEP mode it can not respond to commands,
including the CHECK POWER command.  Instead of waking up the
sleeping disk, fake the reply to the CHECK POWER command to
indicate the disk is in standby mode.  This prevents udisks
from waking up sleeping disks when it polls to see if they
are awake or not before trying to read their smart status.
---
 drivers/ata/libata-core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 83b1a9f..686c441 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5084,6 +5084,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 
 	/* if device is sleeping, schedule reset and abort the link */
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER))
+		{
+			/* fake reply to avoid waking drive */
+			qc->flags &= ~ATA_QCFLAG_RESULT_TF;
+			qc->result_tf.nsect = 0;
+			ata_qc_complete(qc);
+			return;
+		}
 		link->eh_info.action |= ATA_EH_RESET;
 		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
 		ata_link_abort(link);
-- 
1.8.3.2

[PATCH 3/6] sd: don't bother spinning up disks on resume

[Phillip Susi]

Don't bother forcing disks to spin up on resume, as they
will do so automatically when accessed, and forcing them
to spin up slows down the resume.  Add a second bit to the
manage_start_stop flag to restore the previous behavior.
---
 drivers/scsi/sd.c          | 6 +++---
 include/scsi/scsi_device.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..3143311 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3083,7 +3083,7 @@ static void sd_shutdown(struct device *dev)
 		sd_sync_cache(sdkp);
 	}
 
-	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
+	if (system_state != SYSTEM_RESTART && (sdkp->device->manage_start_stop & 1)) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		sd_start_stop_device(sdkp, 0);
 	}
@@ -3107,7 +3107,7 @@ static int sd_suspend(struct device *dev)
 			goto done;
 	}
 
-	if (sdkp->device->manage_start_stop) {
+	if (sdkp->device->manage_start_stop & 1) {
 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
 		ret = sd_start_stop_device(sdkp, 0);
 	}
@@ -3122,7 +3122,7 @@ static int sd_resume(struct device *dev)
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
 
-	if (!sdkp->device->manage_start_stop)
+	if (!(sdkp->device->manage_start_stop & 2))
 		goto done;
 
 	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d65fbec..1c46d2d 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -152,7 +152,7 @@ struct scsi_device {
 	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
 	unsigned no_start_on_add:1;	/* do not issue start on add */
 	unsigned allow_restart:1; /* issue START_UNIT in error handler */
-	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
+	unsigned manage_start_stop:2;	/* Let HLD (sd) manage start/stop */
 	unsigned start_stop_pwr_cond:1;	/* Set power cond. in START_STOP_UNIT */
 	unsigned no_uld_attach:1; /* disable connecting to upper level drivers */
 	unsigned select_no_atn:1;
-- 
1.8.3.2

[PATCH 4/6] libata: resume in the background

[Phillip Susi]

Don't block the resume path waiting for the disk to
spin up.
---
 drivers/ata/libata-core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 686c441..128ce0d 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5421,20 +5421,18 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 {
 	struct ata_port *ap = to_ata_port(dev);
+	static int dontcare;
 
-	return __ata_port_resume_common(ap, mesg, NULL);
+	return __ata_port_resume_common(ap, mesg, &dontcare);
 }
 
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
 	rc = ata_port_resume_common(dev, PMSG_RESUME);
-	if (!rc) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
 
 	return rc;
 }
-- 
1.8.3.2

[PATCH 5/6] libata: don't start disks on resume

[Phillip Susi]

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Suppress this until the disk
is actually accessed.
---
 drivers/ata/libata-core.c | 12 ++++++++++--
 drivers/ata/libata-eh.c   | 10 ++++++++--
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 128ce0d..c53c210 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1993,7 +1993,13 @@ retry:
 			goto err_out;
 	}
 
-	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
+	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
+	{
+		dev->flags |= ATA_DFLAG_SLEEPING;
+		return -EAGAIN;
+	}
+	if (!(flags & ATA_READID_NOSTART) &&
+	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
 		tried_spinup = 1;
 		/*
 		 * Drive powered-up in standby mode, and requires a specific
@@ -4024,6 +4030,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 
 	/* re-read ID */
 	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc == -EAGAIN)
+		return rc;
 	if (rc)
 		goto fail;
 
@@ -5414,7 +5422,7 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 	int rc;
 
 	rc = ata_port_request_pm(ap, mesg, ATA_EH_RESET,
-		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, async);
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET | ATA_EHI_NOSTART, async);
 	return rc;
 }
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index c69fcce..4c2e6e8 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3086,6 +3086,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
+		if (ehc->i.flags & ATA_EHI_NOSTART)
+			readid_flags |= ATA_READID_NOSTART;
 
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
@@ -3098,6 +3100,10 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
 			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
 						readid_flags);
+			if (rc == -EAGAIN) {
+				rc = 0; /* start required but suppressed, handle later */
+				continue;
+			}
 			if (rc)
 				goto err;
 
@@ -3424,7 +3430,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy != ATA_LPM_MIN_POWER && dipm) {
+		if (policy != ATA_LPM_MIN_POWER && dipm && !(dev->flags & ATA_DFLAG_SLEEPING)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
@@ -3468,7 +3474,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	/* host config updated, enable DIPM if transitioning to MIN_POWER */
 	ata_for_each_dev(dev, link, ENABLED) {
 		if (policy == ATA_LPM_MIN_POWER && !no_dipm &&
-		    ata_id_has_dipm(dev->id)) {
+		    ata_id_has_dipm(dev->id) && !(dev->flags & ATA_DFLAG_SLEEPING)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_ENABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index eeeb778..146d62d 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -42,6 +42,7 @@ struct ata_scsi_args {
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
 
 	/* selector for ata_down_xfermask_limit() */
 	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0e23c26..3929bb8 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -362,6 +362,7 @@ enum {
 
 	/* ata_eh_info->flags */
 	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */
-- 
1.8.3.2

[PATCH 6/6] libata: fake some more commands when drive is sleeping

[Phillip Susi]

The sd driver issues a flush cache when suspending, and this was causing
a sleeping drive to wake up for no reason.  Another sleep or standby
command when the drive is already sleeping obviously should also not cause
the disk to spin up.
---
 drivers/ata/libata-core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index c53c210..25d141e 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5092,7 +5092,11 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 
 	/* if device is sleeping, schedule reset and abort the link */
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
-		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER))
+		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER ||
+			     qc->tf.command == ATA_CMD_SLEEP ||
+			     qc->tf.command == ATA_CMD_FLUSH ||
+			     qc->tf.command == ATA_CMD_FLUSH_EXT ||
+			     qc->tf.command == ATA_CMD_STANDBYNOW1))
 		{
 			/* fake reply to avoid waking drive */
 			qc->flags &= ~ATA_QCFLAG_RESULT_TF;
-- 
1.8.3.2

Re: [PATCH 2/6] libata: avoid waking disk to check power

[Sergei Shtylyov]

Hello.

On 10-11-2013 1:03, Phillip Susi wrote:

> When a disk is in SLEEP mode it can not respond to commands,
> including the CHECK POWER command.  Instead of waking up the
> sleeping disk, fake the reply to the CHECK POWER command to
> indicate the disk is in standby mode.  This prevents udisks
> from waking up sleeping disks when it polls to see if they
> are awake or not before trying to read their smart status.
> ---
>   drivers/ata/libata-core.c | 8 ++++++++
>   1 file changed, 8 insertions(+)

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 83b1a9f..686c441 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5084,6 +5084,14 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
>
>   	/* if device is sleeping, schedule reset and abort the link */
>   	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
> +		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER))
> +		{

    Keep { on the same line as *if* please.

WBR, Sergei

Re: [PATCH 3/6] sd: don't bother spinning up disks on resume

[Sergei Shtylyov]

Hello.

On 10-11-2013 1:03, Phillip Susi wrote:

> Don't bother forcing disks to spin up on resume, as they
> will do so automatically when accessed, and forcing them
> to spin up slows down the resume.  Add a second bit to the
> manage_start_stop flag to restore the previous behavior.
> ---
>   drivers/scsi/sd.c          | 6 +++---
>   include/scsi/scsi_device.h | 2 +-
>   2 files changed, 4 insertions(+), 4 deletions(-)

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..3143311 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
[...]
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index d65fbec..1c46d2d 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -152,7 +152,7 @@ struct scsi_device {
>   	unsigned use_192_bytes_for_3f:1; /* ask for 192 bytes from page 0x3f */
>   	unsigned no_start_on_add:1;	/* do not issue start on add */
>   	unsigned allow_restart:1; /* issue START_UNIT in error handler */
> -	unsigned manage_start_stop:1;	/* Let HLD (sd) manage start/stop */
> +	unsigned manage_start_stop:2;	/* Let HLD (sd) manage start/stop */

    I think you should better document this 2-bit field, or better still, make 
it 2 1-bit fields.

WBR, Sergei

Re: [PATCH 4/6] libata: resume in the background

[Sergei Shtylyov]

On 10-11-2013 1:03, Phillip Susi wrote:

> Don't block the resume path waiting for the disk to
> spin up.
> ---
>   drivers/ata/libata-core.c | 10 ++++------
>   1 file changed, 4 insertions(+), 6 deletions(-)

> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 686c441..128ce0d 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5421,20 +5421,18 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
>   static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
>   {
>   	struct ata_port *ap = to_ata_port(dev);
> +	static int dontcare;
>
> -	return __ata_port_resume_common(ap, mesg, NULL);
> +	return __ata_port_resume_common(ap, mesg, &dontcare);
>   }
>
>   static int ata_port_resume(struct device *dev)
>   {
>   	int rc;
>
> +	if (pm_runtime_suspended(dev))
> +		return 0;
>   	rc = ata_port_resume_common(dev, PMSG_RESUME);
> -	if (!rc) {
> -		pm_runtime_disable(dev);
> -		pm_runtime_set_active(dev);
> -		pm_runtime_enable(dev);
> -	}

     With this modification, you don't need 'rc' anymore.

>   	return rc;
>   }
>

MBR, Sergei

Re: [PATCH 3/6] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/11/2013 8:08 AM, Sergei Shtylyov wrote:
>> -    unsigned manage_start_stop:1;    /* Let HLD (sd) manage 
>> start/stop */ +    unsigned manage_start_stop:2;    /* Let HLD
>> (sd) manage start/stop */
> 
> I think you should better document this 2-bit field, or better
> still, make it 2 1-bit fields.

Not a bad idea, though I'm really not sure that it shouldn't just be
removed entirely.  It is a bad idea to not stop the disk on
suspend/shutdown since it leaves the disk to take an emergency head
retract, which isn't good for it.  At the very least it should park
the heads, though currently libata does not handle the scsi cmd for that.

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

iQEcBAEBAgAGBQJSgOmiAAoJEJrBOlT6nu755CcH/2ckSBdvCtMHFe1ech1XFOdJ
PUZc5VEHk/rzPqXqxS26H9eR5FIbgu437yVizJ/w5Fy4MAX0rVKyz61FK/HavGL7
aqNYgKWIjucg7panlWZsPIraDl/bVPYVS2PnthVItabC+GskYRR0g92xcDqDPSeX
kmFIG0JOx2bU6JYWtFWxECpdjsBJBQxAnoLtzI+SONmlhp2GxH9Bc9Msa5hqwBoW
vmUJmggBaPoL3ZTQUFGZyYWU8/7n2d5lhXpJdcsvcrd+PJyrsfv9GGKqgQW/kLmL
sYAFwO83/5YJUK03l68S9xHt48NW2rdFm2ph035/CpbsNib2fKTaOsyfIgf/Idg=
=y73A
-----END PGP SIGNATURE-----

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Todd E Brandt]

500ms is actually quite alot when you compare it with android or iOS,
or even with other subsystems like USB. USB is the second worst offender 
after SATA disks and it tends to take around 600ms for most standard 
systems with an onboard webcam, and with other devices plugged in ever 
longer. I think we should optimize out every millisecond we can.

You mentioned that you tuned your system to remove all cases that trigger
non-essential disk wakeups, and that works great for you, but it won't 
help anyone using fedora or OpenSUSE or Yocto, etc. And even if you
could formallize your changes to Ubuntu to create a fully optimized 
user space, you're still at the mercy of the user's install choices, which
can quickly throw a wrench in the works.

On Sat, Nov 09, 2013 at 03:59:04PM -0500, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 
> On 11/08/2013 08:20 PM, Todd E Brandt wrote:
> > I tested your patches and they do function. We tried a similar
> > approach a few months back where instead of waking the scsi disks
> > we just set them all to runtime_suspended and skipped the resume.
> > Then we let them be awakened later by read/write access just as you
> > have. It's a really tempting approach, in theory, since you're
> > saving both time and power by only waking those disks you know you
> > need. But in practice I've found that userspace doesn't play nice.
> 
> I've just about gotten rid of all instances of user space waking the
> disks on my system.  The one I have left to do is to fake the IDENTIFY
> DEVICE command using the cached identity info to satisfy a script that
> tries to set the APM mode of the disk after resume.  If I disable that
> script, my extra disks stay asleep and Xwindows fires up nice and fast.
> 
> > In my experience the user layer almost always manages to wake up
> > every mounted disk after resume, even if you didn't deliberately
> > use them prior to suspend. The accesses can come from the file
> > manager doing a scan after resume, or from any number of apps
> > running on the system that decide they need to get even the
> > smallest piece of information from the disks. A simple space check
> > will wake them up.
> 
> By simple space check I assume you mean df?  The superblocks easily
> fit in the cache so that shouldn't generate any IO.
> 
> > Thus when you leave all the disks stopped, user space ends up
> > triggering a traffic jam when the OS wakes back up, which makes
> > disk access take even longer.
> > 
> > My patch works very similarly to yours but just triggers an
> > asynchronous wakeup to all the disks in anticipation of userspace's
> > needs. We've tested it pretty heavily on ubuntu machines of all
> > types and it's done well.
> 
> I don't think the wakeup is needed since ( ata ) disks normally wake
> up on their own unless you enable power up in standby and pro-actively
> initiating the wakeup only buys you maybe 500ms or less?  What is that
> compared to a typical 10 second startup time?
> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.14 (GNU/Linux)
> Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
> 
> iQEcBAEBCgAGBQJSfqIUAAoJEJrBOlT6nu75v3sH/R374d/Sgx3DB0DVGgDch3jA
> ZlR4eb5x5umw2CApGE0jbbj91/330Z5uxgr76tn6/nSRftDJ5ZgLc6dBTF1VwX4q
> fqxKgNY1euIARiCL4jLxiK9JfX7hB0GtknJaMRvG4JHaSP4d0Cvhr0sbd5mpmJp7
> P0TMVslJJHyIFVk0QjvisDBcFgo1onBkbVnX6B5Z6mPZXhAd+WCA3CJfiHnAK7t+
> mINmlTBXnZQFXLXY2rDrmZEUCLFfTqtlprkAuGdlfXsMVYBTD31notuZ74Xbv7C7
> vBJLiQ6b7dyF8eqcHoc49qqNO1n38nhRmYhIOSYgsyRFhECjVms5/mfEj9UBkiY=
> =iDTY
> -----END PGP SIGNATURE-----

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Phillip Susi]

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

On 11/11/2013 11:59 AM, Todd E Brandt wrote:
> 500ms is actually quite alot when you compare it with android or
> iOS, or even with other subsystems like USB. USB is the second
> worst offender after SATA disks and it tends to take around 600ms
> for most standard systems with an onboard webcam, and with other
> devices plugged in ever longer. I think we should optimize out
> every millisecond we can.

I think you missed my point: if the disk is going to be started in
9000ms vs 8500ms, it doesn't make much difference, and I think in
practice the difference will be less than that.  I don't think it is
worth requiring that all disks start up to save a few percent in the
cases where the disk is required ( and I'm not clear that there even
are any such cases ).

> You mentioned that you tuned your system to remove all cases that
> trigger non-essential disk wakeups, and that works great for you,
> but it won't help anyone using fedora or OpenSUSE or Yocto, etc.
> And even if you could formallize your changes to Ubuntu to create a
> fully optimized user space, you're still at the mercy of the user's
> install choices, which can quickly throw a wrench in the works.

No, I did not tune my system; I fixed the kernel so that userspace's
activities do not start those disks.

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

iQEcBAEBAgAGBQJSgQ8hAAoJEJrBOlT6nu75c04H/2ej9NzdyrWkuPv2SLGgb51Y
iBmeAkJEoFXwgu0UCZ2Ty3dpUkW7jcRiKKv5q8d1Anj4eGFIpx+CjUptCE3MCs1w
PWQm7uB6+DgOhj3aqF9DulBx0Yp3+gISwOTXYLAIirIReE4Vk904ZuYHo4IvJmug
vbk4p6n5iGLMqV/QFsymPoOFtUzKQPHj4YDL3Piu0fse8AOIHk5BJtmmtj1YbAck
oAsMIYz6UQZ2tXpzxcy1ISgZYUO8k6CcrXg8WE2ws9gZLNJtBHniLC1t2VCE10KF
BSzP+tgN+Glbk+HyJUuwjALa/CkFk9FKQJT8LJQfn0rRhjku/vZXsZoFgCQiIx0=
=THLl
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Mark Lord]

On 13-11-07 01:21 PM, Douglas Gilbert wrote:
> On 13-11-06 08:57 PM, Phillip Susi wrote:
>> Don't bother forcing disks to spin up on resume, as they
>> will do so automatically when accessed, and forcing them
>> to spin up slows down the resume.  Add a second bit to the
>> manage_start_stop flag to restore the previous behavior.
> 
> SCSI disks when in STOP state do not spin up "automatically
> when accessed".
..

Ditto for many SATA disks with "power up in standby" enabled.

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/16/2013 12:23 AM, Mark Lord wrote:
> Ditto for many SATA disks with "power up in standby" enabled.

Ditto for my previous response; the kernel ( in this case the libata
layer ) notices this and takes care of starting it.  The third patch I
posted suppresses this startup at resume time so that it will be done
on access instead.

I currently have two disks with PuiS enabled that no longer have to
spin up and down again every time I resume from suspend, but wake up
nicely when I mount them.


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

iQEcBAEBCgAGBQJSh4aPAAoJEJrBOlT6nu75bV0IAImEyIJlIF6O69mqLS3A6z/r
xtUQlxg5gfE91PteNdtNYd9v54uwCw2uldSAGOEm4DR7LOErVu54J+6rVZKFRF0L
ocmE2VEKhzNTA21pt5rCPDsQ9BHJ7ljWXbGTUC7zFN4VCYTEkPVuO5Owedjd9sb8
+BUHCD9+au7CRf5SLU4qSoqT8DNUPREv70WL1Ze8LhQt9R+APBclyeXHBYc+Hgpd
Idzp/EFt+SPRGrJL+XyC6/F0Mz3kLRD/2Ij0kRSux1E5DNG1SQTq+FdqUIa2sTDf
volJRiUS3/rtNr/2c4F9awQNbsCZRceTIq71PA3dvgxM21sjpXpLUe2A65u9gNQ=
=jhXC
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[James Bottomley]

On Thu, 2013-11-07 at 16:16 -0500, Phillip Susi wrote:
> On 11/7/2013 1:21 PM, Douglas Gilbert wrote:
> > On 13-11-06 08:57 PM, Phillip Susi wrote:
> >> Don't bother forcing disks to spin up on resume, as they will do
> >> so automatically when accessed, and forcing them to spin up slows
> >> down the resume.  Add a second bit to the manage_start_stop flag
> >> to restore the previous behavior.
> > 
> > SCSI disks when in STOP state do not spin up "automatically when
> > accessed".
> 
> The drive does not, but the scsi error handling notices when a command
> fails because the drive needs started, starts it, and retries the command.

No disk does, neither SCSI nor ATA.  The error handler is not
automatically activated for a not ready/initializing command required
because of multi-path.  We override the default behaviour via
allow_restart only for IBM vfc/vscsi and ATA disks.  With this patch
SCSI devices would no longer ever restart after suspend.

> > And your choice of bits looks like it will favour fixing broken
> > SATA behaviour but as a by-product break working SCSI disk
> > behaviour.
> 
> No, it has nothing to do with sata behavior; it has to do with whether
> or not all disk drives should be restarted immediately after a resume,
> or only when they are accessed.  I happen to have a few older drives
> that I very rarely access and would rather they not start up every
> time I resume.

As Doug said, if you don't restart a SCSI device after resume, it will
never get started.

James

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/16/2013 01:20 PM, James Bottomley wrote:
> No disk does, neither SCSI nor ATA.  The error handler is not 
> automatically activated for a not ready/initializing command
> required because of multi-path.  We override the default behaviour
> via allow_restart only for IBM vfc/vscsi and ATA disks.  With this
> patch SCSI devices would no longer ever restart after suspend.

I don't follow.  A scsi disk returns a sense status saying it requires
a START UNIT command when issued a command before being started.  This
triggers the eh which notices that sense status and issues the
command.  My first version seemed to cause hdparm to fail on an ata
drive that required SET FEATURES command to start up after power on,
so the next version I set the DFLAG_SLEEPING bit which causes a
preemptive start command on access even for SGIO, which otherwise
seems to suppress eh.

> As Doug said, if you don't restart a SCSI device after resume, it
> will never get started.

If something ( and I am still not clear on what ) is suppressing the
error handling code for scsi disks, it should be able to be handled in
a simiar way to ata: set a flag that causes preemptive startup even if
eh is suppressed.


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

iQEcBAEBCgAGBQJSiDzoAAoJEJrBOlT6nu75kXIIAJ+7wO8jcnw4LzlBoB8ELY8m
pZxsgvM0c4fQlqNvsCgXbhRYaNK5aHTk1RtguTuV4iJ8rQj+73jmuftar4kq5As2
yZZ2LCYKNdHgBhCe0t2o+vVFKdh6Vaqawm0Gsaqy1D6WNUWXh8D0wicD9gLwJBxo
R1nH2UNw337e6fnhTvF6uzA3OKUC9mJyJIrfrM7djCg46I/QkL0G2gDif8EQbTvo
/XpHywFheFUz5LEz6Ctbxb7VvacE62Nj9kmnDNc2rE+bpP/GeQ72sxmKCtVcm515
HqMRaU8efo8EIRIBNUqYRxZuIDr9vA9rsHq5XxtDAeWPXfjibcEaA/iwBZp6oyY=
=vVEg
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[James Bottomley]

On Sat, 2013-11-16 at 22:50 -0500, Phillip Susi wrote:
> On 11/16/2013 01:20 PM, James Bottomley wrote:
> > No disk does, neither SCSI nor ATA.  The error handler is not 
> > automatically activated for a not ready/initializing command
> > required because of multi-path.  We override the default behaviour
> > via allow_restart only for IBM vfc/vscsi and ATA disks.  With this
> > patch SCSI devices would no longer ever restart after suspend.
> 
> I don't follow.  A scsi disk returns a sense status saying it requires
> a START UNIT command when issued a command before being started.  This
> triggers the eh which notices that sense status and issues the
> command.

OK, so three people have now told you that's not how the code works.
Why don't you just read it?  because there's not really much point us
reading your patches until you do.

James

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/17/2013 01:43 AM, James Bottomley wrote:
> OK, so three people have now told you that's not how the code
> works. Why don't you just read it?  because there's not really much
> point us reading your patches until you do.

I have, which is why I said it handles it via scsi_eh_stu.  If I'm not
understanding it correctly, then by all means, explain.


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

iQEcBAEBCgAGBQJSiOumAAoJEJrBOlT6nu75j6QH/3QURQwEXSjeQRaHdEo3dPUp
eKSf7ix/6kOlWDzqzyNB0BWb0NDmCVc7v0GfKmcO0/7U2VoC4N5WqhvcXHU6X/PO
ZjhcvKvuZEK9z8H9+Dx2CsB2nZ1p3hnQq6GXk1oa4tNx4id+sXioG8qX8Uw5AurA
6O+ceHnb3hggvC8Oojo2YCwEMJiOYw+xhEjnleMcDAj+T5X3T4SMR/N6rM9NgX1L
4ekrQMi3dNrizZaFKXjkf8/xult/nsnk1jKnhvJ/Sdl3EOSMTMypjdVo3W4VHpZ1
0aDE8qIiLaU8x/xNjT4oQrVfseHexjzHVlQ7O5fc79+Tz+ej8PbjDLTtltPmn5M=
=fnx7
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Douglas Gilbert]

On 13-11-17 11:15 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 11/17/2013 01:43 AM, James Bottomley wrote:
>> OK, so three people have now told you that's not how the code
>> works. Why don't you just read it?  because there's not really much
>> point us reading your patches until you do.
>
> I have, which is why I said it handles it via scsi_eh_stu.  If I'm not
> understanding it correctly, then by all means, explain.

Even if the SCSI EH code does spin-up the disk,
it is inefficient and undesirable to send a device
into EH for what is a normal, predictable situation
(i.e. that a SCSI disk will need a START STOP UNIT
(start) command as part of a resume operation).

A big server machine could have thousands of SCSI
disks (many potentially virtual). Sending them all
into EH during a resume would be a really good
test for EH, but horrible for performance. If I was
designing the SCSI EH I would probably assume not
all disks would go into EH at roughly the same time.
Also if that many devices went into EH on the same
controller (HBA) it might be reasonable to assume
that the controller they share needs resetting.

Doug Gilbert

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[James Bottomley]

On Sun, 2013-11-17 at 11:15 -0500, Phillip Susi wrote:
> On 11/17/2013 01:43 AM, James Bottomley wrote:
> > OK, so three people have now told you that's not how the code
> > works. Why don't you just read it?  because there's not really much
> > point us reading your patches until you do.
> 
> I have, which is why I said it handles it via scsi_eh_stu.  If I'm not
> understanding it correctly, then by all means, explain.

Because you don't damn well listen.  Two emails ago I said:

        The error handler is not automatically activated for a not
        ready/initializing command required because of multi-path

I don't really understand how I can be clearer.  You keep wittering
about the error handler spin up but if the error handler doesn't
activate, the disk is never spun up with your patch.  This is the
problem we keep telling you about.  You can't make the error handler
always activate because then multi-path would fail.

James

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/17/2013 06:54 PM, Douglas Gilbert wrote:
> Even if the SCSI EH code does spin-up the disk, it is inefficient
> and undesirable to send a device into EH for what is a normal,
> predictable situation (i.e. that a SCSI disk will need a START STOP
> UNIT (start) command as part of a resume operation).

The device doesn't need the START STOP UNIT command as part of the
resume operation; it only needs it prior to other commands.  What is
inefficient is starting up drives that won't be accessed.  I don't see
anything particularly inefficient about issuing the command in the eh
path ( in fact, libata always uses the eh path to handle power
management ).  You can of course, use the manage_start_stop flag to
have the disk start on resume if you really want.

> A big server machine could have thousands of SCSI disks (many
> potentially virtual). Sending them all into EH during a resume
> would be a really good test for EH, but horrible for performance.
> If I was designing the SCSI EH I would probably assume not all
> disks would go into EH at roughly the same time. Also if that many
> devices went into EH on the same controller (HBA) it might be
> reasonable to assume that the controller they share needs
> resetting.

They don't all go into EH during resume; they go into EH when they are
finally accessed, which for many of them is probably not for some
time.  This also sounds like a bit of speculation and/or hand waving.
 Is there really a problem with multiple devices going into EH?


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

iQEcBAEBCgAGBQJSiWgGAAoJEJrBOlT6nu75klwIAK29G/p+pZhccbfXBnIpvq8m
oIuCOCgMjIbSkJDEo52BiEf6d52tK8aQNTXcHZCC7T2/yvWV55vGHeXbxZ9iospp
IsoJOMLtcM9p5DHZido5RubXrq63cU3yGovGbsp834Ij+d51HB17Dh4Dc1iU/mQV
4X6623hH0ooHWjmtoK6l5rGfKxkPe2ZKP3RAOpW5hiFaGPFWpc375Kukf6Zvw161
mBRfdWtc1SRGme/dcEEggH5t20R0kJteAey4RLF77W8VcooVbE3zCESpVNwqHxYK
6Mc67avVzMyk+Cmb2IN9iMB05oPDIzhKU4kStyYKVGqiS1D2NHUUi56GZTN5Ry0=
=kmdB
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/17/2013 07:09 PM, James Bottomley wrote:
> Because you don't damn well listen.  Two emails ago I said:
> 
> The error handler is not automatically activated for a not 
> ready/initializing command required because of multi-path

And I replied that that *if* eh is suppressed ( and I'm still not
clear on why multipath would do that ), then a flag could be used to
pro actively start the drive despite the suppression, like I did with
libata.

> I don't really understand how I can be clearer.  You keep
> wittering about the error handler spin up but if the error handler
> doesn't activate, the disk is never spun up with your patch.  This
> is the problem we keep telling you about.  You can't make the error
> handler always activate because then multi-path would fail.

Why does multi-path -> no eh?

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

iQEcBAEBCgAGBQJSiWlcAAoJEJrBOlT6nu755h4H/AwoDzPc/iNAw/v3h+HGszCx
3AZUJUbabF7XB8NxbfyLG/vvaGEqxatv5fX3y0I6CmWpWBGQYyDL7W6uQIgKjqAt
OmUCEEv3PqnytTg7VjyBBif+TMxM35gZvqzFYWU5DOMB9D+QtvfIHmyH4rCMujYl
tLJqArxLMBLnLmpSTp0/TTrvjxDThjkCF6vlM0iSe7C8xZsByRcpoIQJlsvTW7YT
IWVop4H9iAW4CZhiSQwdf+IiK8Av45RW8i20em+LlaVvbofoBlUOgUrxIIXu7trT
cslbwtPQ2N3SsTEd5E43W3bzDO19gzQHJf5wHTiMg3++yPWqP0hMUgU0ci0Y4aI=
=DleI
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/17/2013 8:06 PM, Phillip Susi wrote:
> I don't see anything particularly inefficient about issuing the
> command in the eh path ( in fact, libata always uses the eh path to
> handle power management ).  You can of course, use the
> manage_start_stop flag to have the disk start on resume if you
> really want.

Yikes, I think I see now why scsi eh is so bad: the entire host, not
just the device is quiesced.

I suppose now the question is how to issue the START STOP UNIT from
sd_prep_fn()?  I gather you can't just call scsi_execute_req() from
there?  Is there maybe a way to insert a START STOP UNIT request into
the queue ahead of the current request being prepped?  Can
sd_prep_fn() be called concurrently on multiple cpus?  Hrm.. what
about TCQ?  Would it be bad if the original request were queued ( to
the drive ) before the START STOP UNIT command completed?

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

iQEcBAEBAgAGBQJSijguAAoJEJrBOlT6nu75AlIH/3n1XFrWWjU2D209hwaBlRCu
OHdAI40OUW0dqRrF7hyhH62X6SmDFG9pkZcCeZUNmSOEBtvkVjxJRv2VvpXpQL50
xXEiLCLukOYWG7krnVhD1BCrt9SfAjMsCpWmbC14m3epCWd9OQNFfU9vqU7mW18b
2NJPBukq+l8nzZtCl+8UMIQxH4RSkceJs3X7y4PhM65nN4kDMmJUABjJhHBZX30/
MQqu6pu2AW6Wf6SH0Sh1qIWfu45cIneuSDRP9qK5U/9902vhmGCLlt4zgbow8CQh
muspp8A+bhcVQAJVdZvHz9gQMHS0dDRjLgBE23Yik0t+zBl0wvoFsaAG4BEi0lY=
=6HCd
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Mark Lord]

On 13-11-18 10:54 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 11/17/2013 8:06 PM, Phillip Susi wrote:
>> I don't see anything particularly inefficient about issuing the
>> command in the eh path ( in fact, libata always uses the eh path to
>> handle power management ).  You can of course, use the
>> manage_start_stop flag to have the disk start on resume if you
>> really want.
> 
> Yikes, I think I see now why scsi eh is so bad: the entire host, not
> just the device is quiesced.
> 
> I suppose now the question is how to issue the START STOP UNIT from
> sd_prep_fn()?  I gather you can't just call scsi_execute_req() from
> there?  Is there maybe a way to insert a START STOP UNIT request into
> the queue ahead of the current request being prepped?  Can
> sd_prep_fn() be called concurrently on multiple cpus?  Hrm.. what
> about TCQ?  Would it be bad if the original request were queued ( to
> the drive ) before the START STOP UNIT command completed?

Yeah, solve that, and you're golden.
I totally understand the motivation for this patch,
and would love to have it working on my machines here too.

But it may have to be some kind of sysfs optional setting,
defaulting to current behaviour, to keep the server keepers happy.

Cheers

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/20/2013 9:23 AM, Mark Lord wrote:
> Yeah, solve that, and you're golden. I totally understand the
> motivation for this patch, and would love to have it working on my
> machines here too.
> 
> But it may have to be some kind of sysfs optional setting, 
> defaulting to current behaviour, to keep the server keepers happy.

My original idea was actually to just have the system resume force the
runtime pm status to suspended, then it will take care of calling the
runtime resume which can easily start the drive, but I was hoping to
get it working whether or not runtime pm is configured.  I may just
have to fall back to that.


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

iQEcBAEBAgAGBQJSjMvDAAoJEJrBOlT6nu75J5QH/2APABstv5HejNp5R4uvmAuL
xTZyHHOFvcMBDpCFLtlWK6PhnKmN7C/eFlzc7xaadtd0wmgjOqZVLTgfYSWBON6X
/kRz/QtDMhsppah7t1T/QzPPQ/z2j4j2/ffwDRRN4bCdM39B/QQCUeVCxVK0mEx3
w5A2Xiro1gAknh1xb/0SfSb9M35H5bLBt4Pz5Ql/75to9jEeH0MlTthZmm+CJPC/
RcBgCkOG6kEOeqCj4tehnTOrs4a8ghdeGNnR6W6UDaeRh3QjRwOvAmwIbZsxkXgs
XSP52adzt5fnZi8uQAzV0bIGN+8Zttx0PHdl3AsVt56CMfdfAHVbL1RVo+Pl85E=
=fnz3
-----END PGP SIGNATURE-----

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Phillip Susi]

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

On 10/17/2013 3:33 PM, Todd E Brandt wrote:
> sd_resume patch (2/2):
> 
> On resume, the SD driver currently waits until the block driver 
> finishes executing a disk start command with blk_execute_rq. This 
> patch changes the sd_resume callback to use blk_execute_rq_nowait 
> instead, which allows it to return immediately, thus allowing the

Can another request in the queue race with the start/stop command and
possibly beat it to dispatch?

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

iQEcBAEBAgAGBQJSlhtuAAoJEI5FoCIzSKrwgJ0H/ioXLuLHh4aJ3ZNs9LZ3gloA
hQb9bTbUOFpC2HM0GpWntNs9V1KXKha7zqiW7alqmmiaUor7mN8CoVILIrwgEe10
9Fd8z0DNBjCM+BdhdSO1g2opjMYif9ID74E/nA4tP3cXV2TiPjax/lF861iirEAl
xaK5WdI7Ua5qbpIc1Xr6t8EbH1DSwEbiFOkN5HGs2ldFWB+f3jTRMZiGwh2RhUzV
OZt2UKIsCAhwO6opnUPJ3Vv4IHQR248uijGxcuy5KDERVbKtfz8If6CG43FVdSr/
c59AEXgUFtvPm/J9DRO/rVO7Dp2e+DTwJ67Mx9TDqtcKwuRxzsdaxn1yJEXoZfk=
=IOxx
-----END PGP SIGNATURE-----

Re: [PATCH 1/3] sd: don't bother spinning up disks on resume

[Phillip Susi]

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

On 11/20/2013 09:48 AM, Phillip Susi wrote:
> My original idea was actually to just have the system resume force
> the runtime pm status to suspended, then it will take care of
> calling the runtime resume which can easily start the drive, but I
> was hoping to get it working whether or not runtime pm is
> configured.  I may just have to fall back to that.

So the problem with this is that some drives spin up without being
issued the START UNIT command.  ATA disks without PuiS enabled do, and
the SCSI standards indicate that scsi disks can be configured to do so
as well, though the method for configuring that is not defined in the
standard ( do such things actually exist? ).  This would leave the
drive spun up but the kernel thinking it is runtime suspended, which
isn't good either.

I think the way to deal with this is to throw a work queue item in the
system resume method that issues TEST UNIT READY and if the drive
indicates that it is currently active, then force the runtime pm
status back to active.  According to SAT-3, an ata emulation layer
should issue CHECK POWER MODE and return POWER CONDITION CHANGE TO
STANDBY, but libata does not do this; instead it always returns 0/0/0
for the sense status, so this would need fixed as well.

Does this sound like the right idea?  Also, I don't see any of the
ASQ/ASCQ codes defined in the headers.  Am I missing them or are they
just not there?

Also, I'm a bit unclear on whether you actually have to issue TEST
UNIT READY or can just issue REQUEST SENSE to check the power
condition.  The standards seem to indicate the latter, but I'm not
sure if REQUEST SENSE would return the sense status of the previous
command rather than the power status, so you need the TEST UNIT READY
first.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBCgAGBQJSlp7ZAAoJEI5FoCIzSKrw5PIH/0wGsyAEr9xR6BkN/C7SOSNh
ArhET3A+Af4Zo1HTe9vKDOIsf9OTrjXxWuLYDBoZSCC0GOw3XPYPdnZ1V4xIBtey
0lMSa8VU9KZMZs8KTYNp6LWToBqChBHSFEd9FLwdWnqLKCp/CtOqgeHO1wOyt1LJ
NkYM4Mpsx6dGO7dzCTZ6Q5ogoXtGcokbGvUXbvuR1idNy1dEdBHaGStaVmIXxImd
u2uJI+1ygk9lMvIa66zJK4f5X6OATd3+SkJlJTsAiEayOEqSAxXNh3QmYJzmJdy/
PDVdgjkgavpKDvbJjNBDSF9skbF9z40EK2NQ9R2kvUMHA8ui66/AQfQHAr04aZI=
=kjuO
-----END PGP SIGNATURE-----

[PATCH 0/6] Let sleeping disks lie

[Phillip Susi]

This is the set I currently have.  I fixed libata to implement the
REQUEST SENSE scsi command, and set up sd to use blk_pre_runtime_suspend
to force the queue to the RPM_SUSPENDING state while issuing the
REQUEST SENSE command to make sure other requests are blocked, then
if REQUEST SENSE indicates that the drive does not need started,
then complete suspending the device, otherwise cancel the suspend
by passing an error value to blk_post_runtime_suspend.

For some reason, the system hangs on resume if I issue the REQUEST SENSE
command after blk_pre_runtime_suspend.  My understanding is that the
REQ_PM flag should make the command process even though the queue is
RPM_SUSPENDING, but it doesn't seem to work.  Anyone have any idea why?

Phillip Susi (6):
  libata: use sleep instead of standby command
  libata: avoid waking disk for several commands
  libata: resume in the background
  libata: don't start disks on resume
  sd: don't start disks on system resume
  libata: return power status in REQUEST SENSE command

 drivers/ata/libata-core.c | 38 +++++++++++++++----
 drivers/ata/libata-eh.c   | 10 ++++-
 drivers/ata/libata-scsi.c | 44 ++++++++++++++++++----
 drivers/ata/libata.h      |  1 +
 drivers/scsi/sd.c         | 94 ++++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/sd.h         |  1 +
 include/linux/libata.h    |  1 +
 7 files changed, 162 insertions(+), 27 deletions(-)

-- 
1.8.3.2

[PATCH 1/6] libata: use sleep instead of standby command

[Phillip Susi]

The ATA SLEEP mode saves some more power than SUSPEND, and
has basically the same recovery time, so use it instead.
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index db6dfcf..f92eb21 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1362,8 +1362,8 @@ static unsigned int ata_scsi_start_stop_xlat(struct ata_queued_cmd *qc)
 		     system_entering_hibernation())
 			goto skip;
 
-		/* Issue ATA STANDBY IMMEDIATE command */
-		tf->command = ATA_CMD_STANDBYNOW1;
+		/* Issue ATA SLEEP command */
+		tf->command = ATA_CMD_SLEEP;
 	}
 
 	/*
-- 
1.8.3.2

[PATCH 2/6] libata: avoid waking disk for several commands

[Phillip Susi]

When a disk is in SLEEP mode it can not respond to any
commands.  Instead of waking up the sleeping disk, fake the
commands.  The commands include:

CHECK POWER
FLUSH CACHE
SLEEP
STANDBY IMMEDIATE
IDENTIFY

If we konw the disk is sleeping, we don't need to wake it up
to to find out if it is in standby, so just pretend it is in
standby.  While alseep, there's no dirty pages in the cache,
so there's no need to flush it.  There's no point in waking
a disk from sleep just to put it back to sleep.  We also have
a cache of the IDENTIFY information so just return that
instead of waking the disk.
---
 drivers/ata/libata-core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 81a94a3..8f856bb 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5085,6 +5085,22 @@ void ata_qc_issue(struct ata_queued_cmd *qc)
 
 	/* if device is sleeping, schedule reset and abort the link */
 	if (unlikely(qc->dev->flags & ATA_DFLAG_SLEEPING)) {
+		if (unlikely(qc->tf.command == ATA_CMD_CHK_POWER ||
+			     qc->tf.command == ATA_CMD_SLEEP ||
+			     qc->tf.command == ATA_CMD_FLUSH ||
+			     qc->tf.command == ATA_CMD_FLUSH_EXT ||
+			     qc->tf.command == ATA_CMD_STANDBYNOW1 ||
+			     (qc->tf.command == ATA_CMD_ID_ATA &&
+			      !ata_tag_internal(qc->tag))))
+		{
+			/* fake reply to avoid waking drive */
+			qc->flags &= ~ATA_QCFLAG_RESULT_TF;
+			qc->result_tf.nsect = 0;
+			if (qc->tf.command == ATA_CMD_ID_ATA)
+				sg_copy_from_buffer(qc->sg, 1, qc->dev->id, 2 * ATA_ID_WORDS);
+			ata_qc_complete(qc);
+			return;
+		}
 		link->eh_info.action |= ATA_EH_RESET;
 		ata_ehi_push_desc(&link->eh_info, "waking up from sleep");
 		ata_link_abort(link);
-- 
1.8.3.2

[PATCH 3/6] libata: resume in the background

[Phillip Susi]

Don't block the resume path waiting for the disk to
spin up.
---
 drivers/ata/libata-core.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8f856bb..4a28caf 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5430,20 +5430,18 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 {
 	struct ata_port *ap = to_ata_port(dev);
+	static int dontcare;
 
-	return __ata_port_resume_common(ap, mesg, NULL);
+	return __ata_port_resume_common(ap, mesg, &dontcare);
 }
 
 static int ata_port_resume(struct device *dev)
 {
 	int rc;
 
+	if (pm_runtime_suspended(dev))
+		return 0;
 	rc = ata_port_resume_common(dev, PMSG_RESUME);
-	if (!rc) {
-		pm_runtime_disable(dev);
-		pm_runtime_set_active(dev);
-		pm_runtime_enable(dev);
-	}
 
 	return rc;
 }
-- 
1.8.3.2

[PATCH 4/6] libata: don't start disks on resume

[Phillip Susi]

Disks with Power Up In Standby enabled that required the
SET FEATURES command to start up were being issued the
command during resume.  Suppress this until the disk
is actually accessed.
---
 drivers/ata/libata-core.c | 12 ++++++++++--
 drivers/ata/libata-eh.c   | 10 ++++++++--
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  1 +
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4a28caf..c9a1fa5a 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1993,7 +1993,13 @@ retry:
 			goto err_out;
 	}
 
-	if (!tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
+	if (flags & ATA_READID_NOSTART && id[2] == 0x37c8)
+	{
+		dev->flags |= ATA_DFLAG_SLEEPING;
+		return -EAGAIN;
+	}
+	if (!(flags & ATA_READID_NOSTART) &&
+	    !tried_spinup && (id[2] == 0x37c8 || id[2] == 0x738c)) {
 		tried_spinup = 1;
 		/*
 		 * Drive powered-up in standby mode, and requires a specific
@@ -4024,6 +4030,8 @@ int ata_dev_revalidate(struct ata_device *dev, unsigned int new_class,
 
 	/* re-read ID */
 	rc = ata_dev_reread_id(dev, readid_flags);
+	if (rc == -EAGAIN)
+		return rc;
 	if (rc)
 		goto fail;
 
@@ -5423,7 +5431,7 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
 	int rc;
 
 	rc = ata_port_request_pm(ap, mesg, ATA_EH_RESET,
-		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET, async);
+		ATA_EHI_NO_AUTOPSY | ATA_EHI_QUIET | ATA_EHI_NOSTART, async);
 	return rc;
 }
 
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 92d7797..0147b0d 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3094,6 +3094,8 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 
 		if (ehc->i.flags & ATA_EHI_DID_RESET)
 			readid_flags |= ATA_READID_POSTRESET;
+		if (ehc->i.flags & ATA_EHI_NOSTART)
+			readid_flags |= ATA_READID_NOSTART;
 
 		if ((action & ATA_EH_REVALIDATE) && ata_dev_enabled(dev)) {
 			WARN_ON(dev->class == ATA_DEV_PMP);
@@ -3106,6 +3108,10 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
 			ata_eh_about_to_do(link, dev, ATA_EH_REVALIDATE);
 			rc = ata_dev_revalidate(dev, ehc->classes[dev->devno],
 						readid_flags);
+			if (rc == -EAGAIN) {
+				rc = 0; /* start required but suppressed, handle later */
+				continue;
+			}
 			if (rc)
 				goto err;
 
@@ -3432,7 +3438,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 			hints &= ~ATA_LPM_HIPM;
 
 		/* disable DIPM before changing link config */
-		if (policy != ATA_LPM_MIN_POWER && dipm) {
+		if (policy != ATA_LPM_MIN_POWER && dipm && !(dev->flags & ATA_DFLAG_SLEEPING)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_DISABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
@@ -3476,7 +3482,7 @@ static int ata_eh_set_lpm(struct ata_link *link, enum ata_lpm_policy policy,
 	/* host config updated, enable DIPM if transitioning to MIN_POWER */
 	ata_for_each_dev(dev, link, ENABLED) {
 		if (policy == ATA_LPM_MIN_POWER && !no_dipm &&
-		    ata_id_has_dipm(dev->id)) {
+		    ata_id_has_dipm(dev->id) && !(dev->flags & ATA_DFLAG_SLEEPING)) {
 			err_mask = ata_dev_set_feature(dev,
 					SETFEATURES_SATA_ENABLE, SATA_DIPM);
 			if (err_mask && err_mask != AC_ERR_DEV) {
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 45b5ab3..8939345 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -42,6 +42,7 @@ struct ata_scsi_args {
 enum {
 	/* flags for ata_dev_read_id() */
 	ATA_READID_POSTRESET	= (1 << 0), /* reading ID after reset */
+	ATA_READID_NOSTART	= (1 << 1), /* do not start drive */
 
 	/* selector for ata_down_xfermask_limit() */
 	ATA_DNXFER_PIO		= 0,	/* speed down PIO */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 0e23c26..3929bb8 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -362,6 +362,7 @@ enum {
 
 	/* ata_eh_info->flags */
 	ATA_EHI_HOTPLUGGED	= (1 << 0),  /* could have been hotplugged */
+	ATA_EHI_NOSTART		= (1 << 1),  /* don't start the disk */
 	ATA_EHI_NO_AUTOPSY	= (1 << 2),  /* no autopsy */
 	ATA_EHI_QUIET		= (1 << 3),  /* be quiet */
 	ATA_EHI_NO_RECOVERY	= (1 << 4),  /* no recovery */
-- 
1.8.3.2

[PATCH 5/6] sd: don't start disks on system resume

[Phillip Susi]

Instead of forcing a disk to start up with the START STOP UNIT
command when the system resumes, let it stay asleep if runtime
pm is enabled, and it will start the drive when it is accessed.
Query the drive to see if it starts up on its own ( like most
ATA disks do ) and update the runtime pm status if so.
---
 drivers/scsi/sd.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++------
 drivers/scsi/sd.h |  1 +
 2 files changed, 86 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e6c4bff..98c082a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -107,7 +107,8 @@ static int  sd_remove(struct device *);
 static void sd_shutdown(struct device *);
 static int sd_suspend_system(struct device *);
 static int sd_suspend_runtime(struct device *);
-static int sd_resume(struct device *);
+static int sd_resume_system(struct device *);
+static int sd_resume_runtime(struct device *dev);
 static void sd_rescan(struct device *);
 static int sd_done(struct scsi_cmnd *);
 static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int);
@@ -486,11 +487,11 @@ static struct class sd_disk_class = {
 
 static const struct dev_pm_ops sd_pm_ops = {
 	.suspend		= sd_suspend_system,
-	.resume			= sd_resume,
+	.resume			= sd_resume_system,
 	.poweroff		= sd_suspend_system,
-	.restore		= sd_resume,
+	.restore		= sd_resume_system,
 	.runtime_suspend	= sd_suspend_runtime,
-	.runtime_resume		= sd_resume,
+	.runtime_resume		= sd_resume_runtime,
 };
 
 static struct scsi_driver sd_template = {
@@ -3166,22 +3167,97 @@ static int sd_suspend_runtime(struct device *dev)
 	return sd_suspend_common(dev, false);
 }
 
-static int sd_resume(struct device *dev)
+static int sd_resume_runtime(struct device *dev)
 {
 	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
 	int ret = 0;
 
-	if (!sdkp->device->manage_start_stop)
-		goto done;
-
 	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
+	dump_stack();
 	ret = sd_start_stop_device(sdkp, 1);
 
-done:
 	scsi_disk_put(sdkp);
 	return ret;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static void sd_resume_work(struct work_struct *work)
+{
+	struct scsi_disk *sdkp = container_of(work, struct scsi_disk, resume_work.work);
+	struct scsi_device *sdp = sdkp->device;
+	struct device *dev = &sdkp->dev;
+	int res;
+	unsigned char cmd[10] = { 0 };
+	struct scsi_sense_hdr sshdr;
+	const int timeout = sdp->request_queue->rq_timeout;
+
+	cmd[0] = REQUEST_SENSE;
+	printk(KERN_NOTICE "Requesting sense\n");
+	res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
+				     &sshdr, timeout, SD_MAX_RETRIES,
+				     NULL, REQ_PM);
+	if (res) {
+		sd_print_result(sdkp, res);
+		goto wakeup;
+	}
+	if (driver_byte(res) & DRIVER_SENSE)
+		sd_print_sense_hdr(sdkp, &sshdr);
+	/* we need to evaluate the error return  */
+	if (scsi_sense_valid(&sshdr) &&
+	    sshdr.sense_key == 0 &&
+	    sshdr.asc == 0 &&
+	    sshdr.ascq == 0)
+		goto wakeup;
+
+	/* finalize suspend */
+	printk(KERN_NOTICE "Finalizing suspend\n");
+	pm_runtime_disable(dev);
+	pm_runtime_set_suspended(dev);
+	pm_runtime_enable(dev);
+	blk_post_runtime_suspend(sdp->request_queue, 0);
+	scsi_disk_put(sdkp);
+	return;
+
+wakeup:
+	/* abort suspend */
+	sd_printk(KERN_NOTICE, sdkp, "Activating disk %s\n", kobject_name(&dev->kobj));
+	blk_post_runtime_suspend(sdp->request_queue, -EBUSY);
+	scsi_disk_put(sdkp);
+}
+#endif
+
+
+static int sd_resume_system(struct device *dev)
+{
+	struct scsi_disk *sdkp;
+#ifdef CONFIG_PM_RUNTIME
+	struct scsi_device *sdp;
+	int ret = 0;
+
+	sdkp = scsi_disk_get_from_dev(dev);
+	sdp = sdkp->device;
+	if (!scsi_device_online(sdp))
+	{
+		ret = -ENODEV;
+		goto err;
+	}
+	sd_printk(KERN_NOTICE, sdkp, "Suspending disk\n");
+	/* force runtime pm status to suspending */
+	blk_pre_runtime_suspend(sdp->request_queue);
+	execute_in_process_context(sd_resume_work,
+				   &sdkp->resume_work);
+
+err:
+	scsi_disk_put(sdkp);
+	return ret;
+#else
+	if (sdkp->device->manage_start_stop)
+		return sd_resume_runtime(dev);
+	return 0;
+#endif
+}
+
+
 /**
  *	init_sd - entry point for this driver (both when built in or when
  *	a module).
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 26895ff..0b8afb3 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -90,6 +90,7 @@ struct scsi_disk {
 	unsigned	lbpvpd : 1;
 	unsigned	ws10 : 1;
 	unsigned	ws16 : 1;
+	struct execute_work resume_work;
 };
 #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
 
-- 
1.8.3.2

[PATCH 6/6] libata: return power status in REQUEST SENSE command

[Phillip Susi]

SAT-3 says REQUEST SENSE should issue CHECK POWER and return
a sense status indicating the drive's power status.
---
 drivers/ata/libata-scsi.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index f92eb21..8b17352 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3313,6 +3313,38 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
 	return 1;
 }
 
+static void ata_scsi_request_sense_complete(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	cmd->result = (DRIVER_SENSE << 24);
+	if (qc->result_tf.nsect == 0)
+		/* POWER STATE CHANGE TO STANDBY */
+	{
+		scsi_build_sense_buffer(0, cmd->sense_buffer, 0, 0x5E, 0x43);
+	}
+	else scsi_build_sense_buffer(0, cmd->sense_buffer, 0, 0, 0);
+	qc->scsidone(cmd);
+	ata_qc_free(qc);
+}
+
+/**
+ *	ata_scsi_request_sense_xlat - Simulate REQUEST SENSE command
+ *	@qc: Storage for translated ATA taskfile
+ *
+ *	Converts a REQUEST SENSE command to an ATA CHECK POWER MODE taskfile.
+ *
+ *	LOCKING:
+ *	spin_lock_irqsave(host lock)
+ */
+static unsigned int ata_scsi_request_sense_xlat(struct ata_queued_cmd *qc)
+{
+	qc->tf.command = ATA_CMD_CHK_POWER;
+	qc->flags |= ATA_QCFLAG_RESULT_TF;
+	qc->tf.protocol = ATA_PROT_NODATA;
+	qc->complete_fn = ata_scsi_request_sense_complete;
+	return 0;
+}
+
 /**
  *	ata_get_xlat_func - check if SCSI to ATA translation is possible
  *	@dev: ATA device
@@ -3360,6 +3392,8 @@ static inline ata_xlat_func_t ata_get_xlat_func(struct ata_device *dev, u8 cmd)
 
 	case START_STOP:
 		return ata_scsi_start_stop_xlat;
+	case REQUEST_SENSE:
+		return ata_scsi_request_sense_xlat;
 	}
 
 	return NULL;
@@ -3565,12 +3599,6 @@ void ata_scsi_simulate(struct ata_device *dev, struct scsi_cmnd *cmd)
 		ata_scsi_rbuf_fill(&args, ata_scsiop_report_luns);
 		break;
 
-	case REQUEST_SENSE:
-		ata_scsi_set_sense(cmd, 0, 0, 0);
-		cmd->result = (DRIVER_SENSE << 24);
-		cmd->scsi_done(cmd);
-		break;
-
 	/* if we reach this, then writeback caching is disabled,
 	 * turning this into a no-op.
 	 */
-- 
1.8.3.2

Re: [PATCH 5/6] sd: don't start disks on system resume

[James Bottomley]

On Mon, 2013-12-16 at 18:30 -0500, Phillip Susi wrote:
> Instead of forcing a disk to start up with the START STOP UNIT
> command when the system resumes, let it stay asleep if runtime
> pm is enabled, and it will start the drive when it is accessed.
> Query the drive to see if it starts up on its own ( like most
> ATA disks do ) and update the runtime pm status if so.
> ---
>  drivers/scsi/sd.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++------
>  drivers/scsi/sd.h |  1 +
>  2 files changed, 86 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e6c4bff..98c082a 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -107,7 +107,8 @@ static int  sd_remove(struct device *);
>  static void sd_shutdown(struct device *);
>  static int sd_suspend_system(struct device *);
>  static int sd_suspend_runtime(struct device *);
> -static int sd_resume(struct device *);
> +static int sd_resume_system(struct device *);
> +static int sd_resume_runtime(struct device *dev);
>  static void sd_rescan(struct device *);
>  static int sd_done(struct scsi_cmnd *);
>  static int sd_eh_action(struct scsi_cmnd *, unsigned char *, int, int);
> @@ -486,11 +487,11 @@ static struct class sd_disk_class = {
>  
>  static const struct dev_pm_ops sd_pm_ops = {
>  	.suspend		= sd_suspend_system,
> -	.resume			= sd_resume,
> +	.resume			= sd_resume_system,
>  	.poweroff		= sd_suspend_system,
> -	.restore		= sd_resume,
> +	.restore		= sd_resume_system,
>  	.runtime_suspend	= sd_suspend_runtime,
> -	.runtime_resume		= sd_resume,
> +	.runtime_resume		= sd_resume_runtime,
>  };
>  
>  static struct scsi_driver sd_template = {
> @@ -3166,22 +3167,97 @@ static int sd_suspend_runtime(struct device *dev)
>  	return sd_suspend_common(dev, false);
>  }
>  
> -static int sd_resume(struct device *dev)
> +static int sd_resume_runtime(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
>  	int ret = 0;
>  
> -	if (!sdkp->device->manage_start_stop)
> -		goto done;
> -

This would make every disk start on resume ... even those which were
spun down before suspend.

>  	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> +	dump_stack();

I assume this is a debug left over that should be removed?

>  	ret = sd_start_stop_device(sdkp, 1);
>  
> -done:
>  	scsi_disk_put(sdkp);
>  	return ret;
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME

I really don't like the ifdefs.  The behaviour changes look pretty
radical for something that's supposed to be an add on to system suspend.

> +static void sd_resume_work(struct work_struct *work)
> +{
> +	struct scsi_disk *sdkp = container_of(work, struct scsi_disk, resume_work.work);
> +	struct scsi_device *sdp = sdkp->device;
> +	struct device *dev = &sdkp->dev;
> +	int res;
> +	unsigned char cmd[10] = { 0 };
> +	struct scsi_sense_hdr sshdr;
> +	const int timeout = sdp->request_queue->rq_timeout;
> +
> +	cmd[0] = REQUEST_SENSE;

You can't just send a random request sense because the reply will also
be pretty random (either no sense or the sense of the last check
condition depending on how the disk is feeling).  To see if a disk is
spun down, you need to send a test unit ready (TUR).

> +	printk(KERN_NOTICE "Requesting sense\n");
> +	res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> +				     &sshdr, timeout, SD_MAX_RETRIES,
> +				     NULL, REQ_PM);
> +	if (res) {
> +		sd_print_result(sdkp, res);
> +		goto wakeup;
> +	}
> +	if (driver_byte(res) & DRIVER_SENSE)
> +		sd_print_sense_hdr(sdkp, &sshdr);
> +	/* we need to evaluate the error return  */
> +	if (scsi_sense_valid(&sshdr) &&
> +	    sshdr.sense_key == 0 &&
> +	    sshdr.asc == 0 &&
> +	    sshdr.ascq == 0)

When you send the TUR, you only test the sense if check condition was
returned.   Plus, I think you only want to start the disk if it sends
back an ASC/ASCQ saying initialising command is required, don't you?

> +		goto wakeup;
> +
> +	/* finalize suspend */
> +	printk(KERN_NOTICE "Finalizing suspend\n");

OK, I think I'm lost here ... why are we finalising suspend in the
resume thread?

> +	pm_runtime_disable(dev);
> +	pm_runtime_set_suspended(dev);
> +	pm_runtime_enable(dev);
> +	blk_post_runtime_suspend(sdp->request_queue, 0);
> +	scsi_disk_put(sdkp);
> +	return;
> +
> +wakeup:
> +	/* abort suspend */
> +	sd_printk(KERN_NOTICE, sdkp, "Activating disk %s\n", kobject_name(&dev->kobj));
> +	blk_post_runtime_suspend(sdp->request_queue, -EBUSY);
> +	scsi_disk_put(sdkp);
> +}
> +#endif
> +
> +
> +static int sd_resume_system(struct device *dev)
> +{
> +	struct scsi_disk *sdkp;
> +#ifdef CONFIG_PM_RUNTIME
> +	struct scsi_device *sdp;
> +	int ret = 0;
> +
> +	sdkp = scsi_disk_get_from_dev(dev);
> +	sdp = sdkp->device;
> +	if (!scsi_device_online(sdp))
> +	{
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +	sd_printk(KERN_NOTICE, sdkp, "Suspending disk\n");
> +	/* force runtime pm status to suspending */
> +	blk_pre_runtime_suspend(sdp->request_queue);
> +	execute_in_process_context(sd_resume_work,
> +				   &sdkp->resume_work);

Why is this necessary ... I thought resume was run on a thread (so it
can sleep)?

> +err:
> +	scsi_disk_put(sdkp);
> +	return ret;
> +#else
> +	if (sdkp->device->manage_start_stop)
> +		return sd_resume_runtime(dev);
> +	return 0;
> +#endif

This doesn't really look right: if we have runtime resume enabled, the
system resume will always spin up; if we don't, it will conditionally
spin up?

James

> +
> +
>  /**
>   *	init_sd - entry point for this driver (both when built in or when
>   *	a module).
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 26895ff..0b8afb3 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -90,6 +90,7 @@ struct scsi_disk {
>  	unsigned	lbpvpd : 1;
>  	unsigned	ws10 : 1;
>  	unsigned	ws16 : 1;
> +	struct execute_work resume_work;
>  };
>  #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
>  

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Tejun Heo]

On Mon, Nov 11, 2013 at 12:08:49PM -0500, Phillip Susi wrote:
> No, I did not tune my system; I fixed the kernel so that userspace's
> activities do not start those disks.

So, umm, implementing things in kernel to facilitate userland is great
but please don't try to work around userland from kernel.  It may be
easy now but inevitably becomes a maintenance burden where we don't
have much visibility into what userland is depending on.  If a certain
feature needs cooperation from userland, update userland accordingly.

Thanks.

-- 
tejun

Re: [PATCH/RESEND v2 0/2] SATA disk resume time optimization

[Phillip Susi]

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

On 12/17/2013 7:15 AM, Tejun Heo wrote:
> On Mon, Nov 11, 2013 at 12:08:49PM -0500, Phillip Susi wrote:
>> No, I did not tune my system; I fixed the kernel so that
>> userspace's activities do not start those disks.
> 
> So, umm, implementing things in kernel to facilitate userland is
> great but please don't try to work around userland from kernel.  It
> may be easy now but inevitably becomes a maintenance burden where
> we don't have much visibility into what userland is depending on.
> If a certain feature needs cooperation from userland, update
> userland accordingly.

udisks already cooperates by checking the power status before issuing
more commands that would cause the drive to spin up.  I suppose I
could have added a sysfs flag exposing the sleep state and had
userspace check that first, but it seems silly to add a whole new ABI
and to check the power state rather than simply fixing the existing
command to not wake the disk in order to tell userspace what we
already know using a code path we already have.


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

iQEcBAEBAgAGBQJSsF61AAoJEI5FoCIzSKrwMvEH/3IOti/mKMvwm6b0ie3+knMT
QLDPuq7K/7qkbHPuSD/Bm2OwJR69SRFYSUYLpLL/u8r1JAgRKJJx1YywFgpmRJm9
43bE+R3uXhpYxSy5zf0s8T1VbNalWWVZKjfrrXjJic+H37y0E/lZcUtq8fSPnNu9
xNXLZ2/dkmWbAw+zu2Zszao3zPeYtP3bqhYa6HFZYDKxFEl4IPg9uKnQ9rAq7D/Y
/WtqOlckSqWdPSLKbQh3nbEd9dvdIqEx2wuqK8jthoHR0+HFoAGRqRHY6PKLKal5
bnxl7T8Wr6dRlb13vZZy6TwHU6HRxK6w9vCmRhSg04hHvtA8q2DmDM5Go+4PZEg=
=w2m9
-----END PGP SIGNATURE-----

Re: [PATCH 5/6] sd: don't start disks on system resume

[Phillip Susi]

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

On 12/17/2013 1:43 AM, James Bottomley wrote:

>> -static int sd_resume(struct device *dev) +static int
>> sd_resume_runtime(struct device *dev) { struct scsi_disk *sdkp =
>> scsi_disk_get_from_dev(dev); int ret = 0;
>> 
>> -	if (!sdkp->device->manage_start_stop) -		goto done; -
> 
> This would make every disk start on resume ... even those which
> were spun down before suspend.

What?  Those are lines I removed.

>> sd_printk(KERN_NOTICE, sdkp, "Starting disk\n"); +	dump_stack();
> 
> I assume this is a debug left over that should be removed?

Yes.

>> ret = sd_start_stop_device(sdkp, 1);
>> 
>> -done: scsi_disk_put(sdkp); return ret; }
>> 
>> +#ifdef CONFIG_PM_RUNTIME
> 
> I really don't like the ifdefs.  The behaviour changes look pretty 
> radical for something that's supposed to be an add on to system
> suspend.

How is it radical?  Either we wake the disk as we did before, or we
use runtime pm to (try to) keep it suspended until accessed.  The
#ifdefs are required because it depends on runtime pm.

>> +static void sd_resume_work(struct work_struct *work) +{ +	struct
>> scsi_disk *sdkp = container_of(work, struct scsi_disk,
>> resume_work.work); +	struct scsi_device *sdp = sdkp->device; +
>> struct device *dev = &sdkp->dev; +	int res; +	unsigned char
>> cmd[10] = { 0 }; +	struct scsi_sense_hdr sshdr; +	const int
>> timeout = sdp->request_queue->rq_timeout; + +	cmd[0] =
>> REQUEST_SENSE;
> 
> You can't just send a random request sense because the reply will
> also be pretty random (either no sense or the sense of the last
> check condition depending on how the disk is feeling).  To see if a
> disk is spun down, you need to send a test unit ready (TUR).

- From what I can see in the standards, this is not true.  TEST UNIT
READY will give an error response if the drive requires START STOP
UNIT, but if it does not, either because it is actually an ata disk,
or because it auto starts ( the standard indicates scsi disks can be
configured to do this, but does not say how ), and is simply in
standby, then it will return success.

In other words, ata drives always return ready and scsi disks do as
well, if they are in standby mode rather than stopped mode.

On the other hand, there are several references in the standards to
using REQUEST SENSE to poll the drive status rather than to read sense
data for the last command, and I believe there was a passage somewhere
that mentioned the last command status is cleared once it has been
read once, presumably so that a subsequent REQUEST SENSE will instead
get the current status of the drive.  SAT-3 specifies that REQUEST
SENSE is to be translated to an ata CHECK POWER CONDITION specifically
so that it can be used to poll the drive to see if it is currently in
standby.

You can use sdparm --command=sense to issue a REQUEST SENSE command to
a drive and see for yourself.  If you configure the drive to auto
suspend using the timer, then poll it, you should see the response
indicating it is in standby.

See SPC-4 section 5.6 and SAT-3 section 8.10.

> When you send the TUR, you only test the sense if check condition
> was returned.   Plus, I think you only want to start the disk if it
> sends back an ASC/ASCQ saying initialising command is required,
> don't you?

REQUEST SENSE gives GOOD rather than CHECK CONDITION.  You want to NOT
start the disk if you get back that status.  The whole point is to let
the disk stay asleep.  The idea is to force the disk into runtime
suspend so it can sleep, and be started when accessed, but if it is
already up and running ( as most ata disks will be ), then don't
pretend it is suspended since it really isn't.

> OK, I think I'm lost here ... why are we finalising suspend in the 
> resume thread?

Because we want to let runtime pm start the disk at a later time, once
it is accessed, rather than when the system resumes.

>> +	execute_in_process_context(sd_resume_work, +
>> &sdkp->resume_work);
> 
> Why is this necessary ... I thought resume was run on a thread (so
> it can sleep)?

Because we don't want to block the resume path for many seconds and
delay user space unfreezing.  I actually need to fix that to
unconditionally queues the work item since right now it is in process
context, so it just directly calls the other function.

>> +err: +	scsi_disk_put(sdkp); +	return ret; +#else +	if
>> (sdkp->device->manage_start_stop) +		return
>> sd_resume_runtime(dev); +	return 0; +#endif
> 
> This doesn't really look right: if we have runtime resume enabled,
> the system resume will always spin up; if we don't, it will
> conditionally spin up?

Other way around; if we *don't* have runtime pm configured, then
system resume always spins up, and if we do, then we try to delay
spinning up until the disk is accessed, but check to see if the drive
has spun up on its own.


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

iQEcBAEBAgAGBQJSsGdQAAoJEI5FoCIzSKrwWRgH+wTIMDt7lyXbZVX/JNS7qkOI
eRLZnWurNSXpgkXQ2B+OQIgM+l18URbN9cf0j66o6tGH6M+CbKzT2iiQMR5lqbVY
ejkUVlZ3rruYSAc8sHGHJi1CYoHRh4wgQ65mGiRVBI6TPS5Gmqmy53QVOGaGRJwo
IZDV3oOaWelAQB4VF6T1jJHHHbraEuaXsGMwP+900mbN0uboYgkQ5pkwdNSd9dpO
CBDG0Dgn31U4P1dZUQpKtytUwm64keGzMQ8kgckiR77Y9fJ82hJ4QnFX0CmQuFgM
EeiEhTikYZ5Q1jHg8QFXkqnbigWj4o1fAJungs2iqwNrrhkwTaZW5OyUA8FV5xk=
=ZaZs
-----END PGP SIGNATURE-----

Re: [PATCH 5/6] sd: don't start disks on system resume

[James Bottomley]

On Tue, 2013-12-17 at 10:01 -0500, Phillip Susi wrote:
> On 12/17/2013 1:43 AM, James Bottomley wrote:
> 
> >> -static int sd_resume(struct device *dev) +static int
> >> sd_resume_runtime(struct device *dev) { struct scsi_disk *sdkp =
> >> scsi_disk_get_from_dev(dev); int ret = 0;
> >> 
> >> -	if (!sdkp->device->manage_start_stop) -		goto done; -
> > 
> > This would make every disk start on resume ... even those which
> > were spun down before suspend.
> 
> What?  Those are lines I removed.

Yes, they say that in sd_resume, if we're managing the disk start stop,
spin it up.  Removing them spins everything up unconditionally.

> >> sd_printk(KERN_NOTICE, sdkp, "Starting disk\n"); +	dump_stack();
> > 
> > I assume this is a debug left over that should be removed?
> 
> Yes.
> 
> >> ret = sd_start_stop_device(sdkp, 1);
> >> 
> >> -done: scsi_disk_put(sdkp); return ret; }
> >> 
> >> +#ifdef CONFIG_PM_RUNTIME
> > 
> > I really don't like the ifdefs.  The behaviour changes look pretty 
> > radical for something that's supposed to be an add on to system
> > suspend.
> 
> How is it radical?  Either we wake the disk as we did before, or we
> use runtime pm to (try to) keep it suspended until accessed.  The
> #ifdefs are required because it depends on runtime pm.

Good systems engineering practise does not have different behaviours for
code with and without ifdefs.  You want to be adding incremental
behaviour not modifying existing.  The reason is testing; now you have
to build two different kernels to test the two behaviours and mostly one
path gets selected by every distro and the other bitrots.

> >> +static void sd_resume_work(struct work_struct *work) +{ +	struct
> >> scsi_disk *sdkp = container_of(work, struct scsi_disk,
> >> resume_work.work); +	struct scsi_device *sdp = sdkp->device; +
> >> struct device *dev = &sdkp->dev; +	int res; +	unsigned char
> >> cmd[10] = { 0 }; +	struct scsi_sense_hdr sshdr; +	const int
> >> timeout = sdp->request_queue->rq_timeout; + +	cmd[0] =
> >> REQUEST_SENSE;
> > 
> > You can't just send a random request sense because the reply will
> > also be pretty random (either no sense or the sense of the last
> > check condition depending on how the disk is feeling).  To see if a
> > disk is spun down, you need to send a test unit ready (TUR).
> 
> From what I can see in the standards, this is not true.  TEST UNIT
> READY will give an error response if the drive requires START STOP
> UNIT, but if it does not, either because it is actually an ata disk,
> or because it auto starts ( the standard indicates scsi disks can be
> configured to do this, but does not say how ), and is simply in
> standby, then it will return success.

Are you confused about the power states?  in SCSI, stand by and idle are
different from spun down (or stopped).  Right at the moment, start stop
manages spin down/spin up not the granular power states for almost every
device.  The only subsystem that actually uses power state management is
firewire.  Even if you enable power state management for ATA, we have to
cope with non-ATA devices.

The standards actually say that SCSI devices automatically come out of
standby power conditions unless the transport protocol says something
different.

> In other words, ata drives always return ready and scsi disks do as
> well, if they are in standby mode rather than stopped mode.

Yes, but they aren't going to be in standby mode apart from firewire
devices; they're going to be stopped.

> On the other hand, there are several references in the standards to
> using REQUEST SENSE to poll the drive status rather than to read sense
> data for the last command, and I believe there was a passage somewhere
> that mentioned the last command status is cleared once it has been
> read once, presumably so that a subsequent REQUEST SENSE will instead
> get the current status of the drive.  SAT-3 specifies that REQUEST
> SENSE is to be translated to an ata CHECK POWER CONDITION specifically
> so that it can be used to poll the drive to see if it is currently in
> standby.
> 
> You can use sdparm --command=sense to issue a REQUEST SENSE command to
> a drive and see for yourself.  If you configure the drive to auto
> suspend using the timer, then poll it, you should see the response
> indicating it is in standby.
> 
> See SPC-4 section 5.6

That's "self test operations".  I assume you mean 6.29 "REQUEST SENSE"?

>  and SAT-3 section 8.10.

What SAT says is irrelevant.  That's about how to translate ATA to SCSI,
not how SCSI devices should behave.

> > When you send the TUR, you only test the sense if check condition
> > was returned.   Plus, I think you only want to start the disk if it
> > sends back an ASC/ASCQ saying initialising command is required,
> > don't you?
> 
> REQUEST SENSE gives GOOD rather than CHECK CONDITION.  You want to NOT
> start the disk if you get back that status.  The whole point is to let
> the disk stay asleep.  The idea is to force the disk into runtime
> suspend so it can sleep, and be started when accessed, but if it is
> already up and running ( as most ata disks will be ), then don't
> pretend it is suspended since it really isn't.
> 
> > OK, I think I'm lost here ... why are we finalising suspend in the 
> > resume thread?
> 
> Because we want to let runtime pm start the disk at a later time, once
> it is accessed, rather than when the system resumes.

I still don't get why a resume function call is doing suspend
operations.

> >> +	execute_in_process_context(sd_resume_work, +
> >> &sdkp->resume_work);
> > 
> > Why is this necessary ... I thought resume was run on a thread (so
> > it can sleep)?
> 
> Because we don't want to block the resume path for many seconds and
> delay user space unfreezing.  I actually need to fix that to
> unconditionally queues the work item since right now it is in process
> context, so it just directly calls the other function.

Um, then you need to read the code.  execute_in_process_context() is a
direct function call if we have process context, so it's not going to
use a work queue.

> >> +err: +	scsi_disk_put(sdkp); +	return ret; +#else +	if
> >> (sdkp->device->manage_start_stop) +		return
> >> sd_resume_runtime(dev); +	return 0; +#endif
> > 
> > This doesn't really look right: if we have runtime resume enabled,
> > the system resume will always spin up; if we don't, it will
> > conditionally spin up?
> 
> Other way around; if we *don't* have runtime pm configured, then
> system resume always spins up, and if we do, then we try to delay
> spinning up until the disk is accessed, but check to see if the drive
> has spun up on its own.

Please don't do this.  Significant behaviour changes depending on what
options are selected are not a good idea from the systems point of view.
Options should control incremental changes (function present or not
present).

James

Re: [PATCH 6/6] libata: return power status in REQUEST SENSE command

[Sergei Shtylyov]

Hello.

On 12/17/2013 02:30 AM, Phillip Susi wrote:

> SAT-3 says REQUEST SENSE should issue CHECK POWER and return
> a sense status indicating the drive's power status.
> ---
>   drivers/ata/libata-scsi.c | 40 ++++++++++++++++++++++++++++++++++------
>   1 file changed, 34 insertions(+), 6 deletions(-)

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index f92eb21..8b17352 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3313,6 +3313,38 @@ static unsigned int ata_scsi_mode_select_xlat(struct ata_queued_cmd *qc)
>   	return 1;
>   }
>
> +static void ata_scsi_request_sense_complete(struct ata_queued_cmd *qc)
> +{
> +	struct scsi_cmnd *cmd = qc->scsicmd;

    Please insert empty line here, after declaration.

> +	cmd->result = (DRIVER_SENSE << 24);

    () not needed.

> +	if (qc->result_tf.nsect == 0)
> +		/* POWER STATE CHANGE TO STANDBY */
> +	{

    Wrong *if* style, should be "*if* () {" on the same line.

> +		scsi_build_sense_buffer(0, cmd->sense_buffer, 0, 0x5E, 0x43);
> +	}
> +	else scsi_build_sense_buffer(0, cmd->sense_buffer, 0, 0, 0);

    Wrong style again, scsi_build_sense_buffer() should be on its own line, 
and {} must be used in both branches of *if* if used in one.

> +	qc->scsidone(cmd);
> +	ata_qc_free(qc);
> +}
> +
> +/**
> + *	ata_scsi_request_sense_xlat - Simulate REQUEST SENSE command
> + *	@qc: Storage for translated ATA taskfile
> + *
> + *	Converts a REQUEST SENSE command to an ATA CHECK POWER MODE taskfile.
> + *
> + *	LOCKING:
> + *	spin_lock_irqsave(host lock)
> + */
> +static unsigned int ata_scsi_request_sense_xlat(struct ata_queued_cmd *qc)
> +{
> +	qc->tf.command = ATA_CMD_CHK_POWER;

    Isn't it an optional command, belonging to the power management feature set?

WBR, Sergei

Re: [PATCH 5/6] sd: don't start disks on system resume

[Phillip Susi]

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

On 12/17/2013 12:48 PM, James Bottomley wrote:
> Yes, they say that in sd_resume, if we're managing the disk start
> stop, spin it up.  Removing them spins everything up
> unconditionally.

Ahh, right, I got rid of the manage_start_stop check.  Probably
shouldn't be in this patch, but I do think this flag should be removed
as we shouldn't be putting extra wear and tear on the disk by letting
it take an emergency head retract on power off.

> Good systems engineering practise does not have different
> behaviours for code with and without ifdefs.  You want to be adding
> incremental behaviour not modifying existing.  The reason is
> testing; now you have to build two different kernels to test the
> two behaviours and mostly one path gets selected by every distro
> and the other bitrots.

Unfortunately, the scsi error handling code is terrible so my attempt
to use that was not acceptable, so to do it requires runtime pm.

> Are you confused about the power states?  in SCSI, stand by and
> idle are different from spun down (or stopped).  Right at the
> moment, start stop manages spin down/spin up not the granular power
> states for almost every device.  The only subsystem that actually
> uses power state management is firewire.  Even if you enable power
> state management for ATA, we have to cope with non-ATA devices.

Yes, and ata drives do not have a stopped state.  They always power on
in either active or standby.  The SCSI standards even mention that
SCSI disks may be configured ( but don't say how ) to power on in
active rather than stopped.

> The standards actually say that SCSI devices automatically come out
> of standby power conditions unless the transport protocol says
> something different.

Yes, so when in standby the drive does not require START UNIT, but if
we set the runtime status to suspended, then we will issue one as soon
as a request comes in, causing the drive to spin up.  We don't want to
do that since some commands don't need the media to be spinning.

My goal was for the runtime pm status to reflect the drive status, so
if the drive is in standby, even though it doesn't need started, the
runtime status should show it is suspended.  I suppose that if I
didn't care about that, I could use TEST UNIT READY and only runtime
suspend if it requires the START UNIT command.

> Yes, but they aren't going to be in standby mode apart from
> firewire devices; they're going to be stopped.

No, because again, ata has no stopped state.  ata disks will either be
active, or standby if they have power up in standby enabled.

> What SAT says is irrelevant.  That's about how to translate ATA to
> SCSI, not how SCSI devices should behave.

It is relevant because they wouldn't have said to translate it that
way if they didn't intend REQUEST SENSE to be used that way.  If it
was the way you say, then it would say that the TEST UNIT READY
command should be translated to CHECK POWER, rather than REQUEST
SENSE.  Of course, SPC is the more authoritative source, which also
confirms it.

>> Because we want to let runtime pm start the disk at a later time,
>> once it is accessed, rather than when the system resumes.
> 
> I still don't get why a resume function call is doing suspend 
> operations.

Because runtime pm only resumes it if it is runtime suspended... if we
don't start the drive in the system resume handler, then we need to
block any requests until it is started, which is exactly what being
runtime suspended does.

>> Because we don't want to block the resume path for many seconds
>> and delay user space unfreezing.  I actually need to fix that to 
>> unconditionally queues the work item since right now it is in
>> process context, so it just directly calls the other function.
> 
> Um, then you need to read the code.  execute_in_process_context()
> is a direct function call if we have process context, so it's not
> going to use a work queue.

That's what I just said.

> Please don't do this.  Significant behaviour changes depending on
> what options are selected are not a good idea from the systems
> point of view. Options should control incremental changes (function
> present or not present).

Well, that is what it is doing.  The function of remaining suspended
after system resume is either present or not.

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

iQEcBAEBAgAGBQJSsJhEAAoJEI5FoCIzSKrwFg0H/3xMLeKAA3ne7dTGfsDJu/E1
MQ9GBNgp+A/b08y8etJF2rDXNbWt0XqPWJnZWNJ/ubirShSVbQrTwIIatJ8qumG5
g6Q3nAmL7LtuqBFKwJJbo9/b6PdlCSXvkD5m4Dz4ZiCMHJQxD91n4m/iEtTXG/Cr
/xxdZwPqd1QQ8fOm573QeSeilIsoqDH51HSdaHx8c+C9bubBSAIiWtXDPRac8zLL
X7fIDJFyLtv/SoygCOrKuyalfjlDsbxl44qGT9mAgLJyi6Dj+StYbXvDwTtRP360
ZIlYpphLt0ybqFr588rluEu6PgI2V+93ukvmrY8YliaoyKiSP1G5c6Q48kk25Nw=
=BE0e
-----END PGP SIGNATURE-----

Re: [PATCH 6/6] libata: return power status in REQUEST SENSE command

[Phillip Susi]

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

On 12/17/2013 1:02 PM, Sergei Shtylyov wrote:
> Isn't it an optional command, belonging to the power management 
> feature set?

Seems to be required.  ATA-8 section 4.16 says:

"A General feature set device shall implement power management"


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

iQEcBAEBAgAGBQJSsJlVAAoJEI5FoCIzSKrwieEIAKA6y3RdJO20gUyyxBQ8lRYV
jcEOcZpbBZPXNAFKXG8HvGwIIOiFNmygoSmq2q4WL2M2PUjBMkdltqcKIJgyxjW5
071HdiwAclt/KibVM2CC2PaZBVTMlCBb615B+wwjmiRveVMED9nh7htqxiKWHX16
u7T9J6Lq6nuTsoYrwGbd7c/oB5AQE6M1qDIJFtw6V7otRPYaL+P2zofhrcmBeOAe
COi/SZiQXksUHA0aSIvTpOS2zMnuHu8Uo9cpIS++gsUqsfuIkqqsyp79fbPIvqvm
WaZg43lzrFhoiZmyy/cvPm7Ty4dTkRbGo7BQ0V4S9GeSGcGRki8eZBIkHd1F4d8=
=i1f0
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

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

On 12/16/2013 06:30 PM, Phillip Susi wrote:
> For some reason, the system hangs on resume if I issue the REQUEST
> SENSE command after blk_pre_runtime_suspend.  My understanding is
> that the REQ_PM flag should make the command process even though
> the queue is RPM_SUSPENDING, but it doesn't seem to work.  Anyone
> have any idea why?

So I found the problem but I'm confused by it.  I thought that the
REQ_PM flag was supposed to make sure the request was dispatched even
though the device was still suspended ( or suspending ).  It seems
that this is not the case, and only requests with the type set to
REQ_TYPE_PM_RESUME are dispatched during suspend/resume.  The
following patch fixes the hang, but I'm not sure why it is needed or
if it is generally appropriate:

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7bd7f0d..c5ce50d 100644
- --- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -227,7 +227,9 @@ int scsi_execute(struct scsi_device *sdev, const
unsigned char *cmd,
        req->sense_len = 0;
        req->retries = retries;
        req->timeout = timeout;
- -       req->cmd_type = REQ_TYPE_BLOCK_PC;
+       if (flags & REQ_PM)
+               req->cmd_type = REQ_TYPE_PM_RESUME;
+       else req->cmd_type = REQ_TYPE_BLOCK_PC;
        req->cmd_flags |= flags | REQ_QUIET | REQ_PREEMPT;

        /*


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

iQEcBAEBCgAGBQJSyhGRAAoJEI5FoCIzSKrwbB4IAJqrtJuw92+rJQqkC7W5qRe8
psqgHiplkb9HCb3po95kM3c+05xjukoYNOk4+1s2iS0Adu288pSch2D20qPsT9Pc
DOHdU4iyaZ85gLxOIxdpCa+YYBYAzB8LEkqSMMvuhRkMqa008kclvVfqYtGhmw2g
DJcvuyowXLHQrpx+AA+CqY7aWxMK46uiNIwE3v8GA17augRzfltijMDZU/9SjG/7
lIe4V4lVf9SeWphVh0BSWULT84QOVQhcYGO/zK+I7xei2igPAHpBzHuUfXqs44PB
Z37eHf1JENZkuyekBDth+PsrnuiVZG20ECvymipoEQGv1iQ6R/QZsviNv/XUDgo=
=nAGL
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Sujit Reddy Thumma]

On 1/6/2014 7:44 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 12/16/2013 06:30 PM, Phillip Susi wrote:
>> For some reason, the system hangs on resume if I issue the REQUEST
>> SENSE command after blk_pre_runtime_suspend.  My understanding is
>> that the REQ_PM flag should make the command process even though
>> the queue is RPM_SUSPENDING, but it doesn't seem to work.  Anyone
>> have any idea why?
>
> So I found the problem but I'm confused by it.  I thought that the
> REQ_PM flag was supposed to make sure the request was dispatched even
> though the device was still suspended ( or suspending ).  It seems
> that this is not the case, and only requests with the type set to
> REQ_TYPE_PM_RESUME are dispatched during suspend/resume.  The
> following patch fixes the hang, but I'm not sure why it is needed or
> if it is generally appropriate:
>

Adding Aaron Lu, author of block layer runtime PM.
I have seen similar issue and fixed it by relaxing the rpm_status check
to allow processing of requests while suspended -

diff --git a/block/blk-core.c b/block/blk-core.c
index 6853ef6..f99165b 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2140,8 +2140,7 @@ static void blk_account_io_done(struct request *req)
  static struct request *blk_pm_peek_request(struct request_queue *q,
  					   struct request *rq)
  {
-	if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
-	    (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM))))
+	if (q->dev && q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM))
  		return NULL;
  	else
  		return rq;


> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7bd7f0d..c5ce50d 100644
> - --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -227,7 +227,9 @@ int scsi_execute(struct scsi_device *sdev, const
> unsigned char *cmd,
>          req->sense_len = 0;
>          req->retries = retries;
>          req->timeout = timeout;
> - -       req->cmd_type = REQ_TYPE_BLOCK_PC;
> +       if (flags & REQ_PM)
> +               req->cmd_type = REQ_TYPE_PM_RESUME;
> +       else req->cmd_type = REQ_TYPE_BLOCK_PC;
>          req->cmd_flags |= flags | REQ_QUIET | REQ_PREEMPT;
>
>          /*
>
>

-- 
Regards,
Sujit

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Aaron Lu]

On 01/06/2014 03:36 PM, Sujit Reddy Thumma wrote:
> On 1/6/2014 7:44 AM, Phillip Susi wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA512
>>
>> On 12/16/2013 06:30 PM, Phillip Susi wrote:
>>> For some reason, the system hangs on resume if I issue the REQUEST
>>> SENSE command after blk_pre_runtime_suspend.  My understanding is
>>> that the REQ_PM flag should make the command process even though
>>> the queue is RPM_SUSPENDING, but it doesn't seem to work.  Anyone
>>> have any idea why?
>>
>> So I found the problem but I'm confused by it.  I thought that the
>> REQ_PM flag was supposed to make sure the request was dispatched even
>> though the device was still suspended ( or suspending ).  It seems

The blk_pm_peek_request will return NULL if the block queue is runtime
suspended.
The block queue will be transferred to runtime active state once there
is a normal request put into the suspended queue.
A suspended queue means there is no request left.

The REQ_PM flag is used to carry out some last commands for the underlying
device to get into or out of a low power state, and at that time, the
queue's status is either RPM_SUSPENDING or RPM_RESUMING.

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.

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
pm_request_resume for the device in its system resume callback should be
enough, or if you want to further delay it, just leave it as it is. Once
there is a new request comes in, it will be automatically runtime
resumed(actually, there are some user space tools accessing it during
system resume, so it will be resumed pretty soon after kernel thaw user
space process, unless you have made those app gone). It's simple and clean.
The only problem is, it depends on RUNTIME_PM, while Todd's implementation
doesn't.

>> that this is not the case, and only requests with the type set to
>> REQ_TYPE_PM_RESUME are dispatched during suspend/resume.  The
>> following patch fixes the hang, but I'm not sure why it is needed or
>> if it is generally appropriate:
>>
> 
> Adding Aaron Lu, author of block layer runtime PM.
> I have seen similar issue and fixed it by relaxing the rpm_status check
> to allow processing of requests while suspended -

I'm not sure what exactly the issue you have is, care to explain?

> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 6853ef6..f99165b 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -2140,8 +2140,7 @@ static void blk_account_io_done(struct request *req)
>   static struct request *blk_pm_peek_request(struct request_queue *q,
>   					   struct request *rq)
>   {
> -	if (q->dev && (q->rpm_status == RPM_SUSPENDED ||
> -	    (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM))))
> +	if (q->dev && q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM))

The queue is not supposed to get a REQ_PM request while it is in
RPM_SUSPENDED state, at least, this is the case when we first made this
function.

Thanks,
Aaron

>   		return NULL;
>   	else
>   		return rq;
> 
> 
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index 7bd7f0d..c5ce50d 100644
>> - --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -227,7 +227,9 @@ int scsi_execute(struct scsi_device *sdev, const
>> unsigned char *cmd,
>>          req->sense_len = 0;
>>          req->retries = retries;
>>          req->timeout = timeout;
>> - -       req->cmd_type = REQ_TYPE_BLOCK_PC;
>> +       if (flags & REQ_PM)
>> +               req->cmd_type = REQ_TYPE_PM_RESUME;
>> +       else req->cmd_type = REQ_TYPE_BLOCK_PC;
>>          req->cmd_flags |= flags | REQ_QUIET | REQ_PREEMPT;
>>
>>          /*
>>
>>
> 

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

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

> 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
blk_pre_runtime_suspend to force it into RPM_SUSPENDING so as to make
sure that after the system resume function returns, user space IO will
be blocked until we can verify whether the disk has spun up on its own
or not.

> pm_request_resume for the device in its system resume callback
> should be enough, or if you want to further delay it, just leave it
> as it is. Once there is a new request comes in, it will be
> automatically runtime resumed(actually, there are some user space
> tools accessing it during system resume, so it will be resumed
> pretty soon after kernel thaw user space process, unless you have
> made those app gone). It's simple and clean. The only problem is,
> it depends on RUNTIME_PM, while Todd's implementation doesn't.

Yes, that's the idea; leave it runtime suspended until someone
actually tries to access it.  Unless the drive spins up on its own (
as ATA disks without PuiS enabled do ).

>>> that this is not the case, and only requests with the type set
>>> to REQ_TYPE_PM_RESUME are dispatched during suspend/resume.
>>> The following patch fixes the hang, but I'm not sure why it is
>>> needed or if it is generally appropriate:
>>> 
>> 
>> Adding Aaron Lu, author of block layer runtime PM. I have seen
>> similar issue and fixed it by relaxing the rpm_status check to
>> allow processing of requests while suspended -
> 
> I'm not sure what exactly the issue you have is, care to explain?

The issue is that the REQ_PM flagged REQUEST SENSE command issued
during the system_resume is never dispatched.  I patched scsi_execute
to make the request type REQ_TYPE_PM_RESUME and that got the request
dispatched.

Looking at the code a bit more today, it looks like REQ_PM has
blk_peek_request return the request to scsi_request_fn, but it refuses
to dispatch it unless it is of type REQ_TYPE_PM_RESUME apparently
because the queue is stopped.  I suppose now the question is why is it
stopped?

> The queue is not supposed to get a REQ_PM request while it is in 
> RPM_SUSPENDED state, at least, this is the case when we first made
> this function.

Right... that's why I forced it into RPM_SUSPENDING; so I can issue
the REQUEST SENSE, but other IO is blocked.  Then either complete the
transition to RPM_SUSPENDED, or abort and go back to RPM_ACTIVE.

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

iQEcBAEBAgAGBQJSysBnAAoJEI5FoCIzSKrwM5MH/R+Pvk2hdzk1adSrxuvi28RV
bGxHSRc2RCqU0CyIwpZtNnZlUiqk34xuM1hzcqsPMdRStmOCpQciGuBhxynoODDW
Lm2XLtCfaNihsfuOHrc/nVmBZGPtzXP+OdOWEi9JfkUEcsS8ePFRh9YNXJOl1g26
pU41dmTwWrPYSnN7SRO+xArb1TWh7jcCSGqyxY2BVJL1aSpSicIlbnJDzijFSQ2h
+Rs7fZwnnzRLbQFmNcpems++vIVIYLRbuOJNFjeMLKt1KEX+Eiq4q/p5bJmt1H9n
zre1zoz7P5kU2zW90W+seFfAW2ka8stq4BAQGvz4KwEiV+altbEpYHLbtanXXEo=
=ZDJw
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Alan Stern]

On Mon, 6 Jan 2014, Phillip Susi wrote:

> The issue is that the REQ_PM flagged REQUEST SENSE command issued
> during the system_resume is never dispatched.  I patched scsi_execute
> to make the request type REQ_TYPE_PM_RESUME and that got the request
> dispatched.

Note that REQ_TYPE_PM_RESUME is a fossil.  It is used for power 
management in the legacy IDE driver and nowhere else (although there is 
a single vestige of that use remaining in the block layer).  It should 
be removed -- if anybody feels like cleaning up the IDE driver.

> Looking at the code a bit more today, it looks like REQ_PM has
> blk_peek_request return the request to scsi_request_fn, but it refuses
> to dispatch it unless it is of type REQ_TYPE_PM_RESUME apparently
> because the queue is stopped.  I suppose now the question is why is it
> stopped?

Indeed.  I can't see any place where the SCSI or block layers would
stop the queue of a device when it gets runtime suspended.  Maybe some
other layer is doing this.

Alan Stern

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

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

On 01/06/2014 10:38 AM, Alan Stern wrote:
> Indeed.  I can't see any place where the SCSI or block layers
> would stop the queue of a device when it gets runtime suspended.
> Maybe some other layer is doing this.

So I've stepped through it in qemu a good bit, and found some strange
things.  At the time that sd_resume_system is called, the host is
still in the recovery state as a result of ata_port_resume, so this is
why scsi_request_fn doesn't dispatch the request.  It requeues it for
after the eh finishes, but when the eh finishes and sets the state
back to running, scsi_request_fn calls blk_peek_request, and it
returns NULL rather than the requeued REQUEST SENSE.  It's like the
request is lost rather than having been requeued.


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

iQEcBAEBCgAGBQJSy2oiAAoJEI5FoCIzSKrwTu4H/RONbp4lsY5jfSCwRCB23W5q
dn8ZmGDzUbjzEeq/DJzARlWdxYadepIDDUKLNVYW1eUrN0+sPLV8jIlDZZgg3dbm
kh0MMPjB2sMbz1zgTQ0/7rZw7WQZfE/vwAo95QQD42FK+SsRMwMoLaN6GsZrikrT
MXzDMXCRLqaow4kSPj7uonN5wlw+1Y5Sn5LJ/SdzPyz2JX/x0lk8zMekNC0i6/tF
ooQymcRFdiDFhr9E6VKj3+TEpVH708zA9vVSjzIZ6KIUD8mA+r2d8pPUWSrjuSzC
U9EVNmawGJMsq70Ql2I8+0J9csz8I3ijQjpfxhw7+V89Fx+t+AH901hYnNY8Qeo=
=hCxT
-----END PGP SIGNATURE-----

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

[Alan Stern]

On Mon, 6 Jan 2014, Phillip Susi wrote:

> On 01/06/2014 10:38 AM, Alan Stern wrote:
> > Indeed.  I can't see any place where the SCSI or block layers
> > would stop the queue of a device when it gets runtime suspended.
> > Maybe some other layer is doing this.
> 
> So I've stepped through it in qemu a good bit, and found some strange
> things.  At the time that sd_resume_system is called, the host is
> still in the recovery state as a result of ata_port_resume, so this is
> why scsi_request_fn doesn't dispatch the request.  It requeues it for
> after the eh finishes, but when the eh finishes and sets the state
> back to running, scsi_request_fn calls blk_peek_request, and it
> returns NULL rather than the requeued REQUEST SENSE.  It's like the
> request is lost rather than having been requeued.

This raises two questions:

	Should the host's resume be allowed to complete while the
	host is still in error recovery?  Wouldn't it be better to
	wait until the recovery is finished?

	How does the queue end up in a stopped state?  (The stopped
	queue explains why blk_peek_request returns NULL.)

You're in a better position than I am to answer these questions.

Alan Stern

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:20 AM, Alan Stern wrote:
> This raises two questions:
> 
> Should the host's resume be allowed to complete while the host is
> still in error recovery?  Wouldn't it be better to wait until the
> recovery is finished?

Ahh, right, my other patch changed the ata_port stuff to schedule the
eh, and return  from resume, so that IT doesn't block the system
resume path for a long period of time while IT tries to spin up the
disk.  Both the libata and sd layers were spinning up the disk.

Still, the sd request should just wait in the queue until after the
recovery completes shouldn't it?

> How does the queue end up in a stopped state?  (The stopped queue
> explains why blk_peek_request returns NULL.)

I was guessing that was the cause from reading the code, after
stepping through it with gdb and qemu, it turned out it was in
SHOST_RECOVERY rather that stopped.  I suppose it may be stopped as
well, so I guess I'll have to go do another debug session and make
sure, but I think that was a red herring.

The flow seems to simply be:

host is in recovery
sd_resume queues REQUEST SENSE
scsi_request_fn notices host is in recovery and requeus REQUEST SENSE
recovery completes
REQUEST SENSE seems to be lost...

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

iQEcBAEBAgAGBQJSzB/yAAoJEI5FoCIzSKrwk0oH/Ays68TYzM2T0GQ6l95YyZy/
j3UU6gRrqv4HGcbRjXwyX1itCTrL+1QKVE4sa1cDc2Uv7oHSU8e28Es428WjseUm
fCfeil+lQpDcSAVX78Iaw7ehWr/EhFlEqk9vtV6ch36nLAujbFUoCmt7AzREoah8
1tcYDlHgU4oT95mvP8JLiEGkhxeJ8THOxSp2QUgP6oEZ72XdHgr7YC/oEPHjvAaG
3CeaYHvetKWxvK/Pzcmm6P5hkbv0GfiXBGR3MdEk5NAyug7JLzfajfxeamxayt4Q
0h2gSak05JHVt/H4l5jNrSpuqwmgriSxn0hZM/ZUwwGC9zS3XR0MQmzV8TY2jJs=
=Xs7s
-----END PGP SIGNATURE-----

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:20 AM, Alan Stern wrote:
> > This raises two questions:
> > 
> > Should the host's resume be allowed to complete while the host is
> > still in error recovery?  Wouldn't it be better to wait until the
> > recovery is finished?
> 
> Ahh, right, my other patch changed the ata_port stuff to schedule the
> eh, and return  from resume, so that IT doesn't block the system
> resume path for a long period of time while IT tries to spin up the
> disk.  Both the libata and sd layers were spinning up the disk.
> 
> Still, the sd request should just wait in the queue until after the
> recovery completes shouldn't it?

Yes.

> > How does the queue end up in a stopped state?  (The stopped queue
> > explains why blk_peek_request returns NULL.)
> 
> I was guessing that was the cause from reading the code, after
> stepping through it with gdb and qemu, it turned out it was in
> SHOST_RECOVERY rather that stopped.  I suppose it may be stopped as
> well, so I guess I'll have to go do another debug session and make
> sure, but I think that was a red herring.
> 
> The flow seems to simply be:
> 
> host is in recovery
> sd_resume queues REQUEST SENSE
> scsi_request_fn notices host is in recovery and requeus REQUEST SENSE
> recovery completes
> REQUEST SENSE seems to be lost...

If the earlier patch did fix the problem, then the REQUEST SENSE req
was in the queue but wasn't being returned by blk_peek_request because
the queue was stopped.

Alan Stern

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

[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

[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

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

[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

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Douglas Gilbert]

On 14-01-07 10:40 AM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 1/7/2014 10:20 AM, Alan Stern wrote:
>> This raises two questions:
>>
>> Should the host's resume be allowed to complete while the host is
>> still in error recovery?  Wouldn't it be better to wait until the
>> recovery is finished?
>
> Ahh, right, my other patch changed the ata_port stuff to schedule the
> eh, and return  from resume, so that IT doesn't block the system
> resume path for a long period of time while IT tries to spin up the
> disk.  Both the libata and sd layers were spinning up the disk.
>
> Still, the sd request should just wait in the queue until after the
> recovery completes shouldn't it?
>
>> How does the queue end up in a stopped state?  (The stopped queue
>> explains why blk_peek_request returns NULL.)
>
> I was guessing that was the cause from reading the code, after
> stepping through it with gdb and qemu, it turned out it was in
> SHOST_RECOVERY rather that stopped.  I suppose it may be stopped as
> well, so I guess I'll have to go do another debug session and make
> sure, but I think that was a red herring.
>
> The flow seems to simply be:
>
> host is in recovery
> sd_resume queues REQUEST SENSE

You need to be careful with the SCSI REQUEST SENSE command
when used for getting power management information from
SCSI devices (mainly disks and tape drives).

Many moons ago REQUEST SENSE was used to explicitly fetch
sense data when the previous SCSI command failed and only
yielded a single byte status: CHECK CONDITION.

All modern SCSI transports have "autosense" so the original
semantics of REQUEST SENSE became pointless. So some crafty
folks at T10 decided to re-purpose REQUEST SENSE into what
it is today. That transition has been going on for around
10 years (and may not have finished).

When REQUEST SENSE had its original semantics, TEST UNIT
READY was the only game in town for monitoring power
management. From my reading of spc4r36n.pdf section  5.1.2
"Important commands for all SCSI device servers" TEST UNIT
READY should be called before REQUEST SENSE during device
initialization (including resume). If the responses to TEST
UNIT READY and REQUEST SENSE contradict then the former is
correct.

There are SCSI devices out caught in that transition. For
example with sg_format I monitor progress with either TEST
UNIT READY or REQUEST SENSE, defaulting to the former.
[Progress indication is another new role for REQUEST SENSE.]

Note that the billion or so USB keys out there that say that
they comply with SCSI-2 should respond to a REQUEST SENSE
with its original semantics.

The ATA REQUEST SENSE DATA EXT command refers to the SPC-4
"standard" (not yet it ain't) which is naturally the new
SCSI semantics of REQUEST SENSE. I believe the ATA REQUEST
SENSE DATA EXT command is a relatively new ATA command so
it does not carry the historical baggage of its SCSI
counterpart.


Just a little more hand waving for Phillip's amusement.

Doug Gilbert

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

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

On 1/9/2014 1:29 PM, Douglas Gilbert wrote:
> When REQUEST SENSE had its original semantics, TEST UNIT READY was
> the only game in town for monitoring power management. From my
> reading of spc4r36n.pdf section  5.1.2 "Important commands for all
> SCSI device servers" TEST UNIT READY should be called before
> REQUEST SENSE during device initialization (including resume). If
> the responses to TEST UNIT READY and REQUEST SENSE contradict then
> the former is correct.

I don't see language there remotely like that.  How did you arrive at
this conclusion?

> There are SCSI devices out caught in that transition. For example
> with sg_format I monitor progress with either TEST UNIT READY or
> REQUEST SENSE, defaulting to the former. [Progress indication is
> another new role for REQUEST SENSE.]
> 
> Note that the billion or so USB keys out there that say that they
> comply with SCSI-2 should respond to a REQUEST SENSE with its
> original semantics.

Then they will return 0/0/0, so we will assume they are up and
running, which is correct.  SPC-2 is coming up on 13 years old and
listed this usage for REQUEST SENSE.  Are there any actual disk drives
out there ( and still in use ) that return 0/0/0 when in the stopped
state?  I can't imagine there would be.

> The ATA REQUEST SENSE DATA EXT command refers to the SPC-4 
> "standard" (not yet it ain't) which is naturally the new SCSI
> semantics of REQUEST SENSE. I believe the ATA REQUEST SENSE DATA
> EXT command is a relatively new ATA command so it does not carry
> the historical baggage of its SCSI counterpart.

It is also only for ATAPI devices.


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

iQEcBAEBAgAGBQJSzvaMAAoJEI5FoCIzSKrwcxIH/3Y7DdnXr6Fj9MiWs/Hn2kte
EZVkHbj5jN4/OdKj3gnR3BK8gjrA7AEtbD3J6g7zJSj50UM1EA5RqMYAutZQkvYk
UsKNuV+qIcN3OpSltAGK5TQPGUo2M9apcYuy9NKTZEaWI9dXejGJ/2GitxYXO35H
drsTNlzEErnQvJwBhEYq6+rtferv7/vdZOljSGM1pP48gK0IWz+r2cwXxojkUvk1
lsiU97BxHCVNJc7kHmKVzBw0JV4R2mrtI/I/bE/VZzeljvjA9fNoXqdQxFU1RPDT
PNLHBecjgbDlc70l1CNKNc4ApqBDSYg9SwBtG1nCa36MLgLoN1ZDEatzqT9IdYw=
=qsZP
-----END PGP SIGNATURE-----

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Douglas Gilbert]

On 14-01-09 02:20 PM, Phillip Susi wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 1/9/2014 1:29 PM, Douglas Gilbert wrote:
>> When REQUEST SENSE had its original semantics, TEST UNIT READY was
>> the only game in town for monitoring power management. From my
>> reading of spc4r36n.pdf section  5.1.2 "Important commands for all
>> SCSI device servers" TEST UNIT READY should be called before
>> REQUEST SENSE during device initialization (including resume). If
>> the responses to TEST UNIT READY and REQUEST SENSE contradict then
>> the former is correct.
>
> I don't see language there remotely like that.  How did you arrive at
> this conclusion?

I'm wasting my time.

>> There are SCSI devices out caught in that transition. For example
>> with sg_format I monitor progress with either TEST UNIT READY or
>> REQUEST SENSE, defaulting to the former. [Progress indication is
>> another new role for REQUEST SENSE.]
>>
>> Note that the billion or so USB keys out there that say that they
>> comply with SCSI-2 should respond to a REQUEST SENSE with its
>> original semantics.
>
> Then they will return 0/0/0, so we will assume they are up and
> running, which is correct.  SPC-2 is coming up on 13 years old and
> listed this usage for REQUEST SENSE.  Are there any actual disk drives
> out there ( and still in use ) that return 0/0/0 when in the stopped
> state?  I can't imagine there would be.
>
>> The ATA REQUEST SENSE DATA EXT command refers to the SPC-4
>> "standard" (not yet it ain't) which is naturally the new SCSI
>> semantics of REQUEST SENSE. I believe the ATA REQUEST SENSE DATA
>> EXT command is a relatively new ATA command so it does not carry
>> the historical baggage of its SCSI counterpart.
>
> It is also only for ATAPI devices.

It is a command in the "Sense Data Reporting feature set"
which is optional for ATA devices and "P" as in
"Prohibited" for ATAPI devices. See the Feature set summary
table.

You had me fooled: I thought you had some knowledge of
the ATA command set. Thanks for proving me wrong.

EOF
Doug Gilbert

Re: REQ_PM vs REQ_TYPE_PM_RESUME

[Phillip Susi]

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

On 1/10/2014 12:23 AM, Douglas Gilbert wrote:
> It is a command in the "Sense Data Reporting feature set" which is
> optional for ATA devices and "P" as in "Prohibited" for ATAPI
> devices. See the Feature set summary table.

Oops, you're right.

> You had me fooled: I thought you had some knowledge of the ATA
> command set. Thanks for proving me wrong.

No need to be a complete dick.



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

iQEcBAEBAgAGBQJS0AOvAAoJEI5FoCIzSKrwxMkH/0cszltV/h/7tRXuCAJSp8EM
Z/BlrNLFUaKrTZMLt16bJBI+xhr2RbhBl9alyMWHoQI0NTEQyZ4RcClK+ByElzVh
aq5FI19an16RcYroTnU2EbJQ/bbH9P0gyz7JV/sNHAkMrM+pPZnQS/rx40fAYlnA
kWGgXESJMMr4oSGEwQku+1kUmh1SIYHyU2fa6domOlGoPdNQMt69HyiqBGyjEQ1/
Ms9hF7w2xW0bxN9fH68wu2YnQ9NIgZDYCXpmutxGzANNkGEsvs62u51X1d+CWAwB
8inOe7JTONk5r0g12XFjOnWvsnH0FplVI6fQlggchhuETP/WVXAXUY7shfeJ23w=
=6cKn
-----END PGP SIGNATURE-----

Re: [PATCH 3/6] libata: resume in the background

[Dan Williams]

On Mon, Dec 16, 2013 at 3:30 PM, Phillip Susi <psusi@ubuntu.com> wrote:
> Don't block the resume path waiting for the disk to
> spin up.
> ---
>  drivers/ata/libata-core.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 8f856bb..4a28caf 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5430,20 +5430,18 @@ static int __ata_port_resume_common(struct ata_port *ap, pm_message_t mesg,
>  static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
>  {
>         struct ata_port *ap = to_ata_port(dev);
> +       static int dontcare;
>
> -       return __ata_port_resume_common(ap, mesg, NULL);
> +       return __ata_port_resume_common(ap, mesg, &dontcare);
>  }

I was going to comment that this leaves us open to a race to submit
new i/o before recovery starts, but scsi_schedule_eh already blocks
new i/o, so I think we're good.  I think it deserves a comment here
about why it's safe to not care.

>
>  static int ata_port_resume(struct device *dev)
>  {
>         int rc;
>
> +       if (pm_runtime_suspended(dev))
> +               return 0;

Why?  If we dpm_resume() a port that was rpm_suspend()ed the state
needs to be reset to active.  I think this check also forces the port
to resume twice, once for system resume and again for the eventual
runtime_resume.

>         rc = ata_port_resume_common(dev, PMSG_RESUME);
> -       if (!rc) {
> -               pm_runtime_disable(dev);
> -               pm_runtime_set_active(dev);
> -               pm_runtime_enable(dev);
> -       }
>
>         return rc;
>  }

...so, the pm_runtime_suspended() check should go and something like
this folded in:

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 92d7797223be..a6cc107c9434 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -4143,5 +4143,11 @@ static void ata_eh_handle_port_resume(struct
ata_port *ap)
                ap->pm_result = NULL;
        }
        spin_unlock_irqrestore(ap->lock, flags);
+
+       if (rc == 0) {
+               pm_runtime_disable(dev);
+               pm_runtime_set_active(dev);
+               pm_runtime_enable(dev);
+       }
 }
 #endif /* CONFIG_PM */

Re: [PATCH 3/6] libata: resume in the background

[Phillip Susi]

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

On 01/10/2014 05:26 PM, Dan Williams wrote:
> I was going to comment that this leaves us open to a race to
> submit new i/o before recovery starts, but scsi_schedule_eh already
> blocks new i/o, so I think we're good.  I think it deserves a
> comment here about why it's safe to not care.

I don't follow, could you explain?

>> 
>> static int ata_port_resume(struct device *dev) { int rc;
>> 
>> +       if (pm_runtime_suspended(dev)) +               return 0;
> 
> Why?  If we dpm_resume() a port that was rpm_suspend()ed the state 
> needs to be reset to active.  I think this check also forces the
> port to resume twice, once for system resume and again for the
> eventual runtime_resume.

It stops the first resume by returning early, so only the second one
happens.

> ...so, the pm_runtime_suspended() check should go and something
> like this folded in:
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c 
> index 92d7797223be..a6cc107c9434 100644 ---
> a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -4143,5
> +4143,11 @@ static void ata_eh_handle_port_resume(struct ata_port
> *ap) ap->pm_result = NULL; } spin_unlock_irqrestore(ap->lock,
> flags); + +       if (rc == 0) { +
> pm_runtime_disable(dev); +
> pm_runtime_set_active(dev); +
> pm_runtime_enable(dev); +       } } #endif /* CONFIG_PM */

Ahh, and that will stop the second resume, rather than the previous
change to stop the first?

Don't we want to stop the first rather than the second?  Otherwise if
the port is runtime suspended at system suspend time ( maybe no drive
connected to it? ) then there is no sense in resuming it at system
resume time.


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

iQEcBAEBCgAGBQJS0KujAAoJEI5FoCIzSKrwhQsH/0f4AywqDM1y1BOPSFNuiF/X
5SRz1BaXCs7AZhSBcBrS5H9lZJLUQoKshUFXHtV9A4DuRhIghl+cu7JsJt3aPlmM
u2yx3xETTj/iLqnL4shHVta/y9gXCfXhO7qZNtcDyPmiSgPNlFM4ZFTnnXtKaVgj
PxZPzLqLA+Oh/iTgOXLsC/CECrLdWm6paTE6ii04QiabUjbK9vKk7EqazRnrlnsC
I3MzulTu8jDLxMtwpdPPYUvi93BQr2P1Q11u/Rt8za+Y19h3UXpb2ATEiuHeYcNK
F/HpXR+zDDSKSDnE591pF5q6gsC1MHt4+Zq0g7ZfpparO+2TciQo0hLs09hVEhI=
=Cu86
-----END PGP SIGNATURE-----

Re: [PATCH 3/6] libata: resume in the background

[Dan Williams]

On Fri, Jan 10, 2014 at 6:25 PM, Phillip Susi <psusi@ubuntu.com> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 01/10/2014 05:26 PM, Dan Williams wrote:
>> I was going to comment that this leaves us open to a race to
>> submit new i/o before recovery starts, but scsi_schedule_eh already
>> blocks new i/o, so I think we're good.  I think it deserves a
>> comment here about why it's safe to not care.
>
> I don't follow, could you explain?

The question I have when reading "__ata_port_resume_common(ap, mesg,
&dontcare);" is what makes it safe for the port to not actually be
resumed upon return.  The answer I believe is that the host is
guaranteed to be in the SHOST_RECOVERY state upon return and no new
i/o submissions will occur until the error handler has a chance to
run.

>>> static int ata_port_resume(struct device *dev) { int rc;
>>>
>>> +       if (pm_runtime_suspended(dev)) +               return 0;
>>
>> Why?  If we dpm_resume() a port that was rpm_suspend()ed the state
>> needs to be reset to active.  I think this check also forces the
>> port to resume twice, once for system resume and again for the
>> eventual runtime_resume.
>
> It stops the first resume by returning early, so only the second one
> happens.

Got it, but it looks odd.

>
>> ...so, the pm_runtime_suspended() check should go and something
>> like this folded in:
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 92d7797223be..a6cc107c9434 100644 ---
>> a/drivers/ata/libata-eh.c +++ b/drivers/ata/libata-eh.c @@ -4143,5
>> +4143,11 @@ static void ata_eh_handle_port_resume(struct ata_port
>> *ap) ap->pm_result = NULL; } spin_unlock_irqrestore(ap->lock,
>> flags); + +       if (rc == 0) { +
>> pm_runtime_disable(dev); +
>> pm_runtime_set_active(dev); +
>> pm_runtime_enable(dev); +       } } #endif /* CONFIG_PM */
>
> Ahh, and that will stop the second resume, rather than the previous
> change to stop the first?
>
> Don't we want to stop the first rather than the second?  Otherwise if
> the port is runtime suspended at system suspend time ( maybe no drive
> connected to it? ) then there is no sense in resuming it at system
> resume time.

Hmm, if the drive disconnected during suspend I think we want to find
that out sooner rather than later.  The changelog should really read
"Don't block the resume path waiting for the disk re-establish its
link".

Given it's async do we gain that much by deferring a one time check of
the ports at resume? At the very least I think tying rpm_resume to
dpm_resume like that is an additional change to defer to a separate
patch.