diff mbox series

[V7,1/2] scsi: ufs: Fix runtime PM dependencies getting broken

Message ID 20211005134445.234671-2-adrian.hunter@intel.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: Let devices remain runtime suspended during system suspend | expand

Commit Message

Adrian Hunter Oct. 5, 2021, 1:44 p.m. UTC 
UFS SCSI devices make use of device links to establish PM dependencies.
However, SCSI PM will force devices' runtime PM state to be active during
system resume. That can break runtime PM dependencies for UFS devices.
Fix by adding a flag 'preserve_rpm' to let UFS SCSI devices opt-out of
the unwanted behaviour.

Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/scsi/scsi_pm.c     | 16 +++++++++++-----
 drivers/scsi/ufs/ufshcd.c  |  1 +
 include/scsi/scsi_device.h |  1 +
 3 files changed, 13 insertions(+), 5 deletions(-)

Comments

Bart Van Assche Oct. 5, 2021, 6:52 p.m. UTC | #1 
On 10/5/21 6:44 AM, Adrian Hunter wrote:
> UFS SCSI devices make use of device links to establish PM dependencies.
> However, SCSI PM will force devices' runtime PM state to be active during
> system resume. That can break runtime PM dependencies for UFS devices.
> Fix by adding a flag 'preserve_rpm' to let UFS SCSI devices opt-out of
> the unwanted behaviour.
> 
> Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>   drivers/scsi/scsi_pm.c     | 16 +++++++++++-----
>   drivers/scsi/ufs/ufshcd.c  |  1 +
>   include/scsi/scsi_device.h |  1 +
>   3 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
> index 3717eea37ecb..0557c1ad304d 100644
> --- a/drivers/scsi/scsi_pm.c
> +++ b/drivers/scsi/scsi_pm.c
> @@ -73,13 +73,22 @@ static int scsi_dev_type_resume(struct device *dev,
>   		int (*cb)(struct device *, const struct dev_pm_ops *))
>   {
>   	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> +	struct scsi_device *sdev = NULL;
> +	bool preserve_rpm = false;
>   	int err = 0;
>   
> +	if (scsi_is_sdev_device(dev)) {
> +		sdev = to_scsi_device(dev);
> +		preserve_rpm = sdev->preserve_rpm;
> +		if (preserve_rpm && pm_runtime_suspended(dev))
> +			return 0;
> +	}
> +
>   	err = cb(dev, pm);
>   	scsi_device_resume(to_scsi_device(dev));
>   	dev_dbg(dev, "scsi resume: %d\n", err);
>   
> -	if (err == 0) {
> +	if (err == 0 && !preserve_rpm) {
>   		pm_runtime_disable(dev);
>   		err = pm_runtime_set_active(dev);
>   		pm_runtime_enable(dev);
> @@ -91,11 +100,8 @@ static int scsi_dev_type_resume(struct device *dev,
>   		 *
>   		 * The resume hook will correct runtime PM status of the disk.
>   		 */
> -		if (!err && scsi_is_sdev_device(dev)) {
> -			struct scsi_device *sdev = to_scsi_device(dev);
> -
> +		if (!err && sdev)
>   			blk_set_runtime_active(sdev->request_queue);
> -		}
>   	}
>   
>   	return err;
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index d91a405fd181..b70f566f7f8a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5016,6 +5016,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>   		pm_runtime_get_noresume(&sdev->sdev_gendev);
>   	else if (ufshcd_is_rpm_autosuspend_allowed(hba))
>   		sdev->rpm_autosuspend = 1;
> +	sdev->preserve_rpm = 1;
>   
>   	ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
>   
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 09a17f6e93a7..47eb30a6b7b2 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -197,6 +197,7 @@ struct scsi_device {
>   	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
>   	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
>   	unsigned try_rc_10_first:1;	/* Try READ_CAPACACITY_10 first */
> +	unsigned preserve_rpm:1;	/* Preserve runtime PM */
>   	unsigned security_supported:1;	/* Supports Security Protocols */
>   	unsigned is_visible:1;	/* is the device visible in sysfs */
>   	unsigned wce_default_on:1;	/* Cache is ON by default */

So a new flag is added in struct scsi_device and that flag is only used by
the UFS driver? I'm less than enthusiast about this patch. I think that the
SCSI core needs to be modified such that system suspend and resume is
separated from runtime suspend and resume. The following code:

	if (err == 0) {
		pm_runtime_disable(dev);
		err = pm_runtime_set_active(dev);
		pm_runtime_enable(dev);
		[ ... ]
	}

has been introduced in scsi_dev_type_resume() by commit 3c31b52f96f7
("scsi: async sd resume"). I'm in favor of removing that code.

Thanks,

Bart.
Adrian Hunter Oct. 6, 2021, 6:35 a.m. UTC | #2 
On 05/10/2021 21:52, Bart Van Assche wrote:
> On 10/5/21 6:44 AM, Adrian Hunter wrote:
>> UFS SCSI devices make use of device links to establish PM dependencies.
>> However, SCSI PM will force devices' runtime PM state to be active during
>> system resume. That can break runtime PM dependencies for UFS devices.
>> Fix by adding a flag 'preserve_rpm' to let UFS SCSI devices opt-out of
>> the unwanted behaviour.
>>
>> Fixes: b294ff3e34490f ("scsi: ufs: core: Enable power management for wlun")
>> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
>> ---
>>   drivers/scsi/scsi_pm.c     | 16 +++++++++++-----
>>   drivers/scsi/ufs/ufshcd.c  |  1 +
>>   include/scsi/scsi_device.h |  1 +
>>   3 files changed, 13 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
>> index 3717eea37ecb..0557c1ad304d 100644
>> --- a/drivers/scsi/scsi_pm.c
>> +++ b/drivers/scsi/scsi_pm.c
>> @@ -73,13 +73,22 @@ static int scsi_dev_type_resume(struct device *dev,
>>           int (*cb)(struct device *, const struct dev_pm_ops *))
>>   {
>>       const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
>> +    struct scsi_device *sdev = NULL;
>> +    bool preserve_rpm = false;
>>       int err = 0;
>>   +    if (scsi_is_sdev_device(dev)) {
>> +        sdev = to_scsi_device(dev);
>> +        preserve_rpm = sdev->preserve_rpm;
>> +        if (preserve_rpm && pm_runtime_suspended(dev))
>> +            return 0;
>> +    }
>> +
>>       err = cb(dev, pm);
>>       scsi_device_resume(to_scsi_device(dev));
>>       dev_dbg(dev, "scsi resume: %d\n", err);
>>   -    if (err == 0) {
>> +    if (err == 0 && !preserve_rpm) {
>>           pm_runtime_disable(dev);
>>           err = pm_runtime_set_active(dev);
>>           pm_runtime_enable(dev);
>> @@ -91,11 +100,8 @@ static int scsi_dev_type_resume(struct device *dev,
>>            *
>>            * The resume hook will correct runtime PM status of the disk.
>>            */
>> -        if (!err && scsi_is_sdev_device(dev)) {
>> -            struct scsi_device *sdev = to_scsi_device(dev);
>> -
>> +        if (!err && sdev)
>>               blk_set_runtime_active(sdev->request_queue);
>> -        }
>>       }
>>         return err;
>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
>> index d91a405fd181..b70f566f7f8a 100644
>> --- a/drivers/scsi/ufs/ufshcd.c
>> +++ b/drivers/scsi/ufs/ufshcd.c
>> @@ -5016,6 +5016,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>>           pm_runtime_get_noresume(&sdev->sdev_gendev);
>>       else if (ufshcd_is_rpm_autosuspend_allowed(hba))
>>           sdev->rpm_autosuspend = 1;
>> +    sdev->preserve_rpm = 1;
>>         ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
>>   diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
>> index 09a17f6e93a7..47eb30a6b7b2 100644
>> --- a/include/scsi/scsi_device.h
>> +++ b/include/scsi/scsi_device.h
>> @@ -197,6 +197,7 @@ struct scsi_device {
>>       unsigned no_read_disc_info:1;    /* Avoid READ_DISC_INFO cmds */
>>       unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
>>       unsigned try_rc_10_first:1;    /* Try READ_CAPACACITY_10 first */
>> +    unsigned preserve_rpm:1;    /* Preserve runtime PM */
>>       unsigned security_supported:1;    /* Supports Security Protocols */
>>       unsigned is_visible:1;    /* is the device visible in sysfs */
>>       unsigned wce_default_on:1;    /* Cache is ON by default */
> 
> So a new flag is added in struct scsi_device and that flag is only used by
> the UFS driver? I'm less than enthusiast about this patch. I think that the
> SCSI core needs to be modified such that system suspend and resume is
> separated from runtime suspend and resume.

That is a tidy-up, whereas this patch is a fix needed also in stable
kernels.

> The following code:
> 
>     if (err == 0) {
>         pm_runtime_disable(dev);
>         err = pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         [ ... ]
>     }
> 
> has been introduced in scsi_dev_type_resume() by commit 3c31b52f96f7
> ("scsi: async sd resume"). I'm in favor of removing that code.

Presumably that code is necessary.  Trying to remove it really seems a
separate issue.
Bart Van Assche Oct. 6, 2021, 4:01 p.m. UTC | #3 
On 10/5/21 11:35 PM, Adrian Hunter wrote:
> On 05/10/2021 21:52, Bart Van Assche wrote:
>> The following code:
>>
>>      if (err == 0) {
>>          pm_runtime_disable(dev);
>>          err = pm_runtime_set_active(dev);
>>          pm_runtime_enable(dev);
>>          [ ... ]
>>      }
>>
>> has been introduced in scsi_dev_type_resume() by commit 3c31b52f96f7
>> ("scsi: async sd resume"). I'm in favor of removing that code.
> 
> Presumably that code is necessary.  Trying to remove it really seems a
> separate issue.

I want to move that code into the sd driver before this patch goes upstream.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/scsi_pm.c b/drivers/scsi/scsi_pm.c
index 3717eea37ecb..0557c1ad304d 100644
--- a/drivers/scsi/scsi_pm.c
+++ b/drivers/scsi/scsi_pm.c
@@ -73,13 +73,22 @@  static int scsi_dev_type_resume(struct device *dev,
 		int (*cb)(struct device *, const struct dev_pm_ops *))
 {
 	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	struct scsi_device *sdev = NULL;
+	bool preserve_rpm = false;
 	int err = 0;
 
+	if (scsi_is_sdev_device(dev)) {
+		sdev = to_scsi_device(dev);
+		preserve_rpm = sdev->preserve_rpm;
+		if (preserve_rpm && pm_runtime_suspended(dev))
+			return 0;
+	}
+
 	err = cb(dev, pm);
 	scsi_device_resume(to_scsi_device(dev));
 	dev_dbg(dev, "scsi resume: %d\n", err);
 
-	if (err == 0) {
+	if (err == 0 && !preserve_rpm) {
 		pm_runtime_disable(dev);
 		err = pm_runtime_set_active(dev);
 		pm_runtime_enable(dev);
@@ -91,11 +100,8 @@  static int scsi_dev_type_resume(struct device *dev,
 		 *
 		 * The resume hook will correct runtime PM status of the disk.
 		 */
-		if (!err && scsi_is_sdev_device(dev)) {
-			struct scsi_device *sdev = to_scsi_device(dev);
-
+		if (!err && sdev)
 			blk_set_runtime_active(sdev->request_queue);
-		}
 	}
 
 	return err;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index d91a405fd181..b70f566f7f8a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5016,6 +5016,7 @@  static int ufshcd_slave_configure(struct scsi_device *sdev)
 		pm_runtime_get_noresume(&sdev->sdev_gendev);
 	else if (ufshcd_is_rpm_autosuspend_allowed(hba))
 		sdev->rpm_autosuspend = 1;
+	sdev->preserve_rpm = 1;
 
 	ufshcd_crypto_setup_rq_keyslot_manager(hba, q);
 
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 09a17f6e93a7..47eb30a6b7b2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -197,6 +197,7 @@  struct scsi_device {
 	unsigned no_read_disc_info:1;	/* Avoid READ_DISC_INFO cmds */
 	unsigned no_read_capacity_16:1; /* Avoid READ_CAPACITY_16 cmds */
 	unsigned try_rc_10_first:1;	/* Try READ_CAPACACITY_10 first */
+	unsigned preserve_rpm:1;	/* Preserve runtime PM */
 	unsigned security_supported:1;	/* Supports Security Protocols */
 	unsigned is_visible:1;	/* is the device visible in sysfs */
 	unsigned wce_default_on:1;	/* Cache is ON by default */