Re: [PATCH for-next v4 09/12] RDMA/erdma: Add connection management (CM) support

[Wenpeng Liang <liangwenpeng@huawei.com>]

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