On 2022/3/14 14:47, Cheng Xu wrote:
<...>
> +static int erdma_cmdq_eq_init(struct erdma_dev *dev)
> +{
> + struct erdma_cmdq *cmdq = &dev->cmdq;
> + struct erdma_eq *eq = &cmdq->eq;
> + u32 buf_size;
> +
> + eq->depth = cmdq->max_outstandings;
> + buf_size = eq->depth << EQE_SHIFT;
> +
> + eq->qbuf = dma_alloc_coherent(&dev->pdev->dev,
> + WARPPED_BUFSIZE(buf_size),
> + &eq->qbuf_dma_addr,
> + GFP_KERNEL | __GFP_ZERO);
> + if (!eq->qbuf)
> + return -ENOMEM;
> +
> + spin_lock_init(&eq->lock);
> + atomic64_set(&eq->event_num, 0);
This patchset sets and increases the reference count of event_num, but does not
call other interfaces such as atomic_dec_and_test to judge event_num. This
variable seems to be redundant in this patchset. Will subsequent patches
extend the function of event_num?
Similar to notify_num, armed_num.
<...>
> +
> +static void erdma_poll_single_cmd_completion(struct erdma_cmdq *cmdq,
> + __be32 *cqe)
> +{
> + struct erdma_comp_wait *comp_wait;
> + u16 sqe_idx, ctx_id;
> + u64 *sqe;
> + int i;
> + u32 hdr0 = __be32_to_cpu(*cqe);
> +
> + sqe_idx = __be32_to_cpu(*(cqe + 1));
> + sqe = (u64 *)get_cmdq_sqe(cmdq, sqe_idx);
The pointer type returned by get_cmdq_sqe is "void *",
which does not need to be cast.
<...>
> +
> +static void erdma_polling_cmd_completions(struct erdma_cmdq *cmdq)
> +{
> + u32 hdr;
> + __be32 *cqe;
> + unsigned long flags;
> + u16 comp_num = 0;
> + u8 owner, expect_owner;
> + u16 cqe_idx;
> +
> + spin_lock_irqsave(&cmdq->cq.lock, flags);
> +
> + expect_owner = cmdq->cq.owner;
> + cqe_idx = cmdq->cq.ci & (cmdq->cq.depth - 1);
> +
> + while (1) {
> + cqe = (__be32 *)get_cmdq_cqe(cmdq, cqe_idx);
> + hdr = __be32_to_cpu(READ_ONCE(*cqe));
> +
> + owner = FIELD_GET(ERDMA_CQE_HDR_OWNER_MASK, hdr);
> + if (owner != expect_owner)
> + break;
> +
> + dma_rmb();
> + erdma_poll_single_cmd_completion(cmdq, cqe);
> + comp_num++;
> + if (cqe_idx == cmdq->cq.depth - 1) {
> + cqe_idx = 0;
> + expect_owner = !expect_owner;
> + } else {
> + cqe_idx++;
> + }
> + }
> +
> + if (comp_num) {
> + cmdq->cq.ci += comp_num;
> + cmdq->cq.owner = expect_owner;
> +
> + if (cmdq->use_event)
> + arm_cmdq_cq(cmdq);
> + }
> +
> + spin_unlock_irqrestore(&cmdq->cq.lock, flags);
> +}
The logic for judging whether cqe is valid is too complicated,
you can refer to the function get_sw_cqe_v2() of hns roce,
I hope it will help you.
Thanks,
Wenpeng
