On 3/18/22 8:38 PM, Wenpeng Liang wrote:
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
Will fix.
<...>
+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.
Will fix.
<...>
+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.
Seems better, will change to it.
<...>
+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.
OK, will fix.
<...>
+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?
Yes, Will fix.
<...>
+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.
Will fix.
+ 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.
Will fix.
+ 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.
OK, will fix.
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.
Will fix.
Thanks,
Cheng Xu
Thanks,
Wenpeng
+ if (qp->cep) {
+ erdma_cep_put(cep);
+ qp->cep = NULL;
+ }
+
