On 2020-05-01 09:50, Bart Van Assche wrote:
On 2020-04-30 18:42, Can Guo wrote:
On 2020-05-01 04:32, Bart Van Assche wrote:
> Has it been considered to test directly whether a SCSI device has been
> runtime suspended instead of relying on blk_queue_pm_only()? How about
> using pm_runtime_status_suspended() or adding a function in
> block/blk-pm.h that checks whether q->rpm_status == RPM_SUSPENDED?
Yes, I used to make the patch like that way, and it also worked well,
as
both ways are equal actually. I kinda like the current code because we
should be confident that after scsi_dev_type_resume() returns, pm_only
must be 0. Different reviewers may have different opinions, either way
works well anyways.
Hi Can,
Please note that this is not a matter of personal preferences of a
reviewer but a matter of correctness. blk_queue_pm_only() does not only
return a value > 0 if a SCSI device has been runtime suspended but also
returns true if scsi_device_quiesce() was called for another reason.
Hence my request to test the "runtime suspended" status directly and
not
to rely on blk_queue_pm_only().
Thanks,
Bart.
Hi Bart,
I agree we are pursuing correctness here, but as I said, I think both
way are equally correct. I also agree with you that the alternative way,
see [2], is much easier to be understood, we can take the alternative
way
if you are OK with it.
[1] Currently, scsi_dev_type_resume() is the hooker for resume, thaw and
restore. Per my understanding, when scsi_dev_type_resume() is running,
it is not possible that scsi_device_quiesce() can be called to this
sdev,
at least not possible in current code base. So it is OK to rely on
blk_queue_pm_only() in scsi_dev_type_resume().
[2] The alternative way which I have tested with is like below. I think
it is what you requested for if my understanding is right, please
correct
me if I am wrong.
diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 3717eea..d18271d 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -74,12 +74,15 @@ static int scsi_dev_type_resume(struct device *dev,
{
const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm :
NULL;
int err = 0;
+ bool was_rpm_suspended = false;
err = cb(dev, pm);
scsi_device_resume(to_scsi_device(dev));
dev_dbg(dev, "scsi resume: %d\n", err);
if (err == 0) {
+ was_rpm_suspended = pm_runtime_suspended(dev);
+
pm_runtime_disable(dev);
err = pm_runtime_set_active(dev);
pm_runtime_enable(dev);
@@ -93,8 +96,10 @@ static int scsi_dev_type_resume(struct device *dev,
*/
if (!err && scsi_is_sdev_device(dev)) {
struct scsi_device *sdev = to_scsi_device(dev);
-
- blk_set_runtime_active(sdev->request_queue);
+ if (was_rpm_suspended)
+
blk_post_runtime_resume(sdev->request_queue, 0);
+ else
+
blk_set_runtime_active(sdev->request_queue);
}
}
Thanks,
Can Guo
