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

[Jesper Krogh <jesper@krogh.cc>]

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 */