Only one series issue, along with a handful of nits.
Tom Tucker wrote:
+#include <rdma/ib_sa.h>
Why is ib_sa.h needed?
+static int cma_iw_handler(struct iw_cm_id* iw_id, struct iw_cm_event* event)
+{
+ struct rdma_id_private *id_priv = iw_id->context;
+ enum rdma_cm_event_type event_type = 0;
For consistency with the cma_ib_handler, can you rename 'event' to 'iw_event'
and 'event_type' to event?
+ int ret = 0;
+
+ atomic_inc(&id_priv->dev_remove);
+
+ switch (event->event) {
+ case IW_CM_EVENT_LLP_DISCONNECT:
+ case IW_CM_EVENT_LLP_RESET:
+ case IW_CM_EVENT_LLP_TIMEOUT:
+ case IW_CM_EVENT_CLOSE:
+ event_type = RDMA_CM_EVENT_DISCONNECTED;
+ break;
+
+ case IW_CM_EVENT_CONNECT_REPLY: {
+ if (event->status)
+ event_type = RDMA_CM_EVENT_REJECTED;
+ else
+ event_type = RDMA_CM_EVENT_ESTABLISHED;
+ break;
+ }
+
+ case IW_CM_EVENT_ESTABLISHED:
+ event_type = RDMA_CM_EVENT_ESTABLISHED;
+ break;
+
+ default:
+ BUG_ON(1);
+ break;
+
+ }
Nit: please remove the extra spacing and braces in the above code.
+
+ ret = cma_notify_user(id_priv,
+ event_type,
+ event->status,
+ event->private_data,
+ event->private_data_len);
Nit: elsewhere in the code uses multiple parameters per line.
+ if (ret) {
+ /* Destroy the CM ID by returning a non-zero value. */
+ id_priv->cm_id.iw = NULL;
+ cma_exch(id_priv, CMA_DESTROYING);
+ cma_release_remove(id_priv);
+ rdma_destroy_id(&id_priv->id);
+ return ret;
+ }
+
+ cma_release_remove(id_priv);
+ return ret;
+}
+
+static int iw_conn_req_handler(struct iw_cm_id *cm_id,
+ struct iw_cm_event *iw_event)
+{
+ struct rdma_cm_id* new_cm_id;
+ struct rdma_id_private *listen_id, *conn_id;
+ struct sockaddr_in* sin;
+ int ret;
+
+ listen_id = cm_id->context;
+ atomic_inc(&listen_id->dev_remove);
+ if (!cma_comp(listen_id, CMA_LISTEN)) {
+ ret = -ECONNABORTED;
+ goto out;
+ }
+
+ /* Create a new RDMA id the new IW CM ID */
+ new_cm_id = rdma_create_id(listen_id->id.event_handler,
+ listen_id->id.context,
+ RDMA_PS_TCP);
+ if (!new_cm_id) {
+ ret = -ENOMEM;
+ goto out;
+ }
+ conn_id = container_of(new_cm_id, struct rdma_id_private, id);
+ atomic_inc(&conn_id->dev_remove);
+ conn_id->state = CMA_CONNECT;
+
+ /* New connection inherits device and address from parent */
+ memcpy(&new_cm_id->route.addr.dev_addr,
+ &listen_id->id.route.addr.dev_addr,
+ sizeof(new_cm_id->route.addr.dev_addr));
+ /* New connection inherits device from parent */
+ cma_acquire_dev(conn_id);
We are not assured that we'll be able to acquire the device. It could have been
removed as we entered the req_handler.
+
+ conn_id->cm_id.iw = cm_id;
+ cm_id->context = conn_id;
+ cm_id->cm_handler = cma_iw_handler;
+
+ sin = (struct sockaddr_in*)&new_cm_id->route.addr.src_addr;
+ *sin = iw_event->local_addr;
+
+ sin = (struct sockaddr_in*)&new_cm_id->route.addr.dst_addr;
+ *sin = iw_event->remote_addr;
+
+ ret = cma_notify_user(conn_id, RDMA_CM_EVENT_CONNECT_REQUEST, 0,
+ iw_event->private_data,
+ iw_event->private_data_len);
+ if (ret) {
+ /* Destroy the CM ID by returning a non-zero value. */
+ conn_id->cm_id.iw = NULL;
+ cma_exch(conn_id, CMA_DESTROYING);
+ cma_release_remove(conn_id);
+ rdma_destroy_id(&conn_id->id);
+ }
+
+out:
+ cma_release_remove(listen_id);
+ return ret;
+}
+
{snip}
+static int cma_connect_iw(struct rdma_id_private *id_priv,
+ struct rdma_conn_param *conn_param)
+{
+ struct iw_cm_id* cm_id;
+ struct sockaddr_in* sin;
+ int ret;
+
+ cm_id = iw_create_cm_id(id_priv->id.device, cma_iw_handler, id_priv);
+ if (IS_ERR(cm_id)) {
+ ret = PTR_ERR(cm_id);
+ goto out;
+ }
+
+ id_priv->cm_id.iw = cm_id;
+
+ sin = (struct sockaddr_in*)&id_priv->id.route.addr.src_addr;
+ cm_id->local_addr = *sin;
+
+ sin = (struct sockaddr_in*)&id_priv->id.route.addr.dst_addr;
+ cm_id->remote_addr = *sin;
+
+ cm_id->qp = id_priv->id.qp;
+ cm_id->qp_num = id_priv->qp_num;
It seems strange to me to set values field values in the cm_id in this way.
Maybe these should be parameters to iw_cm_connect()?
+
+ ret = iw_cm_connect(cm_id, conn_param->private_data,
+ conn_param->private_data_len);
+
+out:
+ return ret;
+}
{snip}
break;
+ case RDMA_TRANSPORT_IWARP:
+ id_priv->cm_id.iw->qp = id_priv->id.qp;
+ id_priv->cm_id.iw->qp_num = id_priv->qp_num;
+
+ ret = iw_cm_accept(id_priv->cm_id.iw, conn_param->private_data,
+ conn_param->private_data_len);
See comment above. Maybe add QP/QPN as parameters to iw_cm_accept()?
{snip}
+static void ucma_copy_iw_route(struct rdma_ucm_query_route_resp *resp,
+ struct rdma_route *route)
+{
+ struct rdma_dev_addr *dev_addr;
+
+ resp->num_paths = 0;
+ dev_addr = &route->addr.dev_addr;
+ memset(&resp->ib_route[0], 0, sizeof(resp->ib_route[0]));
+ memcpy(&resp->ib_route[0].dgid, iw_addr_get_dgid(dev_addr),
+ sizeof(union ib_gid));
+ memcpy(&resp->ib_route[0].sgid, iw_addr_get_sgid(dev_addr),
+ sizeof(union ib_gid));
Nit: spacing is off here. Also, please use sizeof resp->ib_route[0].dgid.
It's somewhat confusing to re-use ib_route to convey iWarp data. Can we use a
union here?
+ resp->ib_route[0].pkey = 0;
memset() should have cleared this already.
- Sean
_______________________________________________
openib-general mailing list
[email protected]
http://openib.org/mailman/listinfo/openib-general
To unsubscribe, please visit http://openib.org/mailman/listinfo/openib-general
