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

[Leon Romanovsky <leon@kernel.org>]

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