diff mbox series

scsi: ufs: Zero utp_upiu_req at the beginning of each command

Message ID 20240911053951.4032533-1-avri.altman@wdc.com (mailing list archive)
State Superseded
Headers show
Series scsi: ufs: Zero utp_upiu_req at the beginning of each command | expand

Commit Message

Avri Altman Sept. 11, 2024, 5:39 a.m. UTC
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.

No changes to  struct utp_upiu_req memory layout: not its size nor
cacheline usage.

Signed-off-by: Avri Altman <avri.altman@wdc.com>
---
 drivers/ufs/core/ufshcd.c        | 14 +++++++++++++-
 include/uapi/scsi/scsi_bsg_ufs.h | 12 +++++++-----
 2 files changed, 20 insertions(+), 6 deletions(-)

Comments

Bart Van Assche Sept. 12, 2024, 1:32 a.m. UTC | #1
On 9/10/24 10:39 PM, Avri Altman wrote:
> +static void zero_utp_upiu(struct utp_upiu_req *req)
> +{
> +	memset(&req->utp_upiu, 0, sizeof(req->utp_upiu));
> +}

Introducing a function that only calls memset() seems like overkill to
me. Please call memset() directly.

> diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
> index 8c29e498ef98..b0d60d54d6c9 100644
> --- a/include/uapi/scsi/scsi_bsg_ufs.h
> +++ b/include/uapi/scsi/scsi_bsg_ufs.h
> @@ -162,11 +162,13 @@ struct utp_upiu_cmd {
>    */
>   struct utp_upiu_req {
>   	struct utp_upiu_header header;
> -	union {
> -		struct utp_upiu_cmd		sc;
> -		struct utp_upiu_query		qr;
> -		struct utp_upiu_query		uc;
> -	};
> +	struct_group(utp_upiu,
> +		union {
> +			struct utp_upiu_cmd	sc;
> +			struct utp_upiu_query	qr;
> +			struct utp_upiu_query	uc;
> +		};
> +	);
>   };

Is the above change perhaps independent of the rest of this patch? I
think that this change can be left out.

Thanks,

Bart.
kernel test robot Sept. 12, 2024, 2:58 a.m. UTC | #2
Hi Avri,

kernel test robot noticed the following build errors:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next linus/master v6.11-rc7 next-20240911]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Avri-Altman/scsi-ufs-Zero-utp_upiu_req-at-the-beginning-of-each-command/20240911-134349
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20240911053951.4032533-1-avri.altman%40wdc.com
patch subject: [PATCH] scsi: ufs: Zero utp_upiu_req at the beginning of each command
config: x86_64-buildonly-randconfig-004-20240912 (https://download.01.org/0day-ci/archive/20240912/202409121041.ZoEfIT5S-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240912/202409121041.ZoEfIT5S-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409121041.ZoEfIT5S-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <built-in>:1:
>> ./usr/include/scsi/scsi_bsg_ufs.h:165:2: error: type name requires a specifier or qualifier
     165 |         struct_group(utp_upiu,
         |         ^
>> ./usr/include/scsi/scsi_bsg_ufs.h:166:3: error: expected identifier
     166 |                 union {
         |                 ^
>> ./usr/include/scsi/scsi_bsg_ufs.h:170:4: error: unexpected ';' before ')'
     170 |                 };
         |                  ^
   3 errors generated.
kernel test robot Sept. 12, 2024, 5:52 a.m. UTC | #3
Hi Avri,

kernel test robot noticed the following build errors:

[auto build test ERROR on mkp-scsi/for-next]
[also build test ERROR on jejb-scsi/for-next linus/master v6.11-rc7 next-20240911]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Avri-Altman/scsi-ufs-Zero-utp_upiu_req-at-the-beginning-of-each-command/20240911-134349
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20240911053951.4032533-1-avri.altman%40wdc.com
patch subject: [PATCH] scsi: ufs: Zero utp_upiu_req at the beginning of each command
config: i386-buildonly-randconfig-001-20240912 (https://download.01.org/0day-ci/archive/20240912/202409121226.ngZ7ruK0-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240912/202409121226.ngZ7ruK0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409121226.ngZ7ruK0-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from <command-line>:
>> ./usr/include/scsi/scsi_bsg_ufs.h:165:9: error: expected specifier-qualifier-list before 'struct_group'
     165 |         struct_group(utp_upiu,
         |         ^~~~~~~~~~~~
Avri Altman Sept. 12, 2024, 6:51 a.m. UTC | #4
> 
> On 9/10/24 10:39 PM, Avri Altman wrote:
> > +static void zero_utp_upiu(struct utp_upiu_req *req) {
> > +     memset(&req->utp_upiu, 0, sizeof(req->utp_upiu)); }
> 
> Introducing a function that only calls memset() seems like overkill to me. Please
> call memset() directly.
Done.

> 
> > diff --git a/include/uapi/scsi/scsi_bsg_ufs.h
> > b/include/uapi/scsi/scsi_bsg_ufs.h
> > index 8c29e498ef98..b0d60d54d6c9 100644
> > --- a/include/uapi/scsi/scsi_bsg_ufs.h
> > +++ b/include/uapi/scsi/scsi_bsg_ufs.h
> > @@ -162,11 +162,13 @@ struct utp_upiu_cmd {
> >    */
> >   struct utp_upiu_req {
> >       struct utp_upiu_header header;
> > -     union {
> > -             struct utp_upiu_cmd             sc;
> > -             struct utp_upiu_query           qr;
> > -             struct utp_upiu_query           uc;
> > -     };
> > +     struct_group(utp_upiu,
> > +             union {
> > +                     struct utp_upiu_cmd     sc;
> > +                     struct utp_upiu_query   qr;
> > +                     struct utp_upiu_query   uc;
> > +             };
> > +     );
> >   };
> 
> Is the above change perhaps independent of the rest of this patch? I think that
> this change can be left out.
Done.

P.S.
I wanted to make zero_utp_upiu() oblivious of its callers.
I guess I can just do that by just naming that union.

Thanks,
Avri

> 
> Thanks,
> 
> Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 8ea5a82503a9..0f7ad1acfda0 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2735,6 +2735,11 @@  ufshcd_prepare_req_desc_hdr(struct ufs_hba *hba, struct ufshcd_lrb *lrbp,
 	req_desc->prd_table_length = 0;
 }
 
+static void zero_utp_upiu(struct utp_upiu_req *req)
+{
+	memset(&req->utp_upiu, 0, sizeof(req->utp_upiu));
+}
+
 /**
  * ufshcd_prepare_utp_scsi_cmd_upiu() - fills the utp_transfer_req_desc,
  * for scsi commands
@@ -2758,10 +2763,11 @@  void ufshcd_prepare_utp_scsi_cmd_upiu(struct ufshcd_lrb *lrbp, u8 upiu_flags)
 
 	WARN_ON_ONCE(ucd_req_ptr->header.task_tag != lrbp->task_tag);
 
+	zero_utp_upiu(ucd_req_ptr);
+
 	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));
@@ -2795,6 +2801,8 @@  static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba,
 				0,
 	};
 
+	zero_utp_upiu(ucd_req_ptr);
+
 	/* Copy the Query Request buffer as is */
 	memcpy(&ucd_req_ptr->qr, &query->request.upiu_req,
 			QUERY_OSF_SIZE);
@@ -7170,6 +7178,8 @@  static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	/* update the task tag in the request upiu */
 	req_upiu->header.task_tag = tag;
 
+	zero_utp_upiu(lrbp->ucd_req_ptr);
+
 	/* 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) {
@@ -7322,6 +7332,8 @@  int ufshcd_advanced_rpmb_req_handler(struct ufs_hba *hba, struct utp_upiu_req *r
 	/* update the task tag */
 	req_upiu->header.task_tag = tag;
 
+	zero_utp_upiu(lrbp->ucd_req_ptr);
+
 	/* copy the UPIU(contains CDB) request as it is */
 	memcpy(lrbp->ucd_req_ptr, req_upiu, sizeof(*lrbp->ucd_req_ptr));
 	/* Copy EHS, starting with byte32, immediately after the CDB package */
diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h
index 8c29e498ef98..b0d60d54d6c9 100644
--- a/include/uapi/scsi/scsi_bsg_ufs.h
+++ b/include/uapi/scsi/scsi_bsg_ufs.h
@@ -162,11 +162,13 @@  struct utp_upiu_cmd {
  */
 struct utp_upiu_req {
 	struct utp_upiu_header header;
-	union {
-		struct utp_upiu_cmd		sc;
-		struct utp_upiu_query		qr;
-		struct utp_upiu_query		uc;
-	};
+	struct_group(utp_upiu,
+		union {
+			struct utp_upiu_cmd	sc;
+			struct utp_upiu_query	qr;
+			struct utp_upiu_query	uc;
+		};
+	);
 };
 
 struct ufs_arpmb_meta {