| Message ID | 20240915074842.4111336-1-avri.altman@wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] scsi: ufs: Zero utp_upiu_req at the beginning of each command | expand |
On 9/15/24 12:48 AM, Avri Altman wrote: > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 8ea5a82503a9..1f6575afc1c5 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)); > @@ -2834,6 +2833,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba, > u8 upiu_flags; > int ret = 0; > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > + > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0); > > if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY) > @@ -2858,6 +2859,8 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) > unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq)); > u8 upiu_flags; > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > + > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, lrbp->cmd->sc_data_direction, 0); > if (ioprio_class == IOPRIO_CLASS_RT) > upiu_flags |= UPIU_CMD_FLAGS_CP; > @@ -7165,6 +7168,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, > > ufshcd_setup_dev_cmd(hba, lrbp, cmd_type, 0, tag); > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > + > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0); > > /* update the task tag in the request upiu */ > @@ -7317,6 +7322,8 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r > > ufshcd_setup_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, UFS_UPIU_RPMB_WLUN, tag); > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > + > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, ehs); > > /* update the task tag */ Something unfortunate about the above patch is that it spreads the initialization of *lrbp->ucd_req_ptr over two functions. Wouldn't it be better to have all the code that initializes *lrbp->ucd_req_ptr in the same function, e.g. as in the untested patch below? diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 8d90af6434da..9d826e5d610b 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2770,13 +2770,14 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags) .task_tag = lrbp->task_tag, .command_set_type = UPIU_COMMAND_SET_TYPE_SCSI, }; + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - + sizeof(ucd_req_ptr->header)); WARN_ON_ONCE(ucd_req_ptr->header.task_tag != lrbp->task_tag); 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)); @@ -2809,6 +2810,8 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba, cpu_to_be16(len) : 0, }; + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - + sizeof(ucd_req_ptr->header)); /* Copy the Query Request buffer as is */ memcpy(&ucd_req_ptr->qr, &query->request.upiu_req, @@ -2825,12 +2828,12 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb *lrbp) { struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr; - memset(ucd_req_ptr, 0, sizeof(struct utp_upiu_req)); - ucd_req_ptr->header = (struct utp_upiu_header){ .transaction_code = UPIU_TRANSACTION_NOP_OUT, .task_tag = lrbp->task_tag, }; + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - + sizeof(ucd_req_ptr->header)); memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp)); } Thanks, Bart.
> > On 9/15/24 12:48 AM, Avri Altman wrote: > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > > index 8ea5a82503a9..1f6575afc1c5 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)); @@ > > -2834,6 +2833,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba > *hba, > > u8 upiu_flags; > > int ret = 0; > > > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > > + > > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, > > 0); > > > > if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY) @@ -2858,6 +2859,8 > > @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb > *lrbp) > > unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq)); > > u8 upiu_flags; > > > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > > + > > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, lrbp->cmd- > >sc_data_direction, 0); > > if (ioprio_class == IOPRIO_CLASS_RT) > > upiu_flags |= UPIU_CMD_FLAGS_CP; @@ -7165,6 +7168,8 @@ > > static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, > > > > ufshcd_setup_dev_cmd(hba, lrbp, cmd_type, 0, tag); > > > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > > + > > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, > > 0); > > > > /* update the task tag in the request upiu */ @@ -7317,6 +7322,8 > > @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct > > utp_upiu_req *r > > > > ufshcd_setup_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, > > UFS_UPIU_RPMB_WLUN, tag); > > > > + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); > > + > > ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, > > ehs); > > > > /* update the task tag */ > > Something unfortunate about the above patch is that it spreads the initialization > of *lrbp->ucd_req_ptr over two functions. Wouldn't it be better to have all the > code that initializes *lrbp->ucd_req_ptr in the same function, e.g. as in the > untested patch below? It does that for the 4 different required instances: command, query, raw query, and rpmb extended header. Is the below proposal evidently better? e.g. with respect of efficiency, simplicity, readability etc.? Thanks, Avri > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index > 8d90af6434da..9d826e5d610b 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2770,13 +2770,14 @@ void ufshcd_prepare_utp_scsi_cmd_upiu(struct > ufshcd_lrb *lrbp, u8 upiu_flags) > .task_tag = lrbp->task_tag, > .command_set_type = UPIU_COMMAND_SET_TYPE_SCSI, > }; > + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - > + sizeof(ucd_req_ptr->header)); > > WARN_ON_ONCE(ucd_req_ptr->header.task_tag != lrbp->task_tag); > > 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)); @@ -2809,6 > +2810,8 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba > *hba, > cpu_to_be16(len) : > 0, > }; > + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - > + sizeof(ucd_req_ptr->header)); > > /* Copy the Query Request buffer as is */ > memcpy(&ucd_req_ptr->qr, &query->request.upiu_req, @@ -2825,12 > +2828,12 @@ static inline void ufshcd_prepare_utp_nop_upiu(struct ufshcd_lrb > *lrbp) > { > struct utp_upiu_req *ucd_req_ptr = lrbp->ucd_req_ptr; > > - memset(ucd_req_ptr, 0, sizeof(struct utp_upiu_req)); > - > ucd_req_ptr->header = (struct utp_upiu_header){ > .transaction_code = UPIU_TRANSACTION_NOP_OUT, > .task_tag = lrbp->task_tag, > }; > + memset(&ucd_req_ptr->header + 1, 0, sizeof(*ucd_req_ptr) - > + sizeof(ucd_req_ptr->header)); > > memset(lrbp->ucd_rsp_ptr, 0, sizeof(struct utp_upiu_rsp)); > } > > Thanks, > > Bart.
On 9/16/24 11:52 PM, Avri Altman wrote: > Is the below proposal evidently better? e.g. with respect of > efficiency, simplicity, readability etc.? The approach I proposed has two advantages: * All code that initializes *lrbp->ucd_req_ptr occurs in the same function. I think that is a significant advantage with regard to maintainability. * ucd_req_ptr->header is initialized once instead of twice. Hence the approach I proposed is more efficient. Thanks, Bart.
> On 9/16/24 11:52 PM, Avri Altman wrote: > > Is the below proposal evidently better? e.g. with respect of > > efficiency, simplicity, readability etc.? > > The approach I proposed has two advantages: > * All code that initializes *lrbp->ucd_req_ptr occurs in the same function. I think > that is a significant advantage with regard to maintainability. > * ucd_req_ptr->header is initialized once instead of twice. Hence the approach I > proposed is more efficient. Sorry, not being stubborn or anything, I might be mis-reading your proposal. My proposal is making 4 changes, attending the 5 upiu types: 1) Zero query upiu and nop upiu in ufshcd_compose_devman_upiu 2) zero command upiu in ufshcd_comp_scsi_upiu 3) zero raw query upiu in ufshcd_issue_devman_upiu_cmd, and 4) zero rpmb extended header (raw command upiu) in ufshcd_advanced_rpmb_req_handler Your proposal is making 3 changes: - zero query upiu in ufshcd_prepare_utp_query_req_upiu - zero nop upiu in ufshcd_prepare_utp_nop_upiu - zero command upiu in ufshcd_prepare_utp_scsi_cmd_upiu And you haven't zero the raw query upiu nor the rpmb extended header . What am I missing? Thanks, Avri > > Thanks, > > Bart.
On 9/17/24 11:03 PM, Avri Altman wrote: > My proposal is making 4 changes, attending the 5 upiu types: > 1) Zero query upiu and nop upiu in ufshcd_compose_devman_upiu > 2) zero command upiu in ufshcd_comp_scsi_upiu > 3) zero raw query upiu in ufshcd_issue_devman_upiu_cmd, and > 4) zero rpmb extended header (raw command upiu) in ufshcd_advanced_rpmb_req_handler > > Your proposal is making 3 changes: > - zero query upiu in ufshcd_prepare_utp_query_req_upiu > - zero nop upiu in ufshcd_prepare_utp_nop_upiu > - zero command upiu in ufshcd_prepare_utp_scsi_cmd_upiu > And you haven't zero the raw query upiu nor the rpmb extended header . Hi Avri, Would it be possible to combine our patches in such a way that all UPIU types are covered and such that the memset() calls for query, nop and command UPIUs occur in the functions that initialize *ucd_req_ptr->header? Thanks, Bart.
> On 9/17/24 11:03 PM, Avri Altman wrote: > > My proposal is making 4 changes, attending the 5 upiu types: > > 1) Zero query upiu and nop upiu in ufshcd_compose_devman_upiu > > 2) zero command upiu in ufshcd_comp_scsi_upiu > > 3) zero raw query upiu in ufshcd_issue_devman_upiu_cmd, and > > 4) zero rpmb extended header (raw command upiu) in > > ufshcd_advanced_rpmb_req_handler > > > > Your proposal is making 3 changes: > > - zero query upiu in ufshcd_prepare_utp_query_req_upiu > > - zero nop upiu in ufshcd_prepare_utp_nop_upiu > > - zero command upiu in ufshcd_prepare_utp_scsi_cmd_upiu And you > > haven't zero the raw query upiu nor the rpmb extended header . > Hi Avri, > > Would it be possible to combine our patches in such a way that all UPIU types are > covered and such that the memset() calls for query, nop and command UPIUs > occur in the functions that initialize *ucd_req_ptr->header? I guess so. One approach is to use the fact that all prep instances calls ufshcd_prepare_req_desc_hdr which instantiate the UTRD header. Although it reduces the memset() calls to a single call, this option is less good IMO because the UTRD header shouldn't have anything to do with the upiu itself. Alternatively, we can move it to ufshcd_setup_dev_cmd in which we'll end up with 2 calls: One for command upiu in ufshcd_comp_scsi_upiu and one for all others in ufshcd_setup_dev_cmd, Which makes more sense to me. Thanks, Avri > > Thanks, > > Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 8ea5a82503a9..1f6575afc1c5 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)); @@ -2834,6 +2833,8 @@ static int ufshcd_compose_devman_upiu(struct ufs_hba *hba, u8 upiu_flags; int ret = 0; + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); + ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0); if (hba->dev_cmd.type == DEV_CMD_TYPE_QUERY) @@ -2858,6 +2859,8 @@ static void ufshcd_comp_scsi_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp) unsigned int ioprio_class = IOPRIO_PRIO_CLASS(req_get_ioprio(rq)); u8 upiu_flags; + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); + ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, lrbp->cmd->sc_data_direction, 0); if (ioprio_class == IOPRIO_CLASS_RT) upiu_flags |= UPIU_CMD_FLAGS_CP; @@ -7165,6 +7168,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba, ufshcd_setup_dev_cmd(hba, lrbp, cmd_type, 0, tag); + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); + ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, 0); /* update the task tag in the request upiu */ @@ -7317,6 +7322,8 @@ int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r ufshcd_setup_dev_cmd(hba, lrbp, DEV_CMD_TYPE_RPMB, UFS_UPIU_RPMB_WLUN, tag); + memset(lrbp->ucd_req_ptr, 0, sizeof(*lrbp->ucd_req_ptr)); + ufshcd_prepare_req_desc_hdr(hba, lrbp, &upiu_flags, DMA_NONE, ehs); /* update the task tag */
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. Signed-off-by: Avri Altman <avri.altman@wdc.com> --- Changes in v2: - Simplify things (Bart) --- drivers/ufs/core/ufshcd.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)