On 2022/3/14 14:47, Cheng Xu wrote:
> Event queue (EQ) is the main notifcaition way from erdma hardware to its
> driver. Each erdma device contains 2 kinds EQs: asynchronous EQ (AEQ) and
> completion EQ (CEQ). Per device has 1 AEQ, which used for RDMA async event
> report, and max to 32 CEQs (numbered for CEQ0 to CEQ31). CEQ0 is used for
> cmdq completion event report, and the rest CEQs are used for RDMA
> completion event report.
>
notifcaition -> notification.
<...>
> +static void *get_eq_entry(struct erdma_eq *eq, u16 idx)
> +{
> + idx &= (eq->depth - 1);
> +
> + return eq->qbuf + (idx << EQE_SHIFT);
> +}
> +
> +void *get_next_valid_eqe(struct erdma_eq *eq)
> +{
> + u64 *eqe = (u64 *)get_eq_entry(eq, eq->ci);
The pointer type returned by get_eq_entry is "void *",
which does not need to be cast.
<...>
> +void erdma_aeq_event_handler(struct erdma_dev *dev)
> +{
> + struct erdma_aeqe *aeqe;
> + u32 cqn, qpn;
> + struct erdma_qp *qp;
> + struct erdma_cq *cq;
> + struct ib_event event;
> + u32 poll_cnt = 0;
> +
> + memset(&event, 0, sizeof(event));
> +
> + while (poll_cnt < MAX_POLL_CHUNK_SIZE) {
> + aeqe = (struct erdma_aeqe *)get_next_valid_eqe(&dev->aeq);
Ditto.
> + if (!aeqe)
> + break;
> +
> + poll_cnt++;
> +
> + if (FIELD_GET(ERDMA_AEQE_HDR_TYPE_MASK, aeqe->hdr) ==
> + ERDMA_AE_TYPE_CQ_ERR) {
> + cqn = aeqe->event_data0;
> + cq = find_cq_by_cqn(dev, cqn);
> + if (!cq)
> + continue;
As with the else branch, also having a blank line makes the code clearer.
> + event.device = cq->ibcq.device;
> + event.element.cq = &cq->ibcq;
> + event.event = IB_EVENT_CQ_ERR;
> + if (cq->ibcq.event_handler)
> + cq->ibcq.event_handler(&event,
> + cq->ibcq.cq_context);
> + } else {
> + qpn = aeqe->event_data0;
> + qp = find_qp_by_qpn(dev, qpn);
> + if (!qp)
> + continue;
> +
> + event.device = qp->ibqp.device;
> + event.element.qp = &qp->ibqp;
<...>
> +void erdma_ceq_completion_handler(struct erdma_eq_cb *ceq_cb)
> +{
> + int cqn;
> + struct erdma_cq *cq;
> + struct erdma_dev *dev = ceq_cb->dev;
> + u32 poll_cnt = 0;
> + u64 *hdr;
> +
> + if (!ceq_cb->ready)
> + return;
> +
> + while (poll_cnt < MAX_POLL_CHUNK_SIZE) {
> + hdr = (u64 *)get_next_valid_eqe(&ceq_cb->eq);
The pointer type returned by get_next_valid_eqe is "void *",
which does not need to be cast.
<...>
> +static int erdma_set_ceq_irq(struct erdma_dev *dev, u16 ceqn)
> +{
> + struct erdma_eq_cb *eqc = &dev->ceqs[ceqn];
> + cpumask_t affinity_hint_mask;
> + u32 cpu;
> + int err;
> +
> + snprintf(eqc->irq_name, ERDMA_IRQNAME_SIZE, "erdma-ceq%u@pci:%s",
> + ceqn, pci_name(dev->pdev));
Parameters in parentheses are not vertically aligned, a space is missing before "ceqn".
<...>
> +static int create_eq_cmd(struct erdma_dev *dev, u32 eqn, struct erdma_eq *eq)
> +{
> + struct erdma_cmdq_create_eq_req req;
> + dma_addr_t db_info_dma_addr;
> +
> + erdma_cmdq_build_reqhdr(&req.hdr, CMDQ_SUBMOD_COMMON,
> + CMDQ_OPCODE_CREATE_EQ);
> + req.eqn = eqn;
> + req.depth = ilog2(eq->depth);
> + req.qbuf_addr = eq->qbuf_dma_addr;
> + req.qtype = 1; /* CEQ */
Use a macro to represent "1", so this comment is not needed.
<...>
> +static int erdma_ceq_init_one(struct erdma_dev *dev, u16 ceqn)
> +{
> + struct erdma_eq *eq = &dev->ceqs[ceqn].eq;
> + u32 buf_size = ERDMA_DEFAULT_EQ_DEPTH << EQE_SHIFT;
> + int ret;
> +
> + 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);
> + atomic64_set(&eq->notify_num, 0);
> +
> + eq->depth = ERDMA_DEFAULT_EQ_DEPTH;
> + eq->db_addr = (u64 __iomem *)(dev->func_bar +
> + ERDMA_REGS_CEQ_DB_BASE_REG +
> + (ceqn + 1) * 8);
Does this "8" represent the byte width of a "u64 __iomem*" type?
<...>
> +int erdma_ceqs_init(struct erdma_dev *dev)
> +{
> + u32 i, j;
> + int err = 0;
The "err" does not need to be initialized to 0, it has been reassigned
before the function returns.
Thanks,
Wenpeng
> +
> + for (i = 0; i < dev->attrs.irq_num - 1; i++) {
> + err = erdma_ceq_init_one(dev, i);
> + if (err)
> + goto out_err;
> +
> + err = erdma_set_ceq_irq(dev, i);
> + if (err) {
> + erdma_ceq_uninit_one(dev, i);
> + goto out_err;
> + }
> + }
> +
> + return 0;
> +
> +out_err:
> + for (j = 0; j < i; j++) {
> + erdma_free_ceq_irq(dev, j);
> + erdma_ceq_uninit_one(dev, j);
> + }
> +
> + return err;
> +}
> +
