Skip to contentSkip to conversation

infiniband/cxgb4: add null check

PATCH Applied Dan Carpenter 3 participants 6 messages May 31, 2010 – Jul 19, 2010 3 messages not in the archive

Subsystems (3)
Kernel janitors InfiniBand CXGB4 iWARP RNIC driver (IW_CXGB4)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 2/2 infiniband/cxgb4: add null check

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 <[email protected]>
drivers/infiniband/hw/cxgb4/cm.c +2 -1
index 60b5beb..9d11946 100644
@@ -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;
 	}
#1 lore raw

drivers/infiniband/hw/cxgb4/cm.c static void connect_reply_upcall(struct c4iw_ep *ep, int status) · line 790

Quoted 4 lines
drivers/infiniband/hw/cxgb4/cm.c +2 -1
 	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?

Signature · 2 lines
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

A message that is not in the archive (sent off-list or not yet received): adaeigpqepg.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org

#3 lore raw
Quoted 7 lines · Roland Dreier

drivers/infiniband/hw/cxgb4/cm.c static void connect_reply_upcall(struct c4iw_ep *ep, int status) · line 790

Quoted 4 lines
drivers/infiniband/hw/cxgb4/cm.c +2 -1
 	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...

A message that is not in the archive (sent off-list or not yet received): 4C07177C.5050502-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org

#5 lore raw
Quoted 4 lines

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);

?

Signature · 2 lines
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html

A message that is not in the archive (sent off-list or not yet received): adaaarcr8qo.fsf-BjVyx320WGW9gfZ95n9DRSW4+XlvGpQz@public.gmane.org

#7 lore raw
Quoted 13 lines · Roland Dreier
Quoted 4 lines

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);
	}
Quoted 8 lines

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.

#8 lore raw
In reply to Dan Carpenter

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 <[email protected]>
Signed-off-by Roland Dreier <[email protected]>
---
 drivers/infiniband/hw/cxgb4/cm.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/infiniband/hw/cxgb4/cm.c +5 -5
index b5e676c..4185c3b 100644
@@ -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;
Signature · 6 lines
1.7.1.1


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