Re: [PATCH 1/2] don't wait on disk to start on resume

Re: [PATCH 1/2] don't wait on disk to start on resume

[James Bottomley]

Missing cc to linux-scsi added 

On Thu, 2012-12-20 at 20:35 -0800, Derek Basehore wrote:
> We no longer wait for the disk to spin up in sd_resume. It now enters the
> request to spinup the disk into the elevator and returns.
> 
> A function is scheduled under the scsi_sd_probe_domain to wait for the command
> to spinup the disk to complete. This function then checks for errors and cleans
> up after the sd resume function.
> 
> This allows system resume to complete much faster on systems with an HDD since
> spinning up the disk is a significant portion of resume time.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/scsi/sd.c |  106 +++++++++++++++++++++++++++++++++++++++++++++++++---
>  drivers/scsi/sd.h |    9 ++++
>  2 files changed, 108 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 7992635..2fe051f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3045,6 +3045,7 @@ static void sd_shutdown(struct device *dev)
>  	if (!sdkp)
>  		return;         /* this can happen */
>  
> +	async_synchronize_full_domain(&scsi_sd_probe_domain);

What's the point of this?  We already have one in sd_remove() which is
the correct last callback.

>  	if (pm_runtime_suspended(dev))
>  		goto exit;
>  
> @@ -3070,6 +3071,7 @@ static int sd_suspend(struct device *dev)
>  	if (!sdkp)
>  		return 0;	/* this can happen */
>  
> +	async_synchronize_full_domain(&scsi_sd_probe_domain);
>  	if (sdkp->WCE) {
>  		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
>  		ret = sd_sync_cache(sdkp);
> @@ -3087,20 +3089,110 @@ done:
>  	return ret;
>  }
>  
> +/*
> + * Callback function for when the request that starts the disk completes. Passed
> + * into blk_execute_rq_nowait in sd_resume.
> + */
> +static void sd_end_start_rq(struct request *rq, int error)
> +{
> +	struct completion *waiting = rq->end_io_data;
> +
> +	rq->end_io_data = NULL;
> +	/*
> +	 * Set the end_io_data to NULL before calling complete. This (with
> +	 * sd_resume async) ensures that it is set to NULL before we call
> +	 * blk_put_request.
> +	 */
> +	complete(waiting);
> +}
> +
> +/*
> + * Asynchronous part of sd_resume. This is separate from sd_end_start_rq since
> + * we need to block on resume for suspend and shutdown. We already do this by
> + * synchronizing on the scsi_sd_probe_domain, so use that.
> + */
> +static void sd_resume_async(void *data, async_cookie_t cookie)
> +{
> +	struct scsi_sense_hdr sshdr;
> +	struct resume_async_struct *resume = data;
> +	struct scsi_disk *sdkp = resume->sdkp;
> +	struct request *req = resume->req;
> +
> +	wait_for_completion(&resume->wait);
> +
> +	if (req->errors) {
> +		scsi_normalize_sense(resume->sense,
> +				     SCSI_SENSE_BUFFERSIZE,
> +				     &sshdr);
> +		sd_printk(KERN_WARNING, sdkp, "START_STOP FAILED\n");
> +		sd_print_result(sdkp, req->errors);
> +		if (driver_byte(req->errors) & DRIVER_SENSE)
> +			sd_print_sense_hdr(sdkp, &sshdr);
> +	}
> +	kfree(resume);
> +	scsi_disk_put(sdkp);
> +	blk_put_request(req);
> +}
> +
> +/*
> + * This does not wait for the disk to spin up. This function enters the request
> + * to spinup the disk and schedules a function, sd_resume_async, that waits for
> + * that request to complete.
> + */
>  static int sd_resume(struct device *dev)
>  {
>  	struct scsi_disk *sdkp = scsi_disk_get_from_dev(dev);
> -	int ret = 0;
> +	struct scsi_device *sdp = sdkp->device;
> +	struct resume_async_struct *resume = NULL;
> +	struct request *req;
> +	unsigned char cmd[6] = { START_STOP };	/* START_VALID */
>  
> -	if (!sdkp->device->manage_start_stop)
> -		goto done;
> +	if (!sdkp->device->manage_start_stop) {
> +		scsi_disk_put(sdkp);
> +		return 0;
> +	}
>  
>  	sd_printk(KERN_NOTICE, sdkp, "Starting disk\n");
> -	ret = sd_start_stop_device(sdkp, 1);
>  
> -done:
> -	scsi_disk_put(sdkp);
> -	return ret;
> +	cmd[4] |= 1;	/* START */
> +
> +	if (sdp->start_stop_pwr_cond)
> +		cmd[4] |= 1 << 4;	/* Active */
> +
> +	if (!scsi_device_online(sdp))
> +		return -ENODEV;
> +
> +	resume = kzalloc(sizeof(struct resume_async_struct), GFP_NOIO);
> +	if (!resume)
> +		return DRIVER_ERROR << 24;
> +
> +	req = blk_get_request(sdp->request_queue, 0, __GFP_WAIT);
> +	if (!req)
> +		return DRIVER_ERROR << 24;
> +
> +	resume->req = req;
> +	resume->sdkp = sdkp;
> +	init_completion(&resume->wait);
> +
> +	req->cmd_len = COMMAND_SIZE(cmd[0]);
> +	memcpy(req->cmd, cmd, req->cmd_len);
> +	req->sense = resume->sense;
> +	req->sense_len = 0;
> +	req->retries = SD_MAX_RETRIES;
> +	req->timeout = SD_TIMEOUT;
> +	req->cmd_type = REQ_TYPE_BLOCK_PC;
> +	req->cmd_flags |= REQ_QUIET | REQ_PREEMPT;
> +	req->end_io_data = &resume->wait;
> +
> +	async_schedule_domain(sd_resume_async, resume, &scsi_sd_probe_domain);
> +	/*
> +	 * don't wait here for the disk to spin up, since we can resume faster
> +	 * if we don't. Cleanup and checking for errors is done the the
> +	 * previously scheduled sd_resume_async function
> +	 */
> +	blk_execute_rq_nowait(req->q, NULL, req, 1, sd_end_start_rq);

What is the point of open coding sd_start_stop_device() instead of just
calling it in the async function?

> +
> +	return 0;
>  }
>  
>  /**
> diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
> index 74a1e4c..b09f255 100644
> --- a/drivers/scsi/sd.h
> +++ b/drivers/scsi/sd.h
> @@ -1,6 +1,8 @@
>  #ifndef _SCSI_DISK_H
>  #define _SCSI_DISK_H
>  
> +#include <scsi/scsi_cmnd.h>
> +
>  /*
>   * More than enough for everybody ;)  The huge number of majors
>   * is a leftover from 16bit dev_t days, we don't really need that
> @@ -87,6 +89,13 @@ struct scsi_disk {
>  };
>  #define to_scsi_disk(obj) container_of(obj,struct scsi_disk,dev)
>  
> +struct resume_async_struct {
> +	struct scsi_disk *sdkp;
> +	struct request *req;
> +	struct completion wait;
> +	char sense[SCSI_SENSE_BUFFERSIZE];
> +};

If you don't open code sd_start_stop_device() you don't need any of this
because you don't pass information around to async functions.

James

>  static inline struct scsi_disk *scsi_disk(struct gendisk *disk)
>  {
>  	return container_of(disk->private_data, struct scsi_disk, driver);

Re: [PATCH 1/2] don't wait on disk to start on resume

[dbasehore .]

(Resending message as plain text so it's tracked through vger lists)

I didn't notice the other call to async_synchronize_full, I'll remove that one.

I split up the functionality of sd_start_stop_device to ensure that
the command to start the disk was sent before resume was completed.
For now, it's open coded, but that could be changed if I make
additions are made to scsi_lib.c to allow us to return immediately
after scheduling the command.

Re: [PATCH 1/2] don't wait on disk to start on resume

[James Bottomley]

On Thu, 2013-01-31 at 14:02 -0800, dbasehore . wrote:
> (Resending message as plain text so it's tracked through vger lists)

Just FYI ... I don't get messages sent to me personally and the list, so
if it wasn't on vger, I haven't seen it.

> I didn't notice the other call to async_synchronize_full, I'll remove that one.
> 
> I split up the functionality of sd_start_stop_device to ensure that
> the command to start the disk was sent before resume was completed.
> For now, it's open coded, but that could be changed if I make
> additions are made to scsi_lib.c to allow us to return immediately
> after scheduling the command.

That would be better; I'll wait to see the proposed patch.

James

Re: [PATCH 1/2] don't wait on disk to start on resume

[dbasehore .]

(Resending as plain text so the message is tracked on vger)

Hi, thanks for reading through my patch.

With regards to SCSI_SENSE_BUFFERSIZE, I'm following the precedent of
scsi_execute_req found in drivers/scsi/scsi_lib.c

It seems that it is used by the scsi_normalize_sense function which I
call in sd_resume_async. I just input SCSI_SENSE_BUFFERSIZE directly
there though. I didn't know if anything would change its behavior on a
lower level if I made sense_len = SCSI_SENSE_BUFFERSIZE, so I just
went with what was already done.

I'll make sure the semantic fixes go into the final patch.

Also, I forgot to mention one possible problem earlier. I understand
that some hard drives have a command buffer that can be executed by
the hard drive in an order it determines. Does anyone know of a
potential problem if the following happens?

-resume finishes (hard drive not started yet)
-read/write sent to disk, inserted before start command

Could this happen? If so, could it cause any problems?

I've tested the possibility of a program trying to read/write from the
disk before it has started, and the read/write blocks until the disk
has actually been spun up. I don't know if there are specific hard
drives where this could be a problem though.

Re: [PATCH 1/2] don't wait on disk to start on resume

[James Bottomley]

On Thu, 2013-01-31 at 14:00 -0800, dbasehore . wrote:
> (Resending as plain text so the message is tracked on vger)
> 
> Hi, thanks for reading through my patch.
> 
> With regards to SCSI_SENSE_BUFFERSIZE, I'm following the precedent of
> scsi_execute_req found in drivers/scsi/scsi_lib.c

I have no context for this: I don't recall making any comment on sense
buffers; what are you replying to?

James

> It seems that it is used by the scsi_normalize_sense function which I
> call in sd_resume_async. I just input SCSI_SENSE_BUFFERSIZE directly
> there though. I didn't know if anything would change its behavior on a
> lower level if I made sense_len = SCSI_SENSE_BUFFERSIZE, so I just
> went with what was already done.
> 
> I'll make sure the semantic fixes go into the final patch.
> 
> Also, I forgot to mention one possible problem earlier. I understand
> that some hard drives have a command buffer that can be executed by
> the hard drive in an order it determines. Does anyone know of a
> potential problem if the following happens?
> 
> -resume finishes (hard drive not started yet)
> -read/write sent to disk, inserted before start command
> 
> Could this happen? If so, could it cause any problems?
> 
> I've tested the possibility of a program trying to read/write from the
> disk before it has started, and the read/write blocks until the disk
> has actually been spun up. I don't know if there are specific hard
> drives where this could be a problem though.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] don't wait on disk to start on resume

[dbasehore .]

That was a response to Sergei's message (which is properly tracked on
the lists).

On Thu, Jan 31, 2013 at 2:27 PM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Thu, 2013-01-31 at 14:00 -0800, dbasehore . wrote:
>> (Resending as plain text so the message is tracked on vger)
>>
>> Hi, thanks for reading through my patch.
>>
>> With regards to SCSI_SENSE_BUFFERSIZE, I'm following the precedent of
>> scsi_execute_req found in drivers/scsi/scsi_lib.c
>
> I have no context for this: I don't recall making any comment on sense
> buffers; what are you replying to?
>
> James
>
>> It seems that it is used by the scsi_normalize_sense function which I
>> call in sd_resume_async. I just input SCSI_SENSE_BUFFERSIZE directly
>> there though. I didn't know if anything would change its behavior on a
>> lower level if I made sense_len = SCSI_SENSE_BUFFERSIZE, so I just
>> went with what was already done.
>>
>> I'll make sure the semantic fixes go into the final patch.
>>
>> Also, I forgot to mention one possible problem earlier. I understand
>> that some hard drives have a command buffer that can be executed by
>> the hard drive in an order it determines. Does anyone know of a
>> potential problem if the following happens?
>>
>> -resume finishes (hard drive not started yet)
>> -read/write sent to disk, inserted before start command
>>
>> Could this happen? If so, could it cause any problems?
>>
>> I've tested the possibility of a program trying to read/write from the
>> disk before it has started, and the read/write blocks until the disk
>> has actually been spun up. I don't know if there are specific hard
>> drives where this could be a problem though.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>

Re: [PATCH 1/2] don't wait on disk to start on resume

[Aaron Lu]

On 02/01/2013 11:28 PM, Alan Stern wrote:
> On Fri, 1 Feb 2013, Aaron Lu wrote:
> 
>> Hi Derek,
>>
>> On 12/21/2012 12:35 PM, Derek Basehore wrote:
>>> We no longer wait for the disk to spin up in sd_resume. It now enters the
>>> request to spinup the disk into the elevator and returns.
>>>
>>> A function is scheduled under the scsi_sd_probe_domain to wait for the command
>>> to spinup the disk to complete. This function then checks for errors and cleans
>>> up after the sd resume function.
>>>
>>> This allows system resume to complete much faster on systems with an HDD since
>>> spinning up the disk is a significant portion of resume time.
>>
>> An alternative way of possibly solving this problem from PM's point of
>> view might be:
>> 1 Set both ata port and scsi device's runtime status to RPM_SUSPENDED
>>   in their system suspend callback;
>> 2 On resume, do nothing in their system resume callback;
>> 3 With request based runtime PM introduced here:
>>   http://thread.gmane.org/gmane.linux.power-management.general/30405
>>   When a request comes for the HDD after system resume, both the ata
>>   port and the scsi device will be runtime resumed, which involves
>>   re-initialize the ata link(in ata port's runtime resume callback) and
>>   spin up the HDD(in sd's runtime resume callback).
>>
>> To make HDD un-affetcted by runtime PM during normal use, a large enough
>> autosuspend_delay can be set for it.
>>
>> Just my 2 cents, I've not verified or tried in any way yet :-)
> 
> It's a good idea.  The problem is that it won't work if CONFIG_PM_SLEEP
> is enabled but CONFIG_PM_RUNTIME isn't.

Right, what a pity... thanks for the hint.

But the code is really simple if we ignore this problem(with some proper
modifications to scsi bus PM callbacks, see the delayed_resume branch if
you are interested), so I'm tempted to post it here :-)

 drivers/ata/libata-core.c | 22 +++++++++++-----------
 drivers/scsi/scsi_pm.c    | 14 +++++++++++---
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 497adea..38000fc 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5355,10 +5355,19 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
 
 static int ata_port_suspend(struct device *dev)
 {
+	int ret;
+
 	if (pm_runtime_suspended(dev))
 		return 0;
 
-	return ata_port_suspend_common(dev, PMSG_SUSPEND);
+	ret = ata_port_suspend_common(dev, PMSG_SUSPEND);
+	if (!ret) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return ret;
 }
 
 static int ata_port_do_freeze(struct device *dev)
@@ -5393,16 +5402,7 @@ static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
 
 static int ata_port_resume(struct device *dev)
 {
-	int rc;
-
-	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;
+	return 0;
 }
 
 /*
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d9956b6..d0b6997 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -127,13 +127,21 @@ static int scsi_bus_prepare(struct device *dev)
 static int scsi_bus_suspend(struct device *dev)
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
-	return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
+	int ret;
+
+	ret = scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
+	if (!ret) {
+		__pm_runtime_disable(dev, false);
+		pm_runtime_set_suspended(dev);
+		pm_runtime_enable(dev);
+	}
+
+	return ret;
 }
 
 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);
+	return 0;
 }
 
 static int scsi_bus_freeze(struct device *dev)
-- 
1.8.1


Below branch has all the code necessary to try this out:
https://github.com/aaronlu/linux.git delayed_resume

And you will have to enable runtime PM for both ata port and scsi device,
it works for HDD and normal ODD(ZPODD needs some extra work), tested on
my thinkpad R61i with a HDD and an ODD.

But yeah, as Alan has said, this can't be a general solution.
But someday it may be, when CONFIG_RUNTIME_PM and CONFIG_PM_SLEEP are
regarded as base functions of the kernel and always compiled in :-)

Sorry for the noise.

-Aaron

Re: [PATCH 1/2] don't wait on disk to start on resume

[Alan Stern]

On Sat, 2 Feb 2013, Aaron Lu wrote:

> >> An alternative way of possibly solving this problem from PM's point of
> >> view might be:
> >> 1 Set both ata port and scsi device's runtime status to RPM_SUSPENDED
> >>   in their system suspend callback;

By the way, what reason is there for doing this to the ATA port?  Does 
the port take a long time to resume, in the same way that a disk can 
take a few seconds to spin back up?

> >> 2 On resume, do nothing in their system resume callback;
> >> 3 With request based runtime PM introduced here:
> >>   http://thread.gmane.org/gmane.linux.power-management.general/30405
> >>   When a request comes for the HDD after system resume, both the ata
> >>   port and the scsi device will be runtime resumed, which involves
> >>   re-initialize the ata link(in ata port's runtime resume callback) and
> >>   spin up the HDD(in sd's runtime resume callback).
> >>
> >> To make HDD un-affetcted by runtime PM during normal use, a large enough
> >> autosuspend_delay can be set for it.
> >>
> >> Just my 2 cents, I've not verified or tried in any way yet :-)
> > 
> > It's a good idea.  The problem is that it won't work if CONFIG_PM_SLEEP
> > is enabled but CONFIG_PM_RUNTIME isn't.
> 
> Right, what a pity... thanks for the hint.
> 
> But the code is really simple if we ignore this problem(with some proper
> modifications to scsi bus PM callbacks, see the delayed_resume branch if
> you are interested), so I'm tempted to post it here :-)
> 
>  drivers/ata/libata-core.c | 22 +++++++++++-----------
>  drivers/scsi/scsi_pm.c    | 14 +++++++++++---
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 497adea..38000fc 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5355,10 +5355,19 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
>  
>  static int ata_port_suspend(struct device *dev)
>  {
> +	int ret;
> +
>  	if (pm_runtime_suspended(dev))
>  		return 0;
>  
> -	return ata_port_suspend_common(dev, PMSG_SUSPEND);
> +	ret = ata_port_suspend_common(dev, PMSG_SUSPEND);
> +	if (!ret) {
> +		__pm_runtime_disable(dev, false);

Don't you mean pm_runtime_disable(dev)?

> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +	}
> +
> +	return ret;
>  }
>  
>  static int ata_port_do_freeze(struct device *dev)
> @@ -5393,16 +5402,7 @@ static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
>  
>  static int ata_port_resume(struct device *dev)
>  {
> -	int rc;
> -
> -	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;
> +	return 0;
>  }
>  
>  /*
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index d9956b6..d0b6997 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -127,13 +127,21 @@ static int scsi_bus_prepare(struct device *dev)
>  static int scsi_bus_suspend(struct device *dev)
>  {
>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> -	return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
> +	int ret;
> +
> +	ret = scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
> +	if (!ret) {
> +		__pm_runtime_disable(dev, false);
> +		pm_runtime_set_suspended(dev);
> +		pm_runtime_enable(dev);
> +	}
> +
> +	return ret;
>  }
>  
>  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);
> +	return 0;
>  }

This doesn't look like it would work very well with something like a CD 
drive, which doesn't use block-layer runtime PM.  Is that what you 
meant when you talked about modifying the SCSI PM callbacks?

Alan Stern

Re: [PATCH 1/2] don't wait on disk to start on resume

[Aaron Lu]

On 02/02/2013 11:09 PM, Alan Stern wrote:
> On Sat, 2 Feb 2013, Aaron Lu wrote:
> 
>>>> An alternative way of possibly solving this problem from PM's point of
>>>> view might be:
>>>> 1 Set both ata port and scsi device's runtime status to RPM_SUSPENDED
>>>>   in their system suspend callback;
> 
> By the way, what reason is there for doing this to the ATA port?  Does 
> the port take a long time to resume, in the same way that a disk can 
> take a few seconds to spin back up?

For SATA controllers that is in AHCI programming interface, the hard
drive will be spined up when the link is put to active state, so the
most time consuming part is in ata, not in scsi, as the below data
showed on my computer(hard disk is a HDD attached to a sata controller
in AHCI mode):
The ata port resume callback takes several seconds(2s-5s) to finish,
while sd_resume takes only 17ms...

I'm not sure about other programming interfaces.

> 
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index 497adea..38000fc 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -5355,10 +5355,19 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
>>  
>>  static int ata_port_suspend(struct device *dev)
>>  {
>> +	int ret;
>> +
>>  	if (pm_runtime_suspended(dev))
>>  		return 0;
>>  
>> -	return ata_port_suspend_common(dev, PMSG_SUSPEND);
>> +	ret = ata_port_suspend_common(dev, PMSG_SUSPEND);
>> +	if (!ret) {
>> +		__pm_runtime_disable(dev, false);
> 
> Don't you mean pm_runtime_disable(dev)?

I don't think it is necessary to check_resume here, no?

> 
>> +		pm_runtime_set_suspended(dev);
>> +		pm_runtime_enable(dev);
>> +	}
>> +
>> +	return ret;
>>  }
>>  
>>  static int ata_port_do_freeze(struct device *dev)
>> @@ -5393,16 +5402,7 @@ static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
>>  
>>  static int ata_port_resume(struct device *dev)
>>  {
>> -	int rc;
>> -
>> -	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;
>> +	return 0;
>>  }
>>  
>>  /*
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index d9956b6..d0b6997 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -127,13 +127,21 @@ static int scsi_bus_prepare(struct device *dev)
>>  static int scsi_bus_suspend(struct device *dev)
>>  {
>>  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> -	return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
>> +	int ret;
>> +
>> +	ret = scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
>> +	if (!ret) {
>> +		__pm_runtime_disable(dev, false);
>> +		pm_runtime_set_suspended(dev);
>> +		pm_runtime_enable(dev);
>> +	}
>> +
>> +	return ret;
>>  }
>>  
>>  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);
>> +	return 0;
>>  }
> 
> This doesn't look like it would work very well with something like a CD 
> drive, which doesn't use block-layer runtime PM.

No problem, we have the in-kernel-event-poll to resume the CD.
And actually, during resume, some udisk program will also open the block
device to find something out, which will also resume the CD.

> Is that what you meant when you talked about modifying the SCSI PM
> callbacks?

No, the modification is actually for disk.
With v8 of block layer runtime PM, it is no longer the case runtime
suspend is the same as system suspend for hard disk that utilize block
layer runtime PM: we quiesce the device and run its suspend callback for
the device during system suspend but we didn't touch the queue's
rpm_status as we do in runtime_suspend callback. So I did some
modifications to scsi_pm.c to make runtime suspend and system suspend do
exactly the same thing for disk type scsi device, no matter if they are
using block layer runtime PM or not.

Probably I had better post code here, this is a replacement for the
patch 4 of v8 block layer runtime PM patchset(I omit the sd part, since
it is irrevelant), please kindly review, see if you like it :-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 8f6b12c..d9956b6 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -16,17 +16,44 @@
 
 #include "scsi_priv.h"
 
+static int sdev_blk_suspend(struct scsi_device *sdev)
+{
+	int err;
+
+	err = blk_pre_runtime_suspend(sdev->request_queue);
+	if (err)
+		return err;
+	err = pm_generic_runtime_suspend(&sdev->sdev_gendev);
+	blk_post_runtime_suspend(sdev->request_queue, err);
+
+	return err;
+}
+
+static int sdev_blk_resume(struct scsi_device *sdev)
+{
+	int err;
+
+	blk_pre_runtime_resume(sdev->request_queue);
+	err = pm_generic_runtime_resume(&sdev->sdev_gendev);
+	blk_post_runtime_resume(sdev->request_queue, err);
+
+	return err;
+}
+
 static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
 	int err;
 
-	err = scsi_device_quiesce(to_scsi_device(dev));
+	err = scsi_device_quiesce(sdev);
 	if (err == 0) {
-		if (cb) {
+		if (sdev->request_queue->dev)
+			err = sdev_blk_suspend(sdev);
+		else if (cb)
 			err = cb(dev);
-			if (err)
-				scsi_device_resume(to_scsi_device(dev));
-		}
+
+		if (err)
+			scsi_device_resume(sdev);
 	}
 	dev_dbg(dev, "scsi suspend: %d\n", err);
 	return err;
@@ -34,11 +61,14 @@ static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
 
 static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
 {
+	struct scsi_device *sdev = to_scsi_device(dev);
 	int err = 0;
 
-	if (cb)
+	if (sdev->request_queue->dev)
+		err = sdev_blk_resume(sdev);
+	else if (cb)
 		err = cb(dev);
-	scsi_device_resume(to_scsi_device(dev));
+	scsi_device_resume(sdev);
 	dev_dbg(dev, "scsi resume: %d\n", err);
 	return err;
 }
@@ -185,10 +215,18 @@ static int scsi_runtime_idle(struct device *dev)
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
-	if (scsi_is_sdev_device(dev))
-		err = pm_schedule_suspend(dev, 100);
-	else
+	if (scsi_is_sdev_device(dev)) {
+		struct scsi_device *sdev = to_scsi_device(dev);
+
+		if (sdev->request_queue->dev) {
+			pm_runtime_mark_last_busy(dev);
+			err = pm_runtime_autosuspend(dev);
+		} else {
+			err = pm_schedule_suspend(dev, 100);
+		}
+	} else {
 		err = pm_runtime_suspend(dev);
+	}
 	return err;
 }
 
-- 
1.8.1

With this patch, the runtime suspend and system suspend for the device
is identical, so that we can safely return in system's suspend callback
when we found the device is already runtime suspended.

Thanks,
Aaron

Re: [PATCH 1/2] don't wait on disk to start on resume

[dbasehore .]

On the topic that we do a fast return for both scsi and ata. Now I
don't remember everything about this (and correct me if I'm wrong)
since I figured this out a few months ago.

There are some dependencies that scsi has on the resume path of ata. I
think it's that before we can send the command to spin up the disk, we
need to wait for the ata host controller to come up. As Aaron Lu
pointed out, it takes seconds for the ata port to resume. On the hand,
the resume for sd needs to wait for this to complete, so even if we
return early for ata, but not the scsi disk, suddenly it will be the
scsi disk that takes 2-5 seconds to resume.



On Sat, Feb 2, 2013 at 10:23 PM, Aaron Lu <aaron.lu@intel.com> wrote:
> On 02/02/2013 11:09 PM, Alan Stern wrote:
>> On Sat, 2 Feb 2013, Aaron Lu wrote:
>>
>>>>> An alternative way of possibly solving this problem from PM's point of
>>>>> view might be:
>>>>> 1 Set both ata port and scsi device's runtime status to RPM_SUSPENDED
>>>>>   in their system suspend callback;
>>
>> By the way, what reason is there for doing this to the ATA port?  Does
>> the port take a long time to resume, in the same way that a disk can
>> take a few seconds to spin back up?
>
> For SATA controllers that is in AHCI programming interface, the hard
> drive will be spined up when the link is put to active state, so the
> most time consuming part is in ata, not in scsi, as the below data
> showed on my computer(hard disk is a HDD attached to a sata controller
> in AHCI mode):
> The ata port resume callback takes several seconds(2s-5s) to finish,
> while sd_resume takes only 17ms...
>
> I'm not sure about other programming interfaces.
>
>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index 497adea..38000fc 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -5355,10 +5355,19 @@ static int ata_port_suspend_common(struct device *dev, pm_message_t mesg)
>>>
>>>  static int ata_port_suspend(struct device *dev)
>>>  {
>>> +    int ret;
>>> +
>>>      if (pm_runtime_suspended(dev))
>>>              return 0;
>>>
>>> -    return ata_port_suspend_common(dev, PMSG_SUSPEND);
>>> +    ret = ata_port_suspend_common(dev, PMSG_SUSPEND);
>>> +    if (!ret) {
>>> +            __pm_runtime_disable(dev, false);
>>
>> Don't you mean pm_runtime_disable(dev)?
>
> I don't think it is necessary to check_resume here, no?
>
>>
>>> +            pm_runtime_set_suspended(dev);
>>> +            pm_runtime_enable(dev);
>>> +    }
>>> +
>>> +    return ret;
>>>  }
>>>
>>>  static int ata_port_do_freeze(struct device *dev)
>>> @@ -5393,16 +5402,7 @@ static int ata_port_resume_common(struct device *dev, pm_message_t mesg)
>>>
>>>  static int ata_port_resume(struct device *dev)
>>>  {
>>> -    int rc;
>>> -
>>> -    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;
>>> +    return 0;
>>>  }
>>>
>>>  /*
>>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>>> index d9956b6..d0b6997 100644
>>> --- a/drivers/scsi/scsi_pm.c
>>> +++ b/drivers/scsi/scsi_pm.c
>>> @@ -127,13 +127,21 @@ static int scsi_bus_prepare(struct device *dev)
>>>  static int scsi_bus_suspend(struct device *dev)
>>>  {
>>>      const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>>> -    return scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
>>> +    int ret;
>>> +
>>> +    ret = scsi_bus_suspend_common(dev, pm ? pm->suspend : NULL);
>>> +    if (!ret) {
>>> +            __pm_runtime_disable(dev, false);
>>> +            pm_runtime_set_suspended(dev);
>>> +            pm_runtime_enable(dev);
>>> +    }
>>> +
>>> +    return ret;
>>>  }
>>>
>>>  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);
>>> +    return 0;
>>>  }
>>
>> This doesn't look like it would work very well with something like a CD
>> drive, which doesn't use block-layer runtime PM.
>
> No problem, we have the in-kernel-event-poll to resume the CD.
> And actually, during resume, some udisk program will also open the block
> device to find something out, which will also resume the CD.
>
>> Is that what you meant when you talked about modifying the SCSI PM
>> callbacks?
>
> No, the modification is actually for disk.
> With v8 of block layer runtime PM, it is no longer the case runtime
> suspend is the same as system suspend for hard disk that utilize block
> layer runtime PM: we quiesce the device and run its suspend callback for
> the device during system suspend but we didn't touch the queue's
> rpm_status as we do in runtime_suspend callback. So I did some
> modifications to scsi_pm.c to make runtime suspend and system suspend do
> exactly the same thing for disk type scsi device, no matter if they are
> using block layer runtime PM or not.
>
> Probably I had better post code here, this is a replacement for the
> patch 4 of v8 block layer runtime PM patchset(I omit the sd part, since
> it is irrevelant), please kindly review, see if you like it :-)
>
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 8f6b12c..d9956b6 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -16,17 +16,44 @@
>
>  #include "scsi_priv.h"
>
> +static int sdev_blk_suspend(struct scsi_device *sdev)
> +{
> +       int err;
> +
> +       err = blk_pre_runtime_suspend(sdev->request_queue);
> +       if (err)
> +               return err;
> +       err = pm_generic_runtime_suspend(&sdev->sdev_gendev);
> +       blk_post_runtime_suspend(sdev->request_queue, err);
> +
> +       return err;
> +}
> +
> +static int sdev_blk_resume(struct scsi_device *sdev)
> +{
> +       int err;
> +
> +       blk_pre_runtime_resume(sdev->request_queue);
> +       err = pm_generic_runtime_resume(&sdev->sdev_gendev);
> +       blk_post_runtime_resume(sdev->request_queue, err);
> +
> +       return err;
> +}
> +
>  static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
>  {
> +       struct scsi_device *sdev = to_scsi_device(dev);
>         int err;
>
> -       err = scsi_device_quiesce(to_scsi_device(dev));
> +       err = scsi_device_quiesce(sdev);
>         if (err == 0) {
> -               if (cb) {
> +               if (sdev->request_queue->dev)
> +                       err = sdev_blk_suspend(sdev);
> +               else if (cb)
>                         err = cb(dev);
> -                       if (err)
> -                               scsi_device_resume(to_scsi_device(dev));
> -               }
> +
> +               if (err)
> +                       scsi_device_resume(sdev);
>         }
>         dev_dbg(dev, "scsi suspend: %d\n", err);
>         return err;
> @@ -34,11 +61,14 @@ static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
>
>  static int scsi_dev_type_resume(struct device *dev, int (*cb)(struct device *))
>  {
> +       struct scsi_device *sdev = to_scsi_device(dev);
>         int err = 0;
>
> -       if (cb)
> +       if (sdev->request_queue->dev)
> +               err = sdev_blk_resume(sdev);
> +       else if (cb)
>                 err = cb(dev);
> -       scsi_device_resume(to_scsi_device(dev));
> +       scsi_device_resume(sdev);
>         dev_dbg(dev, "scsi resume: %d\n", err);
>         return err;
>  }
> @@ -185,10 +215,18 @@ static int scsi_runtime_idle(struct device *dev)
>
>         /* Insert hooks here for targets, hosts, and transport classes */
>
> -       if (scsi_is_sdev_device(dev))
> -               err = pm_schedule_suspend(dev, 100);
> -       else
> +       if (scsi_is_sdev_device(dev)) {
> +               struct scsi_device *sdev = to_scsi_device(dev);
> +
> +               if (sdev->request_queue->dev) {
> +                       pm_runtime_mark_last_busy(dev);
> +                       err = pm_runtime_autosuspend(dev);
> +               } else {
> +                       err = pm_schedule_suspend(dev, 100);
> +               }
> +       } else {
>                 err = pm_runtime_suspend(dev);
> +       }
>         return err;
>  }
>
> --
> 1.8.1
>
> With this patch, the runtime suspend and system suspend for the device
> is identical, so that we can safely return in system's suspend callback
> when we found the device is already runtime suspended.
>
> Thanks,
> Aaron
>

Re: [PATCH 1/2] don't wait on disk to start on resume

[Aaron Lu]

On 02/04/2013 08:07 AM, dbasehore . wrote:
> On the topic that we do a fast return for both scsi and ata. Now I
> don't remember everything about this (and correct me if I'm wrong)
> since I figured this out a few months ago.
>
> There are some dependencies that scsi has on the resume path of ata. I
> think it's that before we can send the command to spin up the disk, we
> need to wait for the ata host controller to come up. As Aaron Lu
> pointed out, it takes seconds for the ata port to resume. On the hand,

I just did some more recording, the result is:
host controller takes 1ms or less to resume;
port reset takes the most time, almost the same as the whole port resume
callback, 2-4 seconds;
sd resume callback takes 16ms.

So for ata disks, the most time consuming part is in port's reset
routine, which is executed in port's resume callback.

-Aaron

Re: [PATCH 1/2] don't wait on disk to start on resume

[Aaron Lu]

On 02/03/2013 02:23 PM, Aaron Lu wrote:
> No, the modification is actually for disk.
> With v8 of block layer runtime PM, it is no longer the case runtime
> suspend is the same as system suspend for hard disk that utilize block
> layer runtime PM: we quiesce the device and run its suspend callback for
> the device during system suspend but we didn't touch the queue's
> rpm_status as we do in runtime_suspend callback. So I did some
> modifications to scsi_pm.c to make runtime suspend and system suspend do
> exactly the same thing for disk type scsi device, no matter if they are
> using block layer runtime PM or not.
> 
> Probably I had better post code here, this is a replacement for the
> patch 4 of v8 block layer runtime PM patchset(I omit the sd part, since
> it is irrevelant), please kindly review, see if you like it :-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 8f6b12c..d9956b6 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -16,17 +16,44 @@
>  
>  #include "scsi_priv.h"
>  
> +static int sdev_blk_suspend(struct scsi_device *sdev)
> +{
> +	int err;
> +
> +	err = blk_pre_runtime_suspend(sdev->request_queue);
> +	if (err)
> +		return err;
> +	err = pm_generic_runtime_suspend(&sdev->sdev_gendev);
> +	blk_post_runtime_suspend(sdev->request_queue, err);
> +
> +	return err;
> +}
> +
>  static int scsi_dev_type_suspend(struct device *dev, int (*cb)(struct device *))
>  {
> +	struct scsi_device *sdev = to_scsi_device(dev);
>  	int err;
>  
> -	err = scsi_device_quiesce(to_scsi_device(dev));
> +	err = scsi_device_quiesce(sdev);
>  	if (err == 0) {
> -		if (cb) {
> +		if (sdev->request_queue->dev)
> +			err = sdev_blk_suspend(sdev);
> +		else if (cb)
>  			err = cb(dev);
> -			if (err)
> -				scsi_device_resume(to_scsi_device(dev));
> -		}
> +
> +		if (err)
> +			scsi_device_resume(sdev);
>  	}
>  	dev_dbg(dev, "scsi suspend: %d\n", err);
>  	return err;

I just realized that this will not work, as we can't request that there
is no request pending in the device's request queue when doing system
suspend. And if we can't request this, we will not be able to runtime
resume the device due to nr_pending may not be zero when new request
comes after system resumed, though this can be solved if we modify the
runtime resume policy.

During my test, I'm not doing any IO when suspend, so the nr_pending is
zero and it worked...

Please ignore my suggestion for this thread, sorry for the noise.

-Aaron