On Tue, May 7, 2024 at 10:04 PM Jason Gunthorpe <jgg@xxxxxxxx> wrote:
>
> 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@xxxxxxxx> 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@xxxxxxxxxxxx>
> > > > ---
> > > > 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.
sure. will include it in v2
>
> Jason
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
