On 12/29/2017 5:23 PM, Or Gerlitz wrote:
validate_driver_qp() deals @ 95% of it's contents with DC but has no
DC in it's name, also it does assignment which goes beyond validation.
OK, will rename it to be prepare_driver_qp().
The function name follows the input QP type (i.e. IB_QPT_DRIVER) and
what it does on. Specifically, its purpose is to prepare the input data
(ucmd, init_attr) to be used later on. Similar name and logic is used as
part of RQ creation as part of this file. (i.e. prepare_user_rq()).
There are bunch of things to consider fixing here. The patch spaghettizes
the driver code w.r.t where the qp type is stored, in the ib qp
It doesn't make sense to overwrite the input QP type (i.e.
IB_QPT_DRIVER) which was supplied by the core layer to some internal
sub-type.
or the mlx5 qp
The sub type is from type enum ib_qp_type (MLX5_IB_QPT_DCI
<->IB_QPT_RESERVED3) and not an internal mlx5 PRM QP type.
However, to make the code more clearer and accurate, we'll change as
part of V2 'u32 driver_qp_type' to be 'enum ib_qp_type sub_qp_type' and
clean some redundant checks when applicable. (e.g. in
mlx5_ib_query_qp()).
Also you added a "is qp ok" self function which AFAIK uses a different practice
from the IB core copy, did you consider to refactor the core code and allow
a re-use from the driver?
Yes, we considered that, however, as this QP type has some specific
driver behavior we found it better to stay with that specific logic for
DCI QP in the driver code, no reason to refactor the core layer as of
that. As part of V2 will rename 'mlx5_ib_modify_qp_is_ok()' to be
'modify_dc_qp_is_ok()' to better represent the specific use case around.
did you consider to use a similar practice
for your "is ok" function?
Yes, however, we found it more clearer and simple to use current code
for the specific DCI case instead of introducing some general mapping
table without any real use.
--
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
