On Thu, Oct 17, 2013 at 6:05 PM, Yann Droneaud <[email protected]> wrote:
> Hi,
>
> Le 17.10.2013 16:41, Matan Barak a écrit :
>
>> On 14/10/2013 6:38 PM, Matan Barak wrote:
>>>
>>> On 11/10/2013 8:57 PM, Yann Droneaud wrote:
>>>>
>>>> Commit 400dbc9 added an infrastructure for extensible uverbs
>>>> commands while later commit 436f2ad exported
>>>> ib_create_flow()/ib_destroy_flow()
>>>> functions using this new infrastructure.
>>>>
>
>>>> diff --git a/drivers/infiniband/core/uverbs_main.c
>>>> b/drivers/infiniband/core/uverbs_main.c
>>>> index 75ad86c..4f00654 100644
>>>> --- a/drivers/infiniband/core/uverbs_main.c
>>>> +++ b/drivers/infiniband/core/uverbs_main.c
>>>> @@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
>>>> @@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
>>>> *filp, const char __user *buf,
>>>> if (copy_from_user(&hdr, buf, sizeof hdr))
>>>> return -EFAULT;
>>>>
>>>> - if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
>>>> - !uverbs_cmd_table[hdr.command])
>>>> - return -EINVAL;
>>>> + flags =hdr.command &
>>>> + IB_USER_VERBS_CMD_FLAGS_MASK) >>
>>>> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>>>>
>>>> - if (!file->ucontext &&
>>>> - hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>>> - return -EINVAL;
>>>> + if (!flags) {
>>>> + __u32 command;
>>>>
>>>> - if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>>>> hdr.command)))
>>>> - return -ENOSYS;
>>>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK
>>>> |
>>>> +
>>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>>> + return -EINVAL;
>>>>
>>>> - if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
>>>> - struct ib_uverbs_cmd_hdr_ex hdr_ex;
>>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>>>
>>>> - if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
>>>> - return -EFAULT;
>>>> + if (command >=RRAY_SIZE(uverbs_cmd_table) ||
>>>> + !uverbs_cmd_table[command])
>>>> + return -EINVAL;
>>>>
>>>> - if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4)
>>>> !=ount)
>>>> + if (!file->ucontext &&
>>>> + command !=B_USER_VERBS_CMD_GET_CONTEXT)
>>>> return -EINVAL;
>>>>
>>>> - return uverbs_cmd_table[hdr.command](file,
>>>> - buf +
>>>> sizeof(hdr_ex),
>>>> - (hdr_ex.in_words +
>>>> -
>>>> hdr_ex.provider_in_words) * 4,
>>>> - (hdr_ex.out_words +
>>>> -
>>>> hdr_ex.provider_out_words) * 4);
>>>> - } else {
>>>> + if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
>>>> command)))
>>>> + return -ENOSYS;
>>>> +
>>>> if (hdr.in_words * 4 !=ount)
>>>> return -EINVAL;
>>>>
>>>> - return uverbs_cmd_table[hdr.command](file,
>>>> - buf + sizeof(hdr),
>>>> - hdr.in_words * 4,
>>>> - hdr.out_words * 4);
>>>> + return uverbs_cmd_table[command](file,
>>>> + buf + sizeof(hdr),
>>>> + hdr.in_words * 4,
>>>> + hdr.out_words * 4);
>>>> +
>>>> + } else if (flags =IB_USER_VERBS_CMD_FLAG_EXTENDED) {
>>>> + __u32 command;
>>>> +
>>>> + struct ib_uverbs_ex_cmd_hdr ex_hdr;
>>>> + struct ib_udata ucore;
>>>> + struct ib_udata uhw;
>>>> + int err;
>>>> +
>>>> + if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK
>>>> |
>>>> +
>>>> IB_USER_VERBS_CMD_COMMAND_MASK))
>>>> + return -EINVAL;
>>>> +
>>>> + command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>>>> +
>>>> + if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
>>>> + !uverbs_ex_cmd_table[command])
>>>> + return -ENOSYS;
>>>> +
>>>> + if (!file->ucontext)
>>>> + return -EINVAL;
>>>> +
>>>> + if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull
>>>> << command)))
>>>> + return -ENOSYS;
>>>> +
>>>> + if (count < (sizeof(hdr) + sizeof(ex_hdr)))
>>>> + return -EINVAL;
>>>> +
>>>> + if (copy_from_user(&ex_hdr, buf + sizeof(hdr),
>>>> sizeof(ex_hdr)))
>>>> + return -EFAULT;
>>>> +
>>>> + count -=izeof(hdr) + sizeof(ex_hdr);
>>
>>
>> A small issue: count is reduced here and the write function returns
>> the reduced size.
>> I think we should return the amount of data the user wrote and not to
>> subtract the headers from the returned size.
>>
>
> Good catch.
Yann, the 3.13 merge window is coming closer, can you please post V2
of this series with fixes to the few
issues Matan was pointing on and possibly one squash @ the beginning
of the series, if this is possible.
>
>
>>>> +
>>>> + return count;
>>
>>
>> ====================^
>>
>
>
>>>> diff --git a/include/uapi/rdma/ib_user_verbs.h
>>>> b/include/uapi/rdma/ib_user_verbs.h
>>>> index 93577fc..cbfdd4c 100644
>>>> --- a/include/uapi/rdma/ib_user_verbs.h
>>>> +++ b/include/uapi/rdma/ib_user_verbs.h
>>>> @@ -87,8 +87,11 @@ enum {
>>>> IB_USER_VERBS_CMD_CLOSE_XRCD,
>>>> IB_USER_VERBS_CMD_CREATE_XSRQ,
>>>> IB_USER_VERBS_CMD_OPEN_QP,
>>>> - IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
>>>>
>>>> - IB_USER_VERBS_CMD_DESTROY_FLOW
>>>> +};
>>>> +
>>>> +enum {
>>>> + IB_USER_VERBS_EX_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
>>>> + IB_USER_VERBS_EX_CMD_DESTROY_FLOW
>>>> };
>>>
>>>
>>> I think B_USER_VERBS_CMD_THRESHOLD could be removed.
>>>
>
> If it's removed, CREATE_FLOW will be the first command.
>
> I thought it could be interesting to keep room for the "legacy"
> commands if they're going to be supported as extended commands
> in the future.
>
> Regards.
>
> --
> Yann Droneaud
> OPTEYA
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
