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

Sriharsha Basavapatna
2025-11-10T14:50:01Z

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

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.
Thanks,
-Harsha
>
> Thanks

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