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

[Bart Van Assche <bvanassche@acm.org>]

On 3/18/21 5:35 PM, Asutosh Das wrote:
> During runtime-suspend of ufs host, the scsi devices are
> already suspended and so are the queues associated with them.
> But the ufs host sends SSU to wlun during its runtime-suspend.
> During the process blk_queue_enter checks if the queue is not in
> suspended state. If so, it waits for the queue to resume, and never
> comes out of it.
> The commit
> (d55d15a33: scsi: block: Do not accept any requests while suspended)
> adds the check if the queue is in suspended state in blk_queue_enter().

What is the role of the WLUN during runtime suspend and why does a
command need to be sent to the WLUN during runtime suspend? Although it
is possible to derive this from the source code, please explain this in
the patch description.

What does the acronym SSU stand for? This doesn't seem like a commonly
used kernel acronym to me so please expand that acronym.

> Fix this by registering ufs device wlun as a scsi driver and
> registering it for block runtime-pm. Also make this as a
> supplier for all other luns. That way, this device wlun
> suspends after all the consumers and resumes after
> hba resumes.

That's an interesting solution.

> -void __exit ufs_debugfs_exit(void)
> +void ufs_debugfs_exit(void)

Is the above change related to the rest of this patch?

>  static struct platform_driver ufs_qcom_pltform = {
> diff --git a/drivers/scsi/ufs/ufs_bsg.c b/drivers/scsi/ufs/ufs_bsg.c
> index 5b2bc1a..cbb5a90 100644
> --- a/drivers/scsi/ufs/ufs_bsg.c
> +++ b/drivers/scsi/ufs/ufs_bsg.c
> @@ -97,7 +97,7 @@ static int ufs_bsg_request(struct bsg_job *job)
>  
>  	bsg_reply->reply_payload_rcv_len = 0;
>  
> -	pm_runtime_get_sync(hba->dev);
> +	scsi_autopm_get_device(hba->sdev_ufs_device);

Can the pm_runtime_get_sync() to scsi_autopm_get_device() changes be
moved into a separate patch?

> +static inline bool is_rpmb_wlun(struct scsi_device *sdev)
> +{
> +	return (sdev->lun == ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_RPMB_WLUN));
> +}

Has this patch been verified with checkpatch? Checkpatch should have
reported the following for the above code:

	return is not a function, parentheses are not required

> +static inline bool is_device_wlun(struct scsi_device *sdev)
> +{
> +	return (sdev->lun ==
> +		ufshcd_upiu_wlun_to_scsi_wlun(UFS_UPIU_UFS_DEVICE_WLUN));
> +}

Same comment here.

>  		/*
> -		 * Don't assume anything of pm_runtime_get_sync(), if
> +		 * Don't assume anything of resume, if
>  		 * resume fails, irq and clocks can be OFF, and powers
>  		 * can be OFF or in LPM.
>  		 */

Please make better use of the horizontal space in the above comment by
making comment lines longer.

Thanks,

Bart.