Date: Sat, 20 Sep 2014 01:22:18 +0000 (UTC) From: Adrian Chadd <adrian@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r271887 - head/sys/dev/ath Message-ID: <201409200122.s8K1MILV062894@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Sat Sep 20 01:22:17 2014 New Revision: 271887 URL: http://svnweb.freebsd.org/changeset/base/271887 Log: Fix up the EDMA RX setup path to correctly initialise and reset the RX FIFO. The original code was .. well, slightly more than incorrect. It showed up as stalled RX queues if the NIC needed to be frequently reinitialised (eg during scans.) This is inspired by work done by Matt Dillon over at the DragonflyBSD project. So: * track when EDMA RX has been stopped and when the MAC has been reset; * re-initialise the ring only after a reset; * track whether RX has been stopped/started - just for debugging now; * don't bother with the RX EOL stuff for EDMA - we don't need the interrupt at all. We also don't need to disable/enable the interrupt or start DMA - once new frames are pushed into the ring via the normal RX path, it'll just restart RX DMA on its own. Tested: * AR9380, STA mode * AR9380, AP mode * AR9485, STA mode * AR9462, STA mode Modified: head/sys/dev/ath/if_ath.c head/sys/dev/ath/if_ath_rx_edma.c head/sys/dev/ath/if_athvar.h Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Sat Sep 20 01:18:36 2014 (r271886) +++ head/sys/dev/ath/if_ath.c Sat Sep 20 01:22:17 2014 (r271887) @@ -1715,12 +1715,22 @@ ath_suspend(struct ath_softc *sc) * NB: don't worry about putting the chip in low power * mode; pci will power off our socket on suspend and * CardBus detaches the device. + * + * XXX TODO: well, that's great, except for non-cardbus + * devices! */ /* - * XXX ensure none of the taskqueues are running + * XXX This doesn't wait until all pending taskqueue + * items and parallel transmit/receive/other threads + * are running! + */ + ath_hal_intrset(sc->sc_ah, 0); + taskqueue_block(sc->sc_tq); + callout_drain(&sc->sc_cal_ch); + + /* * XXX ensure sc_invalid is 1 - * XXX ensure the calibration callout is disabled */ /* Disable the PCIe PHY, complete with workarounds */ @@ -1811,6 +1821,11 @@ ath_resume(struct ath_softc *sc) AH_FALSE, &status); ath_reset_keycache(sc); + ATH_RX_LOCK(sc); + sc->sc_rx_stopped = 1; + sc->sc_rx_resetted = 1; + ATH_RX_UNLOCK(sc); + /* Let DFS at it in case it's a DFS channel */ ath_dfs_radar_enable(sc, ic->ic_curchan); @@ -2015,44 +2030,46 @@ ath_intr(void *arg) if (status & HAL_INT_RXEOL) { int imask; ATH_KTR(sc, ATH_KTR_ERROR, 0, "ath_intr: RXEOL"); - ATH_PCU_LOCK(sc); - /* - * NB: the hardware should re-read the link when - * RXE bit is written, but it doesn't work at - * least on older hardware revs. - */ - sc->sc_stats.ast_rxeol++; - /* - * Disable RXEOL/RXORN - prevent an interrupt - * storm until the PCU logic can be reset. - * In case the interface is reset some other - * way before "sc_kickpcu" is called, don't - * modify sc_imask - that way if it is reset - * by a call to ath_reset() somehow, the - * interrupt mask will be correctly reprogrammed. - */ - imask = sc->sc_imask; - imask &= ~(HAL_INT_RXEOL | HAL_INT_RXORN); - ath_hal_intrset(ah, imask); - /* - * Only blank sc_rxlink if we've not yet kicked - * the PCU. - * - * This isn't entirely correct - the correct solution - * would be to have a PCU lock and engage that for - * the duration of the PCU fiddling; which would include - * running the RX process. Otherwise we could end up - * messing up the RX descriptor chain and making the - * RX desc list much shorter. - */ - if (! sc->sc_kickpcu) - sc->sc_rxlink = NULL; - sc->sc_kickpcu = 1; - ATH_PCU_UNLOCK(sc); + if (! sc->sc_isedma) { + ATH_PCU_LOCK(sc); + /* + * NB: the hardware should re-read the link when + * RXE bit is written, but it doesn't work at + * least on older hardware revs. + */ + sc->sc_stats.ast_rxeol++; + /* + * Disable RXEOL/RXORN - prevent an interrupt + * storm until the PCU logic can be reset. + * In case the interface is reset some other + * way before "sc_kickpcu" is called, don't + * modify sc_imask - that way if it is reset + * by a call to ath_reset() somehow, the + * interrupt mask will be correctly reprogrammed. + */ + imask = sc->sc_imask; + imask &= ~(HAL_INT_RXEOL | HAL_INT_RXORN); + ath_hal_intrset(ah, imask); + /* + * Only blank sc_rxlink if we've not yet kicked + * the PCU. + * + * This isn't entirely correct - the correct solution + * would be to have a PCU lock and engage that for + * the duration of the PCU fiddling; which would include + * running the RX process. Otherwise we could end up + * messing up the RX descriptor chain and making the + * RX desc list much shorter. + */ + if (! sc->sc_kickpcu) + sc->sc_rxlink = NULL; + sc->sc_kickpcu = 1; + ATH_PCU_UNLOCK(sc); + } /* - * Enqueue an RX proc, to handled whatever + * Enqueue an RX proc to handle whatever * is in the RX queue. - * This will then kick the PCU. + * This will then kick the PCU if required. */ sc->sc_rx.recv_sched(sc, 1); } @@ -2348,6 +2365,12 @@ ath_init(void *arg) ATH_UNLOCK(sc); return; } + + ATH_RX_LOCK(sc); + sc->sc_rx_stopped = 1; + sc->sc_rx_resetted = 1; + ATH_RX_UNLOCK(sc); + ath_chan_change(sc, ic->ic_curchan); /* Let DFS at it in case it's a DFS channel */ @@ -2406,8 +2429,7 @@ ath_init(void *arg) * Enable interrupts. */ sc->sc_imask = HAL_INT_RX | HAL_INT_TX - | HAL_INT_RXEOL | HAL_INT_RXORN - | HAL_INT_TXURN + | HAL_INT_RXORN | HAL_INT_TXURN | HAL_INT_FATAL | HAL_INT_GLOBAL; /* @@ -2418,6 +2440,14 @@ ath_init(void *arg) sc->sc_imask |= (HAL_INT_RXHP | HAL_INT_RXLP); /* + * If we're an EDMA NIC, we don't care about RXEOL. + * Writing a new descriptor in will simply restart + * RX DMA. + */ + if (! sc->sc_isedma) + sc->sc_imask |= HAL_INT_RXEOL; + + /* * Enable MIB interrupts when there are hardware phy counters. * Note we only do this (at the moment) for station mode. */ @@ -2735,6 +2765,11 @@ ath_reset(struct ifnet *ifp, ATH_RESET_T __func__, status); sc->sc_diversity = ath_hal_getdiversity(ah); + ATH_RX_LOCK(sc); + sc->sc_rx_stopped = 1; + sc->sc_rx_resetted = 1; + ATH_RX_UNLOCK(sc); + /* Let DFS at it in case it's a DFS channel */ ath_dfs_radar_enable(sc, ic->ic_curchan); @@ -5333,14 +5368,15 @@ ath_chan_set(struct ath_softc *sc, struc ATH_PCU_LOCK(sc); + /* Disable interrupts */ + ath_hal_intrset(ah, 0); + /* Stop new RX/TX/interrupt completion */ if (ath_reset_grablock(sc, 1) == 0) { device_printf(sc->sc_dev, "%s: concurrent reset! Danger!\n", __func__); } - ath_hal_intrset(ah, 0); - /* Stop pending RX/TX completion */ ath_txrx_stop_locked(sc); @@ -5384,6 +5420,11 @@ ath_chan_set(struct ath_softc *sc, struc } sc->sc_diversity = ath_hal_getdiversity(ah); + ATH_RX_LOCK(sc); + sc->sc_rx_stopped = 1; + sc->sc_rx_resetted = 1; + ATH_RX_UNLOCK(sc); + /* Let DFS at it in case it's a DFS channel */ ath_dfs_radar_enable(sc, chan); Modified: head/sys/dev/ath/if_ath_rx_edma.c ============================================================================== --- head/sys/dev/ath/if_ath_rx_edma.c Sat Sep 20 01:18:36 2014 (r271886) +++ head/sys/dev/ath/if_ath_rx_edma.c Sat Sep 20 01:22:17 2014 (r271887) @@ -160,10 +160,20 @@ ath_edma_stoprecv(struct ath_softc *sc, struct ath_hal *ah = sc->sc_ah; ATH_RX_LOCK(sc); + ath_hal_stoppcurecv(ah); ath_hal_setrxfilter(ah, 0); - ath_hal_stopdmarecv(ah); + /* + * + */ + if (ath_hal_stopdmarecv(ah) == AH_TRUE) + sc->sc_rx_stopped = 1; + + /* + * Give the various bus FIFOs (not EDMA descriptor FIFO) + * time to finish flushing out data. + */ DELAY(3000); /* Flush RX pending for each queue */ @@ -218,10 +228,6 @@ ath_edma_reinit_fifo(struct ath_softc *s /* * Start receive. - * - * XXX TODO: this needs to reallocate the FIFO entries when a reset - * occurs, in case the FIFO is filled up and no new descriptors get - * thrown into the FIFO. */ static int ath_edma_startrecv(struct ath_softc *sc) @@ -230,35 +236,31 @@ ath_edma_startrecv(struct ath_softc *sc) ATH_RX_LOCK(sc); + /* + * Sanity check - are we being called whilst RX + * isn't stopped? If so, we may end up pushing + * too many entries into the RX FIFO and + * badness occurs. + */ + /* Enable RX FIFO */ ath_hal_rxena(ah); /* - * Entries should only be written out if the - * FIFO is empty. - * - * XXX This isn't correct. I should be looking - * at the value of AR_RXDP_SIZE (0x0070) to determine - * how many entries are in here. - * - * A warm reset will clear the registers but not the FIFO. - * - * And I believe this is actually the address of the last - * handled buffer rather than the current FIFO pointer. - * So if no frames have been (yet) seen, we'll reinit the - * FIFO. - * - * I'll chase that up at some point. + * In theory the hardware has been initialised, right? */ - if (ath_hal_getrxbuf(sc->sc_ah, HAL_RX_QUEUE_HP) == 0) { + if (sc->sc_rx_resetted == 1) { DPRINTF(sc, ATH_DEBUG_EDMA_RX, "%s: Re-initing HP FIFO\n", __func__); ath_edma_reinit_fifo(sc, HAL_RX_QUEUE_HP); - } - if (ath_hal_getrxbuf(sc->sc_ah, HAL_RX_QUEUE_LP) == 0) { DPRINTF(sc, ATH_DEBUG_EDMA_RX, "%s: Re-initing LP FIFO\n", __func__); ath_edma_reinit_fifo(sc, HAL_RX_QUEUE_LP); + sc->sc_rx_resetted = 0; + } else { + device_printf(sc->sc_dev, + "%s: called without resetting chip?\n", + __func__); } /* Add up to m_fifolen entries in each queue */ @@ -266,6 +268,9 @@ ath_edma_startrecv(struct ath_softc *sc) * These must occur after the above write so the FIFO buffers * are pushed/tracked in the same order as the hardware will * process them. + * + * XXX TODO: is this really necessary? We should've stopped + * the hardware already and reinitialised it, so it's a no-op. */ ath_edma_rxfifo_alloc(sc, HAL_RX_QUEUE_HP, sc->sc_rxedma[HAL_RX_QUEUE_HP].m_fifolen); @@ -276,6 +281,11 @@ ath_edma_startrecv(struct ath_softc *sc) ath_mode_init(sc); ath_hal_startpcurecv(ah); + /* + * We're now doing RX DMA! + */ + sc->sc_rx_stopped = 0; + ATH_RX_UNLOCK(sc); return (0); @@ -380,6 +390,21 @@ ath_edma_recv_proc_queue(struct ath_soft ATH_RX_LOCK(sc); +#if 1 + if (sc->sc_rx_resetted == 1) { + /* + * XXX We shouldn't ever be scheduled if + * receive has been stopped - so complain + * loudly! + */ + device_printf(sc->sc_dev, + "%s: sc_rx_resetted=1! Bad!\n", + __func__); + ATH_RX_UNLOCK(sc); + return; + } +#endif + do { bf = re->m_fifo[re->m_fifo_head]; /* This shouldn't occur! */ @@ -451,24 +476,6 @@ ath_edma_recv_proc_queue(struct ath_soft "ath edma rx proc: npkts=%d\n", npkts); - /* Handle resched and kickpcu appropriately */ - ATH_PCU_LOCK(sc); - if (dosched && sc->sc_kickpcu) { - ATH_KTR(sc, ATH_KTR_ERROR, 0, - "ath_edma_recv_proc_queue(): kickpcu"); - if (npkts > 0) - device_printf(sc->sc_dev, - "%s: handled npkts %d\n", - __func__, npkts); - - /* - * XXX TODO: what should occur here? Just re-poke and - * re-enable the RX FIFO? - */ - sc->sc_kickpcu = 0; - } - ATH_PCU_UNLOCK(sc); - return; } Modified: head/sys/dev/ath/if_athvar.h ============================================================================== --- head/sys/dev/ath/if_athvar.h Sat Sep 20 01:18:36 2014 (r271886) +++ head/sys/dev/ath/if_athvar.h Sat Sep 20 01:22:17 2014 (r271887) @@ -566,6 +566,8 @@ struct ath_softc { int sc_tx_statuslen; int sc_tx_nmaps; /* Number of TX maps */ int sc_edma_bufsize; + int sc_rx_stopped; /* XXX only for EDMA */ + int sc_rx_resetted; /* XXX only for EDMA */ void (*sc_node_cleanup)(struct ieee80211_node *); void (*sc_node_free)(struct ieee80211_node *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201409200122.s8K1MILV062894>