Re: [PATCH for-next 2/2] RDMA/bnxt_re: Expose the MSN table capability for user library

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, May 07, 2024 at 09:57:17AM +0530, Selvin Xavier wrote:
> On Mon, May 6, 2024 at 11:17 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Fri, May 03, 2024 at 09:10:19PM -0700, Selvin Xavier wrote:
> > > Expose the MSN table capability to the user space. Rename
> > > the current macro as the driver/library is allocating the
> > > table based on the MSN capability reported by FW.
> > >
> > > Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> > > ---
> > >  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 3 +++
> > >  include/uapi/rdma/bnxt_re-abi.h          | 2 +-
> > >  2 files changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > index ce9c5ba..d261b09 100644
> > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> > > @@ -4201,6 +4201,9 @@ int bnxt_re_alloc_ucontext(struct ib_ucontext *ctx, struct ib_udata *udata)
> > >       if (rdev->pacing.dbr_pacing)
> > >               resp.comp_mask |= BNXT_RE_UCNTX_CMASK_DBR_PACING_ENABLED;
> > >
> > > +     if (_is_host_msn_table(rdev->qplib_res.dattr->dev_cap_flags2))
> > > +             resp.comp_mask |= BNXT_RE_UCNTX_CMASK_MSN_TABLE_ENABLED;
> > > +
> > >       if (udata->inlen >= sizeof(ureq)) {
> > >               rc = ib_copy_from_udata(&ureq, udata, min(udata->inlen, sizeof(ureq)));
> > >               if (rc)
> > > diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h
> > > index c0c34ac..e61104f 100644
> > > --- a/include/uapi/rdma/bnxt_re-abi.h
> > > +++ b/include/uapi/rdma/bnxt_re-abi.h
> > > @@ -55,7 +55,7 @@ enum {
> > >       BNXT_RE_UCNTX_CMASK_WC_DPI_ENABLED = 0x04ULL,
> > >       BNXT_RE_UCNTX_CMASK_DBR_PACING_ENABLED = 0x08ULL,
> > >       BNXT_RE_UCNTX_CMASK_POW2_DISABLED = 0x10ULL,
> > > -     BNXT_RE_COMP_MASK_UCNTX_HW_RETX_ENABLED = 0x40,
> > > +     BNXT_RE_UCNTX_CMASK_MSN_TABLE_ENABLED = 0x40,
> >
> > Wah? How can you rename this bit in the uapi?
> >
> > Looks really strange, userspace is even using this constant.
> >
> > Please explain in detail what is going on here in the commit message. :\
> 
> BNXT_RE_COMP_MASK_UCNTX_HW_RETX_ENABLED was introduced to share the HW
> retransmit capability between driver and lib. The main difference in
> implementation for HW Retransmit support is the usage of MSN table or
> PSN table . When HW retrans is enabled, HW expects MSN table to be
> allocated by driver/lib, else PSN table (for older adapters). So when
> FW started exposing the MSN capability as a new field, we can actually
> depend on the new field instead of HW Retrasns capability. For
> adapters which support HW_RETX feature, MSN table capability will be
> set. For older adapters, this value will be 0  (to maintain backward
> compatibility with older FW).  I renamed the UAPI just to capture the
> correct name of the HW capability that driver/library is interested
> in.
> 
> I pushed an rdma-core PR [1] also with the associated change. Even if
> an older version of library is using
> BNXT_RE_COMP_MASK_UCNTX_HW_RETX_ENABLED, it doesn't affect the
> functionality and this is reason for renaming and not defining a new
> UAPI.  If you feel that I should totally avoid this UAPI change, will
> add a new comp mask and leave the current value unused.

It is fine if it works, I asked you to decribe in detail the reasoning
and outline why it is correct in the commit message.

Jason