Regression in bonding between 2.6.26.8 and 2.6.27.6

Regression in bonding between 2.6.26.8 and 2.6.27.6

[Jesper Krogh]

Hi.

I have something that looks like a regression in bonding between 
2.6.26.8 and 2.6.27.6 (I'll try the mid-steps later).

Setup: LACP bond(mode=4,mmimon=100) with 3 NIC's and dhcp on top (static 
ip didn't work either).

Problem: The bond doesn't get up after bootup. Subsequence ifdown/ifup 
brings it up.

I suspect it it timing related. The interface being configured before 
it's ready:
root@quad01:~# dmesg | egrep '(dhc|bond)'
[   12.421963] bonding: MII link monitoring set to 100 ms
[   12.483370] bonding: bond0: enslaving eth0 as a backup interface with 
an up link.
[   12.523372] bonding: bond0: enslaving eth1 as a backup interface with 
an up link.
[   12.611731] bonding: bond0: enslaving eth2 as a backup interface with 
a down link.
[   12.780816] warning: `dhclient3' uses 32-bit capabilities (legacy 
support in use)
[   15.720491] bonding: bond0: link status definitely up for interface eth2.
[   87.800324] bond0: no IPv6 routers present


The setup is a 3 NIC bond on a Sun X2200 dual-cpu Quad-core server.
I have similar bond on a X4600 where they works with 2.6.27.6 so I 
suspect that the difference is that the X4600 has all NIC's from the
same vendor where as the X2200 has 2 Broadcom NIC's and 2 NVidia nics.

root@quad01:~# lspci | grep -i ethernet
00:08.0 Bridge: nVidia Corporation MCP55 Ethernet (rev a3)
00:09.0 Bridge: nVidia Corporation MCP55 Ethernet (rev a3)
06:04.0 Ethernet controller: Broadcom Corporation NetXtreme BCM5715 
Gigabit Ethernet (rev a3)
06:04.1 Ethernet controller: Broadcom Corporation NetXtreme BCM5715 
Gigabit Ethernet (rev a3)




-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jesper Krogh]

Ok, update, Rome wasn't build in one day.

A collegue of mine (on CC) found time to do a bisect of this on.

The offending commit seems to be:

bonding: refactor mii monitor

Refactor mii monitor.  As with the previous ARP monitor refactor,
the motivation for this is to handle locking rationally (in this case,
removing conditional locking) and generally clean up the code.

This patch breaks up the monolithic mii monitor into two phases:
an inspection phase, followed by an optional commit phase.  The commit phase
is the only portion that requires RTNL or makes changes to state, and is
only called when inspection finds something to change.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
Signed-off-by: Jeff Garzik <jgarzik@redhat.com>


A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed 
subsequently.. but still exists in 2.6.27-newest.  (havent tested 
2.6.28-newest yet).

Any ideas of what the "fixing" commit is .. or should that also be 
bisected?

Dmesg of 2-6-29-rc6 working system here:
http://krogh.cc/~jesper/dmesg-2.6.29-rc6.txt .. shows that when bonding 
is initialized it doesnt have link up on any of the interfaces.


git bisect log

# bad: [adee14b2e1557d0a8559f29681732d05a89dfc35] Linux 2.6.27-rc6
# good: [63e0e67b17dc233f93f709610971bbfadc97f75e] Linux 2.6.26.8
git-bisect start 'v2.6.27-rc6' 'v2.6.26.8'
# good: [d536b1f86591fb081c7a56eab04e711eb4dab951] x86: fix crash due to 
missing debugctlmsr on AMD K6-3
git-bisect good d536b1f86591fb081c7a56eab04e711eb4dab951
# good: [d536b1f86591fb081c7a56eab04e711eb4dab951] x86: fix crash due to 
missing debugctlmsr on AMD K6-3
git-bisect good d536b1f86591fb081c7a56eab04e711eb4dab951
# good: [8d0b1c51eb8375f88c0886d2e9f71881e19d42a7] gbefb: cmap FIFO timeout
git-bisect good 8d0b1c51eb8375f88c0886d2e9f71881e19d42a7
# bad: [ab1666c1364a209e6141d7c14e47a42b5f00eca2] USB: quirk PLL power 
down mode
git-bisect bad ab1666c1364a209e6141d7c14e47a42b5f00eca2
# good: [37193fb4639fa94f91cbbab1e8aca596300e1d94] docbook: fix s390 
fatal error after header files moved
git-bisect good 37193fb4639fa94f91cbbab1e8aca596300e1d94
# bad: [f2d7499be1b1fe1cd8a5e6a01c1f44173894a241] Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-2.6
git-bisect bad f2d7499be1b1fe1cd8a5e6a01c1f44173894a241
# good: [b588e2bbd7b872c9cdf635fe0f04840534e72443] Merge 
git://git.kernel.org/pub/scm/linux/kernel/git/wim/linux-2.6-watchdog
git-bisect good b588e2bbd7b872c9cdf635fe0f04840534e72443
# bad: [86b89eed9aca2a4a335b9c1bf7380f9183db431f] ath9k: Revamp wireless 
mode usage
git-bisect bad 86b89eed9aca2a4a335b9c1bf7380f9183db431f
# good: [9a5d3414202a21ed4b053657345ea0fd492d513a] 3c59x: use 
netdev_alloc_skb
git-bisect good 9a5d3414202a21ed4b053657345ea0fd492d513a
# bad: [3859069bc3358772b08bd91efe9edec39a746ea8] Merge branch 
'for-jeff' of 
git://git.kernel.org/pub/scm/linux/kernel/git/chris/linux-2.6 into tmp
git-bisect bad 3859069bc3358772b08bd91efe9edec39a746ea8
# good: [52e8a6a2d8dc19002d1757870d16051157ce999c] WAN: Convert 
Zilog-based drivers to generic HDLC
git-bisect good 52e8a6a2d8dc19002d1757870d16051157ce999c
# bad: [0d7a3681232f545c6a59f77e60f7667673ef0e93] net/core: Allow 
certain receives on inactive slave.
git-bisect bad 0d7a3681232f545c6a59f77e60f7667673ef0e93
# bad: [f0c76d61779b153dbfb955db3f144c62d02173c2] bonding: refactor mii 
monitor
git-bisect bad f0c76d61779b153dbfb955db3f144c62d02173c2
# good: [c16d118537cadb21d186e35aebad90a13cd78846] [netdrvr] Drivers 
should not set IFF_* flag themselves
git-bisect good c16d118537cadb21d186e35aebad90a13cd78846

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jay Vosburgh]

Jesper Krogh <jesper@krogh.cc> wrote:
[...]
>The offending commit seems to be:
>
>bonding: refactor mii monitor
>
>Refactor mii monitor.  As with the previous ARP monitor refactor,
>the motivation for this is to handle locking rationally (in this case,
>removing conditional locking) and generally clean up the code.
>
>This patch breaks up the monolithic mii monitor into two phases:
>an inspection phase, followed by an optional commit phase.  The commit phase
>is the only portion that requires RTNL or makes changes to state, and is
>only called when inspection finds something to change.
>
>Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
>Signed-off-by: Jeff Garzik <jgarzik@redhat.com>
>
>
>A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed
>subsequently.. but still exists in 2.6.27-newest.  (havent tested
>2.6.28-newest yet).
>
>Any ideas of what the "fixing" commit is .. or should that also be
>bisected?

	I went back and looked at your earlier mail.  Since you're using
802.3ad mode, my first guess would be this commit:

commit fd989c83325cb34795bc4d4aa6b13c06f90eac99
Author: Jay Vosburgh <fubar@us.ibm.com>
Date:   Tue Nov 4 17:51:16 2008 -0800

    bonding: alternate agg selection policies for 802.3ad
    
        This patch implements alternative aggregator selection policies
    for 802.3ad.  The existing policy, now termed "stable," selects the active
    aggregator by greatest bandwidth, and only reselects a new aggregator
    if the active aggregator is entirely disabled (no more ports or all ports
    down).
    
        This patch adds two new policies: bandwidth and count, selecting
    the active aggregator by total bandwidth (like the stable policy) or by
    the number of ports in the aggregator, respectively.  These two policies
    also differ from the stable policy in that they will reselect the active
    aggregator when availability-related changes occur in the bond (e.g.,
    link state change).
    
        This permits "gang failover" within 802.3ad, allowing redundant
    aggregators along parallel paths to always maintain the "best" aggregator
    as the active aggregator (rather than having to wait for the active to
    entirely fail).
    
        This patch also updates the driver version to 3.5.0.
    
    Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
    Signed-off-by: Jeff Garzik <jgarzik@redhat.com>


	This changed or refactored a great deal of the aggregator
selection logic, and it might be that it also fixed your problem by mere
happenstance.

	-J

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

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jesper Krogh]

Jay Vosburgh wrote:
> Jesper Krogh <jesper@krogh.cc> wrote:
> [...]
>> The offending commit seems to be:
>>
>> A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed
>> subsequently.. but still exists in 2.6.27-newest.  (havent tested
>> 2.6.28-newest yet).
>>
>> Any ideas of what the "fixing" commit is .. or should that also be
>> bisected?
> 
> 	I went back and looked at your earlier mail.  Since you're using
> 802.3ad mode, my first guess would be this commit:
> 
> commit fd989c83325cb34795bc4d4aa6b13c06f90eac99
> Author: Jay Vosburgh <fubar@us.ibm.com>
> Date:   Tue Nov 4 17:51:16 2008 -0800
> 
>     bonding: alternate agg selection policies for 802.3ad

That didn't do it.. I applied it to 2.6.27.19 but it didnt make that work.
dmesg | grep bond (2.6.27.19 + above patch).

[   13.643301] bonding: MII link monitoring set to 100 ms
[   13.730455] bonding: bond0: enslaving eth0 as a backup interface with 
an up link.
[   13.781934] bonding: bond0: enslaving eth1 as a backup interface with 
an up link.
[   13.904665] bonding: bond0: enslaving eth2 as a backup interface with 
a down link.
[   16.945264] bonding: bond0: link status definitely up for interface eth2.
[   75.040290] bond0: no IPv6 routers present

dmesg | grep bond (2.6.29-rc6)

$ ssh quad02 dmesg | grep bond
[   27.437877] bonding: MII link monitoring set to 100 ms
[   27.445246] ADDRCONF(NETDEV_UP): bond0: link is not ready
[   27.493260] bonding: bond0: enslaving eth0 as a backup interface with 
a down link.
[   27.521397] bonding: bond0: enslaving eth1 as a backup interface with 
a down link.
[   27.542332] bonding: bond0: Warning: No 802.3ad response from the 
link partner for any adapters in the bond
[   27.611509] bonding: bond0: enslaving eth2 as a backup interface with 
a down link.
[   27.617017] ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
[   27.642330] bonding: bond0: Warning: No 802.3ad response from the 
link partner for any adapters in the bond
[   30.042501] bonding: bond0: link status definitely up for interface eth1.
[   30.142505] bonding: bond0: link status definitely up for interface eth0.
[   30.742547] bonding: bond0: link status definitely up for interface eth2.
[   37.875044] bond0: no IPv6 routers present

I just tested 2.6.28.7.. it still broken. So the fix probably has to be 
somewhere in the post 2.6.28 sets.



-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jay Vosburgh]

Jesper Krogh <jesper@krogh.cc> wrote:

>Jay Vosburgh wrote:
>> Jesper Krogh <jesper@krogh.cc> wrote:
>> [...]
>>> The offending commit seems to be:
>>>
>>> A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed
>>> subsequently.. but still exists in 2.6.27-newest.  (havent tested
>>> 2.6.28-newest yet).
>>>
>>> Any ideas of what the "fixing" commit is .. or should that also be
>>> bisected?
>>
>> 	I went back and looked at your earlier mail.  Since you're using
>> 802.3ad mode, my first guess would be this commit:
>>
>> commit fd989c83325cb34795bc4d4aa6b13c06f90eac99
>> Author: Jay Vosburgh <fubar@us.ibm.com>
>> Date:   Tue Nov 4 17:51:16 2008 -0800
>>
>>     bonding: alternate agg selection policies for 802.3ad
>
>That didn't do it.. I applied it to 2.6.27.19 but it didnt make that work.
>dmesg | grep bond (2.6.27.19 + above patch).

	That was the only real functional change to 802.3ad, there are a
lot of other commits, but they're all style or cleanup sorts of things.

>[   13.643301] bonding: MII link monitoring set to 100 ms
>[   13.730455] bonding: bond0: enslaving eth0 as a backup interface with
>an up link.
>[   13.781934] bonding: bond0: enslaving eth1 as a backup interface with
>an up link.
>[   13.904665] bonding: bond0: enslaving eth2 as a backup interface with a
>down link.
>[   16.945264] bonding: bond0: link status definitely up for interface eth2.
>[   75.040290] bond0: no IPv6 routers present
>
>dmesg | grep bond (2.6.29-rc6)
>
>$ ssh quad02 dmesg | grep bond
>[   27.437877] bonding: MII link monitoring set to 100 ms
>[   27.445246] ADDRCONF(NETDEV_UP): bond0: link is not ready
>[   27.493260] bonding: bond0: enslaving eth0 as a backup interface with a
>down link.
>[   27.521397] bonding: bond0: enslaving eth1 as a backup interface with a
>down link.
>[   27.542332] bonding: bond0: Warning: No 802.3ad response from the link
>partner for any adapters in the bond
>[   27.611509] bonding: bond0: enslaving eth2 as a backup interface with a
>down link.
>[   27.617017] ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
>[   27.642330] bonding: bond0: Warning: No 802.3ad response from the link
>partner for any adapters in the bond
>[   30.042501] bonding: bond0: link status definitely up for interface eth1.
>[   30.142505] bonding: bond0: link status definitely up for interface eth0.
>[   30.742547] bonding: bond0: link status definitely up for interface eth2.
>[   37.875044] bond0: no IPv6 routers present
>
>I just tested 2.6.28.7.. it still broken. So the fix probably has to be
>somewhere in the post 2.6.28 sets.

	It looks like the above two tests are on different machines, or
were at least done with different network cards.  Is that the case?

	I'm just wondering if what you're seeing is somehow tied to the
network devices' respective autonegotiation speeds, or some difference
in the device drivers.  The first dmesg looks to have one slow (3 sec)
and two fast ones; the second dmesg looks to have all slow devices.

	Have you tried the kernels the other way around (the first
kernel on the second machine, and vice versa)?

	I'll compile 2.6.28.7 here and see if it works for me.

	-J

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

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jesper Krogh]

Jay Vosburgh wrote:
> Jesper Krogh <jesper@krogh.cc> wrote:
> 
>> Jay Vosburgh wrote:
>>> Jesper Krogh <jesper@krogh.cc> wrote:
>>> [...]
>>>> The offending commit seems to be:
>>>>
>>>> A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed
>>>> subsequently.. but still exists in 2.6.27-newest.  (havent tested
>>>> 2.6.28-newest yet).
>>>>
>>>> Any ideas of what the "fixing" commit is .. or should that also be
>>>> bisected?
>>> 	I went back and looked at your earlier mail.  Since you're using
>>> 802.3ad mode, my first guess would be this commit:
>>>
>>> commit fd989c83325cb34795bc4d4aa6b13c06f90eac99
>>> Author: Jay Vosburgh <fubar@us.ibm.com>
>>> Date:   Tue Nov 4 17:51:16 2008 -0800
>>>
>>>     bonding: alternate agg selection policies for 802.3ad
>> That didn't do it.. I applied it to 2.6.27.19 but it didnt make that work.
>> dmesg | grep bond (2.6.27.19 + above patch).
> 
> 	That was the only real functional change to 802.3ad, there are a
> lot of other commits, but they're all style or cleanup sorts of things.
> 
>> [   13.643301] bonding: MII link monitoring set to 100 ms
>> [   13.730455] bonding: bond0: enslaving eth0 as a backup interface with
>> an up link.
>> [   13.781934] bonding: bond0: enslaving eth1 as a backup interface with
>> an up link.
>> [   13.904665] bonding: bond0: enslaving eth2 as a backup interface with a
>> down link.
>> [   16.945264] bonding: bond0: link status definitely up for interface eth2.
>> [   75.040290] bond0: no IPv6 routers present
>>
>> dmesg | grep bond (2.6.29-rc6)
>>
>> $ ssh quad02 dmesg | grep bond
>> [   27.437877] bonding: MII link monitoring set to 100 ms
>> [   27.445246] ADDRCONF(NETDEV_UP): bond0: link is not ready
>> [   27.493260] bonding: bond0: enslaving eth0 as a backup interface with a
>> down link.
>> [   27.521397] bonding: bond0: enslaving eth1 as a backup interface with a
>> down link.
>> [   27.542332] bonding: bond0: Warning: No 802.3ad response from the link
>> partner for any adapters in the bond
>> [   27.611509] bonding: bond0: enslaving eth2 as a backup interface with a
>> down link.
>> [   27.617017] ADDRCONF(NETDEV_CHANGE): bond0: link becomes ready
>> [   27.642330] bonding: bond0: Warning: No 802.3ad response from the link
>> partner for any adapters in the bond
>> [   30.042501] bonding: bond0: link status definitely up for interface eth1.
>> [   30.142505] bonding: bond0: link status definitely up for interface eth0.
>> [   30.742547] bonding: bond0: link status definitely up for interface eth2.
>> [   37.875044] bond0: no IPv6 routers present
>>
>> I just tested 2.6.28.7.. it still broken. So the fix probably has to be
>> somewhere in the post 2.6.28 sets.
> 
> 	It looks like the above two tests are on different machines, or
> were at least done with different network cards.  Is that the case?

There is 12 Sun Fire X2200 in the rack, they are fully identical (some 
with a small difference in memory configuration as the only difference.

So yes, different machines, but same hardware (bought in the same 
shipment, etc. etc).

> 	I'm just wondering if what you're seeing is somehow tied to the
> network devices' respective autonegotiation speeds, or some difference
> in the device drivers.  The first dmesg looks to have one slow (3 sec)
> and two fast ones; the second dmesg looks to have all slow devices.
> 
> 	Have you tried the kernels the other way around (the first
> kernel on the second machine, and vice versa)?

Yes, I've randomly picked a machine in the set to do the test, they all 
falls out as "predicted".

> 	I'll compile 2.6.28.7 here and see if it works for me.



Jesper
-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected

[Jesper Krogh]

Jay Vosburgh wrote:
> Jesper Krogh <jesper@krogh.cc> wrote:
> 
>> Jay Vosburgh wrote:
>>> Jesper Krogh <jesper@krogh.cc> wrote:
>>> [...]
>>>> The offending commit seems to be:
>>>>
>>>> A test with a fresh 2.6.29-rc6 revealed that the problem has been fixed
>>>> subsequently.. but still exists in 2.6.27-newest.  (havent tested
>>>> 2.6.28-newest yet).
>>>>
>>>> Any ideas of what the "fixing" commit is .. or should that also be
>>>> bisected?
>>> 	I went back and looked at your earlier mail.  Since you're using
>>> 802.3ad mode, my first guess would be this commit:
>>>
>>> commit fd989c83325cb34795bc4d4aa6b13c06f90eac99
 >
> 	I'll compile 2.6.28.7 here and see if it works for me.

I appreciate that you spend time on it, but my feeling is that it 
definately isn't reproducible in all environments (otherwise we would
probably have seen a large cry by now).

I'm trying to bisect the "fix" down and hope that'll tell us something 
more.

If you do the test, remember, that it is not like "bonding isn't 
working". It just fails to initialize correctly at bootup and doesnt get 
  the link state by itself. Subsequently doing a /etc/init.d/networking 
restart brigs it correct up.

-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jesper Krogh]

Jesper Krogh wrote:
> I appreciate that you spend time on it, but my feeling is that it 
> definately isn't reproducible in all environments (otherwise we would
> probably have seen a large cry by now).
> 
> I'm trying to bisect the "fix" down and hope that'll tell us something 
> more.

Ok, The fixing commit seems to be: cb52deba12f27af90a46d2f8667a64888118a888

Applying it to 2.6.28.7 and 2.6.27.19 makes them both work.

It also explains why my e1000 based bonds didnt break, allthough the 
commitmessage doesnt mention anything about how it should effect 
bonding. Wouldn't it make sense to propose this patch for 2.6.27 and 
2.6.28 stable kernels?


commit cb52deba12f27af90a46d2f8667a64888118a888
Author: Ed Swierk <eswierk@arastra.com>
Date:   Mon Dec 1 12:24:43 2008 +0000

     forcedeth: power down phy when interface is down

     Bring the physical link down when the interface is down by placing 
the PHY
     in power-down state, unless WOL is enabled.  This mirrors the 
behavior of
     other drivers including e1000 and tg3.

     Without the patch, ifconfig down leaves the physical link up, which 
confuses
     datacenter users who expect the link lights both on the NIC and the 
switch to
     go out when they bring an interface down.

     Furthermore, even though the phy is powered on, autonegotiation 
stops working,
     so a normally gigabit link might suddenly become 100 Mbit 
half-duplex when the
     interface goes down, and become gigabit when it comes up again.

     Ayaz said:

       I would not include this patch until further testing is 
performed.  NVIDIA
       MCP chips use 3rd party PHY vendors.  By powering down the phy, 
it could
       have adverse affects on certain phys.

     Arthur Jones said:

       I just ran across this patch.  Tested on a Marvell 88E1121R (GigE 
PHY)
       and works great.  This is a very important feature for me.

     Signed-off-by: Ed Swierk <eswierk@arastra.com>
     Tested-by: Arthur Jones <ajones@riverbed.com>
     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
     Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/drivers/net/forcedeth.c b/drivers/net/forcedeth.c
index 0d7e575..12384df 100644
--- a/drivers/net/forcedeth.c
+++ b/drivers/net/forcedeth.c
@@ -1446,9 +1446,9 @@ static int phy_init(struct net_device *dev)
         /* some phys clear out pause advertisment on reset, set it back */
         mii_rw(dev, np->phyaddr, MII_ADVERTISE, reg);

-       /* restart auto negotiation */
+       /* restart auto negotiation, power down phy */
         mii_control = mii_rw(dev, np->phyaddr, MII_BMCR, MII_READ);
-       mii_control |= (BMCR_ANRESTART | BMCR_ANENABLE);
+       mii_control |= (BMCR_ANRESTART | BMCR_ANENABLE | BMCR_PDOWN);
         if (mii_rw(dev, np->phyaddr, MII_BMCR, mii_control)) {
                 return PHY_ERROR;
         }
@@ -5208,6 +5208,10 @@ static int nv_open(struct net_device *dev)

         dprintk(KERN_DEBUG "nv_open: begin\n");

+       /* power up phy */
+       mii_rw(dev, np->phyaddr, MII_BMCR,
+              mii_rw(dev, np->phyaddr, MII_BMCR, MII_READ) & ~BMCR_PDOWN);
+
         /* erase previous misconfiguration */
         if (np->driver_data & DEV_HAS_POWER_CNTRL)
                 nv_mac_reset(dev);
@@ -5401,6 +5405,10 @@ static int nv_close(struct net_device *dev)
         if (np->wolenabled) {
                 writel(NVREG_PFF_ALWAYS|NVREG_PFF_MYADDR, base + 
NvRegPacketFilterFlags);
                 nv_start_rx(dev);
+       } else {
+               /* power down phy */
+               mii_rw(dev, np->phyaddr, MII_BMCR,
+                      mii_rw(dev, np->phyaddr, MII_BMCR, 
MII_READ)|BMCR_PDOWN);
         }

         /* FIXME: power down nic */

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jay Vosburgh]

Jesper Krogh <jesper@krogh.cc> wrote:

>Jesper Krogh wrote:
>> I appreciate that you spend time on it, but my feeling is that it
>> definately isn't reproducible in all environments (otherwise we would
>> probably have seen a large cry by now).
>>
>> I'm trying to bisect the "fix" down and hope that'll tell us something
>> more.
>
>Ok, The fixing commit seems to be: cb52deba12f27af90a46d2f8667a64888118a888
>
>Applying it to 2.6.28.7 and 2.6.27.19 makes them both work.
>
>It also explains why my e1000 based bonds didnt break, allthough the
>commitmessage doesnt mention anything about how it should effect
>bonding. Wouldn't it make sense to propose this patch for 2.6.27 and
>2.6.28 stable kernels?

	Perhaps.

	I don't have a forcedeth to test with, and as you surmised, I
was unable to reproduce the problem with other chipsets (tg3 or e1000).

	However, I did find another bug I introduced during the "mii
refactor" patch that you mentioned as being the original source of the
problem.  That bug will cause 802.3ad to not notice speed changes.

	Could you test the patch below on your 2.6.68.7 and/or 2.6.27.19
and see if it resolves your problem (without the forcedeth patch)?

	-J

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 2c96b93..ad81474 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3545,11 +3545,27 @@ static int bond_slave_netdev_event(unsigned long event, struct net_device *slave
 		}
 		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->params.mode == BOND_MODE_8023AD ||
+		    bond_is_lb(bond)) {
+			struct slave *slave;
+
+			slave = bond_get_slave_by_dev(bond, slave_dev);
+			if (slave) {
+				u16 old_speed = slave->speed;
+				u16 old_duplex = slave->duplex;
+
+				bond_update_speed_duplex(slave);
+
+				if (bond_is_lb(bond))
+					break;
+
+				if (old_speed != slave->speed)
+					bond_3ad_adapter_speed_changed(slave);
+				if (old_duplex != slave->duplex)
+					bond_3ad_adapter_duplex_changed(slave);
+			}
+		}
+
 		break;
 	case NETDEV_DOWN:
 		/*


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

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jesper Krogh]

Jay Vosburgh wrote:
> 	However, I did find another bug I introduced during the "mii
> refactor" patch that you mentioned as being the original source of the
> problem.  That bug will cause 802.3ad to not notice speed changes.
> 
> 	Could you test the patch below on your 2.6.68.7 and/or 2.6.27.19
> and see if it resolves your problem (without the forcedeth patch)?

There was something missing from the header to make it compile.. I found 
that in a later version. Patch below fixed the problem (without the 
forcedeth patch).

diff --git a/drivers/net/bonding/bond_main.c 
b/drivers/net/bonding/bond_main.c
index 1b9c4dc..fd61dfb 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3516,11 +3516,27 @@ static int bond_slave_netdev_event(unsigned long 
event, struct net_device *slave
                 }
                 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->params.mode == BOND_MODE_8023AD ||
+                   bond_is_lb(bond)) {
+                       struct slave *slave;
+
+                       slave = bond_get_slave_by_dev(bond, slave_dev);
+                       if (slave) {
+                               u16 old_speed = slave->speed;
+                               u16 old_duplex = slave->duplex;
+
+                               bond_update_speed_duplex(slave);
+
+                               if (bond_is_lb(bond))
+                                       break;
+
+                               if (old_speed != slave->speed)
+ 
bond_3ad_adapter_speed_changed(slave);
+                               if (old_duplex != slave->duplex)
+ 
bond_3ad_adapter_duplex_changed(slave);
+                       }
+               }
+
                 break;
         case NETDEV_DOWN:
                 /*
diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index fb730ec..b1315e4 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -248,6 +248,14 @@ static inline struct bonding 
*bond_get_bond_by_slave(struct slave *slave)
         return (struct bonding *)slave->dev->master->priv;
  }

+static inline bool bond_is_lb(const struct bonding *bond)
+{
+        return bond->params.mode == BOND_MODE_TLB
+                || bond->params.mode == BOND_MODE_ALB;
+}
+
+
+
  #define BOND_FOM_NONE                  0
  #define BOND_FOM_ACTIVE                        1
  #define BOND_FOM_FOLLOW                        2

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[David Miller]

From: Jesper Krogh <jesper@krogh.cc>
Date: Mon, 09 Mar 2009 21:53:39 +0100

> Jay Vosburgh wrote:
> > 	However, I did find another bug I introduced during the "mii
> > refactor" patch that you mentioned as being the original source of the
> > problem.  That bug will cause 802.3ad to not notice speed changes.
> > 	Could you test the patch below on your 2.6.68.7 and/or 2.6.27.19
> > and see if it resolves your problem (without the forcedeth patch)?
> 
> There was something missing from the header to make it compile.. I found that in a later version. Patch below fixed the problem (without the forcedeth patch).

Jay please resend this with proper signoffs etc. if you want
me to apply it.

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: Jesper Krogh <jesper@krogh.cc>
>Date: Mon, 09 Mar 2009 21:53:39 +0100
>
>> Jay Vosburgh wrote:
>> > 	However, I did find another bug I introduced during the "mii
>> > refactor" patch that you mentioned as being the original source of the
>> > problem.  That bug will cause 802.3ad to not notice speed changes.
>> > 	Could you test the patch below on your 2.6.68.7 and/or 2.6.27.19
>> > and see if it resolves your problem (without the forcedeth patch)?
>> 
>> There was something missing from the header to make it compile.. I found that in a later version. Patch below fixed the problem (without the forcedeth patch).
>
>Jay please resend this with proper signoffs etc. if you want
>me to apply it.

	I posted it again with the usual stuff a day or two after I
posted the test patch; I'll append it to the end of this email.  Note
that the below patch has a minor cosmetic change from the test patch.

	I believe this fix should go to -stable for 2.6.26 and 2.6.27,
but it'll need the change Jesper added to pick up a macro that was added
to mainline:

diff --git a/drivers/net/bonding/bonding.h b/drivers/net/bonding/bonding.h
index fb730ec..b1315e4 100644
--- a/drivers/net/bonding/bonding.h
+++ b/drivers/net/bonding/bonding.h
@@ -248,6 +248,14 @@ static inline struct bonding
*bond_get_bond_by_slave(struct slave *slave)
        return (struct bonding *)slave->dev->master->priv;
 }

+static inline bool bond_is_lb(const struct bonding *bond)
+{
+        return bond->params.mode == BOND_MODE_TLB
+                || bond->params.mode == BOND_MODE_ALB;
+}
+
+
+
 #define BOND_FOM_NONE                  0
 #define BOND_FOM_ACTIVE                        1
 #define BOND_FOM_FOLLOW                        2

	The above fragment isn't needed for mainline, only for -stable.

	-J

From: Jay Vosburgh <fubar@us.ibm.com>
To: netdev@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>, stable@kernel.org
Subject: [PATCH net-next-2.6] bonding: Fix updating of speed/duplex changes
Date: 	Fri, 06 Mar 2009 15:27:33 -0800


	This patch corrects an omission from the following commit:

commit f0c76d61779b153dbfb955db3f144c62d02173c2
Author: Jay Vosburgh <fubar@us.ibm.com>
Date:   Wed Jul 2 18:21:58 2008 -0700

    bonding: refactor mii monitor

	The un-refactored code checked the link speed and duplex of
every slave on every pass; the refactored code did not do so.

	The 802.3ad and balance-alb/tlb modes utilize the speed and
duplex information, and require it to be kept up to date.  This patch
adds a notifier check to perform the appropriate updating when the slave
device speed changes.

Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
---
 drivers/net/bonding/bond_main.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index bfe1ed8..dce3cf9 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -3545,11 +3545,26 @@ static int bond_slave_netdev_event(unsigned long event, struct net_device *slave
 		}
 		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->params.mode == BOND_MODE_8023AD || bond_is_lb(bond)) {
+			struct slave *slave;
+
+			slave = bond_get_slave_by_dev(bond, slave_dev);
+			if (slave) {
+				u16 old_speed = slave->speed;
+				u16 old_duplex = slave->duplex;
+
+				bond_update_speed_duplex(slave);
+
+				if (bond_is_lb(bond))
+					break;
+
+				if (old_speed != slave->speed)
+					bond_3ad_adapter_speed_changed(slave);
+				if (old_duplex != slave->duplex)
+					bond_3ad_adapter_duplex_changed(slave);
+			}
+		}
+
 		break;
 	case NETDEV_DOWN:
 		/*
-- 
1.6.0.2

--
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: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jesper Krogh]

Jay Vosburgh wrote:
> 	The above fragment isn't needed for mainline, only for -stable.
> 

Did you sent it off to the stable kernel maintainers?

-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[David Miller]

From: Jesper Krogh <jesper@krogh.cc>
Date: Mon, 16 Mar 2009 21:34:38 +0100

> Jay Vosburgh wrote:
> > 	The above fragment isn't needed for mainline, only for -stable.
> > 
> 
> Did you sent it off to the stable kernel maintainers?

The fix has to go into Linus's tree first.

I haven't integrated Jay's changes yet.

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[Jesper Krogh]

David Miller wrote:
> From: Jesper Krogh <jesper@krogh.cc>
> Date: Mon, 16 Mar 2009 21:34:38 +0100
> 
>> Jay Vosburgh wrote:
>>> 	The above fragment isn't needed for mainline, only for -stable.
>>>
>> Did you sent it off to the stable kernel maintainers?
> 
> The fix has to go into Linus's tree first.
> 
> I haven't integrated Jay's changes yet.

Excellent. I was just trying to make sure that it wasn't lost somewhere 
in the process.

-- 
Jesper

Re: Regression in bonding between 2.6.26.8 and 2.6.27.6 - bisected - twice

[David Miller]

From: Jay Vosburgh <fubar@us.ibm.com>
Date: Fri, 13 Mar 2009 16:27:16 -0700

> David Miller <davem@davemloft.net> wrote:
> 
> >From: Jesper Krogh <jesper@krogh.cc>
> >Date: Mon, 09 Mar 2009 21:53:39 +0100
> >
> >> Jay Vosburgh wrote:
> >> > 	However, I did find another bug I introduced during the "mii
> >> > refactor" patch that you mentioned as being the original source of the
> >> > problem.  That bug will cause 802.3ad to not notice speed changes.
> >> > 	Could you test the patch below on your 2.6.68.7 and/or 2.6.27.19
> >> > and see if it resolves your problem (without the forcedeth patch)?
> >> 
> >> There was something missing from the header to make it compile.. I found that in a later version. Patch below fixed the problem (without the forcedeth patch).
> >
> >Jay please resend this with proper signoffs etc. if you want
> >me to apply it.
> 
> 	I posted it again with the usual stuff a day or two after I
> posted the test patch; I'll append it to the end of this email.  Note
> that the below patch has a minor cosmetic change from the test patch.
> 
> 	I believe this fix should go to -stable for 2.6.26 and 2.6.27,
> but it'll need the change Jesper added to pick up a macro that was added
> to mainline:

Applied and I'll queue it up for -stable too, thanks!