Re: [PATCH v20 1/2] scsi: ufs: Enable power management for wlun

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

On 4/23/2021 1:01 AM, Adrian Hunter wrote:
>>
>>
> 

Hi Adrian,
Thanks for the help.
I made the changes and tried to reproduce it.
My setup becomes non-responsive and resets.
I don't think it's related to this issue though.

I hadn't set rpm_lvl and spm_lvl to 0 while testing the other day.
Setting those to 0, it proceeds further and now it becomes unresponsive. 
So I still don't clearly see the issue that you're seeing. However, I'd 
make the suggested changes and push it in next version.

> I think we also need to runtime resume RPMB WLUN before system suspend.
> e.g.
> 
> +static int ufshcd_rpmb_rpm_get_sync(struct ufs_hba *hba)
> +{
> +	return pm_runtime_get_sync(&hba->sdev_rpmb->sdev_gendev);
> +}
> +
> +static int ufshcd_rpmb_rpm_put(struct ufs_hba *hba)
> +{
> +	return pm_runtime_put(&hba->sdev_rpmb->sdev_gendev);
> +}
> +
>   void ufshcd_resume_complete(struct device *dev)
>   {
>   	struct ufs_hba *hba = dev_get_drvdata(dev);
>   
> +	if (hba->rpmb_complete_put) {
> +		hba->rpmb_complete_put = false;
> +		ufshcd_rpmb_rpm_put(hba);
> +	}
>   	if (hba->complete_put) {
>   		hba->complete_put = false;
>   		ufshcd_rpm_put(hba);
> @@ -9611,6 +9625,11 @@ int ufshcd_suspend_prepare(struct device *dev)
>   		return ret;
>   	}
>   	hba->complete_put = true;
> +
> +	if (hba->sdev_rpmb) {
> +		ufshcd_rpmb_rpm_get_sync(hba);
> +		hba->rpmb_complete_put = true;
> +	}
>   	return 0;
>   }
> 
> That also avoids another issue: if RPMB WLUN is runtime suspended at system resume, we have to skip clearing UAC, but SCSI PM will force the runtime status to RPM_ACTIVE after system resume, so the UAC never gets cleared in that case.
> 
> Furthermore, it seems better not to report errors from RPMB resume and instead let the error handler sort it out.
> So, with the above change, we can simplify a bit:
> 
> -static int ufshcd_rpmb_runtime_resume(struct device *dev)
> -{
> -	struct ufs_hba *hba = wlun_dev_to_hba(dev);
> -
> -	if (hba->sdev_rpmb)
> -		return ufshcd_clear_rpmb_uac(hba);
> -	return 0;
> -}
> -
>   static int ufshcd_rpmb_resume(struct device *dev)
>   {
>   	struct ufs_hba *hba = wlun_dev_to_hba(dev);
>   
> -	if (hba->sdev_rpmb && !pm_runtime_suspended(dev))
> -		return ufshcd_clear_rpmb_uac(hba);
> +	if (hba->sdev_rpmb)
> +		ufshcd_clear_rpmb_uac(hba);
>   	return 0;
>   }
>   
>   static const struct dev_pm_ops ufs_rpmb_pm_ops = {
> -	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_runtime_resume, NULL)
> +	SET_RUNTIME_PM_OPS(NULL, ufshcd_rpmb_resume, NULL)
>   	SET_SYSTEM_SLEEP_PM_OPS(NULL, ufshcd_rpmb_resume)
>   };
> 
> 


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