Re: [PATCH v8 4/4] sd: change to auto suspend mode
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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 4/4] sd: change to auto suspend mode
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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 4/4] sd: change to auto suspend mode
From: Lin Ming ming.m@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@intel.com
Signed-off-by: Aaron Lu aaron...@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
Re: [PATCH v8 4/4] sd: change to auto suspend mode
On Wed, 30 Jan 2013, Aaron Lu wrote:
From: Lin Ming ming.m@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@intel.com
Signed-off-by: Aaron Lu aaron...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 4/4] sd: change to auto suspend mode
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@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@intel.com
Signed-off-by: Aaron Lu aaron...@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
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
