[openib-general] [PATCH] dont drop device reference before use (was Re: sdp: cant unload ib_ipoib module)

[Michael S. Tsirkin]

Quoting r. Hal Rosenstock <halr at voltaire.com>:
> Subject: Re: sdp: cant unload ib_ipoib module
> 
> Hi Michael,
> 
> On Tue, 2005-08-09 at 08:46, Michael S. Tsirkin wrote:
> > ip_rt_put now looks right, but it looks like device_put is still done too early.
> 
> Any idea where it should be done ?

The following should finally fix the dev_put issue.
I think an extra dev_hold on a path lookup is not a big deal, and
helps make the code much simpler. Works fine, for me.
Hal, can you give it a whirl?

Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>

Index: linux-2.6.12.2/drivers/infiniband/ulp/sdp/sdp_link.c
===================================================================
--- linux-2.6.12.2.orig/drivers/infiniband/ulp/sdp/sdp_link.c
+++ linux-2.6.12.2/drivers/infiniband/ulp/sdp/sdp_link.c
@@ -327,7 +327,7 @@ static void do_link_path_lookup(void *da
 {
 	struct sdp_path_info *info = data;
 	struct ipoib_dev_priv *priv;
-	struct net_device *loopback = NULL;
+	struct net_device *dev = NULL;
 	struct rtable *rt;
 	int counter = 0;
 	int result = 0;
@@ -387,7 +387,7 @@ static void do_link_path_lookup(void *da
 	 * check for IB device or loopback, the later requires extra
 	 * handling.
 	 */
-	if (ARPHRD_INFINIBAND != rt->u.dst.neighbour->dev->type &&
+	if (rt->u.dst.neighbour->dev->type != ARPHRD_INFINIBAND &&
 	    !(rt->u.dst.neighbour->dev->flags & IFF_LOOPBACK)) {
 		result = -ENETUNREACH;
 		goto error;
@@ -402,23 +402,28 @@ static void do_link_path_lookup(void *da
 	 * In case of loopback find a valid IB device on which to
 	 * direct the loopback traffic.
 	 */
-	info->dev = ((rt->u.dst.neighbour->dev->flags & IFF_LOOPBACK) ?
-		     (loopback = ip_dev_find(rt->rt_src)) :
-		     rt->u.dst.neighbour->dev);
+	if (rt->u.dst.neighbour->dev->flags & IFF_LOOPBACK)
+		dev = ip_dev_find(rt->rt_src);
+	else {
+		dev = rt->u.dst.neighbour->dev;
+		dev_hold(dev);
+	}
 
 	info->gw  = rt->rt_gateway;
 	info->src = rt->rt_src;     /* true source IP address */
 
-	if (info->dev->flags & IFF_LOOPBACK)
-		while ((info->dev = dev_get_by_index(++counter))) {
-
-			dev_put(info->dev);
-			if (ARPHRD_INFINIBAND == info->dev->type &&
-			    (info->dev->flags & IFF_UP))
+	if (dev->flags & IFF_LOOPBACK) {
+		dev_put(dev);
+		while ((dev = dev_get_by_index(++counter))) {
+			if (dev->type == ARPHRD_INFINIBAND &&
+			    (dev->flags & IFF_UP))
 				break;
+			else
+				dev_put(dev);
 		}
+	}
 
-	if (!info->dev) {
+	if (!dev) {
 		sdp_dbg_warn(NULL, "No device for IB comm <%s:%08x:%08x>",
 			     rt->u.dst.neighbour->dev->name,
 			     rt->u.dst.neighbour->dev->flags,
@@ -429,20 +434,20 @@ static void do_link_path_lookup(void *da
 	/*
 	 * lookup local info.
 	 */
-	priv = info->dev->priv;
+	priv = dev->priv;
 
 	info->ca             = priv->ca;
 	info->port           = priv->port;
 	info->path.pkey      = cpu_to_be16(priv->pkey);
 	info->path.numb_path = 1;
 
-	memcpy(&info->path.sgid, info->dev->dev_addr + 4, sizeof(union ib_gid));
+	memcpy(&info->path.sgid, dev->dev_addr + 4, sizeof(union ib_gid));
 	/*
 	 * If the routing device is loopback save the device address of
 	 * the IB device which was found.
 	 */
 	if (rt->u.dst.neighbour->dev->flags & IFF_LOOPBACK) {
-		memcpy(&info->path.dgid, info->dev->dev_addr + 4,
+		memcpy(&info->path.dgid, dev->dev_addr + 4,
 		       sizeof(union ib_gid));
 
 		goto path;
@@ -466,10 +471,10 @@ arp:
 	arp_send(ARPOP_REQUEST,
 		 ETH_P_ARP,
 		 info->gw,
-		 info->dev,
+		 dev,
 		 info->src,
 		 NULL,
-		 info->dev->dev_addr,
+		 dev->dev_addr,
 		 NULL);
 	/*
 	 * start arp timer if it's not already going.
@@ -498,8 +503,8 @@ arp:
 
 	info->flags |= SDP_LINK_F_ARP;
 	queue_delayed_work(link_wq, &info->timer, info->arp_time);
-	if (loopback)
-		dev_put(loopback);
+	if (dev)
+		dev_put(dev);
 	ip_rt_put(rt);
 	return;
 path:
@@ -509,14 +514,14 @@ path:
 		goto error;
 	}
 done:
-	if (loopback)
-		dev_put(loopback);
+	if (dev)
+		dev_put(dev);
 	ip_rt_put(rt);
 	return;
 error:
 	sdp_path_info_destroy(info, result);
-	if (loopback)
-		dev_put(loopback);
+	if (dev)
+		dev_put(dev);
 	ip_rt_put(rt);
 }
 
@@ -690,7 +695,7 @@ static int sdp_link_arp_recv(struct sk_b
 
 	arp_hdr = (struct arphdr *)skb->nh.raw;
 
-	if (ARPHRD_INFINIBAND != dev->type ||
+	if (dev->type != ARPHRD_INFINIBAND ||
 	    (arp_hdr->ar_op != __constant_htons(ARPOP_REPLY) &&
 	     arp_hdr->ar_op != __constant_htons(ARPOP_REQUEST)))
 		goto done;
Index: linux-2.6.12.2/drivers/infiniband/ulp/sdp/sdp_link.h
===================================================================
--- linux-2.6.12.2.orig/drivers/infiniband/ulp/sdp/sdp_link.h
+++ linux-2.6.12.2/drivers/infiniband/ulp/sdp/sdp_link.h
@@ -59,8 +59,6 @@ struct sdp_path_info {
 
 	struct work_struct timer;   /* arp request timers. */
 
-	struct net_device     *dev; /* ipoib device */
-
 	struct sdp_path_info  *next; /* next element in path list. */
 	struct sdp_path_info **pext; /* previous next element in path list */
 


-- 
MST