[PATCH v6 0/4] block layer runtime pm

[PATCH v6 0/4] block layer runtime pm

[Aaron Lu]

In August 2010, Jens and Alan discussed about "Runtime PM and the block
layer". http://marc.info/?t=128259108400001&r=1&w=2
And then Alan has given a detailed implementation guide:
http://marc.info/?l=linux-scsi&m=133727953625963&w=2

To test:
# 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 10000 > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/autosuspend_delay_ms
# echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/control
Then you'll see sda is suspended after 10secs idle.

[ 1767.680192] sd 2:0:0:0: [sda] Synchronizing SCSI cache
[ 1767.680317] sd 2:0:0:0: [sda] Stopping disk

And if you do some IO, it will resume immediately.
[ 1791.052438] sd 2:0:0:0: [sda] Starting disk

For test, I often set the autosuspend time to 1 second. If you are using
a GUI, the 10 seconds delay may be too long that the disk can not enter
runtime suspended state.

Note that sd's runtime suspend callback will dump some kernel messages
and the syslog daemon will write kernel message to /var/log/messages,
making the disk instantly resume after suspended. So for test, the
syslog daemon should better be temporarily stopped.

v6:
Take over from Lin Ming.

- Instead of put the device into autosuspend state in
  blk_post_runtime_suspend, do it when the last request is finished.
  This can also solve the problem illustrated below:

      thread A				      thread B
|suspend timer expired			|
|  ... ...				|a new request comes in,
|  ... ...				|blk_pm_add_request
|  ... ...				|skip request_resume due to
|  ... ...				|q->status is still RPM_ACTIVE
|  rpm_suspend				|  ... ...
|    scsi_runtime_suspend		|  ... ...
|      blk_pre_runtime_suspend		|  ... ...
|      return -EBUSY due to nr_pending	|  ... ...
|  rpm_suspend done			|  ... ...
|					|    blk_pm_put_request, mark last busy

But no more trigger point, and the device will stay at RPM_ACTIVE state.
Run pm_runtime_autosuspend after the last request is finished solved
this problem.

- Requests which have the REQ_PM flag should not involve nr_pending
  counting, or we may lose the condition to resume the device:
  Suppose queue is active and nr_pending is 0. Then a REQ_PM request
  comes and nr_pending will be increased to 1, but since the request has
  REQ_PM flag, it will not cause resume. Before it is finished, a normal
  request comes in, and since nr_pending is 1 now, it will not trigger
  the resume of the device either. Bug.

- Do not quiesce the device in scsi bus level runtime suspend callback.
  Since the only reason the device is to be runtime suspended is due to
  no more requests pending for it, quiesce it is pointless.

- Remove scsi_autopm_* from sd_check_events as we are request driven.

- Call blk_pm_runtime_init in scsi_sysfs_initialize_dev, so that we do
  not need to check queue's device in blk_pm_add/put_request.

- Do not mark last busy and initiate an autosuspend for the device in
  blk_pm_runtime_init function.

- Do not mark last busy and initiate an autosuspend for the device in
  block_post_runtime_resume, as when the request that triggered the
  resume finished, the blk_pm_put_request will mark last busy and
  initiate an autosuspend.

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
  sd: change to auto suspend mode

 block/blk-core.c           | 62 +++++++++++++++++++++++++++++++++++++++++
 block/elevator.c           |  4 +++
 drivers/scsi/scsi_lib.c    |  9 +++---
 drivers/scsi/scsi_pm.c     | 35 +++++++++++++++--------
 drivers/scsi/scsi_sysfs.c  |  1 +
 drivers/scsi/sd.c          | 21 ++++----------
 include/linux/blk_types.h  |  2 ++
 include/linux/blkdev.h     | 69 ++++++++++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h | 17 +++++++++---
 9 files changed, 183 insertions(+), 37 deletions(-)

-- 
1.7.11.7

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

[Aaron Lu]

From: Lin Ming <ming.m.lin@intel.com>

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>
Signed-off-by: Aaron Lu <aaron.lu@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 f1bf5af..af1b8b3 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -271,11 +271,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;
@@ -286,14 +285,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 7992635..8ca160e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1424,8 +1424,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;
 	}
@@ -3021,8 +3022,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 cdf1119..fcc1ce2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -175,6 +175,7 @@ enum rq_flag_bits {
 	__REQ_IO_STAT,		/* account I/O stat */
 	__REQ_MIXED_MERGE,	/* merge of different types, fail separately */
 	__REQ_KERNEL, 		/* direct IO to kernel pages */
+	__REQ_PM,		/* runtime pm request */
 	__REQ_NR_BITS,		/* stops here */
 };
 
@@ -223,5 +224,6 @@ enum rq_flag_bits {
 #define REQ_MIXED_MERGE		(1 << __REQ_MIXED_MERGE)
 #define REQ_SECURE		(1 << __REQ_SECURE)
 #define REQ_KERNEL		(1 << __REQ_KERNEL)
+#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 e65c62e..aff494c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -393,10 +393,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.11.7

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

[Aaron Lu]

From: Lin Ming <ming.m.lin@intel.com>

Add runtime pm helper functions:

void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
  - Initialization function for drivers to call.

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.

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 __blk_run_queue.
    Otherwise set q->rpm_status to RPM_SUSPENDED.

[aaron.lu@intel.com: do not touch last busy in these helper functions]
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/blk-core.c       | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h | 28 +++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index c973249..6fc24bb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -3055,6 +3055,61 @@ 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);
+}
+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;
+	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);
+	} 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 f94bc83..a96e144 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>
 
@@ -360,6 +361,12 @@ struct request_queue {
 	 */
 	struct kobject kobj;
 
+#ifdef CONFIG_PM_RUNTIME
+	struct device		*dev;
+	int			rpm_status;
+	unsigned int		nr_pending;
+#endif
+
 	/*
 	 * queue settings
 	 */
@@ -959,6 +966,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.11.7

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

[Aaron Lu]

From: Lin Ming <ming.m.lin@intel.com>

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

When the last request finishes:
    Call pm_runtime_mark_last_busy() and pm_runtime_autosuspend().

When pick a request:
    If device is resuming/suspending, then only PM request is allowed to go.
    Return NULL for other cases.

[aaron.lu@intel.com: PM request does not involve nr_pending counting]
[aaron.lu@intel.com: No need to check q->dev]
[aaron.lu@intel.com: Autosuspend when the last request finished]
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/blk-core.c       |  7 +++++++
 block/elevator.c       |  4 ++++
 include/linux/blkdev.h | 41 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 6fc24bb..93c1461 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1274,6 +1274,8 @@ void __blk_put_request(struct request_queue *q, struct request *req)
 	if (unlikely(--req->ref_count))
 		return;
 
+	blk_pm_put_request(req);
+
 	elv_completed_request(q, req);
 
 	/* this is a bio leak */
@@ -2080,6 +2082,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 9edba1b..61e9b49 100644
--- a/block/elevator.c
+++ b/block/elevator.c
@@ -544,6 +544,8 @@ void elv_requeue_request(struct request_queue *q, struct request *rq)
 
 	rq->cmd_flags &= ~REQ_STARTED;
 
+	blk_pm_requeue_request(rq);
+
 	__elv_add_request(q, rq, ELEVATOR_INSERT_REQUEUE);
 }
 
@@ -566,6 +568,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 a96e144..884c405 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -974,6 +974,40 @@ 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 *rq)
+{
+	if (!(rq->cmd_flags & REQ_PM) && !--rq->q->nr_pending) {
+		pm_runtime_mark_last_busy(rq->q->dev);
+		pm_runtime_autosuspend(rq->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 *rq)
+{
+	if (!(rq->cmd_flags & REQ_PM))
+		rq->q->nr_pending--;
+}
+
+static inline void blk_pm_add_request(struct request_queue *q,
+	struct request *rq)
+{
+	if (!(rq->cmd_flags & REQ_PM) &&
+	    q->nr_pending++ == 0 &&
+	    (q->rpm_status == RPM_SUSPENDED ||
+	     q->rpm_status == RPM_SUSPENDING))
+		pm_request_resume(q->dev);
+}
 #else
 static inline void blk_pm_runtime_init(struct request_queue *q,
 	struct device *dev) {}
@@ -984,6 +1018,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 *rq) {}
+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 *rq) {}
+static inline void blk_pm_add_request(struct request_queue *q,
+	struct request *req) {}
 #endif
 
 /*
-- 
1.7.11.7

[PATCH v6 4/4] sd: change to auto suspend mode

[Aaron Lu]

From: Lin Ming <ming.m.lin@intel.com>

Uses block layer runtime pm helper functions in
scsi_runtime_suspend/resume.
Remove scsi_autopm_* from sd open/release path and check_events path.
And remove the quiesce call in runtime suspend path, as we know there is
no request to quiesce for the device.

[aaron.lu@intel.com: Do not quiesce the device in runtime suspend]
[aaron.lu@intel.com: Remove scsi_autopm_* from sd_check_events]
[aaron.lu@intel.com: Move blk_pm_runtime_init to sdev init time]
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 drivers/scsi/scsi_pm.c    | 35 +++++++++++++++++++++++------------
 drivers/scsi/scsi_sysfs.c |  1 +
 drivers/scsi/sd.c         | 12 ------------
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 8f6b12c..d5ddc2c 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -147,15 +147,19 @@ static int scsi_bus_restore(struct device *dev)
 static int scsi_runtime_suspend(struct device *dev)
 {
 	int err = 0;
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	dev_dbg(dev, "scsi_runtime_suspend\n");
 	if (scsi_is_sdev_device(dev)) {
-		err = scsi_dev_type_suspend(dev,
-				pm ? pm->runtime_suspend : NULL);
-		if (err == -EAGAIN)
-			pm_schedule_suspend(dev, jiffies_to_msecs(
-				round_jiffies_up_relative(HZ/10)));
+		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 = pm_generic_runtime_suspend(dev);
+
+		blk_post_runtime_suspend(q, err);
 	}
 
 	/* Insert hooks here for targets, hosts, and transport classes */
@@ -166,11 +170,16 @@ static int scsi_runtime_suspend(struct device *dev)
 static int scsi_runtime_resume(struct device *dev)
 {
 	int err = 0;
-	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
 
 	dev_dbg(dev, "scsi_runtime_resume\n");
-	if (scsi_is_sdev_device(dev))
-		err = scsi_dev_type_resume(dev, pm ? pm->runtime_resume : NULL);
+	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 = pm_generic_runtime_resume(dev);
+		blk_post_runtime_resume(q, err);
+	}
 
 	/* Insert hooks here for targets, hosts, and transport classes */
 
@@ -185,10 +194,12 @@ 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 931a7d9..63384cf 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -1112,6 +1112,7 @@ void scsi_sysfs_device_initialize(struct scsi_device *sdev)
 	sdev->sdev_gendev.type = &scsi_dev_type;
 	dev_set_name(&sdev->sdev_gendev, "%d:%d:%d:%d",
 		     sdev->host->host_no, sdev->channel, sdev->id, sdev->lun);
+	blk_pm_runtime_init(sdev->request_queue, &sdev->sdev_gendev);
 
 	device_initialize(&sdev->sdev_dev);
 	sdev->sdev_dev.parent = get_device(&sdev->sdev_gendev);
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8ca160e..70c202c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1121,10 +1121,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.
@@ -1169,8 +1165,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;	
 }
@@ -1205,7 +1199,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;
 }
@@ -1367,14 +1360,9 @@ static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
 	retval = -ENODEV;
 
 	if (scsi_block_when_processing_errors(sdp)) {
-		retval = scsi_autopm_get_device(sdp);
-		if (retval)
-			goto out;
-
 		sshdr  = kzalloc(sizeof(*sshdr), GFP_KERNEL);
 		retval = scsi_test_unit_ready(sdp, SD_TIMEOUT, SD_MAX_RETRIES,
 					      sshdr);
-		scsi_autopm_put_device(sdp);
 	}
 
 	/* failed to execute TUR, assume media not present */
-- 
1.7.11.7

Re: [PATCH v6 4/4] sd: change to auto suspend mode

[Oliver Neukum]

On Sunday 06 January 2013 16:41:37 Aaron Lu wrote:
> From: Lin Ming <ming.m.lin@intel.com>
> 
> Uses block layer runtime pm helper functions in
> scsi_runtime_suspend/resume.
> Remove scsi_autopm_* from sd open/release path and check_events path.
> And remove the quiesce call in runtime suspend path, as we know there is
> no request to quiesce for the device.

How does this handle ioctl() ?

	Regards
		Oliver

Re: [PATCH v6 4/4] sd: change to auto suspend mode

[Aaron Lu]

On 01/07/2013 05:19 PM, Oliver Neukum wrote:
> On Sunday 06 January 2013 16:41:37 Aaron Lu wrote:
>> From: Lin Ming <ming.m.lin@intel.com>
>>
>> Uses block layer runtime pm helper functions in
>> scsi_runtime_suspend/resume.
>> Remove scsi_autopm_* from sd open/release path and check_events path.
>> And remove the quiesce call in runtime suspend path, as we know there is
>> no request to quiesce for the device.
> 
> How does this handle ioctl() ?

The ioctl code will allocate a new request and execute it through block
layer API, which involves adding the request to the queue and releasing
it when it is finished.

Thanks,
Aaron

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

[Alan Stern]

On Sun, 6 Jan 2013, Aaron Lu wrote:

> In August 2010, Jens and Alan discussed about "Runtime PM and the block
> layer". http://marc.info/?t=128259108400001&r=1&w=2
> And then Alan has given a detailed implementation guide:
> http://marc.info/?l=linux-scsi&m=133727953625963&w=2
> 
> To test:
> # 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 10000 > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/autosuspend_delay_ms
> # echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/control
> Then you'll see sda is suspended after 10secs idle.
> 
> [ 1767.680192] sd 2:0:0:0: [sda] Synchronizing SCSI cache
> [ 1767.680317] sd 2:0:0:0: [sda] Stopping disk
> 
> And if you do some IO, it will resume immediately.
> [ 1791.052438] sd 2:0:0:0: [sda] Starting disk
> 
> For test, I often set the autosuspend time to 1 second. If you are using
> a GUI, the 10 seconds delay may be too long that the disk can not enter
> runtime suspended state.
> 
> Note that sd's runtime suspend callback will dump some kernel messages
> and the syslog daemon will write kernel message to /var/log/messages,
> making the disk instantly resume after suspended. So for test, the
> syslog daemon should better be temporarily stopped.
> 
> v6:
> Take over from Lin Ming.
> 
> - Instead of put the device into autosuspend state in
>   blk_post_runtime_suspend, do it when the last request is finished.
>   This can also solve the problem illustrated below:
> 
>       thread A				      thread B
> |suspend timer expired			|
> |  ... ...				|a new request comes in,
> |  ... ...				|blk_pm_add_request
> |  ... ...				|skip request_resume due to
> |  ... ...				|q->status is still RPM_ACTIVE
> |  rpm_suspend				|  ... ...
> |    scsi_runtime_suspend		|  ... ...
> |      blk_pre_runtime_suspend		|  ... ...
> |      return -EBUSY due to nr_pending	|  ... ...
> |  rpm_suspend done			|  ... ...
> |					|    blk_pm_put_request, mark last busy
> 
> But no more trigger point, and the device will stay at RPM_ACTIVE state.
> Run pm_runtime_autosuspend after the last request is finished solved
> this problem.

This doesn't look like the best solution, because it involves adding a 
nontrivial routine (pm_runtime_autosuspend) to a hot path.

How about this instead?  When blk_pre_runtime_suspend returns -EBUSY,
have it do a mark-last-busy.  Then rpm_suspend will automatically
reschedule the autosuspend for later.

> - Requests which have the REQ_PM flag should not involve nr_pending
>   counting, or we may lose the condition to resume the device:
>   Suppose queue is active and nr_pending is 0. Then a REQ_PM request
>   comes and nr_pending will be increased to 1, but since the request has
>   REQ_PM flag, it will not cause resume. Before it is finished, a normal
>   request comes in, and since nr_pending is 1 now, it will not trigger
>   the resume of the device either. Bug.
> 
> - Do not quiesce the device in scsi bus level runtime suspend callback.
>   Since the only reason the device is to be runtime suspended is due to
>   no more requests pending for it, quiesce it is pointless.
> 
> - Remove scsi_autopm_* from sd_check_events as we are request driven.
> 
> - Call blk_pm_runtime_init in scsi_sysfs_initialize_dev, so that we do
>   not need to check queue's device in blk_pm_add/put_request.

I think you still need to have that check.  After all, the block layer 
has other users besides the SCSI stack, and those users don't call 
blk_pm_runtime_init.

> - Do not mark last busy and initiate an autosuspend for the device in
>   blk_pm_runtime_init function.
> 
> - Do not mark last busy and initiate an autosuspend for the device in
>   block_post_runtime_resume, as when the request that triggered the
>   resume finished, the blk_pm_put_request will mark last busy and
>   initiate an autosuspend.

If you make the change that I recommended above then this is still 
necessary.

Alan Stern

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

[Alan Stern]

On Sun, 6 Jan 2013, Aaron Lu wrote:

> From: Lin Ming <ming.m.lin@intel.com>
> 
> Add runtime pm helper functions:
> 
> void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
>   - Initialization function for drivers to call.
> 
> 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.
> 
> 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 __blk_run_queue.
>     Otherwise set q->rpm_status to RPM_SUSPENDED.
> 
> [aaron.lu@intel.com: do not touch last busy in these helper functions]
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

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

This doesn't belong here.  Clients of the block layer don't need to 
know about pm_runtime.h.  Move this #include to block/blk-core.c.

Alan Stern

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

[Alan Stern]

On Sun, 6 Jan 2013, Aaron Lu wrote:

> From: Lin Ming <ming.m.lin@intel.com>
> 
> When a request is added:
>     If device is suspended or is suspending and the request is not a
>     PM request, resume the device.
> 
> When the last request finishes:
>     Call pm_runtime_mark_last_busy() and pm_runtime_autosuspend().
> 
> When pick a request:
>     If device is resuming/suspending, then only PM request is allowed to go.
>     Return NULL for other cases.
> 
> [aaron.lu@intel.com: PM request does not involve nr_pending counting]
> [aaron.lu@intel.com: No need to check q->dev]
> [aaron.lu@intel.com: Autosuspend when the last request finished]
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -974,6 +974,40 @@ 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 *rq)
> +{
> +	if (!(rq->cmd_flags & REQ_PM) && !--rq->q->nr_pending) {
> +		pm_runtime_mark_last_busy(rq->q->dev);
> +		pm_runtime_autosuspend(rq->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 *rq)
> +{
> +	if (!(rq->cmd_flags & REQ_PM))
> +		rq->q->nr_pending--;
> +}
> +
> +static inline void blk_pm_add_request(struct request_queue *q,
> +	struct request *rq)
> +{
> +	if (!(rq->cmd_flags & REQ_PM) &&
> +	    q->nr_pending++ == 0 &&
> +	    (q->rpm_status == RPM_SUSPENDED ||
> +	     q->rpm_status == RPM_SUSPENDING))
> +		pm_request_resume(q->dev);
> +}

These routines also don't belong in include/linux.  And they don't need 
to be marked inline.

Alan Stern

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

[Aaron Lu]

On 01/08/2013 01:11 AM, Alan Stern wrote:
> On Sun, 6 Jan 2013, Aaron Lu wrote:
> 
>> In August 2010, Jens and Alan discussed about "Runtime PM and the block
>> layer". http://marc.info/?t=128259108400001&r=1&w=2
>> And then Alan has given a detailed implementation guide:
>> http://marc.info/?l=linux-scsi&m=133727953625963&w=2
>>
>> To test:
>> # 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 10000 > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/autosuspend_delay_ms
>> # echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata1/host0/target0:0:0/0:0:0:0/power/control
>> Then you'll see sda is suspended after 10secs idle.
>>
>> [ 1767.680192] sd 2:0:0:0: [sda] Synchronizing SCSI cache
>> [ 1767.680317] sd 2:0:0:0: [sda] Stopping disk
>>
>> And if you do some IO, it will resume immediately.
>> [ 1791.052438] sd 2:0:0:0: [sda] Starting disk
>>
>> For test, I often set the autosuspend time to 1 second. If you are using
>> a GUI, the 10 seconds delay may be too long that the disk can not enter
>> runtime suspended state.
>>
>> Note that sd's runtime suspend callback will dump some kernel messages
>> and the syslog daemon will write kernel message to /var/log/messages,
>> making the disk instantly resume after suspended. So for test, the
>> syslog daemon should better be temporarily stopped.
>>
>> v6:
>> Take over from Lin Ming.
>>
>> - Instead of put the device into autosuspend state in
>>   blk_post_runtime_suspend, do it when the last request is finished.
>>   This can also solve the problem illustrated below:
>>
>>       thread A				      thread B
>> |suspend timer expired			|
>> |  ... ...				|a new request comes in,
>> |  ... ...				|blk_pm_add_request
>> |  ... ...				|skip request_resume due to
>> |  ... ...				|q->status is still RPM_ACTIVE
>> |  rpm_suspend				|  ... ...
>> |    scsi_runtime_suspend		|  ... ...
>> |      blk_pre_runtime_suspend		|  ... ...
>> |      return -EBUSY due to nr_pending	|  ... ...
>> |  rpm_suspend done			|  ... ...
>> |					|    blk_pm_put_request, mark last busy
>>
>> But no more trigger point, and the device will stay at RPM_ACTIVE state.
>> Run pm_runtime_autosuspend after the last request is finished solved
>> this problem.
> 
> This doesn't look like the best solution, because it involves adding a 
> nontrivial routine (pm_runtime_autosuspend) to a hot path.

Oh right, I didn't realize this. Thanks for pointing this out.

> 
> How about this instead?  When blk_pre_runtime_suspend returns -EBUSY,
> have it do a mark-last-busy.  Then rpm_suspend will automatically
> reschedule the autosuspend for later.

Yes, this is better.

> 
>> - Requests which have the REQ_PM flag should not involve nr_pending
>>   counting, or we may lose the condition to resume the device:
>>   Suppose queue is active and nr_pending is 0. Then a REQ_PM request
>>   comes and nr_pending will be increased to 1, but since the request has
>>   REQ_PM flag, it will not cause resume. Before it is finished, a normal
>>   request comes in, and since nr_pending is 1 now, it will not trigger
>>   the resume of the device either. Bug.
>>
>> - Do not quiesce the device in scsi bus level runtime suspend callback.
>>   Since the only reason the device is to be runtime suspended is due to
>>   no more requests pending for it, quiesce it is pointless.
>>
>> - Remove scsi_autopm_* from sd_check_events as we are request driven.
>>
>> - Call blk_pm_runtime_init in scsi_sysfs_initialize_dev, so that we do
>>   not need to check queue's device in blk_pm_add/put_request.
> 
> I think you still need to have that check.  After all, the block layer 
> has other users besides the SCSI stack, and those users don't call 
> blk_pm_runtime_init.

Right...

So this also reminds me that as long as CONFIG_PM_RUNTIME is selected,
the blk_pm_add/put/peek_request functions will be in the block IO path.
Shall we introduce a new config option to selectively build block
runtime PM functionality? something like CONFIG_BLK_PM_RUNTIME perhaps?

Just some condition checks in those functions, not sure if it is worth a
new config though. Please suggest, thanks.

> 
>> - Do not mark last busy and initiate an autosuspend for the device in
>>   blk_pm_runtime_init function.
>>
>> - Do not mark last busy and initiate an autosuspend for the device in
>>   block_post_runtime_resume, as when the request that triggered the
>>   resume finished, the blk_pm_put_request will mark last busy and
>>   initiate an autosuspend.
> 
> If you make the change that I recommended above then this is still 
> necessary.

Yes, they are needed. Thanks!

-Aaron

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

[Aaron Lu]

On 01/08/2013 01:21 AM, Alan Stern wrote:
> On Sun, 6 Jan 2013, Aaron Lu wrote:
> 
>> From: Lin Ming <ming.m.lin@intel.com>
>>
>> When a request is added:
>>     If device is suspended or is suspending and the request is not a
>>     PM request, resume the device.
>>
>> When the last request finishes:
>>     Call pm_runtime_mark_last_busy() and pm_runtime_autosuspend().
>>
>> When pick a request:
>>     If device is resuming/suspending, then only PM request is allowed to go.
>>     Return NULL for other cases.
>>
>> [aaron.lu@intel.com: PM request does not involve nr_pending counting]
>> [aaron.lu@intel.com: No need to check q->dev]
>> [aaron.lu@intel.com: Autosuspend when the last request finished]
>> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
>> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -974,6 +974,40 @@ 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 *rq)
>> +{
>> +	if (!(rq->cmd_flags & REQ_PM) && !--rq->q->nr_pending) {
>> +		pm_runtime_mark_last_busy(rq->q->dev);
>> +		pm_runtime_autosuspend(rq->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 *rq)
>> +{
>> +	if (!(rq->cmd_flags & REQ_PM))
>> +		rq->q->nr_pending--;
>> +}
>> +
>> +static inline void blk_pm_add_request(struct request_queue *q,
>> +	struct request *rq)
>> +{
>> +	if (!(rq->cmd_flags & REQ_PM) &&
>> +	    q->nr_pending++ == 0 &&
>> +	    (q->rpm_status == RPM_SUSPENDED ||
>> +	     q->rpm_status == RPM_SUSPENDING))
>> +		pm_request_resume(q->dev);
>> +}
> 
> These routines also don't belong in include/linux.  And they don't need 
> to be marked inline.

OK, will move them.

What about create a new file blk-pm.c for all these block pm related
code?

Thanks,
Aaron

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

[Alan Stern]

On Tue, 8 Jan 2013, Aaron Lu wrote:

> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -974,6 +974,40 @@ 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 *rq)
> >> +{
> >> +	if (!(rq->cmd_flags & REQ_PM) && !--rq->q->nr_pending) {
> >> +		pm_runtime_mark_last_busy(rq->q->dev);
> >> +		pm_runtime_autosuspend(rq->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 *rq)
> >> +{
> >> +	if (!(rq->cmd_flags & REQ_PM))
> >> +		rq->q->nr_pending--;
> >> +}
> >> +
> >> +static inline void blk_pm_add_request(struct request_queue *q,
> >> +	struct request *rq)
> >> +{
> >> +	if (!(rq->cmd_flags & REQ_PM) &&
> >> +	    q->nr_pending++ == 0 &&
> >> +	    (q->rpm_status == RPM_SUSPENDED ||
> >> +	     q->rpm_status == RPM_SUSPENDING))
> >> +		pm_request_resume(q->dev);
> >> +}
> > 
> > These routines also don't belong in include/linux.  And they don't need 
> > to be marked inline.
> 
> OK, will move them.
> 
> What about create a new file blk-pm.c for all these block pm related
> code?

Sure, go ahead if Jens doesn't mind.  Alternatively, you could put each
of these functions in the file where it gets used.

Just as importantly, all of the public routines added in patch 2/4 to
blk-core.c should have kerneldoc explaining how and where to use them.  
In particular, the kerneldoc for blk_pm_runtime_init() has to mention
that the block runtime PM implementation works only for drivers that
use request structures for their I/O; it doesn't work for drivers that
use bio's directly.

Alan Stern

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

[Alan Stern]

On Tue, 8 Jan 2013, Aaron Lu wrote:

> So this also reminds me that as long as CONFIG_PM_RUNTIME is selected,
> the blk_pm_add/put/peek_request functions will be in the block IO path.
> Shall we introduce a new config option to selectively build block
> runtime PM functionality? something like CONFIG_BLK_PM_RUNTIME perhaps?
> 
> Just some condition checks in those functions, not sure if it is worth a
> new config though. Please suggest, thanks.

I don't think it is needed.  The new routines will be very quick when 
blk_pm_runtime_init() hasn't been called, once you add back the checks 
for the queue's device.

Alan Stern

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

[Aaron Lu]

On Tue, Jan 08, 2013 at 10:22:45AM -0500, Alan Stern wrote:
> Just as importantly, all of the public routines added in patch 2/4 to
> blk-core.c should have kerneldoc explaining how and where to use them.  
> In particular, the kerneldoc for blk_pm_runtime_init() has to mention
> that the block runtime PM implementation works only for drivers that
> use request structures for their I/O; it doesn't work for drivers that
> use bio's directly.

How about the following description for them?

/**
 * blk_pm_runtime_init - Block layer runtime PM initialization routine
 * @q: the queue of the device
 * @dev: the device the queue belongs to
 *
 * Description:
 *    Initialize runtime PM related fields for @q and start auto suspend
 *    for @dev. Drivers that want to take advantage of request based runtime
 *    PM should call this function after @dev has been initialized, and its
 *    request queue @q has been allocated, and runtime PM for it is not
 *    ready yet(either disabled/forbidden or its usage count >= 0).
 *
 *    The block layer runtime PM is request based, so only works for drivers
 *    that use request as their IO unit instead of those directly use bio's.
 */

/**
 * blk_pre_runtime_suspend - Pre runtime suspend check
 * @q: the queue of the device
 *
 * Description:
 *    This function will check if runtime suspend is allowed for the device
 *    by examining if there are any requests pending in the queue. If there
 *    are requests pending, the device can not be runtime suspended; otherwise,
 *    the queue's status will be updated to SUSPENDING and the driver can
 *    proceed to suspend the device.
 *
 *    For the not allowed case, we mark last busy for the device so that
 *    runtime PM core will try to autosuspend it some time later.
 *
 *    This function should be called in the device's runtime suspend callback,
 *    before its runtime suspend function is called.
 *
 * Return:
 *    0		- OK to runtime suspend the device
 *    -EBUSY	- Device should not be runtime suspended
 */

/**
 * blk_post_runtime_suspend - Post runtime suspend processing
 * @q: the queue of the device
 * @err: return value of the device's runtime suspend function
 *
 * Description:
 *    Update the queue's runtime status according to the return value of the
 *    device's runtime suspend function.
 *
 *    This function should be called in the device's runtime suspend callback,
 *    after its runtime suspend function is called.
 */

/**
 * blk_pre_runtime_resume - Pre runtime resume processing
 * @q: the queue of the device
 *
 * Description:
 *    Update the queue's runtime status to RESUMING in preparation for the
 *    runtime resume of the device.
 *
 *    This function should be called in the device's runtime resume callback,
 *    before its runtime resume function is called.
 */

/**
 * blk_post_runtime_resume - Post runtime resume processing
 * @q: the queue of the device
 * @err: return value of the device's runtime resume function
 *
 * Description:
 *    Update the queue's runtime status according to the return value of the
 *    device's runtime resume function. If it is successfully resumed, process
 *    the requests that are queued into the device's queue when it is resuming
 *    and then mark last busy and initiate autosuspend for it.
 *
 *    This function should be called in the device's runtime resume callback,
 *    after its runtime resume function is called.
 */

Please feel free to suggest, thanks.

-Aaron

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

[Aaron Lu]

On Tue, Jan 08, 2013 at 10:27:54AM -0500, Alan Stern wrote:
> On Tue, 8 Jan 2013, Aaron Lu wrote:
> 
> > So this also reminds me that as long as CONFIG_PM_RUNTIME is selected,
> > the blk_pm_add/put/peek_request functions will be in the block IO path.
> > Shall we introduce a new config option to selectively build block
> > runtime PM functionality? something like CONFIG_BLK_PM_RUNTIME perhaps?
> > 
> > Just some condition checks in those functions, not sure if it is worth a
> > new config though. Please suggest, thanks.
> 
> I don't think it is needed.  The new routines will be very quick when 
> blk_pm_runtime_init() hasn't been called, once you add back the checks 
> for the queue's device.

Is it necessary to also add the q->dev check in the following case?

static void blk_pm_requeue_request(struct request *rq)
{
	if (rq->q->dev && !(rq->cmd_flags & REQ_PM))
		rq->q->nr_pending--;
}

So that we do not have a meanlingless value of nr_pending for those
drivers that don't use block runtime PM, and this also has the benefit
of making those drivers return early.

Thanks,
Aaron

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

[Alan Stern]

On Mon, 14 Jan 2013, Aaron Lu wrote:

> On Tue, Jan 08, 2013 at 10:27:54AM -0500, Alan Stern wrote:
> > On Tue, 8 Jan 2013, Aaron Lu wrote:
> > 
> > > So this also reminds me that as long as CONFIG_PM_RUNTIME is selected,
> > > the blk_pm_add/put/peek_request functions will be in the block IO path.
> > > Shall we introduce a new config option to selectively build block
> > > runtime PM functionality? something like CONFIG_BLK_PM_RUNTIME perhaps?
> > > 
> > > Just some condition checks in those functions, not sure if it is worth a
> > > new config though. Please suggest, thanks.
> > 
> > I don't think it is needed.  The new routines will be very quick when 
> > blk_pm_runtime_init() hasn't been called, once you add back the checks 
> > for the queue's device.
> 
> Is it necessary to also add the q->dev check in the following case?
> 
> static void blk_pm_requeue_request(struct request *rq)
> {
> 	if (rq->q->dev && !(rq->cmd_flags & REQ_PM))
> 		rq->q->nr_pending--;
> }
> 
> So that we do not have a meanlingless value of nr_pending for those
> drivers that don't use block runtime PM, and this also has the benefit
> of making those drivers return early.

It doesn't really matter.  Since nr_pending doesn't get used when 
q->dev isn't set, it doesn't hurt for it to have a meaningless value.

Alan Stern

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

[Alan Stern]

On Mon, 14 Jan 2013, Aaron Lu wrote:

> On Tue, Jan 08, 2013 at 10:22:45AM -0500, Alan Stern wrote:
> > Just as importantly, all of the public routines added in patch 2/4 to
> > blk-core.c should have kerneldoc explaining how and where to use them.  
> > In particular, the kerneldoc for blk_pm_runtime_init() has to mention
> > that the block runtime PM implementation works only for drivers that
> > use request structures for their I/O; it doesn't work for drivers that
> > use bio's directly.
> 
> How about the following description for them?

Overall this is very good.

> /**
>  * blk_pm_runtime_init - Block layer runtime PM initialization routine
>  * @q: the queue of the device
>  * @dev: the device the queue belongs to
>  *
>  * Description:
>  *    Initialize runtime PM related fields for @q and start auto suspend
>  *    for @dev. Drivers that want to take advantage of request based runtime
>  *    PM should call this function after @dev has been initialized, and its
>  *    request queue @q has been allocated, and runtime PM for it is not
>  *    ready yet(either disabled/forbidden or its usage count >= 0).
>  *
>  *    The block layer runtime PM is request based, so only works for drivers
>  *    that use request as their IO unit instead of those directly use bio's.
>  */
> 
> /**
>  * blk_pre_runtime_suspend - Pre runtime suspend check
>  * @q: the queue of the device
>  *
>  * Description:
>  *    This function will check if runtime suspend is allowed for the device
>  *    by examining if there are any requests pending in the queue. If there
>  *    are requests pending, the device can not be runtime suspended; otherwise,
>  *    the queue's status will be updated to SUSPENDING and the driver can
>  *    proceed to suspend the device.
>  *
>  *    For the not allowed case, we mark last busy for the device so that
>  *    runtime PM core will try to autosuspend it some time later.
>  *
>  *    This function should be called in the device's runtime suspend callback,
>  *    before its runtime suspend function is called.

This doesn't quite make sense, because the runtime_suspend callback 
_is_ the runtime-suspend function.  How about "... should be called 
near the start of the device's runtime_suspend callback."?

A similar comment applies to the other functions.

>  *
>  * Return:
>  *    0		- OK to runtime suspend the device
>  *    -EBUSY	- Device should not be runtime suspended
>  */
> 
> /**
>  * blk_post_runtime_suspend - Post runtime suspend processing
>  * @q: the queue of the device
>  * @err: return value of the device's runtime suspend function
>  *
>  * Description:
>  *    Update the queue's runtime status according to the return value of the
>  *    device's runtime suspend function.
>  *
>  *    This function should be called in the device's runtime suspend callback,
>  *    after its runtime suspend function is called.
>  */
> 
> /**
>  * blk_pre_runtime_resume - Pre runtime resume processing
>  * @q: the queue of the device
>  *
>  * Description:
>  *    Update the queue's runtime status to RESUMING in preparation for the
>  *    runtime resume of the device.
>  *
>  *    This function should be called in the device's runtime resume callback,
>  *    before its runtime resume function is called.
>  */
> 
> /**
>  * blk_post_runtime_resume - Post runtime resume processing
>  * @q: the queue of the device
>  * @err: return value of the device's runtime resume function
>  *
>  * Description:
>  *    Update the queue's runtime status according to the return value of the
>  *    device's runtime resume function. If it is successfully resumed, process
>  *    the requests that are queued into the device's queue when it is resuming
>  *    and then mark last busy and initiate autosuspend for it.
>  *
>  *    This function should be called in the device's runtime resume callback,
>  *    after its runtime resume function is called.
>  */
> 
> Please feel free to suggest, thanks.

I would hypenate some of these words, such as "runtime-PM-related
fields" or "request-based runtime PM".  Also, "runtime_suspend" and 
"runtime_resume" generally should have either a '_' or a '-'.

But that's a very minor point; your descriptions are quite good.

Alan Stern