On 3/18/22 8:24 PM, Wenpeng Liang wrote:
On 2022/3/14 14:47, Cheng Xu wrote:
The RDMA verbs implementation of erdma is divided into three files:
erdma_qp.c, erdma_cq.c, and erdma_verbs.c. Internal used functions and
datapath functions of QP/CQ are put in erdma_qp.c and erdma_cq.c, the reset
is in erdma_verbs.c.
reset -> rest.
Will fix.
<...>
+static int erdma_poll_one_cqe(struct erdma_cq *cq, struct erdma_cqe *cqe,
+ struct ib_wc *wc)
+{
+ struct erdma_dev *dev = to_edev(cq->ibcq.device);
+ struct erdma_qp *qp;
+ struct erdma_kqp *kern_qp;
+ u64 *wqe_hdr;
+ u64 *id_table;
+ u32 qpn = be32_to_cpu(cqe->qpn);
+ u16 wqe_idx = be32_to_cpu(cqe->qe_idx);
+ u32 hdr = be32_to_cpu(cqe->hdr);
+ u16 depth;
+ u8 opcode, syndrome, qtype;
+
+ qp = find_qp_by_qpn(dev, qpn);
+ kern_qp = &qp->kern_qp;
The qp returned by find_qp_by_qpn may be null.
Thanks, this is a bug, will fix.
+
+ qtype = FIELD_GET(ERDMA_CQE_HDR_QTYPE_MASK, hdr);
+ syndrome = FIELD_GET(ERDMA_CQE_HDR_SYNDROME_MASK, hdr);
+ opcode = FIELD_GET(ERDMA_CQE_HDR_OPCODE_MASK, hdr);
+
+ if (qtype == ERDMA_CQE_QTYPE_SQ) {
+ id_table = kern_qp->swr_tbl;
+ depth = qp->attrs.sq_size;
+ wqe_hdr = (u64 *)get_sq_entry(qp, wqe_idx);
The pointer type returned by get_sq_entry is "void *",
which does not need to be cast.
Also will fix.
<...>
+int erdma_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc)
+{
+ struct erdma_cq *cq = to_ecq(ibcq);
+ struct erdma_cqe *cqe;
+ unsigned long flags;
+ u32 owner;
+ u32 ci;
+ int i, ret;
+ int new = 0;
+ u32 hdr;
+
+ spin_lock_irqsave(&cq->kern_cq.lock, flags);
+
+ owner = cq->kern_cq.owner;
+ ci = cq->kern_cq.ci;
+
+ for (i = 0; i < num_entries; i++) {
+ cqe = &cq->kern_cq.qbuf[ci & (cq->depth - 1)];
+
+ hdr = be32_to_cpu(READ_ONCE(cqe->hdr));
+ if (FIELD_GET(ERDMA_CQE_HDR_OWNER_MASK, hdr) != owner)
+ break;
+
+ /* cqbuf should be ready when we poll*/
"poll*/" -> "poll */".
Will fix.
+ dma_rmb();
+ ret = erdma_poll_one_cqe(cq, cqe, wc);
+ ci++;
+ if ((ci & (cq->depth - 1)) == 0)
+ owner = !owner;
+ if (ret)
The return value of erdma_poll_one_cqe is always 0.
As you mentioned above, find_qp_by_qpn may return NULL, and then
erdma_poll_one_cqe should return a non-zero value. The code here is all
right, and I will fix the bug in erdma_poll_one_cqe.
+ continue;
+ wc++;
+ new++;
+ }
+ cq->kern_cq.owner = owner;
+ cq->kern_cq.ci = ci;
+
+ spin_unlock_irqrestore(&cq->kern_cq.lock, flags);
+ return new;
+}
<...>
+struct ib_qp *erdma_get_ibqp(struct ib_device *ibdev, int id)
+{
+ struct erdma_qp *qp = find_qp_by_qpn(to_edev(ibdev), id);
+
+ if (qp)
+ return &qp->ibqp;
+
+ return (struct ib_qp *)NULL;
Redundant cast?
Will fix.
<...>
+int erdma_modify_qp_internal(struct erdma_qp *qp, struct erdma_qp_attrs *attrs,
+ enum erdma_qp_attr_mask mask)
+{
+ int drop_conn = 0, ret = 0;
+
+ if (!mask)
+ return 0;
+
+ if (!(mask & ERDMA_QP_ATTR_STATE))
+ return 0;
+
+ switch (qp->attrs.state) {
+ case ERDMA_QP_STATE_IDLE:
+ case ERDMA_QP_STATE_RTR:
+ if (attrs->state == ERDMA_QP_STATE_RTS) {
+ ret = erdma_modify_qp_state_to_rts(qp, attrs, mask);
+ } else if (attrs->state == ERDMA_QP_STATE_ERROR) {
+ qp->attrs.state = ERDMA_QP_STATE_ERROR;
+ if (qp->cep) {
+ erdma_cep_put(qp->cep);
+ qp->cep = NULL;
+ }
+ ret = erdma_modify_qp_state_to_stop(qp, attrs, mask);
+ }
+ break;
+ case ERDMA_QP_STATE_RTS:
+ if (attrs->state == ERDMA_QP_STATE_CLOSING) {
+ ret = erdma_modify_qp_state_to_stop(qp, attrs, mask);
+ drop_conn = 1;
+ } else if (attrs->state == ERDMA_QP_STATE_TERMINATE) {
+ qp->attrs.state = ERDMA_QP_STATE_TERMINATE;
+ ret = erdma_modify_qp_state_to_stop(qp, attrs, mask);
+ drop_conn = 1;
+ } else if (attrs->state == ERDMA_QP_STATE_ERROR) {
+ ret = erdma_modify_qp_state_to_stop(qp, attrs, mask);
+ qp->attrs.state = ERDMA_QP_STATE_ERROR;
+ drop_conn = 1;
+ }
+ break;
+ case ERDMA_QP_STATE_TERMINATE:
+ if (attrs->state == ERDMA_QP_STATE_ERROR)
+ qp->attrs.state = ERDMA_QP_STATE_ERROR;
+ break;
+ case ERDMA_QP_STATE_CLOSING:
+ if (attrs->state == ERDMA_QP_STATE_IDLE)
+ qp->attrs.state = ERDMA_QP_STATE_IDLE;
+ else if (attrs->state == ERDMA_QP_STATE_ERROR) {
+ ret = erdma_modify_qp_state_to_stop(qp, attrs, mask);
+ qp->attrs.state = ERDMA_QP_STATE_ERROR;
+ } else if (attrs->state != ERDMA_QP_STATE_CLOSING) {
+ return -ECONNABORTED;
+ }
If the conditional statement has a branch with curly braces,
then all branches use curly braces.
Yes, will fix.
+ break;
+ default:
+ break;
+ }
+
+ if (drop_conn)
+ erdma_qp_cm_drop(qp);
Can this conditional statement be placed in the ERDMA_QP_STATE_RTS branch
of the switch-case alternative branch?
The logic related to drop_conn is concentrated into one piece to improve
code aggregation.
Fine, it's better, and I will fix it.
+
+ return ret;
+}
<...>
+static int erdma_push_one_sqe(struct erdma_qp *qp, u16 *pi,
+ const struct ib_send_wr *send_wr)
+{
+ struct erdma_write_sqe *write_sqe;
+ struct erdma_send_sqe *send_sqe;
+ struct erdma_readreq_sqe *read_sqe;
+ struct erdma_reg_mr_sqe *regmr_sge;
+ struct erdma_mr *mr;
+ struct ib_rdma_wr *rdma_wr;
+ struct ib_sge *sge;
+ u32 wqe_size, wqebb_cnt, hw_op;
+ int ret;
+ u32 flags = send_wr->send_flags;
+ u32 idx = *pi & (qp->attrs.sq_size - 1);
+ u64 *entry = (u64 *)get_sq_entry(qp, idx);
<...>
+ sge = (struct ib_sge *)get_sq_entry(qp, idx + 1);
<...>
+ inline_data = (u64 *)get_sq_entry(qp, idx + 1);
The pointer type returned by get_sq_entry is "void *",
which does not need to be cast.
Will fix.
<...>
+static int erdma_post_recv_one(struct ib_qp *ibqp,
+ const struct ib_recv_wr *recv_wr,
+ const struct ib_recv_wr **bad_recv_wr)
bad_recv_wr is a redundant input parameter.
Will fix.
+{
+ struct erdma_qp *qp = to_eqp(ibqp);
+ struct erdma_rqe *rqe;
+ unsigned int rq_pi;
+ u16 idx;
+
+ rq_pi = qp->kern_qp.rq_pi;
+ idx = rq_pi & (qp->attrs.rq_size - 1);
+ rqe = (struct erdma_rqe *)qp->kern_qp.rq_buf + idx;
+
+ rqe->qe_idx = rq_pi + 1;
+ rqe->qpn = QP_ID(qp);
+
+ if (recv_wr->num_sge == 0) {
+ rqe->length = 0;
+ } else if (recv_wr->num_sge == 1) {
+ rqe->stag = recv_wr->sg_list[0].lkey;
+ rqe->to = recv_wr->sg_list[0].addr;
+ rqe->length = recv_wr->sg_list[0].length;
+ } else {
+ return -EINVAL;
+ }
+
+ *(u64 *)qp->kern_qp.rq_db_info = *(u64 *)rqe;
+ writeq(*(u64 *)rqe, qp->kern_qp.hw_rq_db);
+
+ qp->kern_qp.rwr_tbl[idx] = recv_wr->wr_id;
+ qp->kern_qp.rq_pi = rq_pi + 1;
+
+ return 0;
+}
+
+int erdma_post_recv(struct ib_qp *qp, const struct ib_recv_wr *recv_wr,
+ const struct ib_recv_wr **bad_recv_wr)
+{
+ struct erdma_qp *eqp = to_eqp(qp);
+ int ret = 0;
+ const struct ib_recv_wr *wr = recv_wr;
+ unsigned long flags;
+
+ if (!qp || !recv_wr)
+ return -EINVAL;
+
+ spin_lock_irqsave(&eqp->lock, flags);
+ while (wr) {
+ ret = erdma_post_recv_one(qp, wr, bad_recv_wr);
+ if (ret) {
+ *bad_recv_wr = wr;
+ break;
+ }
+ wr = wr->next;
+ }
+ spin_unlock_irqrestore(&eqp->lock, flags);
+ return ret;
+}
The "ret" does not need to be initialized to 0, it has been reassigned
before the function returns.
Will fix.
<...>
+static int erdma_create_stag(struct erdma_dev *dev, u32 *stag)
+{
+ int stag_idx;
+ u32 key = 0;
+
+ stag_idx = erdma_alloc_idx(&dev->res_cb[ERDMA_RES_TYPE_STAG_IDX]);
+ if (stag_idx < 0)
+ return stag_idx;
+
+ *stag = (stag_idx << 8) | (key & 0xFF);
The "key" is initialized to 0 and never assigned again.
Will fix it.
<...>
+int erdma_query_qp(struct ib_qp *ibqp, struct ib_qp_attr *qp_attr,
+ int qp_attr_mask, struct ib_qp_init_attr *qp_init_attr)
+{
+ struct erdma_qp *qp;
+ struct erdma_dev *dev;
+
+ if (ibqp && qp_attr && qp_init_attr) {
+ qp = to_eqp(ibqp);
+ dev = to_edev(ibqp->device);
+ } else
+ return -EINVAL;
If the conditional statement has a branch with curly braces,
then all branches use curly braces.
Will fix it.
Thanks,
Cheng Xu
Thanks,
Wenpeng
