Re: [PATCH v3 1/1] scsi: pm: Balance pm_only counter of request queue during system resume

[Can Guo <cang@codeaurora.org>]

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