On 16/11/2020 22:23, Jason Gunthorpe wrote:
> When the user calls efa_query_device_ex() it should not cause the context
> values to be mutated, only the attribute shuld be returned.
>
> Move this code to a dedicated function that is only called during context
> setup.
>
> Cc: Gal Pressman <galpress@xxxxxxxxxx>
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> providers/efa/efa.c | 14 +------------
> providers/efa/verbs.c | 46 +++++++++++++++++++++++++++++++++++--------
> providers/efa/verbs.h | 1 +
> 3 files changed, 40 insertions(+), 21 deletions(-)
>
> diff --git a/providers/efa/efa.c b/providers/efa/efa.c
> index 35f9b246a711ec..b24c14f7fa1fe1 100644
> --- a/providers/efa/efa.c
> +++ b/providers/efa/efa.c
> @@ -54,10 +54,7 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
> {
> struct efa_alloc_ucontext_resp resp = {};
> struct efa_alloc_ucontext cmd = {};
> - struct ibv_device_attr_ex attr;
> - unsigned int qp_table_sz;
> struct efa_context *ctx;
> - int err;
>
> cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_TX_BATCH;
> cmd.comp_mask |= EFA_ALLOC_UCONTEXT_CMD_COMP_MIN_SQ_WR;
> @@ -86,17 +83,8 @@ static struct verbs_context *efa_alloc_context(struct ibv_device *vdev,
>
> verbs_set_ops(&ctx->ibvctx, &efa_ctx_ops);
>
> - err = efa_query_device_ex(&ctx->ibvctx.context, NULL, &attr,
> - sizeof(attr));
> - if (err)
> + if (!efa_query_device_ctx(ctx))
Remove the not.
> goto err_free_spinlock;
> -
> - qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
> - ctx->qp_table_sz_m1 = qp_table_sz - 1;
> - ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
> - if (!ctx->qp_table)
> - goto err_free_spinlock;
> -
> return &ctx->ibvctx;
>
> err_free_spinlock:
> diff --git a/providers/efa/verbs.c b/providers/efa/verbs.c
> index 1a9633155c62f8..52d6285f1f409c 100644
> --- a/providers/efa/verbs.c
> +++ b/providers/efa/verbs.c
> @@ -106,14 +106,6 @@ int efa_query_device_ex(struct ibv_context *context,
> if (err)
> return err;
>
> - ctx->device_caps = resp.device_caps;
> - ctx->max_sq_wr = resp.max_sq_wr;
> - ctx->max_rq_wr = resp.max_rq_wr;
> - ctx->max_sq_sge = resp.max_sq_sge;
> - ctx->max_rq_sge = resp.max_rq_sge;
> - ctx->max_rdma_size = resp.max_rdma_size;
> - ctx->max_wr_rdma_sge = a->max_sge_rd;
> -
> a->max_qp_wr = min_t(int, a->max_qp_wr,
> ctx->max_llq_size / sizeof(struct efa_io_tx_wqe));
> snprintf(a->fw_ver, sizeof(a->fw_ver), "%u.%u.%u.%u",
> @@ -122,6 +114,44 @@ int efa_query_device_ex(struct ibv_context *context,
> return 0;
> }
>
> +int efa_query_device_ctx(struct efa_context *ctx)
> +{
> + struct ibv_device_attr_ex attr;
> + struct efa_query_device_ex_resp resp;
Preferably I would put this first.
> + size_t resp_size = sizeof(resp);
> + unsigned int qp_table_sz;
> + int err;
> +
> + if (ctx->cmds_supp_udata_mask & EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE) {
> + err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
> + &attr, sizeof(attr),
> + &resp.ibv_resp, &resp_size);
> + if (err)
> + return err;
> +
> + ctx->device_caps = resp.device_caps;
> + ctx->max_sq_wr = resp.max_sq_wr;
> + ctx->max_rq_wr = resp.max_rq_wr;
> + ctx->max_sq_sge = resp.max_sq_sge;
> + ctx->max_rq_sge = resp.max_rq_sge;
> + ctx->max_rdma_size = resp.max_rdma_size;
> + ctx->max_wr_rdma_sge = attr.orig_attr.max_sge_rd;
max_wr_rdma_sge assignment can be done in the else clause as well.
> + } else {
> + err = ibv_cmd_query_device_any(&ctx->ibvctx.context, NULL,
> + &attr, sizeof(attr.orig_attr),
> + NULL, NULL);
> + if (err)
> + return err;
> + }
> +
> + qp_table_sz = roundup_pow_of_two(attr.orig_attr.max_qp);
> + ctx->qp_table_sz_m1 = qp_table_sz - 1;
> + ctx->qp_table = calloc(qp_table_sz, sizeof(*ctx->qp_table));
> + if (!ctx->qp_table)
> + return ENOMEM;
> + return 0;
> +}
