On 2022/3/14 14:47, Cheng Xu wrote:
> ERDMA's transport procotol is iWarp, so the driver must support CM
> interface. In CM part, we use the same way as SoftiWarp: using kernel
> socket to setup the connection, then performing MPA negotiation in kernel.
> So, this part of code mainly comes from SoftiWarp, base on it, we add some
> more features, such as non-blocking iw_connect implementation.
procotol -> protocol
setup -> set up
<...>
> +static void erdma_socket_disassoc(struct socket *s)
> +{
> + struct sock *sk = s->sk;
> + struct erdma_cep *cep;
> +
> + if (sk) {
> + write_lock_bh(&sk->sk_callback_lock);
> + cep = sk_to_cep(sk);
> + if (cep) {
> + erdma_sk_restore_upcalls(sk, cep);
> + erdma_cep_put(cep);
> + } else
> + WARN_ON_ONCE(1);
If the conditional statement has a branch with curly braces,
then all branches use curly braces.
<...>
> +static void erdma_cep_set_inuse(struct erdma_cep *cep)
> +{
> + unsigned long flags;
> +retry:
> + spin_lock_irqsave(&cep->lock, flags);
> +
> + if (cep->in_use) {
> + spin_unlock_irqrestore(&cep->lock, flags);
> + wait_event_interruptible(cep->waitq, !cep->in_use);
> + if (signal_pending(current))
> + flush_signals(current);
> + goto retry;
> + } else {
> + cep->in_use = 1;
> + spin_unlock_irqrestore(&cep->lock, flags);
> + }
> +}
Using while(cep->inuse){...} instead of goto statement to
implement cep->inuse status check may make the code cleaner.
<...>
> +static int erdma_cm_upcall(struct erdma_cep *cep, enum iw_cm_event_type reason,
> + int status)
> +{
> + struct iw_cm_event event;
> + struct iw_cm_id *cm_id;
> +
> + memset(&event, 0, sizeof(event));
> + event.status = status;
> + event.event = reason;
> +
> + if (reason == IW_CM_EVENT_CONNECT_REQUEST) {
> + event.provider_data = cep;
> + cm_id = cep->listen_cep->cm_id;
> +
> + event.ird = cep->dev->attrs.max_ird;
> + event.ord = cep->dev->attrs.max_ord;
> + } else {
> + cm_id = cep->cm_id;
> + }
> +
> + if (reason == IW_CM_EVENT_CONNECT_REQUEST ||
> + reason == IW_CM_EVENT_CONNECT_REPLY) {
> + u16 pd_len = be16_to_cpu(cep->mpa.hdr.params.pd_len);
> +
> + if (pd_len) {
> + event.private_data_len = pd_len;
> + event.private_data = cep->mpa.pdata;
> + if (cep->mpa.pdata == NULL)
> + event.private_data_len = 0;
> + }
Using an if-else to assign a value to event.private_data_len
may make the code clearer.
<...>
> +static int erdma_proc_mpareq(struct erdma_cep *cep)
> +{
> + struct mpa_rr *req;
> + int ret;
> +
> + ret = erdma_recv_mpa_rr(cep);
> + if (ret)
> + return ret;
> +
> + req = &cep->mpa.hdr;
> +
> + if (memcmp(req->key, MPA_KEY_REQ, MPA_KEY_SIZE))
> + return -EPROTO;
> +
> + memcpy(req->key, MPA_KEY_REP, 16);
Are "16" and "MPA_KEY_SIZE" equivalent?
<...>
> +static int erdma_proc_mpareply(struct erdma_cep *cep)
> +{
> + struct erdma_qp_attrs qp_attrs;
> + struct erdma_qp *qp = cep->qp;
> + struct mpa_rr *rep;
> + int ret;
> +
> + ret = erdma_recv_mpa_rr(cep);
> + if (ret != -EAGAIN)
> + erdma_cancel_mpatimer(cep);
> + if (ret)
> + goto out_err;
> +
> + rep = &cep->mpa.hdr;
> +
> + if (memcmp(rep->key, MPA_KEY_REP, MPA_KEY_SIZE)) {
> + ret = -EPROTO;
> + goto out_err;
> + }
Missing a blank line.
> + if (rep->params.bits & MPA_RR_FLAG_REJECT) {
> + erdma_cm_upcall(cep, IW_CM_EVENT_CONNECT_REPLY, -ECONNRESET);
> + return -ECONNRESET;
> + }
<...>
> + if (cep->qp) {
> + struct erdma_qp *qp = cep->qp;
> + /*
> + * Serialize a potential race with application
> + * closing the QP and calling erdma_qp_cm_drop()
> + */
> + erdma_qp_get(qp);
> + erdma_cep_set_free(cep);
> +
> + erdma_qp_llp_close(qp);
> + erdma_qp_put(qp);
> +
> + erdma_cep_set_inuse(cep);
> + cep->qp = NULL;
> + erdma_qp_put(qp);
> + }
Missing a blank line.
> + if (cep->sock) {
> + erdma_socket_disassoc(cep->sock);
> + sock_release(cep->sock);
> + cep->sock = NULL;
> + }
> +
<...>
> +static void erdma_cm_llp_data_ready(struct sock *sk)
> +{
> + struct erdma_cep *cep;
> +
> + read_lock(&sk->sk_callback_lock);
> +
> + cep = sk_to_cep(sk);
> + if (!cep)
> + goto out;
> +
> + switch (cep->state) {
> + case ERDMA_EPSTATE_RDMA_MODE:
> + case ERDMA_EPSTATE_LISTENING:
> + break;
> + case ERDMA_EPSTATE_AWAIT_MPAREQ:
> + case ERDMA_EPSTATE_AWAIT_MPAREP:
> + erdma_cm_queue_work(cep, ERDMA_CM_WORK_READ_MPAHDR);
> + break;
> + default:
> + break;
> + }
Use if instead of switch-case to reduce code complexity.
if (cep->state == ERDMA_EPSTATE_AWAIT_MPAREQ ||
cep->state == ERDMA_EPSTATE_AWAIT_MPAREP ||)
erdma_cm_queue_work(cep, ERDMA_CM_WORK_READ_MPAHDR);
<...>
> + if (cep->cm_id) {
> + cep->cm_id->rem_ref(id);
> + cep->cm_id = NULL;
> + }
Missing a blank line.
Thanks,
Wenpeng
> + if (qp->cep) {
> + erdma_cep_put(cep);
> + qp->cep = NULL;
> + }
> +
