On Wed, Dec 27, 2017 at 11:16:09PM +0200, Or Gerlitz wrote:
> On Wed, Dec 27, 2017 at 7:15 PM, Leon wrote:
> > From: Moni Shoua <monis@xxxxxxxxxxxx>
>
> > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > @@ -202,6 +202,8 @@ struct mlx5_ib_flow_db {
> > * creates the actual hardware QP.
> > */
> > #define MLX5_IB_QPT_HW_GSI IB_QPT_RESERVED2
> > +#define MLX5_IB_QPT_DCI IB_QPT_RESERVED3
> > +#define MLX5_IB_QPT_DCT IB_QPT_RESERVED4
>
> When we added the reserved QPTs we made an explicit statement at the
> verbs header file that this is a "range for qp types internal to the
> low level driver"
>
> In the down stream patches you are actually assuming user-space
> knows that for mlx5 DCI/DCT equals IB_QPT_RESERVED3/4 which
> violates this statement.
IB_QPT_RESERVED should NOT leak to the user driver ever, for some
reason a flag is used. I thought we agreed to an enum during the
original RFC?
> Maybe you need
>
> IB_QPT_VENDOR1
> IB_QPT_VENDOR2
> ...
> IB_QPT_VENDOR10
No, no. The driver specific QP type is carried only within the driver
specific udata and is never part of the common API.
> you added these two flags in the ABI file, but you never use them in downstream
>
> MLX5_QP_FLAG_TYPE_DCT
> MLX5_QP_FLAG_TYPE_DCI
>
> patches, something must be wrong here even according to your design
Hm? they are used here:
+ if (ucmd->flags & MLX5_QP_FLAG_TYPE_DCI)
+ init_attr->qp_type = MLX5_IB_QPT_DCI;
+ else
+ init_attr->qp_type = MLX5_IB_QPT_DCT;
Which is a bad ABI design, check flags explicitly never use else.
Not sure how much I like this - why try so hard to keep the udata
unchanged? I would have added a 'u32 mlx_qp_type' and a variable
length struct I think..
Or use the new kabi :|
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
