[patch 2/2] infiniband/cxgb4: add null check

[patch 2/2] infiniband/cxgb4: add null check

[Dan Carpenter]

Earlier in the function we assume that ep->com.cm_id can be null.  I
looked through the code briefly but couldn't tell if it actually is ever
null or not.  Adding a null check here seemed like a cautious thing to
do.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index 60b5beb..9d11946 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -786,7 +786,8 @@ static void connect_reply_upcall(struct c4iw_ep *ep, int status)
 		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
 	}
 	if (status < 0) {
-		ep->com.cm_id->rem_ref(ep->com.cm_id);
+		if (ep->com.cm_id)
+			ep->com.cm_id->rem_ref(ep->com.cm_id);
 		ep->com.cm_id = NULL;
 		ep->com.qp = NULL;
 	}

Re: [patch 2/2] infiniband/cxgb4: add null check

[Roland Dreier]

 >  	if (status < 0) {
 > -		ep->com.cm_id->rem_ref(ep->com.cm_id);
 > +		if (ep->com.cm_id)
 > +			ep->com.cm_id->rem_ref(ep->com.cm_id);

Steve, does this make sense?
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [patch 2/2] infiniband/cxgb4: add null check

[Steve Wise]

Roland Dreier wrote:
>  >  	if (status < 0) {
>  > -		ep->com.cm_id->rem_ref(ep->com.cm_id);
>  > +		if (ep->com.cm_id)
>  > +			ep->com.cm_id->rem_ref(ep->com.cm_id);
>
> Steve, does this make sense?
>   
I actually think that ep->com.cm_id is always valid when 
connect_reply_upcall() is called.  But I'd have to test it more.  I'd 
rather not add this change unless someone convinces me that there 
actually is a path where the cm_id null...

Re: [patch 2/2] infiniband/cxgb4: add null check

[Roland Dreier]

 > I actually think that ep->com.cm_id is always valid when
 > connect_reply_upcall() is called.  But I'd have to test it more.  I'd
 > rather not add this change unless someone convinces me that there
 > actually is a path where the cm_id null...

Then would it make sense to change the code

	if (ep->com.cm_id) {
		PDBG("%s ep %p tid %u status %d\n", __func__, ep,
		     ep->hwtid, status);
		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
	}

to just

	PDBG("%s ep %p tid %u status %d\n", __func__, ep,
	     ep->hwtid, status);
	ep->com.cm_id->event_handler(ep->com.cm_id, &event);

?
-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

Re: [patch 2/2] infiniband/cxgb4: add null check

[Steve Wise]

Roland Dreier wrote:
>  > I actually think that ep->com.cm_id is always valid when
>  > connect_reply_upcall() is called.  But I'd have to test it more.  I'd
>  > rather not add this change unless someone convinces me that there
>  > actually is a path where the cm_id null...
>
> Then would it make sense to change the code
>
> 	if (ep->com.cm_id) {
> 		PDBG("%s ep %p tid %u status %d\n", __func__, ep,
> 		     ep->hwtid, status);
> 		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
> 	}
>
> to just
>
> 	PDBG("%s ep %p tid %u status %d\n", __func__, ep,
> 	     ep->hwtid, status);
> 	ep->com.cm_id->event_handler(ep->com.cm_id, &event);
>
> ?
>   


Yes.  I haven't tested it though.    But if you wanna push it, then I'll 
test it here soon.

Steve.

Re: [patch 2/2] infiniband/cxgb4: add null check

[Roland Dreier]

Since we agreed the NULL check probably wasn't needed, I added this:

[PATCH] RDMA/cxgb4: Remove unneeded NULL check

The rest of the code seems to assume that ep->com.cm_id can't be NULL,
so remove an unneeded test.

Reported-by: Dan Carpenter <error27@gmail.com>
Signed-off-by: Roland Dreier <rolandd@cisco.com>
---
 drivers/infiniband/hw/cxgb4/cm.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cm.c b/drivers/infiniband/hw/cxgb4/cm.c
index b5e676c..4185c3b 100644
--- a/drivers/infiniband/hw/cxgb4/cm.c
+++ b/drivers/infiniband/hw/cxgb4/cm.c
@@ -780,11 +780,11 @@ static void connect_reply_upcall(struct c4iw_ep *ep, int status)
 		event.private_data_len = ep->plen;
 		event.private_data = ep->mpa_pkt + sizeof(struct mpa_message);
 	}
-	if (ep->com.cm_id) {
-		PDBG("%s ep %p tid %u status %d\n", __func__, ep,
-		     ep->hwtid, status);
-		ep->com.cm_id->event_handler(ep->com.cm_id, &event);
-	}
+
+	PDBG("%s ep %p tid %u status %d\n", __func__, ep,
+	     ep->hwtid, status);
+	ep->com.cm_id->event_handler(ep->com.cm_id, &event);
+
 	if (status < 0) {
 		ep->com.cm_id->rem_ref(ep->com.cm_id);
 		ep->com.cm_id = NULL;
-- 
1.7.1.1


-- 
Roland Dreier <rolandd@cisco.com> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html