[PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Can Guo]

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(-)

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;
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Bart Van Assche]

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.

Re: [PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Can Guo]

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.

Re: [PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Can Guo]

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.

Re: [PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Bart Van Assche]

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.

Re: [PATCH RFC v1 1/1] scsi: pm: Leave runtime resume along if block layer PM is enabled

[Can Guo]

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.