[PATCH v7 0/4] block layer runtime pm

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

A git repo for it:
https://github.com/aaronlu/linux.git blockpm_v7

v7:
- Add kernel doc for block layer runtime PM API as suggested by
  Alan Stern;

- Add back check for q->dev, as that serves as a flag if driver
  is using block layer runtime PM;

- Do not auto suspend when last request is finished, as that's a hot
  path and auto suspend is not a trivial function. Instead, mark last
  busy in pre_suspend so that runtim PM core will retry suspend some
  time later to solve the 1st problem demostrated in v6, suggested by
  Alan Stern.

- Move block layer runtime PM strtegy functions to where they are
  needed instead of in include/linux/blkdev.h as suggested by Alan
  Stern since clients of block layer do not need to know those
  functions.

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           | 175 +++++++++++++++++++++++++++++++++++++++++++++
 block/elevator.c           |  26 +++++++
 drivers/scsi/scsi_lib.c    |   9 ++-
 drivers/scsi/scsi_pm.c     |  35 +++++----
 drivers/scsi/scsi_sysfs.c  |   2 +
 drivers/scsi/sd.c          |  21 ++----
 include/linux/blk_types.h  |   2 +
 include/linux/blkdev.h     |  27 +++++++
 include/scsi/scsi_device.h |  17 +++--
 9 files changed, 277 insertions(+), 37 deletions(-)

-- 
1.8.0.2

[PATCH v7 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, such requests
will not change the device request queue's runtime status. It is
intended to be used in driver's runtime PM callback, so that driver can
perform some IO to the device there with the queue's runtime status
unaffected. e.g. in SCSI disk's runtime suspend callback, the disk will
be put into stopped power state, and this require send a command to the
device. Such command processing should not change the disk's runtime
status.

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

[PATCH v7 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, mark last busy and 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, then mark last busy and autosuspend.
    Otherwise set q->rpm_status to RPM_SUSPENDED.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/blk-core.c       | 136 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |  27 ++++++++++
 2 files changed, 163 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index 66d3168..e441daf 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>
@@ -3043,6 +3044,141 @@ void blk_finish_plug(struct blk_plug *plug)
 }
 EXPORT_SYMBOL(blk_finish_plug);
 
+#ifdef CONFIG_PM_RUNTIME
+/**
+ * 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 can not happen
+ *    yet(either due to 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.
+ */
+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);
+
+/**
+ * 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 near the start of the device's
+ *    runtime_suspend callback.
+ *
+ * Return:
+ *    0		- OK to runtime suspend the device
+ *    -EBUSY	- Device should not be runtime suspended
+ */
+int blk_pre_runtime_suspend(struct request_queue *q)
+{
+	int ret = 0;
+
+	spin_lock_irq(q->queue_lock);
+	if (q->nr_pending) {
+		ret = -EBUSY;
+		pm_runtime_mark_last_busy(q->dev);
+	} else {
+		q->rpm_status = RPM_SUSPENDING;
+	}
+	spin_unlock_irq(q->queue_lock);
+	return ret;
+}
+EXPORT_SYMBOL(blk_pre_runtime_suspend);
+
+/**
+ * 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 near the end of the device's
+ *    runtime_suspend callback.
+ */
+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);
+
+/**
+ * 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 near the start of the device's
+ *    runtime_resume callback.
+ */
+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);
+
+/**
+ * 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 near the end of the device's
+ *    runtime_resume callback.
+ */
+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_runtime_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 78feda9..89d89c7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -361,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
 	 */
@@ -961,6 +967,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.8.0.2

[PATCH v7 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().

When pick a request:
    If device is resuming/suspending, then only PM request is allowed
    to go.

Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
 block/blk-core.c | 39 +++++++++++++++++++++++++++++++++++++++
 block/elevator.c | 26 ++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/block/blk-core.c b/block/blk-core.c
index e441daf..d1bf5dc 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -1264,6 +1264,16 @@ void part_round_stats(int cpu, struct hd_struct *part)
 }
 EXPORT_SYMBOL_GPL(part_round_stats);
 
+#ifdef CONFIG_PM_RUNTIME
+static void blk_pm_put_request(struct request *rq)
+{
+	if (rq->q->dev && !(rq->cmd_flags & REQ_PM) && !--rq->q->nr_pending)
+		pm_runtime_mark_last_busy(rq->q->dev);
+}
+#else
+static inline void blk_pm_put_request(struct request *rq) {}
+#endif
+
 /*
  * queue lock must be held
  */
@@ -1274,6 +1284,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 */
@@ -2051,6 +2063,28 @@ static void blk_account_io_done(struct request *req)
 	}
 }
 
+#ifdef CONFIG_PM_RUNTIME
+/*
+ * Don't process normal requests when queue is suspended
+ * or in the process of suspending/resuming
+ */
+static 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;
+}
+#else
+static inline struct request *blk_pm_peek_request(struct request_queue *q,
+					   struct request *rq)
+{
+	return rq;
+}
+#endif
+
 /**
  * blk_peek_request - peek at the top of a request queue
  * @q: request queue to peek at
@@ -2073,6 +2107,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 11683bb..0e954e3 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>
 
@@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
 		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static void blk_pm_requeue_request(struct request *rq)
+{
+	if (!(rq->cmd_flags & REQ_PM))
+		rq->q->nr_pending--;
+}
+
+static void blk_pm_add_request(struct request_queue *q, struct request *rq)
+{
+	if (q->dev && !(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_requeue_request(struct request *rq) {}
+static inline void blk_pm_add_request(struct request_queue *q,
+				      struct request *rq)
+{
+}
+#endif
+
 void elv_requeue_request(struct request_queue *q, struct request *rq)
 {
 	/*
@@ -529,6 +551,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);
 }
 
@@ -551,6 +575,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) {
-- 
1.8.0.2

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

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 |  2 ++
 drivers/scsi/sd.c         | 12 ------------
 3 files changed, 25 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..fff7637 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -893,6 +893,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 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.8.0.2

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

[Alan Stern]

On Wed, 16 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().
> 
> When pick a request:
>     If device is resuming/suspending, then only PM request is allowed
>     to go.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

Just a couple of minor problems remaining...

> --- a/block/blk-core.c
> +++ b/block/blk-core.c

> @@ -2051,6 +2063,28 @@ static void blk_account_io_done(struct request *req)
>  	}
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME
> +/*
> + * Don't process normal requests when queue is suspended
> + * or in the process of suspending/resuming
> + */
> +static 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;
> +}

You don't check q->dev here, so the result is indefinite for devices
that don't use runtime PM.  (Actually it will work out because
RPM_ACTIVE is defined as 0, but that's a pretty fragile approach.)

Either do check q->dev here, or else explicitly initialize
q->rpm_status when the queue is created.


> --- 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>
>  
> @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
>  		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME
> +static void blk_pm_requeue_request(struct request *rq)
> +{
> +	if (!(rq->cmd_flags & REQ_PM))
> +		rq->q->nr_pending--;
> +}

You don't check q->dev here.  That's okay, but it means that
q->nr_pending will be meaningless or wrong if any I/O takes place
before blk_pm_runtime_init is called.

Therefore the kerneldoc for blk_pm_runtime_init should mention that it
must not be called after any requests have been submitted.  Also
mention that blk_pm_runtime_init enables runtime PM for q->dev, so the 
caller shouldn't do it.

Alan Stern

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

[Aaron Lu]

On Wed, Jan 16, 2013 at 10:30:45AM -0500, Alan Stern wrote:
> On Wed, 16 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().
> > 
> > When pick a request:
> >     If device is resuming/suspending, then only PM request is allowed
> >     to go.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> Just a couple of minor problems remaining...
> 
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> 
> > @@ -2051,6 +2063,28 @@ static void blk_account_io_done(struct request *req)
> >  	}
> >  }
> >  
> > +#ifdef CONFIG_PM_RUNTIME
> > +/*
> > + * Don't process normal requests when queue is suspended
> > + * or in the process of suspending/resuming
> > + */
> > +static 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;
> > +}
> 
> You don't check q->dev here, so the result is indefinite for devices
> that don't use runtime PM.  (Actually it will work out because
> RPM_ACTIVE is defined as 0, but that's a pretty fragile approach.)
> 
> Either do check q->dev here, or else explicitly initialize
> q->rpm_status when the queue is created.

I think I'll check q->dev, which is more clear.

> 
> 
> > --- 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>
> >  
> > @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
> >  		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
> >  }
> >  
> > +#ifdef CONFIG_PM_RUNTIME
> > +static void blk_pm_requeue_request(struct request *rq)
> > +{
> > +	if (!(rq->cmd_flags & REQ_PM))
> > +		rq->q->nr_pending--;
> > +}
> 
> You don't check q->dev here.  That's okay, but it means that
> q->nr_pending will be meaningless or wrong if any I/O takes place
> before blk_pm_runtime_init is called.

Right, so I had better check q->dev here too.

> 
> Therefore the kerneldoc for blk_pm_runtime_init should mention that it
> must not be called after any requests have been submitted.  Also

So with the q->dev check added above, I believe this is not needed.

> mention that blk_pm_runtime_init enables runtime PM for q->dev, so the 
> caller shouldn't do it.

OK, thanks for the suggestion.

-Aaron

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

[Aaron Lu]

On Wed, Jan 16, 2013 at 10:30:45AM -0500, Alan Stern wrote:
> On Wed, 16 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().
> > 
> > When pick a request:
> >     If device is resuming/suspending, then only PM request is allowed
> >     to go.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> Just a couple of minor problems remaining...
> 
> > --- a/block/blk-core.c
> > +++ b/block/blk-core.c
> 
> > @@ -2051,6 +2063,28 @@ static void blk_account_io_done(struct request *req)
> >  	}
> >  }
> >  
> > +#ifdef CONFIG_PM_RUNTIME
> > +/*
> > + * Don't process normal requests when queue is suspended
> > + * or in the process of suspending/resuming
> > + */
> > +static 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;
> > +}
> 
> You don't check q->dev here, so the result is indefinite for devices
> that don't use runtime PM.  (Actually it will work out because
> RPM_ACTIVE is defined as 0, but that's a pretty fragile approach.)
> 
> Either do check q->dev here, or else explicitly initialize
> q->rpm_status when the queue is created.
> 
> 
> > --- 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>
> >  
> > @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
> >  		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
> >  }
> >  
> > +#ifdef CONFIG_PM_RUNTIME
> > +static void blk_pm_requeue_request(struct request *rq)
> > +{
> > +	if (!(rq->cmd_flags & REQ_PM))
> > +		rq->q->nr_pending--;
> > +}
> 
> You don't check q->dev here.  That's okay, but it means that
> q->nr_pending will be meaningless or wrong if any I/O takes place
> before blk_pm_runtime_init is called.
> 
> Therefore the kerneldoc for blk_pm_runtime_init should mention that it
> must not be called after any requests have been submitted.  Also

------------
> mention that blk_pm_runtime_init enables runtime PM for q->dev, so the 
> caller shouldn't do it.

I may misunderstandd this in last email.

We didn't enable runtime PM for the device in blk_pm_runtime_init, just
some auto suspend related settings.

Thanks,
Aaron

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

[Alan Stern]

On Thu, 17 Jan 2013, Aaron Lu wrote:

> > > @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
> > >  		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
> > >  }
> > >  
> > > +#ifdef CONFIG_PM_RUNTIME
> > > +static void blk_pm_requeue_request(struct request *rq)
> > > +{
> > > +	if (!(rq->cmd_flags & REQ_PM))
> > > +		rq->q->nr_pending--;
> > > +}
> > 
> > You don't check q->dev here.  That's okay, but it means that
> > q->nr_pending will be meaningless or wrong if any I/O takes place
> > before blk_pm_runtime_init is called.
> 
> Right, so I had better check q->dev here too.
> 
> > 
> > Therefore the kerneldoc for blk_pm_runtime_init should mention that it
> > must not be called after any requests have been submitted.  Also
> 
> So with the q->dev check added above, I believe this is not needed.

No, it is still needed.  With the q->dev check added, q->nr_pending 
will always be 0 before blk_pm_runtime_init is called.  But if I/O is 
taking place then the number of pending requests really isn't 0.

Either you have to make sure the q->nr_pending is always correct, even 
when runtime PM isn't being used, or else the caller has to make sure 
that no I/O takes place before blk_pm_runtime_init is called.

> ------------
> > mention that blk_pm_runtime_init enables runtime PM for q->dev, so the
> > caller shouldn't do it.
> 
> I may misunderstandd this in last email.
> 
> We didn't enable runtime PM for the device in blk_pm_runtime_init, just
> some auto suspend related settings.

Sorry, yes, you are right.  In fact, the caller has to make sure that 
runtime PM is enabled before calling blk_pm_runtime_init; otherwise the 
autosuspend timer might not work right.

Alan Stern

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

[Aaron Lu]

On Thu, Jan 17, 2013 at 10:11:31AM -0500, Alan Stern wrote:
> On Thu, 17 Jan 2013, Aaron Lu wrote:
> 
> > > > @@ -515,6 +516,27 @@ void elv_bio_merged(struct request_queue *q, struct request *rq,
> > > >  		e->type->ops.elevator_bio_merged_fn(q, rq, bio);
> > > >  }
> > > >  
> > > > +#ifdef CONFIG_PM_RUNTIME
> > > > +static void blk_pm_requeue_request(struct request *rq)
> > > > +{
> > > > +	if (!(rq->cmd_flags & REQ_PM))
> > > > +		rq->q->nr_pending--;
> > > > +}
> > > 
> > > You don't check q->dev here.  That's okay, but it means that
> > > q->nr_pending will be meaningless or wrong if any I/O takes place
> > > before blk_pm_runtime_init is called.
> > 
> > Right, so I had better check q->dev here too.
> > 
> > > 
> > > Therefore the kerneldoc for blk_pm_runtime_init should mention that it
> > > must not be called after any requests have been submitted.  Also
> > 
> > So with the q->dev check added above, I believe this is not needed.
> 
> No, it is still needed.  With the q->dev check added, q->nr_pending 
> will always be 0 before blk_pm_runtime_init is called.  But if I/O is 
> taking place then the number of pending requests really isn't 0.

Right, we can't allow I/O to happen when blk_pm_runtime_init is
executing, or we will have an incorrect nr_pending.

> 
> Either you have to make sure the q->nr_pending is always correct, even 
> when runtime PM isn't being used, or else the caller has to make sure 
> that no I/O takes place before blk_pm_runtime_init is called.

I think we can say:
blk_pm_runtime_init can't be called after any requests have been
submitted but not finished.
Sounds more accurate?

Thanks,
Aaron

> 
> > ------------
> > > mention that blk_pm_runtime_init enables runtime PM for q->dev, so the
> > > caller shouldn't do it.
> > 
> > I may misunderstandd this in last email.
> > 
> > We didn't enable runtime PM for the device in blk_pm_runtime_init, just
> > some auto suspend related settings.
> 
> Sorry, yes, you are right.  In fact, the caller has to make sure that 
> runtime PM is enabled before calling blk_pm_runtime_init; otherwise the 
> autosuspend timer might not work right.
> 
> Alan Stern
> 

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

[Alan Stern]

On Fri, 18 Jan 2013, Aaron Lu wrote:

> > Either you have to make sure the q->nr_pending is always correct, even 
> > when runtime PM isn't being used, or else the caller has to make sure 
> > that no I/O takes place before blk_pm_runtime_init is called.
> 
> I think we can say:
> blk_pm_runtime_init can't be called after any requests have been
> submitted but not finished.
> Sounds more accurate?

Okay.  I think you can add (in parentheses) that in most cases drivers
should call the routine before any I/O has taken place.

Alan Stern

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

[Alan Stern]

On Wed, 16 Jan 2013, 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.
> 
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>

> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -893,6 +893,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,

Someone just asked about the default autosuspend delay, and I realized
your patch series doesn't set one.  Since we don't know the properties
of the disk drive at this point (or even whether the device is a disk
drive), the only safe course is to call

	pm_runtime_set_autosuspend_delay(&sdev->sdev_gendev, -1);

before calling blk_pm_runtime_init().  Then autosuspends will be 
prevented until userspace writes a non-negative value into the device's 
control/autosuspend_delay_ms file.

The kerneldoc for blk_pm_runtime_init() should mention that the caller 
needs to set the autosuspend delay beforehand.

Alan Stern

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

[Aaron Lu]

On 01/18/2013 11:26 PM, Alan Stern wrote:
> On Fri, 18 Jan 2013, Aaron Lu wrote:
>
>>> Either you have to make sure the q->nr_pending is always correct, even
>>> when runtime PM isn't being used, or else the caller has to make sure
>>> that no I/O takes place before blk_pm_runtime_init is called.
>>
>> I think we can say:
>> blk_pm_runtime_init can't be called after any requests have been
>> submitted but not finished.
>> Sounds more accurate?
>
> Okay.  I think you can add (in parentheses) that in most cases drivers
> should call the routine before any I/O has taken place.

The reason I put it that way is, in patch 4, the blk_pm_runtime_init is
called after a request is executed(scsi_probe_lun). I do not want people
get confused by the comments for blk_pm_runtime_init and the example
code in patch 4 where we didn't follow it :-)

Considering ODD's use case, I was thinking of moving the
blk_pm_runtime_init call to sd.c, as sr will not use request based auto
suspend. Probably right before we decrease usage count for the device in
sd_probe_async. What do you think?

Thanks,
Aaron

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

[Alan Stern]

On Sat, 19 Jan 2013, Aaron Lu wrote:

> > Okay.  I think you can add (in parentheses) that in most cases drivers
> > should call the routine before any I/O has taken place.
> 
> The reason I put it that way is, in patch 4, the blk_pm_runtime_init is
> called after a request is executed(scsi_probe_lun). I do not want people
> get confused by the comments for blk_pm_runtime_init and the example
> code in patch 4 where we didn't follow it :-)

Right.

> Considering ODD's use case, I was thinking of moving the
> blk_pm_runtime_init call to sd.c, as sr will not use request based auto
> suspend. Probably right before we decrease usage count for the device in
> sd_probe_async. What do you think?

That makes sense.  But then you should review the changes in scsi_pm.c 
to make sure they will work okay for devices that aren't using 
block-layer runtime PM.

Alan Stern

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

[Aaron Lu]

On 01/19/2013 05:25 AM, Alan Stern wrote:
> On Wed, 16 Jan 2013, 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.
>>
>> Signed-off-by: Lin Ming<ming.m.lin@intel.com>
>> Signed-off-by: Aaron Lu<aaron.lu@intel.com>
>
>> --- a/drivers/scsi/scsi_sysfs.c
>> +++ b/drivers/scsi/scsi_sysfs.c
>> @@ -893,6 +893,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,
>
> Someone just asked about the default autosuspend delay, and I realized
> your patch series doesn't set one.  Since we don't know the properties
> of the disk drive at this point (or even whether the device is a disk
> drive), the only safe course is to call
>
> 	pm_runtime_set_autosuspend_delay(&sdev->sdev_gendev, -1);
>
> before calling blk_pm_runtime_init().  Then autosuspends will be
> prevented until userspace writes a non-negative value into the device's
> control/autosuspend_delay_ms file.

OK, that would be safer, thanks for the suggestion.
I often set the autosuspend delay before allowing runtime PM for the
device, since we forbid it in scsi_sysfs_add_sdev.

>
> The kerneldoc for blk_pm_runtime_init() should mention that the caller
> needs to set the autosuspend delay beforehand.

OK.

Thanks,
Aaron

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

[Aaron Lu]

On Fri, Jan 18, 2013 at 04:25:10PM -0500, Alan Stern wrote:
> On Wed, 16 Jan 2013, 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.
> > 
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
> 
> > --- a/drivers/scsi/scsi_sysfs.c
> > +++ b/drivers/scsi/scsi_sysfs.c
> > @@ -893,6 +893,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,
> 
> Someone just asked about the default autosuspend delay, and I realized
> your patch series doesn't set one.  Since we don't know the properties
> of the disk drive at this point (or even whether the device is a disk
> drive), the only safe course is to call
> 
> 	pm_runtime_set_autosuspend_delay(&sdev->sdev_gendev, -1);
> 
> before calling blk_pm_runtime_init().  Then autosuspends will be 
> prevented until userspace writes a non-negative value into the device's 
> control/autosuspend_delay_ms file.

Shall we do it inside blk_pm_runtime_init? This way, we do not need to
do it for every driver. And for drivers that do know a proper value for
autosuspend delay, they can call pm_runtime_set_autosuspend_delay
somewhere after blk_pm_runtime_init.

Thanks,
Aaron

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

[Aaron Lu]

On Sat, Jan 19, 2013 at 01:11:45PM -0500, Alan Stern wrote:
> On Sat, 19 Jan 2013, Aaron Lu wrote:
> > Considering ODD's use case, I was thinking of moving the
> > blk_pm_runtime_init call to sd.c, as sr will not use request based auto
> > suspend. Probably right before we decrease usage count for the device in
> > sd_probe_async. What do you think?
> 
> That makes sense.  But then you should review the changes in scsi_pm.c 
> to make sure they will work okay for devices that aren't using 
> block-layer runtime PM.

Now that we have two different runtime PM schemes for scsi device, and
I think there are two ways to make them work:

1 Do it all in scsi_pm.c. In bus' runtime PM callback, check if this
  device is using block layer runtime PM API, and act accordingly;
2 Do it in indivisual drivers' runtime PM callback. Bus' runtime PM
  callbacks just call pm_generic_runtime_xxx, and each driver's runtime
  PM callback will need to do what is appropriate for them.

Personally I want to go with option 1, but I would like to hear your
opinion :-)

And for option 1, the code would be like this:

static int scsi_blk_runtime_suspend(struct device *dev)
{
	struct scsi_device *sdev = to_scsi_device(dev);
	int err;

	err = blk_pre_runtime_suspend(sdev->request_queue);
	if (err)
		return err;
	err = pm_generic_runtime_suspend(dev);
	blk_post_runtime_suspend(sdev->request_queue, err);

	return err;
}

static int scsi_dev_runtime_suspend(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
	int err;

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

	return err;
}

static int scsi_runtime_suspend(struct device *dev)
{
	int err = 0;

	dev_dbg(dev, "scsi_runtime_suspend\n");
	if (scsi_is_sdev_device(dev)) {
		struct scsi_device *sdev = to_scsi_device(dev);
		if (sdev->request_queue->dev)
			err = scsi_blk_runtime_suspend(dev);
		else
			err = scsi_dev_runtime_suspend(dev);
	}

	/* Insert hooks here for targets, hosts, and transport classes */

	return err;
}

static int scsi_blk_runtime_resume(struct device *dev)
{
	struct scsi_device *sdev = to_scsi_device(dev);
	int err;

	blk_pre_runtime_resume(sdev->request_queue);
	err = pm_generic_runtime_resume(dev);
	blk_post_runtime_resume(sdev->request_queue, err);

	return err;
}

static int scsi_dev_runtime_resume(struct device *dev)
{
	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
	return scsi_dev_type_resume(dev, pm ? pm->runtime_resume : NULL);
}

static int scsi_runtime_resume(struct device *dev)
{
	int err = 0;

	dev_dbg(dev, "scsi_runtime_resume\n");
	if (scsi_is_sdev_device(dev)) {
		struct scsi_device *sdev = to_scsi_device(dev);
		if (sdev->request_queue->dev)
			err = scsi_blk_runtime_resume(dev);
		else
			err = scsi_dev_runtime_resume(dev);
	}

	/* Insert hooks here for targets, hosts, and transport classes */

	return err;
}

static int scsi_runtime_idle(struct device *dev)
{
	int err;

	dev_dbg(dev, "scsi_runtime_idle\n");

	/* Insert hooks here for targets, hosts, and transport classes */

	if (scsi_is_sdev_device(dev)) {
		struct scsi_device *sdev = to_scsi_device(dev);
		if (sdev->request_queue->dev) {
			pm_runtime_mark_last_busy(dev);
			err = pm_runtime_autosuspend(dev);
		} else {
			err = pm_schedule_suspend(dev, 100);
		}
	} else {
		err = pm_runtime_suspend(dev);
	}
	return err;
}

Please feel free to suggest, thanks.

-Aaron

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

[Alan Stern]

On Mon, 28 Jan 2013, Aaron Lu wrote:

> On Sat, Jan 19, 2013 at 01:11:45PM -0500, Alan Stern wrote:
> > On Sat, 19 Jan 2013, Aaron Lu wrote:
> > > Considering ODD's use case, I was thinking of moving the
> > > blk_pm_runtime_init call to sd.c, as sr will not use request based auto
> > > suspend. Probably right before we decrease usage count for the device in
> > > sd_probe_async. What do you think?
> > 
> > That makes sense.  But then you should review the changes in scsi_pm.c 
> > to make sure they will work okay for devices that aren't using 
> > block-layer runtime PM.
> 
> Now that we have two different runtime PM schemes for scsi device, and
> I think there are two ways to make them work:
> 
> 1 Do it all in scsi_pm.c. In bus' runtime PM callback, check if this
>   device is using block layer runtime PM API, and act accordingly;
> 2 Do it in indivisual drivers' runtime PM callback. Bus' runtime PM
>   callbacks just call pm_generic_runtime_xxx, and each driver's runtime
>   PM callback will need to do what is appropriate for them.
> 
> Personally I want to go with option 1, but I would like to hear your
> opinion :-)

I don't think it matters very much.  Option 1 is okay with me.  James 
might have a stronger feeling one way or the other.

Alan Stern

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

[Alan Stern]

On Mon, 28 Jan 2013, Aaron Lu wrote:

> > Someone just asked about the default autosuspend delay, and I realized
> > your patch series doesn't set one.  Since we don't know the properties
> > of the disk drive at this point (or even whether the device is a disk
> > drive), the only safe course is to call
> > 
> > 	pm_runtime_set_autosuspend_delay(&sdev->sdev_gendev, -1);
> > 
> > before calling blk_pm_runtime_init().  Then autosuspends will be 
> > prevented until userspace writes a non-negative value into the device's 
> > control/autosuspend_delay_ms file.
> 
> Shall we do it inside blk_pm_runtime_init? This way, we do not need to
> do it for every driver. And for drivers that do know a proper value for
> autosuspend delay, they can call pm_runtime_set_autosuspend_delay
> somewhere after blk_pm_runtime_init.

Yes, that seems like a good approach.  Be sure to mention it in the 
kerneldoc.

Alan Stern