[PATCH] bonding cleanup 2.6 - Consolidate error handling in all xmit functions

drivers/net/bonding/bond_3ad.c

@@ -2360,32 +2360,18 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 	int i;
 	struct ad_info ad_info;
 
-	if (!IS_UP(dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
-	if (bond == NULL) {
-		printk(KERN_ERR DRV_NAME ": Error: bond is NULL on device %s\n", dev->name);
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
+	/* make sure that the slaves list will
+	 * not change during tx
+	 */
 	read_lock(&bond->lock);
 
-	/* check if bond is empty */
-	if (bond->slave_cnt == 0) {
-		printk(KERN_DEBUG DRV_NAME ": Error: bond is empty\n");
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
 	}
 
 	if (bond_3ad_get_active_agg_info(bond, &ad_info)) {
 		printk(KERN_DEBUG "ERROR: bond_3ad_get_active_agg_info failed\n");
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+		goto free_out;
 	}
 
 	slaves_in_agg = ad_info.ports;
@@ -2394,9 +2380,7 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 	if (slaves_in_agg == 0) {
 		/*the aggregator is empty*/
 		printk(KERN_DEBUG "ERROR: active aggregator is empty\n");
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+		goto free_out;
 	}
 
 	slave_agg_no = (data->h_dest[5]^bond->dev->dev_addr[5]) % slaves_in_agg;
@@ -2414,9 +2398,7 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 
 	if (slave_agg_no >= 0) {
 		printk(KERN_ERR DRV_NAME ": Error: Couldn't find a slave to tx on for aggregator ID %d\n", agg_id);
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+		goto free_out;
 	}
 
 	start_at = slave;
@@ -2434,15 +2416,17 @@ int bond_3ad_xmit_xor(struct sk_buff *skb, struct net_device *dev)
 			skb->dev = slave->dev;			
 			skb->priority = 1;
 			dev_queue_xmit(skb);
-			read_unlock(&bond->lock);
-			return 0;
 		}
 	}
 
-	/* no suitable interface, frame not sent */
-	dev_kfree_skb(skb);
+out:
 	read_unlock(&bond->lock);
 	return 0;
+
+free_out:
+	/* no suitable interface, frame not sent */
+	dev_kfree_skb(skb);
+	goto out;
 }
 
 int bond_3ad_lacpdu_recv(struct sk_buff *skb, struct net_device *dev, struct packet_type* ptype)

drivers/net/bonding/bond_alb.c

@@ -1158,25 +1158,16 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 	u32 hash_index = 0;
 	u8 *hash_start = NULL;
 
-	if (!IS_UP(bond_dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
 	/* make sure that the curr_active_slave and the slaves list do
 	 * not change during tx
 	 */
 	read_lock(&bond->lock);
+	read_lock(&bond->curr_slave_lock);
 
-	if (bond->slave_cnt == 0) {
-		/* no suitable interface, frame not sent */
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
 	}
 
-	read_lock(&bond->curr_slave_lock);
-
 	switch (ntohs(skb->protocol)) {
 	case ETH_P_IP:
 		if ((memcmp(eth_data->h_dest, mac_bcast, ETH_ALEN) == 0) ||
@@ -1256,12 +1247,17 @@ int bond_alb_xmit(struct sk_buff *skb, struct net_device *bond_dev)
 		if (tx_slave) {
 			tlb_clear_slave(bond, tx_slave, 0);
 		}
-		dev_kfree_skb(skb);
+		goto free_out;
 	}
 
+out:
 	read_unlock(&bond->curr_slave_lock);
 	read_unlock(&bond->lock);
 	return 0;
+
+free_out:
+	dev_kfree_skb(skb);
+	goto out;
 }
 
 void bond_alb_monitor(struct bonding *bond)

drivers/net/bonding/bond_main.c

@@ -2913,22 +2913,18 @@ static int bond_xmit_broadcast(struct sk_buff *skb, struct net_device *bond_dev)
 	struct net_device *tx_dev = NULL;
 	int i;
 
-	if (!IS_UP(bond_dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
 	read_lock(&bond->lock);
 
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
+	}
+	
 	read_lock(&bond->curr_slave_lock);
 	start_at = bond->curr_active_slave;
 	read_unlock(&bond->curr_slave_lock);
 
-	if (!start_at) { /* we're at the root, get the first slave */
-		/* no suitable interface, frame not sent */
-		read_unlock(&bond->lock);
-		dev_kfree_skb(skb);
-		return 0;
+	if (!start_at) {
+		goto free_out;
 	}
 
 	bond_for_each_slave_from(bond, slave, i, start_at) {
@@ -2957,12 +2953,18 @@ static int bond_xmit_broadcast(struct sk_buff *skb, struct net_device *bond_dev)
 		skb->priority = 1;
 		dev_queue_xmit(skb);
 	} else {
-		dev_kfree_skb(skb);
+		goto free_out;
 	}
 
+out:
 	/* frame sent to all suitable interfaces */
 	read_unlock(&bond->lock);
 	return 0;
+
+free_out:
+	/* no suitable interface, frame not sent */
+	dev_kfree_skb(skb);
+	goto out;
 }
 
 static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev)
@@ -2971,22 +2973,18 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 	struct slave *slave, *start_at;
 	int i;
 
-	if (!IS_UP(bond_dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
 	read_lock(&bond->lock);
 
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
+	}
+	
 	read_lock(&bond->curr_slave_lock);
 	slave = start_at = bond->curr_active_slave;
 	read_unlock(&bond->curr_slave_lock);
 
-	if (!slave) { /* we're at the root, get the first slave */
-		/* no suitable interface, frame not sent */
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+	if (!slave) {
+		goto free_out;
 	}
 
 	bond_for_each_slave_from(bond, slave, i, start_at) {
@@ -3002,15 +3000,18 @@ static int bond_xmit_roundrobin(struct sk_buff *skb, struct net_device *bond_dev
 			bond->curr_active_slave = slave->next;
 			write_unlock(&bond->curr_slave_lock);
 
-			read_unlock(&bond->lock);
-			return 0;
+			goto out;
 		}
 	}
 
-	/* no suitable interface, frame not sent */
-	dev_kfree_skb(skb);
+out:
 	read_unlock(&bond->lock);
 	return 0;
+
+free_out:
+	/* no suitable interface, frame not sent */
+	dev_kfree_skb(skb);
+	goto out;
 }
 
 /* 
@@ -3026,18 +3027,10 @@ static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
 	int slave_no;
 	int i;
 
-	if (!IS_UP(bond_dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
 	read_lock(&bond->lock);
 
-	if (bond->slave_cnt == 0) {
-		/* no suitable interface, frame not sent */
-		dev_kfree_skb(skb);
-		read_unlock(&bond->lock);
-		return 0;
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
 	}
 
 	slave_no = (data->h_dest[5]^bond_dev->dev_addr[5]) % bond->slave_cnt;
@@ -3060,15 +3053,18 @@ static int bond_xmit_xor(struct sk_buff *skb, struct net_device *bond_dev)
 			skb->priority = 1;
 			dev_queue_xmit(skb);
 
-			read_unlock(&bond->lock);
-			return 0;
+			goto out;
 		}
 	}
 
-	/* no suitable interface, frame not sent */
-	dev_kfree_skb(skb);
+out:
 	read_unlock(&bond->lock);
 	return 0;
+
+free_out:
+	/* no suitable interface, frame not sent */
+	dev_kfree_skb(skb);
+	goto out;
 }
 
 /* 
@@ -3079,11 +3075,6 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
 {
 	struct bonding *bond = (struct bonding *)bond_dev->priv;
 
-	if (!IS_UP(bond_dev)) { /* bond down */
-		dev_kfree_skb(skb);
-		return 0;
-	}
-
 	/* if we are sending arp packets, try to at least 
 	   identify our own ip address */
 	if (arp_interval && !my_ip &&
@@ -3096,24 +3087,29 @@ static int bond_xmit_activebackup(struct sk_buff *skb, struct net_device *bond_d
 	}
 
 	read_lock(&bond->lock);
-
 	read_lock(&bond->curr_slave_lock);
+
+	if (!BOND_IS_OK(bond)) {
+		goto free_out;
+	}
+	
 	if (bond->curr_active_slave) { /* one usable interface */
 		skb->dev = bond->curr_active_slave->dev;
-		read_unlock(&bond->curr_slave_lock);
 		skb->priority = 1;
 		dev_queue_xmit(skb);
-		read_unlock(&bond->lock);
-		return 0;
+		goto out;
 	} else {
-		read_unlock(&bond->curr_slave_lock);
+		goto free_out;
 	}
+out:
+	read_unlock(&bond->curr_slave_lock);
+	read_unlock(&bond->lock);
+	return 0;
 
+free_out:
 	/* no suitable interface, frame not sent */
-	dprintk("There was no suitable interface, so we don't transmit\n");
 	dev_kfree_skb(skb);
-	read_unlock(&bond->lock);
-	return 0;
+	goto out;
 }
 
 static struct net_device_stats *bond_get_stats(struct net_device *bond_dev)

drivers/net/bonding/bonding.h

@@ -45,18 +45,27 @@
 #define dprintk(fmt, args...)
 #endif /* BONDING_DEBUG */
 
-#define IS_UP(dev)  ((((dev)->flags & (IFF_UP)) == (IFF_UP)) && \
-		     (netif_running(dev) && netif_carrier_ok(dev)))
-
-/* Checks whether the dev is ready for transmit. We do not check netif_running
- * since a device can be stopped by the driver for short periods of time for
- * maintainance. dev_queue_xmit() handles this by queing the packet until the
- * the dev is running again. Keeping packets ordering requires sticking the
- * same dev as much as possible
+#define IS_UP(dev)					   \
+	      ((((dev)->flags & IFF_UP) == IFF_UP)	&& \
+	       netif_running(dev)			&& \
+	       netif_carrier_ok(dev))
+
+/*
+ * Checks whether bond is ready for transmit.
+ * 
+ * Caller must hold bond->lock
+ */
+#define BOND_IS_OK(bond)			     \
+		   (((bond)->dev->flags & IFF_UP) && \
+		    netif_running((bond)->dev)	  && \
+		    ((bond)->slave_cnt > 0))
+
+/*
+ * Checks whether slave is ready for transmit.
  */
-#define SLAVE_IS_OK(slave) \
-		     ((((slave)->dev->flags & (IFF_UP)) == (IFF_UP)) && \
-		     netif_carrier_ok((slave)->dev) && \
+#define SLAVE_IS_OK(slave)			        \
+		    (((slave)->dev->flags & IFF_UP)  && \
+		     netif_running((slave)->dev)     && \
 		     ((slave)->link == BOND_LINK_UP) && \
 		     ((slave)->state == BOND_STATE_ACTIVE))