On 01-Apr-19 11:47, Leon Romanovsky wrote:
> From: Mark Zhang <markz@xxxxxxxxxxxx>
>
> In manual mode a QP is bound to a counter manually. If counter is not
> specified then a new one will be allocated.
> Manually mode is enabled when user binds a QP, and disabled when the
> last manually bound QP is unbound.
> When auto-mode is turned off and there are counters left, manual mode
> is enabled so that the user is able to access these counters.
IMO, an API to allocate a counter makes more sense than allocating it on the
first bind when a counter is not specified.
The part about turning off auto mode with counters left seems like a mess, do we
really want all these hidden "rules" instead of keeping it simple? Don't let the
user change modes when a counter still has resources bind to it.
>
> Signed-off-by: Mark Zhang <markz@xxxxxxxxxxxx>
> Reviewed-by: Majd Dibbiny <majd@xxxxxxxxxxxx>
> Signed-off-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
> ---
> +static int rdma_counter_bind_qp_manual(struct rdma_counter *counter,
> + struct ib_qp *qp)
> +{
> + int ret = 0;
> +
> + if (qp->port != counter->port)
> + return -EINVAL;
> +
> + ret = __rdma_counter_bind_qp(counter, qp);
> + if (!ret)
> + atomic_inc(&counter->usecnt);
Better to keep the 'if (ret)' indented and the success flow unindented.
> +
> + return ret;
> +}
> +
> +/**
> + * rdma_counter_bind_qpn() - Bind QP @qp_num to counter @counter_id
> + */
> +int rdma_counter_bind_qpn(struct ib_device *dev, u8 port,
> + u32 qp_num, u32 counter_id)
> +{
> + struct rdma_counter *counter;
> + struct ib_qp *qp;
> + int ret = 0;
The zero assignment is redundant.
> +
> + qp = rdma_counter_get_qp(dev, qp_num);
> + if (!qp)
> + return -ENOENT;
> +
> + counter = rdma_get_counter_by_id(dev, counter_id);
> + if (!counter) {
> + ret = -ENOENT;
> + goto err;
> + }
> +
> + if (counter->res.task != qp->res.task) {
> + ret = -EINVAL;
> + goto err_task;
> + }
> +
> + ret = rdma_counter_bind_qp_manual(counter, qp);
> + if (ret)
> + goto err_task;
> +
> + rdma_restrack_put(&qp->res);
> + return 0;
> +
> +err_task:
> + rdma_restrack_put(&counter->res);
> +err:
> + rdma_restrack_put(&qp->res);
> + return ret;
> +}
> +
> +/**
> + * rdma_counter_bind_qpn_alloc() - Alloc a counter and bind QP @qp_num to it
> + * The id of new counter is returned in @counter_id
> + */
> +int rdma_counter_bind_qpn_alloc(struct ib_device *dev, u8 port,
> + u32 qp_num, u32 *counter_id)
> +{
> + struct rdma_counter *counter;
> + struct ib_qp *qp;
> + int ret = 0;
Same.
> +
> + if (!rdma_is_port_valid(dev, port))
> + return -EINVAL;
> +
> + qp = rdma_counter_get_qp(dev, qp_num);
> + if (!qp)
> + return -ENOENT;
> +
> + if (rdma_is_port_valid(dev, qp->port) && (qp->port != port)) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + counter = rdma_counter_alloc(dev, port, RDMA_COUNTER_MODE_MANUAL);
> + if (!counter) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + ret = rdma_counter_bind_qp_manual(counter, qp);
> + if (ret)
> + goto err_bind;
> +
> + if (counter_id)
> + *counter_id = counter->id;
> +
> + rdma_counter_res_add(counter, qp);
> +
> + if (!rdma_restrack_get(&counter->res)) {
> + rdma_counter_unbind_qp(qp, false);
> + ret = -EINVAL;
> + }
> +
> + rdma_restrack_put(&qp->res);
> + return ret;
> +
> +err_bind:
> + rdma_counter_dealloc(counter);
> +err:
> + rdma_restrack_put(&qp->res);
> + return ret;
> +}
