On 12-Feb-19 12:33, Leon Romanovsky wrote:
> On Tue, Feb 12, 2019 at 11:16:17AM +0200, Gal Pressman wrote:
>> On 11-Feb-19 20:21, Leon Romanovsky wrote:
>>> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
>>> index 5ac143f22df0..08385fb71c7f 100644
>>> --- a/drivers/infiniband/core/uverbs_cmd.c
>>> +++ 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;
>>> }
>>>
>>> + ucontext->res.type = 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;
>>> @@ -240,10 +246,6 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
>>>
>>> mutex_init(&ucontext->per_mm_list_lock);
>>> INIT_LIST_HEAD(&ucontext->per_mm_list);
>>> - if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_ON_DEMAND_PAGING))
>>> - ucontext->invalidate_range = NULL;
>>> -
>>> - resp.num_comp_vectors = file->device->num_comp_vectors;
>>>
>>> ret = get_unused_fd_flags(O_CLOEXEC);
>>> if (ret < 0)
>>> @@ -256,15 +258,23 @@ static int ib_uverbs_get_context(struct uverbs_attr_bundle *attrs)
>>> goto err_fd;
>>> }
>>>
>>> + resp.num_comp_vectors = file->device->num_comp_vectors;
>>> + resp.async_fd = ret;
>>
>> Where did this come from?
>
> The idea is to initialize as much as possible, before calling to drivers.
What I meant is, other lines of code just moved within the function. I can't see
a removal of resp.async_fd assignment, only addition. Did you remove the
original assignment?
>
>>
>>> +
>>> ret = uverbs_response(attrs, &resp, sizeof(resp));
>>> if (ret)
>>> goto err_file;
>>>
>>> - fd_install(resp.async_fd, filp);
>>> + ret = ib_dev->ops.alloc_ucontext(ucontext, &attrs->driver_udata);
>>
>> Why is alloc_ucontext called twice?
>
> Mistake in rebase, there is no need in first alloc_ucontext().
>
>>
>>> + if (ret)
>>> + goto err_file;
>>> + if (!(ib_dev->attrs.device_cap_flags & IB_DEVICE_ON_DEMAND_PAGING))
>>> + ucontext->invalidate_range = NULL;
>>>
>>> - ucontext->res.type = RDMA_RESTRACK_CTX;
>>> rdma_restrack_uadd(&ucontext->res);
>>>
>>> + fd_install(resp.async_fd, filp);
>>> +
>>> /*
>>> * Make sure that ib_uverbs_get_ucontext() sees the pointer update
>>> * only after all writes to setup the ucontext have completed
>>> @@ -283,7 +293,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);
>>>
>>> err_alloc:
>>> ib_rdmacg_uncharge(&cg_obj, ib_dev, RDMACG_RESOURCE_HCA_HANDLE);
