From owner-svn-src-head@freebsd.org Sat Jun 10 23:26:26 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 6C609BF8398; Sat, 10 Jun 2017 23:26:26 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3DADC66661; Sat, 10 Jun 2017 23:26:26 +0000 (UTC) (envelope-from ian@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v5ANQPBS035343; Sat, 10 Jun 2017 23:26:25 GMT (envelope-from ian@FreeBSD.org) Received: (from ian@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v5ANQPhV035342; Sat, 10 Jun 2017 23:26:25 GMT (envelope-from ian@FreeBSD.org) Message-Id: <201706102326.v5ANQPhV035342@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: ian set sender to ian@FreeBSD.org using -f From: Ian Lepore Date: Sat, 10 Jun 2017 23:26:25 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r319811 - head/sys/dev/ffec X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 10 Jun 2017 23:26:26 -0000 Author: ian Date: Sat Jun 10 23:26:25 2017 New Revision: 319811 URL: https://svnweb.freebsd.org/changeset/base/319811 Log: if_ffec bugfixes related to harvesting of hardware-maintained statistics... After harvesting the hardware statistics counters and summing them into the interface stats, properly clear the hardware counters back to zero. On imx5 and earlier hardware it is necessary to disable collection of stats while writing zeroes to all the registers. On imx6 and newer it turns out it's not even possible to write zeroes, instead you have to toggle a special "zero everything" control bit in a register. Count incoming packets with a bad start frame delim as input errors, and incoming packets dropped due to no fifo space as input drops. Remove all code related to harvesting the hardware stats less often than once per second. It turns out the 32-bit stats registers are backed by 16-bit counters under the hood, and they can easily roll over if you only harvest them once every 3 seconds like the old code was doing. Now we just read all the regs once a second. The combination of not properly zeroing the stats registers and 16-bit counters sometimes wrapping between harvest calls resulted in basically unusable statistics before these changes. Modified: head/sys/dev/ffec/if_ffec.c Modified: head/sys/dev/ffec/if_ffec.c ============================================================================== --- head/sys/dev/ffec/if_ffec.c Sat Jun 10 23:16:00 2017 (r319810) +++ head/sys/dev/ffec/if_ffec.c Sat Jun 10 23:26:25 2017 (r319811) @@ -129,7 +129,6 @@ static struct ofw_compat_data compat_data[] = { #define TX_DESC_SIZE (sizeof(struct ffec_hwdesc) * TX_DESC_COUNT) #define WATCHDOG_TIMEOUT_SECS 5 -#define STATS_HARVEST_INTERVAL 3 struct ffec_bufmap { struct mbuf *mbuf; @@ -160,7 +159,6 @@ struct ffec_softc { boolean_t is_attached; boolean_t is_detaching; int tx_watchdog_count; - int stats_harvest_count; bus_dma_tag_t rxdesc_tag; bus_dmamap_t rxdesc_map; @@ -462,22 +460,41 @@ ffec_media_change(struct ifnet * ifp) static void ffec_clear_stats(struct ffec_softc *sc) { + uint32_t mibc; - WR4(sc, FEC_RMON_R_PACKETS, 0); - WR4(sc, FEC_RMON_R_MC_PKT, 0); - WR4(sc, FEC_RMON_R_CRC_ALIGN, 0); - WR4(sc, FEC_RMON_R_UNDERSIZE, 0); - WR4(sc, FEC_RMON_R_OVERSIZE, 0); - WR4(sc, FEC_RMON_R_FRAG, 0); - WR4(sc, FEC_RMON_R_JAB, 0); - WR4(sc, FEC_RMON_T_PACKETS, 0); - WR4(sc, FEC_RMON_T_MC_PKT, 0); - WR4(sc, FEC_RMON_T_CRC_ALIGN, 0); - WR4(sc, FEC_RMON_T_UNDERSIZE, 0); - WR4(sc, FEC_RMON_T_OVERSIZE , 0); - WR4(sc, FEC_RMON_T_FRAG, 0); - WR4(sc, FEC_RMON_T_JAB, 0); - WR4(sc, FEC_RMON_T_COL, 0); + mibc = RD4(sc, FEC_MIBC_REG); + + /* + * On newer hardware the statistic regs are cleared by toggling a bit in + * the mib control register. On older hardware the clear procedure is + * to disable statistics collection, zero the regs, then re-enable. + */ + if (sc->fectype == FECTYPE_IMX6 || sc->fectype == FECTYPE_MVF) { + WR4(sc, FEC_MIBC_REG, mibc | FEC_MIBC_CLEAR); + WR4(sc, FEC_MIBC_REG, mibc & ~FEC_MIBC_CLEAR); + } else { + WR4(sc, FEC_MIBC_REG, mibc | FEC_MIBC_DIS); + + WR4(sc, FEC_IEEE_R_DROP, 0); + WR4(sc, FEC_IEEE_R_MACERR, 0); + WR4(sc, FEC_RMON_R_CRC_ALIGN, 0); + WR4(sc, FEC_RMON_R_FRAG, 0); + WR4(sc, FEC_RMON_R_JAB, 0); + WR4(sc, FEC_RMON_R_MC_PKT, 0); + WR4(sc, FEC_RMON_R_OVERSIZE, 0); + WR4(sc, FEC_RMON_R_PACKETS, 0); + WR4(sc, FEC_RMON_R_UNDERSIZE, 0); + WR4(sc, FEC_RMON_T_COL, 0); + WR4(sc, FEC_RMON_T_CRC_ALIGN, 0); + WR4(sc, FEC_RMON_T_FRAG, 0); + WR4(sc, FEC_RMON_T_JAB, 0); + WR4(sc, FEC_RMON_T_MC_PKT, 0); + WR4(sc, FEC_RMON_T_OVERSIZE , 0); + WR4(sc, FEC_RMON_T_PACKETS, 0); + WR4(sc, FEC_RMON_T_UNDERSIZE, 0); + + WR4(sc, FEC_MIBC_REG, mibc); + } } static void @@ -485,29 +502,22 @@ ffec_harvest_stats(struct ffec_softc *sc) { struct ifnet *ifp; - /* We don't need to harvest too often. */ - if (++sc->stats_harvest_count < STATS_HARVEST_INTERVAL) - return; + ifp = sc->ifp; /* - * Try to avoid harvesting unless the IDLE flag is on, but if it has - * been too long just go ahead and do it anyway, the worst that'll - * happen is we'll lose a packet count or two as we clear at the end. + * - FEC_IEEE_R_DROP is "dropped due to invalid start frame delimiter" + * so it's really just another type of input error. + * - FEC_IEEE_R_MACERR is "no receive fifo space"; count as input drops. */ - if (sc->stats_harvest_count < (2 * STATS_HARVEST_INTERVAL) && - ((RD4(sc, FEC_MIBC_REG) & FEC_MIBC_IDLE) == 0)) - return; - - sc->stats_harvest_count = 0; - ifp = sc->ifp; - if_inc_counter(ifp, IFCOUNTER_IPACKETS, RD4(sc, FEC_RMON_R_PACKETS)); if_inc_counter(ifp, IFCOUNTER_IMCASTS, RD4(sc, FEC_RMON_R_MC_PKT)); if_inc_counter(ifp, IFCOUNTER_IERRORS, RD4(sc, FEC_RMON_R_CRC_ALIGN) + RD4(sc, FEC_RMON_R_UNDERSIZE) + RD4(sc, FEC_RMON_R_OVERSIZE) + RD4(sc, FEC_RMON_R_FRAG) + - RD4(sc, FEC_RMON_R_JAB)); + RD4(sc, FEC_RMON_R_JAB) + RD4(sc, FEC_IEEE_R_DROP)); + if_inc_counter(ifp, IFCOUNTER_IQDROPS, RD4(sc, FEC_IEEE_R_MACERR)); + if_inc_counter(ifp, IFCOUNTER_OPACKETS, RD4(sc, FEC_RMON_T_PACKETS)); if_inc_counter(ifp, IFCOUNTER_OMCASTS, RD4(sc, FEC_RMON_T_MC_PKT)); if_inc_counter(ifp, IFCOUNTER_OERRORS, @@ -1016,7 +1026,6 @@ ffec_stop_locked(struct ffec_softc *sc) ifp = sc->ifp; ifp->if_drv_flags &= ~(IFF_DRV_RUNNING | IFF_DRV_OACTIVE); sc->tx_watchdog_count = 0; - sc->stats_harvest_count = 0; /* * Stop the hardware, mask all interrupts, and clear all current @@ -1194,7 +1203,8 @@ ffec_init_locked(struct ffec_softc *sc) WR4(sc, FEC_IEM_REG, FEC_IER_TXF | FEC_IER_RXF | FEC_IER_EBERR); /* - * MIBC - MIB control (hardware stats). + * MIBC - MIB control (hardware stats); clear all statistics regs, then + * enable collection of statistics. */ regval = RD4(sc, FEC_MIBC_REG); WR4(sc, FEC_MIBC_REG, regval | FEC_MIBC_DIS);