| Message ID | 1568194890-24439-2-git-send-email-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] scsi: core: allow auto suspend override by low-level driver | expand |
> > Rework from previous work by: > Sujit Reddy Thumma <sthumma@codeaurora.org> > > Until now the scsi mid-layer forbids runtime suspend till userspace enables it. > This is mainly to quarantine some disks with broken runtime power > management or have high latencies executing suspend resume callbacks. If > the userspace doesn't enable the runtime suspend the underlying hardware > will be always on even when it is not doing any useful work and thus wasting > power. > > Some low-level drivers for the controllers can efficiently use runtime power > management to reduce power consumption and improve battery life. > Allow runtime suspend parameters override within the LLD itself instead of > waiting for userspace to control the power management. > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> Reviewed-by: Avri Altman <avri.altman@wdc.com> Stanley hi, Your series looks fine. I added some comments/questions - feel free to ignore it. Thanks, Avri > --- > drivers/scsi/scsi_sysfs.c | 3 ++- > drivers/scsi/sd.c | 3 +++ > include/scsi/scsi_device.h | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index > 64c96c7828ee..66a8a5c74352 100644 > --- a/drivers/scsi/scsi_sysfs.c > +++ b/drivers/scsi/scsi_sysfs.c > @@ -1300,7 +1300,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) > device_enable_async_suspend(&sdev->sdev_gendev); > scsi_autopm_get_target(starget); > pm_runtime_set_active(&sdev->sdev_gendev); > - pm_runtime_forbid(&sdev->sdev_gendev); > + if (sdev->rpm_autosuspend_delay <= 0) > + pm_runtime_forbid(&sdev->sdev_gendev); > pm_runtime_enable(&sdev->sdev_gendev); > scsi_autopm_put_target(starget); > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index > 149d406aacc9..2218d57c4c0c 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev) > } > > blk_pm_runtime_init(sdp->request_queue, dev); > + if (sdp->rpm_autosuspend_delay > 0) > + pm_runtime_set_autosuspend_delay(dev, > + Redundant line ? > + sdp->rpm_autosuspend_delay); Don't you need to call now pm_runtime_use_autosuspend() ? > device_add_disk(dev, gd, NULL); > if (sdkp->capacity) > sd_dif_config_host(sdkp); diff --git a/include/scsi/scsi_device.h > b/include/scsi/scsi_device.h index 202f4d6a4342..133b282fae5a 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -199,7 +199,7 @@ struct scsi_device { > unsigned broken_fua:1; /* Don't set FUA bit */ > unsigned lun_in_cdb:1; /* Store LUN bits in CDB[1] */ > unsigned unmap_limit_for_ws:1; /* Use the UNMAP limit for WRITE > SAME */ > - > + int rpm_autosuspend_delay; Can suspend be negative? > atomic_t disk_events_disable_depth; /* disable depth for disk events */ > > DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* > supported events */ > -- > 2.18.0
Hi Avri, > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index > > 149d406aacc9..2218d57c4c0c 100644 > > --- a/drivers/scsi/sd.c > > +++ b/drivers/scsi/sd.c > > @@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev) > > } > > > > blk_pm_runtime_init(sdp->request_queue, dev); > > + if (sdp->rpm_autosuspend_delay > 0) > > + pm_runtime_set_autosuspend_delay(dev, > > + > Redundant line ? checkpatch reported "WARNING:LONG_LINE:line over 80 characters" when I made this as oneline : ( > > + sdp->rpm_autosuspend_delay); > Don't you need to call now pm_runtime_use_autosuspend() ? dev->power.user_autosuspend was set by blk_pm_runtime_init() above, thus pm_runtime_use_autosuspend() is not necessary here. > > > device_add_disk(dev, gd, NULL); > > if (sdkp->capacity) > > sd_dif_config_host(sdkp); diff --git a/include/scsi/scsi_device.h > > b/include/scsi/scsi_device.h index 202f4d6a4342..133b282fae5a 100644 > > --- a/include/scsi/scsi_device.h > > +++ b/include/scsi/scsi_device.h > > @@ -199,7 +199,7 @@ struct scsi_device { > > unsigned broken_fua:1; /* Don't set FUA bit */ > > unsigned lun_in_cdb:1; /* Store LUN bits in CDB[1] */ > > unsigned unmap_limit_for_ws:1; /* Use the UNMAP limit for WRITE > > SAME */ > > - > > + int rpm_autosuspend_delay; > Can suspend be negative? Yes, however negative delay value will block rpm. Here we just use the same type as parameter "delay" of pm_runtime_set_autosuspend() even though we do not set it as negative value in this version. But thank you so much to remind me that pm_runtime_set_autosuspend_delay() can accept "zero" delay so we shall allow "zero" sdev->rpm_autosuspend_delay as well. I will fix it in v2. > > > atomic_t disk_events_disable_depth; /* disable depth for disk events */ > > > > DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* > > supported events */ > > -- > > 2.18.0 > Thanks, Stanley
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c index 64c96c7828ee..66a8a5c74352 100644 --- a/drivers/scsi/scsi_sysfs.c +++ b/drivers/scsi/scsi_sysfs.c @@ -1300,7 +1300,8 @@ int scsi_sysfs_add_sdev(struct scsi_device *sdev) device_enable_async_suspend(&sdev->sdev_gendev); scsi_autopm_get_target(starget); pm_runtime_set_active(&sdev->sdev_gendev); - pm_runtime_forbid(&sdev->sdev_gendev); + if (sdev->rpm_autosuspend_delay <= 0) + pm_runtime_forbid(&sdev->sdev_gendev); pm_runtime_enable(&sdev->sdev_gendev); scsi_autopm_put_target(starget); diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 149d406aacc9..2218d57c4c0c 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -3371,6 +3371,9 @@ static int sd_probe(struct device *dev) } blk_pm_runtime_init(sdp->request_queue, dev); + if (sdp->rpm_autosuspend_delay > 0) + pm_runtime_set_autosuspend_delay(dev, + sdp->rpm_autosuspend_delay); device_add_disk(dev, gd, NULL); if (sdkp->capacity) sd_dif_config_host(sdkp); diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 202f4d6a4342..133b282fae5a 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -199,7 +199,7 @@ struct scsi_device { unsigned broken_fua:1; /* Don't set FUA bit */ unsigned lun_in_cdb:1; /* Store LUN bits in CDB[1] */ unsigned unmap_limit_for_ws:1; /* Use the UNMAP limit for WRITE SAME */ - + int rpm_autosuspend_delay; atomic_t disk_events_disable_depth; /* disable depth for disk events */ DECLARE_BITMAP(supported_events, SDEV_EVT_MAXBITS); /* supported events */
Rework from previous work by: Sujit Reddy Thumma <sthumma@codeaurora.org> Until now the scsi mid-layer forbids runtime suspend till userspace enables it. This is mainly to quarantine some disks with broken runtime power management or have high latencies executing suspend resume callbacks. If the userspace doesn't enable the runtime suspend the underlying hardware will be always on even when it is not doing any useful work and thus wasting power. Some low-level drivers for the controllers can efficiently use runtime power management to reduce power consumption and improve battery life. Allow runtime suspend parameters override within the LLD itself instead of waiting for userspace to control the power management. Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/scsi_sysfs.c | 3 ++- drivers/scsi/sd.c | 3 +++ include/scsi/scsi_device.h | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)