On 06/08/17 07:38, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@xxxxxxxxxxxx>
>
> RDMA netlink has complicated infrastructure to add and remove netlink
> clients to NETLINK_RDMA family. This complicates the code and not in
> use because not many clients are available (3 clients) and most of them
> (2 clients) are statically compiled together with netlink.c.
>
> The following patch refactors RDMA netlink and opens door for the future
> patches which will be able to get rid of a lot of dead iwcm* code.
Hello Leon,
There are multiple changes in this patch:
- Renaming ibnl_add_client() and ibnl_remove_client() into
rdma_nl_register() and rdma_nl_unregister().
- Removal of the module pointers from ibnl_ls_cb_table[].
- Conversion of client_list from a linked list into an array
(rdma_nl_types[]).
Could this patch have been split?
> +static bool is_nl_msg_valid(unsigned int type, unsigned int op)
> {
> - [ ... ]
> + unsigned int max_num_ops;
> +
> + switch (type) {
> + case RDMA_NL_RDMA_CM:
> + max_num_ops = RDMA_NL_RDMA_CM_NUM_OPS;
> + break;
> + case RDMA_NL_IWCM:
> + max_num_ops = RDMA_NL_IWPM_NUM_OPS;
> + break;
> + case RDMA_NL_LS:
> + max_num_ops = RDMA_NL_LS_NUM_OPS;
> + break;
> + case RDMA_NL_RSVD:
> + case RDMA_NL_I40IW:
> + default:
> + return false;
> }
> + return (op < max_num_ops) ? true : false;
> +}
Can this code be made more compact by storing the *_NUM_OPS constants in
an array?
> +void rdma_nl_register(unsigned int index,
> + const struct ibnl_client_cbs cb_table[])
> {
> - [ ... ]
> - return -EINVAL;
> + rdma_nl_types[index].cb_table = cb_table;
> + mutex_unlock(&rdma_nl_mutex);
> +}
> +EXPORT_SYMBOL(rdma_nl_register);
Shouldn't this function return an error code or print a warning message
if rdma_nl_types[index].cb_table had already been set?
Otherwise this patch looks like a welcome cleanup and simplification to me.
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
