[PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes

[PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

Includes some of the minor bug fixes in bnxt_re. Please
review and apply.

Thanks,
Selvin Xavier

Kalesh AP (5):
  RDMA/bnxt_re: Avoid creating fence MR for newer adapters
  RDMA/bnxt_re: Remove a redundant check inside bnxt_re_vf_res_config
  RDMA/bnxt_re: Fix unconditional fence for newer adapters
  RDMA/bnxt_re: Return error for SRQ resize
  RDMA/bnxt_re: Add a missing check in bnxt_qplib_query_srq

 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 43 +++++++++++++++++++++-----------
 drivers/infiniband/hw/bnxt_re/main.c     |  3 ---
 drivers/infiniband/hw/bnxt_re/qplib_fp.c |  3 ++-
 3 files changed, 30 insertions(+), 19 deletions(-)

-- 
2.5.5


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]

Re: [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes

[Leon Romanovsky]

On Mon, 22 Jan 2024 20:54:32 -0800, Selvin Xavier wrote:
> Includes some of the minor bug fixes in bnxt_re. Please
> review and apply.
> 
> Thanks,
> Selvin Xavier
> 
> Kalesh AP (5):
>   RDMA/bnxt_re: Avoid creating fence MR for newer adapters
>   RDMA/bnxt_re: Remove a redundant check inside bnxt_re_vf_res_config
>   RDMA/bnxt_re: Fix unconditional fence for newer adapters
>   RDMA/bnxt_re: Return error for SRQ resize
>   RDMA/bnxt_re: Add a missing check in bnxt_qplib_query_srq
> 
> [...]

Applied, thanks!

[1/5] RDMA/bnxt_re: Avoid creating fence MR for newer adapters
      https://git.kernel.org/rdma/rdma/c/282fd66e2ef6e5
[2/5] RDMA/bnxt_re: Remove a redundant check inside bnxt_re_vf_res_config
      https://git.kernel.org/rdma/rdma/c/8fcbf0a55f71be
[3/5] RDMA/bnxt_re: Fix unconditional fence for newer adapters
      https://git.kernel.org/rdma/rdma/c/8eaca6b5997bd8
[4/5] RDMA/bnxt_re: Return error for SRQ resize
      https://git.kernel.org/rdma/rdma/c/3687b450c5f32e
[5/5] RDMA/bnxt_re: Add a missing check in bnxt_qplib_query_srq
      https://git.kernel.org/rdma/rdma/c/80dde187f734cf

Best regards,
-- 
Leon Romanovsky <leon@kernel.org>

[PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes

[Kalesh AP]

This series contains some bug fixes for bnxt_re driver.
Please review and apply.

Thanks,
Kalesh AP

Kalesh AP (2):
  RDMA/bnxt_re: Fix error recovery sequence
  RDMA/bnxt_re: Fix bnxt_re_destroy_qp()

Kashyap Desai (2):
  RDMA/bnxt_re: Fix max SGEs for the Work Request
  RDMA/bnxt_re: Avoid sending the modify QP workaround for latest
    adapters

Selvin Xavier (1):
  RDMA/bnxt_re: Avoid initializing the software queue for user queues

 drivers/infiniband/hw/bnxt_re/ib_verbs.c   | 21 +++++------
 drivers/infiniband/hw/bnxt_re/main.c       |  8 +----
 drivers/infiniband/hw/bnxt_re/qplib_fp.c   | 42 ++++++++++++----------
 drivers/infiniband/hw/bnxt_re/qplib_fp.h   |  3 +-
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c |  7 ++--
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.h |  3 ++
 6 files changed, 41 insertions(+), 43 deletions(-)

-- 
2.31.1

[PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request

[Kalesh AP]

From: Kashyap Desai <kashyap.desai@broadcom.com>

Gen P7 supports up to 13 SGEs for now. WQE software structure
can hold only 6 now. Since the max send sge is reported as
13, the stack can give requests up to 13 SGEs. This is causing
traffic failures and system crashes.

Use the define for max SGE supported for variable size. This
will work for both static and variable WQEs.

Fixes: 227f51743b61 ("RDMA/bnxt_re: Fix the max WQE size for static WQE support")
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_fp.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.h b/drivers/infiniband/hw/bnxt_re/qplib_fp.h
index ef3424c81345..19e279871f10 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_fp.h
+++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.h
@@ -114,7 +114,6 @@ struct bnxt_qplib_sge {
 	u32				size;
 };
 
-#define BNXT_QPLIB_QP_MAX_SGL	6
 struct bnxt_qplib_swq {
 	u64				wr_id;
 	int				next_idx;
@@ -154,7 +153,7 @@ struct bnxt_qplib_swqe {
 #define BNXT_QPLIB_SWQE_FLAGS_UC_FENCE			BIT(2)
 #define BNXT_QPLIB_SWQE_FLAGS_SOLICIT_EVENT		BIT(3)
 #define BNXT_QPLIB_SWQE_FLAGS_INLINE			BIT(4)
-	struct bnxt_qplib_sge		sg_list[BNXT_QPLIB_QP_MAX_SGL];
+	struct bnxt_qplib_sge		sg_list[BNXT_VAR_MAX_SGE];
 	int				num_sge;
 	/* Max inline data is 96 bytes */
 	u32				inline_len;
-- 
2.31.1

[PATCH for-rc 2/5] RDMA/bnxt_re: Avoid initializing the software queue for user queues

[Kalesh AP]

From: Selvin Xavier <selvin.xavier@broadcom.com>

Software Queues to hold the WRs needs to be created
for only kernel queues. Avoid allocating the unnecessary
memory for user Queues.

Fixes: 1ac5a4047975 ("RDMA/bnxt_re: Add bnxt_re RoCE driver")
Fixes: 159fb4ceacd7 ("RDMA/bnxt_re: introduce a function to allocate swq")
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/qplib_fp.c | 42 +++++++++++++-----------
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/qplib_fp.c b/drivers/infiniband/hw/bnxt_re/qplib_fp.c
index e42abf5be6c0..8c6a09ac14ef 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_fp.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_fp.c
@@ -659,13 +659,6 @@ int bnxt_qplib_create_srq(struct bnxt_qplib_res *res,
 	rc = bnxt_qplib_alloc_init_hwq(&srq->hwq, &hwq_attr);
 	if (rc)
 		return rc;
-
-	srq->swq = kcalloc(srq->hwq.max_elements, sizeof(*srq->swq),
-			   GFP_KERNEL);
-	if (!srq->swq) {
-		rc = -ENOMEM;
-		goto fail;
-	}
 	srq->dbinfo.flags = 0;
 	bnxt_qplib_rcfw_cmd_prep((struct cmdq_base *)&req,
 				 CMDQ_BASE_OPCODE_CREATE_SRQ,
@@ -694,9 +687,17 @@ int bnxt_qplib_create_srq(struct bnxt_qplib_res *res,
 	spin_lock_init(&srq->lock);
 	srq->start_idx = 0;
 	srq->last_idx = srq->hwq.max_elements - 1;
-	for (idx = 0; idx < srq->hwq.max_elements; idx++)
-		srq->swq[idx].next_idx = idx + 1;
-	srq->swq[srq->last_idx].next_idx = -1;
+	if (!srq->hwq.is_user) {
+		srq->swq = kcalloc(srq->hwq.max_elements, sizeof(*srq->swq),
+				   GFP_KERNEL);
+		if (!srq->swq) {
+			rc = -ENOMEM;
+			goto fail;
+		}
+		for (idx = 0; idx < srq->hwq.max_elements; idx++)
+			srq->swq[idx].next_idx = idx + 1;
+		srq->swq[srq->last_idx].next_idx = -1;
+	}
 
 	srq->id = le32_to_cpu(resp.xid);
 	srq->dbinfo.hwq = &srq->hwq;
@@ -1044,13 +1045,14 @@ int bnxt_qplib_create_qp(struct bnxt_qplib_res *res, struct bnxt_qplib_qp *qp)
 	if (rc)
 		return rc;
 
-	rc = bnxt_qplib_alloc_init_swq(sq);
-	if (rc)
-		goto fail_sq;
-
-	if (psn_sz)
-		bnxt_qplib_init_psn_ptr(qp, psn_sz);
+	if (!sq->hwq.is_user) {
+		rc = bnxt_qplib_alloc_init_swq(sq);
+		if (rc)
+			goto fail_sq;
 
+		if (psn_sz)
+			bnxt_qplib_init_psn_ptr(qp, psn_sz);
+	}
 	req.sq_size = cpu_to_le32(bnxt_qplib_set_sq_size(sq, qp->wqe_mode));
 	pbl = &sq->hwq.pbl[PBL_LVL_0];
 	req.sq_pbl = cpu_to_le64(pbl->pg_map_arr[0]);
@@ -1076,9 +1078,11 @@ int bnxt_qplib_create_qp(struct bnxt_qplib_res *res, struct bnxt_qplib_qp *qp)
 		rc = bnxt_qplib_alloc_init_hwq(&rq->hwq, &hwq_attr);
 		if (rc)
 			goto sq_swq;
-		rc = bnxt_qplib_alloc_init_swq(rq);
-		if (rc)
-			goto fail_rq;
+		if (!rq->hwq.is_user) {
+			rc = bnxt_qplib_alloc_init_swq(rq);
+			if (rc)
+				goto fail_rq;
+		}
 
 		req.rq_size = cpu_to_le32(rq->max_wqe);
 		pbl = &rq->hwq.pbl[PBL_LVL_0];
-- 
2.31.1

[PATCH for-rc 3/5] RDMA/bnxt_re: Avoid sending the modify QP workaround for latest adapters

[Kalesh AP]

From: Kashyap Desai <kashyap.desai@broadcom.com>

The workaround to modify the UD QP from RTS to RTS is required
only for older adapters. Issuing this for latest adapters can caus
some unexpected behavior. Fix it

Fixes: 1801d87b3598 ("RDMA/bnxt_re: Support new 5760X P7 devices")
Signed-off-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 82023394e330..5428a1408cee 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -2824,7 +2824,8 @@ static int bnxt_re_post_send_shadow_qp(struct bnxt_re_dev *rdev,
 		wr = wr->next;
 	}
 	bnxt_qplib_post_send_db(&qp->qplib_qp);
-	bnxt_ud_qp_hw_stall_workaround(qp);
+	if (!bnxt_qplib_is_chip_gen_p5_p7(qp->rdev->chip_ctx))
+		bnxt_ud_qp_hw_stall_workaround(qp);
 	spin_unlock_irqrestore(&qp->sq_lock, flags);
 	return rc;
 }
@@ -2936,7 +2937,8 @@ int bnxt_re_post_send(struct ib_qp *ib_qp, const struct ib_send_wr *wr,
 		wr = wr->next;
 	}
 	bnxt_qplib_post_send_db(&qp->qplib_qp);
-	bnxt_ud_qp_hw_stall_workaround(qp);
+	if (!bnxt_qplib_is_chip_gen_p5_p7(qp->rdev->chip_ctx))
+		bnxt_ud_qp_hw_stall_workaround(qp);
 	spin_unlock_irqrestore(&qp->sq_lock, flags);
 
 	return rc;
-- 
2.31.1

[PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Kalesh AP]

Fixed to return ENXIO from __send_message_basic_sanity()
to indicate that device is in error state. In the case of
ERR_DEVICE_DETACHED state, the driver should not post the
commands to the firmware as it will time out eventually.

Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
as it is a no-op.

Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")

Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
 drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
index b7af0d5ff3b6..c143f273b759 100644
--- a/drivers/infiniband/hw/bnxt_re/main.c
+++ b/drivers/infiniband/hw/bnxt_re/main.c
@@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
 
 static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
 {
-	int mask = IB_QP_STATE;
-	struct ib_qp_attr qp_attr;
 	struct bnxt_re_qp *qp;
 
-	qp_attr.qp_state = IB_QPS_ERR;
 	mutex_lock(&rdev->qp_lock);
 	list_for_each_entry(qp, &rdev->qp_list, list) {
 		/* Modify the state of all QPs except QP1/Shadow QP */
@@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
 			if (qp->qplib_qp.state !=
 			    CMDQ_MODIFY_QP_NEW_STATE_RESET &&
 			    qp->qplib_qp.state !=
-			    CMDQ_MODIFY_QP_NEW_STATE_ERR) {
+			    CMDQ_MODIFY_QP_NEW_STATE_ERR)
 				bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
 						       1, IB_EVENT_QP_FATAL);
-				bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
-						  NULL);
-			}
 		}
 	}
 	mutex_unlock(&rdev->qp_lock);
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
index 5e90ea232de8..c8e65169f58a 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
@@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
 	cmdq = &rcfw->cmdq;
 
 	/* Prevent posting if f/w is not in a state to process */
-	if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
-		return bnxt_qplib_map_rc(opcode);
+	if (RCFW_NO_FW_ACCESS(rcfw))
+		return -ENXIO;
+
 	if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
 		return -ETIMEDOUT;
 
@@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
 
 	rc = __send_message_basic_sanity(rcfw, msg, opcode);
 	if (rc)
-		return rc;
+		return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
 
 	rc = __send_message(rcfw, msg, opcode);
 	if (rc)
diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
index 88814cb3aa74..4f7d800e35c3 100644
--- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
+++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
@@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
 
 #define RCFW_MAX_COOKIE_VALUE		(BNXT_QPLIB_CMDQE_MAX_CNT - 1)
 #define RCFW_CMD_IS_BLOCKING		0x8000
+#define RCFW_NO_FW_ACCESS(rcfw)						\
+	(test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||		\
+	 pci_channel_offline((rcfw)->pdev))
 
 #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
 /* HWRM version 1.10.3.18 */
-- 
2.31.1

[PATCH for-rc 5/5] RDMA/bnxt_re: Fix bnxt_re_destroy_qp()

[Kalesh AP]

1. Fixed to return 0 always from bnxt_re_destroy_qp().
2. Moved the code to delete QP debufgs dentries to the
   beginning of the function.

Fixes: d7d54769c042 ("RDMA/bnxt_re: Add debugfs hook in the driver")

Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 5428a1408cee..215074c0860b 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -967,13 +967,13 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 	unsigned int flags;
 	int rc;
 
+	bnxt_re_debug_rem_qpinfo(rdev, qp);
+
 	bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
 
 	rc = bnxt_qplib_destroy_qp(&rdev->qplib_res, &qp->qplib_qp);
-	if (rc) {
+	if (rc)
 		ibdev_err(&rdev->ibdev, "Failed to destroy HW QP");
-		return rc;
-	}
 
 	if (rdma_is_kernel_res(&qp->ib_qp.res)) {
 		flags = bnxt_re_lock_cqs(qp);
@@ -983,11 +983,8 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 
 	bnxt_qplib_free_qp_res(&rdev->qplib_res, &qp->qplib_qp);
 
-	if (ib_qp->qp_type == IB_QPT_GSI && rdev->gsi_ctx.gsi_sqp) {
-		rc = bnxt_re_destroy_gsi_sqp(qp);
-		if (rc)
-			return rc;
-	}
+	if (ib_qp->qp_type == IB_QPT_GSI && rdev->gsi_ctx.gsi_sqp)
+		bnxt_re_destroy_gsi_sqp(qp);
 
 	mutex_lock(&rdev->qp_lock);
 	list_del(&qp->list);
@@ -998,8 +995,6 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 	else if (qp->qplib_qp.type == CMDQ_CREATE_QP_TYPE_UD)
 		atomic_dec(&rdev->stats.res.ud_qp_count);
 
-	bnxt_re_debug_rem_qpinfo(rdev, qp);
-
 	ib_umem_release(qp->rumem);
 	ib_umem_release(qp->sumem);
 
-- 
2.31.1

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Leon Romanovsky]

On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> Fixed to return ENXIO from __send_message_basic_sanity()
> to indicate that device is in error state. In the case of
> ERR_DEVICE_DETACHED state, the driver should not post the
> commands to the firmware as it will time out eventually.
> 
> Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> as it is a no-op.
> 
> Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> 

Please don't add blank line here.

> Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
>  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
>  3 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> index b7af0d5ff3b6..c143f273b759 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
>  
>  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
>  {
> -	int mask = IB_QP_STATE;
> -	struct ib_qp_attr qp_attr;
>  	struct bnxt_re_qp *qp;
>  
> -	qp_attr.qp_state = IB_QPS_ERR;
>  	mutex_lock(&rdev->qp_lock);
>  	list_for_each_entry(qp, &rdev->qp_list, list) {
>  		/* Modify the state of all QPs except QP1/Shadow QP */
> @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
>  			if (qp->qplib_qp.state !=
>  			    CMDQ_MODIFY_QP_NEW_STATE_RESET &&
>  			    qp->qplib_qp.state !=
> -			    CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> +			    CMDQ_MODIFY_QP_NEW_STATE_ERR)
>  				bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
>  						       1, IB_EVENT_QP_FATAL);
> -				bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> -						  NULL);
> -			}
>  		}
>  	}
>  	mutex_unlock(&rdev->qp_lock);
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 5e90ea232de8..c8e65169f58a 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
>  	cmdq = &rcfw->cmdq;
>  
>  	/* Prevent posting if f/w is not in a state to process */
> -	if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> -		return bnxt_qplib_map_rc(opcode);
> +	if (RCFW_NO_FW_ACCESS(rcfw))
> +		return -ENXIO;
> +
>  	if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
>  		return -ETIMEDOUT;
>  
> @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
>  
>  	rc = __send_message_basic_sanity(rcfw, msg, opcode);
>  	if (rc)
> -		return rc;
> +		return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
>  
>  	rc = __send_message(rcfw, msg, opcode);
>  	if (rc)
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> index 88814cb3aa74..4f7d800e35c3 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
>  
>  #define RCFW_MAX_COOKIE_VALUE		(BNXT_QPLIB_CMDQE_MAX_CNT - 1)
>  #define RCFW_CMD_IS_BLOCKING		0x8000
> +#define RCFW_NO_FW_ACCESS(rcfw)						\
> +	(test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||		\
> +	 pci_channel_offline((rcfw)->pdev))

There is some disconnection between description and implementation.
ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
FW commands should stop already and if they are not, bnxt_re has bugs
in cleanup path. It should flush/cancel/e.t.c and not randomly test some
bit.

In addition, pci_channel_offline() in driver which doesn't manage PCI
device looks strange to me. It should be part of bnxt core and not
related to IB.

Thanks

>  
>  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
>  /* HWRM version 1.10.3.18 */
> -- 
> 2.31.1
> 

Re: [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes

[Leon Romanovsky]

On Wed, Dec 04, 2024 at 01:24:11PM +0530, Kalesh AP wrote:
> This series contains some bug fixes for bnxt_re driver.
> Please review and apply.
> 
> Thanks,
> Kalesh AP
> 
> Kalesh AP (2):
>   RDMA/bnxt_re: Fix error recovery sequence

Applied all patches except the patch above.

Thanks

>   RDMA/bnxt_re: Fix bnxt_re_destroy_qp()
> 
> Kashyap Desai (2):
>   RDMA/bnxt_re: Fix max SGEs for the Work Request
>   RDMA/bnxt_re: Avoid sending the modify QP workaround for latest
>     adapters
> 
> Selvin Xavier (1):
>   RDMA/bnxt_re: Avoid initializing the software queue for user queues
> 
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c   | 21 +++++------
>  drivers/infiniband/hw/bnxt_re/main.c       |  8 +----
>  drivers/infiniband/hw/bnxt_re/qplib_fp.c   | 42 ++++++++++++----------
>  drivers/infiniband/hw/bnxt_re/qplib_fp.h   |  3 +-
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c |  7 ++--
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h |  3 ++
>  6 files changed, 41 insertions(+), 43 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 5710 bytes --]

On Thu, Dec 5, 2024 at 2:37 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> > Fixed to return ENXIO from __send_message_basic_sanity()
> > to indicate that device is in error state. In the case of
> > ERR_DEVICE_DETACHED state, the driver should not post the
> > commands to the firmware as it will time out eventually.
> >
> > Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> > as it is a no-op.
> >
> > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> >
>
> Please don't add blank line here.
Sure, my bad. I missed it. Thanks for pointing it out.
>
> > Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > ---
> >  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
> >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
> >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
> >  3 files changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > index b7af0d5ff3b6..c143f273b759 100644
> > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> >
> >  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> >  {
> > -     int mask = IB_QP_STATE;
> > -     struct ib_qp_attr qp_attr;
> >       struct bnxt_re_qp *qp;
> >
> > -     qp_attr.qp_state = IB_QPS_ERR;
> >       mutex_lock(&rdev->qp_lock);
> >       list_for_each_entry(qp, &rdev->qp_list, list) {
> >               /* Modify the state of all QPs except QP1/Shadow QP */
> > @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> >                       if (qp->qplib_qp.state !=
> >                           CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> >                           qp->qplib_qp.state !=
> > -                         CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> > +                         CMDQ_MODIFY_QP_NEW_STATE_ERR)
> >                               bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> >                                                      1, IB_EVENT_QP_FATAL);
> > -                             bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> > -                                               NULL);
> > -                     }
> >               }
> >       }
> >       mutex_unlock(&rdev->qp_lock);
> > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > index 5e90ea232de8..c8e65169f58a 100644
> > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
> >       cmdq = &rcfw->cmdq;
> >
> >       /* Prevent posting if f/w is not in a state to process */
> > -     if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> > -             return bnxt_qplib_map_rc(opcode);
> > +     if (RCFW_NO_FW_ACCESS(rcfw))
> > +             return -ENXIO;
> > +
> >       if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
> >               return -ETIMEDOUT;
> >
> > @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
> >
> >       rc = __send_message_basic_sanity(rcfw, msg, opcode);
> >       if (rc)
> > -             return rc;
> > +             return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
> >
> >       rc = __send_message(rcfw, msg, opcode);
> >       if (rc)
> > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > index 88814cb3aa74..4f7d800e35c3 100644
> > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
> >
> >  #define RCFW_MAX_COOKIE_VALUE                (BNXT_QPLIB_CMDQE_MAX_CNT - 1)
> >  #define RCFW_CMD_IS_BLOCKING         0x8000
> > +#define RCFW_NO_FW_ACCESS(rcfw)                                              \
> > +     (test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||          \
> > +      pci_channel_offline((rcfw)->pdev))
>
> There is some disconnection between description and implementation.
> ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
> FW commands should stop already and if they are not, bnxt_re has bugs
> in cleanup path. It should flush/cancel/e.t.c and not randomly test some
> bit.
Yes, the device is in reset state. All outstanding firmware commands
are suspended. We do not want to post any new commands to firmware in
the recovery teardown path. Any commands sent to firmware at this
point will time out.
To avoid that, before posting the command driver checks the state and
returns early.
>
> In addition, pci_channel_offline() in driver which doesn't manage PCI
> device looks strange to me. It should be part of bnxt core and not
> related to IB.
The bnxt_re driver also has a firmware communication channel where it
writes to BAR to issue firmware commands. When the PCI channel is
offline, any commands issued from the driver will time out eventually.
To prevent that we added this extra check to detect that condition early.

>
> Thanks
>
> >
> >  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
> >  /* HWRM version 1.10.3.18 */
> > --
> > 2.31.1
> >



-- 
Regards,
Kalesh A P

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Leon Romanovsky]

On Thu, Dec 05, 2024 at 03:01:25PM +0530, Kalesh Anakkur Purayil wrote:
> On Thu, Dec 5, 2024 at 2:37 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> > > Fixed to return ENXIO from __send_message_basic_sanity()
> > > to indicate that device is in error state. In the case of
> > > ERR_DEVICE_DETACHED state, the driver should not post the
> > > commands to the firmware as it will time out eventually.
> > >
> > > Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> > > as it is a no-op.
> > >
> > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > >
> >
> > Please don't add blank line here.
> Sure, my bad. I missed it. Thanks for pointing it out.
> >
> > > Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > ---
> > >  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
> > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
> > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
> > >  3 files changed, 8 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > index b7af0d5ff3b6..c143f273b759 100644
> > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> > >
> > >  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > >  {
> > > -     int mask = IB_QP_STATE;
> > > -     struct ib_qp_attr qp_attr;
> > >       struct bnxt_re_qp *qp;
> > >
> > > -     qp_attr.qp_state = IB_QPS_ERR;
> > >       mutex_lock(&rdev->qp_lock);
> > >       list_for_each_entry(qp, &rdev->qp_list, list) {
> > >               /* Modify the state of all QPs except QP1/Shadow QP */
> > > @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > >                       if (qp->qplib_qp.state !=
> > >                           CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> > >                           qp->qplib_qp.state !=
> > > -                         CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> > > +                         CMDQ_MODIFY_QP_NEW_STATE_ERR)
> > >                               bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> > >                                                      1, IB_EVENT_QP_FATAL);
> > > -                             bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> > > -                                               NULL);
> > > -                     }
> > >               }
> > >       }
> > >       mutex_unlock(&rdev->qp_lock);
> > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > index 5e90ea232de8..c8e65169f58a 100644
> > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
> > >       cmdq = &rcfw->cmdq;
> > >
> > >       /* Prevent posting if f/w is not in a state to process */
> > > -     if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> > > -             return bnxt_qplib_map_rc(opcode);
> > > +     if (RCFW_NO_FW_ACCESS(rcfw))
> > > +             return -ENXIO;
> > > +
> > >       if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
> > >               return -ETIMEDOUT;
> > >
> > > @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
> > >
> > >       rc = __send_message_basic_sanity(rcfw, msg, opcode);
> > >       if (rc)
> > > -             return rc;
> > > +             return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
> > >
> > >       rc = __send_message(rcfw, msg, opcode);
> > >       if (rc)
> > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > index 88814cb3aa74..4f7d800e35c3 100644
> > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
> > >
> > >  #define RCFW_MAX_COOKIE_VALUE                (BNXT_QPLIB_CMDQE_MAX_CNT - 1)
> > >  #define RCFW_CMD_IS_BLOCKING         0x8000
> > > +#define RCFW_NO_FW_ACCESS(rcfw)                                              \
> > > +     (test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||          \
> > > +      pci_channel_offline((rcfw)->pdev))
> >
> > There is some disconnection between description and implementation.
> > ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
> > FW commands should stop already and if they are not, bnxt_re has bugs
> > in cleanup path. It should flush/cancel/e.t.c and not randomly test some
> > bit.
> Yes, the device is in reset state. All outstanding firmware commands
> are suspended. We do not want to post any new commands to firmware in
> the recovery teardown path. Any commands sent to firmware at this
> point will time out.
> To avoid that, before posting the command driver checks the state and
> returns early.

I understand that. Please reread my sentence "all FW commands should stop
already and if they are not, bnxt_re has bugs in cleanup path.", and answer
is how is it possible to get FW commands during restore.

> >
> > In addition, pci_channel_offline() in driver which doesn't manage PCI
> > device looks strange to me. It should be part of bnxt core and not
> > related to IB.
> The bnxt_re driver also has a firmware communication channel where it
> writes to BAR to issue firmware commands. When the PCI channel is
> offline, any commands issued from the driver will time out eventually.
> To prevent that we added this extra check to detect that condition early.

This micro optimization where you check in some random place for pci channel
status is not correct.

Thanks

> 
> >
> > Thanks
> >
> > >
> > >  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
> > >  /* HWRM version 1.10.3.18 */
> > > --
> > > 2.31.1
> > >
> 
> 
> 
> -- 
> Regards,
> Kalesh A P

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 7340 bytes --]

On Thu, Dec 5, 2024 at 5:08 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Thu, Dec 05, 2024 at 03:01:25PM +0530, Kalesh Anakkur Purayil wrote:
> > On Thu, Dec 5, 2024 at 2:37 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> > > > Fixed to return ENXIO from __send_message_basic_sanity()
> > > > to indicate that device is in error state. In the case of
> > > > ERR_DEVICE_DETACHED state, the driver should not post the
> > > > commands to the firmware as it will time out eventually.
> > > >
> > > > Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> > > > as it is a no-op.
> > > >
> > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > >
> > >
> > > Please don't add blank line here.
> > Sure, my bad. I missed it. Thanks for pointing it out.
> > >
> > > > Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > ---
> > > >  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
> > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
> > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
> > > >  3 files changed, 8 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > index b7af0d5ff3b6..c143f273b759 100644
> > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> > > >
> > > >  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > >  {
> > > > -     int mask = IB_QP_STATE;
> > > > -     struct ib_qp_attr qp_attr;
> > > >       struct bnxt_re_qp *qp;
> > > >
> > > > -     qp_attr.qp_state = IB_QPS_ERR;
> > > >       mutex_lock(&rdev->qp_lock);
> > > >       list_for_each_entry(qp, &rdev->qp_list, list) {
> > > >               /* Modify the state of all QPs except QP1/Shadow QP */
> > > > @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > >                       if (qp->qplib_qp.state !=
> > > >                           CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> > > >                           qp->qplib_qp.state !=
> > > > -                         CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> > > > +                         CMDQ_MODIFY_QP_NEW_STATE_ERR)
> > > >                               bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> > > >                                                      1, IB_EVENT_QP_FATAL);
> > > > -                             bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> > > > -                                               NULL);
> > > > -                     }
> > > >               }
> > > >       }
> > > >       mutex_unlock(&rdev->qp_lock);
> > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > index 5e90ea232de8..c8e65169f58a 100644
> > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
> > > >       cmdq = &rcfw->cmdq;
> > > >
> > > >       /* Prevent posting if f/w is not in a state to process */
> > > > -     if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> > > > -             return bnxt_qplib_map_rc(opcode);
> > > > +     if (RCFW_NO_FW_ACCESS(rcfw))
> > > > +             return -ENXIO;
> > > > +
> > > >       if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
> > > >               return -ETIMEDOUT;
> > > >
> > > > @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
> > > >
> > > >       rc = __send_message_basic_sanity(rcfw, msg, opcode);
> > > >       if (rc)
> > > > -             return rc;
> > > > +             return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
> > > >
> > > >       rc = __send_message(rcfw, msg, opcode);
> > > >       if (rc)
> > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > index 88814cb3aa74..4f7d800e35c3 100644
> > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
> > > >
> > > >  #define RCFW_MAX_COOKIE_VALUE                (BNXT_QPLIB_CMDQE_MAX_CNT - 1)
> > > >  #define RCFW_CMD_IS_BLOCKING         0x8000
> > > > +#define RCFW_NO_FW_ACCESS(rcfw)                                              \
> > > > +     (test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||          \
> > > > +      pci_channel_offline((rcfw)->pdev))
> > >
> > > There is some disconnection between description and implementation.
> > > ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
> > > FW commands should stop already and if they are not, bnxt_re has bugs
> > > in cleanup path. It should flush/cancel/e.t.c and not randomly test some
> > > bit.
> > Yes, the device is in reset state. All outstanding firmware commands
> > are suspended. We do not want to post any new commands to firmware in
> > the recovery teardown path. Any commands sent to firmware at this
> > point will time out.
> > To avoid that, before posting the command driver checks the state and
> > returns early.
>
> I understand that. Please reread my sentence "all FW commands should stop
> already and if they are not, bnxt_re has bugs in cleanup path.", and answer
> is how is it possible to get FW commands during restore.
Hi Leon,

Not sure if I also got your point correctly. Once the error recovery
gets initiated, we
unregister the ib device in the suspend path. During the ib_unregister_device,
we get verb commands to destroy the QP, CQs etc. We want to prevent sending the
new commands to FW for all these operations. We also want to avoid sending
any resource creation commands from the stack while the device is
getting re-initialized
This is a common check that prevents more commands from the stack down
to Firmware.

>
> > >
> > > In addition, pci_channel_offline() in driver which doesn't manage PCI
> > > device looks strange to me. It should be part of bnxt core and not
> > > related to IB.
> > The bnxt_re driver also has a firmware communication channel where it
> > writes to BAR to issue firmware commands. When the PCI channel is
> > offline, any commands issued from the driver will time out eventually.
> > To prevent that we added this extra check to detect that condition early.
>
> This micro optimization where you check in some random place for pci channel
> status is not correct.
Will remove pci_channel_offline check and come up with some other
mechanism to handle this case.
>
> Thanks
>
> >
> > >
> > > Thanks
> > >
> > > >
> > > >  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
> > > >  /* HWRM version 1.10.3.18 */
> > > > --
> > > > 2.31.1
> > > >
> >
> >
> >
> > --
> > Regards,
> > Kalesh A P
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Leon Romanovsky]

On Mon, Dec 09, 2024 at 10:13:23AM +0530, Selvin Xavier wrote:
> On Thu, Dec 5, 2024 at 5:08 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Thu, Dec 05, 2024 at 03:01:25PM +0530, Kalesh Anakkur Purayil wrote:
> > > On Thu, Dec 5, 2024 at 2:37 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> > > > > Fixed to return ENXIO from __send_message_basic_sanity()
> > > > > to indicate that device is in error state. In the case of
> > > > > ERR_DEVICE_DETACHED state, the driver should not post the
> > > > > commands to the firmware as it will time out eventually.
> > > > >
> > > > > Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> > > > > as it is a no-op.
> > > > >
> > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > >
> > > >
> > > > Please don't add blank line here.
> > > Sure, my bad. I missed it. Thanks for pointing it out.
> > > >
> > > > > Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > > ---
> > > > >  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
> > > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
> > > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
> > > > >  3 files changed, 8 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > index b7af0d5ff3b6..c143f273b759 100644
> > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> > > > >
> > > > >  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > > >  {
> > > > > -     int mask = IB_QP_STATE;
> > > > > -     struct ib_qp_attr qp_attr;
> > > > >       struct bnxt_re_qp *qp;
> > > > >
> > > > > -     qp_attr.qp_state = IB_QPS_ERR;
> > > > >       mutex_lock(&rdev->qp_lock);
> > > > >       list_for_each_entry(qp, &rdev->qp_list, list) {
> > > > >               /* Modify the state of all QPs except QP1/Shadow QP */
> > > > > @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > > >                       if (qp->qplib_qp.state !=
> > > > >                           CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> > > > >                           qp->qplib_qp.state !=
> > > > > -                         CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> > > > > +                         CMDQ_MODIFY_QP_NEW_STATE_ERR)
> > > > >                               bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> > > > >                                                      1, IB_EVENT_QP_FATAL);
> > > > > -                             bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> > > > > -                                               NULL);
> > > > > -                     }
> > > > >               }
> > > > >       }
> > > > >       mutex_unlock(&rdev->qp_lock);
> > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > index 5e90ea232de8..c8e65169f58a 100644
> > > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
> > > > >       cmdq = &rcfw->cmdq;
> > > > >
> > > > >       /* Prevent posting if f/w is not in a state to process */
> > > > > -     if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> > > > > -             return bnxt_qplib_map_rc(opcode);
> > > > > +     if (RCFW_NO_FW_ACCESS(rcfw))
> > > > > +             return -ENXIO;
> > > > > +
> > > > >       if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
> > > > >               return -ETIMEDOUT;
> > > > >
> > > > > @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
> > > > >
> > > > >       rc = __send_message_basic_sanity(rcfw, msg, opcode);
> > > > >       if (rc)
> > > > > -             return rc;
> > > > > +             return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
> > > > >
> > > > >       rc = __send_message(rcfw, msg, opcode);
> > > > >       if (rc)
> > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > index 88814cb3aa74..4f7d800e35c3 100644
> > > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
> > > > >
> > > > >  #define RCFW_MAX_COOKIE_VALUE                (BNXT_QPLIB_CMDQE_MAX_CNT - 1)
> > > > >  #define RCFW_CMD_IS_BLOCKING         0x8000
> > > > > +#define RCFW_NO_FW_ACCESS(rcfw)                                              \
> > > > > +     (test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||          \
> > > > > +      pci_channel_offline((rcfw)->pdev))
> > > >
> > > > There is some disconnection between description and implementation.
> > > > ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
> > > > FW commands should stop already and if they are not, bnxt_re has bugs
> > > > in cleanup path. It should flush/cancel/e.t.c and not randomly test some
> > > > bit.
> > > Yes, the device is in reset state. All outstanding firmware commands
> > > are suspended. We do not want to post any new commands to firmware in
> > > the recovery teardown path. Any commands sent to firmware at this
> > > point will time out.
> > > To avoid that, before posting the command driver checks the state and
> > > returns early.
> >
> > I understand that. Please reread my sentence "all FW commands should stop
> > already and if they are not, bnxt_re has bugs in cleanup path.", and answer
> > is how is it possible to get FW commands during restore.
> Hi Leon,
> 
> Not sure if I also got your point correctly. Once the error recovery
> gets initiated, we
> unregister the ib device in the suspend path. During the ib_unregister_device,
> we get verb commands to destroy the QP, CQs etc. We want to prevent sending the
> new commands to FW for all these operations. We also want to avoid sending
> any resource creation commands from the stack while the device is
> getting re-initialized

The thing is that during ib_unregister_device nothing external to driver
is going to be sent to FW.

> This is a common check that prevents more commands from the stack down
> to Firmware.
> 
> >
> > > >
> > > > In addition, pci_channel_offline() in driver which doesn't manage PCI
> > > > device looks strange to me. It should be part of bnxt core and not
> > > > related to IB.
> > > The bnxt_re driver also has a firmware communication channel where it
> > > writes to BAR to issue firmware commands. When the PCI channel is
> > > offline, any commands issued from the driver will time out eventually.
> > > To prevent that we added this extra check to detect that condition early.
> >
> > This micro optimization where you check in some random place for pci channel
> > status is not correct.
> Will remove pci_channel_offline check and come up with some other
> mechanism to handle this case.
> >
> > Thanks
> >
> > >
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > >  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
> > > > >  /* HWRM version 1.10.3.18 */
> > > > > --
> > > > > 2.31.1
> > > > >
> > >
> > >
> > >
> > > --
> > > Regards,
> > > Kalesh A P
> >
> >

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 9008 bytes --]

On Tue, Dec 10, 2024 at 5:18 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Mon, Dec 09, 2024 at 10:13:23AM +0530, Selvin Xavier wrote:
> > On Thu, Dec 5, 2024 at 5:08 PM Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > On Thu, Dec 05, 2024 at 03:01:25PM +0530, Kalesh Anakkur Purayil wrote:
> > > > On Thu, Dec 5, 2024 at 2:37 PM Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > On Wed, Dec 04, 2024 at 01:24:15PM +0530, Kalesh AP wrote:
> > > > > > Fixed to return ENXIO from __send_message_basic_sanity()
> > > > > > to indicate that device is in error state. In the case of
> > > > > > ERR_DEVICE_DETACHED state, the driver should not post the
> > > > > > commands to the firmware as it will time out eventually.
> > > > > >
> > > > > > Removed bnxt_re_modify_qp() call from bnxt_re_dev_stop()
> > > > > > as it is a no-op.
> > > > > >
> > > > > > Fixes: cc5b9b48d447 ("RDMA/bnxt_re: Recover the device when FW error is detected")
> > > > > >
> > > > >
> > > > > Please don't add blank line here.
> > > > Sure, my bad. I missed it. Thanks for pointing it out.
> > > > >
> > > > > > Reviewed-by: Kashyap Desai <kashyap.desai@broadcom.com>
> > > > > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > > > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > > > > ---
> > > > > >  drivers/infiniband/hw/bnxt_re/main.c       | 8 +-------
> > > > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 7 ++++---
> > > > > >  drivers/infiniband/hw/bnxt_re/qplib_rcfw.h | 3 +++
> > > > > >  3 files changed, 8 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/main.c b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > index b7af0d5ff3b6..c143f273b759 100644
> > > > > > --- a/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > +++ b/drivers/infiniband/hw/bnxt_re/main.c
> > > > > > @@ -1715,11 +1715,8 @@ static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> > > > > >
> > > > > >  static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > > > >  {
> > > > > > -     int mask = IB_QP_STATE;
> > > > > > -     struct ib_qp_attr qp_attr;
> > > > > >       struct bnxt_re_qp *qp;
> > > > > >
> > > > > > -     qp_attr.qp_state = IB_QPS_ERR;
> > > > > >       mutex_lock(&rdev->qp_lock);
> > > > > >       list_for_each_entry(qp, &rdev->qp_list, list) {
> > > > > >               /* Modify the state of all QPs except QP1/Shadow QP */
> > > > > > @@ -1727,12 +1724,9 @@ static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev)
> > > > > >                       if (qp->qplib_qp.state !=
> > > > > >                           CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> > > > > >                           qp->qplib_qp.state !=
> > > > > > -                         CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> > > > > > +                         CMDQ_MODIFY_QP_NEW_STATE_ERR)
> > > > > >                               bnxt_re_dispatch_event(&rdev->ibdev, &qp->ib_qp,
> > > > > >                                                      1, IB_EVENT_QP_FATAL);
> > > > > > -                             bnxt_re_modify_qp(&qp->ib_qp, &qp_attr, mask,
> > > > > > -                                               NULL);
> > > > > > -                     }
> > > > > >               }
> > > > > >       }
> > > > > >       mutex_unlock(&rdev->qp_lock);
> > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > > index 5e90ea232de8..c8e65169f58a 100644
> > > > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> > > > > > @@ -423,8 +423,9 @@ static int __send_message_basic_sanity(struct bnxt_qplib_rcfw *rcfw,
> > > > > >       cmdq = &rcfw->cmdq;
> > > > > >
> > > > > >       /* Prevent posting if f/w is not in a state to process */
> > > > > > -     if (test_bit(ERR_DEVICE_DETACHED, &rcfw->cmdq.flags))
> > > > > > -             return bnxt_qplib_map_rc(opcode);
> > > > > > +     if (RCFW_NO_FW_ACCESS(rcfw))
> > > > > > +             return -ENXIO;
> > > > > > +
> > > > > >       if (test_bit(FIRMWARE_STALL_DETECTED, &cmdq->flags))
> > > > > >               return -ETIMEDOUT;
> > > > > >
> > > > > > @@ -493,7 +494,7 @@ static int __bnxt_qplib_rcfw_send_message(struct bnxt_qplib_rcfw *rcfw,
> > > > > >
> > > > > >       rc = __send_message_basic_sanity(rcfw, msg, opcode);
> > > > > >       if (rc)
> > > > > > -             return rc;
> > > > > > +             return rc == -ENXIO ? bnxt_qplib_map_rc(opcode) : rc;
> > > > > >
> > > > > >       rc = __send_message(rcfw, msg, opcode);
> > > > > >       if (rc)
> > > > > > diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > > index 88814cb3aa74..4f7d800e35c3 100644
> > > > > > --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > > +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.h
> > > > > > @@ -129,6 +129,9 @@ static inline u32 bnxt_qplib_set_cmd_slots(struct cmdq_base *req)
> > > > > >
> > > > > >  #define RCFW_MAX_COOKIE_VALUE                (BNXT_QPLIB_CMDQE_MAX_CNT - 1)
> > > > > >  #define RCFW_CMD_IS_BLOCKING         0x8000
> > > > > > +#define RCFW_NO_FW_ACCESS(rcfw)                                              \
> > > > > > +     (test_bit(ERR_DEVICE_DETACHED, &(rcfw)->cmdq.flags) ||          \
> > > > > > +      pci_channel_offline((rcfw)->pdev))
> > > > >
> > > > > There is some disconnection between description and implementation.
> > > > > ERR_DEVICE_DETACHED is set when device is suspended, at this stage all
> > > > > FW commands should stop already and if they are not, bnxt_re has bugs
> > > > > in cleanup path. It should flush/cancel/e.t.c and not randomly test some
> > > > > bit.
> > > > Yes, the device is in reset state. All outstanding firmware commands
> > > > are suspended. We do not want to post any new commands to firmware in
> > > > the recovery teardown path. Any commands sent to firmware at this
> > > > point will time out.
> > > > To avoid that, before posting the command driver checks the state and
> > > > returns early.
> > >
> > > I understand that. Please reread my sentence "all FW commands should stop
> > > already and if they are not, bnxt_re has bugs in cleanup path.", and answer
> > > is how is it possible to get FW commands during restore.
> > Hi Leon,
> >
> > Not sure if I also got your point correctly. Once the error recovery
> > gets initiated, we
> > unregister the ib device in the suspend path. During the ib_unregister_device,
> > we get verb commands to destroy the QP, CQs etc. We want to prevent sending the
> > new commands to FW for all these operations. We also want to avoid sending
> > any resource creation commands from the stack while the device is
> > getting re-initialized
>
> The thing is that during ib_unregister_device nothing external to driver
> is going to be sent to FW.
ib_unregister will trigger the destroy_qp (at least QP1) and
destroy_cqs. Those verbs will be trying to
issue the FW command and we are trying to prevent sending it to FW here.
We need a check here in the common path to avoid sending any commands
and i dont see a way
to handle this otherwise. Current check has a bug where the return
code check was not correct and we
ended up sending some of the commands that eventually timeout.

Just to add, We have similar implementation in our L2 driver also,
which we were trying to replicate using
bnxt_re data structures.

#define BNXT_NO_FW_ACCESS(bp)                                   \
        (test_bit(BNXT_STATE_FW_FATAL_COND, &(bp)->state) ||    \
         pci_channel_offline((bp)->pdev))

Thanks,
Selvin
>
> > This is a common check that prevents more commands from the stack down
> > to Firmware.
> >
> > >
> > > > >
> > > > > In addition, pci_channel_offline() in driver which doesn't manage PCI
> > > > > device looks strange to me. It should be part of bnxt core and not
> > > > > related to IB.
> > > > The bnxt_re driver also has a firmware communication channel where it
> > > > writes to BAR to issue firmware commands. When the PCI channel is
> > > > offline, any commands issued from the driver will time out eventually.
> > > > To prevent that we added this extra check to detect that condition early.
> > >
> > > This micro optimization where you check in some random place for pci channel
> > > status is not correct.
> > Will remove pci_channel_offline check and come up with some other
> > mechanism to handle this case.
> > >
> > > Thanks
> > >
> > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > >  #define HWRM_VERSION_DEV_ATTR_MAX_DPI  0x1000A0000000DULL
> > > > > >  /* HWRM version 1.10.3.18 */
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Regards,
> > > > Kalesh A P
> > >
> > >
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Jason Gunthorpe]

On Wed, Dec 11, 2024 at 11:33:38AM +0530, Selvin Xavier wrote:

> ib_unregister will trigger the destroy_qp (at least QP1) and
> destroy_cqs. Those verbs will be trying to issue the FW command and
> we are trying to prevent sending it to FW here.  We need a check
> here in the common path to avoid sending any commands and i dont see
> a way to handle this otherwise. Current check has a bug where the
> return code check was not correct and we ended up sending some of
> the commands that eventually timeout.
> 
> Just to add, We have similar implementation in our L2 driver also,
> which we were trying to replicate using
> bnxt_re data structures.

Doesn't that suggest this is layered wrong? Shouldn't these tests be
inside the shared command submission code?

Jason

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Selvin Xavier]

[-- Attachment #1: Type: text/plain, Size: 1115 bytes --]

On Wed, Dec 11, 2024 at 9:26 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 11, 2024 at 11:33:38AM +0530, Selvin Xavier wrote:
>
> > ib_unregister will trigger the destroy_qp (at least QP1) and
> > destroy_cqs. Those verbs will be trying to issue the FW command and
> > we are trying to prevent sending it to FW here.  We need a check
> > here in the common path to avoid sending any commands and i dont see
> > a way to handle this otherwise. Current check has a bug where the
> > return code check was not correct and we ended up sending some of
> > the commands that eventually timeout.
> >
> > Just to add, We have similar implementation in our L2 driver also,
> > which we were trying to replicate using
> > bnxt_re data structures.
>
> Doesn't that suggest this is layered wrong? Shouldn't these tests be
> inside the shared command submission code?
There are two separate communication channel for this adapter. One owned by
L2 driver and another owned by RoCE driver. The RoCE driver checks are
for the second communication channel. L2 doesn't use this channel.
>
> Jason

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4224 bytes --]

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 1417 bytes --]

Hi Leon/Jason,

Are we good here? Or, do you still have any more concerns on this
patch? Please let me know whether I need to push a V2.

Regards,
Kalesh AP

On Wed, Dec 11, 2024 at 10:37 PM Selvin Xavier
<selvin.xavier@broadcom.com> wrote:
>
> On Wed, Dec 11, 2024 at 9:26 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Wed, Dec 11, 2024 at 11:33:38AM +0530, Selvin Xavier wrote:
> >
> > > ib_unregister will trigger the destroy_qp (at least QP1) and
> > > destroy_cqs. Those verbs will be trying to issue the FW command and
> > > we are trying to prevent sending it to FW here.  We need a check
> > > here in the common path to avoid sending any commands and i dont see
> > > a way to handle this otherwise. Current check has a bug where the
> > > return code check was not correct and we ended up sending some of
> > > the commands that eventually timeout.
> > >
> > > Just to add, We have similar implementation in our L2 driver also,
> > > which we were trying to replicate using
> > > bnxt_re data structures.
> >
> > Doesn't that suggest this is layered wrong? Shouldn't these tests be
> > inside the shared command submission code?
> There are two separate communication channel for this adapter. One owned by
> L2 driver and another owned by RoCE driver. The RoCE driver checks are
> for the second communication channel. L2 doesn't use this channel.
> >
> > Jason

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Leon Romanovsky]

On Tue, Dec 17, 2024 at 10:53:08AM +0530, Kalesh Anakkur Purayil wrote:
> Hi Leon/Jason,
> 
> Are we good here? Or, do you still have any more concerns on this
> patch? Please let me know whether I need to push a V2.

Yes, if you make sure that your newly added check is not racy.

Thanks

Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence

[Kalesh Anakkur Purayil]

[-- Attachment #1: Type: text/plain, Size: 594 bytes --]

On Thu, Dec 19, 2024 at 5:16 PM Leon Romanovsky <leon@kernel.org> wrote:
>
> On Tue, Dec 17, 2024 at 10:53:08AM +0530, Kalesh Anakkur Purayil wrote:
> > Hi Leon/Jason,
> >
> > Are we good here? Or, do you still have any more concerns on this
> > patch? Please let me know whether I need to push a V2.
>
> Yes, if you make sure that your newly added check is not racy.
>
> Thanks
Thank you Leon.
Yes, there is no race with this newly added check.
Let me post the same patch as V2 since the other patches in the series
have been accepted already.



-- 
Regards,
Kalesh AP

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4239 bytes --]