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

Selvin Xavier
2024-05-07T04:27:30Z

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

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.

Thanks

[1] https://github.com/linux-rdma/rdma-core/pull/1457
>
> Jason