On Mon, Feb 04, 2019 at 03:56:21PM +0200, Leon Romanovsky wrote:
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 0aeb53ef7869..a48bb388160d 100644
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -224,13 +224,19 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
> if (ret)
> goto err;
>
> - ucontext = ib_dev->ops.alloc_ucontext(ib_dev, &attrs->driver_udata);
> - if (IS_ERR(ucontext)) {
> - ret = PTR_ERR(ucontext);
> + ucontext = rdma_zalloc_drv_obj(ib_dev, ib_ucontext);
> + if (!ucontext) {
> + ret = -ENOMEM;
> goto err_alloc;
> }
>
> + rdma_rt_set_type(&ucontext->res, RDMA_RESTRACK_CTX);
> ucontext->device = ib_dev;
> +
> + ret = ib_dev->ops.alloc_ucontext(ucontext, &attrs->driver_udata);
> + if (ret)
> + goto err_free;
> +
> ucontext->cg_obj = cg_obj;
> /* ufile is required when some objects are released */
> ucontext->ufile = file;
> @@ -262,7 +268,6 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
>
> fd_install(resp.async_fd, filp);
>
> - rdma_rt_set_type(&ucontext->res, RDMA_RESTRACK_CTX);
> rdma_restrack_uadd(&ucontext->res);
>
> /*
> @@ -283,7 +288,7 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
> put_unused_fd(resp.async_fd);
>
> err_free:
> - ib_dev->ops.dealloc_ucontext(ucontext);
> + kfree(ucontext);
This error unwind isn't right any more.
Re-organize it to have the driver call at the end when failure still
works on the existing goto unwind
> @@ -565,6 +556,7 @@ static const struct ib_device_ops c4iw_dev_ops = {
> .reg_user_mr = c4iw_reg_user_mr,
> .req_notify_cq = c4iw_arm_cq,
> INIT_RDMA_OBJ_SIZE(ib_pd, c4iw_pd, ibpd),
> + INIT_RDMA_OBJ_SIZE(ib_ucontext, c4iw_ucontext, ibucontext),
> };
>
> void c4iw_register_device(struct work_struct *work)
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c b/drivers/infiniband/hw/cxgb4/qp.c
> index 0fe87b9c1e10..5da85f7679d8 100644
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2338,7 +2338,6 @@ struct ib_qp *c4iw_create_qp(struct ib_pd *pd, struct ib_qp_init_attr *attrs,
> insert_mmap(ucontext, ma_sync_key_mm);
> }
>
> - c4iw_get_ucontext(ucontext);
> qhp->ucontext = ucontext;
Why? Looks wrong...
Actually this whole cxgb4 stuff looks very peculiar - I don't see how
we can safely move the kfree out of its kref release and into the core
code and be sure it will still work...
Arguably drivers should not be refcounting these structs though - I
wonder if the ref counting could be deleted?
Steve? Is there some reason cxgb4 is the only driver trying to stick a
refcount on the ucontext?
ucontext deletion means that every object contained in the ucontext
has been deleted, the core code guarentees this.
If there is some async work someplace then generally it should be
flushed/canceled during ucontext delete, not managed via kref...
ie there is no reason for that mmap stuff to be under the kref
release, c4iw_mmap will not be called once the core destroyes the
ucontext.
Fixing this needs another patch..
> /**
> * i40iw_dealloc_ucontext - deallocate the user context data structure
> * @context: user context created during alloc
> */
> -static int i40iw_dealloc_ucontext(struct ib_ucontext *context)
> +static void i40iw_dealloc_ucontext(struct ib_ucontext *context)
> {
> struct i40iw_ucontext *ucontext = to_ucontext(context);
> - unsigned long flags;
> -
> - spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> - if (!list_empty(&ucontext->cq_reg_mem_list)) {
> - spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> - return -EBUSY;
> - }
> - spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> - spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
> - if (!list_empty(&ucontext->qp_reg_mem_list)) {
> - spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
> - return -EBUSY;
> - }
> - spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
>
> - kfree(ucontext);
> - return 0;
> + /*
> + * dealloc called before all MRs QPs were freed,
> + * no need to grab XXX_reg_mem_list_lock locks,
> + * it is too lat and the warning below symbol of bug
> + * in th driver.
> + */
various spelling errors
> diff --git a/drivers/infiniband/hw/nes/nes_verbs.h b/drivers/infiniband/hw/nes/nes_verbs.h
> index e02a5662dc20..114a9b59fefd 100644
> +++ b/drivers/infiniband/hw/nes/nes_verbs.h
> @@ -59,7 +59,6 @@ struct nes_ucontext {
> struct list_head cq_reg_mem_list;
> struct list_head qp_reg_mem_list;
> u32 mcrqf;
> - atomic_t usecnt;
Lets have this in its own patch please
> diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> index f44220f72e05..196d7a6e4c8c 100644
> +++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
> @@ -306,41 +306,33 @@ int pvrdma_modify_port(struct ib_device *ibdev, u8 port, int mask,
>
> /**
> * pvrdma_alloc_ucontext - allocate ucontext
> - * @ibdev: the IB device
> + * @uctx: the uverbs countext
> * @udata: user data
> *
> - * @return: the ib_ucontext pointer on success, otherwise errno.
> + * @return: zero on success, otherwise errno.
> */
> -struct ib_ucontext *pvrdma_alloc_ucontext(struct ib_device *ibdev,
> - struct ib_udata *udata)
> +int vrdma_alloc_ucontext(struct ib_ucontext *uctx, struct ib_udata *udata)
Wrong name
Jason
