[PATCH net-next 0/3] bonding: struct bonding cleanup and MAC set fixes

[PATCH net-next 0/3] bonding: struct bonding cleanup and MAC set fixes

[Nikolay Aleksandrov]

Hello,
This patchset removes some unnecessary struct bonding members. It also
fixes MAC setting inconsistencies (i.e. if a bond's MAC is set after
creation but prior to enslaving, it is lost and the first slave's MAC is
cloned again). Before these patches the only way to keep a user-defined MAC
is either to set it after enslaving or to create the bond with that MAC.
Patch 01 - removes setup_by_slave member
Patch 02 - removes dev_addr_from_first member and does the MAC fixing
Patch 03 - introduces the use of NET_ADDR_STOLEN when a MAC is cloned

Best regards,
 Nikolay Aleksandrov

Nikolay Aleksandrov (3):
  bonding: remove unnecessary setup_by_slave member
  bonding: remove unnecessary dev_addr_from_first member
  bonding: when cloning a MAC use NET_ADDR_STOLEN

 drivers/net/bonding/bond_main.c | 45 ++++++++++++++++++++---------------------
 drivers/net/bonding/bonding.h   |  2 --
 2 files changed, 22 insertions(+), 25 deletions(-)

-- 
1.8.1.4

[PATCH net-next 1/3] bonding: remove unnecessary setup_by_slave member

[Nikolay Aleksandrov]

We have a member called setup_by_slave in struct bonding to denote if the
bond dev has different type than ARPHRD_ETHER, but that is already denoted
in bond's netdev type variable if it was setup by the slave, so use that
instead of the member.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 5 +----
 drivers/net/bonding/bonding.h   | 1 -
 2 files changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 142d55d..2e8b9f1 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1382,8 +1382,6 @@ done:
 static void bond_setup_by_slave(struct net_device *bond_dev,
 				struct net_device *slave_dev)
 {
-	struct bonding *bond = netdev_priv(bond_dev);
-
 	bond_dev->header_ops	    = slave_dev->header_ops;
 
 	bond_dev->type		    = slave_dev->type;
@@ -1392,7 +1390,6 @@ static void bond_setup_by_slave(struct net_device *bond_dev,
 
 	memcpy(bond_dev->broadcast, slave_dev->broadcast,
 		slave_dev->addr_len);
-	bond->setup_by_slave = 1;
 }
 
 /* On bonding slaves other than the currently active slave, suppress
@@ -3187,7 +3184,7 @@ static int bond_slave_netdev_event(unsigned long event,
 
 	switch (event) {
 	case NETDEV_UNREGISTER:
-		if (bond->setup_by_slave)
+		if (bond_dev->type != ARPHRD_ETHER)
 			bond_release_and_destroy(bond_dev, slave_dev);
 		else
 			bond_release(bond_dev, slave_dev);
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index 3fb73cc..c6c8d03 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -226,7 +226,6 @@ struct bonding {
 	rwlock_t lock;
 	rwlock_t curr_slave_lock;
 	u8	 send_peer_notif;
-	s8	 setup_by_slave;
 	u8       igmp_retrans;
 #ifdef CONFIG_PROC_FS
 	struct   proc_dir_entry *proc_entry;
-- 
1.8.1.4

[PATCH net-next 2/3] bonding: remove unnecessary dev_addr_from_first member

[Nikolay Aleksandrov]

In struct bonding there's a member called dev_addr_from_first which is
used to denote when the bond dev should clone the first slave's MAC
address but since we have netdev's addr_assign_type variable that is not
necessary. We clone the first slave's MAC each time we have a random MAC
set to the bond device. This has the nice side-effect of also fixing an
inconsistency - when the MAC address of the bond dev is set after its
creation, but prior to having slaves, it's not kept and the first slave's
MAC is cloned. The only way to keep the MAC was to create the bond device
with the MAC address set (e.g. through ip link). In all cases if the
bond device is left without any slaves - its MAC gets reset to a random
one as before.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
 drivers/net/bonding/bond_main.c | 7 ++-----
 drivers/net/bonding/bonding.h   | 1 -
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2e8b9f1..0da3c12 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1596,7 +1596,7 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
 
 	/* If this is the first slave, then we need to set the master's hardware
 	 * address to be the same as the slave's. */
-	if (bond->slave_cnt == 0 && bond->dev_addr_from_first)
+	if (!bond->slave_cnt && bond->dev->addr_assign_type == NET_ADDR_RANDOM)
 		bond_set_dev_addr(bond->dev, slave_dev);
 
 	new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);
@@ -2041,7 +2041,6 @@ static int __bond_release_one(struct net_device *bond_dev,
 	if (bond->slave_cnt == 0) {
 		bond_set_carrier(bond);
 		eth_hw_addr_random(bond_dev);
-		bond->dev_addr_from_first = true;
 
 		if (bond_vlan_used(bond)) {
 			pr_warning("%s: Warning: clearing HW address of %s while it still has VLANs.\n",
@@ -4800,10 +4799,8 @@ static int bond_init(struct net_device *bond_dev)
 
 	/* Ensure valid dev_addr */
 	if (is_zero_ether_addr(bond_dev->dev_addr) &&
-	    bond_dev->addr_assign_type == NET_ADDR_PERM) {
+	    bond_dev->addr_assign_type == NET_ADDR_PERM)
 		eth_hw_addr_random(bond_dev);
-		bond->dev_addr_from_first = true;
-	}
 
 	return 0;
 }
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index c6c8d03..42d1c65 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -248,7 +248,6 @@ struct bonding {
 	/* debugging support via debugfs */
 	struct	 dentry *debug_dir;
 #endif /* CONFIG_DEBUG_FS */
-	bool	dev_addr_from_first;
 };
 
 static inline bool bond_vlan_used(struct bonding *bond)
-- 
1.8.1.4

[PATCH net-next 3/3] bonding: when cloning a MAC use NET_ADDR_STOLEN

[Nikolay Aleksandrov]

A simple semantic change, when a slave's MAC is cloned by the bond
master then set addr_assign_type to NET_ADDR_STOLEN instead of
NET_ADDR_SET. Also use bond_set_dev_addr() in BOND_FOM_ACTIVE mode
to change the bond's MAC address because the assign_type has to be
set properly.

Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
---
Shouldn't we have the pr_debugs on 1 line ? This is not called too often.

 drivers/net/bonding/bond_main.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 0da3c12..ec44580b 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -833,6 +833,24 @@ static void bond_hw_addr_swap(struct bonding *bond, struct slave *new_active,
 	}
 }
 
+/**
+ * bond_set_dev_addr - clone slave's address to bond
+ * @bond_dev: bond net device
+ * @slave_dev: slave net device
+ *
+ * Should be called with RTNL held.
+ */
+static void bond_set_dev_addr(struct net_device *bond_dev,
+			      struct net_device *slave_dev)
+{
+	pr_debug("bond_dev=%p\n", bond_dev);
+	pr_debug("slave_dev=%p\n", slave_dev);
+	pr_debug("slave_dev->addr_len=%d\n", slave_dev->addr_len);
+	memcpy(bond_dev->dev_addr, slave_dev->dev_addr, slave_dev->addr_len);
+	bond_dev->addr_assign_type = NET_ADDR_STOLEN;
+	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
+}
+
 /*
  * bond_do_fail_over_mac
  *
@@ -855,11 +873,9 @@ static void bond_do_fail_over_mac(struct bonding *bond,
 	switch (bond->params.fail_over_mac) {
 	case BOND_FOM_ACTIVE:
 		if (new_active) {
-			memcpy(bond->dev->dev_addr,  new_active->dev->dev_addr,
-			       new_active->dev->addr_len);
 			write_unlock_bh(&bond->curr_slave_lock);
 			read_unlock(&bond->lock);
-			call_netdevice_notifiers(NETDEV_CHANGEADDR, bond->dev);
+			bond_set_dev_addr(bond->dev, new_active->dev);
 			read_lock(&bond->lock);
 			write_lock_bh(&bond->curr_slave_lock);
 		}
@@ -1290,17 +1306,6 @@ static void bond_netpoll_cleanup(struct net_device *bond_dev)
 
 /*---------------------------------- IOCTL ----------------------------------*/
 
-static void bond_set_dev_addr(struct net_device *bond_dev,
-			      struct net_device *slave_dev)
-{
-	pr_debug("bond_dev=%p\n", bond_dev);
-	pr_debug("slave_dev=%p\n", slave_dev);
-	pr_debug("slave_dev->addr_len=%d\n", slave_dev->addr_len);
-	memcpy(bond_dev->dev_addr, slave_dev->dev_addr, slave_dev->addr_len);
-	bond_dev->addr_assign_type = NET_ADDR_SET;
-	call_netdevice_notifiers(NETDEV_CHANGEADDR, bond_dev);
-}
-
 static netdev_features_t bond_fix_features(struct net_device *dev,
 	netdev_features_t features)
 {
-- 
1.8.1.4

Re: [PATCH net-next 0/3] bonding: struct bonding cleanup and MAC set fixes

[David Miller]

From: Nikolay Aleksandrov <nikolay@redhat.com>
Date: Wed, 26 Jun 2013 17:13:36 +0200

> This patchset removes some unnecessary struct bonding members. It also
> fixes MAC setting inconsistencies (i.e. if a bond's MAC is set after
> creation but prior to enslaving, it is lost and the first slave's MAC is
> cloned again). Before these patches the only way to keep a user-defined MAC
> is either to set it after enslaving or to create the bond with that MAC.
> Patch 01 - removes setup_by_slave member
> Patch 02 - removes dev_addr_from_first member and does the MAC fixing
> Patch 03 - introduces the use of NET_ADDR_STOLEN when a MAC is cloned

These cleanups seem pretty reasonabl, all applied.

And yes I agree that those 3 pr_debug()'s in bond_set_dev_addr() should
be condensed into just one.  Feel free to send a patch which does that.

Thanks.