| Message ID | 20230827233042.12945-1-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: Fix the build for big endian 32-bit ARM systems | expand |
On Sun, Aug 27, 2023, at 19:30, Bart Van Assche wrote: > Although it is not clear to me why, this patch fixes the following build > error for big endian 32-bit ARM systems: > > include/linux/build_bug.h:78:41: error: static assertion failed: > "sizeof(struct utp_upiu_header) == 12" > > Cc: Arnd Bergmann <arnd@arndb.de> > Reported-by: kernel test robot <lkp@intel.com> > Closes: > https://lore.kernel.org/oe-kbuild-all/202308251634.tuRn4OVv-lkp@intel.com/ > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Arnd Bergmann <arnd@arndb.de> The fix makes sense, but I think the description is wrong: The weird struct padding on Arm randconfig builds happens with CONFIG_AEABI disabled (implying the old OABI), regardless of CONFIG_CPU_BIG_ENDIAN. > - union { > - __u8 tm_function; > - __u8 query_function; > - }; > + __u8 tm_or_query_function; > __u8 response; The problem on OABI is that any struct or union is word aligned. I would assume that marking the union as __packed also addresses the problem here, but I have not tested that and your patch seems fine. There are bugs like this in many places of the kernel where the struct alignment actually matters but is broken on OABI, but the machines that used to run OABI kernels in the past also run a very small set of drivers in practice. On my own build test setup, I have made CONFIG_AEABI dependent on !CONFIG_COMILE_TEST, which prevents me from running into this problem (and others) on randconfig builds. Maybe I should try again to send that upstream. Arnd
On 8/27/23 17:28, Arnd Bergmann wrote: > The fix makes sense, but I think the description is wrong: > The weird struct padding on Arm randconfig builds happens > with CONFIG_AEABI disabled (implying the old OABI), > regardless of CONFIG_CPU_BIG_ENDIAN. Thanks for the feedback. I will update the patch description. >> - union { >> - __u8 tm_function; >> - __u8 query_function; >> - }; >> + __u8 tm_or_query_function; >> __u8 response; > > The problem on OABI is that any struct or union is word > aligned. I would assume that marking the union as __packed > also addresses the problem here, but I have not tested that > and your patch seems fine. Marking the union as __packed seems to be sufficient. I prefer that approach because I would like to keep the union. I will post a second version of this patch. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 88daf5cb0fe6..e124f66b1f77 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2645,7 +2645,7 @@ static void ufshcd_prepare_utp_query_req_upiu(struct ufs_hba *hba, .flags = upiu_flags, .lun = lrbp->lun, .task_tag = lrbp->task_tag, - .query_function = query->request.query_func, + .tm_or_query_function = query->request.query_func, /* Data segment length only need for WRITE_DESC */ .data_segment_length = query->request.upiu_req.opcode == @@ -7004,7 +7004,7 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, /* Configure task request UPIU */ treq.upiu_req.req_header.transaction_code = UPIU_TRANSACTION_TASK_REQ; treq.upiu_req.req_header.lun = lun_id; - treq.upiu_req.req_header.tm_function = tm_function; + treq.upiu_req.req_header.tm_or_query_function = tm_function; /* * The host shall provide the same value for LUN field in the basic @@ -7160,7 +7160,7 @@ int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba, enum dev_cmd_type cmd_type = DEV_CMD_TYPE_QUERY; struct utp_task_req_desc treq = { }; enum utp_ocs ocs_value; - u8 tm_f = req_upiu->header.tm_function; + u8 tm_f = req_upiu->header.tm_or_query_function; switch (msgcode) { case UPIU_TRANSACTION_NOP_OUT: diff --git a/include/uapi/scsi/scsi_bsg_ufs.h b/include/uapi/scsi/scsi_bsg_ufs.h index bf1832dc35db..165b8443bde8 100644 --- a/include/uapi/scsi/scsi_bsg_ufs.h +++ b/include/uapi/scsi/scsi_bsg_ufs.h @@ -50,9 +50,8 @@ enum ufs_rpmb_op_type { * @task_tag: Task tag. * @iid: Initiator ID. * @command_set_type: 0 for SCSI command set; 1 for UFS specific. - * @tm_function: Task management function in case of a task management request - * UPIU. - * @query_function: Query function in case of a query request UPIU. + * @tm_or_query_function: Task management function in case of a task management + * request UPIU; query function in case of a query request UPIU. * @response: 0 for success; 1 for failure. * @status: SCSI status if this is the header of a response to a SCSI command. * @ehs_length: EHS length in units of 32 bytes. @@ -80,10 +79,7 @@ struct utp_upiu_header { #else #error #endif - union { - __u8 tm_function; - __u8 query_function; - }; + __u8 tm_or_query_function; __u8 response; __u8 status; __u8 ehs_length;
Although it is not clear to me why, this patch fixes the following build error for big endian 32-bit ARM systems: include/linux/build_bug.h:78:41: error: static assertion failed: "sizeof(struct utp_upiu_header) == 12" Cc: Arnd Bergmann <arnd@arndb.de> Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202308251634.tuRn4OVv-lkp@intel.com/ Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 6 +++--- include/uapi/scsi/scsi_bsg_ufs.h | 10 +++------- 2 files changed, 6 insertions(+), 10 deletions(-)