Questions regarding CMA

Questions regarding CMA

[Jack Morgenstein]

I am currently reviewing/cleaning up our CMA LAP patches for submission.

I have several questions regarding the file cma.c, which I would like you to clarify for me.

1. In procedure cma_ib_listen (and elsewhere), if the call to ib_create_cm_id fails,
   id_priv->cm_id.ib is left with the (non-zero) error value instead of NULL.
   However, if there is a failure later in the procedure, you set id_priv->cm_id.ib to NULL.

   Is there a reason for this difference in behavior? 
   (code snippet from current code is below)
	int cma_ib_listen(struct rdma_id_

       		id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
                                            	    id_priv);
	       	if (IS_ERR(id_priv->cm_id.ib))
               		return PTR_ERR(id_priv->cm_id.ib);

		....
	        if (ret) {
                	ib_destroy_cm_id(id_priv->cm_id.ib);
                	id_priv->cm_id.ib = NULL;
        	}

   
2. procedure cma_has_cm_dev looks as follows:

	static int cma_has_cm_dev(struct rdma_id_private *id_priv)
	{
		return (id_priv->id.device && id_priv->cm_id.ib);
	}

   Shouldn't the line be:
		return (id_priv->id.device && id_priv->cm_id.ib &&
			!IS_ERR(id_priv->cm_id.ib));

3. There are several places where the value of id_priv->cm_id.ib
   is checked to be not NULL.

   Wouldn't it be better to call cma_has_cm_dev in these places
   (when cma_has_cm_dev has been fixed, as I suggested in 2 above).
		
Please consider the patch below as a starting point (I did not touch the iwarp code).
Please let me know (ASAP) what you think.
(I still leave a window here where id_priv->cm_id.ib is ERR. Is this a problem?
 Would it be better to use a local variable instead of id_priv->cm_id.ib, and
 only assign to id_priv->cm_id.ib when all the error checks have passed?
 This would leave a window where a successful cm_id creation is not immediately assigned
 to the CMA object -- would this be a problem?).

Thanks!

-Jack
====================================================================
--- cma.c	2011-07-13 09:54:09.000000000 +0300
+++ cma_fixed.c	2011-07-14 10:51:23.000000000 +0300
@@ -424,7 +424,8 @@ static int cma_disable_callback(struct r
 
 static int cma_has_cm_dev(struct rdma_id_private *id_priv)
 {
-	return (id_priv->id.device && id_priv->cm_id.ib);
+	return (id_priv->id.device && id_priv->cm_id.ib &&
+		!IS_ERR(id_priv->cm_id.ib));
 }
 
 struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
@@ -658,7 +659,7 @@ int rdma_init_qp_attr(struct rdma_cm_id 
 	id_priv = container_of(id, struct rdma_id_private, id);
 	switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
 	case RDMA_TRANSPORT_IB:
-		if (!id_priv->cm_id.ib || cma_is_ud_ps(id_priv->id.ps))
+		if (!cma_has_cm_dev(id_priv) || cma_is_ud_ps(id_priv->id.ps))
 			ret = cma_ib_init_qp_attr(id_priv, qp_attr, qp_attr_mask);
 		else
 			ret = ib_cm_init_qp_attr(id_priv->cm_id.ib, qp_attr,
@@ -918,7 +919,7 @@ void rdma_destroy_id(struct rdma_cm_id *
 	if (id_priv->cma_dev) {
 		switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
 		case RDMA_TRANSPORT_IB:
-			if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib))
+			if (cma_has_cm_dev(id_priv))
 				ib_destroy_cm_id(id_priv->cm_id.ib);
 			break;
 		case RDMA_TRANSPORT_IWARP:
@@ -1471,8 +1472,10 @@ static int cma_ib_listen(struct rdma_id_
 
 	id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
 					    id_priv);
-	if (IS_ERR(id_priv->cm_id.ib))
-		return PTR_ERR(id_priv->cm_id.ib);
+	if (IS_ERR(id_priv->cm_id.ib)) {
+		ret = PTR_ERR(id_priv->cm_id.ib);
+		goto out;
+	}
 
 	addr = (struct sockaddr *) &id_priv->id.route.addr.src_addr;
 	svc_id = cma_get_service_id(id_priv->id.ps, addr);
@@ -1482,9 +1485,10 @@ static int cma_ib_listen(struct rdma_id_
 		cma_set_compare_data(id_priv->id.ps, addr, &compare_data);
 		ret = ib_cm_listen(id_priv->cm_id.ib, svc_id, 0, &compare_data);
 	}
-
+out:
 	if (ret) {
-		ib_destroy_cm_id(id_priv->cm_id.ib);
+		if (!IS_ERR(id_priv->cm_id.ib))
+			ib_destroy_cm_id(id_priv->cm_id.ib);
 		id_priv->cm_id.ib = NULL;
 	}
 
@@ -2454,11 +2458,12 @@ static int cma_resolve_ib_udp(struct rdm
 	req.max_cm_retries = CMA_MAX_CM_RETRIES;
 
 	ret = ib_send_cm_sidr_req(id_priv->cm_id.ib, &req);
+out:
 	if (ret) {
-		ib_destroy_cm_id(id_priv->cm_id.ib);
+		if (!IS_ERR(id_priv->cm_id.ib))
+			ib_destroy_cm_id(id_priv->cm_id.ib);
 		id_priv->cm_id.ib = NULL;
 	}
-out:
 	kfree(req.private_data);
 	return ret;
 }
@@ -2516,8 +2521,9 @@ static int cma_connect_ib(struct rdma_id
 
 	ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
 out:
-	if (ret && !IS_ERR(id_priv->cm_id.ib)) {
-		ib_destroy_cm_id(id_priv->cm_id.ib);
+	if (ret) {
+	      	if (!IS_ERR(id_priv->cm_id.ib))
+			ib_destroy_cm_id(id_priv->cm_id.ib);
 		id_priv->cm_id.ib = NULL;
 	}
 
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: Questions regarding CMA

[Hefty, Sean]

> 1. In procedure cma_ib_listen (and elsewhere), if the call to ib_create_cm_id
> fails,
>    id_priv->cm_id.ib is left with the (non-zero) error value instead of NULL.
>    However, if there is a failure later in the procedure, you set id_priv-
> >cm_id.ib to NULL.
> 
>    Is there a reason for this difference in behavior?
>    (code snippet from current code is below)
> 	int cma_ib_listen(struct rdma_id_
> 
>        		id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device,
> cma_req_handler,
>                                             	    id_priv);
> 	       	if (IS_ERR(id_priv->cm_id.ib))
>                		return PTR_ERR(id_priv->cm_id.ib);

This looks like a bug.
 
> 
> 		....
> 	        if (ret) {
>                 	ib_destroy_cm_id(id_priv->cm_id.ib);
>                 	id_priv->cm_id.ib = NULL;
>         	}
> 
> 
> 2. procedure cma_has_cm_dev looks as follows:
> 
> 	static int cma_has_cm_dev(struct rdma_id_private *id_priv)
> 	{
> 		return (id_priv->id.device && id_priv->cm_id.ib);
> 	}
> 
>    Shouldn't the line be:
> 		return (id_priv->id.device && id_priv->cm_id.ib &&
> 			!IS_ERR(id_priv->cm_id.ib));

Given the current code, adding IS_ERR makes sense, but see below.  Thinking about this, we shouldn't need to check id_priv->id.device.  If we have id_priv->cm_id.ib, then the device pointer must be valid.

(fyi: cma_has_cm_dev() was added by commit 6c719f5c6c823901fac2d46b83db5a69ba7e9152.  It replaced a state check with the above device check to handle device removal.)

> 3. There are several places where the value of id_priv->cm_id.ib
>    is checked to be not NULL.
> 
>    Wouldn't it be better to call cma_has_cm_dev in these places
>    (when cma_has_cm_dev has been fixed, as I suggested in 2 above).

Although it's a minor performance gain, I'm leaning towards keeping id_priv->cm_id.ib = NULL on failure, either by resetting it or using a local variable.  cma_has_cm_dev() can then be replaced by checking id_priv->cm_id.ib for non-NULL, and checks for IS_ERR(id_priv->cm_id.ib) are removed.

> Please consider the patch below as a starting point (I did not touch the iwarp
> code).
> Please let me know (ASAP) what you think.
> (I still leave a window here where id_priv->cm_id.ib is ERR. Is this a
> problem?
>  Would it be better to use a local variable instead of id_priv->cm_id.ib, and
>  only assign to id_priv->cm_id.ib when all the error checks have passed?
>  This would leave a window where a successful cm_id creation is not
> immediately assigned
>  to the CMA object -- would this be a problem?).

Without adding more synchronization, we need to ensure that id_priv->cm_id.ib is set before a user can receive any callbacks.  This restricts our use of a local variable to the create_cm_id calls only.  See cma_connect_iw() for an example of using this approach.

Thanks,
- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Questions regarding CMA

[Jack Morgenstein]

On Thursday 14 July 2011 20:46, Hefty, Sean wrote:
> Although it's a minor performance gain, I'm leaning towards keeping id_priv->cm_id.ib = NULL on failure, either by resetting it or using a local variable.  cma_has_cm_dev() can then be replaced by checking id_priv->cm_id.ib for non-NULL, and checks for IS_ERR(id_priv->cm_id.ib) are removed.
> 
> > Please consider the patch below as a starting point (I did not touch the iwarp
> > code).
> > Please let me know (ASAP) what you think.
> > (I still leave a window here where id_priv->cm_id.ib is ERR. Is this a
> > problem?
> >  Would it be better to use a local variable instead of id_priv->cm_id.ib, and
> >  only assign to id_priv->cm_id.ib when all the error checks have passed?
> >  This would leave a window where a successful cm_id creation is not
> > immediately assigned
> >  to the CMA object -- would this be a problem?).
> 
> Without adding more synchronization, we need to ensure that id_priv->cm_id.ib is set before a user can receive any callbacks.
> This restricts our use of a local variable to the create_cm_id calls only.
> See cma_connect_iw() for an example of using this approach.  
> 
Agreed.  That way we never have to test for IS_ERR. I'll send a patch.

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object

[Jack Morgenstein]

Avoid assigning an IS_ERR value to the cm id pointer in the cma_id object.
This fixes a few anomalies in the error flow, and eliminates the need to
test for the IS_ERR value every time we wish to determine if the cma_id object
has a cm device associated with it.

Also, eliminate the now-unnecessary procedure cma_has_cm_dev (we can check directly
for the existence of the device pointer -- for a non-NULL check, makes no difference
if it is the iwarp or the ib pointer).

Finally, I suggest a few code changes here to improve coding consistency.

Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>

---
Sean,
Here is the patch I said I would submit to you.
I got rid of the cma_has_cm_dev() procedure, but if you think that it should be kept,
but simply do the ib-pointer check only (to keep ib out of the rdma level), that is OK with me.

Jack

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index b6a33b3..85ce489 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -406,11 +406,6 @@ static int cma_disable_callback(struct rdma_id_private *id_priv,
 	return 0;
 }
 
-static int cma_has_cm_dev(struct rdma_id_private *id_priv)
-{
-	return (id_priv->id.device && id_priv->cm_id.ib);
-}
-
 struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
 				  void *context, enum rdma_port_space ps,
 				  enum ib_qp_type qp_type)
@@ -920,11 +915,11 @@ void rdma_destroy_id(struct rdma_cm_id *id)
 	if (id_priv->cma_dev) {
 		switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
 		case RDMA_TRANSPORT_IB:
-			if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib))
+			if (id_priv->cm_id.ib)
 				ib_destroy_cm_id(id_priv->cm_id.ib);
 			break;
 		case RDMA_TRANSPORT_IWARP:
-			if (id_priv->cm_id.iw && !IS_ERR(id_priv->cm_id.iw))
+			if (id_priv->cm_id.iw)
 				iw_destroy_cm_id(id_priv->cm_id.iw);
 			break;
 		default:
@@ -1085,12 +1080,12 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 
 	if (cma_get_net_info(ib_event->private_data, listen_id->ps,
 			     &ip_ver, &port, &src, &dst))
-		goto err;
+		return NULL;
 
 	id = rdma_create_id(listen_id->event_handler, listen_id->context,
 			    listen_id->ps, ib_event->param.req_rcvd.qp_type);
 	if (IS_ERR(id))
-		goto err;
+		return NULL;
 
 	cma_save_net_info(&id->route.addr, &listen_id->route.addr,
 			  ip_ver, port, src, dst);
@@ -1100,7 +1095,7 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 	rt->path_rec = kmalloc(sizeof *rt->path_rec * rt->num_paths,
 			       GFP_KERNEL);
 	if (!rt->path_rec)
-		goto destroy_id;
+		goto err;
 
 	rt->path_rec[0] = *ib_event->param.req_rcvd.primary_path;
 	if (rt->num_paths == 2)
@@ -1114,7 +1109,7 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 		ret = rdma_translate_ip((struct sockaddr *) &rt->addr.src_addr,
 					&rt->addr.dev_addr);
 		if (ret)
-			goto destroy_id;
+			goto err;
 	}
 	rdma_addr_set_dgid(&rt->addr.dev_addr, &rt->path_rec[0].dgid);
 
@@ -1122,9 +1117,8 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
 	id_priv->state = RDMA_CM_CONNECT;
 	return id_priv;
 
-destroy_id:
-	rdma_destroy_id(id);
 err:
+	rdma_destroy_id(id);
 	return NULL;
 }
 
@@ -1468,13 +1462,15 @@ static int cma_ib_listen(struct rdma_id_private *id_priv)
 {
 	struct ib_cm_compare_data compare_data;
 	struct sockaddr *addr;
+	struct ib_cm_id	*id;
 	__be64 svc_id;
 	int ret;
 
-	id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
-					    id_priv);
-	if (IS_ERR(id_priv->cm_id.ib))
-		return PTR_ERR(id_priv->cm_id.ib);
+	id = ib_create_cm_id(id_priv->id.device, cma_req_handler, id_priv);
+	if (IS_ERR(id))
+		return PTR_ERR(id);
+
+	id_priv->cm_id.ib = id;
 
 	addr = (struct sockaddr *) &id_priv->id.route.addr.src_addr;
 	svc_id = cma_get_service_id(id_priv->id.ps, addr);
@@ -1497,12 +1493,15 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog)
 {
 	int ret;
 	struct sockaddr_in *sin;
+	struct iw_cm_id	*id;
 
-	id_priv->cm_id.iw = iw_create_cm_id(id_priv->id.device,
-					    iw_conn_req_handler,
-					    id_priv);
-	if (IS_ERR(id_priv->cm_id.iw))
-		return PTR_ERR(id_priv->cm_id.iw);
+	id = iw_create_cm_id(id_priv->id.device,
+			     iw_conn_req_handler,
+			     id_priv);
+	if (IS_ERR(id))
+		return PTR_ERR(id);
+
+	id_priv->cm_id.iw = id;
 
 	sin = (struct sockaddr_in *) &id_priv->id.route.addr.src_addr;
 	id_priv->cm_id.iw->local_addr = *sin;
@@ -2484,6 +2483,7 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
 {
 	struct ib_cm_sidr_req_param req;
 	struct rdma_route *route;
+	struct ib_cm_id	*id;
 	int ret;
 
 	req.private_data_len = sizeof(struct cma_hdr) +
@@ -2501,12 +2501,13 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
 	if (ret)
 		goto out;
 
-	id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device,
-					    cma_sidr_rep_handler, id_priv);
-	if (IS_ERR(id_priv->cm_id.ib)) {
-		ret = PTR_ERR(id_priv->cm_id.ib);
+	id = ib_create_cm_id(id_priv->id.device, cma_sidr_rep_handler,
+			     id_priv);
+	if (IS_ERR(id)) {
+		ret = PTR_ERR(id);
 		goto out;
 	}
+	id_priv->cm_id.ib = id;
 
 	req.path = route->path_rec;
 	req.service_id = cma_get_service_id(id_priv->id.ps,
@@ -2530,6 +2531,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 	struct ib_cm_req_param req;
 	struct rdma_route *route;
 	void *private_data;
+	struct ib_cm_id	*id;
 	int offset, ret;
 
 	memset(&req, 0, sizeof req);
@@ -2543,12 +2545,12 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 		memcpy(private_data + offset, conn_param->private_data,
 		       conn_param->private_data_len);
 
-	id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_ib_handler,
-					    id_priv);
-	if (IS_ERR(id_priv->cm_id.ib)) {
-		ret = PTR_ERR(id_priv->cm_id.ib);
+	id = ib_create_cm_id(id_priv->id.device, cma_ib_handler, id_priv);
+	if (IS_ERR(id)) {
+		ret = PTR_ERR(id);
 		goto out;
 	}
+	id_priv->cm_id.ib = id;
 
 	route = &id_priv->id.route;
 	ret = cma_format_hdr(private_data, id_priv->id.ps, route);
@@ -2577,7 +2579,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
 
 	ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
 out:
-	if (ret && !IS_ERR(id_priv->cm_id.ib)) {
+	if (ret && !IS_ERR(id)) {
 		ib_destroy_cm_id(id_priv->cm_id.ib);
 		id_priv->cm_id.ib = NULL;
 	}
@@ -2595,10 +2597,8 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
 	struct iw_cm_conn_param iw_param;
 
 	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;
-	}
+	if (IS_ERR(cm_id))
+		return PTR_ERR(cm_id);
 
 	id_priv->cm_id.iw = cm_id;
 
@@ -2622,7 +2622,7 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
 		iw_param.qpn = conn_param->qp_num;
 	ret = iw_cm_connect(cm_id, &iw_param);
 out:
-	if (ret && !IS_ERR(cm_id)) {
+	if (ret) {
 		iw_destroy_cm_id(cm_id);
 		id_priv->cm_id.iw = NULL;
 	}
@@ -2795,7 +2795,7 @@ int rdma_notify(struct rdma_cm_id *id, enum ib_event_type event)
 	int ret;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
-	if (!cma_has_cm_dev(id_priv))
+	if (!id_priv->cm_id.ib)
 		return -EINVAL;
 
 	switch (id->device->node_type) {
@@ -2817,7 +2817,7 @@ int rdma_reject(struct rdma_cm_id *id, const void *private_data,
 	int ret;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
-	if (!cma_has_cm_dev(id_priv))
+	if (!id_priv->cm_id.ib)
 		return -EINVAL;
 
 	switch (rdma_node_get_transport(id->device->node_type)) {
@@ -2848,7 +2848,7 @@ int rdma_disconnect(struct rdma_cm_id *id)
 	int ret;
 
 	id_priv = container_of(id, struct rdma_id_private, id);
-	if (!cma_has_cm_dev(id_priv))
+	if (!id_priv->cm_id.ib)
 		return -EINVAL;
 
 	switch (rdma_node_get_transport(id->device->node_type)) {
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object

[Hefty, Sean]

> Avoid assigning an IS_ERR value to the cm id pointer in the cma_id object.
> This fixes a few anomalies in the error flow, and eliminates the need to
> test for the IS_ERR value every time we wish to determine if the cma_id object
> has a cm device associated with it.
> 
> Also, eliminate the now-unnecessary procedure cma_has_cm_dev (we can check
> directly
> for the existence of the device pointer -- for a non-NULL check, makes no
> difference
> if it is the iwarp or the ib pointer).
> 
> Finally, I suggest a few code changes here to improve coding consistency.
> 
> Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
> 
> ---
> Sean,
> Here is the patch I said I would submit to you.
> I got rid of the cma_has_cm_dev() procedure, but if you think that it should
> be kept,
> but simply do the ib-pointer check only (to keep ib out of the rdma level),
> that is OK with me.

Thanks for doing this.  The patch looks good to me with one nit:

> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index b6a33b3..85ce489 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c

> @@ -2577,7 +2579,7 @@ static int cma_connect_ib(struct rdma_id_private
> *id_priv,
> 
>  	ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
>  out:
> -	if (ret && !IS_ERR(id_priv->cm_id.ib)) {
> +	if (ret && !IS_ERR(id)) {
>  		ib_destroy_cm_id(id_priv->cm_id.ib);

I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object

[Roland Dreier]

On Mon, Jul 18, 2011 at 9:35 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> -     if (ret && !IS_ERR(id_priv->cm_id.ib)) {
>> +     if (ret && !IS_ERR(id)) {
>>               ib_destroy_cm_id(id_priv->cm_id.ib);
>
> I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.

OK, I'll grab this and make that suggested change as I merge it... no
need to resend.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object

[Jack Morgenstein]

On Monday 18 July 2011 19:39, Roland Dreier wrote:
> On Mon, Jul 18, 2011 at 9:35 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >> -     if (ret && !IS_ERR(id_priv->cm_id.ib)) {
> >> +     if (ret && !IS_ERR(id)) {
> >>               ib_destroy_cm_id(id_priv->cm_id.ib);
> >
> > I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.
> 
> OK, I'll grab this and make that suggested change as I merge it... no
> need to resend.
> 
>  - R.
Thanks, Roland!

-Jack
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html