Re: [PATCH 1/1] scsi: ufs: remove redundant HPB unmap

[Po-Wen Kao]

Hi Bean,

The general idea looks good.

As ufshpb_issue_umap_all_req in ufshpb_state_toggle won't actually runs 
at cold boot probe due to the fact that sdev are not present yet, this 
should avoid sending HPB command before UAC being cleared while keeps 
HPB mapping clean when rest flow occurs.

Just a concern, shouldn't ufshpb_issue_umap_all_req be send for each 
sdev's corresponding hpb?

Looking forward to your further fixes.


Po-Wen

On 4/12/22 11:20 PM, Bean Huo wrote:
> Hi, Po-Wen,
> Previous email did not properly append the patch, this one is better
> for your review and test.
>
>
>
>
> From: Bean Huo <beanhuo@xxxxxxxxxx>
> Date: Tue, 12 Apr 2022 16:56:34 +0200
> Subject: [PATCH] scsi: ufshpb: UFSHPB cleanup
>
> Remove redundant ufshpb_reset*, and merge into a single helper
> ufshbp_state_toggle().
>
> Delete the redundant Inactivation code of all HPB Regions in the cold
> boot stage,
> and add inactivating all HPB Regions when the HPP status changes from
> HPB_RESET
> to HBP_PRESENT.
>
> Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
> ---
>   drivers/scsi/ufs/ufshcd.c |  4 ++--
>   drivers/scsi/ufs/ufshpb.c | 38 +++++++++++++-------------------------
>   drivers/scsi/ufs/ufshpb.h |  6 ++----
>   3 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 0899d5b8cdad..d8b59d017ce4 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -7223,7 +7223,7 @@ static int ufshcd_host_reset_and_restore(struct
> ufs_hba *hba)
>   	 * Stop the host controller and complete the requests
>   	 * cleared by h/w
>   	 */
> -	ufshpb_reset_host(hba);
> +	ufshpb_state_toggle(hba, HPB_RESET);
>   	ufshcd_hba_stop(hba);
>   	hba->silence_err_logs = true;
>   	ufshcd_complete_requests(hba);
> @@ -8184,7 +8184,7 @@ static int ufshcd_probe_hba(struct ufs_hba *hba,
> bool init_dev_params)
>   	/* Enable Auto-Hibernate if configured */
>   	ufshcd_auto_hibern8_enable(hba);
>   
> -	ufshpb_reset(hba);
> +	ufshpb_state_toggle(hba, HPB_PRESENT);
>   out:
>   	spin_lock_irqsave(hba->host->host_lock, flags);
>   	if (ret)
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 3ca745ad616c..4ed156031413 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -2278,39 +2278,29 @@ static bool ufshpb_check_hpb_reset_query(struct
> ufs_hba *hba)
>   	return flag_res;
>   }
>   
> -void ufshpb_reset(struct ufs_hba *hba)
> +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE state)
>   {
>   	struct ufshpb_lu *hpb;
>   	struct scsi_device *sdev;
>   
>   	shost_for_each_device(sdev, hba->host) {
>   		hpb = ufshpb_get_hpb_data(sdev);
> -		if (!hpb)
> -			continue;
>   
> -		if (ufshpb_get_state(hpb) != HPB_RESET)
> +		if (!hpb || ufshpb_get_state(hpb) != state)
>   			continue;
>   
> -		ufshpb_set_state(hpb, HPB_PRESENT);
> -	}
> -}
> -
> -void ufshpb_reset_host(struct ufs_hba *hba)
> -{
> -	struct ufshpb_lu *hpb;
> -	struct scsi_device *sdev;
> -
> -	shost_for_each_device(sdev, hba->host) {
> -		hpb = ufshpb_get_hpb_data(sdev);
> -		if (!hpb)
> -			continue;
> -
> -		if (ufshpb_get_state(hpb) != HPB_PRESENT)
> -			continue;
> -		ufshpb_set_state(hpb, HPB_RESET);
> -		ufshpb_cancel_jobs(hpb);
> -		ufshpb_discard_rsp_lists(hpb);
> +		ufshpb_set_state(hpb, state);
> +		if (state == HPB_RESET) {
> +			ufshpb_cancel_jobs(hpb);
> +			ufshpb_discard_rsp_lists(hpb);
> +		}
>   	}
> +	/*
> +	 * Inactivating all HPB Region in device side in case HPB
> state changed
> +	 * from HPB_RESET to HPB_PRESENT
> +	 */
> +	if (!hpb->is_hcm && state == HPB_PRESENT)
> +		ufshpb_issue_umap_all_req(hpb);
>   }
>   
>   void ufshpb_suspend(struct ufs_hba *hba)
> @@ -2456,8 +2446,6 @@ static void ufshpb_hpb_lu_prepared(struct ufs_hba
> *hba)
>   			ufshpb_set_state(hpb, HPB_PRESENT);
>   			if ((hpb->lu_pinned_end - hpb-
> > lu_pinned_start) > 0)
>   				queue_work(ufshpb_wq, &hpb->map_work);
> -			if (!hpb->is_hcm)
> -				ufshpb_issue_umap_all_req(hpb);
>   		} else {
>   			dev_err(hba->dev, "destroy HPB lu %d\n", hpb-
> > lun);
>   			ufshpb_destroy_lu(hba, sdev);
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index b475dbd78988..a130f0b16c3e 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -288,8 +288,7 @@ static int ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp) { return 0;
>   static void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb
> *lrbp) {}
>   static void ufshpb_resume(struct ufs_hba *hba) {}
>   static void ufshpb_suspend(struct ufs_hba *hba) {}
> -static void ufshpb_reset(struct ufs_hba *hba) {}
> -static void ufshpb_reset_host(struct ufs_hba *hba) {}
> +static void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE
> state) {}
>   static void ufshpb_init(struct ufs_hba *hba) {}
>   static void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> *sdev) {}
>   static void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device
> *sdev) {}
> @@ -303,8 +302,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp);
>   void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp);
>   void ufshpb_resume(struct ufs_hba *hba);
>   void ufshpb_suspend(struct ufs_hba *hba);
> -void ufshpb_reset(struct ufs_hba *hba);
> -void ufshpb_reset_host(struct ufs_hba *hba);
> +void ufshpb_state_toggle(struct ufs_hba *hba, enum UFSHPB_STATE
> state);
>   void ufshpb_init(struct ufs_hba *hba);
>   void ufshpb_init_hpb_lu(struct ufs_hba *hba, struct scsi_device
> *sdev);
>   void ufshpb_destroy_lu(struct ufs_hba *hba, struct scsi_device *sdev);