[PATCH V7 0/2] scsi: ufs: Let devices remain runtime suspended during system suspend

[PATCH V7 0/2] scsi: ufs: Let devices remain runtime suspended during system suspend

[Adrian Hunter]

Hi

UFS devices can remain runtime suspended at system suspend time,
if the conditions are right.  Add support for that, first fixing
the impediments.

Changes in V7:

      scsi: ufs: Fix error handler clear ua deadlock
	Dropped because superseded by "scsi: ufs: core: Stop clearing
	UNIT ATTENTIONS"

      scsi: ufs: Let devices remain runtime suspended during system suspend
	Re-based

Changes in V6:

      scsi: ufs: Fix error handler clear ua deadlock
	Ensure data byte count bits 1:0 are 11b
	Use ufshcd_compose_dev_cmd() to set up command

Changes in V5:

      scsi: ufs: Fix error handler clear ua deadlock
	Update commit message
	Try to abort REQUEST SENSE if it times out

Changes in V4:

      scsi: ufs: Fix error handler clear ua deadlock

	Do request-sense directly

Changes in V3:

      scsi: ufs: Fix error handler clear ua deadlock

	Correct commit message.
	Amend stable tags to add dependent cherry picks

Changes in V2:

    scsi: ufs: Let devices remain runtime suspended during system suspend

	The ufs-hisi driver uses different RPM and SPM, but it is made
	explicit by a new parameter to suspend prepare.


Adrian Hunter (2):
      scsi: ufs: Fix runtime PM dependencies getting broken
      scsi: ufs: Let devices remain runtime suspended during system suspend

 drivers/scsi/scsi_pm.c      | 16 +++++++++++-----
 drivers/scsi/ufs/ufs-hisi.c |  8 +++++++-
 drivers/scsi/ufs/ufshcd.c   | 46 ++++++++++++++++++++++++++++++++++++++++-----
 drivers/scsi/ufs/ufshcd.h   | 11 +++++++++++
 include/scsi/scsi_device.h  |  1 +
 5 files changed, 71 insertions(+), 11 deletions(-)


Regards
Adrian

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

[Adrian Hunter]

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 */
-- 
2.25.1

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

[Bart Van Assche]

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.

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

[Adrian Hunter]

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.

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

[Bart Van Assche]

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.

[PATCH V7 2/2] scsi: ufs: Let devices remain runtime suspended during system suspend

[Adrian Hunter]

If the UFS Device WLUN is runtime suspended and is in the same power
mode, link state and b_rpm_dev_flush_capable (BKOP or WB buffer flush etc)
state, then it can remain runtime suspended instead of being runtime
resumed and then system suspended.

The following patches have cleared the way for that to happen:
  scsi: ufs: Fix runtime PM dependencies getting broken
  scsi: ufs: core: Stop clearing UNIT ATTENTIONS

So amend the logic accordingly.

Note, the ufs-hisi driver uses different RPM and SPM, but it is made
explicit by a new parameter to suspend prepare.

Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>
---
 drivers/scsi/ufs/ufs-hisi.c |  8 ++++++-
 drivers/scsi/ufs/ufshcd.c   | 45 ++++++++++++++++++++++++++++++++-----
 drivers/scsi/ufs/ufshcd.h   | 11 +++++++++
 3 files changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-hisi.c b/drivers/scsi/ufs/ufs-hisi.c
index 6b706de8354b..4a08fb35642c 100644
--- a/drivers/scsi/ufs/ufs-hisi.c
+++ b/drivers/scsi/ufs/ufs-hisi.c
@@ -396,6 +396,12 @@ static int ufs_hisi_pwr_change_notify(struct ufs_hba *hba,
 	return ret;
 }
 
+static int ufs_hisi_suspend_prepare(struct device *dev)
+{
+	/* RPM and SPM are different. Refer ufs_hisi_suspend() */
+	return __ufshcd_suspend_prepare(dev, false);
+}
+
 static int ufs_hisi_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op)
 {
 	struct ufs_hisi_host *host = ufshcd_get_variant(hba);
@@ -574,7 +580,7 @@ static int ufs_hisi_remove(struct platform_device *pdev)
 static const struct dev_pm_ops ufs_hisi_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(ufshcd_system_suspend, ufshcd_system_resume)
 	SET_RUNTIME_PM_OPS(ufshcd_runtime_suspend, ufshcd_runtime_resume, NULL)
-	.prepare	 = ufshcd_suspend_prepare,
+	.prepare	 = ufs_hisi_suspend_prepare,
 	.complete	 = ufshcd_resume_complete,
 };
 
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index b70f566f7f8a..f34227add27d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -9681,7 +9681,27 @@ void ufshcd_resume_complete(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
 
-int ufshcd_suspend_prepare(struct device *dev)
+static bool ufshcd_rpm_ok_for_spm(struct ufs_hba *hba)
+{
+	struct device *dev = &hba->sdev_ufs_device->sdev_gendev;
+	enum ufs_dev_pwr_mode dev_pwr_mode;
+	enum uic_link_state link_state;
+	unsigned long flags;
+	bool res;
+
+	spin_lock_irqsave(&dev->power.lock, flags);
+	dev_pwr_mode = ufs_get_pm_lvl_to_dev_pwr_mode(hba->spm_lvl);
+	link_state = ufs_get_pm_lvl_to_link_pwr_state(hba->spm_lvl);
+	res = pm_runtime_suspended(dev) &&
+	      hba->curr_dev_pwr_mode == dev_pwr_mode &&
+	      hba->uic_link_state == link_state &&
+	      !hba->dev_info.b_rpm_dev_flush_capable;
+	spin_unlock_irqrestore(&dev->power.lock, flags);
+
+	return res;
+}
+
+int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm)
 {
 	struct ufs_hba *hba = dev_get_drvdata(dev);
 	int ret;
@@ -9693,15 +9713,30 @@ int ufshcd_suspend_prepare(struct device *dev)
 	 * Refer ufshcd_resume_complete()
 	 */
 	if (hba->sdev_ufs_device) {
-		ret = ufshcd_rpm_get_sync(hba);
-		if (ret < 0 && ret != -EACCES) {
-			ufshcd_rpm_put(hba);
-			return ret;
+		/* Prevent runtime suspend */
+		ufshcd_rpm_get_noresume(hba);
+		/*
+		 * Check if already runtime suspended in same state as system
+		 * suspend would be.
+		 */
+		if (!rpm_ok_for_spm || !ufshcd_rpm_ok_for_spm(hba)) {
+			/* RPM state is not ok for SPM, so runtime resume */
+			ret = ufshcd_rpm_resume(hba);
+			if (ret < 0 && ret != -EACCES) {
+				ufshcd_rpm_put(hba);
+				return ret;
+			}
 		}
 		hba->complete_put = true;
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__ufshcd_suspend_prepare);
+
+int ufshcd_suspend_prepare(struct device *dev)
+{
+	return __ufshcd_suspend_prepare(dev, true);
+}
 EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
 
 #ifdef CONFIG_PM_SLEEP
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 782bbe6d5a52..2384da8f30c0 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1191,6 +1191,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 
 int ufshcd_wb_toggle(struct ufs_hba *hba, bool enable);
 int ufshcd_suspend_prepare(struct device *dev);
+int __ufshcd_suspend_prepare(struct device *dev, bool rpm_ok_for_spm);
 void ufshcd_resume_complete(struct device *dev);
 
 /* Wrapper functions for safely calling variant operations */
@@ -1399,6 +1400,16 @@ static inline int ufshcd_rpm_put_sync(struct ufs_hba *hba)
 	return pm_runtime_put_sync(&hba->sdev_ufs_device->sdev_gendev);
 }
 
+static inline void ufshcd_rpm_get_noresume(struct ufs_hba *hba)
+{
+	pm_runtime_get_noresume(&hba->sdev_ufs_device->sdev_gendev);
+}
+
+static inline int ufshcd_rpm_resume(struct ufs_hba *hba)
+{
+	return pm_runtime_resume(&hba->sdev_ufs_device->sdev_gendev);
+}
+
 static inline int ufshcd_rpm_put(struct ufs_hba *hba)
 {
 	return pm_runtime_put(&hba->sdev_ufs_device->sdev_gendev);
-- 
2.25.1