Re: [PATCH rdma-next v2 2/4] RDMA/bnxt_re: Refactor bnxt_qplib_create_qp() function

Leon Romanovsky
2025-11-11T10:14:47Z

On Mon, Nov 10, 2025 at 08:19:45PM +0530, Sriharsha Basavapatna wrote:
> On Sun, Nov 9, 2025 at 2:51 PM Leon Romanovsky <leon@kernel.org> wrote:
> >
> > On Tue, Nov 04, 2025 at 12:53:18PM +0530, Sriharsha Basavapatna wrote:
> > > From: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > >
> > > Inside bnxt_qplib_create_qp(), driver currently is doing
> > > a lot of things like allocating HWQ memory for SQ/RQ/ORRQ/IRRQ,
> > > initializing few of qplib_qp fields etc.
> > >
> > > Refactored the code such that all memory allocation for HWQs
> > > have been moved to bnxt_re_init_qp_attr() function and inside
> > > bnxt_qplib_create_qp() function just initialize the request
> > > structure and issue the HWRM command to firmware.
> > >
> > > Introduced couple of new functions bnxt_re_setup_qp_hwqs() and
> > > bnxt_re_setup_qp_swqs() moved the hwq and swq memory allocation
> > > logic there.
> > >
> > > This patch also introduces a change to store the PD id in
> > > bnxt_qplib_qp. Instead of keeping a pointer to "struct
> > > bnxt_qplib_pd", store PD id directly in "struct bnxt_qplib_qp".
> > > This change is needed for a subsequent change in this patch
> > > series. This PD ID value will be used in new DV implementation
> > > for create_qp(). There is no functional change.
> > >
> > > Signed-off-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
> > > Reviewed-by: Selvin Thyparampil Xavier <selvin.xavier@broadcom.com>
> > > Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> > > ---
> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 207 ++++++++++++--
> > > drivers/infiniband/hw/bnxt_re/qplib_fp.c | 311 +++++++---------------
> > > drivers/infiniband/hw/bnxt_re/qplib_fp.h | 10 +-
> > > drivers/infiniband/hw/bnxt_re/qplib_res.h | 6 +
> > > 4 files changed, 304 insertions(+), 230 deletions(-)
> >
> > <...>
> >
> > > +free_umem:
> > > + if (uctx)
> > > + bnxt_re_qp_free_umem(qp);
> >
> > <...>
> >
> > > + if (udata)
> > > + bnxt_re_qp_free_umem(qp);
> >
> > <...>
> >
> > Do you need to have if (..) here?
> > ib_umem_release() does nothing if pointer is NULL.
> Agreed, no need to have that if() check.
> >
> >
> > > + kfree(sq->swq);
> > > + sq->swq = NULL;
> >
> > Is this SQ reused?
> SQ is not reused after this clean up, no need to reset the pointer,
> will delete that line.
> >
> > > + return rc;
> > > +}
> >
> > <...>
> >
> > > struct bnxt_qplib_qp {
> > > - struct bnxt_qplib_pd *pd;
> > > + u32 pd_id;
> > > struct bnxt_qplib_dpi *dpi;
> > > struct bnxt_qplib_chip_ctx *cctx;
> > > u64 qp_handle;
> > > @@ -279,6 +279,7 @@ struct bnxt_qplib_qp {
> > > u8 wqe_mode;
> > > u8 state;
> > > u8 cur_qp_state;
> > > + u8 is_user;
> >
> > This is already known to IB/core, use rdma_is_kernel_res().
> This one is used in the qplib (fw interface) layer in the driver where
> we don't have the ib context, so I'd prefer to retain it.

My old plan was to rely on restrack for everything related to that,
together with removal of custom book-keeping logic.

This is why mlx5/mlx5 uses rdma_restrack_no_track() for internal objects.

Thanks

> Thanks,
> -Harsha
> >
> > Thanks