SATA disk resume time optimization

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

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

drivers/scsi/sd.c +3 -3

index e62d17d..3143311 100644
@@ -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");

include/scsi/scsi_device.h +1 -1

index d65fbec..1c46d2d 100644
@@ -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;

Quoted 4 lines · 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.

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

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

Quoted 6 lines · Douglas Gilbert

Quoted 4 lines · 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.

Quoted 2 lines

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.

Quoted 3 lines

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.

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.

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

Quoted 5 lines · James Bottomley

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.

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

Quoted 3 lines · James Bottomley

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.

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.

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

Quoted 5 lines · Phillip Susi

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.

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.

Quoted 3 lines · James Bottomley

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.

Quoted 4 lines · 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.

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

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

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

Quoted 6 lines · 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

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

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.

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(+)

drivers/ata/libata-core.c +0 -0

index 83b1a9f..686c441 100644

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

drivers/scsi/sd.c +0 -0

index e62d17d..3143311 100644

Quoted 3 lines

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

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

drivers/ata/libata-core.c +2 -1

index 686c441..128ce0d 100644
@@ -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)
  {

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

drivers/ata/libata-core.c +0 -0

index 8f856bb..4a28caf 100644

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.

Quoted 4 lines

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.

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

drivers/ata/libata-eh.c +0 -0

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

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

Quoted 4 lines · Dan Williams

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?

Quoted 8 lines

Quoted 3 lines

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.

Quoted 6 lines

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

drivers/ata/libata-eh.c +0 -0

index 92d7797223be..a6cc107c9434 100644 ---

Quoted 7 lines

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.

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

drivers/scsi/sd.c +5 -4

index e6c4bff..98c082a 100644
@@ -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);
 }

drivers/scsi/sd.c +5 -0

+	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) {

drivers/scsi/sd.c +25 -0

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

drivers/scsi/sd.c +2 -0

+
+
 /**
  *	init_sd - entry point for this driver (both when built in or when
  *	a module).

drivers/scsi/sd.h +1 -0

index 26895ff..0b8afb3 100644
@@ -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)
 

Review

Quoted 5 lines

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

(quoted diff) +0 -1

-	if (!sdkp->device->manage_start_stop) -		goto done; -

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

Quoted 1 line

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

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

drivers/scsi/sd.c static int sd_suspend_runtime(struct device *dev) · line 3183

Quoted 5 lines

ret = sd_start_stop_device(sdkp, 1);

drivers/scsi/sd.c +1 -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.

Quoted 7 lines

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

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?

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

Quoted 2 lines

(quoted diff) +1 -0

+	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)?

Quoted 3 lines

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

Quoted 4 lines · James Bottomley

Quoted 4 lines

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

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +0 -1

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

Quoted 3 lines

Quoted 1 line

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

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

Yes.

Quoted 9 lines

drivers/scsi/sd.c static int sd_suspend_runtime(struct device *dev) · line 3183

Quoted 5 lines

ret = sd_start_stop_device(sdkp, 1);

drivers/scsi/sd.c +1 -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.

Quoted 8 lines

Quoted 7 lines

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

Quoted 4 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 2 lines

(quoted diff) +1 -0

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

Quoted 4 lines

Quoted 3 lines

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

Quoted 3 lines

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.

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

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.

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.

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

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

Quoted 2 lines

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.

Quoted 4 lines

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.

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

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

drivers/ata/libata-scsi.c +0 -0

index f92eb21..8b17352 100644

drivers/ata/libata-scsi.c +6 -0

+	qc->scsidone(cmd);
+	ata_qc_free(qc);
+}
+
+/**
+ *	ata_scsi_request_sense_xlat - Simulate REQUEST SENSE command

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

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?

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

Quoted 5 lines · Phillip Susi

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:

drivers/scsi/scsi_lib.c +4 -2

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;
 

         /*

Quoted 10 lines · Phillip Susi

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

Quoted 5 lines · Phillip Susi

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?

Quoted 3 lines

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

Quoted 5 lines

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 -

block/blk-core.c +0 -0

  		return NULL;
  	else
  		return rq;

Quoted 20 lines

drivers/scsi/scsi_lib.c +4 -2

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;
 

         /*

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.

Quoted 9 lines

Quoted 5 lines

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

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?

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.

Quoted 3 lines · Alan Stern

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

Quoted 5 lines · Alan Stern

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?

Quoted 2 lines

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 SIGNED MESSAGE-----
Hash: SHA1

Quoted 5 lines · Alan Stern

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?

Quoted 2 lines

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

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.

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

Quoted 7 lines · Douglas Gilbert

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?

Quoted 8 lines

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.

Quoted 5 lines

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.

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

Quoted 4 lines · Aaron Lu

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.

Quoted 6 lines

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.

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

Quoted 3 lines · Aaron Lu

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

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?

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

On 01/07/2014 08:03 PM, Aaron Lu wrote:

Quoted 3 lines

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?

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

Quoted 6 lines · Aaron Lu

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.

Quoted 6 lines

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,

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

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

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

On 01/07/2014 09:11 PM, Aaron Lu wrote:

Quoted 2 lines

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.

Quoted 3 lines

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

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?

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.

Quoted 10 lines · Aaron Lu

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.

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

From: Aaron Lu <[email protected]>
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.

Quoted 5 lines · Alan Stern

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

Quoted 6 lines · Alan Stern

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.

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

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?

Quoted 4 lines · Alan Stern

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.

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

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.

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.

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.

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

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.

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:

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

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

Quoted 4 lines · Alan Stern

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

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.

Quoted 5 lines · Alan Stern

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.

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

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

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

Quoted 8 lines · Alan Stern

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

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

Quoted 3 lines

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

I'm a little unclear on the problem you are describing. Is it just with pm_runtime_in_use()?

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

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

Quoted 9 lines · Phillip Susi

Quoted 8 lines · Alan Stern

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

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

Quoted 6 lines

Quoted 3 lines

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

I'm a little unclear on the problem you are describing. Is it just with pm_runtime_in_use()?

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

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

Quoted 3 lines

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

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

Quoted 6 lines

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

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

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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

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

Quoted 15 lines · Alan Stern

Quoted 5 lines · Alan Stern

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

Quoted 2 lines

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

Rafael:

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

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

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

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

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

What do you think?

Quoted 42 lines · Alan Stern

Quoted 15 lines · Alan Stern

Quoted 5 lines · Alan Stern

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

Quoted 2 lines

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

Rafael:

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

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

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

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

Quoted 4 lines

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

What do you think?

There are two problems here.

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

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

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

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

Quoted 71 lines · Alan Stern

Quoted 44 lines · Alan Stern

Quoted 16 lines · Alan Stern

Quoted 7 lines

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

Quoted 2 lines

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

Rafael:

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

For example, we could do the pm_runtime_put() during dpm_resume(), or

even earlier. That way, the resume routine could do something like:

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

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

Quoted 4 lines

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

What do you think?

There are two problems here.

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

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

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

Quoted 3 lines

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

The situation isn't quite so simple.

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

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

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

Quoted 7 lines

The situation isn't quite so simple.

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

OK

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

Quoted 3 lines

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

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

Quoted 4 lines

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

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

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

bool leave_suspended = false; int rc = 0;

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

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

Quoted 11 lines · Alan Stern

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

bool leave_suspended = false; int rc = 0;

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

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

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

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

bool test_spinning = !pm_runtime_in_use(dev);

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

start_async_spin_up(dev, test_spinning); return 0;

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

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

Quoted 9 lines · Alan Stern

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

bool test_spinning = !pm_runtime_in_use(dev);

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

Quoted 4 lines

start_async_spin_up(dev, test_spinning); return 0;

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

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

Quoted 8 lines · Rafael J. Wysocki

Quoted 7 lines

The situation isn't quite so simple.

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

Quoted 3 lines

OK

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

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

Quoted 7 lines

Quoted 3 lines

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

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

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

	bool leave_suspended = false;
	int rc = 0;

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

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

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

Quoted 7 lines

Quoted 4 lines

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

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

No, currently it cannot be done.

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

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

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

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

Quoted 5 lines · Rafael J. Wysocki

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

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

Quoted 4 lines

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

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

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

.resume_early() runs with runtime PM disabled.

Quoted 14 lines · Alan Stern

Quoted 11 lines · Rafael J. Wysocki

Quoted 7 lines

The situation isn't quite so simple.

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

OK

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

Quoted 4 lines

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

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

Quoted 21 lines

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

	bool leave_suspended = false;
	int rc = 0;

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

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

Quoted 2 lines

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

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

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

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

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

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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

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

Quoted 6 lines

No, currently it cannot be done.

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

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

Quoted 20 lines · Rafael J. Wysocki

Quoted 18 lines · Alan Stern

Quoted 11 lines · Rafael J. Wysocki

Quoted 7 lines

The situation isn't quite so simple.

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

OK

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

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

Quoted 3 lines

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

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

Quoted 37 lines

Quoted 21 lines

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

	bool leave_suspended = false;
	int rc = 0;

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

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

Quoted 2 lines

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

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

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

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

Quoted 2 lines

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

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

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

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

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

Quoted 8 lines

Quoted 6 lines

No, currently it cannot be done.

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

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

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

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

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

Would that be acceptable?

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

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