On Thu, Jan 17, 2019 at 09:11:12PM +0200, Shamir Rabinovitch wrote:
> Now when we have the udata passed to all the ib_xxx object creation APIs
> and the additional function 'rdma_get_ucontext' to get the ib_ucontext
> from ib_udata stored in uverbs_attr_bundle, we can finally start to remove
> the dependency of the drivers in the ib_xxx->uobject->context.
>
> Signed-off-by: Shamir Rabinovitch <shamir.rabinovitch@xxxxxxxxxx>
> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 33 +++++--
> drivers/infiniband/hw/cxgb3/iwch_provider.c | 4 +-
> drivers/infiniband/hw/cxgb4/qp.c | 8 +-
> drivers/infiniband/hw/hns/hns_roce_qp.c | 15 +++-
> drivers/infiniband/hw/i40iw/i40iw_verbs.c | 15 +++-
> drivers/infiniband/hw/mlx4/doorbell.c | 6 ++
> drivers/infiniband/hw/mlx4/mr.c | 9 +-
> drivers/infiniband/hw/mlx4/qp.c | 92 +++++++++++++-------
> drivers/infiniband/hw/mlx4/srq.c | 13 ++-
> drivers/infiniband/hw/mlx5/qp.c | 68 +++++++++++----
> drivers/infiniband/hw/mlx5/srq.c | 13 ++-
> drivers/infiniband/hw/mthca/mthca_provider.c | 28 ++++--
> drivers/infiniband/hw/mthca/mthca_qp.c | 19 ++--
> drivers/infiniband/hw/mthca/mthca_srq.c | 25 ++++--
> drivers/infiniband/hw/nes/nes_verbs.c | 13 ++-
> drivers/infiniband/hw/qedr/verbs.c | 8 +-
> drivers/infiniband/hw/usnic/usnic_ib_verbs.c | 7 +-
> drivers/infiniband/sw/rdmavt/qp.c | 10 ++-
> drivers/infiniband/sw/rdmavt/srq.c | 10 ++-
> drivers/infiniband/sw/rxe/rxe_qp.c | 5 +-
> drivers/infiniband/sw/rxe/rxe_verbs.c | 5 +-
> 21 files changed, 287 insertions(+), 119 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 9bc637e49faa..d811efe49e77 100644
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -733,11 +733,16 @@ struct ib_ah *bnxt_re_create_ah(struct ib_pd *ib_pd,
>
> /* Write AVID to shared page. */
> if (udata) {
> - struct ib_ucontext *ib_uctx = ib_pd->uobject->context;
> + struct ib_ucontext *ib_uctx;
> struct bnxt_re_ucontext *uctx;
> unsigned long flag;
> u32 *wrptr;
>
> + ib_uctx = rdma_get_ucontext(udata);
> + if (IS_ERR(ib_uctx)) {
> + rc = PTR_ERR(ib_uctx);
> + goto fail;
> + }
Hurm.. I think if you are going to use this API so widely then we need
to arrange for it to not fail. If udata is not null, and we are in a
driver callback with an object, then the ucontext should be guaranteed
available.
Otherwise we have weird cases where udata and !rdma_get_ucontext() can
be true, which is really horrible to reason about..
The way to do this is to have the core code places that have a uobject
copy the ucontext from the uobject into the udata, I think I mentioned
this before, and it is a fixme in the code..
For instance something like this would take care of things for the
ioctl path:
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 8c81ff69805276..9e9240645fbe9b 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -198,6 +198,7 @@ static int uverbs_process_idrs_array(struct bundle_priv *pbundle,
ret = PTR_ERR(attr->uobjects[i]);
break;
}
+ pbundle->bundle.ucontext = attr->ubojects[i].ucontext;
}
attr->len = i;
@@ -316,6 +317,7 @@ static int uverbs_process_attr(struct bundle_priv *pbundle,
if (IS_ERR(o_attr->uobject))
return PTR_ERR(o_attr->uobject);
__set_bit(attr_bkey, pbundle->uobj_finalize);
+ pbundle->bundle.ucontext = o_attr->uoboject.ucontext;
if (spec->u.obj.access == UVERBS_ACCESS_NEW) {
unsigned int uattr_idx = uattr - pbundle->uattrs;
And the cmd path needs some editing of its alloc/get macros
> uctx = container_of(ib_uctx, struct bnxt_re_ucontext, ib_uctx);
> spin_lock_irqsave(&uctx->sh_lock, flag);
> wrptr = (u32 *)(uctx->shpg + BNXT_RE_AVID_OFFT);
> @@ -883,10 +888,15 @@ static int bnxt_re_init_user_qp(struct bnxt_re_dev *rdev, struct bnxt_re_pd *pd,
> struct bnxt_qplib_qp *qplib_qp = &qp->qplib_qp;
> struct ib_umem *umem;
> int bytes = 0;
> - struct ib_ucontext *context = pd->ib_pd.uobject->context;
> - struct bnxt_re_ucontext *cntx = container_of(context,
> - struct bnxt_re_ucontext,
> - ib_uctx);
> + struct bnxt_re_ucontext *cntx;
> + struct ib_ucontext *context;
> +
> + context = rdma_get_ucontext(udata);
> + if (IS_ERR(context))
> + return PTR_ERR(context);
Like here, this is getting really long..
And this should probably follow the pattern of the new
rdma_device_to_drv_device() to make it simpler..
Just add something like this at the start of functions:
struct bnxt_re_ucontext *cntx = rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
returns NULL if kernel mode.
And then use 'if (cntx)' instead of 'if (udata)' in that function to
test for user/kernel mode..
Jason
