| Message ID | 1610634408-31356-1-git-send-email-bryantan@vmware.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-rc] RDMA/vmw_pvrdma: Fix network_hdr_type reported in WC | expand |
On Thu, Jan 14, 2021 at 06:26:48AM -0800, Bryan Tan wrote: > The PVRDMA device defines network_hdr_type according to an old > definition of the rdma_network_type enum that has since changed, > resulting in the wrong rdma_network_type being reported. Fix this by > explicitly defining the enum used by the PVRDMA device and adding a > function to convert the pvrdma_network_type to rdma_network_type enum. How come I can't find anything reading this in rdma-core? $ ~/oss/rdma-core#git grep network_hdr_type kernel-headers/rdma/vmw_pvrdma-abi.h: __u8 network_hdr_type; ?? > Fixes: 1c15b4f2a42f ("RDMA/core: Modify enum ib_gid_type and enum rdma_network_type") > Signed-off-by: Bryan Tan <bryantan@vmware.com> Add Cc: stable@vger.kernel.org # 5.10+ > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > index 86a6c054ea26..637d33944f95 100644 > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > @@ -201,6 +201,13 @@ enum pvrdma_device_mode { > PVRDMA_DEVICE_MODE_IB, /* InfiniBand. */ > }; > > +enum pvrdma_network_type { > + PVRDMA_NETWORK_IB, > + PVRDMA_NETWORK_ROCE_V1 = PVRDMA_NETWORK_IB, > + PVRDMA_NETWORK_IPV4, > + PVRDMA_NETWORK_IPV6 > +}; This is in the wrong place, uapi data needs to be in includ/ulp/rdma/vmw_pvrdma-abi.h Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, January 15, 2021 1:24 AM > On Thu, Jan 14, 2021 at 06:26:48AM -0800, Bryan Tan wrote: > > The PVRDMA device defines network_hdr_type according to an old > > definition of the rdma_network_type enum that has since changed, > > resulting in the wrong rdma_network_type being reported. Fix this by > > explicitly defining the enum used by the PVRDMA device and adding a > > function to convert the pvrdma_network_type to rdma_network_type enum. > > How come I can't find anything reading this in rdma-core? > > $ ~/oss/rdma-core#git grep network_hdr_type > kernel-headers/rdma/vmw_pvrdma-abi.h: __u8 network_hdr_type; > > ?? network_hdr_type isn't exposed in the userspace WC ibv_wc. Given that the field is only in the kernel side, it didn't seem like we should add the new enum to vmw_pvrdma-abi.h either. > > Fixes: 1c15b4f2a42f ("RDMA/core: Modify enum ib_gid_type and enum rdma_network_type") > > Signed-off-by: Bryan Tan <bryantan@vmware.com> > > Add Cc: stable@vger.kernel.org # 5.10+ Will do. > > diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > index 86a6c054ea26..637d33944f95 100644 > > --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h > > @@ -201,6 +201,13 @@ enum pvrdma_device_mode { > > PVRDMA_DEVICE_MODE_IB, /* InfiniBand. */ > > }; > > > > +enum pvrdma_network_type { > > + PVRDMA_NETWORK_IB, > > + PVRDMA_NETWORK_ROCE_V1 = PVRDMA_NETWORK_IB, > > + PVRDMA_NETWORK_IPV4, > > + PVRDMA_NETWORK_IPV6 > > +}; > > This is in the wrong place, uapi data needs to be in > includ/ulp/rdma/vmw_pvrdma-abi.h Please see above. Thanks, Bryan
On Fri, Jan 15, 2021 at 04:58:58AM +0000, Bryan Tan wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, January 15, 2021 1:24 AM > > On Thu, Jan 14, 2021 at 06:26:48AM -0800, Bryan Tan wrote: > > > The PVRDMA device defines network_hdr_type according to an old > > > definition of the rdma_network_type enum that has since changed, > > > resulting in the wrong rdma_network_type being reported. Fix this by > > > explicitly defining the enum used by the PVRDMA device and adding a > > > function to convert the pvrdma_network_type to rdma_network_type enum. > > > > How come I can't find anything reading this in rdma-core? > > > > $ ~/oss/rdma-core#git grep network_hdr_type > > kernel-headers/rdma/vmw_pvrdma-abi.h: __u8 network_hdr_type; > > > > ?? > > network_hdr_type isn't exposed in the userspace WC ibv_wc. So this is "HW" API then? > Given that the field is only in the kernel side, it didn't seem like > we should add the new enum to vmw_pvrdma-abi.h either. Well, the struct that holds the value is in a uapi header, so the definition should be too. If you are defining HW data in uapi then may as well define all of it. Jason
From: Jason Gunthorpe <jgg@nvidia.com> Sent: Friday, January 15, 2021 8:51 PM > On Fri, Jan 15, 2021 at 04:58:58AM +0000, Bryan Tan wrote: > > > From: Jason Gunthorpe <jgg@nvidia.com> > > > Sent: Friday, January 15, 2021 1:24 AM > > > On Thu, Jan 14, 2021 at 06:26:48AM -0800, Bryan Tan wrote: > > > > The PVRDMA device defines network_hdr_type according to an old > > > > definition of the rdma_network_type enum that has since changed, > > > > resulting in the wrong rdma_network_type being reported. Fix this by > > > > explicitly defining the enum used by the PVRDMA device and adding a > > > > function to convert the pvrdma_network_type to rdma_network_type enum. > > > > > > How come I can't find anything reading this in rdma-core? > > > > > > $ ~/oss/rdma-core#git grep network_hdr_type > > > kernel-headers/rdma/vmw_pvrdma-abi.h: __u8 network_hdr_type; > > > > > > ?? > > > > network_hdr_type isn't exposed in the userspace WC ibv_wc. > > So this is "HW" API then? > <snip> > Well, the struct that holds the value is in a uapi header, so the > definition should be too. If you are defining HW data in uapi then may > as well define all of it. Yes, the pvrdma_cqe struct is populated by the HW. Both the driver and the userspace library use this struct to populate the corresponding WCs. That makes sense, let me move them into the uapi header. Apologies if this is answered somewhere, couldn't find any info--am I responsible for updating the header in rdma-core then? Bryan
On Mon, Jan 18, 2021 at 11:46:28AM +0000, Bryan Tan wrote: > Yes, the pvrdma_cqe struct is populated by the HW. Both the driver and > the userspace library use this struct to populate the corresponding WCs. > That makes sense, let me move them into the uapi header. Apologies if > this is answered somewhere, couldn't find any info--am I responsible for > updating the header in rdma-core then? It happens automatically unless you need to send a related rdma-core github PR, then you should do it in the PR Jason
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h index c142f5e7f25f..de57f2fed743 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h @@ -509,6 +509,20 @@ static inline int ib_send_flags_to_pvrdma(int flags) return flags & PVRDMA_MASK(PVRDMA_SEND_FLAGS_MAX); } +static inline int pvrdma_network_type_to_ib(enum pvrdma_network_type type) +{ + switch (type) { + case PVRDMA_NETWORK_ROCE_V1: + return RDMA_NETWORK_ROCE_V1; + case PVRDMA_NETWORK_IPV4: + return RDMA_NETWORK_IPV4; + case PVRDMA_NETWORK_IPV6: + return RDMA_NETWORK_IPV6; + default: + return RDMA_NETWORK_IPV6; + } +} + void pvrdma_qp_cap_to_ib(struct ib_qp_cap *dst, const struct pvrdma_qp_cap *src); void ib_qp_cap_to_pvrdma(struct pvrdma_qp_cap *dst, diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c index a119ac3e103c..6aa40bd2fd52 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c @@ -367,7 +367,7 @@ static int pvrdma_poll_one(struct pvrdma_cq *cq, struct pvrdma_qp **cur_qp, wc->dlid_path_bits = cqe->dlid_path_bits; wc->port_num = cqe->port_num; wc->vendor_err = cqe->vendor_err; - wc->network_hdr_type = cqe->network_hdr_type; + wc->network_hdr_type = pvrdma_network_type_to_ib(cqe->network_hdr_type); /* Update shared ring state */ pvrdma_idx_ring_inc(&cq->ring_state->rx.cons_head, cq->ibcq.cqe); diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h index 86a6c054ea26..637d33944f95 100644 --- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h @@ -201,6 +201,13 @@ enum pvrdma_device_mode { PVRDMA_DEVICE_MODE_IB, /* InfiniBand. */ }; +enum pvrdma_network_type { + PVRDMA_NETWORK_IB, + PVRDMA_NETWORK_ROCE_V1 = PVRDMA_NETWORK_IB, + PVRDMA_NETWORK_IPV4, + PVRDMA_NETWORK_IPV6 +}; + struct pvrdma_gos_info { u32 gos_bits:2; /* W: PVRDMA_GOS_BITS_ */ u32 gos_type:4; /* W: PVRDMA_GOS_TYPE_ */
The PVRDMA device defines network_hdr_type according to an old definition of the rdma_network_type enum that has since changed, resulting in the wrong rdma_network_type being reported. Fix this by explicitly defining the enum used by the PVRDMA device and adding a function to convert the pvrdma_network_type to rdma_network_type enum. Fixes: 1c15b4f2a42f ("RDMA/core: Modify enum ib_gid_type and enum rdma_network_type") Signed-off-by: Bryan Tan <bryantan@vmware.com> --- drivers/infiniband/hw/vmw_pvrdma/pvrdma.h | 14 ++++++++++++++ drivers/infiniband/hw/vmw_pvrdma/pvrdma_cq.c | 2 +- drivers/infiniband/hw/vmw_pvrdma/pvrdma_dev_api.h | 7 +++++++ 3 files changed, 22 insertions(+), 1 deletion(-)