[PATCH v4 0/4]: block layer runtime pm

[PATCH v4 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 v4 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.

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           |   79 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/elevator.c           |   13 +++++++++++++
 drivers/scsi/scsi_lib.c    |   25 ++++++++++++++++++++++---
 drivers/scsi/scsi_pm.c     |   32 +++++++++++++++++++++++++-------
 drivers/scsi/scsi_sysfs.c  |    2 ++
 drivers/scsi/sd.c          |   16 +++++-----------
 include/linux/blk_types.h  |    2 ++
 include/linux/blkdev.h     |   27 +++++++++++++++++++++++++++
 include/scsi/scsi_device.h |    4 ++++
 9 files changed, 179 insertions(+), 21 deletions(-)

[PATCH v4 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 6dfb978..45538e5 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 6f72b80..bf8413a 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_flag(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_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 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..e7fdff5 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -388,6 +388,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.10

[PATCH v4 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>
---

v4:
- Adds CONFIG_PM_RUNTIME check

 block/blk-core.c       |   63 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |   27 +++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 3c923a7..b94ec5f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -30,6 +30,7 @@
 #include <linux/list_sort.h>
 #include <linux/delay.h>
 #include <linux/ratelimit.h>
+#include <linux/pm_runtime.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/block.h>
@@ -2982,6 +2983,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..a5b255e 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -346,6 +346,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 +899,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 v4 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>
---

v4:
- Adds CONFIG_PM_RUNTIME check

 block/blk-core.c |   16 ++++++++++++++++
 block/elevator.c |   13 +++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index b94ec5f..4720bec 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1225,6 +1225,11 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 	if (unlikely(--req->ref_count))
 		return;
 
+#ifdef CONFIG_PM_RUNTIME
+	if (!(--q->nr_pending) && q->dev)
+		pm_runtime_mark_last_busy(q->dev);
+#endif
+
 	elv_completed_request(q, req);
 
 	/* this is a bio leak */
@@ -2013,6 +2018,17 @@ struct request *blk_peek_request(struct request_queue *q)
 	int ret;
 
 	while ((rq = __elv_next_request(q)) != NULL) {
+
+#ifdef CONFIG_PM_RUNTIME
+		if (q->rpm_status == RPM_SUSPENDED ||
+			(q->rpm_status != RPM_ACTIVE
+			    && !(rq->cmd_flags & REQ_PM))) {
+
+			rq = NULL;
+			break;
+		}
+#endif
+
 		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..9f82e77 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>
 
@@ -536,6 +537,11 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
+#ifdef CONFIG_PM_RUNTIME
+	/* __elv_add_request will increment the count */
+	if (!(rq->cmd_flags & REQ_PM))
+		q->nr_pending--;
+#endif
 	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
@@ -558,6 +564,13 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where)
 {
 	trace_block_rq_insert(q, rq);
 
+#ifdef CONFIG_PM_RUNTIME
+	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);
+#endif
+
 	rq->q = q;
 
 	if (rq->cmd_flags & REQ_SOFTBARRIER) {
-- 
1.7.10

[PATCH v4 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>
---

v4:
- Update queue pm status in scsi_bus_resume_common

 drivers/scsi/scsi_pm.c    |   32 +++++++++++++++++++++++++-------
 drivers/scsi/scsi_sysfs.c |    2 ++
 drivers/scsi/sd.c         |    7 -------
 3 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index d2a80de..aa43fa2 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -77,6 +77,9 @@ static int scsi_bus_resume_common(struct device *dev)
 	int err = 0;
 
 	if (scsi_is_sdev_device(dev)) {
+		struct scsi_device *sdev = to_scsi_device(dev);
+		struct request_queue *q = sdev->request_queue;
+
 		/*
 		 * Parent device may have runtime suspended as soon as
 		 * it is woken up during the system resume.
@@ -84,12 +87,14 @@ static int scsi_bus_resume_common(struct device *dev)
 		 * Resume it on behalf of child.
 		 */
 		pm_runtime_get_sync(dev->parent);
+		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);
 		}
+		blk_post_runtime_resume(q, err);
 		pm_runtime_put_sync(dev->parent);
 	}
 
@@ -142,10 +147,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 */
@@ -158,8 +169,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 */
 
@@ -174,9 +191,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 bf8413a..bf9f4e8 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

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

[Rafael J. Wysocki]

On Thursday, July 05, 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.
> 
> 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 6dfb978..45538e5 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);

Hmm.  You're only adding one item to the call chain this way and with many
arguments.  I think it would be better to export __scsi_execute_req()
directly and define the wrappers as static inlines in a header.

In which case you won't need the second wrapper even.  So perhaps it would be
a good idea to call the function scsi_execute_req_flags() and add a static
inline wrapper called scsi_execute_req() around it that will pass 0 as flags?

Rafael


> +
>  /*
>   * Function:    scsi_init_cmd_errh()
>   *
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 6f72b80..bf8413a 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_flag(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_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 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..e7fdff5 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -388,6 +388,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 *);
> 

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

[Rafael J. Wysocki]

On Thursday, July 05, 2012, Lin Ming wrote:
> 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()

What exactly do you need these things for?  Please be specific.

Thanks,
Rafael


> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> ---
> 
> v4:
> - Adds CONFIG_PM_RUNTIME check
> 
>  block/blk-core.c       |   63 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/blkdev.h |   27 +++++++++++++++++++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 3c923a7..b94ec5f 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -30,6 +30,7 @@
>  #include <linux/list_sort.h>
>  #include <linux/delay.h>
>  #include <linux/ratelimit.h>
> +#include <linux/pm_runtime.h>
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/block.h>
> @@ -2982,6 +2983,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..a5b255e 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -346,6 +346,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 +899,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
> 

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

[Alan Stern]

On Thu, 5 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>

These changes would look better if you didn't have "#ifdef
CONFIG_PM_RUNTIME" sprinkled throughout the block-layer routines.

Instead, define static helper functions to do your work, and put the 
definitions inside a #ifdef block.  If CONFIG_PM_RUNTIME isn't enabled,
the helper functions can be empty static inlines.
 
> --- a/block/elevator.c +++ b/block/elevator.c @@ -536,6 +537,11 @@

> void elv_requeue_request(struct request_queue *q, struct request *rq)
>  
>  	rq->cmd_flags &= ~REQ_STARTED;
>  
> +#ifdef CONFIG_PM_RUNTIME
> +	/* __elv_add_request will increment the count */
> +	if (!(rq->cmd_flags & REQ_PM))
> +		q->nr_pending--;
> +#endif

__elv_add_request increments nr_pending even when REQ_PM is set.  You 
need to be consistent with that behavior.

Alan Stern

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

[Lin Ming]

On Thu, 2012-07-05 at 15:11 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Lin Ming wrote:
> > 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()
> 
> What exactly do you need these things for?  Please be specific.

Alan described these functions nicely, copied here.
http://marc.info/?l=linux-kernel&m=133727953025955&w=2

I'll add these function descriptions to the patch log.

====
This is not the way to do it.  The block subsystem should not use 
suspend/resume callbacks.

Instead, there should be block functions that can be called by client
drivers: block_pre_runtime_suspend, block_post_runtime_suspend,
bock_pre_runtime_resume, and block_post_runtime_resume.

They should do something like this:

	block_pre_runtime_suspend:
		If any requests are in the queue, return -EBUSY.
		Otherwise set q->rpm_status to RPM_SUSPENDING and
		return 0.

	block_post_runtime_suspend:
		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().

	block_pre_runtime_resume:
		Set q->rpm_status to RPM_RESUMING.

	block_post_runtime_resume:
		If the resume succeeded then set q->rpm_status to
		RPM_ACTIVE and call pm_runtime_mark_last_busy() and
		pm_runtime_request_autosuspend().
		Otherwise set q->rpm_status to RPM_SUSPENDED.

There should also be an initialization function for client drivers to
call.  block_runtime_pm_init() should call pm_runtime_mark_last_busy(),
pm_runtime_use_autosuspend(), and pm_runtime_autosuspend().

Next, you have to modify the parts of the block layer that run when a 
new request is added to the queue or a request is removed.

	When a request is added:
		If q->rpm_status is RPM_SUSPENDED, or if q->rpm_status
		is RPM_SUSPENDING and the REQ_PM flag isn't set, call
		pm_runtime_request_resume().

	When a request finishes:
		Call pm_runtime_mark_last_busy().

Next, you have to change the parts of the block layer responsible for
taking a request from the queue and handing it to the lower-level
driver (both peek and get).  If q->rpm_status is RPM_SUSPENDED, they
shouldn't do anything -- act as though the queue is empty.  If
q->rpm_status is RPM_SUSPENDING or RPM_RESUMING, they should hand over
the request only if it has the REQ_PM flag set.

For this to work, the block layer has to know what struct device
pointer to pass to the pm_runtime_* routines.  You'll have to add that
information to the request_queue structure; I guess q->dev can get set
by block_pm_runtime_init().  In fact, when that's done you won't need
q->rpm_status any more.  You'll be able to use q->dev->power.rpm_status
directly, and you won't have to update it because the PM core does that
for you.

(Or maybe it would be easier to make q->rpm_status be a pointer to 
q->dev->power.rpm_status.  That way, if CONFIG_PM_RUNTIME isn't enabled 
or block_runtime_pm_init() hasn't been called, you can have 
q->rpm_status simply point to a static value that is permanently set to 
RPM_ACTIVE.)

I may have left some parts out from this brief description.  Hopefully 
you'll be able to figure out the general idea and get it to work.
====

Thanks,
Lin Ming