| Message ID | 20181105113528.8317-19-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA: Add support for ib_device_ops | expand |
On 11/5/2018 6:35 AM, Kamal Heib wrote: > The create_ah verb receive as parameter an udata buffer, this parameter > is missing in rvt_create_ah - fixing that. Please add a bit more to the commit message here. This implies it is fixing a bug but there is no bug per se. rdmavt has no need for udata buffer. If perhaps it is needed because of the on-coming patches, you should probably say this. -Denny
On Mon, Nov 05, 2018 at 03:18:02PM -0500, Dennis Dalessandro wrote: > On 11/5/2018 6:35 AM, Kamal Heib wrote: > > The create_ah verb receive as parameter an udata buffer, this parameter > > is missing in rvt_create_ah - fixing that. > > Please add a bit more to the commit message here. This implies it is fixing > a bug but there is no bug per se. rdmavt has no need for udata buffer. If > perhaps it is needed because of the on-coming patches, you should probably > say this. > > -Denny Actually, I think that this is a bug that was found while I was doing the changes to support the ib_device_ops in rdmavt. Before my changes the initialization of the ib_device verbs in rdmavt was done in the check_driver_override() function: static inline int check_driver_override(struct rvt_dev_info *rdi, size_t offset, void *func) { if (!*(void **)((void *)&rdi->ibdev + offset)) { *(void **)((void *)&rdi->ibdev + offset) = func; return 0; } return 1; } So, when initialize the create_ah verbs from check_support() using the following call, there is no guaranty that create_ah() and rvt_create_ah() have the same prototype..., that's why this bug was not found during compilation time. case CREATE_AH: check_driver_override(rdi, offsetof(struct ib_device, create_ah), rvt_create_ah); break; Thanks, Kamal
On Mon, Nov 05, 2018 at 11:20:55PM +0200, Kamal Heib wrote: > On Mon, Nov 05, 2018 at 03:18:02PM -0500, Dennis Dalessandro wrote: > > On 11/5/2018 6:35 AM, Kamal Heib wrote: > > > The create_ah verb receive as parameter an udata buffer, this parameter > > > is missing in rvt_create_ah - fixing that. > > > > Please add a bit more to the commit message here. This implies it is fixing > > a bug but there is no bug per se. rdmavt has no need for udata buffer. If > > perhaps it is needed because of the on-coming patches, you should probably > > say this. > > > > -Denny > > Actually, I think that this is a bug that was found while I was doing the > changes to support the ib_device_ops in rdmavt. Before my changes the > initialization of the ib_device verbs in rdmavt was done in the > check_driver_override() function: > > static inline int check_driver_override(struct rvt_dev_info *rdi, > size_t offset, void *func) > { > if (!*(void **)((void *)&rdi->ibdev + offset)) { > *(void **)((void *)&rdi->ibdev + offset) = func; > return 0; > } > > return 1; > } > > So, when initialize the create_ah verbs from check_support() using the following > call, there is no guaranty that create_ah() and rvt_create_ah() have the same > prototype..., that's why this bug was not found during compilation time. > > case CREATE_AH: > check_driver_override(rdi, offsetof(struct ib_device, > create_ah), > rvt_create_ah); > break; *BARF* This whole mess must go away as soon as possible. This patch, with a better commit message, should be stand alone and go right away. Casting function pointers to different sigantures is a very bad idea. Jason
On Tue, Nov 06, 2018 at 08:40:24AM -0700, Jason Gunthorpe wrote: > On Mon, Nov 05, 2018 at 11:20:55PM +0200, Kamal Heib wrote: > > On Mon, Nov 05, 2018 at 03:18:02PM -0500, Dennis Dalessandro wrote: > > > On 11/5/2018 6:35 AM, Kamal Heib wrote: > > > > The create_ah verb receive as parameter an udata buffer, this parameter > > > > is missing in rvt_create_ah - fixing that. > > > > > > Please add a bit more to the commit message here. This implies it is fixing > > > a bug but there is no bug per se. rdmavt has no need for udata buffer. If > > > perhaps it is needed because of the on-coming patches, you should probably > > > say this. > > > > > > -Denny > > > > Actually, I think that this is a bug that was found while I was doing the > > changes to support the ib_device_ops in rdmavt. Before my changes the > > initialization of the ib_device verbs in rdmavt was done in the > > check_driver_override() function: > > > > static inline int check_driver_override(struct rvt_dev_info *rdi, > > size_t offset, void *func) > > { > > if (!*(void **)((void *)&rdi->ibdev + offset)) { > > *(void **)((void *)&rdi->ibdev + offset) = func; > > return 0; > > } > > > > return 1; > > } > > > > So, when initialize the create_ah verbs from check_support() using the following > > call, there is no guaranty that create_ah() and rvt_create_ah() have the same > > prototype..., that's why this bug was not found during compilation time. > > > > case CREATE_AH: > > check_driver_override(rdi, offsetof(struct ib_device, > > create_ah), > > rvt_create_ah); > > break; > > *BARF* This whole mess must go away as soon as possible. > This will go away as part of the upcoming patch that adds the support for ib_device_ops. > This patch, with a better commit message, should be stand alone and go > right away. Casting function pointers to different sigantures is a > very bad idea. > OK, I'll improve the commit message and send this patch. > Jason Thanks, Kamal
diff --git a/drivers/infiniband/sw/rdmavt/ah.c b/drivers/infiniband/sw/rdmavt/ah.c index 89ec0f64abfc..084bb4baebb5 100644 --- a/drivers/infiniband/sw/rdmavt/ah.c +++ b/drivers/infiniband/sw/rdmavt/ah.c @@ -91,13 +91,15 @@ EXPORT_SYMBOL(rvt_check_ah); * rvt_create_ah - create an address handle * @pd: the protection domain * @ah_attr: the attributes of the AH + * @udata: pointer to user's input output buffer information. * * This may be called from interrupt context. * * Return: newly allocated ah */ struct ib_ah *rvt_create_ah(struct ib_pd *pd, - struct rdma_ah_attr *ah_attr) + struct rdma_ah_attr *ah_attr, + struct ib_udata *udata) { struct rvt_ah *ah; struct rvt_dev_info *dev = ib_to_rvt(pd->device); diff --git a/drivers/infiniband/sw/rdmavt/ah.h b/drivers/infiniband/sw/rdmavt/ah.h index 16105af99189..25271b48a683 100644 --- a/drivers/infiniband/sw/rdmavt/ah.h +++ b/drivers/infiniband/sw/rdmavt/ah.h @@ -51,7 +51,8 @@ #include <rdma/rdma_vt.h> struct ib_ah *rvt_create_ah(struct ib_pd *pd, - struct rdma_ah_attr *ah_attr); + struct rdma_ah_attr *ah_attr, + struct ib_udata *udata); int rvt_destroy_ah(struct ib_ah *ibah); int rvt_modify_ah(struct ib_ah *ibah, struct rdma_ah_attr *ah_attr); int rvt_query_ah(struct ib_ah *ibah, struct rdma_ah_attr *ah_attr);
The create_ah verb receive as parameter an udata buffer, this parameter is missing in rvt_create_ah - fixing that. Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/sw/rdmavt/ah.c | 4 +++- drivers/infiniband/sw/rdmavt/ah.h | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-)