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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
