* [PATCH v8 1/4] block: add a flag to identify PM request
2013-01-30 9:34 [PATCH v8 0/4] block layer runtime pm Aaron Lu
@ 2013-01-30 9:34 ` Aaron Lu
2013-01-30 9:34 ` [PATCH v8 2/4] block: add runtime pm helpers Aaron Lu
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Aaron Lu @ 2013-01-30 9:34 UTC (permalink / raw)
To: Alan Stern, Jens Axboe, Rafael J. Wysocki, James Bottomley
Cc: linux-pm, linux-scsi, linux-kernel, Aaron Lu, Aaron Lu, Shane Huang
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 sending 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 | 8 ++++----
include/linux/blk_types.h | 2 ++
include/scsi/scsi_device.h | 16 ++++++++++++----
4 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 765398c..23f795f 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..698923f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1424,8 +1424,8 @@ 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 +3021,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 a7f9cba..cc64587 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -394,10 +394,18 @@ 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);
+}
extern void sdev_disable_disk_events(struct scsi_device *sdev);
extern void sdev_enable_disk_events(struct scsi_device *sdev);
--
1.8.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v8 2/4] block: add runtime pm helpers
2013-01-30 9:34 [PATCH v8 0/4] block layer runtime pm Aaron Lu
2013-01-30 9:34 ` [PATCH v8 1/4] block: add a flag to identify PM request Aaron Lu
@ 2013-01-30 9:34 ` Aaron Lu
2013-01-30 15:54 ` Alan Stern
2013-01-30 9:34 ` [PATCH v8 3/4] block: implement runtime pm strategy Aaron Lu
2013-01-30 9:34 ` [PATCH v8 4/4] sd: change to auto suspend mode Aaron Lu
3 siblings, 1 reply; 13+ messages in thread
From: Aaron Lu @ 2013-01-30 9:34 UTC (permalink / raw)
To: Alan Stern, Jens Axboe, Rafael J. Wysocki, James Bottomley
Cc: linux-pm, linux-scsi, linux-kernel, Aaron Lu, Aaron Lu, Shane Huang
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 | 143 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/blkdev.h | 27 ++++++++++
2 files changed, 170 insertions(+)
diff --git a/block/blk-core.c b/block/blk-core.c
index 66d3168..1ec09f6 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,148 @@ 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). In most
+ * cases, driver should call this function before any I/O has taken place.
+ *
+ * This function takes care of setting up using auto suspend for the device,
+ * the autosuspend delay is set to -1 to make runtime suspend impossible
+ * until an updated value is either set by user or by driver. Drivers do
+ * not need to touch other autosuspend settings.
+ *
+ * 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_set_autosuspend_delay(q->dev, -1);
+ 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.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 2/4] block: add runtime pm helpers
2013-01-30 9:34 ` [PATCH v8 2/4] block: add runtime pm helpers Aaron Lu
@ 2013-01-30 15:54 ` Alan Stern
2013-01-31 5:35 ` Aaron Lu
0 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2013-01-30 15:54 UTC (permalink / raw)
To: Aaron Lu
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Wed, 30 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, 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>
> +void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
> +{
> + q->dev = dev;
> + q->rpm_status = RPM_ACTIVE;
> + pm_runtime_set_autosuspend_delay(q->dev, -1);
> + pm_runtime_use_autosuspend(q->dev);
> + pm_runtime_mark_last_busy(q->dev);
> + pm_runtime_autosuspend(q->dev);
This last line is no longer needed. It can't do anything useful, since
autosuspends are disabled (the delay is -1).
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 2/4] block: add runtime pm helpers
2013-01-30 15:54 ` Alan Stern
@ 2013-01-31 5:35 ` Aaron Lu
2013-01-31 15:09 ` Alan Stern
0 siblings, 1 reply; 13+ messages in thread
From: Aaron Lu @ 2013-01-31 5:35 UTC (permalink / raw)
To: Alan Stern
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Wed, Jan 30, 2013 at 10:54:53AM -0500, Alan Stern wrote:
> On Wed, 30 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, 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>
>
> > +void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
> > +{
> > + q->dev = dev;
> > + q->rpm_status = RPM_ACTIVE;
> > + pm_runtime_set_autosuspend_delay(q->dev, -1);
> > + pm_runtime_use_autosuspend(q->dev);
> > + pm_runtime_mark_last_busy(q->dev);
> > + pm_runtime_autosuspend(q->dev);
>
> This last line is no longer needed. It can't do anything useful, since
> autosuspends are disabled (the delay is -1).
Right, thanks.
And the mark_last_busy can probably be removed too, it didn't make much
sense here and we can add "driver should call pm_runtime_mark_last_busy
and pm_runtime_autosuspend in its runtime idle callback" to the kernel
doc. What do you think?
Thanks,
Aaron
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 2/4] block: add runtime pm helpers
2013-01-31 5:35 ` Aaron Lu
@ 2013-01-31 15:09 ` Alan Stern
0 siblings, 0 replies; 13+ messages in thread
From: Alan Stern @ 2013-01-31 15:09 UTC (permalink / raw)
To: Aaron Lu
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Thu, 31 Jan 2013, Aaron Lu wrote:
> > > +void blk_pm_runtime_init(struct request_queue *q, struct device *dev)
> > > +{
> > > + q->dev = dev;
> > > + q->rpm_status = RPM_ACTIVE;
> > > + pm_runtime_set_autosuspend_delay(q->dev, -1);
> > > + pm_runtime_use_autosuspend(q->dev);
> > > + pm_runtime_mark_last_busy(q->dev);
> > > + pm_runtime_autosuspend(q->dev);
> >
> > This last line is no longer needed. It can't do anything useful, since
> > autosuspends are disabled (the delay is -1).
>
> Right, thanks.
> And the mark_last_busy can probably be removed too, it didn't make much
> sense here and we can add "driver should call pm_runtime_mark_last_busy
> and pm_runtime_autosuspend in its runtime idle callback" to the kernel
> doc. What do you think?
Yes, okay.
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v8 3/4] block: implement runtime pm strategy
2013-01-30 9:34 [PATCH v8 0/4] block layer runtime pm Aaron Lu
2013-01-30 9:34 ` [PATCH v8 1/4] block: add a flag to identify PM request Aaron Lu
2013-01-30 9:34 ` [PATCH v8 2/4] block: add runtime pm helpers Aaron Lu
@ 2013-01-30 9:34 ` Aaron Lu
2013-01-30 9:34 ` [PATCH v8 4/4] sd: change to auto suspend mode Aaron Lu
3 siblings, 0 replies; 13+ messages in thread
From: Aaron Lu @ 2013-01-30 9:34 UTC (permalink / raw)
To: Alan Stern, Jens Axboe, Rafael J. Wysocki, James Bottomley
Cc: linux-pm, linux-scsi, linux-kernel, Aaron Lu, Aaron Lu, Shane Huang
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 1ec09f6..82cf678 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->dev && (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..29c5c7e 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->q->dev && !(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.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v8 4/4] sd: change to auto suspend mode
2013-01-30 9:34 [PATCH v8 0/4] block layer runtime pm Aaron Lu
` (2 preceding siblings ...)
2013-01-30 9:34 ` [PATCH v8 3/4] block: implement runtime pm strategy Aaron Lu
@ 2013-01-30 9:34 ` Aaron Lu
2013-01-30 15:38 ` Alan Stern
3 siblings, 1 reply; 13+ messages in thread
From: Aaron Lu @ 2013-01-30 9:34 UTC (permalink / raw)
To: Alan Stern, Jens Axboe, Rafael J. Wysocki, James Bottomley
Cc: linux-pm, linux-scsi, linux-kernel, Aaron Lu, Aaron Lu, Shane Huang
From: Lin Ming <ming.m.lin@intel.com>
Uses block layer runtime pm helper functions in
scsi_runtime_suspend/resume for devices that take advantage of it.
Remove scsi_autopm_* from sd open/release path and check_events path.
Signed-off-by: Lin Ming <ming.m.lin@intel.com>
Signed-off-by: Aaron Lu <aaron.lu@intel.com>
---
drivers/scsi/scsi_pm.c | 79 ++++++++++++++++++++++++++++++++++++++++++--------
drivers/scsi/sd.c | 13 +--------
2 files changed, 68 insertions(+), 24 deletions(-)
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 8f6b12c..6ce00c5 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -144,18 +144,44 @@ static int scsi_bus_restore(struct device *dev)
#ifdef CONFIG_PM_RUNTIME
+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;
- 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);
+ 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 */
@@ -163,14 +189,36 @@ static int scsi_runtime_suspend(struct device *dev)
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;
- 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);
+ 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 */
@@ -185,10 +233,17 @@ 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)) {
+ 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;
}
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 698923f..bf04dbf 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 */
@@ -2838,6 +2826,7 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
sd_printk(KERN_NOTICE, sdkp, "Attached SCSI %sdisk\n",
sdp->removable ? "removable " : "");
+ blk_pm_runtime_init(sdp->request_queue, dev);
scsi_autopm_put_device(sdp);
put_device(&sdkp->dev);
}
--
1.8.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] sd: change to auto suspend mode
2013-01-30 9:34 ` [PATCH v8 4/4] sd: change to auto suspend mode Aaron Lu
@ 2013-01-30 15:38 ` Alan Stern
2013-01-31 5:43 ` Aaron Lu
0 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2013-01-30 15:38 UTC (permalink / raw)
To: Aaron Lu
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Wed, 30 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 for devices that take advantage of it.
>
> Remove scsi_autopm_* from sd open/release path and check_events path.
>
> Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> Signed-off-by: Aaron Lu <aaron.lu@intel.com>
A couple of very minor suggestions...
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -144,18 +144,44 @@ static int scsi_bus_restore(struct device *dev)
>
> #ifdef CONFIG_PM_RUNTIME
>
> +static int scsi_blk_runtime_suspend(struct device *dev)
> +{
> + struct scsi_device *sdev = to_scsi_device(dev);
For this routine and the other new ones, it may be slightly more
efficient to pass both dev and sdev as arguments (this depends on how
smart the compiler's optimizer is). The caller already knows both of
them, after all.
> + 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;
> - 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);
There should be a blank line between the declaration and the
executable code.
> + 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 */
> @@ -163,14 +189,36 @@ static int scsi_runtime_suspend(struct device *dev)
> 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;
Blank line.
> + return scsi_dev_type_resume(dev, pm ? pm->runtime_resume : NULL);
> +}
> +
> 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);
Blank line.
> + 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 */
>
> @@ -185,10 +233,17 @@ 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)) {
> + struct scsi_device *sdev = to_scsi_device(dev);
Blank line.
> + 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;
> }
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] sd: change to auto suspend mode
2013-01-30 15:38 ` Alan Stern
@ 2013-01-31 5:43 ` Aaron Lu
2013-01-31 15:13 ` Alan Stern
0 siblings, 1 reply; 13+ messages in thread
From: Aaron Lu @ 2013-01-31 5:43 UTC (permalink / raw)
To: Alan Stern
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Wed, Jan 30, 2013 at 10:38:26AM -0500, Alan Stern wrote:
> On Wed, 30 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 for devices that take advantage of it.
> >
> > Remove scsi_autopm_* from sd open/release path and check_events path.
> >
> > Signed-off-by: Lin Ming <ming.m.lin@intel.com>
> > Signed-off-by: Aaron Lu <aaron.lu@intel.com>
>
> A couple of very minor suggestions...
>
> > --- a/drivers/scsi/scsi_pm.c
> > +++ b/drivers/scsi/scsi_pm.c
>
> > @@ -144,18 +144,44 @@ static int scsi_bus_restore(struct device *dev)
> >
> > #ifdef CONFIG_PM_RUNTIME
> >
> > +static int scsi_blk_runtime_suspend(struct device *dev)
> > +{
> > + struct scsi_device *sdev = to_scsi_device(dev);
>
> For this routine and the other new ones, it may be slightly more
> efficient to pass both dev and sdev as arguments (this depends on how
> smart the compiler's optimizer is). The caller already knows both of
> them, after all.
What about passing only scsi_device? When device is needed, I can use
&sdev->sdev_gendev. Is this equally efficient?
> > 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);
>
> There should be a blank line between the declaration and the
> executable code.
OK, will change this.
> > @@ -185,10 +233,17 @@ 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)) {
> > + struct scsi_device *sdev = to_scsi_device(dev);
>
> Blank line.
>
> > + 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;
Shall we ignore the return value for these pm_xxx_suspend functions?
I mean we do not need to record the return value for them and return it,
since pm core doesn't care the return value of idle callback.
Thanks,
Aaron
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] sd: change to auto suspend mode
2013-01-31 5:43 ` Aaron Lu
@ 2013-01-31 15:13 ` Alan Stern
2013-02-01 3:19 ` Aaron Lu
0 siblings, 1 reply; 13+ messages in thread
From: Alan Stern @ 2013-01-31 15:13 UTC (permalink / raw)
To: Aaron Lu
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Thu, 31 Jan 2013, Aaron Lu wrote:
> > > +static int scsi_blk_runtime_suspend(struct device *dev)
> > > +{
> > > + struct scsi_device *sdev = to_scsi_device(dev);
> >
> > For this routine and the other new ones, it may be slightly more
> > efficient to pass both dev and sdev as arguments (this depends on how
> > smart the compiler's optimizer is). The caller already knows both of
> > them, after all.
>
> What about passing only scsi_device? When device is needed, I can use
> &sdev->sdev_gendev. Is this equally efficient?
I don't know... The difference is very small in any case. The
routines will probably be inlined automatically.
> > > + 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;
>
> Shall we ignore the return value for these pm_xxx_suspend functions?
> I mean we do not need to record the return value for them and return it,
> since pm core doesn't care the return value of idle callback.
Maybe it will care in a future kernel version. You might as well store
the return code and pass it back.
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] sd: change to auto suspend mode
2013-01-31 15:13 ` Alan Stern
@ 2013-02-01 3:19 ` Aaron Lu
2013-02-01 15:11 ` Alan Stern
0 siblings, 1 reply; 13+ messages in thread
From: Aaron Lu @ 2013-02-01 3:19 UTC (permalink / raw)
To: Alan Stern
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Thu, Jan 31, 2013 at 10:13:05AM -0500, Alan Stern wrote:
> On Thu, 31 Jan 2013, Aaron Lu wrote:
>
> > > > +static int scsi_blk_runtime_suspend(struct device *dev)
> > > > +{
> > > > + struct scsi_device *sdev = to_scsi_device(dev);
> > >
> > > For this routine and the other new ones, it may be slightly more
> > > efficient to pass both dev and sdev as arguments (this depends on how
> > > smart the compiler's optimizer is). The caller already knows both of
> > > them, after all.
> >
> > What about passing only scsi_device? When device is needed, I can use
> > &sdev->sdev_gendev. Is this equally efficient?
>
> I don't know... The difference is very small in any case. The
> routines will probably be inlined automatically.
Indeed, I just checked the .s output of the three cases, they are all
the same. So we just need to care about readability and less of code,
passing only scsi_device seems to be the simplest, are you OK with this?
BTW, the compiler I used is gcc-4.7.2.
>
> > > > + 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;
> >
> > Shall we ignore the return value for these pm_xxx_suspend functions?
> > I mean we do not need to record the return value for them and return it,
> > since pm core doesn't care the return value of idle callback.
>
> Maybe it will care in a future kernel version. You might as well store
> the return code and pass it back.
OK.
Thanks,
Aaron
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v8 4/4] sd: change to auto suspend mode
2013-02-01 3:19 ` Aaron Lu
@ 2013-02-01 15:11 ` Alan Stern
0 siblings, 0 replies; 13+ messages in thread
From: Alan Stern @ 2013-02-01 15:11 UTC (permalink / raw)
To: Aaron Lu
Cc: Jens Axboe, Rafael J. Wysocki, James Bottomley, linux-pm,
linux-scsi, linux-kernel, Aaron Lu, Shane Huang
On Fri, 1 Feb 2013, Aaron Lu wrote:
> On Thu, Jan 31, 2013 at 10:13:05AM -0500, Alan Stern wrote:
> > On Thu, 31 Jan 2013, Aaron Lu wrote:
> >
> > > > > +static int scsi_blk_runtime_suspend(struct device *dev)
> > > > > +{
> > > > > + struct scsi_device *sdev = to_scsi_device(dev);
> > > >
> > > > For this routine and the other new ones, it may be slightly more
> > > > efficient to pass both dev and sdev as arguments (this depends on how
> > > > smart the compiler's optimizer is). The caller already knows both of
> > > > them, after all.
> > >
> > > What about passing only scsi_device? When device is needed, I can use
> > > &sdev->sdev_gendev. Is this equally efficient?
> >
> > I don't know... The difference is very small in any case. The
> > routines will probably be inlined automatically.
>
> Indeed, I just checked the .s output of the three cases, they are all
> the same. So we just need to care about readability and less of code,
> passing only scsi_device seems to be the simplest, are you OK with this?
Yes, that's fine. Thanks for checking it out.
Alan Stern
^ permalink raw reply [flat|nested] 13+ messages in thread