Re: [PATCH 3/3] scsi: ufs: Let devices remain runtime suspended during system suspend

["Asutosh Das (asd)" <asutoshd@codeaurora.org>]

On 9/2/2021 3:18 AM, Adrian Hunter wrote:
> 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: Fix error handler clear ua deadlock
> 
> So amend the logic accordingly.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>   drivers/scsi/ufs/ufshcd.c | 45 ++++++++++++++++++++++++++++-----------
>   drivers/scsi/ufs/ufshcd.h | 11 +++++++++-
>   2 files changed, 43 insertions(+), 13 deletions(-)
> 
Hi Adrian,
Thanks for the change.

> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 57ed4b93b949..8e799e47e095 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -9722,13 +9722,29 @@ void ufshcd_resume_complete(struct device *dev)
>   		ufshcd_rpm_put(hba);
>   		hba->complete_put = false;
>   	}
> -	if (hba->rpmb_complete_put) {
> -		ufshcd_rpmb_rpm_put(hba);
> -		hba->rpmb_complete_put = false;
> -	}
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_resume_complete);
>   
> +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;
> +
In the current ufshcd_suspend(), there's a ufshcd_vops_suspend().
That's invoked for different pm_ops independent of the rpm_lvl and spm_lvl.
I'm not sure if any vendor driver does different things for diff pm_op.
Perhaps something to check.

> +	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)
>   {
>   	struct ufs_hba *hba = dev_get_drvdata(dev);
> @@ -9741,17 +9757,22 @@ 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 (!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;
>   	}
> -	if (hba->sdev_rpmb) {
> -		ufshcd_rpmb_rpm_get_sync(hba);
> -		hba->rpmb_complete_put = true;
> -	}
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_suspend_prepare);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 4723f27a55d1..149803d60ecb 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -915,7 +915,6 @@ struct ufs_hba {
>   #endif
>   	u32 luns_avail;
>   	bool complete_put;
> -	bool rpmb_complete_put;
>   };
>   
>   /* Returns true if clocks can be gated. Otherwise false */
> @@ -1383,6 +1382,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);
> 


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