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

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

[Can Guo]

In current UFS driver, if the hba device is in RPM_SUSPENDED status when system suspend
happens, UFS driver chooses not to resume hba device during system resume [1], which is
to achieve power efficiency by leaving the runtime PM to block layer PM.

Similarly, 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
RPM_ACTIVE in scsi_dev_type_resume(), since block layer PM shall resume the SCSI device
on the demand of bios.

Without this change, if situation [1] happens, when scsi_dev_type_resume() invokes
blk_pm_set_active(), we will get an error since it is trying to set the SCSI device's
runtime PM status to RPM_ACTIVE even when its parent is not RPM_ACTIVE, which actually
is not fatal. This change can avoid the annoying error logs when situation [1] happens.

However, there are some rare cases where upper layers start to submit bios into the
request queue of one SCSI device even when scsi_dev_type_resume() is still running on
that SCSI device. In these rare cases, if situation [1] happens, when a bio is sent to
block layer, block layer runtime PM shall try to runtime resume the SCSI device
(invoked from blk_queue_enter()) and if meanwhile scsi_dev_type_resume() has ran over
line #83 but before #85 (see below code snippet), block layer runtime PM won't be able
to resume it due to runtime PM is disabled on this device at this moment. And later if
no more bios kick blk_queue_enter(), the bio will be stuck at blk_queue_enter() forever.
This change can fix the problem too.

<--code snippet-->
72 static int scsi_dev_type_resume(struct device *dev,
73 		int (*cb)(struct device *, const struct dev_pm_ops *))
74 {
...
83 		pm_runtime_disable(dev);
84 		err = pm_runtime_set_active(dev);
85 		pm_runtime_enable(dev);
...
<--code snippet-->

Can Guo (1):
  scsi: pm: Leave runtime resume along if block layer PM is enabled

 drivers/scsi/scsi_pm.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project.

[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.


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

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.