Re: kern/20700

[buhrow%lothlorien.nfbcal.org@localhost (Brian Buhrow)]

The following reply was made to PR kern/20700; it has been noted by GNATS.

From: buhrow%lothlorien.nfbcal.org@localhost (Brian Buhrow)
To: gnats%netbsd.org@localhost
Cc: darcy%druidvex.net@localhost, buhrow%lothlorien.nfbcal.org@localhost
Subject: Re: kern/20700
Date: Wed, 7 Dec 2011 22:43:58 -0800

        Hello.  After working on this problem for a while, I propose
 committing the diff below as a fix for this bug.  The problem of
 negotiating link at a speed other than the 1000-base-T remains, but see
 below for my reasoning on this topic.
 
        the current state of affairs is that Nway autonegotiation works for
 twisted pair copper connections.  Nailed up speed requests work with 10 and
 100mbit connections, but will never work with 1000-base-T connections,
 unless the chip has initialized, and hence negotiated, a connection before
 ifconfig is called in the boot sequence.
 
        What this patch does is turn on Nway autonegotiation when the user
 requests 1000baset with ifconfig and turn off advertisements for speeds
 other than 1000baset in the Nway autonegotiation process.  This makes 
 1000baset nailed links work, which, in my view, is the behavior of least
 astonishment.
 
        Because of the race between Nway negotiation and parallel detection,
 which Thor alluded to earlier in this thread, I've found it very difficult
 to prevent the chip from establishing link with its partners at speeds
 other than the requested speed without totally disabling the ability for
 the link to come up again on its own once the proper speeds and duplexes
 were configured on both ends of the link.  I experimented with disallowing
 the interface from passing traffic if the link speed didn't match the
 requested speed, but that seemed even less intuitive, since ifconfig would
 show the interface as being active, but wouldn't actually work.  Finally,
 I figured out that I could probably implement the requested behavior to fix
 this bug completely, but to do so would require that I touch every phy
 driver in the system, creating a huge patch and a larger testing problem.
 
        In summary, in my view, this patch improves the state of the world,
 permitting more configuration cases to work than do today, as well as
 causing the system to behave in less astonishing ways than it does today.
 Right now, if you get a link at 1000baset when the system boots, and you
 don't have autonegotiation configured, if you pull the plug or otherwise
 bounce the port, you have to reboot or turn autonegotiation on to
 re-establish link.  this patch insures that you'll get a restoration of
 service, regardless of how you set ifconfig, without user intervention.
 
        If folks don't strongly object, I'll commit this patch in the next
 week or so and request a pullup for the 5.x branch.  If someone has an idea
 for fully implementing the requested behavior without having to touch all
 the phy drivers, I'm very interested and will be happy to implement and
 test.
 
 -thanks
 -Brian
 
 P.S.  This patch applies cleanly to 4.x, 5.x and -current source trees.
 
 
 --- mii_physubr.c.40   2007-11-15 20:14:47.000000000 -0800
 +++ mii_physubr.c      2011-12-07 10:01:39.000000000 -0800
 @@ -170,15 +170,21 @@
                bmcr |= BMCR_LOOP;
  
        PHY_WRITE(sc, MII_ANAR, anar);
 -      PHY_WRITE(sc, MII_BMCR, bmcr);
        if (sc->mii_flags & MIIF_HAVE_GTCR)
                PHY_WRITE(sc, MII_100T2CR, gtcr);
 +      if (IFM_SUBTYPE(ife->ifm_media) == IFM_1000_T) {
 +              mii_phy_auto(sc, 0);
 +      } else {
 +              PHY_WRITE(sc, MII_BMCR, bmcr);
 +      }
  }
  
  int
  mii_phy_auto(struct mii_softc *sc, int waitfor)
  {
        int i;
 +      struct mii_data *mii = sc->mii_pdata;
 +      struct ifmedia_entry *ife = mii->mii_media.ifm_cur;
  
        if ((sc->mii_flags & MIIF_DOINGAUTO) == 0) {
                /*
 @@ -212,6 +218,16 @@
                                     (EXTSR_1000THDX|EXTSR_1000TFDX)))
                                        anar |= ANAR_X_PAUSE_ASYM;
                        }
 +
 +                      /*
 +                       *for 1000-base-T, autonegotiation mus be enabled, but 
 +                       *if we're not set to auto, only advertise
 +                       *1000-base-T with the link partner.
 +                       */
 +                      if (IFM_SUBTYPE(ife->ifm_media) == IFM_1000_T) {
 +                              anar &= 
~(ANAR_T4|ANAR_TX_FD|ANAR_TX|ANAR_10_FD|ANAR_10);
 +                      }
 +                              
                        PHY_WRITE(sc, MII_ANAR, anar);
                        if (sc->mii_flags & MIIF_HAVE_GTCR) {
                                uint16_t gtcr = 0;
 @@ -294,7 +310,8 @@
         * status so we can generate an announcement if the status
         * changes.
         */
 -      if (IFM_SUBTYPE(ife->ifm_media) != IFM_AUTO)
 +      if ((IFM_SUBTYPE(ife->ifm_media) != IFM_AUTO) &&
 +      (IFM_SUBTYPE(ife->ifm_media) != IFM_1000_T))
                return (0);
  
        /* Read the status register twice; BMSR_LINK is latch-low. */