[PATCH v4] scsi: ufs: Zero utp_upiu_req at the beginning of each command

[PATCH v4] scsi: ufs: Zero utp_upiu_req at the beginning of each command

[Avri Altman]

This patch introduces a previously missing step: zeroing the
`utp_upiu_req` structure at the beginning of each upiu transaction. This
ensures that the upiu request fields are properly initialized,
preventing potential issues caused by residual data from previous
commands.

While at it, re-use some of the common initializations for query and
command upiu.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Avri Altman <avri.altman@wdc.com>

---
Changes in v4:
 - Remove a redundant argument (Bart)

Changes in v3:
 - initialize *ucd_req_ptr once (Bart)

Changes in v2:
 - Simplify things (Bart)
---
 drivers/ufs/core/ufshcd.c | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8ea5a82503a9..9187cf5c949c 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2761,7 +2761,6 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
 	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
 
 	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
-	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
 	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
 
 	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
@@ -2864,6 +2863,26 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 	ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
 }
 
+static void __ufshcd_setup_cmd(struct ufshcd_lrb *lrbp, struct scsi_cmnd *cmd, u8 lun, int tag)
+{
+	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
+
+	lrbp->cmd = cmd;
+	lrbp->task_tag = tag;
+	lrbp->lun = lun;
+	ufshcd_prepare_lrbp_crypto(cmd ? scsi_cmd_to_rq(cmd) : NULL, lrbp);
+}
+
+static void ufshcd_setup_scsi_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
+				  struct scsi_cmnd *cmd, u8 lun, int tag)
+{
+	__ufshcd_setup_cmd(lrbp, cmd, lun, tag);
+	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba);
+	lrbp->req_abort_skip = false;
+
+	ufshcd_comp_scsi_upiu(hba, lrbp);
+}
+
 /**
  * ufshcd_upiu_wlun_to_scsi_wlun - maps UPIU W-LUN id to SCSI W-LUN ID
  * @upiu_wlun_id: UPIU W-LUN id
@@ -2997,16 +3016,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	ufshcd_hold(hba);
 
 	lrbp = &hba->lrb[tag];
-	lrbp->cmd = cmd;
-	lrbp->task_tag = tag;
-	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
-	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba);
 
-	ufshcd_prepare_lrbp_crypto(scsi_cmd_to_rq(cmd), lrbp);
-
-	lrbp->req_abort_skip = false;
-
-	ufshcd_comp_scsi_upiu(hba, lrbp);
+	ufshcd_setup_scsi_cmd(hba, lrbp, cmd, ufshcd_scsi_to_upiu_lun(cmd->device->lun), tag);
 
 	err = ufshcd_map_sg(hba, lrbp);
 	if (err) {
@@ -3034,11 +3045,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 static void ufshcd_setup_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 = lun;
+	__ufshcd_setup_cmd(lrbp, NULL, lun, tag);
 	lrbp->intr_cmd = true; /* No interrupt aggregation */
-	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
 	hba->dev_cmd.type = cmd_type;
 }
 
-- 
2.25.1

Re: [PATCH v4] scsi: ufs: Zero utp_upiu_req at the beginning of each command

[Bart Van Assche]

On 9/20/24 11:23 PM, Avri Altman wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 8ea5a82503a9..9187cf5c949c 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2761,7 +2761,6 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
>   	ucd_req_ptr->sc.exp_data_transfer_len = cpu_to_be32(cmd->sdb.length);
>   
>   	cdb_len = min_t(unsigned short, cmd->cmd_len, UFS_CDB_SIZE);
> -	memset(ucd_req_ptr->sc.cdb, 0, UFS_CDB_SIZE);
>   	memcpy(ucd_req_ptr->sc.cdb, cmd->cmnd, cdb_len);
>   
>   	memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp));
> @@ -2864,6 +2863,26 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>   	ufshcd_prepare_utp_scsi_cmd_upiu(lrbp, upiu_flags);
>   }
>   
> +static void __ufshcd_setup_cmd(struct ufshcd_lrb *lrbp, struct scsi_cmnd *cmd, u8 lun, int tag)
> +{
> +	memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr));
> +
> +	lrbp->cmd = cmd;
> +	lrbp->task_tag = tag;
> +	lrbp->lun = lun;
> +	ufshcd_prepare_lrbp_crypto(cmd ? scsi_cmd_to_rq(cmd) : NULL, lrbp);
> +}
> +
> +static void ufshcd_setup_scsi_cmd(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
> +				  struct scsi_cmnd *cmd, u8 lun, int tag)
> +{
> +	__ufshcd_setup_cmd(lrbp, cmd, lun, tag);
> +	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba);
> +	lrbp->req_abort_skip = false;
> +
> +	ufshcd_comp_scsi_upiu(hba, lrbp);
> +}
> +
>   /**
>    * ufshcd_upiu_wlun_to_scsi_wlun - maps UPIU W-LUN id to SCSI W-LUN ID
>    * @upiu_wlun_id: UPIU W-LUN id
> @@ -2997,16 +3016,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   	ufshcd_hold(hba);
>   
>   	lrbp = &hba->lrb[tag];
> -	lrbp->cmd = cmd;
> -	lrbp->task_tag = tag;
> -	lrbp->lun = ufshcd_scsi_to_upiu_lun(cmd->device->lun);
> -	lrbp->intr_cmd = !ufshcd_is_intr_aggr_allowed(hba);
>   
> -	ufshcd_prepare_lrbp_crypto(scsi_cmd_to_rq(cmd), lrbp);
> -
> -	lrbp->req_abort_skip = false;
> -
> -	ufshcd_comp_scsi_upiu(hba, lrbp);
> +	ufshcd_setup_scsi_cmd(hba, lrbp, cmd, ufshcd_scsi_to_upiu_lun(cmd->device->lun), tag);
>   
>   	err = ufshcd_map_sg(hba, lrbp);
>   	if (err) {
> @@ -3034,11 +3045,8 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   static void ufshcd_setup_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 = lun;
> +	__ufshcd_setup_cmd(lrbp, NULL, lun, tag);
>   	lrbp->intr_cmd = true; /* No interrupt aggregation */
> -	ufshcd_prepare_lrbp_crypto(NULL, lrbp);
>   	hba->dev_cmd.type = cmd_type;
>   }
>   

This should have been two patches: one patch that introduces the
ufshcd_setup_scsi_cmd() function and a second patch that adds the code
for zeroing the UPIU header. Anyway, I'm fine with this patch.

Bart.

Re: [PATCH v4] scsi: ufs: Zero utp_upiu_req at the beginning of each command

[Martin K. Petersen]

Avri,

> This patch introduces a previously missing step: zeroing the
> `utp_upiu_req` structure at the beginning of each upiu transaction.
> This ensures that the upiu request fields are properly initialized,
> preventing potential issues caused by residual data from previous
> commands.
>
> While at it, re-use some of the common initializations for query and
> command upiu.

Applied to 6.13/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering