On Tue, Nov 21, 2017 at 02:01:33PM +0200, Yishai Hadas wrote:
> On 11/20/2017 9:04 PM, Jason Gunthorpe wrote:
> >On Sun, Nov 19, 2017 at 09:21:08PM +0200, Yishai Hadas wrote:
> >>+ if (mlx5wq_attr) {
> >>+ if (mlx5wq_attr->comp_mask & ~(MLX5DV_WQ_INIT_ATTR_MASK_RESERVED - 1))
> >>+ return -EINVAL;
> >
> >Please no. Check that only the bits this function actually supports
> >are set and get rid of _RESERVED.
> >
>
> OK, PR was updated accordingly:
> https://github.com/linux-rdma/rdma-core/pull/257
This new version is mixing bit widths:
+enum mlx5dv_wq_init_attr_mask {
+ MLX5DV_WQ_INIT_ATTR_MASK_STRIDING_RQ = 1 << 0,
+};
+enum {
+ DV_CREATE_WQ_SUPPORTED_COMP_MASK = MLX5DV_WQ_INIT_ATTR_MASK_STRIDING_RQ
+};
+struct mlx5dv_wq_init_attr {
+ uint64_t comp_mask;
[..]
So this expression:
+ if (mlx5wq_attr->comp_mask & ~DV_CREATE_WQ_SUPPORTED_COMP_MASK)
Will bitwise invert a 32 bit value then 0 extend it to 64 bits, which
is not the intention.
As I suggested earlier, I recommend helper functions for this in
driver.h:
static inline bool check_comp_mask(uint64_t input, uint64_t supported)
{
return (input & ~supported) == 0;
}
if (!check_comp_mask(mlx5wq_attr->comp_mask,
MLX5DV_WQ_INIT_ATTR_MASK_STRIDING_RQ))
return -EINVAL;
I also dislike the unnecessary extra enum..
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
