Re: [PATCH for-next v4 06/12] RDMA/erdma: Add event queue implementation

[Cheng Xu <chengyou@linux.alibaba.com>]

On 3/18/22 7:43 PM, Wenpeng Liang wrote:
> 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.
> 

Sorry for these typos, will fix them.

> <...>
>> +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.
> 

OK, will fix.

> <...>
>> +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.
> 

Will fix.

>> +			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.
> 

Will fix.

> <...>
>> +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".
> 

Will fix.

> <...>
>> +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.

Will fix.

> 
> <...>
>> +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?
> 

Yes, each EQ's doorbell takes 8 Bytes, I will use a micro instead of
this magic number.

> <...>
>> +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.
> 

OK, will fix.

Thanks,
Cheng Xu

> 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;
>> +}
>> +