[openib-general] [PATCH] Extra kfrees, clean up unregisters, etc ...

[Krishna Kumar]

1. Don't kfree sa_dev twice.
2. Unnecessary kref_put : In failure case, we don't seem to have a
   reference until update_sm_ah is called in success case.
3. Clean up code which looks like a hack (i++ in failure).
4. Too many "i -s", no need to keep recalculating this :-)


5. Potential extra cleanup : I could have set index = 0 instead of
   index = i - s, I kept it this way to be quite identical to existing
   code.

Patch applies with -p1 on trunk directory, on top of my previous patch.

Thanks,

- KK

diff -ruNp trunk/src/linux-kernel/infiniband/core/sa_query.c.org trunk/src/linux-kernel/infiniband/core/sa_query.c
--- trunk/src/linux-kernel/infiniband/core/sa_query.c.org	2004-11-05 11:51:06.000000000 -0800
+++ trunk/src/linux-kernel/infiniband/core/sa_query.c	2004-11-05 13:10:50.000000000 -0800
@@ -682,6 +682,7 @@ static void ib_sa_add_one(struct ib_devi
 {
 	struct ib_sa_device *sa_dev;
 	int s, e, i;
+	int index;

 	if (device->node_type == IB_NODE_SWITCH)
 		s = e = 0;
@@ -703,29 +704,29 @@ static void ib_sa_add_one(struct ib_devi
 	sa_dev->start_port = s;
 	sa_dev->end_port   = e;

-	for (i = s; i <= e; ++i) {
-		sa_dev->port[i - s].mr       = NULL;
-		sa_dev->port[i - s].sm_ah    = NULL;
-		sa_dev->port[i - s].port_num = i;
-		spin_lock_init(&sa_dev->port[i - s].ah_lock);
+	for (i = s, index = i - s; i <= e; ++i, ++index) {
+		sa_dev->port[index].mr       = NULL;
+		sa_dev->port[index].sm_ah    = NULL;
+		sa_dev->port[index].port_num = i;
+		spin_lock_init(&sa_dev->port[index].ah_lock);

-		sa_dev->port[i - s].agent =
+		sa_dev->port[index].agent =
 			ib_register_mad_agent(device, i, IB_QPT_GSI,
 					      NULL, 0, send_handler,
 					      recv_handler, sa_dev);
-		if (IS_ERR(sa_dev->port[i - s].agent))
+		if (IS_ERR(sa_dev->port[index].agent))
 			goto err;

-		sa_dev->port[i - s].mr = ib_get_dma_mr(sa_dev->port[i - s].agent->qp->pd,
-						       IB_ACCESS_LOCAL_WRITE);
-		if (IS_ERR(sa_dev->port[i - s].mr)) {
-			/* Bump i so agent from this iter. is freed */
-			++i;
+		sa_dev->port[index].mr =
+				ib_get_dma_mr(sa_dev->port[index].agent->qp->pd,
+				IB_ACCESS_LOCAL_WRITE);
+		if (IS_ERR(sa_dev->port[index].mr)) {
+			ib_unregister_mad_agent(sa_dev->port[index].agent);
 			goto err;
 		}

-		INIT_WORK(&sa_dev->port[i - s].update_task,
-			  update_sm_ah, &sa_dev->port[i - s]);
+		INIT_WORK(&sa_dev->port[index].update_task,
+			  update_sm_ah, &sa_dev->port[index]);
 	}

 	/*
@@ -736,27 +737,20 @@ static void ib_sa_add_one(struct ib_devi
 	 */

 	INIT_IB_EVENT_HANDLER(&sa_dev->event_handler, device, ib_sa_event);
-	if (ib_register_event_handler(&sa_dev->event_handler)) {
-		kfree(sa_dev);
+	if (ib_register_event_handler(&sa_dev->event_handler))
 		goto err;
-	}

-	for (i = s; i <= e; ++i)
-		update_sm_ah(&sa_dev->port[i - s]);
+	while (--index >= 0)
+		update_sm_ah(&sa_dev->port[index]);

 	ib_set_client_data(device, &sa_client, sa_dev);

 	return;

 err:
-	while (--i >= s) {
-		if (sa_dev->port[i - s].mr && !IS_ERR(sa_dev->port[i - s].mr))
-			ib_dereg_mr(sa_dev->port[i - s].mr);
-
-		if (sa_dev->port[i - s].sm_ah)
-			kref_put(&sa_dev->port[i].sm_ah->ref, free_sm_ah);
-
-		ib_unregister_mad_agent(sa_dev->port[i - s].agent);
+	while (--index >= 0) {
+		ib_dereg_mr(sa_dev->port[index].mr);
+		ib_unregister_mad_agent(sa_dev->port[index].agent);
 	}

 	kfree(sa_dev);