[PATCH v2 for-next 0/5] RDMA/hns: Fix and refactor CMDQ related code

[PATCH v2 for-next 0/5] RDMA/hns: Fix and refactor CMDQ related code

[Weihang Li]

Remove some dead code in process of CMDQ transmission, and fix an issue
about missing error code.

Changes since v1:
* Drop #2 from the v1 series because the compatibility with the firmware
  needs to be considered.
* Link: https://patchwork.kernel.org/project/linux-rdma/cover/1612419786-39173-1-git-send-email-liweihang@huawei.com/

Lang Cheng (5):
  RDMA/hns: Remove unused member and variable of CMDQ
  RDMA/hns: Fixes missing error code of CMDQ
  RDMA/hns: Remove redundant operations on CMDQ
  RDMA/hns: Adjust fields and variables about CMDQ tail/head
  RDMA/hns: Refactor process of posting CMDQ

 drivers/infiniband/hw/hns/hns_roce_common.h |   4 +-
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 125 ++++++++--------------------
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h  |   5 --
 3 files changed, 37 insertions(+), 97 deletions(-)

-- 
2.8.1

[PATCH v2 for-next 1/5] RDMA/hns: Remove unused member and variable of CMDQ

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

last_status of structure hns_roce_v2_cmq has never been used, and the
variable named 'complete' in __hns_roce_cmq_send() is meaningless.

Fixes: a04ff739f2a9 ("RDMA/hns: Add command queue support for hip08 RoCE driver")
Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 9 +++------
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h | 1 -
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index a5bbfb1..7a5a41d 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1243,7 +1243,6 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	struct hns_roce_v2_priv *priv = hr_dev->priv;
 	struct hns_roce_v2_cmq_ring *csq = &priv->cmq.csq;
 	struct hns_roce_cmq_desc *desc_to_use;
-	bool complete = false;
 	u32 timeout = 0;
 	int handle = 0;
 	u16 desc_ret;
@@ -1290,7 +1289,6 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	}
 
 	if (hns_roce_cmq_csq_done(hr_dev)) {
-		complete = true;
 		handle = 0;
 		while (handle < num) {
 			/* get the result of hardware write back */
@@ -1302,16 +1300,15 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 				ret = 0;
 			else
 				ret = -EIO;
-			priv->cmq.last_status = desc_ret;
+
 			ntc++;
 			handle++;
 			if (ntc == csq->desc_num)
 				ntc = 0;
 		}
-	}
-
-	if (!complete)
+	} else {
 		ret = -EAGAIN;
+	}
 
 	/* clean the command send queue */
 	handle = hns_roce_cmq_csq_clean(hr_dev);
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index 69bc072..9f97e32 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -1839,7 +1839,6 @@ struct hns_roce_v2_cmq {
 	struct hns_roce_v2_cmq_ring csq;
 	struct hns_roce_v2_cmq_ring crq;
 	u16 tx_timeout;
-	u16 last_status;
 };
 
 enum hns_roce_link_table_type {
-- 
2.8.1

[PATCH v2 for-next 2/5] RDMA/hns: Fixes missing error code of CMDQ

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

When posting a multi-descriptors command, the error code of previous failed
descriptors may be rewrote to 0 by a later successful descriptor.

Fixes: a04ff739f2a9 ("RDMA/hns: Add command queue support for hip08 RoCE driver")
Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 7a5a41d..79cb2d6 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1246,7 +1246,7 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	u32 timeout = 0;
 	int handle = 0;
 	u16 desc_ret;
-	int ret = 0;
+	int ret;
 	int ntc;
 
 	spin_lock_bh(&csq->lock);
@@ -1290,15 +1290,14 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 
 	if (hns_roce_cmq_csq_done(hr_dev)) {
 		handle = 0;
+		ret = 0;
 		while (handle < num) {
 			/* get the result of hardware write back */
 			desc_to_use = &csq->desc[ntc];
 			desc[handle] = *desc_to_use;
 			dev_dbg(hr_dev->dev, "Get cmq desc:\n");
 			desc_ret = le16_to_cpu(desc[handle].retval);
-			if (desc_ret == CMD_EXEC_SUCCESS)
-				ret = 0;
-			else
+			if (unlikely(desc_ret != CMD_EXEC_SUCCESS))
 				ret = -EIO;
 
 			ntc++;
-- 
2.8.1

[PATCH v2 for-next 3/5] RDMA/hns: Remove redundant operations on CMDQ

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

CMDQ works serially, after each successful transmission, the head and tail
pointers will be equal, so there is no need to check whether the queue is
full. At the same time, since the descriptor of each transmission is new,
there is no need to perform a cleanup operation. Then, the field named
next_to_clean in structure hns_roce_v2_cmq_ring is redundant.

Fixes: a04ff739f2a9 ("RDMA/hns: Add command queue support for hip08 RoCE driver")
Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 54 +++---------------------------
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h |  1 -
 2 files changed, 5 insertions(+), 50 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 79cb2d6..1945730 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1059,15 +1059,6 @@ static int hns_roce_v2_rst_process_cmd(struct hns_roce_dev *hr_dev)
 	return 0;
 }
 
-static int hns_roce_cmq_space(struct hns_roce_v2_cmq_ring *ring)
-{
-	int ntu = ring->next_to_use;
-	int ntc = ring->next_to_clean;
-	int used = (ntu - ntc + ring->desc_num) % ring->desc_num;
-
-	return ring->desc_num - used - 1;
-}
-
 static int hns_roce_alloc_cmq_desc(struct hns_roce_dev *hr_dev,
 				   struct hns_roce_v2_cmq_ring *ring)
 {
@@ -1107,7 +1098,6 @@ static int hns_roce_init_cmq_ring(struct hns_roce_dev *hr_dev, bool ring_type)
 					    &priv->cmq.csq : &priv->cmq.crq;
 
 	ring->flag = ring_type;
-	ring->next_to_clean = 0;
 	ring->next_to_use = 0;
 
 	return hns_roce_alloc_cmq_desc(hr_dev, ring);
@@ -1213,30 +1203,6 @@ static int hns_roce_cmq_csq_done(struct hns_roce_dev *hr_dev)
 	return head == priv->cmq.csq.next_to_use;
 }
 
-static int hns_roce_cmq_csq_clean(struct hns_roce_dev *hr_dev)
-{
-	struct hns_roce_v2_priv *priv = hr_dev->priv;
-	struct hns_roce_v2_cmq_ring *csq = &priv->cmq.csq;
-	struct hns_roce_cmq_desc *desc;
-	u16 ntc = csq->next_to_clean;
-	u32 head;
-	int clean = 0;
-
-	desc = &csq->desc[ntc];
-	head = roce_read(hr_dev, ROCEE_TX_CMQ_HEAD_REG);
-	while (head != ntc) {
-		memset(desc, 0, sizeof(*desc));
-		ntc++;
-		if (ntc == csq->desc_num)
-			ntc = 0;
-		desc = &csq->desc[ntc];
-		clean++;
-	}
-	csq->next_to_clean = ntc;
-
-	return clean;
-}
-
 static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 			       struct hns_roce_cmq_desc *desc, int num)
 {
@@ -1251,15 +1217,6 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 
 	spin_lock_bh(&csq->lock);
 
-	if (num > hns_roce_cmq_space(csq)) {
-		spin_unlock_bh(&csq->lock);
-		return -EBUSY;
-	}
-
-	/*
-	 * Record the location of desc in the cmq for this time
-	 * which will be use for hardware to write back
-	 */
 	ntc = csq->next_to_use;
 
 	while (handle < num) {
@@ -1306,15 +1263,14 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 				ntc = 0;
 		}
 	} else {
+		/* FW/HW reset or incorrect number of desc */
+		ntc = roce_read(hr_dev, ROCEE_TX_CMQ_HEAD_REG);
+		dev_warn(hr_dev->dev, "CMDQ move head from %d to %d\n",
+			 csq->next_to_use, ntc);
+		csq->next_to_use = ntc;
 		ret = -EAGAIN;
 	}
 
-	/* clean the command send queue */
-	handle = hns_roce_cmq_csq_clean(hr_dev);
-	if (handle != num)
-		dev_warn(hr_dev->dev, "Cleaned %d, need to clean %d\n",
-			 handle, num);
-
 	spin_unlock_bh(&csq->lock);
 
 	return ret;
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index 9f97e32..7714fa4 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -1830,7 +1830,6 @@ struct hns_roce_v2_cmq_ring {
 	u16 buf_size;
 	u16 desc_num;
 	int next_to_use;
-	int next_to_clean;
 	u8 flag;
 	spinlock_t lock; /* command queue lock */
 };
-- 
2.8.1

[PATCH v2 for-next 4/5] RDMA/hns: Adjust fields and variables about CMDQ tail/head

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

The register 0x07014 is actually the head pointer of CMDQ, and 0x07010
means tail pointer. Current definitions are confusing, so rename them and
related variables.

The next_to_use of structure hns_roce_v2_cmq_ring has the same semantics as
head, merge them into one member. The next_to_clean of structure
hns_roce_v2_cmq_ring has the same semantics as tail. After deleting
next_to_clean, tail should also be deleted.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_common.h |  4 ++--
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c  | 37 +++++++++++++++--------------
 drivers/infiniband/hw/hns/hns_roce_hw_v2.h  |  3 ---
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
index 3ca6e88..23c438c 100644
--- a/drivers/infiniband/hw/hns/hns_roce_common.h
+++ b/drivers/infiniband/hw/hns/hns_roce_common.h
@@ -364,8 +364,8 @@
 #define ROCEE_TX_CMQ_BASEADDR_L_REG		0x07000
 #define ROCEE_TX_CMQ_BASEADDR_H_REG		0x07004
 #define ROCEE_TX_CMQ_DEPTH_REG			0x07008
-#define ROCEE_TX_CMQ_TAIL_REG			0x07010
-#define ROCEE_TX_CMQ_HEAD_REG			0x07014
+#define ROCEE_TX_CMQ_HEAD_REG			0x07010
+#define ROCEE_TX_CMQ_TAIL_REG			0x07014
 
 #define ROCEE_RX_CMQ_BASEADDR_L_REG		0x07018
 #define ROCEE_RX_CMQ_BASEADDR_H_REG		0x0701c
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 1945730..0fb9896 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1098,7 +1098,7 @@ static int hns_roce_init_cmq_ring(struct hns_roce_dev *hr_dev, bool ring_type)
 					    &priv->cmq.csq : &priv->cmq.crq;
 
 	ring->flag = ring_type;
-	ring->next_to_use = 0;
+	ring->head = 0;
 
 	return hns_roce_alloc_cmq_desc(hr_dev, ring);
 }
@@ -1197,10 +1197,10 @@ static void hns_roce_cmq_setup_basic_desc(struct hns_roce_cmq_desc *desc,
 
 static int hns_roce_cmq_csq_done(struct hns_roce_dev *hr_dev)
 {
-	u32 head = roce_read(hr_dev, ROCEE_TX_CMQ_HEAD_REG);
+	u32 tail = roce_read(hr_dev, ROCEE_TX_CMQ_TAIL_REG);
 	struct hns_roce_v2_priv *priv = hr_dev->priv;
 
-	return head == priv->cmq.csq.next_to_use;
+	return tail == priv->cmq.csq.head;
 }
 
 static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
@@ -1212,25 +1212,25 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 	u32 timeout = 0;
 	int handle = 0;
 	u16 desc_ret;
+	u32 tail;
 	int ret;
-	int ntc;
 
 	spin_lock_bh(&csq->lock);
 
-	ntc = csq->next_to_use;
+	tail = csq->head;
 
 	while (handle < num) {
-		desc_to_use = &csq->desc[csq->next_to_use];
+		desc_to_use = &csq->desc[csq->head];
 		*desc_to_use = desc[handle];
 		dev_dbg(hr_dev->dev, "set cmq desc:\n");
-		csq->next_to_use++;
-		if (csq->next_to_use == csq->desc_num)
-			csq->next_to_use = 0;
+		csq->head++;
+		if (csq->head == csq->desc_num)
+			csq->head = 0;
 		handle++;
 	}
 
 	/* Write to hardware */
-	roce_write(hr_dev, ROCEE_TX_CMQ_TAIL_REG, csq->next_to_use);
+	roce_write(hr_dev, ROCEE_TX_CMQ_HEAD_REG, csq->head);
 
 	/*
 	 * If the command is sync, wait for the firmware to write back,
@@ -1250,24 +1250,25 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 		ret = 0;
 		while (handle < num) {
 			/* get the result of hardware write back */
-			desc_to_use = &csq->desc[ntc];
+			desc_to_use = &csq->desc[tail];
 			desc[handle] = *desc_to_use;
 			dev_dbg(hr_dev->dev, "Get cmq desc:\n");
 			desc_ret = le16_to_cpu(desc[handle].retval);
 			if (unlikely(desc_ret != CMD_EXEC_SUCCESS))
 				ret = -EIO;
 
-			ntc++;
+			tail++;
 			handle++;
-			if (ntc == csq->desc_num)
-				ntc = 0;
+			if (tail == csq->desc_num)
+				tail = 0;
 		}
 	} else {
 		/* FW/HW reset or incorrect number of desc */
-		ntc = roce_read(hr_dev, ROCEE_TX_CMQ_HEAD_REG);
-		dev_warn(hr_dev->dev, "CMDQ move head from %d to %d\n",
-			 csq->next_to_use, ntc);
-		csq->next_to_use = ntc;
+		tail = roce_read(hr_dev, ROCEE_TX_CMQ_TAIL_REG);
+		dev_warn(hr_dev->dev, "CMDQ move tail from %d to %d\n",
+			 csq->head, tail);
+		csq->head = tail;
+
 		ret = -EAGAIN;
 	}
 
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
index 7714fa4..e773e82 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.h
@@ -1825,11 +1825,8 @@ struct hns_roce_v2_cmq_ring {
 	dma_addr_t desc_dma_addr;
 	struct hns_roce_cmq_desc *desc;
 	u32 head;
-	u32 tail;
-
 	u16 buf_size;
 	u16 desc_num;
-	int next_to_use;
 	u8 flag;
 	spinlock_t lock; /* command queue lock */
 };
-- 
2.8.1

[PATCH v2 for-next 5/5] RDMA/hns: Refactor process of posting CMDQ

[Weihang Li]

From: Lang Cheng <chenglang@huawei.com>

Simplify __hns_roce_cmq_send() then remove the redundant variables.

Signed-off-by: Lang Cheng <chenglang@huawei.com>
Signed-off-by: Weihang Li <liweihang@huawei.com>
---
 drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 42 ++++++++++++------------------
 1 file changed, 17 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
index 0fb9896..791935c 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v2.c
@@ -1208,32 +1208,26 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 {
 	struct hns_roce_v2_priv *priv = hr_dev->priv;
 	struct hns_roce_v2_cmq_ring *csq = &priv->cmq.csq;
-	struct hns_roce_cmq_desc *desc_to_use;
 	u32 timeout = 0;
-	int handle = 0;
 	u16 desc_ret;
 	u32 tail;
 	int ret;
+	int i;
 
 	spin_lock_bh(&csq->lock);
 
 	tail = csq->head;
 
-	while (handle < num) {
-		desc_to_use = &csq->desc[csq->head];
-		*desc_to_use = desc[handle];
-		dev_dbg(hr_dev->dev, "set cmq desc:\n");
-		csq->head++;
+	for (i = 0; i < num; i++) {
+		csq->desc[csq->head++] = desc[i];
 		if (csq->head == csq->desc_num)
 			csq->head = 0;
-		handle++;
 	}
 
 	/* Write to hardware */
 	roce_write(hr_dev, ROCEE_TX_CMQ_HEAD_REG, csq->head);
 
-	/*
-	 * If the command is sync, wait for the firmware to write back,
+	/* If the command is sync, wait for the firmware to write back,
 	 * if multi descriptors to be sent, use the first one to check
 	 */
 	if (le16_to_cpu(desc->flag) & HNS_ROCE_CMD_FLAG_NO_INTR) {
@@ -1241,26 +1235,24 @@ static int __hns_roce_cmq_send(struct hns_roce_dev *hr_dev,
 			if (hns_roce_cmq_csq_done(hr_dev))
 				break;
 			udelay(1);
-			timeout++;
-		} while (timeout < priv->cmq.tx_timeout);
+		} while (++timeout < priv->cmq.tx_timeout);
 	}
 
 	if (hns_roce_cmq_csq_done(hr_dev)) {
-		handle = 0;
-		ret = 0;
-		while (handle < num) {
-			/* get the result of hardware write back */
-			desc_to_use = &csq->desc[tail];
-			desc[handle] = *desc_to_use;
-			dev_dbg(hr_dev->dev, "Get cmq desc:\n");
-			desc_ret = le16_to_cpu(desc[handle].retval);
-			if (unlikely(desc_ret != CMD_EXEC_SUCCESS))
-				ret = -EIO;
-
-			tail++;
-			handle++;
+		for (ret = 0, i = 0; i < num; i++) {
+			/* check the result of hardware write back */
+			desc[i] = csq->desc[tail++];
 			if (tail == csq->desc_num)
 				tail = 0;
+
+			desc_ret = le16_to_cpu(desc[i].retval);
+			if (likely(desc_ret == CMD_EXEC_SUCCESS))
+				continue;
+
+			dev_err_ratelimited(hr_dev->dev,
+					    "Cmdq IO error, opcode = %x, return = %x\n",
+					    desc->opcode, desc_ret);
+			ret = -EIO;
 		}
 	} else {
 		/* FW/HW reset or incorrect number of desc */
-- 
2.8.1

Re: [PATCH v2 for-next 0/5] RDMA/hns: Fix and refactor CMDQ related code

[Jason Gunthorpe]

On Sun, Feb 07, 2021 at 04:55:38PM +0800, Weihang Li wrote:
> Remove some dead code in process of CMDQ transmission, and fix an issue
> about missing error code.
> 
> Changes since v1:
> * Drop #2 from the v1 series because the compatibility with the firmware
>   needs to be considered.
> * Link: https://patchwork.kernel.org/project/linux-rdma/cover/1612419786-39173-1-git-send-email-liweihang@huawei.com/
> 
> Lang Cheng (5):
>   RDMA/hns: Remove unused member and variable of CMDQ
>   RDMA/hns: Fixes missing error code of CMDQ
>   RDMA/hns: Remove redundant operations on CMDQ
>   RDMA/hns: Adjust fields and variables about CMDQ tail/head
>   RDMA/hns: Refactor process of posting CMDQ

Applied to for-next, thanks

Jason