[PATCH 0/6] bonding related patches

[PATCH 0/6] bonding related patches

[Stephen Hemminger]

These are some non-urgent ehancements and cleanups to bonding.

-- 

[PATCH 1/6] bonding: allow configuration without sysfs

[Stephen Hemminger]

[-- Attachment #1: bonding-no-sysfs.patch --]
[-- Type: text/plain, Size: 3348 bytes --]

In embedded environments sysfs is optional, so stub out and remove
code if not needed.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 drivers/net/bonding/Makefile    |    5 ++++-
 drivers/net/bonding/bond_main.c |    2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

--- a/drivers/net/bonding/Makefile	2008-10-09 14:05:35.000000000 +0200
+++ b/drivers/net/bonding/Makefile	2008-10-09 15:20:52.000000000 +0200
@@ -4,5 +4,8 @@
 
 obj-$(CONFIG_BONDING) += bonding.o
 
-bonding-objs := bond_main.o bond_3ad.o bond_alb.o bond_sysfs.o
+bonding-y := bond_main.o bond_3ad.o bond_alb.o
+
+bonding-$(CONFIG_SYSFS) += bond_sysfs.o
+
 
--- a/drivers/net/bonding/bond_main.c	2008-10-11 13:46:05.000000000 +0200
+++ b/drivers/net/bonding/bond_main.c	2008-10-11 13:53:32.000000000 +0200
@@ -150,7 +150,15 @@ LIST_HEAD(bond_dev_list);
 static struct proc_dir_entry *bond_proc_dir = NULL;
 #endif
 
-extern struct rw_semaphore bonding_rwsem;
+/* Bonding sysfs lock.  Why can't we just use the subsystem lock?
+ * Because kobject_register tries to acquire the subsystem lock.  If
+ * we already hold the lock (which we would if the user was creating
+ * a new bond through the sysfs interface), we deadlock.
+ * This lock is only needed when deleting a bond - we need to make sure
+ * that we don't collide with an ongoing ioctl.
+ */
+struct rw_semaphore bonding_rwsem;
+
 static __be32 arp_target[BOND_MAX_ARP_TARGETS] = { 0, } ;
 static int arp_ip_count	= 0;
 static int bond_mode	= BOND_MODE_ROUNDROBIN;
@@ -3474,10 +3482,12 @@ static int bond_event_changename(struct 
 	bond_remove_proc_entry(bond);
 	bond_create_proc_entry(bond);
 #endif
+#ifdef CONFIG_SYSFS
 	down_write(&(bonding_rwsem));
         bond_destroy_sysfs_entry(bond);
         bond_create_sysfs_entry(bond);
 	up_write(&(bonding_rwsem));
+#endif
 	return NOTIFY_DONE;
 }
 
--- a/drivers/net/bonding/bond_sysfs.c	2008-10-11 13:50:00.000000000 +0200
+++ b/drivers/net/bonding/bond_sysfs.c	2008-10-11 13:53:22.000000000 +0200
@@ -53,22 +53,6 @@ extern struct bond_parm_tbl arp_validate
 extern struct bond_parm_tbl fail_over_mac_tbl[];
 
 static int expected_refcount = -1;
-/*--------------------------- Data Structures -----------------------------*/
-
-/* Bonding sysfs lock.  Why can't we just use the subsystem lock?
- * Because kobject_register tries to acquire the subsystem lock.  If
- * we already hold the lock (which we would if the user was creating
- * a new bond through the sysfs interface), we deadlock.
- * This lock is only needed when deleting a bond - we need to make sure
- * that we don't collide with an ongoing ioctl.
- */
-
-struct rw_semaphore bonding_rwsem;
-
-
-
-
-/*------------------------------ Functions --------------------------------*/
 
 /*
  * "show" function for the bond_masters attribute.
--- a/drivers/net/bonding/bonding.h	2008-10-11 13:51:18.000000000 +0200
+++ b/drivers/net/bonding/bonding.h	2008-10-11 13:51:30.000000000 +0200
@@ -310,6 +310,8 @@ static inline void bond_unset_master_alb
 	bond->dev->priv_flags &= ~IFF_MASTER_ALB;
 }
 
+extern struct rw_semaphore bonding_rwsem;
+
 struct vlan_entry *bond_next_vlan(struct bonding *bond, struct vlan_entry *curr);
 int bond_dev_queue_xmit(struct bonding *bond, struct sk_buff *skb, struct net_device *slave_dev);
 int bond_create(char *name, struct bond_params *params);

-- 

[PATCH 2/6] bonding: fix sparse warnings

[Stephen Hemminger]

[-- Attachment #1: bonding-sparse.patch --]
[-- Type: text/plain, Size: 1997 bytes --]

This fixes sparse warnings due to normal locking, and shadowed variable.
There is still one warning but it is valid, and caused by some dubious locking
in the failover MAC assignment.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

--- a/drivers/net/bonding/bond_main.c	2008-10-17 15:45:13.000000000 -0700
+++ b/drivers/net/bonding/bond_main.c	2008-10-17 18:59:11.000000000 -0700
@@ -3173,6 +3173,7 @@ out:
 #ifdef CONFIG_PROC_FS
 
 static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos)
+	__acquires(dev_base_lock) __acquires(bond->lock)
 {
 	struct bonding *bond = seq->private;
 	loff_t off = 0;
@@ -3212,6 +3213,7 @@ static void *bond_info_seq_next(struct s
 }
 
 static void bond_info_seq_stop(struct seq_file *seq, void *v)
+	__releases(dev_base_lock) __releases(bond->lock)
 {
 	struct bonding *bond = seq->private;
 
@@ -4662,7 +4664,7 @@ static void bond_free_all(void)
  */
 int bond_parse_parm(const char *buf, struct bond_parm_tbl *tbl)
 {
-	int mode = -1, i, rv;
+	int newmode = -1, i, rv;
 	char *p, modestr[BOND_MAX_MODENAME_LEN + 1] = { 0, };
 
 	for (p = (char *)buf; *p; p++)
@@ -4672,13 +4674,13 @@ int bond_parse_parm(const char *buf, str
 	if (*p)
 		rv = sscanf(buf, "%20s", modestr);
 	else
-		rv = sscanf(buf, "%d", &mode);
+		rv = sscanf(buf, "%d", &newmode);
 
 	if (!rv)
 		return -1;
 
 	for (i = 0; tbl[i].modename; i++) {
-		if (mode == tbl[i].mode)
+		if (newmode == tbl[i].mode)
 			return tbl[i].mode;
 		if (strcmp(modestr, tbl[i].modename) == 0)
 			return tbl[i].mode;
--- a/drivers/net/bonding/bonding.h	2008-10-17 15:45:13.000000000 -0700
+++ b/drivers/net/bonding/bonding.h	2008-10-17 16:48:26.000000000 -0700
@@ -336,6 +336,7 @@ void bond_register_arp(struct bonding *)
 void bond_unregister_arp(struct bonding *);
 
 /* exported from bond_main.c */
+extern struct bond_params bonding_defaults;
 extern struct list_head bond_dev_list;
 extern struct bond_parm_tbl bond_lacp_tbl[];
 extern struct bond_parm_tbl bond_mode_tbl[];

-- 

[PATCH 3/6] bonding: event driven carrier detection

[Stephen Hemminger]

[-- Attachment #1: bonding-carrier-notify.patch --]
[-- Type: text/plain, Size: 5391 bytes --]

Rather than polling for carrier status changes, use existing netlink
notifier infrastracture to detect changes. This is enabled by leaving
mii_mon=0.  No matter what the detection method, if admin takes slave 
interface down, it should go down ASAP.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/bonding/bond_main.c	2008-10-17 15:45:14.000000000 -0700
+++ b/drivers/net/bonding/bond_main.c	2008-10-17 16:50:37.000000000 -0700
@@ -1640,8 +1640,7 @@ int bond_enslave(struct net_device *bond
 	}
 
 	/* check for initial state */
-	if (!bond->params.miimon ||
-	    (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS)) {
+	if (bond_check_dev_link(bond, slave_dev, 0) == BMSR_LSTATUS) {
 		if (bond->params.updelay) {
 			dprintk("Initial state of slave_dev is "
 				"BOND_LINK_BACK\n");
@@ -3510,31 +3509,67 @@ static int bond_master_netdev_event(unsi
 	return NOTIFY_DONE;
 }
 
+static void bond_slave_down(struct bonding *bond, struct net_device *slave_dev)
+{
+	struct slave *slave = bond_get_slave_by_dev(bond, slave_dev);
+
+	if (slave && slave->link != BOND_LINK_DOWN) {
+		slave->link = BOND_LINK_DOWN;
+		read_lock(&bond->lock);
+		bond_miimon_commit(bond);
+		read_unlock(&bond->lock);
+	}
+}
+
+static void bond_slave_up(struct bonding *bond, struct net_device *slave_dev)
+{
+	struct slave *slave = bond_get_slave_by_dev(bond, slave_dev);
+
+	if (slave && slave->link == BOND_LINK_DOWN) {
+		if (bond->params.updelay) {
+			slave->link  = BOND_LINK_BACK;
+			slave->delay = bond->params.updelay;
+		} else
+			slave->link  = BOND_LINK_UP;
+
+		read_lock(&bond->lock);
+		bond_miimon_commit(bond);
+		read_unlock(&bond->lock);
+	}
+}
+
 static int bond_slave_netdev_event(unsigned long event, struct net_device *slave_dev)
 {
 	struct net_device *bond_dev = slave_dev->master;
-	struct bonding *bond = bond_dev->priv;
+	struct bonding *bond;
+
+	if (!bond_dev)
+		goto out;
+
+	bond = bond_dev->priv;
 
 	switch (event) {
 	case NETDEV_UNREGISTER:
-		if (bond_dev) {
-			if (bond->setup_by_slave)
-				bond_release_and_destroy(bond_dev, slave_dev);
-			else
-				bond_release(bond_dev, slave_dev);
-		}
-		break;
-	case NETDEV_CHANGE:
-		/*
-		 * TODO: is this what we get if somebody
-		 * sets up a hierarchical bond, then rmmod's
-		 * one of the slave bonding devices?
-		 */
+		if (bond->setup_by_slave)
+			bond_release_and_destroy(bond_dev, slave_dev);
+		else
+			bond_release(bond_dev, slave_dev);
 		break;
 	case NETDEV_DOWN:
-		/*
-		 * ... Or is it this?
-		 */
+		bond_slave_down(bond, slave_dev);
+		break;
+	case NETDEV_UP:
+	case NETDEV_CHANGE:
+		if (bond->params.use_carrier && !bond->params.miimon) {
+			dprintk("%s: status change %s\n",
+				slave_dev->name,
+				IS_UP(slave_dev) ? "up" : "down");
+
+			if (IS_UP(slave_dev))
+				bond_slave_up(bond, slave_dev);
+			else
+				bond_slave_down(bond, slave_dev);
+		}
 		break;
 	case NETDEV_CHANGEMTU:
 		/*
@@ -3558,10 +3593,9 @@ static int bond_slave_netdev_event(unsig
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);
 		break;
-	default:
-		break;
 	}
 
+out:
 	return NOTIFY_DONE;
 }
 
@@ -4788,33 +4822,6 @@ static int bond_check_params(struct bond
 		num_grat_arp = 1;
 	}
 
-	/* reset values for 802.3ad */
-	if (bond_mode == BOND_MODE_8023AD) {
-		if (!miimon) {
-			printk(KERN_WARNING DRV_NAME
-			       ": Warning: miimon must be specified, "
-			       "otherwise bonding will not detect link "
-			       "failure, speed and duplex which are "
-			       "essential for 802.3ad operation\n");
-			printk(KERN_WARNING "Forcing miimon to 100msec\n");
-			miimon = 100;
-		}
-	}
-
-	/* reset values for TLB/ALB */
-	if ((bond_mode == BOND_MODE_TLB) ||
-	    (bond_mode == BOND_MODE_ALB)) {
-		if (!miimon) {
-			printk(KERN_WARNING DRV_NAME
-			       ": Warning: miimon must be specified, "
-			       "otherwise bonding will not detect link "
-			       "failure and link speed which are essential "
-			       "for TLB/ALB load balancing\n");
-			printk(KERN_WARNING "Forcing miimon to 100msec\n");
-			miimon = 100;
-		}
-	}
-
 	if (bond_mode == BOND_MODE_ALB) {
 		printk(KERN_NOTICE DRV_NAME
 		       ": In ALB mode you might experience client "
@@ -4825,19 +4832,7 @@ static int bond_check_params(struct bond
 		       updelay);
 	}
 
-	if (!miimon) {
-		if (updelay || downdelay) {
-			/* just warn the user the up/down delay will have
-			 * no effect since miimon is zero...
-			 */
-			printk(KERN_WARNING DRV_NAME
-			       ": Warning: miimon module parameter not set "
-			       "and updelay (%d) or downdelay (%d) module "
-			       "parameter is set; updelay and downdelay have "
-			       "no effect unless miimon is set\n",
-			       updelay, downdelay);
-		}
-	} else {
+	if (miimon) {
 		/* don't allow arp monitoring */
 		if (arp_interval) {
 			printk(KERN_WARNING DRV_NAME
@@ -4944,15 +4939,6 @@ static int bond_check_params(struct bond
 
 		printk("\n");
 
-	} else if (max_bonds) {
-		/* miimon and arp_interval not set, we need one so things
-		 * work as expected, see bonding.txt for details
-		 */
-		printk(KERN_WARNING DRV_NAME
-		       ": Warning: either miimon or arp_interval and "
-		       "arp_ip_target module parameters must be specified, "
-		       "otherwise bonding will not detect link failures! see "
-		       "bonding.txt for details.\n");
 	}
 
 	if (primary && !USES_PRIMARY(bond_mode)) {

-- 

[PATCH 4/6] netdev: allow rejecting MTU changes from notifiers

[Stephen Hemminger]

[-- Attachment #1: dev-mtu-rollback.patch --]
[-- Type: text/plain, Size: 1406 bytes --]

This patch allows notifiers to reject changes to MTU, and then does rollback
same way change name does).

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
--- a/net/core/dev.c	2008-10-17 18:59:11.000000000 -0700
+++ b/net/core/dev.c	2008-10-17 21:22:30.000000000 -0700
@@ -3453,9 +3453,10 @@ int dev_change_flags(struct net_device *
  */
 int dev_set_mtu(struct net_device *dev, int new_mtu)
 {
-	int err;
+	int ret, err = 0;
+	int old_mtu = dev->mtu;
 
-	if (new_mtu == dev->mtu)
+	if (new_mtu == old_mtu)
 		return 0;
 
 	/*	MTU must be positive.	 */
@@ -3465,13 +3466,31 @@ int dev_set_mtu(struct net_device *dev, 
 	if (!netif_device_present(dev))
 		return -ENODEV;
 
-	err = 0;
-	if (dev->change_mtu)
-		err = dev->change_mtu(dev, new_mtu);
-	else
+rollback:
+	if (dev->change_mtu) {
+		ret = dev->change_mtu(dev, new_mtu);
+		if (ret)
+			return ret;
+	} else
 		dev->mtu = new_mtu;
-	if (!err && dev->flags & IFF_UP)
-		call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
+
+	if (!err && (dev->flags & IFF_UP)) {
+		ret = call_netdevice_notifiers(NETDEV_CHANGEMTU, dev);
+		ret = notifier_to_errno(ret);
+		printk(KERN_DEBUG "dev_set_mtu notifier ret %d\n", ret);
+
+		if (ret) {
+			if (err) {
+				printk(KERN_ERR "%s: mtu rollback failed: %d\n",
+				       dev->name, ret);
+			} else {
+				err = ret;
+				new_mtu = old_mtu;
+				goto rollback;
+			}
+		}
+
+	}
 	return err;
 }
 

-- 

[PATCH 5/6] bonding: prevent MTU changes in slave devices

[Stephen Hemminger]

[-- Attachment #1: bonding-mtu.patch --]
[-- Type: text/plain, Size: 1882 bytes --]

Check in bonding driver disallow changing MTU of one slave.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/bonding/bond_main.c	2008-10-17 18:59:48.000000000 -0700
+++ b/drivers/net/bonding/bond_main.c	2008-10-17 19:02:58.000000000 -0700
@@ -3572,19 +3572,14 @@ static int bond_slave_netdev_event(unsig
 		}
 		break;
 	case NETDEV_CHANGEMTU:
-		/*
-		 * TODO: Should slaves be allowed to
-		 * independently alter their MTU?  For
-		 * an active-backup bond, slaves need
-		 * not be the same type of device, so
-		 * MTUs may vary.  For other modes,
-		 * slaves arguably should have the
-		 * same MTUs. To do this, we'd need to
-		 * take over the slave's change_mtu
-		 * function for the duration of their
-		 * servitude.
+		/* Can not independently change MTU of slave devices
+		 * NOTIFY_BAD causes EPERM in orignal change mtu call
+		 * NB: called on unwind as well
 		 */
+		if (slave_dev->mtu != bond_dev->mtu)
+			return NOTIFY_BAD;
 		break;
+
 	case NETDEV_CHANGENAME:
 		/*
 		 * TODO: handle changing the primary's name
@@ -4132,6 +4127,7 @@ static int bond_change_mtu(struct net_de
 {
 	struct bonding *bond = bond_dev->priv;
 	struct slave *slave, *stop_at;
+	int old_mtu = bond_dev->mtu;
 	int res = 0;
 	int i;
 
@@ -4152,7 +4148,7 @@ static int bond_change_mtu(struct net_de
 	 * list, but without holding a lock around the actual
 	 * call to the base driver.
 	 */
-
+	bond_dev->mtu = new_mtu;
 	bond_for_each_slave(bond, slave, i) {
 		dprintk("s %p s->p %p c_m %p\n", slave,
 			slave->prev, slave->dev->change_mtu);
@@ -4173,11 +4169,12 @@ static int bond_change_mtu(struct net_de
 		}
 	}
 
-	bond_dev->mtu = new_mtu;
 
 	return 0;
 
 unwind:
+	bond_dev->mtu = old_mtu;
+
 	/* unwind from head to the slave that failed */
 	stop_at = slave;
 	bond_for_each_slave_from_to(bond, slave, i, bond->first_slave, stop_at) {

-- 

[PATCH 6/6] bonding: remove change name TODO

[Stephen Hemminger]

[-- Attachment #1: bond-primary-change.patch --]
[-- Type: text/plain, Size: 639 bytes --]

Changing the name of the bonding primary is already handled. The only
place the name is stored is in bond->params.primary, and that is only used
during startup.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


--- a/drivers/net/bonding/bond_main.c	2008-10-17 16:50:42.000000000 -0700
+++ b/drivers/net/bonding/bond_main.c	2008-10-17 16:50:45.000000000 -0700
@@ -3580,11 +3580,6 @@ static int bond_slave_netdev_event(unsig
 			return NOTIFY_BAD;
 		break;
 
-	case NETDEV_CHANGENAME:
-		/*
-		 * TODO: handle changing the primary's name
-		 */
-		break;
 	case NETDEV_FEAT_CHANGE:
 		bond_compute_features(bond);
 		break;

--