| Message ID | 20210811051650.14914-1-pkushwaha@marvell.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] qedr: Move variables reset to qedr_set_common_qp_params() | expand |
On Wed, Aug 11, 2021 at 08:16:50AM +0300, Prabhakar Kushwaha wrote: > Qedr code is tightly coupled with existing both INIT transitions. > Here, during first INIT transition all variables are reset and > the RESET state is checked in post_recv() before any posting. > > Commit dc70f7c3ed34 ("RDMA/cma: Remove unnecessary INIT->INIT > transition") exposed this bug. Since we are reverting this the patch still makse sense? Certainly having a driver depend on two init->init transitions seems wrong to me Jason
> On 11 Aug 2021, at 13:44, Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Aug 11, 2021 at 08:16:50AM +0300, Prabhakar Kushwaha wrote: >> Qedr code is tightly coupled with existing both INIT transitions. >> Here, during first INIT transition all variables are reset and >> the RESET state is checked in post_recv() before any posting. >> >> Commit dc70f7c3ed34 ("RDMA/cma: Remove unnecessary INIT->INIT >> transition") exposed this bug. > > > Since we are reverting this the patch still makse sense? Certainly > having a driver depend on two init->init transitions seems wrong to me If what I wrote about adhering to the IBTA spec and transition the QP to INIT during rdma_{connect,accept} makes sense, I think we need a two-phased approach. 1. Get the ULPs to adhere. That would not demand any changes in cma. 2. Once 1 has been proven working, we can fix cma and make the transition to INIT in rdma_{connect,accept}. OK? Thxs, Håkon
Dear Jason, On Wed, Aug 11, 2021 at 5:15 PM Jason Gunthorpe <jgg@nvidia.com> wrote: > > On Wed, Aug 11, 2021 at 08:16:50AM +0300, Prabhakar Kushwaha wrote: > > Qedr code is tightly coupled with existing both INIT transitions. > > Here, during first INIT transition all variables are reset and > > the RESET state is checked in post_recv() before any posting. > > > > Commit dc70f7c3ed34 ("RDMA/cma: Remove unnecessary INIT->INIT > > transition") exposed this bug. > > > Since we are reverting this the patch still makse sense? Certainly > having a driver depend on two init->init transitions seems wrong to me > Yes, definitely still makes sense, thanks. -- prabhakar (pk)
On Wed, Aug 11, 2021 at 08:16:50AM +0300, Prabhakar Kushwaha wrote: > Qedr code is tightly coupled with existing both INIT transitions. > Here, during first INIT transition all variables are reset and > the RESET state is checked in post_recv() before any posting. > > Commit dc70f7c3ed34 ("RDMA/cma: Remove unnecessary INIT->INIT > transition") exposed this bug. > > So moving variables reset to qedr_set_common_qp_params() and also > avoid RESET state check for post_recv(). > > Signed-off-by: Michal Kalderon <mkalderon@marvell.com> > Signed-off-by: Ariel Elior <aelior@marvell.com> > Signed-off-by: Shai Malin <smalin@marvell.com> > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com> > --- > drivers/infiniband/hw/qedr/verbs.c | 32 +++++++++++++----------------- > 1 file changed, 14 insertions(+), 18 deletions(-) Applied to for-next, thanks Jason
diff --git a/drivers/infiniband/hw/qedr/verbs.c b/drivers/infiniband/hw/qedr/verbs.c index fdc47ef7d861..6b3c7bfbd3bd 100644 --- a/drivers/infiniband/hw/qedr/verbs.c +++ b/drivers/infiniband/hw/qedr/verbs.c @@ -1339,6 +1339,15 @@ static int qedr_copy_qp_uresp(struct qedr_dev *dev, return rc; } +static void qedr_reset_qp_hwq_info(struct qedr_qp_hwq_info *qph) +{ + qed_chain_reset(&qph->pbl); + qph->prod = 0; + qph->cons = 0; + qph->wqe_cons = 0; + qph->db_data.data.value = cpu_to_le16(0); +} + static void qedr_set_common_qp_params(struct qedr_dev *dev, struct qedr_qp *qp, struct qedr_pd *pd, @@ -1354,9 +1363,13 @@ static void qedr_set_common_qp_params(struct qedr_dev *dev, qp->qp_type = attrs->qp_type; qp->max_inline_data = attrs->cap.max_inline_data; qp->state = QED_ROCE_QP_STATE_RESET; + + qp->prev_wqe_size = 0; + qp->signaled = (attrs->sq_sig_type == IB_SIGNAL_ALL_WR) ? true : false; qp->dev = dev; if (qedr_qp_has_sq(qp)) { + qedr_reset_qp_hwq_info(&qp->sq); qp->sq.max_sges = attrs->cap.max_send_sge; qp->sq_cq = get_qedr_cq(attrs->send_cq); DP_DEBUG(dev, QEDR_MSG_QP, @@ -1368,6 +1381,7 @@ static void qedr_set_common_qp_params(struct qedr_dev *dev, qp->srq = get_qedr_srq(attrs->srq); if (qedr_qp_has_rq(qp)) { + qedr_reset_qp_hwq_info(&qp->rq); qp->rq_cq = get_qedr_cq(attrs->recv_cq); qp->rq.max_sges = attrs->cap.max_recv_sge; DP_DEBUG(dev, QEDR_MSG_QP, @@ -2359,15 +2373,6 @@ static enum qed_roce_qp_state qedr_get_state_from_ibqp( } } -static void qedr_reset_qp_hwq_info(struct qedr_qp_hwq_info *qph) -{ - qed_chain_reset(&qph->pbl); - qph->prod = 0; - qph->cons = 0; - qph->wqe_cons = 0; - qph->db_data.data.value = cpu_to_le16(0); -} - static int qedr_update_qp_state(struct qedr_dev *dev, struct qedr_qp *qp, enum qed_roce_qp_state cur_state, @@ -2382,9 +2387,6 @@ static int qedr_update_qp_state(struct qedr_dev *dev, case QED_ROCE_QP_STATE_RESET: switch (new_state) { case QED_ROCE_QP_STATE_INIT: - qp->prev_wqe_size = 0; - qedr_reset_qp_hwq_info(&qp->sq); - qedr_reset_qp_hwq_info(&qp->rq); break; default: status = -EINVAL; @@ -3915,12 +3917,6 @@ int qedr_post_recv(struct ib_qp *ibqp, const struct ib_recv_wr *wr, spin_lock_irqsave(&qp->q_lock, flags); - if (qp->state == QED_ROCE_QP_STATE_RESET) { - spin_unlock_irqrestore(&qp->q_lock, flags); - *bad_wr = wr; - return -EINVAL; - } - while (wr) { int i;
Qedr code is tightly coupled with existing both INIT transitions. Here, during first INIT transition all variables are reset and the RESET state is checked in post_recv() before any posting. Commit dc70f7c3ed34 ("RDMA/cma: Remove unnecessary INIT->INIT transition") exposed this bug. So moving variables reset to qedr_set_common_qp_params() and also avoid RESET state check for post_recv(). Signed-off-by: Michal Kalderon <mkalderon@marvell.com> Signed-off-by: Ariel Elior <aelior@marvell.com> Signed-off-by: Shai Malin <smalin@marvell.com> Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com> --- drivers/infiniband/hw/qedr/verbs.c | 32 +++++++++++++----------------- 1 file changed, 14 insertions(+), 18 deletions(-)