| Message ID | 1605249009-13752-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: pm: Leave runtime resume along if block layer PM is enabled | expand |
On 11/12/20 10:30 PM, Can Guo wrote: > If block layer runtime PM is enabled for one SCSI device, then there is > no need to forcibly change the SCSI device and its request queue's runtime > PM status to active in scsi_dev_type_resume(), since block layer PM shall > resume the SCSI device on the demand of bios. Please change "along" into "alone" in the subject of this patch (if that is what you meant). > + if (scsi_is_sdev_device(dev)) { > + struct scsi_device *sdev; > > + sdev = to_scsi_device(dev); A minor comment: I think that "struct scsi_device *sdev = to_scsi_device(dev);" fits on a single line. > + * If block layer runtime PM is enabled for the SCSI device, > + * let block layer PM handle its runtime PM routines. Please change "its runtime PM routines" into "runtime resume" or similar. I think that will make the comment more clear. > + if (sdev->request_queue->dev) > + return err; > + } The 'dev' member only exists in struct request_queue if CONFIG_PM=y so the above won't compile if CONFIG_PM=n. How about adding a function in include/linux/blk-pm.h to check whether or not runtime PM has been enabled? Otherwise this patch looks good to me. Thanks, Bart.
Hi Bart, On 2020-11-15 04:57, Bart Van Assche wrote: > On 11/12/20 10:30 PM, Can Guo wrote: >> If block layer runtime PM is enabled for one SCSI device, then there >> is >> no need to forcibly change the SCSI device and its request queue's >> runtime >> PM status to active in scsi_dev_type_resume(), since block layer PM >> shall >> resume the SCSI device on the demand of bios. > > Please change "along" into "alone" in the subject of this patch (if > that > is what you meant). > Aha, sorry, a typo here. >> + if (scsi_is_sdev_device(dev)) { >> + struct scsi_device *sdev; >> >> + sdev = to_scsi_device(dev); > > A minor comment: I think that "struct scsi_device *sdev = > to_scsi_device(dev);" fits on a single line. > Sure. >> + * If block layer runtime PM is enabled for the SCSI device, >> + * let block layer PM handle its runtime PM routines. > > Please change "its runtime PM routines" into "runtime resume" or > similar. I think that will make the comment more clear. > Yes, thanks. >> + if (sdev->request_queue->dev) >> + return err; >> + } > > The 'dev' member only exists in struct request_queue if CONFIG_PM=y so > the above won't compile if CONFIG_PM=n. How about adding a function in > include/linux/blk-pm.h to check whether or not runtime PM has been > enabled? > You are right. > Otherwise this patch looks good to me. Actually, I am thinking about removing all the pm_runtime_set_active() codes in both scsi_bus_resume_common() and scsi_dev_type_resume() - we don't need to forcibly set the runtime PM status to RPM_ACTIVE for either SCSI host/target or SCSI devices. Whenever we access one SCSI device, either block layer or somewhere in the path (e.g. throgh sg IOCTL, sg_open() calls scsi_autopm_get_device()) should runtime resume the device first, and the runtime PM framework makes sure device's gets resumed as well. Thus, the pm_runtime_set_active() seems redundant. What do you think? Thanks, Can Guo. > > Thanks, > > Bart.
Hi Bart, Resent, typo fixed. On 2020-11-15 04:57, Bart Van Assche wrote: > On 11/12/20 10:30 PM, Can Guo wrote: >> If block layer runtime PM is enabled for one SCSI device, then there >> is >> no need to forcibly change the SCSI device and its request queue's >> runtime >> PM status to active in scsi_dev_type_resume(), since block layer PM >> shall >> resume the SCSI device on the demand of bios. > > Please change "along" into "alone" in the subject of this patch (if > that > is what you meant). > Aha, sorry, a typo here. >> + if (scsi_is_sdev_device(dev)) { >> + struct scsi_device *sdev; >> >> + sdev = to_scsi_device(dev); > > A minor comment: I think that "struct scsi_device *sdev = > to_scsi_device(dev);" fits on a single line. > Sure. >> + * If block layer runtime PM is enabled for the SCSI device, >> + * let block layer PM handle its runtime PM routines. > > Please change "its runtime PM routines" into "runtime resume" or > similar. I think that will make the comment more clear. > Yes, thanks. >> + if (sdev->request_queue->dev) >> + return err; >> + } > > The 'dev' member only exists in struct request_queue if CONFIG_PM=y so > the above won't compile if CONFIG_PM=n. How about adding a function in > include/linux/blk-pm.h to check whether or not runtime PM has been > enabled? > You are right. > Otherwise this patch looks good to me. > Actually, I am thinking about removing all the pm_runtime_set_active() codes in both scsi_bus_resume_common() and scsi_dev_type_resume() - we don't need to forcibly set the runtime PM status to RPM_ACTIVE for either SCSI host/target or SCSI devices. Whenever we access one SCSI device, either block layer or somewhere in the path (e.g. throgh sg IOCTL, sg_open() calls scsi_autopm_get_device()) should runtime resume the device first, and the runtime PM framework makes sure device's parent (and its parent's parent and so on)gets resumed as well. Thus, the pm_runtime_set_active() seems redundant. What do you think? Thanks, Can Guo. > Thanks, > > Bart.
On 11/15/20 5:42 PM, Can Guo wrote: > Actually, I am thinking about removing all the pm_runtime_set_active() > codes in both scsi_bus_resume_common() and scsi_dev_type_resume() - we > don't need to forcibly set the runtime PM status to RPM_ACTIVE for either > SCSI host/target or SCSI devices. > > Whenever we access one SCSI device, either block layer or somewhere in > the path (e.g. throgh sg IOCTL, sg_open() calls scsi_autopm_get_device()) > should runtime resume the device first, and the runtime PM framework makes > sure device's parent (and its parent's parent and so on)gets resumed as > well. > Thus, the pm_runtime_set_active() seems redundant. What do you think? Hi Can, It is not clear to me why the pm_runtime_set_active() calls occur in the scsi_pm.c source file since the block layer automatically activates block devices if necessary. Maybe these calls are a leftover from a time when runtime suspended devices were not resumed automatically by the block layer? Anyway, I'm fine with removing these calls. Thanks, Bart.
Hi Bart, On 2020-11-18 12:38, Bart Van Assche wrote: > On 11/15/20 5:42 PM, Can Guo wrote: >> Actually, I am thinking about removing all the pm_runtime_set_active() >> codes in both scsi_bus_resume_common() and scsi_dev_type_resume() - we >> don't need to forcibly set the runtime PM status to RPM_ACTIVE for >> either >> SCSI host/target or SCSI devices. >> >> Whenever we access one SCSI device, either block layer or somewhere in >> the path (e.g. throgh sg IOCTL, sg_open() calls >> scsi_autopm_get_device()) >> should runtime resume the device first, and the runtime PM framework >> makes >> sure device's parent (and its parent's parent and so on)gets resumed >> as >> well. >> Thus, the pm_runtime_set_active() seems redundant. What do you think? > > Hi Can, > > It is not clear to me why the pm_runtime_set_active() calls occur in > the > scsi_pm.c source file since the block layer automatically activates > block devices if necessary. Maybe these calls are a leftover from a > time > when runtime suspended devices were not resumed automatically by the > block layer? Anyway, I'm fine with removing these calls. > > Thanks, > > Bart. Yes, I agree with you. Let me test the new patch (which removes all the pm_runtime_set_active() calls) first, if no issue found, I will upload it for review. Thanks, Can Guo.
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c index 3717eea..278c27e 100644 --- a/drivers/scsi/scsi_pm.c +++ b/drivers/scsi/scsi_pm.c @@ -79,23 +79,22 @@ static int scsi_dev_type_resume(struct device *dev, scsi_device_resume(to_scsi_device(dev)); dev_dbg(dev, "scsi resume: %d\n", err); - if (err == 0) { - pm_runtime_disable(dev); - err = pm_runtime_set_active(dev); - pm_runtime_enable(dev); + if (scsi_is_sdev_device(dev)) { + struct scsi_device *sdev; + sdev = to_scsi_device(dev); /* - * Forcibly set runtime PM status of request queue to "active" - * to make sure we can again get requests from the queue - * (see also blk_pm_peek_request()). - * - * The resume hook will correct runtime PM status of the disk. + * If block layer runtime PM is enabled for the SCSI device, + * let block layer PM handle its runtime PM routines. */ - if (!err && scsi_is_sdev_device(dev)) { - struct scsi_device *sdev = to_scsi_device(dev); + if (sdev->request_queue->dev) + return err; + } - blk_set_runtime_active(sdev->request_queue); - } + if (err == 0) { + pm_runtime_disable(dev); + err = pm_runtime_set_active(dev); + pm_runtime_enable(dev); } return err;
If block layer runtime PM is enabled for one SCSI device, then there is no need to forcibly change the SCSI device and its request queue's runtime PM status to active in scsi_dev_type_resume(), since block layer PM shall resume the SCSI device on the demand of bios. Cc: Stanley Chu <stanley.chu@mediatek.com> Cc: Bart Van Assche <bvanassche@acm.org> Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/scsi_pm.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)