Author: adrian
Date: Tue May 6 01:15:42 2014
New Revision: 265409
URL: http://svnweb.freebsd.org/changeset/base/265409
Log:
Modify the RX path to keep the previous RX descriptor around once it's
used.
It turns out that the RX DMA engine does the same last-descriptor-link-
pointer-re-reading trick that the TX DMA engine. That is, the hardware
re-reads the link pointer before it moves onto the next descriptor.
Thus we can't free a descriptor before we move on; it's possible the
hardware will need to re-read the link pointer before we overwrite
it with a new one.
Tested:
* AR5416, STA mode
TODO:
* more thorough AP and STA mode testing!
* test on other pre-AR9380 NICs, just to be sure.
* Break out the RX descriptor grabbing bits from the RX completion
bits, like what is done in the RX EDMA code, so ..
* .. the RX lock can be held during ath_rx_proc(), but not across
packet input.
Modified:
head/sys/dev/ath/if_ath_rx.c
head/sys/dev/ath/if_ath_sysctl.c
head/sys/dev/ath/if_athvar.h
Modified: head/sys/dev/ath/if_ath_rx.c
==============================================================================
--- head/sys/dev/ath/if_ath_rx.c Tue May 6 00:00:07 2014
(r265408)
+++ head/sys/dev/ath/if_ath_rx.c Tue May 6 01:15:42 2014
(r265409)
@@ -245,6 +245,8 @@ ath_legacy_rxbuf_init(struct ath_softc *
struct mbuf *m;
struct ath_desc *ds;
+ /* XXX TODO: ATH_RX_LOCK_ASSERT(sc); */
+
m = bf->bf_m;
if (m == NULL) {
/*
@@ -974,6 +976,14 @@ rx_next:
#define ATH_RX_MAX 128
+/*
+ * XXX TODO: break out the "get buffers" from "call ath_rx_pkt()" like
+ * the EDMA code does.
+ *
+ * XXX TODO: then, do all of the RX list management stuff inside
+ * ATH_RX_LOCK() so we don't end up potentially racing. The EDMA
+ * code is doing it right.
+ */
static void
ath_rx_proc(struct ath_softc *sc, int resched)
{
@@ -995,6 +1005,7 @@ ath_rx_proc(struct ath_softc *sc, int re
u_int64_t tsf;
int npkts = 0;
int kickpcu = 0;
+ int ret;
/* XXX we must not hold the ATH_LOCK here */
ATH_UNLOCK_ASSERT(sc);
@@ -1094,8 +1105,26 @@ ath_rx_proc(struct ath_softc *sc, int re
if (ath_rx_pkt(sc, rs, status, tsf, nf, HAL_RX_QUEUE_HP, bf, m))
ngood++;
rx_proc_next:
- TAILQ_INSERT_TAIL(&sc->sc_rxbuf, bf, bf_list);
- } while (ath_rxbuf_init(sc, bf) == 0);
+ /*
+ * If there's a holding buffer, insert that onto
+ * the RX list; the hardware is now definitely not pointing
+ * to it now.
+ */
+ ret = 0;
+ if (sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf != NULL) {
+ TAILQ_INSERT_TAIL(&sc->sc_rxbuf,
+ sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf,
+ bf_list);
+ ret = ath_rxbuf_init(sc,
+ sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf);
+ }
+ /*
+ * Next, throw our buffer into the holding entry. The hardware
+ * may use the descriptor to read the link pointer before
+ * DMAing the next descriptor in to write out a packet.
+ */
+ sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf = bf;
+ } while (ret == 0);
/* rx signal state monitoring */
ath_hal_rxmonitor(ah, &sc->sc_halstats, sc->sc_curchan);
@@ -1127,6 +1156,13 @@ rx_proc_next:
* constantly write over the same frame, leading
* the RX driver code here to get heavily confused.
*/
+ /*
+ * XXX Has RX DMA stopped enough here to just call
+ * ath_startrecv()?
+ * XXX Do we need to use the holding buffer to restart
+ * RX DMA by appending entries to the final
+ * descriptor? Quite likely.
+ */
#if 1
ath_startrecv(sc);
#else
@@ -1217,6 +1253,58 @@ ath_legacy_flushrecv(struct ath_softc *s
ath_rx_proc(sc, 0);
}
+static void
+ath_legacy_flush_rxpending(struct ath_softc *sc)
+{
+
+ /* XXX ATH_RX_LOCK_ASSERT(sc); */
+
+ if (sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending != NULL) {
+ m_freem(sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending);
+ sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending = NULL;
+ }
+ if (sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending != NULL) {
+ m_freem(sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending);
+ sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending = NULL;
+ }
+}
+
+static int
+ath_legacy_flush_rxholdbf(struct ath_softc *sc)
+{
+ struct ath_buf *bf;
+
+ /* XXX ATH_RX_LOCK_ASSERT(sc); */
+ /*
+ * If there are RX holding buffers, free them here and return
+ * them to the list.
+ *
+ * XXX should just verify that bf->bf_m is NULL, as it must
+ * be at this point!
+ */
+ bf = sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf;
+ if (bf != NULL) {
+ if (bf->bf_m != NULL)
+ m_freem(bf->bf_m);
+ bf->bf_m = NULL;
+ TAILQ_INSERT_TAIL(&sc->sc_rxbuf, bf, bf_list);
+ (void) ath_rxbuf_init(sc, bf);
+ }
+ sc->sc_rxedma[HAL_RX_QUEUE_HP].m_holdbf = NULL;
+
+ bf = sc->sc_rxedma[HAL_RX_QUEUE_LP].m_holdbf;
+ if (bf != NULL) {
+ if (bf->bf_m != NULL)
+ m_freem(bf->bf_m);
+ bf->bf_m = NULL;
+ TAILQ_INSERT_TAIL(&sc->sc_rxbuf, bf, bf_list);
+ (void) ath_rxbuf_init(sc, bf);
+ }
+ sc->sc_rxedma[HAL_RX_QUEUE_LP].m_holdbf = NULL;
+
+ return (0);
+}
+
/*
* Disable the receive h/w in preparation for a reset.
*/
@@ -1228,6 +1316,8 @@ ath_legacy_stoprecv(struct ath_softc *sc
((_pa) - (_sc)->sc_rxdma.dd_desc_paddr)))
struct ath_hal *ah = sc->sc_ah;
+ ATH_RX_LOCK(sc);
+
ath_hal_stoppcurecv(ah); /* disable PCU */
ath_hal_setrxfilter(ah, 0); /* clear recv filter */
ath_hal_stopdmarecv(ah); /* disable DMA engine */
@@ -1261,22 +1351,23 @@ ath_legacy_stoprecv(struct ath_softc *sc
}
}
#endif
- /*
- * Free both high/low RX pending, just in case.
- */
- if (sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending != NULL) {
- m_freem(sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending);
- sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending = NULL;
- }
- if (sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending != NULL) {
- m_freem(sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending);
- sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending = NULL;
- }
+
+ (void) ath_legacy_flush_rxpending(sc);
+ (void) ath_legacy_flush_rxholdbf(sc);
+
sc->sc_rxlink = NULL; /* just in case */
+
+ ATH_RX_UNLOCK(sc);
#undef PA2DESC
}
/*
+ * XXX TODO: something was calling startrecv without calling
+ * stoprecv. Let's figure out what/why. It was showing up
+ * as a mbuf leak (rxpending) and ath_buf leak (holdbf.)
+ */
+
+/*
* Enable the receive h/w following a reset.
*/
static int
@@ -1285,9 +1376,18 @@ ath_legacy_startrecv(struct ath_softc *s
struct ath_hal *ah = sc->sc_ah;
struct ath_buf *bf;
+ ATH_RX_LOCK(sc);
+
+ /*
+ * XXX should verify these are already all NULL!
+ */
sc->sc_rxlink = NULL;
- sc->sc_rxedma[HAL_RX_QUEUE_LP].m_rxpending = NULL;
- sc->sc_rxedma[HAL_RX_QUEUE_HP].m_rxpending = NULL;
+ (void) ath_legacy_flush_rxpending(sc);
+ (void) ath_legacy_flush_rxholdbf(sc);
+
+ /*
+ * Re-chain all of the buffers in the RX buffer list.
+ */
TAILQ_FOREACH(bf, &sc->sc_rxbuf, bf_list) {
int error = ath_rxbuf_init(sc, bf);
if (error != 0) {
@@ -1303,6 +1403,8 @@ ath_legacy_startrecv(struct ath_softc *s
ath_hal_rxena(ah); /* enable recv descriptors */
ath_mode_init(sc); /* set filters, etc. */
ath_hal_startpcurecv(ah); /* re-enable PCU/DMA engine */
+
+ ATH_RX_UNLOCK(sc);
return 0;
}
Modified: head/sys/dev/ath/if_ath_sysctl.c
==============================================================================
--- head/sys/dev/ath/if_ath_sysctl.c Tue May 6 00:00:07 2014
(r265408)
+++ head/sys/dev/ath/if_ath_sysctl.c Tue May 6 01:15:42 2014
(r265409)
@@ -413,12 +413,14 @@ ath_sysctl_txagg(SYSCTL_HANDLER_ARGS)
ATH_RX_LOCK(sc);
for (i = 0; i < 2; i++) {
- printf("%d: fifolen: %d/%d; head=%d; tail=%d\n",
+ printf("%d: fifolen: %d/%d; head=%d; tail=%d; m_pending=%p,
m_holdbf=%p\n",
i,
sc->sc_rxedma[i].m_fifo_depth,
sc->sc_rxedma[i].m_fifolen,
sc->sc_rxedma[i].m_fifo_head,
- sc->sc_rxedma[i].m_fifo_tail);
+ sc->sc_rxedma[i].m_fifo_tail,
+ sc->sc_rxedma[i].m_rxpending,
+ sc->sc_rxedma[i].m_holdbf);
}
i = 0;
TAILQ_FOREACH(bf, &sc->sc_rxbuf, bf_list) {
Modified: head/sys/dev/ath/if_athvar.h
==============================================================================
--- head/sys/dev/ath/if_athvar.h Tue May 6 00:00:07 2014
(r265408)
+++ head/sys/dev/ath/if_athvar.h Tue May 6 01:15:42 2014
(r265409)
@@ -510,6 +510,7 @@ struct ath_rx_edma {
int m_fifo_tail;
int m_fifo_depth;
struct mbuf *m_rxpending;
+ struct ath_buf *m_holdbf;
};
struct ath_tx_edma_fifo {
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
