diff mbox series

[v2,11/12] scsi: ufs: Simplify transfer request header initialization

Message ID 20230727194457.3152309-12-bvanassche@acm.org
State New
Headers show
Series Multiple cleanup patches for the UFS driver | expand

Commit Message

Bart Van Assche July 27, 2023, 7:41 p.m. UTC 
Make the code that initializes UTP transfer request headers easier to
read by using bitfields instead of __le32 where appropriate.

Cc: Bao D. Nguyen <quic_nguyenb@quicinc.com>
Cc: Eric Biggers <ebiggers@google.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufs-mcq.c       |  7 +--
 drivers/ufs/core/ufshcd-crypto.h | 20 ++++-----
 drivers/ufs/core/ufshcd.c        | 77 ++++++++++++++++++++++----------
 include/ufs/ufs.h                |  3 --
 include/ufs/ufshci.h             | 50 ++++++++++++++-------
 5 files changed, 99 insertions(+), 58 deletions(-)

Comments

Avri Altman July 28, 2023, 1:40 p.m. UTC | #1 
> Make the code that initializes UTP transfer request headers easier to read by
> using bitfields instead of __le32 where appropriate.
> 
> Cc: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Cc: Eric Biggers <ebiggers@google.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
Kumar, Udit Aug. 2, 2023, 11:25 a.m. UTC | #2 
Hi Bart,

On 7/28/2023 1:11 AM, Bart Van Assche wrote:
> Make the code that initializes UTP transfer request headers easier to
> read by using bitfields instead of __le32 where appropriate.
>
> Cc: Bao D. Nguyen <quic_nguyenb@quicinc.com>
> Cc: Eric Biggers <ebiggers@google.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   [...]
>   
> +static void ufshcd_check_header_layout(void)
> +{
> +	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
> +				.cci = 3})[0] != 3);
> +
> +	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
> +				.ehs_length = 2})[1] != 2);
> +
> +	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
> +				.enable_crypto = 1})[2]
> +		     != 0x80);
> +
> +	BUILD_BUG_ON((((u8 *)&(struct request_desc_header){
> +					.command_type = 5,
> +					.data_direction = 3,
> +					.interrupt = 1,
> +				})[3]) != ((5 << 4) | (3 << 1) | 1));
> +
> +	BUILD_BUG_ON(((__le32 *)&(struct request_desc_header){
> +				.dunl = cpu_to_le32(0xdeadbeef)})[1] !=
> +		cpu_to_le32(0xdeadbeef));
> +
> +	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
> +				.ocs = 4})[8] != 4);
> +
> +	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
> +				.cds = 5})[9] != 5);
> +
> +	BUILD_BUG_ON(((__le32 *)&(struct request_desc_header){
> +				.dunu = cpu_to_le32(0xbadcafe)})[3] !=
> +		cpu_to_le32(0xbadcafe));
> +}
> +

While building next-20230801 for ARM64 architecture,

this patch is giving compilation error

In function ‘ufshcd_check_header_layout’,
     inlined from ‘ufshcd_core_init’ at drivers/ufs/core/ufshcd.c:10629:2:
././include/linux/compiler_types.h:397:38: error: call to 
‘__compiletime_assert_554’ declared with attribute error: BUILD_BUG_ON 
failed: ((u8 *)&(struct request_desc_header){ .enable_crypto = 1})[2] != 
0x80
   397 |  _compiletime_assert(condition, msg, __compiletime_assert_, 
__COUNTER__)


compiler information

wget 
https://developer.arm.com/-/media/Files/downloads/gnu-a/9.2-2019.12/binrel/gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu.tar.xz

tar -Jxvf gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu.tar.xz

Build steps

make ARCH=arm64 CROSS_COMPILE=aarch64-none-linux-gnu- defconfig

make ARCH=arm64 CROSS_COMPILE=aarch64-none-linux-gnu- Image


>   [...]
Bart Van Assche Aug. 2, 2023, 4:05 p.m. UTC | #3 
On 8/2/23 04:25, Kumar, Udit wrote:
> While building next-20230801 for ARM64 architecture,
> 
> this patch is giving compilation error
> 
> In function ‘ufshcd_check_header_layout’,
>      inlined from ‘ufshcd_core_init’ at drivers/ufs/core/ufshcd.c:10629:2:
> ././include/linux/compiler_types.h:397:38: error: call to 
> ‘__compiletime_assert_554’ declared with attribute error: BUILD_BUG_ON 
> failed: ((u8 *)&(struct request_desc_header){ .enable_crypto = 1})[2] != 
> 0x80
>    397 |  _compiletime_assert(condition, msg, __compiletime_assert_, 
> __COUNTER__)
> 
> 
> compiler information
> 
> wget 
> https://developer.arm.com/-/media/Files/downloads/gnu-a/9.2-2019.12/binrel/gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu.tar.xz

Does this patch help: 
https://lore.kernel.org/linux-scsi/20230801232204.1481902-1-bvanassche@acm.org/?

Thanks,

Bart.
Kumar, Udit Aug. 2, 2023, 4:44 p.m. UTC | #4 
Thanks Bart

>
>On 8/2/23 04:25, Kumar, Udit wrote:
>> While building next-20230801 for ARM64 architecture,
>>
>> this patch is giving compilation error
>>
>> In function ‘ufshcd_check_header_layout’,
>>      inlined from ‘ufshcd_core_init’ at drivers/ufs/core/ufshcd.c:10629:2:
>> ././include/linux/compiler_types.h:397:38: error: call to
>> ‘__compiletime_assert_554’ declared with attribute error: BUILD_BUG_ON
>> failed: ((u8 *)&(struct request_desc_header){ .enable_crypto = 1})[2]
>> !=
>> 0x80
>>    397 |  _compiletime_assert(condition, msg, __compiletime_assert_,
>> __COUNTER__)
>>
>>
>> compiler information
>>
>> wget
>> https://developer.arm.com/-/media/Files/downloads/gnu-a/9.2-2019.12/bi
>> nrel/gcc-arm-9.2-2019.12-x86_64-aarch64-none-linux-gnu.tar.xz
>
>Does this patch help:
>https://lore.kernel.org/linux-scsi/20230801232204.1481902-1-
>bvanassche@acm.org/?

Yes , build works for me with this patch.
You just skipped based upon compiler version  but 
what about checks you want to do in this function. 


>Thanks,
>
>Bart.
Bart Van Assche Aug. 2, 2023, 4:46 p.m. UTC | #5 
On 8/2/23 09:44, Kumar, Udit wrote:
>> Does this patch help:
>> https://lore.kernel.org/linux-scsi/20230801232204.1481902-1-
>> bvanassche@acm.org/?
> 
> Yes , build works for me with this patch.
> You just skipped based upon compiler version  but
> what about checks you want to do in this function.

These checks verify that the layout of the structures matches the layout 
from the UFS standard. I want these checks to performed at compile time. 
I'm not sure how to let gcc 9 or older perform these checks at compile time.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufs-mcq.c b/drivers/ufs/core/ufs-mcq.c
index a3d4ef8aa3b9..66a4e24484a3 100644
--- a/drivers/ufs/core/ufs-mcq.c
+++ b/drivers/ufs/core/ufs-mcq.c
@@ -558,12 +558,7 @@  int ufshcd_mcq_sq_cleanup(struct ufs_hba *hba, int task_tag)
  */
 static void ufshcd_mcq_nullify_sqe(struct utp_transfer_req_desc *utrd)
 {
-	u32 dword_0;
-
-	dword_0 = le32_to_cpu(utrd->header.dword_0);
-	dword_0 &= ~UPIU_COMMAND_TYPE_MASK;
-	dword_0 |= FIELD_PREP(UPIU_COMMAND_TYPE_MASK, 0xF);
-	utrd->header.dword_0 = cpu_to_le32(dword_0);
+	utrd->header.command_type = 0xf;
 }
 
 /**
diff --git a/drivers/ufs/core/ufshcd-crypto.h b/drivers/ufs/core/ufshcd-crypto.h
index 504cc841540b..be8596f20ba2 100644
--- a/drivers/ufs/core/ufshcd-crypto.h
+++ b/drivers/ufs/core/ufshcd-crypto.h
@@ -26,15 +26,15 @@  static inline void ufshcd_prepare_lrbp_crypto(struct request *rq,
 }
 
 static inline void
-ufshcd_prepare_req_desc_hdr_crypto(struct ufshcd_lrb *lrbp, u32 *dword_0,
-				   u32 *dword_1, u32 *dword_3)
+ufshcd_prepare_req_desc_hdr_crypto(struct ufshcd_lrb *lrbp,
+				   struct request_desc_header *h)
 {
-	if (lrbp->crypto_key_slot >= 0) {
-		*dword_0 |= UTP_REQ_DESC_CRYPTO_ENABLE_CMD;
-		*dword_0 |= lrbp->crypto_key_slot;
-		*dword_1 = lower_32_bits(lrbp->data_unit_num);
-		*dword_3 = upper_32_bits(lrbp->data_unit_num);
-	}
+	if (lrbp->crypto_key_slot < 0)
+		return;
+	h->enable_crypto = 1;
+	h->cci = lrbp->crypto_key_slot;
+	h->dunl = cpu_to_le32(lower_32_bits(lrbp->data_unit_num));
+	h->dunu = cpu_to_le32(upper_32_bits(lrbp->data_unit_num));
 }
 
 bool ufshcd_crypto_enable(struct ufs_hba *hba);
@@ -51,8 +51,8 @@  static inline void ufshcd_prepare_lrbp_crypto(struct request *rq,
 					      struct ufshcd_lrb *lrbp) { }
 
 static inline void
-ufshcd_prepare_req_desc_hdr_crypto(struct ufshcd_lrb *lrbp, u32 *dword_0,
-				   u32 *dword_1, u32 *dword_3) { }
+ufshcd_prepare_req_desc_hdr_crypto(struct ufshcd_lrb *lrbp,
+				   struct request_desc_header *h) { }
 
 static inline bool ufshcd_crypto_enable(struct ufs_hba *hba)
 {
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 4348b0dfde29..b85c7a28fff5 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -794,7 +794,7 @@  static enum utp_ocs ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp,
 	if (cqe)
 		return le32_to_cpu(cqe->status) & MASK_OCS;
 
-	return le32_to_cpu(lrbp->utr_descriptor_ptr->header.dword_2) & MASK_OCS;
+	return lrbp->utr_descriptor_ptr->header.ocs & MASK_OCS;
 }
 
 /**
@@ -2587,10 +2587,10 @@  static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, u8 *upiu_flags,
 					enum dma_data_direction cmd_dir, int ehs_length)
 {
 	struct utp_transfer_req_desc *req_desc = lrbp->utr_descriptor_ptr;
-	u32 data_direction;
-	u32 dword_0;
-	u32 dword_1 = 0;
-	u32 dword_3 = 0;
+	struct request_desc_header *h = &req_desc->header;
+	enum utp_data_direction data_direction;
+
+	*h = (typeof(*h)){ };
 
 	if (cmd_dir == DMA_FROM_DEVICE) {
 		data_direction = UTP_DEVICE_TO_HOST;
@@ -2603,25 +2603,22 @@  static void ufshcd_prepare_req_desc_hdr(struct ufshcd_lrb *lrbp, u8 *upiu_flags,
 		*upiu_flags = UPIU_CMD_FLAGS_NONE;
 	}
 
-	dword_0 = data_direction | (lrbp->command_type << UPIU_COMMAND_TYPE_OFFSET) |
-		ehs_length << 8;
+	h->command_type = lrbp->command_type;
+	h->data_direction = data_direction;
+	h->ehs_length = ehs_length;
+
 	if (lrbp->intr_cmd)
-		dword_0 |= UTP_REQ_DESC_INT_CMD;
+		h->interrupt = 1;
 
 	/* Prepare crypto related dwords */
-	ufshcd_prepare_req_desc_hdr_crypto(lrbp, &dword_0, &dword_1, &dword_3);
+	ufshcd_prepare_req_desc_hdr_crypto(lrbp, h);
 
-	/* Transfer request descriptor header fields */
-	req_desc->header.dword_0 = cpu_to_le32(dword_0);
-	req_desc->header.dword_1 = cpu_to_le32(dword_1);
 	/*
 	 * assigning invalid value for command status. Controller
 	 * updates OCS on command completion, with the command
 	 * status
 	 */
-	req_desc->header.dword_2 =
-		cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
-	req_desc->header.dword_3 = cpu_to_le32(dword_3);
+	h->ocs = OCS_INVALID_COMMAND_STATUS;
 
 	req_desc->prd_table_length = 0;
 }
@@ -5445,8 +5442,7 @@  void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
 		if (hba->dev_cmd.complete) {
 			if (cqe) {
 				ocs = le32_to_cpu(cqe->status) & MASK_OCS;
-				lrbp->utr_descriptor_ptr->header.dword_2 =
-					cpu_to_le32(ocs);
+				lrbp->utr_descriptor_ptr->header.ocs = ocs;
 			}
 			complete(hba->dev_cmd.complete);
 			ufshcd_clk_scaling_update_busy(hba);
@@ -7034,8 +7030,8 @@  static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
 	int err;
 
 	/* Configure task request descriptor */
-	treq.header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
-	treq.header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+	treq.header.interrupt = 1;
+	treq.header.ocs = OCS_INVALID_COMMAND_STATUS;
 
 	/* Configure task request UPIU */
 	treq.upiu_req.req_header.dword_0 = cpu_to_be32(lun_id << 8) |
@@ -7053,7 +7049,7 @@  static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id,
 	if (err == -ETIMEDOUT)
 		return err;
 
-	ocs_value = le32_to_cpu(treq.header.dword_2) & MASK_OCS;
+	ocs_value = treq.header.ocs & MASK_OCS;
 	if (ocs_value != OCS_SUCCESS)
 		dev_err(hba->dev, "%s: failed, ocs = 0x%x\n",
 				__func__, ocs_value);
@@ -7213,8 +7209,8 @@  int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 
 		break;
 	case UPIU_TRANSACTION_TASK_REQ:
-		treq.header.dword_0 = cpu_to_le32(UTP_REQ_DESC_INT_CMD);
-		treq.header.dword_2 = cpu_to_le32(OCS_INVALID_COMMAND_STATUS);
+		treq.header.interrupt = 1;
+		treq.header.ocs = OCS_INVALID_COMMAND_STATUS;
 
 		memcpy(&treq.upiu_req, req_upiu, sizeof(*req_upiu));
 
@@ -7222,7 +7218,7 @@  int ufshcd_exec_raw_upiu_cmd(struct ufs_hba *hba,
 		if (err == -ETIMEDOUT)
 			break;
 
-		ocs_value = le32_to_cpu(treq.header.dword_2) & MASK_OCS;
+		ocs_value = treq.header.ocs & MASK_OCS;
 		if (ocs_value != OCS_SUCCESS) {
 			dev_err(hba->dev, "%s: failed, ocs = 0x%x\n", __func__,
 				ocs_value);
@@ -10567,6 +10563,39 @@  static const struct dev_pm_ops ufshcd_wl_pm_ops = {
 	SET_RUNTIME_PM_OPS(ufshcd_wl_runtime_suspend, ufshcd_wl_runtime_resume, NULL)
 };
 
+static void ufshcd_check_header_layout(void)
+{
+	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
+				.cci = 3})[0] != 3);
+
+	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
+				.ehs_length = 2})[1] != 2);
+
+	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
+				.enable_crypto = 1})[2]
+		     != 0x80);
+
+	BUILD_BUG_ON((((u8 *)&(struct request_desc_header){
+					.command_type = 5,
+					.data_direction = 3,
+					.interrupt = 1,
+				})[3]) != ((5 << 4) | (3 << 1) | 1));
+
+	BUILD_BUG_ON(((__le32 *)&(struct request_desc_header){
+				.dunl = cpu_to_le32(0xdeadbeef)})[1] !=
+		cpu_to_le32(0xdeadbeef));
+
+	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
+				.ocs = 4})[8] != 4);
+
+	BUILD_BUG_ON(((u8 *)&(struct request_desc_header){
+				.cds = 5})[9] != 5);
+
+	BUILD_BUG_ON(((__le32 *)&(struct request_desc_header){
+				.dunu = cpu_to_le32(0xbadcafe)})[3] !=
+		cpu_to_le32(0xbadcafe));
+}
+
 /*
  * ufs_dev_wlun_template - describes ufs device wlun
  * ufs-device wlun - used to send pm commands
@@ -10592,6 +10621,8 @@  static int __init ufshcd_core_init(void)
 {
 	int ret;
 
+	ufshcd_check_header_layout();
+
 	ufs_debugfs_init();
 
 	ret = scsi_register_driver(&ufs_dev_wlun_template.gendrv);
diff --git a/include/ufs/ufs.h b/include/ufs/ufs.h
index 0ee1fdf2fe83..80fae9484807 100644
--- a/include/ufs/ufs.h
+++ b/include/ufs/ufs.h
@@ -473,9 +473,6 @@  enum {
 	UPIU_COMMAND_SET_TYPE_QUERY	= 0x2,
 };
 
-/* UTP Transfer Request Command Offset */
-#define UPIU_COMMAND_TYPE_OFFSET	28
-
 /* Offset of the response code in the UPIU header */
 #define UPIU_RSP_CODE_OFFSET		8
 
diff --git a/include/ufs/ufshci.h b/include/ufs/ufshci.h
index c48135554d5c..d5accacae6bc 100644
--- a/include/ufs/ufshci.h
+++ b/include/ufs/ufshci.h
@@ -127,7 +127,6 @@  enum {
 };
 
 #define SQ_ICU_ERR_CODE_MASK		GENMASK(7, 4)
-#define UPIU_COMMAND_TYPE_MASK		GENMASK(31, 28)
 #define UFS_MASK(mask, offset)		((mask) << (offset))
 
 /* UFS Version 08h */
@@ -439,15 +438,13 @@  enum {
 	UTP_SCSI_COMMAND		= 0x00000000,
 	UTP_NATIVE_UFS_COMMAND		= 0x10000000,
 	UTP_DEVICE_MANAGEMENT_FUNCTION	= 0x20000000,
-	UTP_REQ_DESC_INT_CMD		= 0x01000000,
-	UTP_REQ_DESC_CRYPTO_ENABLE_CMD	= 0x00800000,
 };
 
 /* UTP Transfer Request Data Direction (DD) */
-enum {
-	UTP_NO_DATA_TRANSFER	= 0x00000000,
-	UTP_HOST_TO_DEVICE	= 0x02000000,
-	UTP_DEVICE_TO_HOST	= 0x04000000,
+enum utp_data_direction {
+	UTP_NO_DATA_TRANSFER	= 0,
+	UTP_HOST_TO_DEVICE	= 1,
+	UTP_DEVICE_TO_HOST	= 2,
 };
 
 /* Overall command status values */
@@ -506,17 +503,38 @@  struct utp_transfer_cmd_desc {
 
 /**
  * struct request_desc_header - Descriptor Header common to both UTRD and UTMRD
- * @dword0: Descriptor Header DW0
- * @dword1: Descriptor Header DW1
- * @dword2: Descriptor Header DW2
- * @dword3: Descriptor Header DW3
  */
 struct request_desc_header {
-	__le32 dword_0;
-	__le32 dword_1;
-	__le32 dword_2;
-	__le32 dword_3;
-};
+	u8 cci;
+	u8 ehs_length;
+#if defined(__BIG_ENDIAN)
+	u8 enable_crypto:1;
+	u8 reserved2:7;
+
+	u8 command_type:4;
+	u8 reserved1:1;
+	u8 data_direction:2;
+	u8 interrupt:1;
+#elif defined(__LITTLE_ENDIAN)
+	u8 reserved2:7;
+	u8 enable_crypto:1;
+
+	u8 interrupt:1;
+	u8 data_direction:2;
+	u8 reserved1:1;
+	u8 command_type:4;
+#else
+#error
+#endif
+
+	__le32 dunl;
+	u8 ocs;
+	u8 cds;
+	__le16 ldbc;
+	__le32 dunu;
+};
+
+static_assert(sizeof(struct request_desc_header) == 16);
 
 /**
  * struct utp_transfer_req_desc - UTP Transfer Request Descriptor (UTRD)