* [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes
@ 2024-12-04 7:54 Kalesh AP
2024-12-04 7:54 ` [PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request Kalesh AP
` (5 more replies)
0 siblings, 6 replies; 18+ messages in thread
From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw)
To: leon, jgg; +Cc: linux-rdma, andrew.gospodarek, selvin.xavier, 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP @ 2024-12-04 7:54 ` Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 2/5] RDMA/bnxt_re: Avoid initializing the software queue for user queues Kalesh AP ` (4 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw) To: leon, jgg; +Cc: linux-rdma, andrew.gospodarek, selvin.xavier, Kashyap Desai 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH for-rc 2/5] RDMA/bnxt_re: Avoid initializing the software queue for user queues 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request Kalesh AP @ 2024-12-04 7:54 ` Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 3/5] RDMA/bnxt_re: Avoid sending the modify QP workaround for latest adapters Kalesh AP ` (3 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw) To: leon, jgg; +Cc: linux-rdma, andrew.gospodarek, selvin.xavier 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH for-rc 3/5] RDMA/bnxt_re: Avoid sending the modify QP workaround for latest adapters 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 2/5] RDMA/bnxt_re: Avoid initializing the software queue for user queues Kalesh AP @ 2024-12-04 7:54 ` Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence Kalesh AP ` (2 subsequent siblings) 5 siblings, 0 replies; 18+ messages in thread From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw) To: leon, jgg; +Cc: linux-rdma, andrew.gospodarek, selvin.xavier, Kashyap Desai 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP ` (2 preceding siblings ...) 2024-12-04 7:54 ` [PATCH for-rc 3/5] RDMA/bnxt_re: Avoid sending the modify QP workaround for latest adapters Kalesh AP @ 2024-12-04 7:54 ` Kalesh AP 2024-12-05 9:07 ` Leon Romanovsky 2024-12-04 7:54 ` [PATCH for-rc 5/5] RDMA/bnxt_re: Fix bnxt_re_destroy_qp() Kalesh AP 2024-12-05 9:08 ` [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Leon Romanovsky 5 siblings, 1 reply; 18+ messages in thread From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw) To: leon, jgg Cc: linux-rdma, andrew.gospodarek, selvin.xavier, Kalesh AP, Kashyap Desai 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-04 7:54 ` [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence Kalesh AP @ 2024-12-05 9:07 ` Leon Romanovsky 2024-12-05 9:31 ` Kalesh Anakkur Purayil 0 siblings, 1 reply; 18+ messages in thread From: Leon Romanovsky @ 2024-12-05 9:07 UTC (permalink / raw) To: Kalesh AP Cc: jgg, linux-rdma, andrew.gospodarek, selvin.xavier, Kashyap Desai 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-05 9:07 ` Leon Romanovsky @ 2024-12-05 9:31 ` Kalesh Anakkur Purayil 2024-12-05 11:38 ` Leon Romanovsky 0 siblings, 1 reply; 18+ messages in thread From: Kalesh Anakkur Purayil @ 2024-12-05 9:31 UTC (permalink / raw) To: Leon Romanovsky Cc: jgg, linux-rdma, andrew.gospodarek, selvin.xavier, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-05 9:31 ` Kalesh Anakkur Purayil @ 2024-12-05 11:38 ` Leon Romanovsky 2024-12-09 4:43 ` Selvin Xavier 0 siblings, 1 reply; 18+ messages in thread From: Leon Romanovsky @ 2024-12-05 11:38 UTC (permalink / raw) To: Kalesh Anakkur Purayil Cc: jgg, linux-rdma, andrew.gospodarek, selvin.xavier, Kashyap Desai 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-05 11:38 ` Leon Romanovsky @ 2024-12-09 4:43 ` Selvin Xavier 2024-12-10 11:48 ` Leon Romanovsky 0 siblings, 1 reply; 18+ messages in thread From: Selvin Xavier @ 2024-12-09 4:43 UTC (permalink / raw) To: Leon Romanovsky Cc: Kalesh Anakkur Purayil, jgg, linux-rdma, andrew.gospodarek, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-09 4:43 ` Selvin Xavier @ 2024-12-10 11:48 ` Leon Romanovsky 2024-12-11 6:03 ` Selvin Xavier 0 siblings, 1 reply; 18+ messages in thread From: Leon Romanovsky @ 2024-12-10 11:48 UTC (permalink / raw) To: Selvin Xavier Cc: Kalesh Anakkur Purayil, jgg, linux-rdma, andrew.gospodarek, Kashyap Desai 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 > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-10 11:48 ` Leon Romanovsky @ 2024-12-11 6:03 ` Selvin Xavier 2024-12-11 15:56 ` Jason Gunthorpe 0 siblings, 1 reply; 18+ messages in thread From: Selvin Xavier @ 2024-12-11 6:03 UTC (permalink / raw) To: Leon Romanovsky Cc: Kalesh Anakkur Purayil, jgg, linux-rdma, andrew.gospodarek, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-11 6:03 ` Selvin Xavier @ 2024-12-11 15:56 ` Jason Gunthorpe 2024-12-11 17:06 ` Selvin Xavier 0 siblings, 1 reply; 18+ messages in thread From: Jason Gunthorpe @ 2024-12-11 15:56 UTC (permalink / raw) To: Selvin Xavier Cc: Leon Romanovsky, Kalesh Anakkur Purayil, linux-rdma, andrew.gospodarek, Kashyap Desai 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-11 15:56 ` Jason Gunthorpe @ 2024-12-11 17:06 ` Selvin Xavier 2024-12-17 5:23 ` Kalesh Anakkur Purayil 0 siblings, 1 reply; 18+ messages in thread From: Selvin Xavier @ 2024-12-11 17:06 UTC (permalink / raw) To: Jason Gunthorpe Cc: Leon Romanovsky, Kalesh Anakkur Purayil, linux-rdma, andrew.gospodarek, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-11 17:06 ` Selvin Xavier @ 2024-12-17 5:23 ` Kalesh Anakkur Purayil 2024-12-19 11:46 ` Leon Romanovsky 0 siblings, 1 reply; 18+ messages in thread From: Kalesh Anakkur Purayil @ 2024-12-17 5:23 UTC (permalink / raw) To: Selvin Xavier Cc: Jason Gunthorpe, Leon Romanovsky, linux-rdma, andrew.gospodarek, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-17 5:23 ` Kalesh Anakkur Purayil @ 2024-12-19 11:46 ` Leon Romanovsky 2024-12-20 5:53 ` Kalesh Anakkur Purayil 0 siblings, 1 reply; 18+ messages in thread From: Leon Romanovsky @ 2024-12-19 11:46 UTC (permalink / raw) To: Kalesh Anakkur Purayil Cc: Selvin Xavier, Jason Gunthorpe, linux-rdma, andrew.gospodarek, Kashyap Desai 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence 2024-12-19 11:46 ` Leon Romanovsky @ 2024-12-20 5:53 ` Kalesh Anakkur Purayil 0 siblings, 0 replies; 18+ messages in thread From: Kalesh Anakkur Purayil @ 2024-12-20 5:53 UTC (permalink / raw) To: Leon Romanovsky Cc: Selvin Xavier, Jason Gunthorpe, linux-rdma, andrew.gospodarek, Kashyap Desai [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH for-rc 5/5] RDMA/bnxt_re: Fix bnxt_re_destroy_qp() 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP ` (3 preceding siblings ...) 2024-12-04 7:54 ` [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence Kalesh AP @ 2024-12-04 7:54 ` Kalesh AP 2024-12-05 9:08 ` [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Leon Romanovsky 5 siblings, 0 replies; 18+ messages in thread From: Kalesh AP @ 2024-12-04 7:54 UTC (permalink / raw) To: leon, jgg Cc: linux-rdma, andrew.gospodarek, selvin.xavier, Kalesh AP, Kashyap Desai 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP ` (4 preceding siblings ...) 2024-12-04 7:54 ` [PATCH for-rc 5/5] RDMA/bnxt_re: Fix bnxt_re_destroy_qp() Kalesh AP @ 2024-12-05 9:08 ` Leon Romanovsky 5 siblings, 0 replies; 18+ messages in thread From: Leon Romanovsky @ 2024-12-05 9:08 UTC (permalink / raw) To: Kalesh AP; +Cc: jgg, linux-rdma, andrew.gospodarek, selvin.xavier 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-12-20 5:54 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-12-04 7:54 [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 1/5] RDMA/bnxt_re: Fix max SGEs for the Work Request Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 2/5] RDMA/bnxt_re: Avoid initializing the software queue for user queues Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 3/5] RDMA/bnxt_re: Avoid sending the modify QP workaround for latest adapters Kalesh AP 2024-12-04 7:54 ` [PATCH for-rc 4/5] RDMA/bnxt_re: Fix error recovery sequence Kalesh AP 2024-12-05 9:07 ` Leon Romanovsky 2024-12-05 9:31 ` Kalesh Anakkur Purayil 2024-12-05 11:38 ` Leon Romanovsky 2024-12-09 4:43 ` Selvin Xavier 2024-12-10 11:48 ` Leon Romanovsky 2024-12-11 6:03 ` Selvin Xavier 2024-12-11 15:56 ` Jason Gunthorpe 2024-12-11 17:06 ` Selvin Xavier 2024-12-17 5:23 ` Kalesh Anakkur Purayil 2024-12-19 11:46 ` Leon Romanovsky 2024-12-20 5:53 ` Kalesh Anakkur Purayil 2024-12-04 7:54 ` [PATCH for-rc 5/5] RDMA/bnxt_re: Fix bnxt_re_destroy_qp() Kalesh AP 2024-12-05 9:08 ` [PATCH for-rc 0/5] RDMA/bnxt_re: Bug fixes Leon Romanovsky
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox