From owner-freebsd-net@FreeBSD.ORG Sat Jan 13 10:33:46 2007 Return-Path: X-Original-To: net@freebsd.org Delivered-To: freebsd-net@FreeBSD.ORG Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id DC61216A412; Sat, 13 Jan 2007 10:33:46 +0000 (UTC) (envelope-from oleg@lath.rinet.ru) Received: from lath.rinet.ru (lath.rinet.ru [195.54.192.90]) by mx1.freebsd.org (Postfix) with ESMTP id 3BEEC13C458; Sat, 13 Jan 2007 10:33:45 +0000 (UTC) (envelope-from oleg@lath.rinet.ru) Received: from lath.rinet.ru (localhost [127.0.0.1]) by lath.rinet.ru (8.13.8/8.13.8) with ESMTP id l0DAXiWG015713 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 13 Jan 2007 13:33:44 +0300 (MSK) (envelope-from oleg@lath.rinet.ru) Received: (from oleg@localhost) by lath.rinet.ru (8.13.8/8.13.8/Submit) id l0DAXiZf015712; Sat, 13 Jan 2007 13:33:44 +0300 (MSK) (envelope-from oleg) Date: Sat, 13 Jan 2007 13:33:44 +0300 From: Oleg Bulyzhin To: John Polstra Message-ID: <20070113103344.GB15045@lath.rinet.ru> References: <20070109235649.GC5246@lath.rinet.ru> <20070113101455.GA15045@lath.rinet.ru> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="nVMJ2NtxeReIH9PS" Content-Disposition: inline In-Reply-To: <20070113101455.GA15045@lath.rinet.ru> User-Agent: Mutt/1.5.13 (2006-08-11) Cc: Doug Barton , net@freebsd.org Subject: Re: [Fwd: Re: bge Ierr rate increase from 5.3R -> 6.1R] X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Jan 2007 10:33:47 -0000 --nVMJ2NtxeReIH9PS Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Sat, Jan 13, 2007 at 01:14:55PM +0300, Oleg Bulyzhin wrote: > On Tue, Jan 09, 2007 at 04:14:16PM -0800, John Polstra wrote: > > > I've been using #2 today, and it's working well so far. I > > implemented it like this. (Ignore the version numbers; I'm working > > in a private repository.) > > > > --- if_bge.c 8 Jan 2007 22:46:51 -0000 1.26 > > +++ if_bge.c 9 Jan 2007 22:52:43 -0000 1.27 > > @@ -3122,8 +3122,8 @@ bge_tick(void *xsc) > > > > if ((sc->bge_flags & BGE_FLAG_TBI) == 0) { > > mii = device_get_softc(sc->bge_miibus); > > - /* Don't mess with the PHY in IPMI/ASF mode */ > > - if (!((sc->bge_asf_mode & ASF_STACKUP) && (sc->bge_link))) > > + /* Don't mess with the PHY unless link is down. */ > > + if (!sc->bge_link) > > mii_tick(mii); > > } else { > > /* > > > > > > John > > Could you please test attached patch? It should fix ierrs issue and should not > break link events (would be fine to test it: unplug/plug cable, try to change > media with ifconfig, change media on other end of wire). > Oops, previous patch was incomplete (forgot to add brgphy.c diff). Try this one. -- Oleg. ================================================================ === Oleg Bulyzhin -- OBUL-RIPN -- OBUL-RIPE -- oleg@rinet.ru === ================================================================ --nVMJ2NtxeReIH9PS Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="bge_link.diff" Index: sys/dev/bge/if_bgereg.h =================================================================== RCS file: /home/ncvs/src/sys/dev/bge/if_bgereg.h,v retrieving revision 1.66 diff -u -r1.66 if_bgereg.h --- sys/dev/bge/if_bgereg.h 11 Jan 2007 01:43:24 -0000 1.66 +++ sys/dev/bge/if_bgereg.h 13 Jan 2007 10:31:47 -0000 @@ -2479,8 +2479,13 @@ uint32_t bge_tx_buf_ratio; int bge_if_flags; int bge_txcnt; - int bge_link; /* link state */ - int bge_link_evt; /* pending link event */ + uint32_t bge_sts; +#define BGE_STS_LINK 0x00000001 /* MAC link status */ +#define BGE_STS_LINK_EVT 0x00000002 /* pending link event */ +#define BGE_STS_AUTOPOLL 0x00000004 /* PHY auto-polling */ +#define BGE_STS_BIT(sc, x) ((sc)->bge_sts & (x)) +#define BGE_STS_SETBIT(sc, x) ((sc)->bge_sts |= (x)) +#define BGE_STS_CLRBIT(sc, x) ((sc)->bge_sts &= ~(x)) int bge_timer; struct callout bge_stat_ch; uint32_t bge_rx_discards; Index: sys/dev/bge/if_bge.c =================================================================== RCS file: /home/ncvs/src/sys/dev/bge/if_bge.c,v retrieving revision 1.172 diff -u -r1.172 if_bge.c --- sys/dev/bge/if_bge.c 26 Dec 2006 18:33:55 -0000 1.172 +++ sys/dev/bge/if_bge.c 13 Jan 2007 10:31:52 -0000 @@ -590,6 +590,7 @@ /* Reading with autopolling on may trigger PCI errors */ autopoll = CSR_READ_4(sc, BGE_MI_MODE); if (autopoll & BGE_MIMODE_AUTOPOLL) { + BGE_STS_CLRBIT(sc, BGE_STS_AUTOPOLL); BGE_CLRBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL); DELAY(40); } @@ -613,6 +614,7 @@ done: if (autopoll & BGE_MIMODE_AUTOPOLL) { + BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL); BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL); DELAY(40); } @@ -635,6 +637,7 @@ /* Reading with autopolling on may trigger PCI errors */ autopoll = CSR_READ_4(sc, BGE_MI_MODE); if (autopoll & BGE_MIMODE_AUTOPOLL) { + BGE_STS_CLRBIT(sc, BGE_STS_AUTOPOLL); BGE_CLRBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL); DELAY(40); } @@ -648,6 +651,7 @@ } if (autopoll & BGE_MIMODE_AUTOPOLL) { + BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL); BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL); DELAY(40); } @@ -1588,6 +1592,7 @@ if (sc->bge_flags & BGE_FLAG_TBI) { CSR_WRITE_4(sc, BGE_MI_STS, BGE_MISTS_LINK); } else { + BGE_STS_SETBIT(sc, BGE_STS_AUTOPOLL); BGE_SETBIT(sc, BGE_MI_MODE, BGE_MIMODE_AUTOPOLL|10<<16); if (sc->bge_asicrev == BGE_ASICREV_BCM5700 && sc->bge_chipid != BGE_CHIPID_BCM5700_B2) @@ -2992,12 +2997,13 @@ /* Note link event. It will be processed by POLL_AND_CHECK_STATUS cmd */ if (statusword & BGE_STATFLAG_LINKSTATE_CHANGED) - sc->bge_link_evt++; + BGE_STS_SETBIT(sc, BGE_STS_LINK_EVT); if (cmd == POLL_AND_CHECK_STATUS) if ((sc->bge_asicrev == BGE_ASICREV_BCM5700 && sc->bge_chipid != BGE_CHIPID_BCM5700_B2) || - sc->bge_link_evt || (sc->bge_flags & BGE_FLAG_TBI)) + BGE_STS_BIT(sc, BGE_STS_LINK_EVT) || + (sc->bge_flags & BGE_FLAG_TBI)) bge_link_upd(sc); sc->rxcycles = count; @@ -3065,7 +3071,7 @@ if ((sc->bge_asicrev == BGE_ASICREV_BCM5700 && sc->bge_chipid != BGE_CHIPID_BCM5700_B2) || - statusword || sc->bge_link_evt) + statusword || BGE_STS_BIT(sc, BGE_STS_LINK_EVT)) bge_link_upd(sc); if (ifp->if_drv_flags & IFF_DRV_RUNNING) { @@ -3121,10 +3127,15 @@ bge_stats_update(sc); if ((sc->bge_flags & BGE_FLAG_TBI) == 0) { - mii = device_get_softc(sc->bge_miibus); - /* Don't mess with the PHY in IPMI/ASF mode */ - if (!((sc->bge_asf_mode & ASF_STACKUP) && (sc->bge_link))) + /* + * Do not touch PHY if we have link up. This could break + * IPMI/ASF mode or produce extra input errors. + * (extra input errors was reported for bcm5701 & bcm5704). + */ + if (!BGE_STS_BIT(sc, BGE_STS_LINK)) { + mii = device_get_softc(sc->bge_miibus); mii_tick(mii); + } } else { /* * Since in TBI mode auto-polling can't be used we should poll @@ -3136,7 +3147,7 @@ if (!(sc->bge_ifp->if_capenable & IFCAP_POLLING)) #endif { - sc->bge_link_evt++; + BGE_STS_SETBIT(sc, BGE_STS_LINK_EVT); BGE_SETBIT(sc, BGE_MISC_LOCAL_CTL, BGE_MLC_INTR_SET); } } @@ -3352,7 +3363,7 @@ sc = ifp->if_softc; - if (!sc->bge_link || IFQ_DRV_IS_EMPTY(&ifp->if_snd)) + if (!BGE_STS_BIT(sc, BGE_STS_LINK) || IFQ_DRV_IS_EMPTY(&ifp->if_snd)) return; prodidx = sc->bge_tx_prodidx; @@ -3633,7 +3644,7 @@ return (0); } - sc->bge_link_evt++; + BGE_STS_SETBIT(sc, BGE_STS_LINK_EVT); mii = device_get_softc(sc->bge_miibus); if (mii->mii_instance) { struct mii_softc *miisc; @@ -3935,9 +3946,9 @@ sc->bge_tx_saved_considx = BGE_TXCONS_UNSET; /* Clear MAC's link state (PHY may still have link UP). */ - if (bootverbose && sc->bge_link) + if (bootverbose && BGE_STS_BIT(sc, BGE_STS_LINK)) if_printf(sc->bge_ifp, "link DOWN\n"); - sc->bge_link = 0; + BGE_STS_CLRBIT(sc, BGE_STS_LINK); ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); } @@ -3995,12 +4006,12 @@ bge_link_upd(struct bge_softc *sc) { struct mii_data *mii; - uint32_t link, status; + uint32_t status; BGE_LOCK_ASSERT(sc); /* Clear 'pending link event' flag. */ - sc->bge_link_evt = 0; + BGE_STS_CLRBIT(sc, BGE_STS_LINK_EVT); /* * Process link state changes. @@ -4023,16 +4034,16 @@ if (status & BGE_MACSTAT_MI_INTERRUPT) { mii = device_get_softc(sc->bge_miibus); mii_pollstat(mii); - if (!sc->bge_link && + if (!BGE_STS_BIT(sc, BGE_STS_LINK) && mii->mii_media_status & IFM_ACTIVE && IFM_SUBTYPE(mii->mii_media_active) != IFM_NONE) { - sc->bge_link++; + BGE_STS_SETBIT(sc, BGE_STS_LINK); if (bootverbose) if_printf(sc->bge_ifp, "link UP\n"); - } else if (sc->bge_link && + } else if (BGE_STS_BIT(sc, BGE_STS_LINK) && (!(mii->mii_media_status & IFM_ACTIVE) || IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE)) { - sc->bge_link = 0; + BGE_STS_CLRBIT(sc, BGE_STS_LINK); if (bootverbose) if_printf(sc->bge_ifp, "link DOWN\n"); } @@ -4050,8 +4061,8 @@ if (sc->bge_flags & BGE_FLAG_TBI) { status = CSR_READ_4(sc, BGE_MAC_STS); if (status & BGE_MACSTAT_TBI_PCS_SYNCHED) { - if (!sc->bge_link) { - sc->bge_link++; + if (!BGE_STS_BIT(sc, BGE_STS_LINK)) { + BGE_STS_SETBIT(sc, BGE_STS_LINK); if (sc->bge_asicrev == BGE_ASICREV_BCM5704) BGE_CLRBIT(sc, BGE_MAC_MODE, BGE_MACMODE_TBI_SEND_CFGS); @@ -4061,40 +4072,38 @@ if_link_state_change(sc->bge_ifp, LINK_STATE_UP); } - } else if (sc->bge_link) { - sc->bge_link = 0; + } else if (BGE_STS_BIT(sc, BGE_STS_LINK)) { + BGE_STS_CLRBIT(sc, BGE_STS_LINK); if (bootverbose) if_printf(sc->bge_ifp, "link DOWN\n"); if_link_state_change(sc->bge_ifp, LINK_STATE_DOWN); } - /* Discard link events for MII/GMII cards if MI auto-polling disabled */ - } else if (CSR_READ_4(sc, BGE_MI_MODE) & BGE_MIMODE_AUTOPOLL) { - /* - * Some broken BCM chips have BGE_STATFLAG_LINKSTATE_CHANGED bit - * in status word always set. Workaround this bug by reading - * PHY link status directly. - */ - link = (CSR_READ_4(sc, BGE_MI_STS) & BGE_MISTS_LINK) ? 1 : 0; + /* + * Discard link events for MII/GMII cards if MI auto-polling disabled. + * This should not happen since mii callouts are locked now, but + * we keep this check for debug. + */ + } else if (BGE_STS_BIT(sc, BGE_STS_AUTOPOLL)) { + mii = device_get_softc(sc->bge_miibus); + mii_pollstat(mii); - if (link != sc->bge_link || - sc->bge_asicrev == BGE_ASICREV_BCM5700) { - mii = device_get_softc(sc->bge_miibus); - mii_pollstat(mii); - if (!sc->bge_link && - mii->mii_media_status & IFM_ACTIVE && - IFM_SUBTYPE(mii->mii_media_active) != IFM_NONE) { - sc->bge_link++; - if (bootverbose) - if_printf(sc->bge_ifp, "link UP\n"); - } else if (sc->bge_link && - (!(mii->mii_media_status & IFM_ACTIVE) || - IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE)) { - sc->bge_link = 0; - if (bootverbose) - if_printf(sc->bge_ifp, "link DOWN\n"); - } + if (!BGE_STS_BIT(sc, BGE_STS_LINK) && + mii->mii_media_status & IFM_ACTIVE && + IFM_SUBTYPE(mii->mii_media_active) != IFM_NONE) { + BGE_STS_SETBIT(sc, BGE_STS_LINK); + if (bootverbose) + if_printf(sc->bge_ifp, "link UP\n"); + } else if (BGE_STS_BIT(sc, BGE_STS_LINK) && + (!(mii->mii_media_status & IFM_ACTIVE) || + IFM_SUBTYPE(mii->mii_media_active) == IFM_NONE)) { + BGE_STS_CLRBIT(sc, BGE_STS_LINK); + if (bootverbose) + if_printf(sc->bge_ifp, "link DOWN\n"); } - } + } else + /* Should not happen, see above. */ + if_printf(sc->bge_ifp, + "link event discarded: PHY auto-polling is off.\n"); /* Clear the attention. */ CSR_WRITE_4(sc, BGE_MAC_STS, BGE_MACSTAT_SYNC_CHANGED| Index: sys/dev/mii/brgphy.c =================================================================== RCS file: /home/ncvs/src/sys/dev/mii/brgphy.c,v retrieving revision 1.52 diff -u -r1.52 brgphy.c --- sys/dev/mii/brgphy.c 20 Dec 2006 00:34:12 -0000 1.52 +++ sys/dev/mii/brgphy.c 13 Jan 2007 10:32:03 -0000 @@ -398,6 +398,7 @@ } if (bmsr & BRGPHY_BMSR_LINK) { + sc->mii_ticks = 0; /* Reset autoneg timer. */ switch (PHY_READ(sc, BRGPHY_MII_AUXSTS) & BRGPHY_AUXSTS_AN_RES) { case BRGPHY_RES_1000FD: --nVMJ2NtxeReIH9PS--