diff mbox series

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

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

Commit Message

Selvin Xavier May 4, 2024, 4:10 a.m. UTC
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(-)

Comments

Jason Gunthorpe May 6, 2024, 5:47 p.m. UTC | #1
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
Selvin Xavier May 7, 2024, 4:27 a.m. UTC | #2
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
Jason Gunthorpe May 7, 2024, 4:34 p.m. UTC | #3
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
Selvin Xavier May 7, 2024, 7:36 p.m. UTC | #4
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 mbox series

Patch

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 {