[RFC PATCH v3 0/4]: block layer runtime pm

[RFC PATCH v3 0/4]: block layer runtime pm

[Lin Ming]

Hi,

In August 2010, Jens and Alan discussed about "Runtime PM and the block
layer". http://marc.info/?t=128259108400001&r=1&w=2

Here are the RFC v3 patches that try to implement the ideas discussed.
Welcome to give it a try.

git pull git://git.kernel.org/pub/scm/linux/kernel/git/mlin/linux.git block_pm

The test steps, for example

# ls -l /sys/block/sda
/sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/block/sda

# echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata1/power/control
# echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/control
# echo 10000 > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/autosuspend_delay_ms

Then you'll see sda is suspended after 10secs idle.

# cat /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/runtime_status
suspended

And if you do some IO, it will resume immediately.

v3:
- remove block layer suspend/resume callbacks
- add block layer runtime pm helper functions

v2:
- remove queue idle timer, use runtime pm core's auto suspend

Lin Ming (4):
      block: add a flag to identify PM request
      block: add runtime pm helpers
      block: implement runtime pm strategy
      [SCSI] sd: change to auto suspend mode

 block/blk-core.c           |   71 ++++++++++++++++++++++++++++++++++++++++++++
 block/elevator.c           |    9 +++++
 drivers/scsi/scsi_lib.c    |   25 +++++++++++++--
 drivers/scsi/scsi_pm.c     |   27 ++++++++++++----
 drivers/scsi/scsi_sysfs.c  |    2 +
 drivers/scsi/sd.c          |   19 ++++--------
 include/linux/blk_types.h  |    2 +
 include/linux/blkdev.h     |   13 ++++++++
 include/scsi/scsi_device.h |    4 ++
 9 files changed, 149 insertions(+), 23 deletions(-)

[RFC PATCH v3 1/4] block: add a flag to identify PM request

[Lin Ming]

Add a flag REQ_PM to identify the request is PM related.
As an example, modify scsi code to use this flag.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/scsi_lib.c    |   25 ++++++++++++++++++++++---
 drivers/scsi/sd.c          |    9 +++++----
 include/linux/blk_types.h  |    2 ++
 include/scsi/scsi_device.h |    4 ++++
 4 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5dfd749..db4a40d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -256,10 +256,10 @@ int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 EXPORT_SYMBOL(scsi_execute);
 
 
-int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
+static int __scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 		     int data_direction, void *buffer, unsigned bufflen,
 		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
-		     int *resid)
+		     int *resid, int flags)
 {
 	char *sense = NULL;
 	int result;
@@ -270,15 +270,34 @@ int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			return DRIVER_ERROR << 24;
 	}
 	result = scsi_execute(sdev, cmd, data_direction, buffer, bufflen,
-			      sense, timeout, retries, 0, resid);
+			      sense, timeout, retries, flags, resid);
 	if (sshdr)
 		scsi_normalize_sense(sense, SCSI_SENSE_BUFFERSIZE, sshdr);
 
 	kfree(sense);
 	return result;
 }
+
+int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
+		     int data_direction, void *buffer, unsigned bufflen,
+		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
+		     int *resid)
+{
+	return __scsi_execute_req(sdev, cmd, data_direction, buffer, bufflen,
+			      sshdr, timeout, retries, resid, 0);
+}
 EXPORT_SYMBOL(scsi_execute_req);
 
+int scsi_execute_req_flag(struct scsi_device *sdev, const unsigned char *cmd,
+		     int data_direction, void *buffer, unsigned bufflen,
+		     struct scsi_sense_hdr *sshdr, int timeout, int retries,
+		     int *resid, int flags)
+{
+	return __scsi_execute_req(sdev, cmd, data_direction, buffer, bufflen,
+			      sshdr, timeout, retries, resid, flags);
+}
+EXPORT_SYMBOL(scsi_execute_req_flag);
+
 /*
  * Function:    scsi_init_cmd_errh()
  *
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5ba5c2a..1205a8b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1269,8 +1269,9 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 		 * Leave the rest of the command zero to indicate
 		 * flush everything.
 		 */
-		res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL);
+		res = scsi_execute_req_flag(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
+				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL,
+				       REQ_PM);
 		if (res == 0)
 			break;
 	}
@@ -2812,8 +2813,8 @@ static int sd_start_stop_device(struct scsi_disk *sdkp, int start)
 	if (!scsi_device_online(sdp))
 		return -ENODEV;
 
-	res = scsi_execute_req(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
-			       SD_TIMEOUT, SD_MAX_RETRIES, NULL);
+	res = scsi_execute_req_flag(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
+			       SD_TIMEOUT, SD_MAX_RETRIES, NULL, REQ_PM);
 	if (res) {
 		sd_printk(KERN_WARNING, sdkp, "START_STOP FAILED\n");
 		sd_print_result(sdkp, res);
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 4053cbd..a75f854 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -150,6 +150,7 @@ enum rq_flag_bits {
 	__REQ_FLUSH_SEQ,	/* request for flush sequence */
 	__REQ_IO_STAT,		/* account I/O stat */
 	__REQ_MIXED_MERGE,	/* merge of different types, fail separately */
+	__REQ_PM,		/* runtime pm request */
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -191,5 +192,6 @@ enum rq_flag_bits {
 #define REQ_IO_STAT		(1 << __REQ_IO_STAT)
 #define REQ_MIXED_MERGE		(1 << __REQ_MIXED_MERGE)
 #define REQ_SECURE		(1 << __REQ_SECURE)
+#define REQ_PM			(1 << __REQ_PM)
 
 #endif /* __LINUX_BLK_TYPES_H */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 6efb2e1..7735b46 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -387,6 +387,10 @@ extern int scsi_execute_req(struct scsi_device *sdev, const unsigned char *cmd,
 			    int data_direction, void *buffer, unsigned bufflen,
 			    struct scsi_sense_hdr *, int timeout, int retries,
 			    int *resid);
+extern int scsi_execute_req_flag(struct scsi_device *sdev, const unsigned char *cmd,
+			    int data_direction, void *buffer, unsigned bufflen,
+			    struct scsi_sense_hdr *, int timeout, int retries,
+			    int *resid, int flags);
 
 #ifdef CONFIG_PM_RUNTIME
 extern int scsi_autopm_get_device(struct scsi_device *);
-- 
1.7.2.5

[RFC PATCH v3 2/4] block: add runtime pm helpers

[Lin Ming]

Add runtime pm helper functions:

blk_pm_runtime_init()
blk_pre_runtime_suspend()
blk_post_runtime_suspend()
blk_pre_runtime_resume()
blk_post_runtime_suspend()

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 block/blk-core.c       |   61 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |   13 ++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1f61b74..2d8e38e 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -29,6 +29,7 @@
 #include <linux/fault-inject.h>
 #include <linux/list_sort.h>
 #include <linux/delay.h>
+#include <linux/pm_runtime.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/block.h>
@@ -2887,6 +2888,66 @@ void blk_finish_plug(struct blk_plug *plug)
 }
 EXPORT_SYMBOL(blk_finish_plug);
 
+void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
+{
+	q->dev = dev;
+	q->rpm_status = RPM_ACTIVE;
+
+	pm_runtime_use_autosuspend(q->dev);
+	pm_runtime_mark_last_busy(q->dev);
+	pm_runtime_autosuspend(q->dev);
+}
+EXPORT_SYMBOL(blk_pm_runtime_init);
+
+int blk_pre_runtime_suspend(struct request_queue *q)
+{
+	int ret = 0;
+
+	spin_lock_irq(q->queue_lock);
+	if (q->nr_pending)
+		ret = -EBUSY;
+	else
+		q->rpm_status = RPM_SUSPENDING;
+	spin_unlock_irq(q->queue_lock);
+	return ret;
+}
+EXPORT_SYMBOL(blk_pre_runtime_suspend);
+
+void blk_post_runtime_suspend(struct request_queue *q, int err)
+{
+	spin_lock_irq(q->queue_lock);
+	if (!err)
+		q->rpm_status = RPM_SUSPENDED;
+	else {
+		q->rpm_status = RPM_ACTIVE;
+		pm_runtime_mark_last_busy(q->dev);
+	}
+	spin_unlock_irq(q->queue_lock);
+}
+EXPORT_SYMBOL(blk_post_runtime_suspend);
+
+void blk_pre_runtime_resume(struct request_queue *q)
+{
+	spin_lock_irq(q->queue_lock);
+	q->rpm_status = RPM_RESUMING;
+	spin_unlock_irq(q->queue_lock);
+}
+EXPORT_SYMBOL(blk_pre_runtime_resume);
+
+void blk_post_runtime_resume(struct request_queue *q, int err)
+{
+	spin_lock_irq(q->queue_lock);
+	if (!err) {
+		q->rpm_status = RPM_ACTIVE;
+		__blk_run_queue(q);
+		pm_runtime_mark_last_busy(q->dev);
+		pm_request_autosuspend(q->dev);
+	} else
+		q->rpm_status = RPM_SUSPENDED;
+	spin_unlock_irq(q->queue_lock);
+}
+EXPORT_SYMBOL(blk_post_runtime_resume);
+
 int __init blk_dev_init(void)
 {
 	BUILD_BUG_ON(__REQ_NR_BITS > 8 *
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4d4ac24..cb0c4fc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -339,6 +339,9 @@ struct request_queue {
 	 */
 	struct kobject kobj;
 
+	struct device		*dev;
+	int			rpm_status;
+
 	/*
 	 * queue settings
 	 */
@@ -346,6 +349,7 @@ struct request_queue {
 	unsigned int		nr_congestion_on;
 	unsigned int		nr_congestion_off;
 	unsigned int		nr_batching;
+	unsigned int		nr_pending;
 
 	unsigned int		dma_drain_size;
 	void			*dma_drain_buffer;
@@ -875,6 +879,15 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);
 extern void blk_put_queue(struct request_queue *);
 
 /*
+ * block layer runtime pm functions
+ */
+extern void blk_pm_runtime_init(struct request_queue *q, struct device *dev);
+extern int blk_pre_runtime_suspend(struct request_queue *q);
+extern void blk_post_runtime_suspend(struct request_queue *q, int err);
+extern void blk_pre_runtime_resume(struct request_queue *q);
+extern void blk_post_runtime_resume(struct request_queue *q, int err);
+
+/*
  * blk_plug permits building a queue of related requests by holding the I/O
  * fragments for a short period. This allows merging of sequential requests
  * into single larger request. As the requests are moved from a per-task list to
-- 
1.7.2.5

[RFC PATCH v3 3/4] block: implement runtime pm strategy

[Lin Ming]

When a request is added:
    If device is suspended or is suspending and the request is not a
    PM request, resume the device.

When a request finishes:
    Call pm_runtime_mark_last_busy().

When pick a request:
    If device is not active, then only PM request is allowed to go.
    Return NULL for other request.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 block/blk-core.c |   10 ++++++++++
 block/elevator.c |    9 +++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2d8e38e..c3a5f3a 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1130,6 +1130,10 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 	if (unlikely(--req->ref_count))
 		return;
 
+	/* PM request is not accounted */
+	if (!(req->cmd_flags & REQ_PM) && !(--q->nr_pending) && q->dev)
+		pm_runtime_mark_last_busy(q->dev);
+
 	elv_completed_request(q, req);
 
 	/* this is a bio leak */
@@ -1918,6 +1922,12 @@ struct request *blk_peek_request(struct request_queue *q)
 	int ret;
 
 	while ((rq = __elv_next_request(q)) != NULL) {
+		/* Only PM request is allowed to go if the queue is suspended */
+		if (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)) {
+			rq = NULL;
+			break;
+		}
+
 		if (!(rq->cmd_flags & REQ_STARTED)) {
 			/*
 			 * This is the first time the device driver
diff --git a/block/elevator.c b/block/elevator.c
index f016855..06d83c5 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -34,6 +34,7 @@
 #include <linux/blktrace_api.h>
 #include <linux/hash.h>
 #include <linux/uaccess.h>
+#include <linux/pm_runtime.h>
 
 #include <trace/events/block.h>
 
@@ -546,6 +547,9 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
+	/* __elv_add_request will increment the count */
+	if (!(rq->cmd_flags & REQ_PM))
+		q->nr_pending--;
 	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
@@ -587,6 +591,11 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 {
 	trace_block_rq_insert(q, rq);
 
+	if (!(rq->cmd_flags & REQ_PM))
+		if (q->nr_pending++ == 0 && (q->rpm_status == RPM_SUSPENDED ||
+				q->rpm_status == RPM_SUSPENDING) && q->dev)
+			pm_request_resume(q->dev);
+
 	rq->q = q;
 
 	if (rq->cmd_flags & REQ_SOFTBARRIER) {
-- 
1.7.2.5

[RFC PATCH v3 4/4] [SCSI] sd: change to auto suspend mode

[Lin Ming]

Uses block layer runtime pm helper functions in scsi_runtime_suspend/resume.
Remove scsi_autopm_* from sd open/release/remove path.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 drivers/scsi/scsi_pm.c    |   27 ++++++++++++++++++++-------
 drivers/scsi/scsi_sysfs.c |    2 ++
 drivers/scsi/sd.c         |   10 +---------
 3 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index c467064..302a157 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -139,10 +139,16 @@ static int scsi_runtime_suspend(struct device *dev)
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
+		struct scsi_device *sdev = to_scsi_device(dev);
+		struct request_queue *q = sdev->request_queue;
+
+		err = blk_pre_runtime_suspend(q);
+		if (err)
+			return err;
+
 		err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
-		if (err == -EAGAIN)
-			pm_schedule_suspend(dev, jiffies_to_msecs(
-				round_jiffies_up_relative(HZ/10)));
+
+		blk_post_runtime_suspend(q, err);
 	}
 
 	/* Insert hooks here for targets, hosts, and transport classes */
@@ -155,8 +161,14 @@ static int scsi_runtime_resume(struct device *dev)
 	int err = 0;
 
 	dev_dbg(dev, "scsi_runtime_resume\n");
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev)) {
+		struct scsi_device *sdev = to_scsi_device(dev);
+		struct request_queue *q = sdev->request_queue;
+
+		blk_pre_runtime_resume(q);
 		err = scsi_dev_type_resume(dev);
+		blk_post_runtime_resume(q, err);
+	}
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
@@ -171,9 +183,10 @@ 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)) {
+		pm_runtime_mark_last_busy(dev);
+		err = pm_request_autosuspend(dev);
+	} else
 		err = pm_runtime_suspend(dev);
 	return err;
 }
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 04c2a27..1e8975f 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -895,6 +895,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
 	 */
 	scsi_autopm_get_device(sdev);
 
+	blk_pm_runtime_init(rq, &sdev->sdev_gendev);
+
 	error = device_add(&sdev->sdev_gendev);
 	if (error) {
 		sdev_printk(KERN_INFO, sdev,
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 1205a8b..09e5ffe 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -966,10 +966,6 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 
 	sdev = sdkp->device;
 
-	retval = scsi_autopm_get_device(sdev);
-	if (retval)
-		goto error_autopm;
-
 	/*
 	 * If the device is in error recovery, wait until it is done.
 	 * If the device is offline, then disallow any access to it.
@@ -1014,8 +1010,6 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
 	return 0;
 
 error_out:
-	scsi_autopm_put_device(sdev);
-error_autopm:
 	scsi_disk_put(sdkp);
 	return retval;	
 }
@@ -1050,7 +1044,6 @@ static int sd_release(struct gendisk *disk, fmode_t mode)
 	 * XXX is followed by a "rmmod sd_mod"?
 	 */
 
-	scsi_autopm_put_device(sdev);
 	scsi_disk_put(sdkp);
 	return 0;
 }
@@ -2631,7 +2624,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
 
 	sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
 		  sdp->removable ? "removable " : "");
-	scsi_autopm_put_device(sdp);
+	pm_runtime_put_sync_autosuspend(&sdp->sdev_gendev);
 	put_device(&sdkp->dev);
 }
 
@@ -2755,7 +2748,6 @@ static int sd_remove(struct device *dev)
 	struct scsi_disk *sdkp;
 
 	sdkp = dev_get_drvdata(dev);
-	scsi_autopm_get_device(sdkp->device);
 
 	async_synchronize_full();
 	blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
-- 
1.7.2.5

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Alan Stern]

On Tue, 22 May 2012, Lin Ming wrote:

> Hi,
> 
> In August 2010, Jens and Alan discussed about "Runtime PM and the block
> layer". http://marc.info/?t=128259108400001&r=1&w=2
> 
> Here are the RFC v3 patches that try to implement the ideas discussed.
> Welcome to give it a try.

Have you checked to make sure these changes won't break the IDE driver?  
Maybe it will need to set the REQ_PM flag too.

Alan Stern

Re: [RFC PATCH v3 3/4] block: implement runtime pm strategy

[Alan Stern]

On Tue, 22 May 2012, Lin Ming wrote:

> When a request is added:
>     If device is suspended or is suspending and the request is not a
>     PM request, resume the device.
> 
> When a request finishes:
>     Call pm_runtime_mark_last_busy().
> 
> When pick a request:
>     If device is not active, then only PM request is allowed to go.
>     Return NULL for other request.

You've got the right idea.  There are a few things that need fixing, in 
this patch and in 4/4.

> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1130,6 +1130,10 @@ void __blk_put_request(struct request_queue *q, struct request *req)
>  	if (unlikely(--req->ref_count))
>  		return;
>  
> +	/* PM request is not accounted */
> +	if (!(req->cmd_flags & REQ_PM) && !(--q->nr_pending) && q->dev)

I don't see that it makes any difference.  You might as well count PM 
requests along with the others, here and elsewhere.

> +		pm_runtime_mark_last_busy(q->dev);
> +
>  	elv_completed_request(q, req);
>  
>  	/* this is a bio leak */
> @@ -1918,6 +1922,12 @@ struct request *blk_peek_request(struct request_queue *q)
>  	int ret;
>  
>  	while ((rq = __elv_next_request(q)) != NULL) {
> +		/* Only PM request is allowed to go if the queue is suspended */
> +		if (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)) {
> +			rq = NULL;
> +			break;
> +		}

Not even PM requests should be allowed to go if the status is 
RPM_SUSPENDED.

Is this the only interface by which a client driver can get a request 
from the queue?

Alan Stern

Re: [RFC PATCH v3 3/4] block: implement runtime pm strategy

[Lin Ming]

On Wed, May 23, 2012 at 12:14 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 22 May 2012, Lin Ming wrote:
>
>> When a request is added:
>>     If device is suspended or is suspending and the request is not a
>>     PM request, resume the device.
>>
>> When a request finishes:
>>     Call pm_runtime_mark_last_busy().
>>
>> When pick a request:
>>     If device is not active, then only PM request is allowed to go.
>>     Return NULL for other request.
>
> You've got the right idea.  There are a few things that need fixing, in
> this patch and in 4/4.
>
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1130,6 +1130,10 @@ void __blk_put_request(struct request_queue *q, struct request *req)
>>       if (unlikely(--req->ref_count))
>>               return;
>>
>> +     /* PM request is not accounted */
>> +     if (!(req->cmd_flags & REQ_PM) && !(--q->nr_pending) && q->dev)
>
> I don't see that it makes any difference.  You might as well count PM
> requests along with the others, here and elsewhere.

Need to think about this.

>
>> +             pm_runtime_mark_last_busy(q->dev);
>> +
>>       elv_completed_request(q, req);
>>
>>       /* this is a bio leak */
>> @@ -1918,6 +1922,12 @@ struct request *blk_peek_request(struct request_queue *q)
>>       int ret;
>>
>>       while ((rq = __elv_next_request(q)) != NULL) {
>> +             /* Only PM request is allowed to go if the queue is suspended */
>> +             if (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)) {
>> +                     rq = NULL;
>> +                     break;
>> +             }
>
> Not even PM requests should be allowed to go if the status is
> RPM_SUSPENDED.

PM requests are used to wake up the device.
If they are not allowed to go, then how to wake up the device?

>
> Is this the only interface by which a client driver can get a request
> from the queue?

I searched __elv_next_request and it's only called in blk_peek_request.

Jens,
Could you confirm this?

Thanks,
Lin Ming

>
> Alan Stern

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Lin Ming]

On Wed, May 23, 2012 at 12:10 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 22 May 2012, Lin Ming wrote:
>
>> Hi,
>>
>> In August 2010, Jens and Alan discussed about "Runtime PM and the block
>> layer". http://marc.info/?t=128259108400001&r=1&w=2
>>
>> Here are the RFC v3 patches that try to implement the ideas discussed.
>> Welcome to give it a try.
>
> Have you checked to make sure these changes won't break the IDE driver?
> Maybe it will need to set the REQ_PM flag too.

Will check it.

Another thing need to check is if system suspend/resume works.

>
> Alan Stern

Re: [RFC PATCH v3 4/4] [SCSI] sd: change to auto suspend mode

[Lin Ming]

On Tue, May 22, 2012 at 3:21 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> Uses block layer runtime pm helper functions in scsi_runtime_suspend/resume.
> Remove scsi_autopm_* from sd open/release/remove path.

Hi Alan,

You mentioned that this patch needs some fixing.
Could you point that out?

Thanks,
Lin Ming

>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  drivers/scsi/scsi_pm.c    |   27 ++++++++++++++++++++-------
>  drivers/scsi/scsi_sysfs.c |    2 ++
>  drivers/scsi/sd.c         |   10 +---------
>  3 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index c467064..302a157 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -139,10 +139,16 @@ static int scsi_runtime_suspend(struct device *dev)
>
>        dev_dbg(dev, "scsi_runtime_suspend\n");
>        if (scsi_is_sdev_device(dev)) {
> +               struct scsi_device *sdev = to_scsi_device(dev);
> +               struct request_queue *q = sdev->request_queue;
> +
> +               err = blk_pre_runtime_suspend(q);
> +               if (err)
> +                       return err;
> +
>                err = scsi_dev_type_suspend(dev, PMSG_AUTO_SUSPEND);
> -               if (err == -EAGAIN)
> -                       pm_schedule_suspend(dev, jiffies_to_msecs(
> -                               round_jiffies_up_relative(HZ/10)));
> +
> +               blk_post_runtime_suspend(q, err);
>        }
>
>        /* Insert hooks here for targets, hosts, and transport classes */
> @@ -155,8 +161,14 @@ static int scsi_runtime_resume(struct device *dev)
>        int err = 0;
>
>        dev_dbg(dev, "scsi_runtime_resume\n");
> -       if (scsi_is_sdev_device(dev))
> +       if (scsi_is_sdev_device(dev)) {
> +               struct scsi_device *sdev = to_scsi_device(dev);
> +               struct request_queue *q = sdev->request_queue;
> +
> +               blk_pre_runtime_resume(q);
>                err = scsi_dev_type_resume(dev);
> +               blk_post_runtime_resume(q, err);
> +       }
>
>        /* Insert hooks here for targets, hosts, and transport classes */
>
> @@ -171,9 +183,10 @@ 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)) {
> +               pm_runtime_mark_last_busy(dev);
> +               err = pm_request_autosuspend(dev);
> +       } else
>                err = pm_runtime_suspend(dev);
>        return err;
>  }
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index 04c2a27..1e8975f 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -895,6 +895,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev)
>         */
>        scsi_autopm_get_device(sdev);
>
> +       blk_pm_runtime_init(rq, &sdev->sdev_gendev);
> +
>        error = device_add(&sdev->sdev_gendev);
>        if (error) {
>                sdev_printk(KERN_INFO, sdev,
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 1205a8b..09e5ffe 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -966,10 +966,6 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
>
>        sdev = sdkp->device;
>
> -       retval = scsi_autopm_get_device(sdev);
> -       if (retval)
> -               goto error_autopm;
> -
>        /*
>         * If the device is in error recovery, wait until it is done.
>         * If the device is offline, then disallow any access to it.
> @@ -1014,8 +1010,6 @@ static int sd_open(struct block_device *bdev, fmode_t mode)
>        return 0;
>
>  error_out:
> -       scsi_autopm_put_device(sdev);
> -error_autopm:
>        scsi_disk_put(sdkp);
>        return retval;
>  }
> @@ -1050,7 +1044,6 @@ static int sd_release(struct gendisk *disk, fmode_t mode)
>         * XXX is followed by a "rmmod sd_mod"?
>         */
>
> -       scsi_autopm_put_device(sdev);
>        scsi_disk_put(sdkp);
>        return 0;
>  }
> @@ -2631,7 +2624,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>
>        sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
>                  sdp->removable ? "removable " : "");
> -       scsi_autopm_put_device(sdp);
> +       pm_runtime_put_sync_autosuspend(&sdp->sdev_gendev);
>        put_device(&sdkp->dev);
>  }
>
> @@ -2755,7 +2748,6 @@ static int sd_remove(struct device *dev)
>        struct scsi_disk *sdkp;
>
>        sdkp = dev_get_drvdata(dev);
> -       scsi_autopm_get_device(sdkp->device);
>
>        async_synchronize_full();
>        blk_queue_prep_rq(sdkp->device->request_queue, scsi_prep_fn);
> --
> 1.7.2.5

Re: [RFC PATCH v3 4/4] [SCSI] sd: change to auto suspend mode

[Alan Stern]

On Tue, 22 May 2012, Lin Ming wrote:

> Uses block layer runtime pm helper functions in scsi_runtime_suspend/resume.
> Remove scsi_autopm_* from sd open/release/remove path.

Sorry I didn't have time to get to this yesterday...

> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c

> @@ -171,9 +183,10 @@ 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)) {
> +		pm_runtime_mark_last_busy(dev);
> +		err = pm_request_autosuspend(dev);

This really should be pm_runtime_autosuspend(dev).  In practice there's
very little difference; it's mostly a matter of style.

> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c

> @@ -2631,7 +2624,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>  
>  	sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
>  		  sdp->removable ? "removable " : "");
> -	scsi_autopm_put_device(sdp);
> +	pm_runtime_put_sync_autosuspend(&sdp->sdev_gendev);

This should be left the way it was.  scsi_autopm_put_device() does 
pm_runtime_put_sync(), which will call scsi_runtime_idle(), which will 
now call pm_runtime_autosuspend().

>  	put_device(&sdkp->dev);
>  }
>  
> @@ -2755,7 +2748,6 @@ static int sd_remove(struct device *dev)
>  	struct scsi_disk *sdkp;
>  
>  	sdkp = dev_get_drvdata(dev);
> -	scsi_autopm_get_device(sdkp->device);

This line should be kept as is.  The SCSI core uses the incremented 
usage count to prevent driverless devices from being runtime-suspended.

Alan Stern

Re: [RFC PATCH v3 3/4] block: implement runtime pm strategy

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 640 bytes --]

On Wed, 23 May 2012, Lin Ming wrote:

> >> +             /* Only PM request is allowed to go if the queue is suspended */
> >> +             if (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)) {
> >> +                     rq = NULL;
> >> +                     break;
> >> +             }
> >
> > Not even PM requests should be allowed to go if the status is
> > RPM_SUSPENDED.
> 
> PM requests are used to wake up the device.
> If they are not allowed to go, then how to wake up the device?

When blk_pre_runtime_resume runs, the status is changed to
RPM_RESUMING.  _Then_ PM requests are allowed to go.  Not before.

Alan Stern

Re: [RFC PATCH v3 1/4] block: add a flag to identify PM request

[Alan Stern]

On Tue, 22 May 2012, Lin Ming wrote:

> Add a flag REQ_PM to identify the request is PM related.
> As an example, modify scsi code to use this flag.

Don't forget to check up on the SCSI error handler.  If I'm not
mistaken, the libata drivers use it during suspend and resume.  Also,
the error handler will run if one of the REQ_PM commands encounters an
error.

Therefore it seems likely that every request submitted by the error 
handler needs to have REQ_PM set.

Alan Stern

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Alan Stern]

On Wed, 23 May 2012, Lin Ming wrote:

> Another thing need to check is if system suspend/resume works.

Right!  Although I would hope that your work doesn't interfere at all 
with system sleep, since it touches only the runtime PM pathways.

Alan Stern

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Lin Ming]

On Wed, May 23, 2012 at 11:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 23 May 2012, Lin Ming wrote:
>
>> Another thing need to check is if system suspend/resume works.
>
> Right!  Although I would hope that your work doesn't interfere at all
> with system sleep, since it touches only the runtime PM pathways.

sd_sync_cache and sd_start_stop_device are changed with REQ_PM set.
And they are called for system suspend too.

So it probably will have problem.

>
> Alan Stern

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Alan Stern]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 752 bytes --]

On Wed, 23 May 2012, Lin Ming wrote:

> On Wed, May 23, 2012 at 11:03 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, 23 May 2012, Lin Ming wrote:
> >
> >> Another thing need to check is if system suspend/resume works.
> >
> > Right!  Although I would hope that your work doesn't interfere at all
> > with system sleep, since it touches only the runtime PM pathways.
> 
> sd_sync_cache and sd_start_stop_device are changed with REQ_PM set.
> And they are called for system suspend too.
> 
> So it probably will have problem.

People have already run into problems where system suspend 
occurred at a time when a SCSI device was already runtime suspended.  
That's why scsi_bus_suspend_common() includes an appropriate check.

Alan Stern

Re: [RFC PATCH v3 1/4] block: add a flag to identify PM request

[Lin Ming]

On Wed, May 23, 2012 at 11:02 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 22 May 2012, Lin Ming wrote:
>
>> Add a flag REQ_PM to identify the request is PM related.
>> As an example, modify scsi code to use this flag.
>
> Don't forget to check up on the SCSI error handler.  If I'm not
> mistaken, the libata drivers use it during suspend and resume.  Also,

You are right.

ata_port_suspend_common
  ata_port_request_pm
    ata_port_schedule_eh
      scsi_schedule_eh
        scsi_error_handler ---> libata error handler

I just have a quick look and it seems libata error handler does not
send SCSI request.
Will check more.

> the error handler will run if one of the REQ_PM commands encounters an
> error.
>
> Therefore it seems likely that every request submitted by the error
> handler needs to have REQ_PM set.
>
> Alan Stern

Re: [RFC PATCH v3 0/4]: block layer runtime pm

[Lin Ming]

On Wed, May 23, 2012 at 4:29 PM, Lin Ming <ming.m.lin@intel.com> wrote:
> On Wed, May 23, 2012 at 12:10 AM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> On Tue, 22 May 2012, Lin Ming wrote:
>>
>>> Hi,
>>>
>>> In August 2010, Jens and Alan discussed about "Runtime PM and the block
>>> layer". http://marc.info/?t=128259108400001&r=1&w=2
>>>
>>> Here are the RFC v3 patches that try to implement the ideas discussed.
>>> Welcome to give it a try.
>>
>> Have you checked to make sure these changes won't break the IDE driver?
>> Maybe it will need to set the REQ_PM flag too.

REQ_PM flag is about runtime PM, but IDE driver does not support
runtime PM at all.

# grep runtime drivers/ide/*.c

This gets empty result.

So I think it won't break the IDE driver.

Thanks,
Lin Ming

>
> Will check it.
>
> Another thing need to check is if system suspend/resume works.
>
>>
>> Alan Stern

Re: [RFC PATCH v3 4/4] [SCSI] sd: change to auto suspend mode

[Lin Ming]

On Wed, May 23, 2012 at 10:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, 22 May 2012, Lin Ming wrote:
>
>> Uses block layer runtime pm helper functions in scsi_runtime_suspend/resume.
>> Remove scsi_autopm_* from sd open/release/remove path.
>
> Sorry I didn't have time to get to this yesterday...
>
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>
>> @@ -171,9 +183,10 @@ 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)) {
>> +             pm_runtime_mark_last_busy(dev);
>> +             err = pm_request_autosuspend(dev);
>
> This really should be pm_runtime_autosuspend(dev).  In practice there's
> very little difference; it's mostly a matter of style.
>
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>
>> @@ -2631,7 +2624,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
>>
>>       sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
>>                 sdp->removable ? "removable " : "");
>> -     scsi_autopm_put_device(sdp);
>> +     pm_runtime_put_sync_autosuspend(&sdp->sdev_gendev);
>
> This should be left the way it was.  scsi_autopm_put_device() does
> pm_runtime_put_sync(), which will call scsi_runtime_idle(), which will
> now call pm_runtime_autosuspend().
>
>>       put_device(&sdkp->dev);
>>  }
>>
>> @@ -2755,7 +2748,6 @@ static int sd_remove(struct device *dev)
>>       struct scsi_disk *sdkp;
>>
>>       sdkp = dev_get_drvdata(dev);
>> -     scsi_autopm_get_device(sdkp->device);
>
> This line should be kept as is.  The SCSI core uses the incremented
> usage count to prevent driverless devices from being runtime-suspended.

Will update.

Thanks a lot, Alan!

>
> Alan Stern