[PATCH] bonding: check for assigned mac before adopting the slaves mac address

[PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Strand]

Restore the check for a missing mac address before adopting the first
slaves as it's own. This regression was introduced in:
http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.36.y.git;a=commit;h=c20811a79e671a6a1fe86a8c1afe04aca8a7f085

Signed-off-by: David Strand dpstrand@gmail.com
---
diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c	2010-11-24 11:36:58.125640000 -0800
+++ b/drivers/net/bonding/bond_main.c	2010-11-24 11:40:58.175640000 -0800
@@ -1577,8 +1577,9 @@ int bond_enslave(struct net_device *bond
 	/* 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)
-		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
-		       slave_dev->addr_len);
+		if (is_zero_ether_addr(bond->dev->dev_addr))
+			memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
+			       slave_dev->addr_len);


 	new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[Jay Vosburgh]

David Strand <dpstrand@gmail.com> wrote:

>Restore the check for a missing mac address before adopting the first
>slaves as it's own. This regression was introduced in:
>http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.36.y.git;a=commit;h=c20811a79e671a6a1fe86a8c1afe04aca8a7f085

	How exactly is this a regression?  The above referenced patch
changes the method used to decide if the bonding master needs to have
it's MAC address set.  The original way was "bonding master's MAC is
zero," after the above, it's "adding first slave."

	Do you have some use case that manually sets the master's MAC
address prior to adding any slaves?

	-J

>Signed-off-by: David Strand dpstrand@gmail.com
>---
>diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>--- a/drivers/net/bonding/bond_main.c	2010-11-24 11:36:58.125640000 -0800
>+++ b/drivers/net/bonding/bond_main.c	2010-11-24 11:40:58.175640000 -0800
>@@ -1577,8 +1577,9 @@ int bond_enslave(struct net_device *bond
> 	/* 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)
>-		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>-		       slave_dev->addr_len);
>+		if (is_zero_ether_addr(bond->dev->dev_addr))
>+			memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>+			       slave_dev->addr_len);
>
>
> 	new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);

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

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[Laurent Chavey]

based on the new code, the behavior of ifenslave works the same.


>-       if (bond->slave_cnt == 0)
>+       if (is_zero_ether_addr(bond->dev->dev_addr))
>-              memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>-                            slave_dev->addr_len);
> +            bond_sethwaddr(bond, slave_dev);


On Wed, Nov 24, 2010 at 3:33 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
> David Strand <dpstrand@gmail.com> wrote:
>
>>Restore the check for a missing mac address before adopting the first
>>slaves as it's own. This regression was introduced in:
>>http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.36.y.git;a=commit;h=c20811a79e671a6a1fe86a8c1afe04aca8a7f085
>
>        How exactly is this a regression?  The above referenced patch
> changes the method used to decide if the bonding master needs to have
> it's MAC address set.  The original way was "bonding master's MAC is
> zero," after the above, it's "adding first slave."
>
>        Do you have some use case that manually sets the master's MAC
> address prior to adding any slaves?
>
>        -J
>
>>Signed-off-by: David Strand dpstrand@gmail.com
>>---
>>diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>--- a/drivers/net/bonding/bond_main.c  2010-11-24 11:36:58.125640000 -0800
>>+++ b/drivers/net/bonding/bond_main.c  2010-11-24 11:40:58.175640000 -0800
>>@@ -1577,8 +1577,9 @@ int bond_enslave(struct net_device *bond
>>       /* 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)
>>-              memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>>-                     slave_dev->addr_len);
>>+              if (is_zero_ether_addr(bond->dev->dev_addr))
>>+                      memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
>>+                             slave_dev->addr_len);
>>
>>
>>       new_slave = kzalloc(sizeof(struct slave), GFP_KERNEL);
>
> ---
>        -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
--------------------------------------------------------------------------------

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Strand]

We have a use case where we assign a mac to the bond device, because
the slave device configuration may change periodically. With older
kernels, it honored the assigned mac and everything was fine, with
2.6.36 it now uses the mac of whatever slave device is first instead
of our assigned one.

ifenslave code and documentation appears to still support the old way,
where a bond assigned mac will reign supreme, so this patch restores
that behavior.

On Wed, Nov 24, 2010 at 3:33 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>        How exactly is this a regression?  The above referenced patch
> changes the method used to decide if the bonding master needs to have
> it's MAC address set.  The original way was "bonding master's MAC is
> zero," after the above, it's "adding first slave."
>
>        Do you have some use case that manually sets the master's MAC
> address prior to adding any slaves?
>
>        -J

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[Jay Vosburgh]

David Strand <dpstrand@gmail.com> wrote:

>We have a use case where we assign a mac to the bond device, because
>the slave device configuration may change periodically. With older
>kernels, it honored the assigned mac and everything was fine, with
>2.6.36 it now uses the mac of whatever slave device is first instead
>of our assigned one.
>
>ifenslave code and documentation appears to still support the old way,
>where a bond assigned mac will reign supreme, so this patch restores
>that behavior.

	Ok, fair enough.  If we want to get back to the original
behavior, however, your patch should only test for zero MAC address
instead of testing for zero MAC address in addition to first slave.

	-J

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

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Strand]

On Thu, Nov 25, 2010 at 7:26 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>
>        Ok, fair enough.  If we want to get back to the original
> behavior, however, your patch should only test for zero MAC address
> instead of testing for zero MAC address in addition to first slave.
>
>        -J
>
> ---
>        -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>

Understood, that makes sense. The updated patch is below.

---
diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c	2010-11-24 11:36:58.125640000 -0800
+++ b/drivers/net/bonding/bond_main.c	2010-12-01 10:12:33.728640001 -0800
@@ -1576,7 +1576,7 @@ int bond_enslave(struct net_device *bond

 	/* 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)
+	if (is_zero_ether_addr(bond->dev->dev_addr))
 		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
 		       slave_dev->addr_len);

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Miller]

From: David Strand <dpstrand@gmail.com>
Date: Wed, 1 Dec 2010 10:25:19 -0800

> On Thu, Nov 25, 2010 at 7:26 PM, Jay Vosburgh <fubar@us.ibm.com> wrote:
>>
>>        Ok, fair enough.  If we want to get back to the original
>> behavior, however, your patch should only test for zero MAC address
>> instead of testing for zero MAC address in addition to first slave.
>>
>>        -J
>>
>> ---
>>        -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>>
> 
> Understood, that makes sense. The updated patch is below.

You need to provide a fresh posting of your patch, with a full
commit messag and proper Subject: line, as well as a proper
"Signed-off-by: " tag.

See Documentation/SubmittingPatches

[PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Strand]

Restore the check for an unassigned mac address before adopting the
first slaves as it's own. The change in behavior was introduced with
the following patch:
http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.36.y.git;a=commit;h=c20811a79e671a6a1fe86a8c1afe04aca8a7f085

Signed-off-by: David Strand <dpstrand@gmail.com>
---
diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c	2010-11-24 11:36:58.125640000 -0800
+++ b/drivers/net/bonding/bond_main.c	2010-12-01 10:12:33.728640001 -0800
@@ -1576,7 +1576,7 @@ int bond_enslave(struct net_device *bond

 	/* 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)
+	if (is_zero_ether_addr(bond->dev->dev_addr))
 		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
 		       slave_dev->addr_len);

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[Jay Vosburgh]

From: David Strand <dpstrand@gmail.com>

Restore the check for an unassigned mac address before adopting the
first slaves as it's own. The change in behavior was introduced by:

commit c20811a79e671a6a1fe86a8c1afe04aca8a7f085
Author: Jiri Pirko <jpirko@redhat.com>

    bonding: move dev_addr cpy to bond_enslave


Signed-off-by: David Strand <dpstrand@gmail.com>
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

---
diff -uprN a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
--- a/drivers/net/bonding/bond_main.c	2010-11-24 11:36:58.125640000 -0800
+++ b/drivers/net/bonding/bond_main.c	2010-12-01 10:12:33.728640001 -0800
@@ -1576,7 +1576,7 @@ int bond_enslave(struct net_device *bond

 	/* 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)
+	if (is_zero_ether_addr(bond->dev->dev_addr))
 		memcpy(bond->dev->dev_addr, slave_dev->dev_addr,
 		       slave_dev->addr_len);

Re: [PATCH] bonding: check for assigned mac before adopting the slaves mac address

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Wed, 01 Dec 2010 11:21:08 -0800

> 
> From: David Strand <dpstrand@gmail.com>
> 
> Restore the check for an unassigned mac address before adopting the
> first slaves as it's own. The change in behavior was introduced by:
> 
> commit c20811a79e671a6a1fe86a8c1afe04aca8a7f085
> Author: Jiri Pirko <jpirko@redhat.com>
> 
>     bonding: move dev_addr cpy to bond_enslave
> 
> 
> Signed-off-by: David Strand <dpstrand@gmail.com>
> Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>

Applied, thanks.