bonding: remove rlb_enabled and alb_set_mac_address()

Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself.

Quoted 6 lines · Veaceslav Falico

Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself.

So with this patch @ hand, no more touching of the underlying device dev_addr field directly -- or we still have more accesses of this such, e.g in bond_alb_set_mac_address() and/or friends?

Or.

Quoted 5 lines · Or Gerlitz

So with this patch @ hand, no more touching of the underlying device dev_addr field directly -- or we still have more accesses of this such, e.g in bond_alb_set_mac_address() and/or friends?

Sorry, but no :(. I'm still working on it, and already sent several patchsets approaching it :).

Hopefully it will be ready next week. If you need it urgently - patches welcome :)

Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself.

So remove it, as it's not needed.

From the comments and code, it seems like it should be called for TLB

CC Jay Vosburgh <[email protected]>

CC Andy Gospodarek <[email protected]>

Signed-off-by Veaceslav Falico <[email protected]>

---
 drivers/net/bonding/bond_alb.c | 61 ------------------------------------------
 1 file changed, 61 deletions(-)

drivers/net/bonding/bond_alb.c +0 -61

index 965518b..1d772b5 100644
@@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav
 	return 0;
 }
 
-/**
- * alb_set_mac_address
- * @bond:
- * @addr:
- *
- * In TLB mode all slaves are configured to the bond's hw address, but set
- * their dev_addr field to different addresses (based on their permanent hw
- * addresses).
- *
- * For each slave, this function sets the interface to the new address and then
- * changes its dev_addr field to its previous value.
- *
- * Unwinding assumes bond's mac address has not yet changed.
- */
-static int alb_set_mac_address(struct bonding *bond, void *addr)
-{
-	struct slave *slave, *rollback_slave;
-	struct list_head *iter;
-	struct sockaddr sa;
-	char tmp_addr[ETH_ALEN];
-	int res;
-
-	if (BOND_MODE(bond) == BOND_MODE_ALB)
-		return 0;
-
-	bond_for_each_slave(bond, slave, iter) {
-		/* save net_device's current hw address */
-		ether_addr_copy(tmp_addr, slave->dev->dev_addr);
-
-		res = dev_set_mac_address(slave->dev, addr);
-
-		/* restore net_device's hw address */
-		ether_addr_copy(slave->dev->dev_addr, tmp_addr);
-
-		if (res)
-			goto unwind;
-	}
-
-	return 0;
-
-unwind:
-	memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len);
-	sa.sa_family = bond->dev->type;
-
-	/* unwind from head to the slave that failed */
-	bond_for_each_slave(bond, rollback_slave, iter) {
-		if (rollback_slave == slave)
-			break;
-		ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr);
-		dev_set_mac_address(rollback_slave->dev, &sa);
-		ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr);
-	}
-
-	return res;
-}
-
 /************************ exported alb funcions ************************/
 
 int bond_alb_initialize(struct bonding *bond)

@@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct sockaddr *sa = addr;
 	struct slave *swap_slave;
-	int res;
 
 	if (!is_valid_ether_addr(sa->sa_data))
 		return -EADDRNOTAVAIL;
 
-	res = alb_set_mac_address(bond, addr);
-	if (res)
-		return res;
-
 	memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len);
 
 	/* If there is no curr_active_slave there is nothing else to do.

Quoted 6 lines · Veaceslav Falico

Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself.

Quoted 1 line

So remove it, as it's not needed.

From the comments and code, it seems like it should be called for TLB mode as well as it does that weird half-setting of the address that TLB seems to want. Is that not needed any more?

-vlad

Quoted 92 lines

CC Jay Vosburgh <[email protected]>

CC Andy Gospodarek <[email protected]>

Signed-off-by Veaceslav Falico <[email protected]>

---
 drivers/net/bonding/bond_alb.c | 61 ------------------------------------------
 1 file changed, 61 deletions(-)

drivers/net/bonding/bond_alb.c +0 -61

index 965518b..1d772b5 100644
@@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav
 	return 0;
 }
 
-/**
- * alb_set_mac_address
- * @bond:
- * @addr:
- *
- * In TLB mode all slaves are configured to the bond's hw address, but set
- * their dev_addr field to different addresses (based on their permanent hw
- * addresses).
- *
- * For each slave, this function sets the interface to the new address and then
- * changes its dev_addr field to its previous value.
- *
- * Unwinding assumes bond's mac address has not yet changed.
- */
-static int alb_set_mac_address(struct bonding *bond, void *addr)
-{
-	struct slave *slave, *rollback_slave;
-	struct list_head *iter;
-	struct sockaddr sa;
-	char tmp_addr[ETH_ALEN];
-	int res;
-
-	if (BOND_MODE(bond) == BOND_MODE_ALB)
-		return 0;
-
-	bond_for_each_slave(bond, slave, iter) {
-		/* save net_device's current hw address */
-		ether_addr_copy(tmp_addr, slave->dev->dev_addr);
-
-		res = dev_set_mac_address(slave->dev, addr);
-
-		/* restore net_device's hw address */
-		ether_addr_copy(slave->dev->dev_addr, tmp_addr);
-
-		if (res)
-			goto unwind;
-	}
-
-	return 0;
-
-unwind:
-	memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len);
-	sa.sa_family = bond->dev->type;
-
-	/* unwind from head to the slave that failed */
-	bond_for_each_slave(bond, rollback_slave, iter) {
-		if (rollback_slave == slave)
-			break;
-		ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr);
-		dev_set_mac_address(rollback_slave->dev, &sa);
-		ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr);
-	}
-
-	return res;
-}
-
 /************************ exported alb funcions ************************/
 
 int bond_alb_initialize(struct bonding *bond)

@@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct sockaddr *sa = addr;
 	struct slave *swap_slave;
-	int res;
 
 	if (!is_valid_ether_addr(sa->sa_data))
 		return -EADDRNOTAVAIL;
 
-	res = alb_set_mac_address(bond, addr);
-	if (res)
-		return res;
-
 	memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len);
 
 	/* If there is no curr_active_slave there is nothing else to do.

Currently it's called only from bond_alb_set_mac_address(), which is called only for ALB mode, and it does nothing in case the mode is ALB. So, basically, it's a no-op. All the needed functionality (modifying the active slave's mac address, per example) is handled by the bond_alb_set_mac_address() itself.

So remove it, as it's not needed.

CC Jay Vosburgh <[email protected]>

CC Andy Gospodarek <[email protected]>

Signed-off-by Veaceslav Falico <[email protected]>

---
drivers/net/bonding/bond_alb.c | 61 ------------------------------------------

1 file changed, 61 deletions(-)

drivers/net/bonding/bond_alb.c +0 -61

index 965518b..1d772b5 100644
@@ -1256,62 +1256,6 @@ static int alb_handle_addr_collision_on_attach(struct bonding *bond, struct slav
 	return 0;
 }
 
-/**
- * alb_set_mac_address
- * @bond:
- * @addr:
- *
- * In TLB mode all slaves are configured to the bond's hw address, but set
- * their dev_addr field to different addresses (based on their permanent hw
- * addresses).
- *
- * For each slave, this function sets the interface to the new address and then
- * changes its dev_addr field to its previous value.
- *
- * Unwinding assumes bond's mac address has not yet changed.
- */
-static int alb_set_mac_address(struct bonding *bond, void *addr)
-{
-	struct slave *slave, *rollback_slave;
-	struct list_head *iter;
-	struct sockaddr sa;
-	char tmp_addr[ETH_ALEN];
-	int res;
-
-	if (BOND_MODE(bond) == BOND_MODE_ALB)
-		return 0;
-
-	bond_for_each_slave(bond, slave, iter) {
-		/* save net_device's current hw address */
-		ether_addr_copy(tmp_addr, slave->dev->dev_addr);
-
-		res = dev_set_mac_address(slave->dev, addr);
-
-		/* restore net_device's hw address */
-		ether_addr_copy(slave->dev->dev_addr, tmp_addr);
-
-		if (res)
-			goto unwind;
-	}
-
-	return 0;
-
-unwind:
-	memcpy(sa.sa_data, bond->dev->dev_addr, bond->dev->addr_len);
-	sa.sa_family = bond->dev->type;
-
-	/* unwind from head to the slave that failed */
-	bond_for_each_slave(bond, rollback_slave, iter) {
-		if (rollback_slave == slave)
-			break;
-		ether_addr_copy(tmp_addr, rollback_slave->dev->dev_addr);
-		dev_set_mac_address(rollback_slave->dev, &sa);
-		ether_addr_copy(rollback_slave->dev->dev_addr, tmp_addr);
-	}
-
-	return res;
-}
-
 /************************ exported alb funcions ************************/
 
 int bond_alb_initialize(struct bonding *bond)

@@ -1777,15 +1721,10 @@ int bond_alb_set_mac_address(struct net_device *bond_dev, void *addr)
 	struct bonding *bond = netdev_priv(bond_dev);
 	struct sockaddr *sa = addr;
 	struct slave *swap_slave;
-	int res;
 
 	if (!is_valid_ether_addr(sa->sa_data))
 		return -EADDRNOTAVAIL;
 
-	res = alb_set_mac_address(bond, addr);
-	if (res)
-		return res;
-
 	memcpy(bond_dev->dev_addr, sa->sa_data, bond_dev->addr_len);
 
 	/* If there is no curr_active_slave there is nothing else to do.

Signature · 1 line

1.8.4

Currently rlb_enabled works as
rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB)
so we can always use this verification and drop maintenance of this variable.

alb_set_mac_address(), OTOH, isn't used at all (as the mac changing logic is used in other parts), and isn't ever called, so it's safe to remove it as well.

CC Jay Vosburgh <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Veaceslav Falico <[email protected]>

From: Veaceslav Falico <[email protected]>
Date: Thu, 22 May 2014 15:12:02 +0200

Quoted 5 lines

Currently rlb_enabled works as
rlb_enabled == (BOND_MODE(bond) == BOND_MODE_ALB)
so we can always use this verification and drop maintenance of this variable.

Quoted 8 lines

alb_set_mac_address(), OTOH, isn't used at all (as the mac changing logic is used in other parts), and isn't ever called, so it's safe to remove it as well.

CC Jay Vosburgh <[email protected]>

CC Andy Gospodarek <[email protected]>

CC [email protected]

Signed-off-by Veaceslav Falico <[email protected]>

I agree with Jay's feedback and that you should:

1) Just go to the new MAC stash scheme in net-next instead of trying to fix something you intend to immediately remove.

2) Something to fix this as minimally as possible for 'net' and -stable should be done as well.

I'm therefore not applying this series to the tree.