Re: [PATCH rdma-next v6 4/4] RDMA/bnxt_re: Direct Verbs: Support CQ and QP verbs

Sriharsha Basavapatna
2026-01-13T17:15:39Z

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

On Sat, Jan 10, 2026 at 1:07 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Wed, Dec 24, 2025 at 09:56:02AM +0530, Sriharsha Basavapatna wrote:
> > +static int bnxt_re_dv_create_qplib_cq(struct bnxt_re_dev *rdev,
> > + struct bnxt_re_ucontext *re_uctx,
> > + struct bnxt_re_cq *cq,
> > + struct bnxt_re_cq_req *req)
> > +{
> > + struct bnxt_qplib_dev_attr *dev_attr = rdev->dev_attr;
> > + struct bnxt_qplib_cq *qplcq;
> > + struct ib_umem *umem;
> > + u32 cqe = req->ncqe;
> > + u32 max_active_cqs;
> > + int rc = -EINVAL;
> > +
> > + if (atomic_read(&rdev->stats.res.cq_count) >= dev_attr->max_cq) {
> > + ibdev_dbg(&rdev->ibdev, "Create CQ failed - max exceeded(CQs)");
> > + return rc;
> > + }
>
> This is a racy way to use atomics, this should be an
> atomic_inc_return, then check the return code, doing a dec on all
> error paths.
Ack.
>
> > +static void bnxt_re_dv_init_ib_cq(struct bnxt_re_dev *rdev,
> > + struct bnxt_re_cq *re_cq)
> > +{
> > + struct ib_cq *ib_cq;
> > +
> > + ib_cq = &re_cq->ib_cq;
> > + ib_cq->device = &rdev->ibdev;
>
>
> > + ib_cq->uobject = NULL;
> > + ib_cq->comp_handler = NULL;
> > + ib_cq->event_handler = NULL;
> > + atomic_set(&ib_cq->usecnt, 0);
> > +}
>
> All these should already be 0 since this was freshly zallocated, no?
This function is not needed, deleted (design changed from direct
uverbs to driver specific uverbs extension).
>
> > +int bnxt_re_dv_create_cq(struct bnxt_re_dev *rdev, struct ib_udata *udata,
> > + struct bnxt_re_cq *re_cq, struct bnxt_re_cq_req *req)
> > +{
> > + struct bnxt_re_ucontext *re_uctx =
> > + rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
> > + struct bnxt_re_cq_resp resp = {};
> > + int ret;
> > +
> > + ret = bnxt_re_dv_create_qplib_cq(rdev, re_uctx, re_cq, req);
> > + if (ret)
> > + return ret;
> > +
> > + ret = bnxt_re_dv_create_cq_resp(rdev, re_cq, &resp);
> > + if (ret)
> > + goto fail_resp;
> > +
> > + ret = ib_copy_to_udata(udata, &resp, min(sizeof(resp), udata->outlen));
> > + if (ret)
> > + goto fail_resp;
> > +
> > + bnxt_re_dv_init_ib_cq(rdev, re_cq);
> > + re_cq->is_dv_cq = true;
> > + atomic_inc(&rdev->dv_cq_count);
> > + return 0;
> > +
> > +fail_resp:
> > + bnxt_qplib_destroy_cq(&rdev->qplib_res, &re_cq->qplib_cq);
> > + bnxt_re_put_nq(rdev, re_cq->qplib_cq.nq);
> > + ib_umem_release(re_cq->umem);
>
> This seems really weird error unwinding, I expect to see functions
> with error unwinds that match the calls within the function.
>
> So why doesn't bnxt_qplib_destroy_cq() do the umem release and
> req_put_nq?
Because bnxt_qplib_destroy_cq() deals with only the FW side and the
wrapper function bnxt_re_dv_create_qplib_cq() calls umem_get() and
get_nq().
Thanks,
-Harsha
>
> Jason

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