| Message ID | 1359538494-2936-5-git-send-email-aaron.lu@intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); }