[openib-general] [RFC] ib_mad

[Sean Hefty]

On Thu, 16 Sep 2004 15:29:45 -0400
Hal Rosenstock <halr at voltaire.com> wrote:

> Now that I think ib_mad is far enough along, I would like to request any
> code comments. 

This patch embeds struct ib_mad_agent within struct ib_mad_agent_private and fixes up some cleanup issues related to the MAD registration.

- Sean

Index: access/ib_mad_priv.h
===================================================================
--- access/ib_mad_priv.h	(revision 877)
+++ access/ib_mad_priv.h	(working copy)
@@ -100,7 +100,7 @@
 
 struct ib_mad_agent_private {
 	struct list_head agent_list;
-	struct ib_mad_agent *agent;
+	struct ib_mad_agent agent;
 	struct ib_mad_reg_req *reg_req;
 	u8 rmpp_version;
 };
Index: access/ib_mad.c
===================================================================
--- access/ib_mad.c	(revision 877)
+++ access/ib_mad.c	(working copy)
@@ -100,7 +100,7 @@
 					   void *context)
 {
 	struct ib_mad_port_private *entry, *port_priv = NULL;
-	struct ib_mad_agent *mad_agent, *ret;
+	struct ib_mad_agent *ret;
 	struct ib_mad_agent_private *mad_agent_priv;
 	struct ib_mad_reg_req *reg_req = NULL;
 	struct ib_mad_mgmt_class_table *class;
@@ -162,11 +162,10 @@
 	}
 
 	/* Allocate structures */
-	mad_agent = kmalloc(sizeof *mad_agent, GFP_KERNEL);
 	mad_agent_priv = kmalloc(sizeof *mad_agent_priv, GFP_KERNEL);
-	if (!mad_agent || !mad_agent_priv) { 
+	if (!mad_agent_priv) { 
 		ret = ERR_PTR(-ENOMEM);
-		goto error2;
+		goto error1;
 	}
 
 	if (mad_reg_req) {
@@ -203,38 +202,33 @@
 
 	/* Now, fill in the various structures */
 	memset(mad_agent_priv, 0, sizeof *mad_agent_priv);
-	mad_agent_priv->agent = mad_agent;
 	mad_agent_priv->reg_req = reg_req;
 	mad_agent_priv->rmpp_version = rmpp_version;
-	memset(mad_agent, 0, sizeof *mad_agent);
-	mad_agent->device = device;
-	mad_agent->recv_handler = recv_handler;
-	mad_agent->send_handler = send_handler;
-	mad_agent->context = context;
-	mad_agent->qp = port_priv->qp[qp_type];
-	mad_agent->hi_tid = ++ib_mad_client_id;
-
-	/* Add mad agent into agent list */
-	list_add_tail(&mad_agent_priv->agent_list, &port_priv->agent_list);
+	mad_agent_priv->agent.device = device;
+	mad_agent_priv->agent.recv_handler = recv_handler;
+	mad_agent_priv->agent.send_handler = send_handler;
+	mad_agent_priv->agent.context = context;
+	mad_agent_priv->agent.qp = port_priv->qp[qp_type];
+	mad_agent_priv->agent.hi_tid = ++ib_mad_client_id;
 
 	ret2 = add_mad_reg_req(mad_reg_req, mad_agent_priv);
 	if (ret2) {
+		spin_unlock_irqrestore(&port_priv->reg_lock, flags);
 		ret = ERR_PTR(ret2);	
 		goto error3;	
 	}
-	spin_unlock_irqrestore(&port_priv->reg_lock, flags);
 
-	return mad_agent;
+	/* Add mad agent into agent list */
+	list_add_tail(&mad_agent_priv->agent_list, &port_priv->agent_list);
 
-error3:
-	/* Remove mad agent from agent list */
-	list_del(&mad_agent_priv->agent_list);
 	spin_unlock_irqrestore(&port_priv->reg_lock, flags);
 
-	/* Release allocated structures */
-	kfree(reg_req);
+	return &mad_agent_priv->agent;
+
+error3:
+	if (reg_req)
+		kfree(reg_req);
 error2:
-	kfree(mad_agent);
 	kfree(mad_agent_priv);
 error1:
 	return ret;	
@@ -248,7 +242,6 @@
 {
 	struct ib_mad_port_private *entry;
 	struct ib_mad_agent_private *entry2, *temp;
-	int not_found = 1;
 	unsigned long flags, flags2;
 
 	/*
@@ -261,22 +254,23 @@
 		spin_lock_irqsave(&entry->reg_lock, flags2);
 		list_for_each_entry_safe(entry2, temp, 
 					 &entry->agent_list, agent_list) {
-			if (entry2->agent == mad_agent) {
+			if (&entry2->agent == mad_agent) {
 				remove_mad_reg_req(entry2);
 				list_del(&entry2->agent_list);
 
+				spin_unlock_irqrestore(&entry->reg_lock, flags2);	
+				spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
 				/* Release allocated structures */
-				kfree(entry2->reg_req);
-				kfree(entry2->agent);
+				if (entry2->reg_req)
+					kfree(entry2->reg_req);
 				kfree(entry2);
-				not_found = 0;
-				break;
+				return 0;
 			}
 		}
 		spin_unlock_irqrestore(&entry->reg_lock, flags2);	
 	}
 	spin_unlock_irqrestore(&ib_mad_port_list_lock, flags);
-	return not_found;
+	return 1;
 }
 EXPORT_SYMBOL(ib_unregister_mad_agent);
 
@@ -472,7 +466,7 @@
 	/* Make sure MAD registration request supplied */
 	if (!mad_reg_req)
 		return 0;
-	private = priv->agent->device->mad;
+	private = priv->agent.device->mad;
 	class = &private->version[mad_reg_req->mgmt_class_version];
 	mgmt_class = convert_mgmt_class(mad_reg_req->mgmt_class);
 	if (!*class) {
@@ -546,7 +540,7 @@
 		return;
 	}
 
-	port_priv = agent_priv->agent->device->mad;
+	port_priv = agent_priv->agent.device->mad;
 	class = port_priv->version[agent_priv->reg_req->mgmt_class_version];
 	if (!class) {
 		printk(KERN_ERR "No class table yet MAD registration request supplied\n");
@@ -629,7 +623,7 @@
 		/* Routing is based on high 32 bits of transaction ID of MAD  */
 		hi_tid = mad->mad_hdr.tid >> 32;
 		list_for_each_entry(entry, &port_priv->agent_list, agent_list) {
-			if (entry->agent->hi_tid == hi_tid) {
+			if (entry->agent.hi_tid == hi_tid) {
 				mad_agent = entry;
 				break;
 			}
@@ -763,8 +757,8 @@
 		}
 
 		/* Invoke receive callback */	
-		mad_agent->agent->recv_handler(mad_agent->agent,
-					       &recv->header.recv_wc);
+		mad_agent->agent.recv_handler(&mad_agent->agent,
+					      &recv->header.recv_wc);
 	}
 	spin_unlock_irqrestore(&port_priv->reg_lock, flags);