| Message ID | 1714795819-12543-3-git-send-email-selvin.xavier@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/bnxt_re: MSN table capability check for latest adapters | expand |
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. :\ Jason
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
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
On Tue, May 7, 2024 at 10:04 PM Jason Gunthorpe <jgg@ziepe.ca> 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@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. sure. will include it in v2 > > Jason
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, }; enum bnxt_re_wqe_mode {
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(-)