[PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[Jay Vosburgh]

	This patch set includes 6 patches: four fixes to the dev_mc_sync /
dev_mc_unsync system; and two patches to bonding, one to utilize the sync
/ unsync functions, and another minor fix related to MAC address handling.

	A brief description of each patch:

	1. uninitialized variable in __hw_addr_create_ex

	2. __hw_addr_unsync_one does not work because "from" addresses
		aren't marked as "synced."

	3. __hw_addr_sync_one as called by __hw_addr_sync_multiple will
		not remove references properly

	4. dev_mc_sync_multiple doesn't call the _multiple helper function

	5. Convert bonding to use dev_{mc,uc}_sync / unsync

	6. Disallow changes of bonding MAC if fail_over_mac is set to
		"follow".

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

[PATCH net-next 1/6] net/core: __hw_addr_create_ex does not initialize sync_cnt

[Jay Vosburgh]

	The sync_cnt field is not being initialized, which can result
in arbitrary values in the field.  Fixed by initializing it to zero.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Cc: Vlad Yasevich <vyasevic@redhat.com> 

---
 net/core/dev_addr_lists.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index c013f38..1f919d9 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -39,6 +39,7 @@ static int __hw_addr_create_ex(struct netdev_hw_addr_list *list,
 	ha->refcount = 1;
 	ha->global_use = global;
 	ha->synced = sync;
+	ha->sync_cnt = 0;
 	list_add_tail_rcu(&ha->list, &list->list);
 	list->count++;
 
-- 
1.8.1.4

[PATCH net-next 2/6] net/core: __hw_addr_unsync_one refcount leak synced

[Jay Vosburgh]

	When an address is added to a subordinate interface (the "to"
list), the address entry in the "from" list is not marked "synced" as
the entry added to the "to" list is.

	When performing the unsync operation (e.g., dev_mc_unsync),
__hw_addr_unsync_one calls __hw_addr_del_entry with the "synced"
parameter set to true for the case when the address reference is being
released from the "from" list.  This causes a test inside to fail,
with the result being that the reference count on the "from" address
is not properly decremeted and the address on the "from" list will
never be freed.

	Correct this by having __hw_addr_unsync_one call the
__hw_addr_del_entry function with the "sync" flag set to false for the
"remove from the from list" case.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Cc: Vlad Yasevich <vyasevic@redhat.com> 

---
 net/core/dev_addr_lists.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 1f919d9..c858e81 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -160,7 +160,8 @@ static void __hw_addr_unsync_one(struct netdev_hw_addr_list *to_list,
 	if (err)
 		return;
 	ha->sync_cnt--;
-	__hw_addr_del_entry(from_list, ha, false, true);
+	/* address on from list is not marked synced */
+	__hw_addr_del_entry(from_list, ha, false, false);
 }
 
 static int __hw_addr_sync_multiple(struct netdev_hw_addr_list *to_list,
-- 
1.8.1.4

[PATCH net-next 3/6] net/core: __hw_addr_sync_one / _multiple broken

[Jay Vosburgh]

	Currently, __hw_addr_sync_one is called in a loop by
__hw_addr_sync_multiple to sync each of a "from" device's hw addresses
to a "to" device.  __hw_addr_sync_one calls __hw_addr_add_ex to attempt
to add each address.  __hw_addr_add_ex is called with global=false, and
sync=true.

	__hw_addr_add_ex checks to see if the new address matches an
address already on the list.  If so, it tests global and sync.  In this
case, sync=true, and it then checks if the address is already synced,
and if so, returns 0.

	This 0 return causes __hw_addr_sync_one to increment the sync_cnt
and refcount for the "from" list's address entry, even though the address
is already synced and has a reference and sync_cnt.  This will cause
the sync_cnt and refcount to increment without bound every time an
addresses is added to the "from" device and synced to the "to" device.

	The fix here has two parts:

	First, when __hw_addr_add_ex finds the address already exists
and is synced, return -EEXIST instead of 0.

	Second, __hw_addr_sync_one checks the error return for -EEXIST,
and if so, it (a) does not add a refcount/sync_cnt, and (b) returns 0
itself so that __hw_addr_sync_multiple will not return an error.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Cc: Vlad Yasevich <vyasevic@redhat.com> 

---
 net/core/dev_addr_lists.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index c858e81..8e2c2ef 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -67,7 +67,7 @@ static int __hw_addr_add_ex(struct netdev_hw_addr_list *list,
 			}
 			if (sync) {
 				if (ha->synced)
-					return 0;
+					return -EEXIST;
 				else
 					ha->synced = true;
 			}
@@ -140,10 +140,13 @@ static int __hw_addr_sync_one(struct netdev_hw_addr_list *to_list,
 
 	err = __hw_addr_add_ex(to_list, ha->addr, addr_len, ha->type,
 			       false, true);
-	if (err)
+	if (err && err != -EEXIST)
 		return err;
-	ha->sync_cnt++;
-	ha->refcount++;
+
+	if (!err) {
+		ha->sync_cnt++;
+		ha->refcount++;
+	}
 
 	return 0;
 }
-- 
1.8.1.4

[PATCH net-next 4/6] net/core: dev_mc_sync_multiple calls wrong helper

[Jay Vosburgh]

	The dev_mc_sync_multiple function is currently calling
__hw_addr_sync, and not __hw_addr_sync_multiple.  This will result in
addresses only being synced to the first device from the set.

	Corrected by calling the _multiple variant.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Cc: Vlad Yasevich <vyasevic@redhat.com> 

---
 net/core/dev_addr_lists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev_addr_lists.c b/net/core/dev_addr_lists.c
index 8e2c2ef..6cda4e2 100644
--- a/net/core/dev_addr_lists.c
+++ b/net/core/dev_addr_lists.c
@@ -801,7 +801,7 @@ int dev_mc_sync_multiple(struct net_device *to, struct net_device *from)
 		return -EINVAL;
 
 	netif_addr_lock_nested(to);
-	err = __hw_addr_sync(&to->mc, &from->mc, to->addr_len);
+	err = __hw_addr_sync_multiple(&to->mc, &from->mc, to->addr_len);
 	if (!err)
 		__dev_set_rx_mode(to);
 	netif_addr_unlock(to);
-- 
1.8.1.4

[PATCH net-next 5/6] bonding: Convert hw addr handling to sync/unsync, support ucast addresses

[Jay Vosburgh]

	This patch converts bonding to use the dev_uc/mc_sync and
dev_uc/mc_sync_multiple functions for updating the hardware addresses
of bonding slaves.

	The existing functions to add or remove addresses are removed,
and their functionality is replaced with calls to dev_mc_sync or
dev_mc_sync_multiple, depending upon the bonding mode.

	Calls to dev_uc_sync and dev_uc_sync_multiple are also added,
so that unicast addresses added to a bond will be properly synced with
its slaves.

	Various functions are renamed to better reflect the new
situation, and relevant comments are updated.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Cc: Vlad Yasevich <vyasevic@redhat.com> 

---
 drivers/net/bonding/bond_main.c | 164 ++++++++++------------------------------
 drivers/net/bonding/bonding.h   |   1 -
 2 files changed, 39 insertions(+), 126 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index f4489d6..92e085f 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -706,45 +706,6 @@ static int bond_set_allmulti(struct bonding *bond, int inc)
 	return err;
 }
 
-/*
- * Add a Multicast address to slaves
- * according to mode
- */
-static void bond_mc_add(struct bonding *bond, void *addr)
-{
-	if (USES_PRIMARY(bond->params.mode)) {
-		/* write lock already acquired */
-		if (bond->curr_active_slave)
-			dev_mc_add(bond->curr_active_slave->dev, addr);
-	} else {
-		struct slave *slave;
-		int i;
-
-		bond_for_each_slave(bond, slave, i)
-			dev_mc_add(slave->dev, addr);
-	}
-}
-
-/*
- * Remove a multicast address from slave
- * according to mode
- */
-static void bond_mc_del(struct bonding *bond, void *addr)
-{
-	if (USES_PRIMARY(bond->params.mode)) {
-		/* write lock already acquired */
-		if (bond->curr_active_slave)
-			dev_mc_del(bond->curr_active_slave->dev, addr);
-	} else {
-		struct slave *slave;
-		int i;
-		bond_for_each_slave(bond, slave, i) {
-			dev_mc_del(slave->dev, addr);
-		}
-	}
-}
-
-
 static void __bond_resend_igmp_join_requests(struct net_device *dev)
 {
 	struct in_device *in_dev;
@@ -803,17 +764,15 @@ static void bond_resend_igmp_join_requests_delayed(struct work_struct *work)
 	bond_resend_igmp_join_requests(bond);
 }
 
-/*
- * flush all members of flush->mc_list from device dev->mc_list
+/* Flush bond's hardware addresses from slave
  */
-static void bond_mc_list_flush(struct net_device *bond_dev,
+static void bond_hw_addr_flush(struct net_device *bond_dev,
 			       struct net_device *slave_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	struct netdev_hw_addr *ha;
 
-	netdev_for_each_mc_addr(ha, bond_dev)
-		dev_mc_del(slave_dev, ha->addr);
+	dev_uc_unsync(slave_dev, bond_dev);
+	dev_mc_unsync(slave_dev, bond_dev);
 
 	if (bond->params.mode == BOND_MODE_8023AD) {
 		/* del lacpdu mc addr from mc list */
@@ -825,22 +784,14 @@ static void bond_mc_list_flush(struct net_device *bond_dev,
 
 /*--------------------------- Active slave change ---------------------------*/
 
-/*
- * Update the mc list and multicast-related flags for the new and
- * old active slaves (if any) according to the multicast mode, and
- * promiscuous flags unconditionally.
+/* Update the hardware address list and promisc/allmulti for the new and
+ * old active slaves (if any).  Modes that are !USES_PRIMARY keep all
+ * slaves up date at all times; only the USES_PRIMARY modes need to call
+ * this function to swap these settings during a failover.
  */
-static void bond_mc_swap(struct bonding *bond, struct slave *new_active,
-			 struct slave *old_active)
+static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
+			      struct slave *old_active)
 {
-	struct netdev_hw_addr *ha;
-
-	if (!USES_PRIMARY(bond->params.mode))
-		/* nothing to do -  mc list is already up-to-date on
-		 * all slaves
-		 */
-		return;
-
 	if (old_active) {
 		if (bond->dev->flags & IFF_PROMISC)
 			dev_set_promiscuity(old_active->dev, -1);
@@ -848,10 +799,7 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active,
 		if (bond->dev->flags & IFF_ALLMULTI)
 			dev_set_allmulti(old_active->dev, -1);
 
-		netif_addr_lock_bh(bond->dev);
-		netdev_for_each_mc_addr(ha, bond->dev)
-			dev_mc_del(old_active->dev, ha->addr);
-		netif_addr_unlock_bh(bond->dev);
+		bond_hw_addr_flush(bond->dev, old_active->dev);
 	}
 
 	if (new_active) {
@@ -863,8 +811,8 @@ static void bond_mc_swap(struct bonding *bond, struct slave *new_active,
 			dev_set_allmulti(new_active->dev, 1);
 
 		netif_addr_lock_bh(bond->dev);
-		netdev_for_each_mc_addr(ha, bond->dev)
-			dev_mc_add(new_active->dev, ha->addr);
+		dev_uc_sync(new_active->dev, bond->dev);
+		dev_mc_sync(new_active->dev, bond->dev);
 		netif_addr_unlock_bh(bond->dev);
 	}
 }
@@ -1083,7 +1031,7 @@ void bond_change_active_slave(struct bonding *bond, struct slave *new_active)
 	}
 
 	if (USES_PRIMARY(bond->params.mode))
-		bond_mc_swap(bond, new_active, old_active);
+		bond_hw_addr_swap(bond, new_active, old_active);
 
 	if (bond_is_lb(bond)) {
 		bond_alb_handle_active_change(bond, new_active);
@@ -1526,7 +1474,6 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 	struct bonding *bond = netdev_priv(bond_dev);
 	const struct net_device_ops *slave_ops = slave_dev->netdev_ops;
 	struct slave *new_slave = NULL;
-	struct netdev_hw_addr *ha;
 	struct sockaddr addr;
 	int link_reporting;
 	int res = 0;
@@ -1706,10 +1653,8 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 			goto err_close;
 	}
 
-	/* If the mode USES_PRIMARY, then the new slave gets the
-	 * master's promisc (and mc) settings only if it becomes the
-	 * curr_active_slave, and that is taken care of later when calling
-	 * bond_change_active()
+	/* If the mode USES_PRIMARY, then the following is handled by
+	 * bond_change_active_slave().
 	 */
 	if (!USES_PRIMARY(bond->params.mode)) {
 		/* set promiscuity level to new slave */
@@ -1727,9 +1672,10 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 		}
 
 		netif_addr_lock_bh(bond_dev);
-		/* upload master's mc_list to new slave */
-		netdev_for_each_mc_addr(ha, bond_dev)
-			dev_mc_add(slave_dev, ha->addr);
+
+		dev_mc_sync_multiple(slave_dev, bond_dev);
+		dev_uc_sync_multiple(slave_dev, bond_dev);
+
 		netif_addr_unlock_bh(bond_dev);
 	}
 
@@ -1908,11 +1854,9 @@ err_dest_symlinks:
 	bond_destroy_slave_symlinks(bond_dev, slave_dev);
 
 err_detach:
-	if (!USES_PRIMARY(bond->params.mode)) {
-		netif_addr_lock_bh(bond_dev);
-		bond_mc_list_flush(bond_dev, slave_dev);
-		netif_addr_unlock_bh(bond_dev);
-	}
+	if (!USES_PRIMARY(bond->params.mode))
+		bond_hw_addr_flush(bond_dev, slave_dev);
+
 	bond_del_vlans_from_slave(bond, slave_dev);
 	write_lock_bh(&bond->lock);
 	bond_detach_slave(bond, new_slave);
@@ -2107,9 +2051,8 @@ static int __bond_release_one(struct net_device *bond_dev,
 
 	bond_del_vlans_from_slave(bond, slave_dev);
 
-	/* If the mode USES_PRIMARY, then we should only remove its
-	 * promisc and mc settings if it was the curr_active_slave, but that was
-	 * already taken care of above when we detached the slave
+	/* If the mode USES_PRIMARY, then this cases was handled above by
+	 * bond_change_active_slave(..., NULL)
 	 */
 	if (!USES_PRIMARY(bond->params.mode)) {
 		/* unset promiscuity level from slave */
@@ -2120,10 +2063,7 @@ static int __bond_release_one(struct net_device *bond_dev,
 		if (bond_dev->flags & IFF_ALLMULTI)
 			dev_set_allmulti(slave_dev, -1);
 
-		/* flush master's mc_list from slave */
-		netif_addr_lock_bh(bond_dev);
-		bond_mc_list_flush(bond_dev, slave_dev);
-		netif_addr_unlock_bh(bond_dev);
+		bond_hw_addr_flush(bond_dev, slave_dev);
 	}
 
 	bond_upper_dev_unlink(bond_dev, slave_dev);
@@ -3660,19 +3600,6 @@ static int bond_do_ioctl(struct net_device *bond_dev, struct ifreq *ifr, int cmd
 	return res;
 }
 
-static bool bond_addr_in_mc_list(unsigned char *addr,
-				 struct netdev_hw_addr_list *list,
-				 int addrlen)
-{
-	struct netdev_hw_addr *ha;
-
-	netdev_hw_addr_list_for_each(ha, list)
-		if (!memcmp(ha->addr, addr, addrlen))
-			return true;
-
-	return false;
-}
-
 static void bond_change_rx_flags(struct net_device *bond_dev, int change)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
@@ -3686,35 +3613,25 @@ static void bond_change_rx_flags(struct net_device *bond_dev, int change)
 				  bond_dev->flags & IFF_ALLMULTI ? 1 : -1);
 }
 
-static void bond_set_multicast_list(struct net_device *bond_dev)
+static void bond_set_rx_mode(struct net_device *bond_dev)
 {
 	struct bonding *bond = netdev_priv(bond_dev);
-	struct netdev_hw_addr *ha;
-	bool found;
+	struct slave *slave;
+	int i;
 
 	read_lock(&bond->lock);
 
-	/* looking for addresses to add to slaves' mc list */
-	netdev_for_each_mc_addr(ha, bond_dev) {
-		found = bond_addr_in_mc_list(ha->addr, &bond->mc_list,
-					     bond_dev->addr_len);
-		if (!found)
-			bond_mc_add(bond, ha->addr);
-	}
-
-	/* looking for addresses to delete from slaves' list */
-	netdev_hw_addr_list_for_each(ha, &bond->mc_list) {
-		found = bond_addr_in_mc_list(ha->addr, &bond_dev->mc,
-					     bond_dev->addr_len);
-		if (!found)
-			bond_mc_del(bond, ha->addr);
+	if (USES_PRIMARY(bond->params.mode) && bond->curr_active_slave) {
+		slave = bond->curr_active_slave;
+		dev_uc_sync(slave->dev, bond_dev);
+		dev_mc_sync(slave->dev, bond_dev);
+	} else {
+		bond_for_each_slave(bond, slave, i) {
+			dev_uc_sync_multiple(slave->dev, bond_dev);
+			dev_mc_sync_multiple(slave->dev, bond_dev);
+		}
 	}
 
-	/* save master's multicast list */
-	__hw_addr_flush(&bond->mc_list);
-	__hw_addr_add_multiple(&bond->mc_list, &bond_dev->mc,
-			       bond_dev->addr_len, NETDEV_HW_ADDR_T_MULTICAST);
-
 	read_unlock(&bond->lock);
 }
 
@@ -4321,7 +4238,7 @@ static const struct net_device_ops bond_netdev_ops = {
 	.ndo_get_stats64	= bond_get_stats,
 	.ndo_do_ioctl		= bond_do_ioctl,
 	.ndo_change_rx_flags	= bond_change_rx_flags,
-	.ndo_set_rx_mode	= bond_set_multicast_list,
+	.ndo_set_rx_mode	= bond_set_rx_mode,
 	.ndo_change_mtu		= bond_change_mtu,
 	.ndo_set_mac_address	= bond_set_mac_address,
 	.ndo_neigh_setup	= bond_neigh_setup,
@@ -4426,8 +4343,6 @@ static void bond_uninit(struct net_device *bond_dev)
 
 	bond_debug_unregister(bond);
 
-	__hw_addr_flush(&bond->mc_list);
-
 	list_for_each_entry_safe(vlan, tmp, &bond->vlan_list, vlan_list) {
 		list_del(&vlan->vlan_list);
 		kfree(vlan);
@@ -4838,7 +4753,6 @@ static int bond_init(struct net_device *bond_dev)
 		bond->dev_addr_from_first = true;
 	}
 
-	__hw_addr_init(&bond->mc_list);
 	return 0;
 }
 
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 2baec24..b38609b 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -231,7 +231,6 @@ struct bonding {
 	char     proc_file_name[IFNAMSIZ];
 #endif /* CONFIG_PROC_FS */
 	struct   list_head bond_list;
-	struct   netdev_hw_addr_list mc_list;
 	int      (*xmit_hash_policy)(struct sk_buff *, int);
 	u16      rr_tx_counter;
 	struct   ad_bond_info ad_info;
-- 
1.8.1.4

[PATCH net-next 6/6] bonding: disallow change of MAC if fail_over_mac enabled

[Jay Vosburgh]

	Currently, if fail_over_mac is set to active, then attempts to
change the MAC of the bond itself silently fail.  However, if fail_over_mac
is set to follow, changes are permitted.

	Permitting the bond's MAC to change with fail_over_mac=follow
will disrupt the follow functionality, which normally controls the
assignment of MAC address to the bond and its slaves, and can cause
multiple ports to be assigned the same MAC address. which will interfere
with the functioning of the device (where the device here is a
virtualization-aware card for s390, qeth, or similar devices).

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_main.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 92e085f..0ff7d8a 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3776,11 +3776,10 @@ static int bond_set_mac_address(struct net_device *bond_dev, void *addr)
 	pr_debug("bond=%p, name=%s\n",
 		 bond, bond_dev ? bond_dev->name : "None");
 
-	/*
-	 * If fail_over_mac is set to active, do nothing and return
-	 * success.  Returning an error causes ifenslave to fail.
+	/* If fail_over_mac is enabled, do nothing and return success.
+	 * Returning an error causes ifenslave to fail.
 	 */
-	if (bond->params.fail_over_mac == BOND_FOM_ACTIVE)
+	if (bond->params.fail_over_mac)
 		return 0;
 
 	if (!is_valid_ether_addr(sa->sa_data))
-- 
1.8.1.4

Re: [PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Thu, 30 May 2013 17:55:38 -0700

> 	This patch set includes 6 patches: four fixes to the dev_mc_sync /
> dev_mc_unsync system; and two patches to bonding, one to utilize the sync
> / unsync functions, and another minor fix related to MAC address handling.

These look like fixes that should go into net, why target net-next?

In any event, I'll let Vlad review this before applying anywhere.

Thanks.

Re: [PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[Shawn Bohrer]

On Fri, May 31, 2013 at 01:31:55AM -0700, David Miller wrote:
> From: Jay Vosburgh <fubar@us.ibm.com>
> Date: Thu, 30 May 2013 17:55:38 -0700
> 
> > 	This patch set includes 6 patches: four fixes to the dev_mc_sync /
> > dev_mc_unsync system; and two patches to bonding, one to utilize the sync
> > / unsync functions, and another minor fix related to MAC address handling.
> 
> These look like fixes that should go into net, why target net-next?

In my oppinion 0-4 should go into net since they fix the bug I
reported in:

http://comments.gmane.org/gmane.linux.network/270477

I've tested patches 0-4 of this series so feel free to add my tested
by to those:

Tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>

>From just a casual observation of patch 5-6 they do not appear to be
bug fixes which is why this was probably marked net-next.

--
Shawn

Re: [PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[Jay Vosburgh]

Shawn Bohrer <shawn.bohrer@gmail.com> wrote:

>On Fri, May 31, 2013 at 01:31:55AM -0700, David Miller wrote:
>> From: Jay Vosburgh <fubar@us.ibm.com>
>> Date: Thu, 30 May 2013 17:55:38 -0700
>> 
>> > 	This patch set includes 6 patches: four fixes to the dev_mc_sync /
>> > dev_mc_unsync system; and two patches to bonding, one to utilize the sync
>> > / unsync functions, and another minor fix related to MAC address handling.
>> 
>> These look like fixes that should go into net, why target net-next?
>
>In my oppinion 0-4 should go into net since they fix the bug I
>reported in:
>
>http://comments.gmane.org/gmane.linux.network/270477
>
>I've tested patches 0-4 of this series so feel free to add my tested
>by to those:
>
>Tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>
>From just a casual observation of patch 5-6 they do not appear to be
>bug fixes which is why this was probably marked net-next.

	They're against net-next because I was working to convert
bonding to dev_sync/unsync against net-next and neglected to rebase then
before I posted.  The bonding patches (5 and 6) do fix a couple of bugs
related to MAC address handling on s390 (the lack of additional unicast
address propagation to the slaves makes qeth unhappy in some cases), so
arguably they could go either way, but I'm ok with those in net-next if
it's an issue.

	I do agree that 1-4 should go into net, once Vlad gives them a
look.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[Vlad Yasevich]

On 05/31/2013 11:56 AM, Jay Vosburgh wrote:
> Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
>
>> On Fri, May 31, 2013 at 01:31:55AM -0700, David Miller wrote:
>>> From: Jay Vosburgh <fubar@us.ibm.com>
>>> Date: Thu, 30 May 2013 17:55:38 -0700
>>>
>>>> 	This patch set includes 6 patches: four fixes to the dev_mc_sync /
>>>> dev_mc_unsync system; and two patches to bonding, one to utilize the sync
>>>> / unsync functions, and another minor fix related to MAC address handling.
>>>
>>> These look like fixes that should go into net, why target net-next?
>>
>> In my oppinion 0-4 should go into net since they fix the bug I
>> reported in:
>>
>> http://comments.gmane.org/gmane.linux.network/270477
>>
>> I've tested patches 0-4 of this series so feel free to add my tested
>> by to those:
>>
>> Tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>
>>From just a casual observation of patch 5-6 they do not appear to be
>> bug fixes which is why this was probably marked net-next.
>
> 	They're against net-next because I was working to convert
> bonding to dev_sync/unsync against net-next and neglected to rebase then
> before I posted.  The bonding patches (5 and 6) do fix a couple of bugs
> related to MAC address handling on s390 (the lack of additional unicast
> address propagation to the slaves makes qeth unhappy in some cases), so
> arguably they could go either way, but I'm ok with those in net-next if
> it's an issue.
>
> 	I do agree that 1-4 should go into net, once Vlad gives them a
> look.
>
> 	-J

I've reviewed the patches and ran a quick test.  They look good and fix
obvious problems.  Thanks to Jay for finding and fixing them.

Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>

-vlad

>
> ---
> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

Re: [PATCH net-next 0/6] net/core, bonding: dev_uc_sync fixes, bonding update

[Jay Vosburgh]

Vlad Yasevich <vyasevic@redhat.com> wrote:

>On 05/31/2013 11:56 AM, Jay Vosburgh wrote:
>> Shawn Bohrer <shawn.bohrer@gmail.com> wrote:
>>
>>> On Fri, May 31, 2013 at 01:31:55AM -0700, David Miller wrote:
>>>> From: Jay Vosburgh <fubar@us.ibm.com>
>>>> Date: Thu, 30 May 2013 17:55:38 -0700
>>>>
>>>>> 	This patch set includes 6 patches: four fixes to the dev_mc_sync /
>>>>> dev_mc_unsync system; and two patches to bonding, one to utilize the sync
>>>>> / unsync functions, and another minor fix related to MAC address handling.
>>>>
>>>> These look like fixes that should go into net, why target net-next?
>>>
>>> In my oppinion 0-4 should go into net since they fix the bug I
>>> reported in:
>>>
>>> http://comments.gmane.org/gmane.linux.network/270477
>>>
>>> I've tested patches 0-4 of this series so feel free to add my tested
>>> by to those:
>>>
>>> Tested-by: Shawn Bohrer <sbohrer@rgmadvisors.com>
>>>
>>>From just a casual observation of patch 5-6 they do not appear to be
>>> bug fixes which is why this was probably marked net-next.
>>
>> 	They're against net-next because I was working to convert
>> bonding to dev_sync/unsync against net-next and neglected to rebase then
>> before I posted.  The bonding patches (5 and 6) do fix a couple of bugs
>> related to MAC address handling on s390 (the lack of additional unicast
>> address propagation to the slaves makes qeth unhappy in some cases), so
>> arguably they could go either way, but I'm ok with those in net-next if
>> it's an issue.
>>
>> 	I do agree that 1-4 should go into net, once Vlad gives them a
>> look.
>>
>> 	-J
>
>I've reviewed the patches and ran a quick test.  They look good and fix
>obvious problems.  Thanks to Jay for finding and fixing them.
>
>Reviewed-by: Vlad Yasevich <vyasevic@redhat.com>

	Please hold off applying these.

	I just spotted an error in patch #5, so I'm going to fix that
and respin the set against -net.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com