| Message ID | 1546236607-15948-7-git-send-email-devesh.sharma@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support Broadcom's 57500 series of adapters | expand |
On Mon, Dec 31, 2018 at 01:10:06AM -0500, Devesh Sharma wrote: > User space verbs provider library would need chip context. > Changing the ABI to add chip version details in structure. > Furthermore, changing the kernel driver ucontext allocation > code to initializ the abi structure with appropriate values. > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > --- > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 12 +++++++++--- > include/uapi/rdma/bnxt_re-abi.h | 9 ++++++--- > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 7a09ca7..3bec789 100644 > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -3693,9 +3693,10 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > struct ib_udata *udata) > { > struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev); > + struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > struct bnxt_re_uctx_resp resp; > struct bnxt_re_ucontext *uctx; > - struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > + u32 chip_met_rev_num = 0; > int rc; > > dev_dbg(rdev_to_dev(rdev), "ABI version requested %d", > @@ -3720,8 +3721,13 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > } > spin_lock_init(&uctx->sh_lock); > > - resp.dev_id = rdev->en_dev->pdev->devfn; /*Temp, Use idr_alloc instead*/ > - resp.max_qp = rdev->qplib_ctx.qpc_count; > + chip_met_rev_num = rdev->chip_ctx->chip_num; > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) << > + BNXT_RE_CHIP_ID0_CHIP_REV_SFT; > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_metal & 0xFF) << > + BNXT_RE_CHIP_ID0_CHIP_MET_SFT; > + resp.chip_id0 = chip_met_rev_num; > + resp.chip_id1 = 0; /* future extension of chip info */ > resp.pg_size = PAGE_SIZE; > resp.cqe_sz = sizeof(struct cq_base); > resp.max_cqd = dev_attr->max_cq_wqes; > diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h > index a7a6111..c62b106 100644 > --- a/include/uapi/rdma/bnxt_re-abi.h > +++ b/include/uapi/rdma/bnxt_re-abi.h > @@ -42,11 +42,14 @@ > > #include <linux/types.h> > > -#define BNXT_RE_ABI_VERSION 1 > +#define BNXT_RE_ABI_VERSION 2 > > +#define BNXT_RE_CHIP_ID0_CHIP_NUM_SFT 0x00 > +#define BNXT_RE_CHIP_ID0_CHIP_REV_SFT 0x10 > +#define BNXT_RE_CHIP_ID0_CHIP_MET_SFT 0x18 > struct bnxt_re_uctx_resp { > - __u32 dev_id; > - __u32 max_qp; > + __u32 chip_id0; > + __u32 chip_id1; I wonder if such changes in UAPI files are allowed. It breaks everyone who compiles against bnxt_re_uctx_resp.dev_id and bnxt_re_uctx_resp.max_qp. It can be union to overcome this naming problem. > __u32 pg_size; > __u32 cqe_sz; > __u32 max_cqd; > -- > 1.8.3.1 >
On Tue, Jan 1, 2019 at 1:21 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Mon, Dec 31, 2018 at 01:10:06AM -0500, Devesh Sharma wrote: > > User space verbs provider library would need chip context. > > Changing the ABI to add chip version details in structure. > > Furthermore, changing the kernel driver ucontext allocation > > code to initializ the abi structure with appropriate values. > > > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > --- > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 12 +++++++++--- > > include/uapi/rdma/bnxt_re-abi.h | 9 ++++++--- > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > index 7a09ca7..3bec789 100644 > > --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > @@ -3693,9 +3693,10 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > > struct ib_udata *udata) > > { > > struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev); > > + struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > > struct bnxt_re_uctx_resp resp; > > struct bnxt_re_ucontext *uctx; > > - struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > > + u32 chip_met_rev_num = 0; > > int rc; > > > > dev_dbg(rdev_to_dev(rdev), "ABI version requested %d", > > @@ -3720,8 +3721,13 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > > } > > spin_lock_init(&uctx->sh_lock); > > > > - resp.dev_id = rdev->en_dev->pdev->devfn; /*Temp, Use idr_alloc instead*/ > > - resp.max_qp = rdev->qplib_ctx.qpc_count; > > + chip_met_rev_num = rdev->chip_ctx->chip_num; > > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) << > > + BNXT_RE_CHIP_ID0_CHIP_REV_SFT; > > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_metal & 0xFF) << > > + BNXT_RE_CHIP_ID0_CHIP_MET_SFT; > > + resp.chip_id0 = chip_met_rev_num; > > + resp.chip_id1 = 0; /* future extension of chip info */ > > resp.pg_size = PAGE_SIZE; > > resp.cqe_sz = sizeof(struct cq_base); > > resp.max_cqd = dev_attr->max_cq_wqes; > > diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h > > index a7a6111..c62b106 100644 > > --- a/include/uapi/rdma/bnxt_re-abi.h > > +++ b/include/uapi/rdma/bnxt_re-abi.h > > @@ -42,11 +42,14 @@ > > > > #include <linux/types.h> > > > > -#define BNXT_RE_ABI_VERSION 1 > > +#define BNXT_RE_ABI_VERSION 2 > > > > +#define BNXT_RE_CHIP_ID0_CHIP_NUM_SFT 0x00 > > +#define BNXT_RE_CHIP_ID0_CHIP_REV_SFT 0x10 > > +#define BNXT_RE_CHIP_ID0_CHIP_MET_SFT 0x18 > > struct bnxt_re_uctx_resp { > > - __u32 dev_id; > > - __u32 max_qp; > > + __u32 chip_id0; > > + __u32 chip_id1; > > I wonder if such changes in UAPI files are allowed. It breaks everyone > who compiles against bnxt_re_uctx_resp.dev_id and bnxt_re_uctx_resp.max_qp. > > It can be union to overcome this naming problem. > Yeah I agree with you this will break previous lib versions. I will take care in a V2. > > __u32 pg_size; > > __u32 cqe_sz; > > __u32 max_cqd; > > -- > > 1.8.3.1 > >
On Mon, Dec 31, 2018 at 01:10:06AM -0500, Devesh Sharma wrote: > User space verbs provider library would need chip context. > Changing the ABI to add chip version details in structure. > Furthermore, changing the kernel driver ucontext allocation > code to initializ the abi structure with appropriate values. > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 12 +++++++++--- > include/uapi/rdma/bnxt_re-abi.h | 9 ++++++--- > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > index 7a09ca7..3bec789 100644 > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > @@ -3693,9 +3693,10 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > struct ib_udata *udata) > { > struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev); > + struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > struct bnxt_re_uctx_resp resp; > struct bnxt_re_ucontext *uctx; > - struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > + u32 chip_met_rev_num = 0; > int rc; > > dev_dbg(rdev_to_dev(rdev), "ABI version requested %d", > @@ -3720,8 +3721,13 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > } > spin_lock_init(&uctx->sh_lock); > > - resp.dev_id = rdev->en_dev->pdev->devfn; /*Temp, Use idr_alloc instead*/ > - resp.max_qp = rdev->qplib_ctx.qpc_count; > + chip_met_rev_num = rdev->chip_ctx->chip_num; > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) << > + BNXT_RE_CHIP_ID0_CHIP_REV_SFT; > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_metal & 0xFF) << > + BNXT_RE_CHIP_ID0_CHIP_MET_SFT; > + resp.chip_id0 = chip_met_rev_num; > + resp.chip_id1 = 0; /* future extension of chip info */ > resp.pg_size = PAGE_SIZE; > resp.cqe_sz = sizeof(struct cq_base); > resp.max_cqd = dev_attr->max_cq_wqes; > diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h > index a7a6111..c62b106 100644 > +++ b/include/uapi/rdma/bnxt_re-abi.h > @@ -42,11 +42,14 @@ > > #include <linux/types.h> > > -#define BNXT_RE_ABI_VERSION 1 > +#define BNXT_RE_ABI_VERSION 2 > > +#define BNXT_RE_CHIP_ID0_CHIP_NUM_SFT 0x00 > +#define BNXT_RE_CHIP_ID0_CHIP_REV_SFT 0x10 > +#define BNXT_RE_CHIP_ID0_CHIP_MET_SFT 0x18 > struct bnxt_re_uctx_resp { > - __u32 dev_id; > - __u32 max_qp; > + __u32 chip_id0; > + __u32 chip_id1; You changed something called max_qp into chip_id1 ? How is that backwards compatible??? Where is the matching rdma-core patch for this change? Jason
On Thu, Jan 3, 2019 at 2:35 AM Jason Gunthorpe <jgg@mellanox.com> wrote: > > On Mon, Dec 31, 2018 at 01:10:06AM -0500, Devesh Sharma wrote: > > User space verbs provider library would need chip context. > > Changing the ABI to add chip version details in structure. > > Furthermore, changing the kernel driver ucontext allocation > > code to initializ the abi structure with appropriate values. > > > > Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> > > drivers/infiniband/hw/bnxt_re/ib_verbs.c | 12 +++++++++--- > > include/uapi/rdma/bnxt_re-abi.h | 9 ++++++--- > > 2 files changed, 15 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > index 7a09ca7..3bec789 100644 > > +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c > > @@ -3693,9 +3693,10 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > > struct ib_udata *udata) > > { > > struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev); > > + struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > > struct bnxt_re_uctx_resp resp; > > struct bnxt_re_ucontext *uctx; > > - struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; > > + u32 chip_met_rev_num = 0; > > int rc; > > > > dev_dbg(rdev_to_dev(rdev), "ABI version requested %d", > > @@ -3720,8 +3721,13 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, > > } > > spin_lock_init(&uctx->sh_lock); > > > > - resp.dev_id = rdev->en_dev->pdev->devfn; /*Temp, Use idr_alloc instead*/ > > - resp.max_qp = rdev->qplib_ctx.qpc_count; > > + chip_met_rev_num = rdev->chip_ctx->chip_num; > > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) << > > + BNXT_RE_CHIP_ID0_CHIP_REV_SFT; > > + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_metal & 0xFF) << > > + BNXT_RE_CHIP_ID0_CHIP_MET_SFT; > > + resp.chip_id0 = chip_met_rev_num; > > + resp.chip_id1 = 0; /* future extension of chip info */ > > resp.pg_size = PAGE_SIZE; > > resp.cqe_sz = sizeof(struct cq_base); > > resp.max_cqd = dev_attr->max_cq_wqes; > > diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h > > index a7a6111..c62b106 100644 > > +++ b/include/uapi/rdma/bnxt_re-abi.h > > @@ -42,11 +42,14 @@ > > > > #include <linux/types.h> > > > > -#define BNXT_RE_ABI_VERSION 1 > > +#define BNXT_RE_ABI_VERSION 2 > > > > +#define BNXT_RE_CHIP_ID0_CHIP_NUM_SFT 0x00 > > +#define BNXT_RE_CHIP_ID0_CHIP_REV_SFT 0x10 > > +#define BNXT_RE_CHIP_ID0_CHIP_MET_SFT 0x18 > > struct bnxt_re_uctx_resp { > > - __u32 dev_id; > > - __u32 max_qp; > > + __u32 chip_id0; > > + __u32 chip_id1; > > You changed something called max_qp into chip_id1 ? How is that > backwards compatible??? Yes, this is breaking indeed as Leon also commented. replacing with union and ABI range check will be implemented in V2. > > Where is the matching rdma-core patch for this change? Slightly lagging behind, That will be posted along with V2. > > Jason
On Thu, Jan 03, 2019 at 11:35:43AM +0530, Devesh Sharma wrote: > > > struct bnxt_re_uctx_resp { > > > - __u32 dev_id; > > > - __u32 max_qp; > > > + __u32 chip_id0; > > > + __u32 chip_id1; > > > > You changed something called max_qp into chip_id1 ? How is that > > backwards compatible??? > Yes, this is breaking indeed as Leon also commented. replacing with > union and ABI range check > will be implemented in V2. I'm not sure how a union will save you - add the new stuff to the end is the usual approach. Jason
On Thu, Jan 03, 2019 at 03:48:05PM +0000, Jason Gunthorpe wrote: > On Thu, Jan 03, 2019 at 11:35:43AM +0530, Devesh Sharma wrote: > > > > > struct bnxt_re_uctx_resp { > > > > - __u32 dev_id; > > > > - __u32 max_qp; > > > > + __u32 chip_id0; > > > > + __u32 chip_id1; > > > > > > You changed something called max_qp into chip_id1 ? How is that > > > backwards compatible??? > > Yes, this is breaking indeed as Leon also commented. replacing with > > union and ABI range check > > will be implemented in V2. > > I'm not sure how a union will save you - add the new stuff to the end > is the usual approach. Why won't union work? struct bnxt_re_uctx_resp { union { __u32 dev_id; __u32 chip_id0; } union { __u32 max_qp; __u32 chip_id1; } .... AND ABI_VERSION is define, so backward compilation will work too #ifdef ABI_VERSION < 2 dev_id #else chip_id0 #endif Thanks
On Thu, Jan 3, 2019 at 9:37 PM Leon Romanovsky <leon@kernel.org> wrote: > > On Thu, Jan 03, 2019 at 03:48:05PM +0000, Jason Gunthorpe wrote: > > On Thu, Jan 03, 2019 at 11:35:43AM +0530, Devesh Sharma wrote: > > > > > > > struct bnxt_re_uctx_resp { > > > > > - __u32 dev_id; > > > > > - __u32 max_qp; > > > > > + __u32 chip_id0; > > > > > + __u32 chip_id1; > > > > > > > > You changed something called max_qp into chip_id1 ? How is that > > > > backwards compatible??? > > > Yes, this is breaking indeed as Leon also commented. replacing with > > > union and ABI range check > > > will be implemented in V2. > > > > I'm not sure how a union will save you - add the new stuff to the end > > is the usual approach. > > Why won't union work? > > struct bnxt_re_uctx_resp { > union { > __u32 dev_id; > __u32 chip_id0; > } > union { > __u32 max_qp; > __u32 chip_id1; > } > .... > > AND ABI_VERSION is define, so backward compilation will work too > > #ifdef ABI_VERSION < 2 > dev_id > #else > chip_id0 > #endif > Yeah, I guess Leon's suggestion would work, unless union is not looking shabby. > Thanks
On Thu, Jan 03, 2019 at 06:07:18PM +0200, Leon Romanovsky wrote: > On Thu, Jan 03, 2019 at 03:48:05PM +0000, Jason Gunthorpe wrote: > > On Thu, Jan 03, 2019 at 11:35:43AM +0530, Devesh Sharma wrote: > > > > > > > struct bnxt_re_uctx_resp { > > > > > - __u32 dev_id; > > > > > - __u32 max_qp; > > > > > + __u32 chip_id0; > > > > > + __u32 chip_id1; > > > > > > > > You changed something called max_qp into chip_id1 ? How is that > > > > backwards compatible??? > > > Yes, this is breaking indeed as Leon also commented. replacing with > > > union and ABI range check > > > will be implemented in V2. > > > > I'm not sure how a union will save you - add the new stuff to the end > > is the usual approach. > > Why won't union work? > > struct bnxt_re_uctx_resp { > union { > __u32 dev_id; > __u32 chip_id0; > } > union { > __u32 max_qp; > __u32 chip_id1; > } > .... > > AND ABI_VERSION is define, so backward compilation will work too > > #ifdef ABI_VERSION < 2 We've never approached ABI_VERSION like this - if you crank ABI version you throw away source compilation compatability too. The issue here is ABI compatability, today's rdma-core provider expectx the bytes for max_qp to be max_qp, you can't just change it to be dev_id without breaking everything. ABI_VERSION should only be used in distress - add the new data to the end of the struct and preserve the existing fields. Jason
diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c index 7a09ca7..3bec789 100644 --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c @@ -3693,9 +3693,10 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, struct ib_udata *udata) { struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev); + struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; struct bnxt_re_uctx_resp resp; struct bnxt_re_ucontext *uctx; - struct bnxt_qplib_dev_attr *dev_attr = &rdev->dev_attr; + u32 chip_met_rev_num = 0; int rc; dev_dbg(rdev_to_dev(rdev), "ABI version requested %d", @@ -3720,8 +3721,13 @@ struct ib_ucontext *bnxt_re_alloc_ucontext(struct ib_device *ibdev, } spin_lock_init(&uctx->sh_lock); - resp.dev_id = rdev->en_dev->pdev->devfn; /*Temp, Use idr_alloc instead*/ - resp.max_qp = rdev->qplib_ctx.qpc_count; + chip_met_rev_num = rdev->chip_ctx->chip_num; + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) << + BNXT_RE_CHIP_ID0_CHIP_REV_SFT; + chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_metal & 0xFF) << + BNXT_RE_CHIP_ID0_CHIP_MET_SFT; + resp.chip_id0 = chip_met_rev_num; + resp.chip_id1 = 0; /* future extension of chip info */ resp.pg_size = PAGE_SIZE; resp.cqe_sz = sizeof(struct cq_base); resp.max_cqd = dev_attr->max_cq_wqes; diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h index a7a6111..c62b106 100644 --- a/include/uapi/rdma/bnxt_re-abi.h +++ b/include/uapi/rdma/bnxt_re-abi.h @@ -42,11 +42,14 @@ #include <linux/types.h> -#define BNXT_RE_ABI_VERSION 1 +#define BNXT_RE_ABI_VERSION 2 +#define BNXT_RE_CHIP_ID0_CHIP_NUM_SFT 0x00 +#define BNXT_RE_CHIP_ID0_CHIP_REV_SFT 0x10 +#define BNXT_RE_CHIP_ID0_CHIP_MET_SFT 0x18 struct bnxt_re_uctx_resp { - __u32 dev_id; - __u32 max_qp; + __u32 chip_id0; + __u32 chip_id1; __u32 pg_size; __u32 cqe_sz; __u32 max_cqd;
User space verbs provider library would need chip context. Changing the ABI to add chip version details in structure. Furthermore, changing the kernel driver ucontext allocation code to initializ the abi structure with appropriate values. Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com> --- drivers/infiniband/hw/bnxt_re/ib_verbs.c | 12 +++++++++--- include/uapi/rdma/bnxt_re-abi.h | 9 ++++++--- 2 files changed, 15 insertions(+), 6 deletions(-)