EFA updates 2020-01-14

https://github.com/jgunthorpe/linux/commit/453e85ed7aa46db22d8be16f9b0c88b17b8968af

---
 .../infiniband/hw/efa/efa_admin_cmds_defs.h   |   6 +
 drivers/infiniband/hw/efa/efa_admin_defs.h    |   5 +
 drivers/infiniband/hw/efa/efa_com.c           | 115 ++++++++----------
 drivers/infiniband/hw/efa/efa_com_cmd.c       |  27 ++--
 drivers/infiniband/hw/efa/efa_common_defs.h   |   6 +
 drivers/infiniband/hw/efa/efa_regs_defs.h     |   9 ++
 6 files changed, 90 insertions(+), 78 deletions(-)

drivers/infiniband/hw/efa/efa_admin_cmds_defs.h +6 -0

index e96bcb16bd2b..a589a471d122 100644
@@ -789,14 +789,17 @@ struct efa_admin_mmio_req_read_less_resp {
 };
 
 /* create_qp_cmd */
+#define EFA_ADMIN_CREATE_QP_CMD_SQ_VIRT_SHIFT               0
 #define EFA_ADMIN_CREATE_QP_CMD_SQ_VIRT_MASK                BIT(0)
 #define EFA_ADMIN_CREATE_QP_CMD_RQ_VIRT_SHIFT               1
 #define EFA_ADMIN_CREATE_QP_CMD_RQ_VIRT_MASK                BIT(1)
 
 /* reg_mr_cmd */
+#define EFA_ADMIN_REG_MR_CMD_PHYS_PAGE_SIZE_SHIFT_SHIFT     0
 #define EFA_ADMIN_REG_MR_CMD_PHYS_PAGE_SIZE_SHIFT_MASK      GENMASK(4, 0)
 #define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN_SHIFT     7
 #define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN_MASK      BIT(7)
+#define EFA_ADMIN_REG_MR_CMD_LOCAL_WRITE_ENABLE_SHIFT       0
 #define EFA_ADMIN_REG_MR_CMD_LOCAL_WRITE_ENABLE_MASK        BIT(0)
 #define EFA_ADMIN_REG_MR_CMD_REMOTE_READ_ENABLE_SHIFT       2
 #define EFA_ADMIN_REG_MR_CMD_REMOTE_READ_ENABLE_MASK        BIT(2)

@@ -806,12 +809,15 @@ struct efa_admin_mmio_req_read_less_resp {
 #define EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED_MASK BIT(5)
 #define EFA_ADMIN_CREATE_CQ_CMD_VIRT_SHIFT                  6
 #define EFA_ADMIN_CREATE_CQ_CMD_VIRT_MASK                   BIT(6)
+#define EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS_SHIFT   0
 #define EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS_MASK    GENMASK(4, 0)
 
 /* get_set_feature_common_desc */
+#define EFA_ADMIN_GET_SET_FEATURE_COMMON_DESC_SELECT_SHIFT  0
 #define EFA_ADMIN_GET_SET_FEATURE_COMMON_DESC_SELECT_MASK   GENMASK(1, 0)
 
 /* feature_device_attr_desc */
+#define EFA_ADMIN_FEATURE_DEVICE_ATTR_DESC_RDMA_READ_SHIFT  0
 #define EFA_ADMIN_FEATURE_DEVICE_ATTR_DESC_RDMA_READ_MASK   BIT(0)
 
 #endif /* _EFA_ADMIN_CMDS_H_ */

drivers/infiniband/hw/efa/efa_admin_defs.h +5 -0

index c8e0c8b905be..199d8eb3cbda 100644
@@ -119,7 +119,9 @@ struct efa_admin_aenq_entry {
 };
 
 /* aq_common_desc */
+#define EFA_ADMIN_AQ_COMMON_DESC_COMMAND_ID_SHIFT           0
 #define EFA_ADMIN_AQ_COMMON_DESC_COMMAND_ID_MASK            GENMASK(11, 0)
+#define EFA_ADMIN_AQ_COMMON_DESC_PHASE_SHIFT                0
 #define EFA_ADMIN_AQ_COMMON_DESC_PHASE_MASK                 BIT(0)
 #define EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_SHIFT            1
 #define EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_MASK             BIT(1)

@@ -127,10 +129,13 @@ struct efa_admin_aenq_entry {
 #define EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT_MASK    BIT(2)
 
 /* acq_common_desc */
+#define EFA_ADMIN_ACQ_COMMON_DESC_COMMAND_ID_SHIFT          0
 #define EFA_ADMIN_ACQ_COMMON_DESC_COMMAND_ID_MASK           GENMASK(11, 0)
+#define EFA_ADMIN_ACQ_COMMON_DESC_PHASE_SHIFT               0
 #define EFA_ADMIN_ACQ_COMMON_DESC_PHASE_MASK                BIT(0)
 
 /* aenq_common_desc */
+#define EFA_ADMIN_AENQ_COMMON_DESC_PHASE_SHIFT              0
 #define EFA_ADMIN_AENQ_COMMON_DESC_PHASE_MASK               BIT(0)
 
 #endif /* _EFA_ADMIN_H_ */

drivers/infiniband/hw/efa/efa_com.c +50 ~~-65~~

262 lines not on this page · raw message

drivers/infiniband/hw/efa/efa_com_cmd.c +14 ~~-13~~

index e20bd84a1014..80670517d950 100644
@@ -161,8 +161,9 @@ int efa_com_create_cq(struct efa_com_dev *edev,
 	int err;
 
 	create_cmd.aq_common_desc.opcode = EFA_ADMIN_CREATE_CQ;
-	create_cmd.cq_caps_2 = (params->entry_size_in_bytes / 4) &
-				EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS_MASK;
+	EFA_SET(&create_cmd.cq_caps_2,
+		EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS,
+		params->entry_size_in_bytes / 4);
 	create_cmd.cq_depth = params->cq_depth;
 	create_cmd.num_sub_cqs = params->num_sub_cqs;
 	create_cmd.uar = params->uarn;

@@ -227,8 +228,8 @@ int efa_com_register_mr(struct efa_com_dev *edev,
 	mr_cmd.aq_common_desc.opcode = EFA_ADMIN_REG_MR;
 	mr_cmd.pd = params->pd;
 	mr_cmd.mr_length = params->mr_length_in_bytes;
-	mr_cmd.flags |= params->page_shift &
-		EFA_ADMIN_REG_MR_CMD_PHYS_PAGE_SIZE_SHIFT_MASK;
+	EFA_SET(&mr_cmd.flags, EFA_ADMIN_REG_MR_CMD_PHYS_PAGE_SIZE_SHIFT,
+		params->page_shift);
 	mr_cmd.iova = params->iova;
 	mr_cmd.permissions = params->permissions;

@@ -242,11 +243,11 @@ int efa_com_register_mr(struct efa_com_dev *edev,
 			params->pbl.pbl.address.mem_addr_low;
 		mr_cmd.pbl.pbl.address.mem_addr_high =
 			params->pbl.pbl.address.mem_addr_high;
-		mr_cmd.aq_common_desc.flags |=
-			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_MASK;
+		EFA_SET(&mr_cmd.aq_common_desc.flags,
+			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA, 1);
 		if (params->indirect)
-			mr_cmd.aq_common_desc.flags |=
-				EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT_MASK;
+			EFA_SET(&mr_cmd.aq_common_desc.flags,
+				EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT, 1);
 	}
 
 	err = efa_com_cmd_exec(aq,

@@ -386,9 +387,8 @@ static int efa_com_get_feature_ex(struct efa_com_dev *edev,
 	get_cmd.aq_common_descriptor.opcode = EFA_ADMIN_GET_FEATURE;
 
 	if (control_buff_size)
-		get_cmd.aq_common_descriptor.flags =
-			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT_MASK;
-
+		EFA_SET(&get_cmd.aq_common_descriptor.flags,
+			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT, 1);
 
 	efa_com_set_dma_addr(control_buf_dma_addr,
 			     &get_cmd.control_buffer.address.mem_addr_high,

@@ -538,8 +538,9 @@ static int efa_com_set_feature_ex(struct efa_com_dev *edev,
 
 	set_cmd->aq_common_descriptor.opcode = EFA_ADMIN_SET_FEATURE;
 	if (control_buff_size) {
-		set_cmd->aq_common_descriptor.flags =
-			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT_MASK;
+		set_cmd->aq_common_descriptor.flags = 0;
+		EFA_SET(&set_cmd->aq_common_descriptor.flags,
+			EFA_ADMIN_AQ_COMMON_DESC_CTRL_DATA_INDIRECT, 1);
 		efa_com_set_dma_addr(control_buf_dma_addr,
 				     &set_cmd->control_buffer.address.mem_addr_high,
 				     &set_cmd->control_buffer.address.mem_addr_low);

drivers/infiniband/hw/efa/efa_common_defs.h +6 -0

index c559ec08898e..845ea5ca9388 100644
@@ -9,6 +9,12 @@
 #define EFA_COMMON_SPEC_VERSION_MAJOR        2
 #define EFA_COMMON_SPEC_VERSION_MINOR        0
 
+#define EFA_GET(ptr, type) \
+	((*(ptr) & type##_MASK) >> type##_SHIFT)
+
+#define EFA_SET(ptr, type, value) \
+	({ *(ptr) |= ((value) << type##_SHIFT) & type##_MASK; })
+
 struct efa_common_mem_addr {
 	u32 mem_addr_low;

drivers/infiniband/hw/efa/efa_regs_defs.h +9 -0

index bb9cad3d6a15..b3119ed8d9b2 100644
@@ -44,11 +44,13 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_MMIO_RESP_HI_OFF                           0x64
 
 /* version register */
+#define EFA_REGS_VERSION_MINOR_VERSION_SHIFT                0
 #define EFA_REGS_VERSION_MINOR_VERSION_MASK                 0xff
 #define EFA_REGS_VERSION_MAJOR_VERSION_SHIFT                8
 #define EFA_REGS_VERSION_MAJOR_VERSION_MASK                 0xff00
 
 /* controller_version register */
+#define EFA_REGS_CONTROLLER_VERSION_SUBMINOR_VERSION_SHIFT  0
 #define EFA_REGS_CONTROLLER_VERSION_SUBMINOR_VERSION_MASK   0xff
 #define EFA_REGS_CONTROLLER_VERSION_MINOR_VERSION_SHIFT     8
 #define EFA_REGS_CONTROLLER_VERSION_MINOR_VERSION_MASK      0xff00

@@ -58,6 +60,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_CONTROLLER_VERSION_IMPL_ID_MASK            0xff000000
 
 /* caps register */
+#define EFA_REGS_CAPS_CONTIGUOUS_QUEUE_REQUIRED_SHIFT       0
 #define EFA_REGS_CAPS_CONTIGUOUS_QUEUE_REQUIRED_MASK        0x1
 #define EFA_REGS_CAPS_RESET_TIMEOUT_SHIFT                   1
 #define EFA_REGS_CAPS_RESET_TIMEOUT_MASK                    0x3e

@@ -67,11 +70,13 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_CAPS_ADMIN_CMD_TO_MASK                     0xf0000
 
 /* aq_caps register */
+#define EFA_REGS_AQ_CAPS_AQ_DEPTH_SHIFT                     0
 #define EFA_REGS_AQ_CAPS_AQ_DEPTH_MASK                      0xffff
 #define EFA_REGS_AQ_CAPS_AQ_ENTRY_SIZE_SHIFT                16
 #define EFA_REGS_AQ_CAPS_AQ_ENTRY_SIZE_MASK                 0xffff0000
 
 /* acq_caps register */
+#define EFA_REGS_ACQ_CAPS_ACQ_DEPTH_SHIFT                   0
 #define EFA_REGS_ACQ_CAPS_ACQ_DEPTH_MASK                    0xffff
 #define EFA_REGS_ACQ_CAPS_ACQ_ENTRY_SIZE_SHIFT              16
 #define EFA_REGS_ACQ_CAPS_ACQ_ENTRY_SIZE_MASK               0xff0000

@@ -79,6 +84,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_ACQ_CAPS_ACQ_MSIX_VECTOR_MASK              0xff000000
 
 /* aenq_caps register */
+#define EFA_REGS_AENQ_CAPS_AENQ_DEPTH_SHIFT                 0
 #define EFA_REGS_AENQ_CAPS_AENQ_DEPTH_MASK                  0xffff
 #define EFA_REGS_AENQ_CAPS_AENQ_ENTRY_SIZE_SHIFT            16
 #define EFA_REGS_AENQ_CAPS_AENQ_ENTRY_SIZE_MASK             0xff0000

@@ -86,6 +92,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_AENQ_CAPS_AENQ_MSIX_VECTOR_MASK            0xff000000
 
 /* dev_ctl register */
+#define EFA_REGS_DEV_CTL_DEV_RESET_SHIFT                    0
 #define EFA_REGS_DEV_CTL_DEV_RESET_MASK                     0x1
 #define EFA_REGS_DEV_CTL_AQ_RESTART_SHIFT                   1
 #define EFA_REGS_DEV_CTL_AQ_RESTART_MASK                    0x2

@@ -93,6 +100,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_DEV_CTL_RESET_REASON_MASK                  0xf0000000
 
 /* dev_sts register */
+#define EFA_REGS_DEV_STS_READY_SHIFT                        0
 #define EFA_REGS_DEV_STS_READY_MASK                         0x1
 #define EFA_REGS_DEV_STS_AQ_RESTART_IN_PROGRESS_SHIFT       1
 #define EFA_REGS_DEV_STS_AQ_RESTART_IN_PROGRESS_MASK        0x2

@@ -106,6 +114,7 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_DEV_STS_FATAL_ERROR_MASK                   0x20
 
 /* mmio_reg_read register */
+#define EFA_REGS_MMIO_REG_READ_REQ_ID_SHIFT                 0
 #define EFA_REGS_MMIO_REG_READ_REQ_ID_MASK                  0xffff
 #define EFA_REGS_MMIO_REG_READ_REG_OFF_SHIFT                16
 #define EFA_REGS_MMIO_REG_READ_REG_OFF_MASK                 0xffff0000

Signature · 1 line

2.24.1

Jason Gunthorpe

2020-01-15 19:31 UTC #2 lore raw

Re: [PATCH for-next 1/6] RDMA/efa: Unified getters/setters for device structs bitmask access

drivers/infiniband/hw/efa/efa_common_defs.h line 17

Quoted 13 lines · Gal Pressman

drivers/infiniband/hw/efa/efa_common_defs.h +7 -0

index c559ec08898e..845ea5ca9388 100644
+++ b/drivers/infiniband/hw/efa/efa_common_defs.h

@@ -9,6 +9,12 @@
 #define EFA_COMMON_SPEC_VERSION_MAJOR        2
 #define EFA_COMMON_SPEC_VERSION_MINOR        0
 
+#define EFA_GET(ptr, type) \
+	((*(ptr) & type##_MASK) >> type##_SHIFT)
+
+#define EFA_SET(ptr, type, value) \
+	({ *(ptr) |= ((value) << type##_SHIFT) & type##_MASK; })
+

Why not just GENMASK properly? You don't need MASK and SHIFT, it is supposed to be written like:

  #define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN GENMASK(8,7)

*ptr |= FIELD_PREP(val, EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN)

FIELD_PREP automatically deduces the correct shift.

And it would be much nicer if this had some type safety.

You should review the stuff Leon is prepping here:

Which is basically doing the same sorts of things, but with better type safety and no need for the various structs

Jason

Gal Pressman

author

2020-01-16 07:05 UTC #3 lore raw

Re: [PATCH for-next 1/6] RDMA/efa: Unified getters/setters for device structs bitmask access

Quoted 24 lines · Jason Gunthorpe

drivers/infiniband/hw/efa/efa_common_defs.h line 17
Quoted 13 lines · Gal Pressman
drivers/infiniband/hw/efa/efa_common_defs.h +7 -0
index c559ec08898e..845ea5ca9388 100644
+++ b/drivers/infiniband/hw/efa/efa_common_defs.h
@@ -9,6 +9,12 @@
 #define EFA_COMMON_SPEC_VERSION_MAJOR        2
 #define EFA_COMMON_SPEC_VERSION_MINOR        0
 
+#define EFA_GET(ptr, type) \
+	((*(ptr) & type##_MASK) >> type##_SHIFT)
+
+#define EFA_SET(ptr, type, value) \
+	({ *(ptr) |= ((value) << type##_SHIFT) & type##_MASK; })
+
Why not just GENMASK properly? You don't need MASK and SHIFT, it is supposed to be written like:
  #define EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN GENMASK(8,7)
*ptr |= FIELD_PREP(val, EFA_ADMIN_REG_MR_CMD_MEM_ADDR_PHY_MODE_EN)
FIELD_PREP automatically deduces the correct shift.

Quoted 8 lines

And it would be much nicer if this had some type safety.
You should review the stuff Leon is prepping here:
https://github.com/jgunthorpe/linux/commit/453e85ed7aa46db22d8be16f9b0c88b17b8968af
Which is basically doing the same sorts of things, but with better type safety and no need for the various structs

I'll take a look, thanks.

Gal Pressman

author

2020-01-14 08:57 UTC #4 lore raw

PATCH 2/6 RDMA/efa: Properly document the interrupt mask register

The fact that the LSB in the register is the enable bit should not be an implicit assumption between the driver and the device, properly document that in the register definition.

---
 drivers/infiniband/hw/efa/efa_com.c       | 4 +---
 drivers/infiniband/hw/efa/efa_regs_defs.h | 4 ++++
 2 files changed, 5 insertions(+), 3 deletions(-)

drivers/infiniband/hw/efa/efa_com.c +1 -3

index 5dd5e429c15c..8622b34705c5 100644
@@ -34,8 +34,6 @@
 #define EFA_DMA_ADDR_TO_UINT32_LOW(x)   ((u32)((u64)(x)))
 #define EFA_DMA_ADDR_TO_UINT32_HIGH(x)  ((u32)(((u64)(x)) >> 32))
 
-#define EFA_REGS_ADMIN_INTR_MASK 1
-
 enum efa_cmd_status {
 	EFA_CMD_SUBMITTED,
 	EFA_CMD_COMPLETED,

@@ -700,7 +698,7 @@ void efa_com_set_admin_polling_mode(struct efa_com_dev *edev, bool polling)
 	u32 mask_value = 0;
 
 	if (polling)
-		mask_value = EFA_REGS_ADMIN_INTR_MASK;
+		EFA_SET(&mask_value, EFA_REGS_INTR_MASK_EN, 1);
 
 	writel(mask_value, edev->reg_bar + EFA_REGS_INTR_MASK_OFF);
 	if (polling)

drivers/infiniband/hw/efa/efa_regs_defs.h +4 -0

index b3119ed8d9b2..48a72c507224 100644
@@ -91,6 +91,10 @@ enum efa_regs_reset_reason_types {
 #define EFA_REGS_AENQ_CAPS_AENQ_MSIX_VECTOR_SHIFT           24
 #define EFA_REGS_AENQ_CAPS_AENQ_MSIX_VECTOR_MASK            0xff000000
 
+/* intr_mask register */
+#define EFA_REGS_INTR_MASK_EN_SHIFT                         0
+#define EFA_REGS_INTR_MASK_EN_MASK                          0x1
+
 /* dev_ctl register */
 #define EFA_REGS_DEV_CTL_DEV_RESET_SHIFT                    0
 #define EFA_REGS_DEV_CTL_DEV_RESET_MASK                     0x1

Signature · 1 line

2.24.1

Gal Pressman

author

2020-01-14 08:57 UTC #5 lore raw

PATCH 3/6 RDMA/efa: Device definitions documentation updates

Various clarifications and updates to the documentation of the device definitions.
No functional changes in this patch.

---
 .../infiniband/hw/efa/efa_admin_cmds_defs.h   | 37 ++++++++++++-------
 1 file changed, 24 insertions(+), 13 deletions(-)

drivers/infiniband/hw/efa/efa_admin_cmds_defs.h +24 ~~-13~~

index a589a471d122..d5f3d8795ac0 100644
@@ -160,10 +160,16 @@ struct efa_admin_create_qp_resp {
 	/* Common Admin Queue completion descriptor */
 	struct efa_admin_acq_common_desc acq_common_desc;
 
-	/* Opaque handle to be used for consequent operations on the QP */
+	/*
+	 * Opaque handle to be used for consequent admin operations on the
+	 * QP
+	 */
 	u32 qp_handle;
 
-	/* QP number in the given EFA virtual device */
+	/*
+	 * QP number in the given EFA virtual device. Least-significant bits
+	 *    (as needed according to max_qp) carry unique QP ID
+	 */
 	u16 qp_num;
 
 	/* MBZ */

@@ -286,6 +292,7 @@ struct efa_admin_create_ah_cmd {
 	/* PD number */
 	u16 pd;
 
+	/* MBZ */
 	u16 reserved;
 };

@@ -296,6 +303,7 @@ struct efa_admin_create_ah_resp {
 	/* Target interface address handle (opaque) */
 	u16 ah;
 
+	/* MBZ */
 	u16 reserved;
 };

@@ -372,6 +380,7 @@ struct efa_admin_reg_mr_cmd {
 	 */
 	u8 permissions;
 
+	/* MBZ */
 	u16 reserved16_w5;
 
 	/* number of pages in PBL (redundant, could be calculated) */

@@ -419,20 +428,20 @@ struct efa_admin_create_cq_cmd {
 	struct efa_admin_aq_common_desc aq_common_desc;
 
 	/*
-	 * 4:0 : reserved5
+	 * 4:0 : reserved5 - MBZ
 	 * 5 : interrupt_mode_enabled - if set, cq operates
 	 *    in interrupt mode (i.e. CQ events and MSI-X are
 	 *    generated), otherwise - polling
 	 * 6 : virt - If set, ring base address is virtual
 	 *    (IOVA returned by MR registration)
-	 * 7 : reserved6
+	 * 7 : reserved6 - MBZ
 	 */
 	u8 cq_caps_1;
 
 	/*
 	 * 4:0 : cq_entry_size_words - size of CQ entry in
 	 *    32-bit words, valid values: 4, 8.
-	 * 7:5 : reserved7
+	 * 7:5 : reserved7 - MBZ
 	 */
 	u8 cq_caps_2;

@@ -478,6 +487,7 @@ struct efa_admin_destroy_cq_cmd {
 
 	u16 cq_idx;
 
+	/* MBZ */
 	u16 reserved1;
 };

@@ -530,7 +540,7 @@ struct efa_admin_get_set_feature_common_desc {
 	/*
 	 * 1:0 : select - 0x1 - current value; 0x3 - default
 	 *    value
-	 * 7:3 : reserved3
+	 * 7:3 : reserved3 - MBZ
 	 */
 	u8 flags;

@@ -557,10 +567,10 @@ struct efa_admin_feature_device_attr_desc {
 	/* Bar used for SQ and RQ doorbells */
 	u16 db_bar;
 
-	/* Indicates how many bits are used physical address access */
+	/* Indicates how many bits are used on physical address access */
 	u8 phys_addr_width;
 
-	/* Indicates how many bits are used virtual address access */
+	/* Indicates how many bits are used on virtual address access */
 	u8 virt_addr_width;
 
 	/*

@@ -578,27 +588,28 @@ struct efa_admin_feature_queue_attr_desc {
 	/* The maximum number of queue pairs supported */
 	u32 max_qp;
 
+	/* Maximum number of WQEs per Send Queue */
 	u32 max_sq_depth;
 
-	/* max send wr used in inline-buf */
+	/* Maximum size of data that can be sent inline in a Send WQE */
 	u32 inline_buf_size;
 
+	/* Maximum number of buffer descriptors per Recv Queue */
 	u32 max_rq_depth;
 
 	/* The maximum number of completion queues supported per VF */
 	u32 max_cq;
 
+	/* Maximum number of CQEs per Completion Queue */
 	u32 max_cq_depth;
 
 	/* Number of sub-CQs to be created for each CQ */
 	u16 sub_cqs_per_cq;
 
+	/* MBZ */
 	u16 reserved;
 
-	/*
-	 * Maximum number of SGEs (buffs) allowed for a single send work
-	 *    queue element (WQE)
-	 */
+	/* Maximum number of SGEs (buffers) allowed for a single send WQE */
 	u16 max_wr_send_sges;
 
 	/* Maximum number of SGEs allowed for a single recv WQE */

Signature · 1 line

2.24.1

Gal Pressman

author

2020-01-14 08:57 UTC #6 lore raw

PATCH 4/6 RDMA/efa: Remove {} brackets from single statement if

The {} brackets are not needed according to the Linux coding style.

Reviewed-by Daniel Kranzdorf <[email protected]>

---
 drivers/infiniband/hw/efa/efa_verbs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/infiniband/hw/efa/efa_verbs.c +1 -2

index 50c22575aed6..ce89c0b9c315 100644
@@ -1608,13 +1608,12 @@ static int __efa_mmap(struct efa_dev *dev, struct efa_ucontext *ucontext,
 		err = -EINVAL;
 	}
 
-	if (err) {
+	if (err)
 		ibdev_dbg(
 			&dev->ibdev,
 			"Couldn't mmap address[%#llx] length[%#zx] mmap_flag[%d] err[%d]\n",
 			entry->address, rdma_entry->npages * PAGE_SIZE,
 			entry->mmap_flag, err);
-	}
 
 	rdma_user_mmap_entry_put(rdma_entry);
 	return err;

Signature · 1 line

2.24.1

Gal Pressman

author

2020-01-14 08:57 UTC #7 lore raw

PATCH 5/6 RDMA/efa: Remove unused ucontext parameter from efa_qp_user_mmap_entries_remove

The ucontext parameter is unused, remove it.

---
 drivers/infiniband/hw/efa/efa_verbs.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

drivers/infiniband/hw/efa/efa_verbs.c +4 -7

index ce89c0b9c315..9a6cff718c49 100644
@@ -387,8 +387,7 @@ static int efa_destroy_qp_handle(struct efa_dev *dev, u32 qp_handle)
 	return efa_com_destroy_qp(&dev->edev, &params);
 }
 
-static void efa_qp_user_mmap_entries_remove(struct efa_ucontext *uctx,
-					    struct efa_qp *qp)
+static void efa_qp_user_mmap_entries_remove(struct efa_qp *qp)
 {
 	rdma_user_mmap_entry_remove(qp->rq_mmap_entry);
 	rdma_user_mmap_entry_remove(qp->rq_db_mmap_entry);

@@ -398,8 +397,6 @@ static void efa_qp_user_mmap_entries_remove(struct efa_ucontext *uctx,
 
 int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
 {
-	struct efa_ucontext *ucontext = rdma_udata_to_drv_context(udata,
-		struct efa_ucontext, ibucontext);
 	struct efa_dev *dev = to_edev(ibqp->pd->device);
 	struct efa_qp *qp = to_eqp(ibqp);
 	int err;

@@ -418,7 +415,7 @@ int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
 				 DMA_TO_DEVICE);
 	}
 
-	efa_qp_user_mmap_entries_remove(ucontext, qp);
+	efa_qp_user_mmap_entries_remove(qp);
 	kfree(qp);
 	return 0;
 }

@@ -510,7 +507,7 @@ static int qp_mmap_entries_setup(struct efa_qp *qp,
 	return 0;
 
 err_remove_mmap:
-	efa_qp_user_mmap_entries_remove(ucontext, qp);
+	efa_qp_user_mmap_entries_remove(qp);
 
 	return -ENOMEM;
 }

@@ -719,7 +716,7 @@ struct ib_qp *efa_create_qp(struct ib_pd *ibpd,
 	return &qp->ibqp;
 
 err_remove_mmap_entries:
-	efa_qp_user_mmap_entries_remove(ucontext, qp);
+	efa_qp_user_mmap_entries_remove(qp);
 err_destroy_qp:
 	efa_destroy_qp_handle(dev, create_qp_resp.qp_handle);
 err_free_mapped:

Signature · 1 line

2.24.1

Gal Pressman

author

2020-01-14 08:57 UTC #8 lore raw

PATCH 6/6 RDMA/efa: Do not delay freeing of DMA pages

When destroying a DMA mmapped object, there is no need to delay the pages freeing to dealloc_ucontext as the kernel itself will keep reference count for these pages.

Remove the special handling of DMA pages and call free_pages_exact on destroy_qp/cq.

---
 drivers/infiniband/hw/efa/efa_verbs.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

drivers/infiniband/hw/efa/efa_verbs.c +4 -9

index 9a6cff718c49..fd2fc60d569f 100644
@@ -413,6 +413,7 @@ int efa_destroy_qp(struct ib_qp *ibqp, struct ib_udata *udata)
 			  &qp->rq_dma_addr);
 		dma_unmap_single(&dev->pdev->dev, qp->rq_dma_addr, qp->rq_size,
 				 DMA_TO_DEVICE);
+		free_pages_exact(qp->rq_cpu_addr, qp->rq_size);
 	}
 
 	efa_qp_user_mmap_entries_remove(qp);

@@ -723,9 +724,7 @@ struct ib_qp *efa_create_qp(struct ib_pd *ibpd,
 	if (qp->rq_size) {
 		dma_unmap_single(&dev->pdev->dev, qp->rq_dma_addr, qp->rq_size,
 				 DMA_TO_DEVICE);
-
-		if (!qp->rq_mmap_entry)
-			free_pages_exact(qp->rq_cpu_addr, qp->rq_size);
+		free_pages_exact(qp->rq_cpu_addr, qp->rq_size);
 	}
 err_free_qp:
 	kfree(qp);

@@ -848,6 +847,7 @@ void efa_destroy_cq(struct ib_cq *ibcq, struct ib_udata *udata)
 	efa_destroy_cq_idx(dev, cq->cq_idx);
 	dma_unmap_single(&dev->pdev->dev, cq->dma_addr, cq->size,
 			 DMA_FROM_DEVICE);
+	free_pages_exact(cq->cpu_addr, cq->size);
 	rdma_user_mmap_entry_remove(cq->mmap_entry);
 }

@@ -987,8 +987,7 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 err_free_mapped:
 	dma_unmap_single(&dev->pdev->dev, cq->dma_addr, cq->size,
 			 DMA_FROM_DEVICE);
-	if (!cq->mmap_entry)
-		free_pages_exact(cq->cpu_addr, cq->size);
+	free_pages_exact(cq->cpu_addr, cq->size);
 
 err_out:
 	atomic64_inc(&dev->stats.sw_stats.create_cq_err);

@@ -1549,10 +1548,6 @@ void efa_mmap_free(struct rdma_user_mmap_entry *rdma_entry)
 {
 	struct efa_user_mmap_entry *entry = to_emmap(rdma_entry);
 
-	/* DMA mapping is already gone, now free the pages */
-	if (entry->mmap_flag == EFA_MMAP_DMA_PAGE)
-		free_pages_exact(phys_to_virt(entry->address),
-				 entry->rdma_entry.npages * PAGE_SIZE);
 	kfree(entry);
 }

Signature · 1 line

2.24.1

Jason Gunthorpe

2020-01-15 19:51 UTC #9 lore raw

Re: [PATCH for-next 6/6] RDMA/efa: Do not delay freeing of DMA pages

Quoted 3 lines · Gal Pressman

When destroying a DMA mmapped object, there is no need to delay the pages freeing to dealloc_ucontext as the kernel itself will keep reference count for these pages.

Why does the commit message talk about dealloc_ucontext but doesn't change dealloc_ucontext?

Quoted 2 lines

(quoted diff) +1 -0

+	free_pages_exact(cq->cpu_addr, cq->size);

	rdma_user_mmap_entry_remove(cq->mmap_entry);

This is out of order, the pages can't be freed until the entry is removed.

There is also a bug in rdma_user_mmap_entry_remove(), entry->driver_removed needs to be set while holding the xa_lock or this is not the required fence.

Jason

Gal Pressman

author

2020-01-16 08:26 UTC #10 lore raw

Re: [PATCH for-next 6/6] RDMA/efa: Do not delay freeing of DMA pages

Quoted 7 lines · Jason Gunthorpe

Quoted 3 lines · Gal Pressman
When destroying a DMA mmapped object, there is no need to delay the pages freeing to dealloc_ucontext as the kernel itself will keep reference count for these pages.
Why does the commit message talk about dealloc_ucontext but doesn't change dealloc_ucontext?

The commit message is wrong :\, we should not delay the free_pages_exact to the mmap_free callback. We can "put" them on destroy flow and the pages will be freed by the last consumer (either unmap or destroy).

Quoted 6 lines

Quoted 3 lines
(quoted diff) +1 -0
+	free_pages_exact(cq->cpu_addr, cq->size);
	rdma_user_mmap_entry_remove(cq->mmap_entry);
This is out of order, the pages can't be freed until the entry is removed.

Right, I think the order is correct except rdma_user_mmap_entry_remove should be moved to the beginning of the function.

Quoted 4 lines

There is also a bug in rdma_user_mmap_entry_remove(), entry->driver_removed needs to be set while holding the xa_lock or this is not the required fence.

I see you sent a patch, I'll take a look.
Thanks for the review.

Jason Gunthorpe

2020-01-15 19:58 UTC #11 lore raw