[PATCH v5 0/4] block layer runtime pm

[PATCH v5 0/4] block layer runtime pm

[Lin Ming]

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

v5:
- rename scsi_execute_req to scsi_execute_req_flags
  and wrap scsi_execute_req around it.
- add detail function descriptions in patch 2 log
- define static helper functions to do runtime pm work on block layer
  and put the definitions inside a #ifdef block

v4:
- add CONFIG_PM_RUNTIME check
- update queue runtime pm status after system resume
- use pm_runtime_autosuspend instead of pm_request_autosuspend in scsi_runtime_idle
- always count PM request

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           |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/elevator.c           |    4 ++++
 drivers/scsi/scsi_lib.c    |    9 ++++-----
 drivers/scsi/scsi_pm.c     |   38 ++++++++++++++++++++++++++++++--------
 drivers/scsi/scsi_sysfs.c  |    2 ++
 drivers/scsi/sd.c          |   16 +++++-----------
 include/linux/blk_types.h  |    2 ++
 include/linux/blkdev.h     |   65 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |   17 +++++++++++++----
 9 files changed, 194 insertions(+), 28 deletions(-)

[PATCH v5 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    |    9 ++++-----
 drivers/scsi/sd.c          |    9 +++++----
 include/linux/blk_types.h  |    2 ++
 include/scsi/scsi_device.h |   17 +++++++++++++----
 4 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6dfb978..98be3f1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -255,11 +255,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,
+int scsi_execute_req_flags(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,14 +269,14 @@ 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;
 }
-EXPORT_SYMBOL(scsi_execute_req);
+EXPORT_SYMBOL(scsi_execute_req_flags);
 
 /*
  * Function:    scsi_init_cmd_errh()
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6f72b80..ce03a0a 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1270,8 +1270,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_flags(sdp, cmd, DMA_NONE, NULL, 0, &sshdr,
+				       SD_FLUSH_TIMEOUT, SD_MAX_RETRIES, NULL,
+				       REQ_PM);
 		if (res == 0)
 			break;
 	}
@@ -2815,8 +2816,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_flags(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 0edb65d..ed53dca 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -160,6 +160,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 */
 };
 
@@ -201,5 +202,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 ba96988..ea6a97f 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -384,10 +384,19 @@ extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 			int data_direction, void *buffer, unsigned bufflen,
 			unsigned char *sense, int timeout, int retries,
 			int flag, int *resid);
-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_flags(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);
+
+static inline 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_flags(sdev, cmd, data_direction, buffer,
+		bufflen, sshdr, timeout, retries, resid, 0);
+}
 
 #ifdef CONFIG_PM_RUNTIME
 extern int scsi_autopm_get_device(struct scsi_device *);
-- 
1.7.10

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

[Lin Ming]

Add runtime pm helper functions:

void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
  - Initialization function for drivers to call. It calls
    pm_runtime_use_autosuspend(), pm_runtime_mark_last_busy() and
    pm_runtime_autosuspend().

int blk_pre_runtime_suspend(struct request_queue *q)
  - If any requests are in the queue, return -EBUSY.
    Otherwise set q->rpm_status to RPM_SUSPENDING and return 0.

void blk_post_runtime_suspend(struct request_queue *q, int err)
  - If the suspend succeeded then set q->rpm_status to RPM_SUSPENDED.
    Otherwise set it to RPM_ACTIVE and call pm_runtime_mark_last_busy().

void blk_pre_runtime_resume(struct request_queue *q)
  - Set q->rpm_status to RPM_RESUMING.

void blk_post_runtime_resume(struct request_queue *q, int err)
  - If the resume succeeded then set q->rpm_status to RPM_ACTIVE
    and call pm_runtime_mark_last_busy() and pm_request_autosuspend().
    Otherwise set q->rpm_status to RPM_SUSPENDED.

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

diff --git a/block/blk-core.c b/block/blk-core.c
index 3c923a7..1cc80ae 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2982,6 +2982,68 @@ void blk_finish_plug(struct blk_plug *plug)
 }
 EXPORT_SYMBOL(blk_finish_plug);
 
+#ifdef CONFIG_PM_RUNTIME
+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);
+#endif
+
 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 ba43f40..9395d39 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -19,6 +19,7 @@
 #include <linux/gfp.h>
 #include <linux/bsg.h>
 #include <linux/smp.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/scatterlist.h>
 
@@ -346,6 +347,12 @@ struct request_queue {
 	 */
 	struct kobject kobj;
 
+#ifdef CONFIG_PM_RUNTIME
+	struct device		*dev;
+	int			rpm_status;
+	unsigned int		nr_pending;
+#endif
+
 	/*
 	 * queue settings
 	 */
@@ -893,6 +900,27 @@ struct request_queue *blk_alloc_queue_node(gfp_t, int);
 extern void blk_put_queue(struct request_queue *);
 
 /*
+ * block layer runtime pm functions
+ */
+#ifdef CONFIG_PM_RUNTIME
+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);
+#else
+static inline void blk_pm_runtime_init(struct request_queue *q,
+	struct device *dev) {}
+static inline int blk_pre_runtime_suspend(struct request_queue *q)
+{
+	return -ENOSYS;
+}
+static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
+static inline void blk_pre_runtime_resume(struct request_queue *q) {}
+static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
+#endif
+
+/*
  * 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.10

[PATCH v5 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 resuming/suspending, then only PM request is allowed to go.
    Return NULL for other cases.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
---
 block/blk-core.c       |    7 +++++++
 block/elevator.c       |    4 ++++
 include/linux/blkdev.h |   37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 1cc80ae..cb93501 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1224,6 +1224,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 	if (unlikely(--req->ref_count))
 		return;
 
+	blk_pm_put_request(q);
+
 	elv_completed_request(q, req);
 
 	/* this is a bio leak */
@@ -2012,6 +2014,11 @@ struct request *blk_peek_request(struct request_queue *q)
 	int ret;
 
 	while ((rq = __elv_next_request(q)) != NULL) {
+
+		rq = blk_pm_peek_request(q, rq);
+		if (!rq)
+			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 6a55d41..37c1a2b 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -536,6 +536,8 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
+	blk_pm_requeue_request(q);
+
 	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
@@ -558,6 +560,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 {
 	trace_block_rq_insert(q, rq);
 
+	blk_pm_add_request(q, rq);
+
 	rq->q = q;
 
 	if (rq->cmd_flags & REQ_SOFTBARRIER) {
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 9395d39..c8c5951 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -908,6 +908,36 @@ 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);
+
+static inline void blk_pm_put_request(struct request_queue *q)
+{
+	if (!(--q->nr_pending) && q->dev)
+		 pm_runtime_mark_last_busy(q->dev);
+}
+
+static inline struct request *blk_pm_peek_request(
+	struct request_queue *q, struct request *rq)
+{
+	if (q->rpm_status == RPM_SUSPENDED ||
+		  (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)))
+		return NULL;
+	else
+		return rq;
+}
+
+static inline void blk_pm_requeue_request(struct request_queue *q)
+{
+	q->nr_pending--;
+}
+
+static inline void blk_pm_add_request(struct request_queue *q,
+	struct request *rq)
+{
+	if (q->nr_pending++ == 0 && !(rq->cmd_flags & REQ_PM) &&
+		    (q->rpm_status == RPM_SUSPENDED ||
+		     q->rpm_status == RPM_SUSPENDING) && q->dev)
+		pm_request_resume(q->dev);
+}
 #else
 static inline void blk_pm_runtime_init(struct request_queue *q,
 	struct device *dev) {}
@@ -918,6 +948,13 @@ static inline int blk_pre_runtime_suspend(struct request_queue *q)
 static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
 static inline void blk_pre_runtime_resume(struct request_queue *q) {}
 static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
+
+static inline void blk_pm_put_request(struct request_queue *q) {}
+static inline struct request *blk_pm_peek_request(
+	struct request_queue *q, struct request *rq) { return rq; }
+static inline void blk_pm_requeue_request(struct request_queue *q) {}
+static inline void blk_pm_add_request(struct request_queue *q,
+	struct request *req) {}
 #endif
 
 /*
-- 
1.7.10

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

[James Bottomley]

On Fri, 2012-07-06 at 12:04 +0800, 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 resuming/suspending, then only PM request is allowed to go.
>     Return NULL for other cases.

This is a complete reinvention of the quiesce state, just with new names
and moved up to block in part ... why do we have to have two separate
systems for stopping a device and sending special commands when the
device is suspended, why not just one?

James

> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
>  block/blk-core.c       |    7 +++++++
>  block/elevator.c       |    4 ++++
>  include/linux/blkdev.h |   37 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 1cc80ae..cb93501 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1224,6 +1224,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
>  	if (unlikely(--req->ref_count))
>  		return;
>  
> +	blk_pm_put_request(q);
> +
>  	elv_completed_request(q, req);
>  
>  	/* this is a bio leak */
> @@ -2012,6 +2014,11 @@ struct request *blk_peek_request(struct request_queue *q)
>  	int ret;
>  
>  	while ((rq = __elv_next_request(q)) != NULL) {
> +
> +		rq = blk_pm_peek_request(q, rq);
> +		if (!rq)
> +			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 6a55d41..37c1a2b 100644
> --- a/block/elevator.c
> +++ b/block/elevator.c
> @@ -536,6 +536,8 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
>  
>  	rq->cmd_flags &= ~REQ_STARTED;
>  
> +	blk_pm_requeue_request(q);
> +
>  	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
>  }
>  
> @@ -558,6 +560,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
>  {
>  	trace_block_rq_insert(q, rq);
>  
> +	blk_pm_add_request(q, rq);
> +
>  	rq->q = q;
>  
>  	if (rq->cmd_flags & REQ_SOFTBARRIER) {
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 9395d39..c8c5951 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -908,6 +908,36 @@ 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);
> +
> +static inline void blk_pm_put_request(struct request_queue *q)
> +{
> +	if (!(--q->nr_pending) && q->dev)
> +		 pm_runtime_mark_last_busy(q->dev);
> +}
> +
> +static inline struct request *blk_pm_peek_request(
> +	struct request_queue *q, struct request *rq)
> +{
> +	if (q->rpm_status == RPM_SUSPENDED ||
> +		  (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)))
> +		return NULL;
> +	else
> +		return rq;
> +}
> +
> +static inline void blk_pm_requeue_request(struct request_queue *q)
> +{
> +	q->nr_pending--;
> +}
> +
> +static inline void blk_pm_add_request(struct request_queue *q,
> +	struct request *rq)
> +{
> +	if (q->nr_pending++ == 0 && !(rq->cmd_flags & REQ_PM) &&
> +		    (q->rpm_status == RPM_SUSPENDED ||
> +		     q->rpm_status == RPM_SUSPENDING) && q->dev)
> +		pm_request_resume(q->dev);
> +}
>  #else
>  static inline void blk_pm_runtime_init(struct request_queue *q,
>  	struct device *dev) {}
> @@ -918,6 +948,13 @@ static inline int blk_pre_runtime_suspend(struct request_queue *q)
>  static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
>  static inline void blk_pre_runtime_resume(struct request_queue *q) {}
>  static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
> +
> +static inline void blk_pm_put_request(struct request_queue *q) {}
> +static inline struct request *blk_pm_peek_request(
> +	struct request_queue *q, struct request *rq) { return rq; }
> +static inline void blk_pm_requeue_request(struct request_queue *q) {}
> +static inline void blk_pm_add_request(struct request_queue *q,
> +	struct request *req) {}
>  #endif
>  
>  /*

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

[Lin Ming]

On Fri, 2012-07-06 at 09:00 +0400, James Bottomley wrote:
> On Fri, 2012-07-06 at 12:04 +0800, 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 resuming/suspending, then only PM request is allowed to go.
> >     Return NULL for other cases.
> 
> This is a complete reinvention of the quiesce state, just with new names
> and moved up to block in part ... why do we have to have two separate
> systems for stopping a device and sending special commands when the
> device is suspended, why not just one?

Yes, there are some duplicates with scsi layer quiesce state.
I'd like to do the cleanup.

Add runtime pm support to block layer, so other block device drivers may
also add runtime pm support easily in future.

Some helper functions are provided on block layer which can be called by
block device drivers.

void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
int blk_pre_runtime_suspend(struct request_queue *q)
void blk_post_runtime_suspend(struct request_queue *q, int err)
void blk_pre_runtime_resume(struct request_queue *q)
void blk_post_runtime_resume(struct request_queue *q, int err)

Thanks,
Lin Ming

> 
> James
> 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > ---
> >  block/blk-core.c       |    7 +++++++
> >  block/elevator.c       |    4 ++++
> >  include/linux/blkdev.h |   37 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 48 insertions(+)
> > 
> > diff --git a/block/blk-core.c b/block/blk-core.c
> > index 1cc80ae..cb93501 100644
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> > @@ -1224,6 +1224,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
> >  	if (unlikely(--req->ref_count))
> >  		return;
> >  
> > +	blk_pm_put_request(q);
> > +
> >  	elv_completed_request(q, req);
> >  
> >  	/* this is a bio leak */
> > @@ -2012,6 +2014,11 @@ struct request *blk_peek_request(struct request_queue *q)
> >  	int ret;
> >  
> >  	while ((rq = __elv_next_request(q)) != NULL) {
> > +
> > +		rq = blk_pm_peek_request(q, rq);
> > +		if (!rq)
> > +			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 6a55d41..37c1a2b 100644
> > --- a/block/elevator.c
> > +++ b/block/elevator.c
> > @@ -536,6 +536,8 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
> >  
> >  	rq->cmd_flags &= ~REQ_STARTED;
> >  
> > +	blk_pm_requeue_request(q);
> > +
> >  	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
> >  }
> >  
> > @@ -558,6 +560,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
> >  {
> >  	trace_block_rq_insert(q, rq);
> >  
> > +	blk_pm_add_request(q, rq);
> > +
> >  	rq->q = q;
> >  
> >  	if (rq->cmd_flags & REQ_SOFTBARRIER) {
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index 9395d39..c8c5951 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -908,6 +908,36 @@ 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);
> > +
> > +static inline void blk_pm_put_request(struct request_queue *q)
> > +{
> > +	if (!(--q->nr_pending) && q->dev)
> > +		 pm_runtime_mark_last_busy(q->dev);
> > +}
> > +
> > +static inline struct request *blk_pm_peek_request(
> > +	struct request_queue *q, struct request *rq)
> > +{
> > +	if (q->rpm_status == RPM_SUSPENDED ||
> > +		  (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)))
> > +		return NULL;
> > +	else
> > +		return rq;
> > +}
> > +
> > +static inline void blk_pm_requeue_request(struct request_queue *q)
> > +{
> > +	q->nr_pending--;
> > +}
> > +
> > +static inline void blk_pm_add_request(struct request_queue *q,
> > +	struct request *rq)
> > +{
> > +	if (q->nr_pending++ == 0 && !(rq->cmd_flags & REQ_PM) &&
> > +		    (q->rpm_status == RPM_SUSPENDED ||
> > +		     q->rpm_status == RPM_SUSPENDING) && q->dev)
> > +		pm_request_resume(q->dev);
> > +}
> >  #else
> >  static inline void blk_pm_runtime_init(struct request_queue *q,
> >  	struct device *dev) {}
> > @@ -918,6 +948,13 @@ static inline int blk_pre_runtime_suspend(struct request_queue *q)
> >  static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
> >  static inline void blk_pre_runtime_resume(struct request_queue *q) {}
> >  static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
> > +
> > +static inline void blk_pm_put_request(struct request_queue *q) {}
> > +static inline struct request *blk_pm_peek_request(
> > +	struct request_queue *q, struct request *rq) { return rq; }
> > +static inline void blk_pm_requeue_request(struct request_queue *q) {}
> > +static inline void blk_pm_add_request(struct request_queue *q,
> > +	struct request *req) {}
> >  #endif
> >  
> >  /*
> 
> 

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

[James Bottomley]

On Fri, 2012-07-06 at 14:07 +0800, Lin Ming wrote:
> On Fri, 2012-07-06 at 09:00 +0400, James Bottomley wrote:
> > On Fri, 2012-07-06 at 12:04 +0800, 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 resuming/suspending, then only PM request is allowed to go.
> > >     Return NULL for other cases.
> > 
> > This is a complete reinvention of the quiesce state, just with new names
> > and moved up to block in part ... why do we have to have two separate
> > systems for stopping a device and sending special commands when the
> > device is suspended, why not just one?
> 
> Yes, there are some duplicates with scsi layer quiesce state.
> I'd like to do the cleanup.

The mechanism is pretty much identical:  For quiesce you set the sdev
state to SDEV_QUIESCE and then send in special requests with REQ_PREEMPT
to bypass the suspend.  In your additional scheme you set a queue flag
RPM_SUSPENDED by a pm specific set of callbacks and you only then accept
requests with REQ_PM.  I don't see any difference in actual effect
(well, except that quiesce can be done on a non-empty queue, but that's
a simple flag difference).

What I don't want to see is duplicated mechanisms.  If you want to make
a general quiesce mechanism in block instead of SCSI, I'm fine with
that, but I want to see our current quiesce mechanism moved to it first
since that demonstrates you got it right.  If you don't want to do that,
then just use the existing mechanism in SCSI.

Now that I look at it, your q->nr_pending is an inexact duplicate of
sdev->device_busy as well.  Again, no objection to moving this to block,
but if you then make SCSI use it for sdev->device_busy, you'll get a
very fast indication of whether you got this right or not, which is an
excellent reason for unifying.

In the new scheme, by the way, all this would be integrated directly
into block, so no duplication of blk_xx as blk_pm_xx

James


> Add runtime pm support to block layer, so other block device drivers may
> also add runtime pm support easily in future.
> 
> Some helper functions are provided on block layer which can be called by
> block device drivers.
> 
> void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
> int blk_pre_runtime_suspend(struct request_queue *q)
> void blk_post_runtime_suspend(struct request_queue *q, int err)
> void blk_pre_runtime_resume(struct request_queue *q)
> void blk_post_runtime_resume(struct request_queue *q, int err)
> 
> Thanks,
> Lin Ming
> 
> > 
> > James
> > 
> > > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > > ---
> > >  block/blk-core.c       |    7 +++++++
> > >  block/elevator.c       |    4 ++++
> > >  include/linux/blkdev.h |   37 +++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/block/blk-core.c b/block/blk-core.c
> > > index 1cc80ae..cb93501 100644
> > > --- a/block/blk-core.c
> > > +++ b/block/blk-core.c
> > > @@ -1224,6 +1224,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
> > >  	if (unlikely(--req->ref_count))
> > >  		return;
> > >  
> > > +	blk_pm_put_request(q);
> > > +
> > >  	elv_completed_request(q, req);
> > >  
> > >  	/* this is a bio leak */
> > > @@ -2012,6 +2014,11 @@ struct request *blk_peek_request(struct request_queue *q)
> > >  	int ret;
> > >  
> > >  	while ((rq = __elv_next_request(q)) != NULL) {
> > > +
> > > +		rq = blk_pm_peek_request(q, rq);
> > > +		if (!rq)
> > > +			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 6a55d41..37c1a2b 100644
> > > --- a/block/elevator.c
> > > +++ b/block/elevator.c
> > > @@ -536,6 +536,8 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
> > >  
> > >  	rq->cmd_flags &= ~REQ_STARTED;
> > >  
> > > +	blk_pm_requeue_request(q);
> > > +
> > >  	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
> > >  }
> > >  
> > > @@ -558,6 +560,8 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
> > >  {
> > >  	trace_block_rq_insert(q, rq);
> > >  
> > > +	blk_pm_add_request(q, rq);
> > > +
> > >  	rq->q = q;
> > >  
> > >  	if (rq->cmd_flags & REQ_SOFTBARRIER) {
> > > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > index 9395d39..c8c5951 100644
> > > --- a/include/linux/blkdev.h
> > > +++ b/include/linux/blkdev.h
> > > @@ -908,6 +908,36 @@ 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);
> > > +
> > > +static inline void blk_pm_put_request(struct request_queue *q)
> > > +{
> > > +	if (!(--q->nr_pending) && q->dev)
> > > +		 pm_runtime_mark_last_busy(q->dev);
> > > +}
> > > +
> > > +static inline struct request *blk_pm_peek_request(
> > > +	struct request_queue *q, struct request *rq)
> > > +{
> > > +	if (q->rpm_status == RPM_SUSPENDED ||
> > > +		  (q->rpm_status != RPM_ACTIVE && !(rq->cmd_flags & REQ_PM)))
> > > +		return NULL;
> > > +	else
> > > +		return rq;
> > > +}
> > > +
> > > +static inline void blk_pm_requeue_request(struct request_queue *q)
> > > +{
> > > +	q->nr_pending--;
> > > +}
> > > +
> > > +static inline void blk_pm_add_request(struct request_queue *q,
> > > +	struct request *rq)
> > > +{
> > > +	if (q->nr_pending++ == 0 && !(rq->cmd_flags & REQ_PM) &&
> > > +		    (q->rpm_status == RPM_SUSPENDED ||
> > > +		     q->rpm_status == RPM_SUSPENDING) && q->dev)
> > > +		pm_request_resume(q->dev);
> > > +}
> > >  #else
> > >  static inline void blk_pm_runtime_init(struct request_queue *q,
> > >  	struct device *dev) {}
> > > @@ -918,6 +948,13 @@ static inline int blk_pre_runtime_suspend(struct request_queue *q)
> > >  static inline void blk_post_runtime_suspend(struct request_queue *q, int err) {}
> > >  static inline void blk_pre_runtime_resume(struct request_queue *q) {}
> > >  static inline void blk_post_runtime_resume(struct request_queue *q, int err) {}
> > > +
> > > +static inline void blk_pm_put_request(struct request_queue *q) {}
> > > +static inline struct request *blk_pm_peek_request(
> > > +	struct request_queue *q, struct request *rq) { return rq; }
> > > +static inline void blk_pm_requeue_request(struct request_queue *q) {}
> > > +static inline void blk_pm_add_request(struct request_queue *q,
> > > +	struct request *req) {}
> > >  #endif
> > >  
> > >  /*
> > 
> > 
> 
> 
> --
> 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 v5 3/4] block: implement runtime pm strategy

[Alan Stern]

On Fri, 6 Jul 2012, James Bottomley wrote:

> The mechanism is pretty much identical:  For quiesce you set the sdev
> state to SDEV_QUIESCE and then send in special requests with REQ_PREEMPT
> to bypass the suspend.  In your additional scheme you set a queue flag
> RPM_SUSPENDED by a pm specific set of callbacks and you only then accept
> requests with REQ_PM.  I don't see any difference in actual effect
> (well, except that quiesce can be done on a non-empty queue, but that's
> a simple flag difference).
> 
> What I don't want to see is duplicated mechanisms.  If you want to make
> a general quiesce mechanism in block instead of SCSI, I'm fine with
> that, but I want to see our current quiesce mechanism moved to it first
> since that demonstrates you got it right.  If you don't want to do that,
> then just use the existing mechanism in SCSI.

Moving the whole thing to block sounds like a good suggestion.  But are
the end results supposed to be the same?  For example, if a rotating
disk is put into the QUIESCE state, does it get spun down (as would
happen with a runtime suspend)?  I rather got the impression that
QUIESCE meant carrying out existing requests and not accepting new
ones, without the other actions needed for suspend.

What is the client interface for the quiesce mechanism?  There are a
few calls in scsi_transport_spi.c, but no other obvious entry points
besides sysfs.

Are you suggesting that writing "quiesce" to the state attribute should
drain the request queue and then initiate a runtime suspend?

> Now that I look at it, your q->nr_pending is an inexact duplicate of
> sdev->device_busy as well.  Again, no objection to moving this to block,
> but if you then make SCSI use it for sdev->device_busy, you'll get a
> very fast indication of whether you got this right or not, which is an
> excellent reason for unifying.

They aren't exactly the same, because nr_pending counts requests on the
queue whether or not they have been started at the SCSI level.  But
obviously they are closely related.

Alan Stern

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

[Alan Stern]

On Fri, 6 Jul 2012, James Bottomley wrote:

> The mechanism is pretty much identical:  For quiesce you set the sdev
> state to SDEV_QUIESCE and then send in special requests with REQ_PREEMPT
> to bypass the suspend.  In your additional scheme you set a queue flag
> RPM_SUSPENDED by a pm specific set of callbacks and you only then accept
> requests with REQ_PM.  I don't see any difference in actual effect
> (well, except that quiesce can be done on a non-empty queue, but that's
> a simple flag difference).
> 
> What I don't want to see is duplicated mechanisms.  If you want to make
> a general quiesce mechanism in block instead of SCSI, I'm fine with
> that, but I want to see our current quiesce mechanism moved to it first
> since that demonstrates you got it right.  If you don't want to do that,
> then just use the existing mechanism in SCSI.

Moving the whole thing to block sounds like a good suggestion.  But are
the end results supposed to be the same?  For example, if a rotating
disk is put into the QUIESCE state, does it get spun down (as would
happen with a runtime suspend)?  I rather got the impression that
QUIESCE meant carrying out existing requests and not accepting new
ones, without the other actions needed for suspend.

What is the client interface for the quiesce mechanism?  There are a
few calls in scsi_transport_spi.c, but no other obvious entry points
besides sysfs.

Are you suggesting that writing "quiesce" to the state attribute should
drain the request queue and then initiate a runtime suspend?

> Now that I look at it, your q->nr_pending is an inexact duplicate of
> sdev->device_busy as well.  Again, no objection to moving this to block,
> but if you then make SCSI use it for sdev->device_busy, you'll get a
> very fast indication of whether you got this right or not, which is an
> excellent reason for unifying.

They aren't exactly the same, because nr_pending counts requests on the
queue whether or not they have been started at the SCSI level.  But
obviously they are closely related.

Alan Stern

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

[Aaron Lu]

Removed LinMing@intel

Hi James,

Sorry for the late reply, as I'm just picking up this work and thought
this may still need some discussion/clarification. Please see below.

On 07/06/2012 04:05 PM, James Bottomley wrote:
> On Fri, 2012-07-06 at 14:07 +0800, Lin Ming wrote:
>> On Fri, 2012-07-06 at 09:00 +0400, James Bottomley wrote:
>>> On Fri, 2012-07-06 at 12:04 +0800, 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 resuming/suspending, then only PM request is allowed to go.
>>>>     Return NULL for other cases.
>>>
>>> This is a complete reinvention of the quiesce state, just with new names
>>> and moved up to block in part ... why do we have to have two separate
>>> systems for stopping a device and sending special commands when the
>>> device is suspended, why not just one?
>>
>> Yes, there are some duplicates with scsi layer quiesce state.
>> I'd like to do the cleanup.
> 
> The mechanism is pretty much identical:  For quiesce you set the sdev
> state to SDEV_QUIESCE and then send in special requests with REQ_PREEMPT
> to bypass the suspend.  In your additional scheme you set a queue flag
> RPM_SUSPENDED by a pm specific set of callbacks and you only then accept
> requests with REQ_PM.  I don't see any difference in actual effect
> (well, except that quiesce can be done on a non-empty queue, but that's
> a simple flag difference).

My understanding of quiesce state is that, it is used by scsi driver to
say: hold on for a moment, no more FS requests, I need to do some
housekeeping work for the device and when I'm done, I'll put it back to
normal state.

The housekeeping work will generally involve executing some commands for
the device using scsi_execute, as all requests originated from
scsi_execute are of type REQ_PREEMPT. But it is not for suspending the
device.

While for runtime suspended state, we can't say: everybody quiet, I'm
gonna sleep, so I'll ignore all of your requests. Instead, we keep track
of the device's activity, and once we have learned that it is not being
used by anyone, we put it into a low power state. And if there is any
requests arrived for it due to whatever user, we will need to resume it
to process the request.

So I think they are different, serves for different purposes.

> 
> What I don't want to see is duplicated mechanisms.  If you want to make
> a general quiesce mechanism in block instead of SCSI, I'm fine with
> that, but I want to see our current quiesce mechanism moved to it first
> since that demonstrates you got it right.  If you don't want to do that,
> then just use the existing mechanism in SCSI.
> 
> Now that I look at it, your q->nr_pending is an inexact duplicate of
> sdev->device_busy as well.  Again, no objection to moving this to block,
> but if you then make SCSI use it for sdev->device_busy, you'll get a
> very fast indication of whether you got this right or not, which is an
> excellent reason for unifying.

The nr_pending is the counter of how many requests are in the device's
request queue, while the device_busy is how many commands are being
processed by the device, and they can be different as Alan Stern has put
it here:
http://www.spinics.net/lists/linux-scsi/msg60497.html

And I think we still need device_busy in scsi layer, for the case when
the scsi prepare phase decides to defer the request(due to no memory or
whatever), we will have 1 request in the queue while no commands are
being processed by the device. And we need to keep track of such
information in order to decide if we need restart the queue when we
return BLKPREP_DEFER in function scsi_prep_return:

        case BLKPREP_DEFER:
                /*
                 * If we defer, the blk_peek_request() returns NULL, but the
                 * queue must be restarted, so we schedule a callback to happen
                 * shortly.
                 */
                if (sdev->device_busy == 0)
                        blk_delay_queue(q, SCSI_QUEUE_DELAY);
                break;

So I think we still need device_busy.

OK, this is my understanding, they may be wrong, so please let me know
what do you think, thanks.

-Aaron

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

[Matthew Wilcox]

On Fri, Jul 06, 2012 at 12:04:31PM +0800, 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 resuming/suspending, then only PM request is allowed to go.
>     Return NULL for other cases.

This only works for drivers which are struct request based.  For those
that handle struct bio directly, nr_pending will always be 0.

I think the right place to increment nr_pending is in generic_make_request.
But I'm not sure where to decrement it.  I think we need a blk_bio_endio()
which does some housekeeping and then calls bio_endio().

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

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

[Alan Stern]

On Fri, 6 Jul 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 resuming/suspending, then only PM request is allowed to go.
>     Return NULL for other cases.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Generally okay, but...

> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -908,6 +908,36 @@ 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);
> +
> +static inline void blk_pm_put_request(struct request_queue *q)
> +{
> +	if (!(--q->nr_pending) && q->dev)
> +		 pm_runtime_mark_last_busy(q->dev);
> +}
...

These new helper routines are private to the block layer, not intended
for use by clients.  Consequently they don't belong in include/linux;
they should go in block/blk.h.

Alan Stern

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

[Alan Stern]

On Fri, 6 Jul 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 resuming/suspending, then only PM request is allowed to go.
>     Return NULL for other cases.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>

Generally okay, but...

> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -908,6 +908,36 @@ 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);
> +
> +static inline void blk_pm_put_request(struct request_queue *q)
> +{
> +	if (!(--q->nr_pending) && q->dev)
> +		 pm_runtime_mark_last_busy(q->dev);
> +}
...

These new helper routines are private to the block layer, not intended
for use by clients.  Consequently they don't belong in include/linux;
they should go in block/blk.h.

Alan Stern

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

[Lin Ming]

On Fri, Jul 6, 2012 at 10:21 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Fri, 6 Jul 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 resuming/suspending, then only PM request is allowed to go.
>>     Return NULL for other cases.
>>
>> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>
> Generally okay, but...
>
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -908,6 +908,36 @@ 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);
>> +
>> +static inline void blk_pm_put_request(struct request_queue *q)
>> +{
>> +     if (!(--q->nr_pending) && q->dev)
>> +              pm_runtime_mark_last_busy(q->dev);
>> +}
> ...
>
> These new helper routines are private to the block layer, not intended
> for use by clients.  Consequently they don't belong in include/linux;
> they should go in block/blk.h.

OK, will move it.

>
> Alan Stern

[PATCH v5 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 path.

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

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index dc0ad85..f0b279a 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -74,6 +74,7 @@ static int scsi_bus_suspend_common(struct device *dev, pm_message_t msg)
 
 static int scsi_bus_resume_common(struct device *dev)
 {
+	struct request_queue *q = NULL;
 	int err = 0;
 
 	/*
@@ -84,14 +85,22 @@ static int scsi_bus_resume_common(struct device *dev)
 	 */
 	pm_runtime_get_sync(dev->parent);
 
-	if (scsi_is_sdev_device(dev))
+	if (scsi_is_sdev_device(dev)) {
+		struct scsi_device *sdev = to_scsi_device(dev);
+		q = sdev->request_queue;
+
+		blk_pre_runtime_resume(q);
 		err = scsi_dev_type_resume(dev);
+	}
 	if (err == 0) {
 		pm_runtime_disable(dev);
 		pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
 	}
 
+	if (q)
+		blk_post_runtime_resume(q, err);
+
 	pm_runtime_put_sync(dev->parent);
 
 	return err;
@@ -143,10 +152,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 */
@@ -159,8 +174,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 */
 
@@ -175,9 +196,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_runtime_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 ce03a0a..db45bda 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -967,10 +967,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.
@@ -1015,8 +1011,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;	
 }
@@ -1051,7 +1045,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;
 }
-- 
1.7.10