[PATCH v1 1/1] scsi: ufs: core: Removed ufshcd_wb_presrv_usrspc_keep_vcc_on()

[PATCH v1 1/1] scsi: ufs: core: Removed ufshcd_wb_presrv_usrspc_keep_vcc_on()

[Bao D. Nguyen]

Merge the ufshcd_wb_presrv_usrspc_keep_vcc_on() function
into ufshcd_wb_need_flush(). The "_keep_vcc_on" part of the
function name is misleading. The function definition may be
deviated from its original intention. This is a small function
only invoked by the ufshcd_wb_need_flush(). To improve the
readability, remove this function and merge its content
into its caller. There is no change to the functionality.

Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
---
 drivers/ufs/core/ufshcd.c | 53 +++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 32 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4e1e214..b9272b1 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -6083,32 +6083,6 @@ int ufshcd_wb_toggle_buf_flush(struct ufs_hba *hba, bool enable)
 	return ret;
 }
 
-static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
-						u32 avail_buf)
-{
-	u32 cur_buf;
-	int ret;
-	u8 index;
-
-	index = ufshcd_wb_get_query_index(hba);
-	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
-					      QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
-					      index, 0, &cur_buf);
-	if (ret) {
-		dev_err(hba->dev, "%s: dCurWriteBoosterBufferSize read failed %d\n",
-			__func__, ret);
-		return false;
-	}
-
-	if (!cur_buf) {
-		dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-space is available\n",
-			 cur_buf);
-		return false;
-	}
-	/* Let it continue to flush when available buffer exceeds threshold */
-	return avail_buf < hba->vps->wb_flush_threshold;
-}
-
 static void ufshcd_wb_force_disable(struct ufs_hba *hba)
 {
 	if (ufshcd_is_wb_buf_flush_allowed(hba))
@@ -6152,9 +6126,9 @@ static bool ufshcd_is_wb_buf_lifetime_available(struct ufs_hba *hba)
 
 static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 {
-	int ret;
-	u32 avail_buf;
+	u32 avail_buf, cur_buf;
 	u8 index;
+	int ret;
 
 	if (!ufshcd_is_wb_allowed(hba))
 		return false;
@@ -6165,15 +6139,13 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 	}
 
 	/*
-	 * The ufs device needs the vcc to be ON to flush.
 	 * With user-space reduction enabled, it's enough to enable flush
 	 * by checking only the available buffer. The threshold
 	 * defined here is > 90% full.
 	 * With user-space preserved enabled, the current-buffer
 	 * should be checked too because the wb buffer size can reduce
 	 * when disk tends to be full. This info is provided by current
-	 * buffer (dCurrentWriteBoosterBufferSize). There's no point in
-	 * keeping vcc on when current buffer is empty.
+	 * buffer (dCurrentWriteBoosterBufferSize).
 	 */
 	index = ufshcd_wb_get_query_index(hba);
 	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
@@ -6188,7 +6160,24 @@ static bool ufshcd_wb_need_flush(struct ufs_hba *hba)
 	if (!hba->dev_info.b_presrv_uspc_en)
 		return avail_buf <= UFS_WB_BUF_REMAIN_PERCENT(10);
 
-	return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf);
+	index = ufshcd_wb_get_query_index(hba);
+	ret = ufshcd_query_attr_retry(hba, UPIU_QUERY_OPCODE_READ_ATTR,
+				      QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
+				      index, 0, &cur_buf);
+	if (ret) {
+		dev_err(hba->dev, "%s: dCurWriteBoosterBufferSize read failed %d\n",
+			__func__, ret);
+		return false;
+	}
+
+	if (!cur_buf) {
+		dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-space is available\n",
+			 cur_buf);
+		return false;
+	}
+
+	/* Let it continue to flush when available buffer exceeds threshold */
+	return avail_buf < hba->vps->wb_flush_threshold;
 }
 
 static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work)
-- 
2.7.4

RE: [PATCH v1 1/1] scsi: ufs: core: Removed ufshcd_wb_presrv_usrspc_keep_vcc_on()

[Avri Altman]

> Merge the ufshcd_wb_presrv_usrspc_keep_vcc_on() function into
> ufshcd_wb_need_flush(). The "_keep_vcc_on" part of the function name is
> misleading. The function definition may be deviated from its original intention.
> This is a small function only invoked by the ufshcd_wb_need_flush(). To
> improve the readability, remove this function and merge its content into its
> caller. There is no change to the functionality.
> 
> Signed-off-by: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> ---
>  drivers/ufs/core/ufshcd.c | 53 +++++++++++++++++++--------------------------
> --
>  1 file changed, 21 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index
> 4e1e214..b9272b1 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -6083,32 +6083,6 @@ int ufshcd_wb_toggle_buf_flush(struct ufs_hba
> *hba, bool enable)
>  	return ret;
>  }
> 
> -static bool ufshcd_wb_presrv_usrspc_keep_vcc_on(struct ufs_hba *hba,
> -						u32 avail_buf)
Maybe just change the function name to better describe what it does,
e.g. ufshcd_wb_exceed_threshold ?

Thanks,
Avri

> -{
> -	u32 cur_buf;
> -	int ret;
> -	u8 index;
> -
> -	index = ufshcd_wb_get_query_index(hba);
> -	ret = ufshcd_query_attr_retry(hba,
> UPIU_QUERY_OPCODE_READ_ATTR,
> -
> QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
> -					      index, 0, &cur_buf);
> -	if (ret) {
> -		dev_err(hba->dev, "%s: dCurWriteBoosterBufferSize read
> failed %d\n",
> -			__func__, ret);
> -		return false;
> -	}
> -
> -	if (!cur_buf) {
> -		dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-
> space is available\n",
> -			 cur_buf);
> -		return false;
> -	}
> -	/* Let it continue to flush when available buffer exceeds threshold */
> -	return avail_buf < hba->vps->wb_flush_threshold;
> -}
> -
>  static void ufshcd_wb_force_disable(struct ufs_hba *hba)  {
>  	if (ufshcd_is_wb_buf_flush_allowed(hba))
> @@ -6152,9 +6126,9 @@ static bool
> ufshcd_is_wb_buf_lifetime_available(struct ufs_hba *hba)
> 
>  static bool ufshcd_wb_need_flush(struct ufs_hba *hba)  {
> -	int ret;
> -	u32 avail_buf;
> +	u32 avail_buf, cur_buf;
>  	u8 index;
> +	int ret;
> 
>  	if (!ufshcd_is_wb_allowed(hba))
>  		return false;
> @@ -6165,15 +6139,13 @@ static bool ufshcd_wb_need_flush(struct
> ufs_hba *hba)
>  	}
> 
>  	/*
> -	 * The ufs device needs the vcc to be ON to flush.
>  	 * With user-space reduction enabled, it's enough to enable flush
>  	 * by checking only the available buffer. The threshold
>  	 * defined here is > 90% full.
>  	 * With user-space preserved enabled, the current-buffer
>  	 * should be checked too because the wb buffer size can reduce
>  	 * when disk tends to be full. This info is provided by current
> -	 * buffer (dCurrentWriteBoosterBufferSize). There's no point in
> -	 * keeping vcc on when current buffer is empty.
> +	 * buffer (dCurrentWriteBoosterBufferSize).
>  	 */
>  	index = ufshcd_wb_get_query_index(hba);
>  	ret = ufshcd_query_attr_retry(hba,
> UPIU_QUERY_OPCODE_READ_ATTR, @@ -6188,7 +6160,24 @@ static bool
> ufshcd_wb_need_flush(struct ufs_hba *hba)
>  	if (!hba->dev_info.b_presrv_uspc_en)
>  		return avail_buf <= UFS_WB_BUF_REMAIN_PERCENT(10);
> 
> -	return ufshcd_wb_presrv_usrspc_keep_vcc_on(hba, avail_buf);
> +	index = ufshcd_wb_get_query_index(hba);
> +	ret = ufshcd_query_attr_retry(hba,
> UPIU_QUERY_OPCODE_READ_ATTR,
> +				      QUERY_ATTR_IDN_CURR_WB_BUFF_SIZE,
> +				      index, 0, &cur_buf);
> +	if (ret) {
> +		dev_err(hba->dev, "%s: dCurWriteBoosterBufferSize read
> failed %d\n",
> +			__func__, ret);
> +		return false;
> +	}
> +
> +	if (!cur_buf) {
> +		dev_info(hba->dev, "dCurWBBuf: %d WB disabled until free-
> space is available\n",
> +			 cur_buf);
> +		return false;
> +	}
> +
> +	/* Let it continue to flush when available buffer exceeds threshold */
> +	return avail_buf < hba->vps->wb_flush_threshold;
>  }
> 
>  static void ufshcd_rpm_dev_flush_recheck_work(struct work_struct *work)
> --
> 2.7.4

Re: [PATCH v1 1/1] scsi: ufs: core: Removed ufshcd_wb_presrv_usrspc_keep_vcc_on()

[Bart Van Assche]

On 3/28/25 1:29 AM, Avri Altman wrote:
> Maybe just change the function name to better describe what it does,
> e.g. ufshcd_wb_exceed_threshold ?

I'm also in favor of renaming ufshcd_wb_presrv_usrspc_keep_vcc_on()
instead of inlining it.

Thanks,

Bart.

Re: [PATCH v1 1/1] scsi: ufs: core: Removed ufshcd_wb_presrv_usrspc_keep_vcc_on()

[Bao D. Nguyen]

On 3/28/2025 12:33 PM, Bart Van Assche wrote:
> On 3/28/25 1:29 AM, Avri Altman wrote:
>> Maybe just change the function name to better describe what it does,
>> e.g. ufshcd_wb_exceed_threshold ?
> 
> I'm also in favor of renaming ufshcd_wb_presrv_usrspc_keep_vcc_on()
> instead of inlining it.
> 
Thank you both. I will update.

Thanks, Bao