Re: [PATCH rdma-core 2/2] mlx5: Allow creation of a Multi-Packet RQ using direct verbs

[Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org>]

On Tue, Nov 21, 2017 at 07:17:00PM +0200, Yishai Hadas wrote:

> >Will bitwise invert a 32 bit value then 0 extend it to 64 bits, which
> >is not the intention.
> 
> Are you are referring to a case that the user will turn on a bit in the
> comp_mask over the 32 bits and won't get an error ? Look at
> MLX5DV_CQ_INIT_ATTR_MASK_RESERVED usage it has the same potential issue.

Yes, I'm not surprised, this is possibly systemically broken. We
should add the helper and deploy it everywhere we are doing this style
of check.

> >if (!check_comp_mask(mlx5wq_attr->comp_mask,
> >      MLX5DV_WQ_INIT_ATTR_MASK_STRIDING_RQ))
> >        return -EINVAL;
> >I also dislike the unnecessary extra enum..
> 
> I prefer staying with the above enum (i.e. DV_CREATE_WQ_SUPPORTED_COMP_MASK)
> to hold the supported bits as done in mlx5 for QP & CQ. (see
> MLX5_CREATE_QP_SUP_COMP_MASK). In that way the code around check_comp_mask()
> won't be changed when extra bits will be added.

Well, it is up to you, but I dislike this because it sprinkles the
logic around too much. Seeing clearly a line 'check_comp_mask' at the
top of the function that lists all the supported bits makes it very
easy to understand and review.

If there was more than one user of DV_CREATE_WQ_SUPPORTED_COMP_MASK I
could understand having it, but since there isn't, it is just ugly
noise.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html