[PATCH 0/4] Re-use device management code fragments

[PATCH 0/4] Re-use device management code fragments

[Avri Altman]

Device management commands are constructed for query commands that are
being issued by the driver, but also for raw device management commands
originated by the bsg module, and recently, by the advanced rpmb
handler. Thus, the same code fragments, e.g. locking, composing the
command, composing the upiu etc., appear over and over. Remove those
duplications.  Theoretically, there should be no functional change.

Avri Altman (4):
  scsi: ufs: Re-use device management locking code
  scsi: ufs: Re-use exec_dev_cmd
  scsi: ufs: Re-use compose_dev_cmd
  scsi: ufs: Re-use compose_devman_upiu

 drivers/ufs/core/ufshcd.c | 200 ++++++++++++++++----------------------
 1 file changed, 83 insertions(+), 117 deletions(-)

-- 
2.42.0

[PATCH 1/4] scsi: ufs: Re-use device management locking code

[Avri Altman]

Group those 3 calls that repeat for every device management command into
a lock and unlock handlers.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 75 +++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 43 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 21429eec1b82..7456f046e7de 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3272,6 +3272,20 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba *hba,
 	return err;
 }
 
+static void ufshcd_dev_man_lock(struct ufs_hba *hba)
+{
+	ufshcd_hold(hba);
+	mutex_lock(&hba->dev_cmd.lock);
+	down_read(&hba->clk_scaling_lock);
+}
+
+static void ufshcd_dev_man_unlock(struct ufs_hba *hba)
+{
+	up_read(&hba->clk_scaling_lock);
+	mutex_unlock(&hba->dev_cmd.lock);
+	ufshcd_release(hba);
+}
+
 /**
  * ufshcd_exec_dev_cmd - API for sending device management requests
  * @hba: UFS hba
@@ -3291,11 +3305,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	struct ufshcd_lrb *lrbp;
 	int err;
 
-	/* Protects use of hba->reserved_slot. */
-	lockdep_assert_held(&hba->dev_cmd.lock);
-
-	down_read(&hba->clk_scaling_lock);
-
 	lrbp = &hba->lrb[tag];
 	lrbp->cmd = NULL;
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
@@ -3312,7 +3321,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
 out:
-	up_read(&hba->clk_scaling_lock);
 	return err;
 }
 
@@ -3383,8 +3391,8 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 
 	BUG_ON(!hba);
 
-	ufshcd_hold(hba);
-	mutex_lock(&hba->dev_cmd.lock);
+	ufshcd_dev_man_lock(hba);
+
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
 			selector);
 
@@ -3426,8 +3434,7 @@ int ufshcd_query_flag(struct ufs_hba *hba, enum query_opcode opcode,
 				MASK_QUERY_UPIU_FLAG_LOC) & 0x1;
 
 out_unlock:
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+	ufshcd_dev_man_unlock(hba);
 	return err;
 }
 
@@ -3457,9 +3464,8 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 		return -EINVAL;
 	}
 
-	ufshcd_hold(hba);
+	ufshcd_dev_man_lock(hba);
 
-	mutex_lock(&hba->dev_cmd.lock);
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
 			selector);
 
@@ -3489,8 +3495,7 @@ int ufshcd_query_attr(struct ufs_hba *hba, enum query_opcode opcode,
 	*attr_val = be32_to_cpu(response->upiu_res.value);
 
 out_unlock:
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+	ufshcd_dev_man_unlock(hba);
 	return err;
 }
 
@@ -3553,9 +3558,8 @@ static int __ufshcd_query_descriptor(struct ufs_hba *hba,
 		return -EINVAL;
 	}
 
-	ufshcd_hold(hba);
+	ufshcd_dev_man_lock(hba);
 
-	mutex_lock(&hba->dev_cmd.lock);
 	ufshcd_init_query(hba, &request, &response, opcode, idn, index,
 			selector);
 	hba->dev_cmd.query.descriptor = desc_buf;
@@ -3588,8 +3592,7 @@ static int __ufshcd_query_descriptor(struct ufs_hba *hba,
 
 out_unlock:
 	hba->dev_cmd.query.descriptor = NULL;
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+	ufshcd_dev_man_unlock(hba);
 	return err;
 }
 
@@ -5070,8 +5073,8 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
 	int err = 0;
 	int retries;
 
-	ufshcd_hold(hba);
-	mutex_lock(&hba->dev_cmd.lock);
+	ufshcd_dev_man_lock(hba);
+
 	for (retries = NOP_OUT_RETRIES; retries > 0; retries--) {
 		err = ufshcd_exec_dev_cmd(hba, DEV_CMD_TYPE_NOP,
 					  hba->nop_out_timeout);
@@ -5081,8 +5084,8 @@ static int ufshcd_verify_dev_init(struct ufs_hba *hba)
 
 		dev_dbg(hba->dev, "%s: error %d retrying\n", __func__, err);
 	}
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+
+	ufshcd_dev_man_unlock(hba);
 
 	if (err)
 		dev_err(hba->dev, "%s: NOP OUT failed %d\n", __func__, err);
@@ -7206,11 +7209,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	int err = 0;
 	u8 upiu_flags;
 
-	/* Protects use of hba->reserved_slot. */
-	lockdep_assert_held(&hba->dev_cmd.lock);
-
-	down_read(&hba->clk_scaling_lock);
-
 	lrbp = &hba->lrb[tag];
 	lrbp->cmd = NULL;
 	lrbp->task_tag = tag;
@@ -7275,7 +7273,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
-	up_read(&hba->clk_scaling_lock);
 	return err;
 }
 
@@ -7314,13 +7311,11 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 		cmd_type = DEV_CMD_TYPE_NOP;
 		fallthrough;
 	case UPIU_TRANSACTION_QUERY_REQ:
-		ufshcd_hold(hba);
-		mutex_lock(&hba->dev_cmd.lock);
+		ufshcd_dev_man_lock(hba);
 		err = ufshcd_issue_devman_upiu_cmd(hba, req_upiu, rsp_upiu,
 						   desc_buff, buff_len,
 						   cmd_type, desc_op);
-		mutex_unlock(&hba->dev_cmd.lock);
-		ufshcd_release(hba);
+		ufshcd_dev_man_unlock(hba);
 
 		break;
 	case UPIU_TRANSACTION_TASK_REQ:
@@ -7380,9 +7375,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	u16 ehs_len;
 
 	/* Protects use of hba->reserved_slot. */
-	ufshcd_hold(hba);
-	mutex_lock(&hba->dev_cmd.lock);
-	down_read(&hba->clk_scaling_lock);
+	ufshcd_dev_man_lock(hba);
 
 	lrbp = &hba->lrb[tag];
 	lrbp->cmd = NULL;
@@ -7448,9 +7441,8 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 		}
 	}
 
-	up_read(&hba->clk_scaling_lock);
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+	ufshcd_dev_man_unlock(hba);
+
 	return err ? : result;
 }
 
@@ -8713,9 +8705,7 @@ static void ufshcd_set_timestamp_attr(struct ufs_hba *hba)
 	if (dev_info->wspecversion < 0x400)
 		return;
 
-	ufshcd_hold(hba);
-
-	mutex_lock(&hba->dev_cmd.lock);
+	ufshcd_dev_man_lock(hba);
 
 	ufshcd_init_query(hba, &request, &response,
 			  UPIU_QUERY_OPCODE_WRITE_ATTR,
@@ -8733,8 +8723,7 @@ static void ufshcd_set_timestamp_attr(struct ufs_hba *hba)
 		dev_err(hba->dev, "%s: failed to set timestamp %d\n",
 			__func__, err);
 
-	mutex_unlock(&hba->dev_cmd.lock);
-	ufshcd_release(hba);
+	ufshcd_dev_man_unlock(hba);
 }
 
 /**
-- 
2.42.0

Re: [PATCH 1/4] scsi: ufs: Re-use device management locking code

[Bart Van Assche]

On 3/4/24 01:23, Avri Altman wrote:
>   /**
>    * ufshcd_exec_dev_cmd - API for sending device management requests
>    * @hba: UFS hba
> @@ -3291,11 +3305,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>   	struct ufshcd_lrb *lrbp;
>   	int err;
>   
> -	/* Protects use of hba->reserved_slot. */
> -	lockdep_assert_held(&hba->dev_cmd.lock);
> -
> -	down_read(&hba->clk_scaling_lock);
> -
>   	lrbp = &hba->lrb[tag];
>   	lrbp->cmd = NULL;
>   	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);

Please restore the lockdep_assert_held() call.

> -	/* Protects use of hba->reserved_slot. */
> -	lockdep_assert_held(&hba->dev_cmd.lock);
> -
> -	down_read(&hba->clk_scaling_lock);
> -
>   	lrbp = &hba->lrb[tag];
>   	lrbp->cmd = NULL;
>   	lrbp->task_tag = tag;

Same comment here - please restore the lockdep_assert_held() call.

Otherwise this patch looks good to me.

Thanks,

Bart.

RE: [PATCH 1/4] scsi: ufs: Re-use device management locking code

[Avri Altman]

 
> On 3/4/24 01:23, Avri Altman wrote:
> >   /**
> >    * ufshcd_exec_dev_cmd - API for sending device management requests
> >    * @hba: UFS hba
> > @@ -3291,11 +3305,6 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba
> *hba,
> >       struct ufshcd_lrb *lrbp;
> >       int err;
> >
> > -     /* Protects use of hba->reserved_slot. */
> > -     lockdep_assert_held(&hba->dev_cmd.lock);
> > -
> > -     down_read(&hba->clk_scaling_lock);
> > -
> >       lrbp = &hba->lrb[tag];
> >       lrbp->cmd = NULL;
> >       err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> 
> Please restore the lockdep_assert_held() call.
Done.

> 
> > -     /* Protects use of hba->reserved_slot. */
> > -     lockdep_assert_held(&hba->dev_cmd.lock);
> > -
> > -     down_read(&hba->clk_scaling_lock);
> > -
> >       lrbp = &hba->lrb[tag];
> >       lrbp->cmd = NULL;
> >       lrbp->task_tag = tag;
> 
> Same comment here - please restore the lockdep_assert_held() call.
Done.

Thanks,
Avri

> 
> Otherwise this patch looks good to me.
> 
> Thanks,
> 
> Bart.

[PATCH 2/4] scsi: ufs: Re-use exec_dev_cmd

[Avri Altman]

Move out the actual command issue from exec_dev_cmd so it can be used
elsewhere.  While at it, as a free bonus, call the upiu trace if it
doesn't.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 52 +++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 7456f046e7de..6661054e4872 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3286,6 +3286,24 @@ static void ufshcd_dev_man_unlock(struct ufs_hba *hba)
 	ufshcd_release(hba);
 }
 
+static int __exec_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
+			  const u32 tag, int timeout)
+{
+	DECLARE_COMPLETION_ONSTACK(wait);
+	int err;
+
+	hba->dev_cmd.complete = &wait;
+
+	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
+
+	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
+	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
+
+	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
+				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
+
+	return err;
+}
 /**
  * ufshcd_exec_dev_cmd - API for sending device management requests
  * @hba: UFS hba
@@ -3300,28 +3318,15 @@ static void ufshcd_dev_man_unlock(struct ufs_hba *hba)
 static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 		enum dev_cmd_type cmd_type, int timeout)
 {
-	DECLARE_COMPLETION_ONSTACK(wait);
 	const u32 tag = hba->reserved_slot;
-	struct ufshcd_lrb *lrbp;
+	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	int err;
 
-	lrbp = &hba->lrb[tag];
-	lrbp->cmd = NULL;
 	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
 	if (unlikely(err))
-		goto out;
-
-	hba->dev_cmd.complete = &wait;
-
-	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
-
-	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
-	err = ufshcd_wait_for_dev_cmd(hba, lrbp, timeout);
-	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
-				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
+		return err;
 
-out:
-	return err;
+	return __exec_dev_cmd(hba, lrbp, tag, timeout);
 }
 
 /**
@@ -7203,7 +7208,6 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 					enum dev_cmd_type cmd_type,
 					enum query_opcode desc_op)
 {
-	DECLARE_COMPLETION_ONSTACK(wait);
 	const u32 tag = hba->reserved_slot;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
@@ -7240,17 +7244,12 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 
 	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 
-	hba->dev_cmd.complete = &wait;
-
-	ufshcd_add_query_upiu_trace(hba, UFS_QUERY_SEND, lrbp->ucd_req_ptr);
-
-	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
 	/*
 	 * ignore the returning value here - ufshcd_check_query_response is
 	 * bound to fail since dev_cmd.query and dev_cmd.type were left empty.
 	 * read the response directly ignoring all errors.
 	 */
-	ufshcd_wait_for_dev_cmd(hba, lrbp, QUERY_REQ_TIMEOUT);
+	__exec_dev_cmd(hba, lrbp, tag, QUERY_REQ_TIMEOUT);
 
 	/* just copy the upiu response as it is */
 	memcpy(rsp_upiu, lrbp->ucd_rsp_ptr, sizeof(*rsp_upiu));
@@ -7365,7 +7364,6 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 			 struct ufs_ehs *rsp_ehs, int sg_cnt, struct scatterlist *sg_list,
 			 enum dma_data_direction dir)
 {
-	DECLARE_COMPLETION_ONSTACK(wait);
 	const u32 tag = hba->reserved_slot;
 	struct ufshcd_lrb *lrbp;
 	int err = 0;
@@ -7412,11 +7410,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 
 	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 
-	hba->dev_cmd.complete = &wait;
-
-	ufshcd_send_command(hba, tag, hba->dev_cmd_queue);
-
-	err = ufshcd_wait_for_dev_cmd(hba, lrbp, ADVANCED_RPMB_REQ_TIMEOUT);
+	err = __exec_dev_cmd(hba, lrbp, tag, ADVANCED_RPMB_REQ_TIMEOUT);
 
 	if (!err) {
 		/* Just copy the upiu response as it is */
-- 
2.42.0

Re: [PATCH 2/4] scsi: ufs: Re-use exec_dev_cmd

[Bart Van Assche]

On 3/4/24 01:23, Avri Altman wrote:
> +static int __exec_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> +			  const u32 tag, int timeout)

Please choose a better name than __exec_dev_cmd. Function names in this 
file should start with the ufshcd_ prefix.

> @@ -3300,28 +3318,15 @@ static void ufshcd_dev_man_unlock(struct ufs_hba *hba)
>   static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>   		enum dev_cmd_type cmd_type, int timeout)
>   {
> -	DECLARE_COMPLETION_ONSTACK(wait);
>   	const u32 tag = hba->reserved_slot;
> -	struct ufshcd_lrb *lrbp;
> +	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
>   	int err;
>   
> -	lrbp = &hba->lrb[tag];
> -	lrbp->cmd = NULL;
>   	err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);

Please restore the "lrbp->cmd = NULL" assignment. I don't think that it
is safe to remove that assignment.

Thanks,

Bart.

RE: [PATCH 2/4] scsi: ufs: Re-use exec_dev_cmd

[Avri Altman]

> On 3/4/24 01:23, Avri Altman wrote:
> > +static int __exec_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> > +                       const u32 tag, int timeout)
> 
> Please choose a better name than __exec_dev_cmd. Function names in this file
> should start with the ufshcd_ prefix.
Done.

> 
> > @@ -3300,28 +3318,15 @@ static void ufshcd_dev_man_unlock(struct
> ufs_hba *hba)
> >   static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
> >               enum dev_cmd_type cmd_type, int timeout)
> >   {
> > -     DECLARE_COMPLETION_ONSTACK(wait);
> >       const u32 tag = hba->reserved_slot;
> > -     struct ufshcd_lrb *lrbp;
> > +     struct ufshcd_lrb *lrbp = &hba->lrb[tag];
> >       int err;
> >
> > -     lrbp = &hba->lrb[tag];
> > -     lrbp->cmd = NULL;
> >       err = ufshcd_compose_dev_cmd(hba, lrbp, cmd_type, tag);
> 
> Please restore the "lrbp->cmd = NULL" assignment. I don't think that it is safe to
> remove that assignment.
This is a redundant assignment - being set to null in ufshcd_compose_dev_cmd.

Thanks,
Avri
> 
> Thanks,
> 
> Bart.

Re: [PATCH 2/4] scsi: ufs: Re-use exec_dev_cmd

[Bart Van Assche]

On 3/5/24 11:54, Avri Altman wrote:
> Bart Van Assche wrote:
>> Please restore the "lrbp->cmd = NULL" assignment. I don't think that it is safe to
>> remove that assignment.
 >
> This is a redundant assignment - being set to null in ufshcd_compose_dev_cmd.

Shouldn't this be mentioned in the patch description?

Thanks,

Bart.

RE: [PATCH 2/4] scsi: ufs: Re-use exec_dev_cmd

[Avri Altman]

> 
> On 3/5/24 11:54, Avri Altman wrote:
> > Bart Van Assche wrote:
> >> Please restore the "lrbp->cmd = NULL" assignment. I don't think that
> >> it is safe to remove that assignment.
>  >
> > This is a redundant assignment - being set to null in
> ufshcd_compose_dev_cmd.
> 
> Shouldn't this be mentioned in the patch description?
Will add.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.

[PATCH 3/4] scsi: ufs: Re-use compose_dev_cmd

[Avri Altman]

Move out some of the dev_cmd initializations so it can be used
elsewhere.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 6661054e4872..a41cd7ed1e38 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -3061,15 +3061,21 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	return err;
 }
 
-static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
-		struct ufshcd_lrb *lrbp, enum dev_cmd_type cmd_type, int tag)
+static void __compose_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
+			     enum dev_cmd_type cmd_type, u8 lun, int tag)
 {
 	lrbp->cmd = NULL;
 	lrbp->task_tag = tag;
-	lrbp->lun = 0; /* device management cmd is not specific to any LUN */
+	lrbp->lun = lun;
 	lrbp->intr_cmd = true; /* No interrupt aggregation */
 	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
 	hba->dev_cmd.type = cmd_type;
+}
+
+static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
+		struct ufshcd_lrb *lrbp, enum dev_cmd_type cmd_type, int tag)
+{
+	__compose_dev_cmd(hba, lrbp, cmd_type, 0, tag);
 
 	return ufshcd_compose_devman_upiu(hba, lrbp);
 }
@@ -7209,17 +7215,11 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 					enum query_opcode desc_op)
 {
 	const u32 tag = hba->reserved_slot;
-	struct ufshcd_lrb *lrbp;
+	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	int err = 0;
 	u8 upiu_flags;
 
-	lrbp = &hba->lrb[tag];
-	lrbp->cmd = NULL;
-	lrbp->task_tag = tag;
-	lrbp->lun = 0;
-	lrbp->intr_cmd = true;
-	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
-	hba->dev_cmd.type = cmd_type;
+	__compose_dev_cmd(hba, lrbp, cmd_type, 0, tag);
 
 	if (hba->ufs_version <= ufshci_version(1, 1))
 		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
@@ -7365,7 +7365,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 			 enum dma_data_direction dir)
 {
 	const u32 tag = hba->reserved_slot;
-	struct ufshcd_lrb *lrbp;
+	struct ufshcd_lrb *lrbp = &hba->lrb[tag];
 	int err = 0;
 	int result;
 	u8 upiu_flags;
@@ -7375,14 +7375,7 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	/* Protects use of hba->reserved_slot. */
 	ufshcd_dev_man_lock(hba);
 
-	lrbp = &hba->lrb[tag];
-	lrbp->cmd = NULL;
-	lrbp->task_tag = tag;
-	lrbp->lun = UFS_UPIU_RPMB_WLUN;
-
-	lrbp->intr_cmd = true;
-	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
-	hba->dev_cmd.type = DEV_CMD_TYPE_RPMB;
+	__compose_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, UFS_UPIU_RPMB_WLUN, tag);
 
 	/* Advanced RPMB starts from UFS 4.0, so its command type is UTP_CMD_TYPE_UFS_STORAGE */
 	lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
-- 
2.42.0

Re: [PATCH 3/4] scsi: ufs: Re-use compose_dev_cmd

[Bart Van Assche]

On 3/4/24 01:23, Avri Altman wrote:
> -static int ufshcd_compose_dev_cmd(struct ufs_hba *hba,
> -		struct ufshcd_lrb *lrbp, enum dev_cmd_type cmd_type, int tag)
> +static void __compose_dev_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> +			     enum dev_cmd_type cmd_type, u8 lun, int tag)
>   {
>   	lrbp->cmd = NULL;
>   	lrbp->task_tag = tag;
> -	lrbp->lun = 0; /* device management cmd is not specific to any LUN */
> +	lrbp->lun = lun;
>   	lrbp->intr_cmd = true; /* No interrupt aggregation */
>   	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
>   	hba->dev_cmd.type = cmd_type;
> +}

Please chose a better function name than __compose_dev_cmd() and please 
make sure that the function name starts with the ufshcd_ prefix.

Thanks,

Bart.

[PATCH 4/4] scsi: ufs: Re-use compose_devman_upiu

[Avri Altman]

Move out some code fragments so it can be used elsewhere.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c | 40 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 25 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index a41cd7ed1e38..b6e27c6daf54 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2840,6 +2840,17 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp)
 	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
 }
 
+static void __compose_devman_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
+				  u8 *upiu_flags, int ehs_length)
+{
+	if (hba->ufs_version <= ufshci_version(1, 1))
+		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
+	else
+		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
+
+	ufshcd_prepare_req_desc_hdr(lrbp, upiu_flags, DMA_NONE, ehs_length);
+}
+
 /**
  * ufshcd_compose_devman_upiu - UFS Protocol Information Unit(UPIU)
  *			     for Device Management Purposes
@@ -2854,12 +2865,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba,
 	u8 upiu_flags;
 	int ret = 0;
 
-	if (hba->ufs_version <= ufshci_version(1, 1))
-		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
-	else
-		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
+	__compose_devman_upiu(hba, lrbp, &upiu_flags, 0);
 
-	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
 	if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY)
 		ufshcd_prepare_utp_query_req_upiu(hba, lrbp, upiu_flags);
 	else if (hba->dev_cmd.type == DEV_CMD_TYPE_NOP)
@@ -7220,17 +7227,11 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	u8 upiu_flags;
 
 	__compose_dev_cmd(hba, lrbp, cmd_type, 0, tag);
-
-	if (hba->ufs_version <= ufshci_version(1, 1))
-		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
-	else
-		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
+	__compose_devman_upiu(hba, lrbp, &upiu_flags, 0);
 
 	/* update the task tag in the request upiu */
 	req_upiu->header.task_tag = tag;
 
-	ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, DMA_NONE, 0);
-
 	/* just copy the upiu request as it is */
 	memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
 	if (desc_buff && desc_op == UPIU_QUERY_OPCODE_WRITE_DESC) {
@@ -7371,24 +7372,13 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	u8 upiu_flags;
 	u8 *ehs_data;
 	u16 ehs_len;
+	int ehs = (hba->capabilities & MASK_EHSLUTRD_SUPPORTED) ? 2 : 0;
 
 	/* Protects use of hba->reserved_slot. */
 	ufshcd_dev_man_lock(hba);
 
 	__compose_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, UFS_UPIU_RPMB_WLUN, tag);
-
-	/* Advanced RPMB starts from UFS 4.0, so its command type is UTP_CMD_TYPE_UFS_STORAGE */
-	lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
-
-	/*
-	 * According to UFSHCI 4.0 specification page 24, if EHSLUTRDS is 0, host controller takes
-	 * EHS length from CMD UPIU, and SW driver use EHS Length field in CMD UPIU. if it is 1,
-	 * HW controller takes EHS length from UTRD.
-	 */
-	if (hba->capabilities & MASK_EHSLUTRD_SUPPORTED)
-		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, dir, 2);
-	else
-		ufshcd_prepare_req_desc_hdr(lrbp, &upiu_flags, dir, 0);
+	__compose_devman_upiu(hba, lrbp, &upiu_flags, ehs);
 
 	/* update the task tag */
 	req_upiu->header.task_tag = tag;
-- 
2.42.0

Re: [PATCH 4/4] scsi: ufs: Re-use compose_devman_upiu

[Bart Van Assche]

On 3/4/24 01:23, Avri Altman wrote:
> +static void __compose_devman_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> +				  u8 *upiu_flags, int ehs_length)
> +{
> +	if (hba->ufs_version <= ufshci_version(1, 1))
> +		lrbp->command_type = UTP_CMD_TYPE_DEV_MANAGE;
> +	else
> +		lrbp->command_type = UTP_CMD_TYPE_UFS_STORAGE;
> +
> +	ufshcd_prepare_req_desc_hdr(lrbp, upiu_flags, DMA_NONE, ehs_length);
> +}

Please move the above if-statement into ufshcd_prepare_req_desc_hdr() 
instead of introducing yet another helper function.

Thanks,

Bart.