Date: Sat, 18 May 2013 18:27:53 +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: r250783 - head/sys/dev/ath Message-ID: <201305181827.r4IIRrOo069292@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Sat May 18 18:27:53 2013 New Revision: 250783 URL: http://svnweb.freebsd.org/changeset/base/250783 Log: Be (very) careful about how to add more TX DMA work. The list-based DMA engine has the following behaviour: * When the DMA engine is in the init state, you can write the first descriptor address to the QCU TxDP register and it will work. * Then when it hits the end of the list (ie, it either hits a NULL link pointer, OR it hits a descriptor with VEOL set) the QCU stops, and the TxDP points to the last descriptor that was transmitted. * Then when you want to transmit a new frame, you can then either: + write the head of the new list into TxDP, or + you write the head of the new list into the link pointer of the last completed descriptor (ie, where TxDP points), then kick TxE to restart transmission on that QCU> * The hardware then will re-read the descriptor to pick up the link pointer and then jump to that. Now, the quirks: * If you write a TxDP when there's been no previous TxDP (ie, it's 0), it works. * If you write a TxDP in any other instance, the TxDP write may actually fail. Thus, when you start transmission, it will re-read the last transmitted descriptor to get the link pointer, NOT just start a new transmission. So the correct thing to do here is: * ALWAYS use the holding descriptor (ie, the last transmitted descriptor that we've kept safe) and use the link pointer in _THAT_ to transmit the next frame. * NEVER write to the TxDP after you've done the initial write. * .. also, don't do this whilst you're also resetting the NIC. With this in mind, the following patch does basically the above. * Since this encapsulates Sam's issues with the QCU behaviour w/ TDMA, kill the TDMA special case and replace it with the above. * Add a new TXQ flag - PUTRUNNING - which indicates that we've started DMA. * Clear that flag when DMA has been shutdown. * Ensure that we're not restarting DMA with PUTRUNNING enabled. * Fix the link pointer logic during TXQ drain - we should always ensure the link pointer does point to something if there's a list of frames. Having it be NULL as an indication that DMA has finished or during a reset causes trouble. Now, given all of this, i want to nuke axq_link from orbit. There's now HAL methods to get and set the link pointer of a descriptor, so what we should do instead is to update the right link pointer. * If there's a holding descriptor and an empty TXQ list, set the link pointer of said holding descriptor to the new frame. * If there's a non-empty TXQ list, set the link pointer of the last descriptor in the list to the new frame. * Nuke axq_link from orbit. Note: * The AR9380 doesn't need this. FIFO TX writes are atomic. As long as we don't append to a list of frames that we've already passed to the hardware, all of the above doesn't apply. The holding descriptor stuff is still needed to ensure the hardware can re-read a completed descriptor to move onto the next one, but we restart DMA by pushing in a new FIFO entry into the TX QCU. That doesn't require any real gymnastics. Tested: * AR5210, AR5211, AR5212, AR5416, AR9380 - STA mode. Modified: head/sys/dev/ath/if_ath.c head/sys/dev/ath/if_ath_beacon.c head/sys/dev/ath/if_ath_misc.h head/sys/dev/ath/if_ath_tx.c head/sys/dev/ath/if_athvar.h Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Sat May 18 18:01:21 2013 (r250782) +++ head/sys/dev/ath/if_ath.c Sat May 18 18:27:53 2013 (r250783) @@ -4620,6 +4620,8 @@ ath_tx_stopdma(struct ath_softc *sc, str { struct ath_hal *ah = sc->sc_ah; + ATH_TXQ_LOCK_ASSERT(txq); + DPRINTF(sc, ATH_DEBUG_RESET, "%s: tx queue [%u] %p, active=%d, hwpending=%d, flags 0x%08x, " "link %p, holdingbf=%p\n", @@ -4633,6 +4635,8 @@ ath_tx_stopdma(struct ath_softc *sc, str txq->axq_holdingbf); (void) ath_hal_stoptxdma(ah, txq->axq_qnum); + /* We've stopped TX DMA, so mark this as stopped. */ + txq->axq_flags &= ~ATH_TXQ_PUTRUNNING; #ifdef ATH_DEBUG if ((sc->sc_debug & ATH_DEBUG_RESET) @@ -4658,17 +4662,25 @@ ath_stoptxdma(struct ath_softc *sc) __func__, sc->sc_bhalq, (caddr_t)(uintptr_t) ath_hal_gettxbuf(ah, sc->sc_bhalq), NULL); + + /* stop the beacon queue */ (void) ath_hal_stoptxdma(ah, sc->sc_bhalq); - for (i = 0; i < HAL_NUM_TX_QUEUES; i++) - if (ATH_TXQ_SETUP(sc, i)) + + /* Stop the data queues */ + for (i = 0; i < HAL_NUM_TX_QUEUES; i++) { + if (ATH_TXQ_SETUP(sc, i)) { + ATH_TXQ_LOCK(&sc->sc_txq[i]); ath_tx_stopdma(sc, &sc->sc_txq[i]); + ATH_TXQ_UNLOCK(&sc->sc_txq[i]); + } + } } return 1; } #ifdef ATH_DEBUG -static void +void ath_tx_dump(struct ath_softc *sc, struct ath_txq *txq) { struct ath_hal *ah = sc->sc_ah; @@ -4702,6 +4714,7 @@ ath_legacy_tx_drain(struct ath_softc *sc #endif struct ifnet *ifp = sc->sc_ifp; int i; + struct ath_buf *bf_last; (void) ath_stoptxdma(sc); @@ -4727,10 +4740,20 @@ ath_legacy_tx_drain(struct ath_softc *sc */ ath_txq_freeholdingbuf(sc, &sc->sc_txq[i]); /* - * Reset the link pointer to NULL; there's - * no frames to chain DMA to. + * Setup the link pointer to be the + * _last_ buffer/descriptor in the list. + * If there's nothing in the list, set it + * to NULL. */ - sc->sc_txq[i].axq_link = NULL; + bf_last = ATH_TXQ_LAST(&sc->sc_txq[i], + axq_q_s); + if (bf_last != NULL) { + ath_hal_gettxdesclinkptr(ah, + bf_last->bf_lastds, + &sc->sc_txq[i].axq_link); + } else { + sc->sc_txq[i].axq_link = NULL; + } ATH_TXQ_UNLOCK(&sc->sc_txq[i]); } else ath_tx_draintxq(sc, &sc->sc_txq[i]); Modified: head/sys/dev/ath/if_ath_beacon.c ============================================================================== --- head/sys/dev/ath/if_ath_beacon.c Sat May 18 18:01:21 2013 (r250782) +++ head/sys/dev/ath/if_ath_beacon.c Sat May 18 18:27:53 2013 (r250783) @@ -642,6 +642,7 @@ ath_beacon_cabq_start_edma(struct ath_so /* Push the first entry into the hardware */ ath_hal_puttxbuf(sc->sc_ah, cabq->axq_qnum, bf->bf_daddr); + cabq->axq_flags |= ATH_TXQ_PUTRUNNING; /* NB: gated by beacon so safe to start here */ ath_hal_txstart(sc->sc_ah, cabq->axq_qnum); @@ -661,6 +662,7 @@ ath_beacon_cabq_start_legacy(struct ath_ /* Push the first entry into the hardware */ ath_hal_puttxbuf(sc->sc_ah, cabq->axq_qnum, bf->bf_daddr); + cabq->axq_flags |= ATH_TXQ_PUTRUNNING; /* NB: gated by beacon so safe to start here */ ath_hal_txstart(sc->sc_ah, cabq->axq_qnum); @@ -736,6 +738,16 @@ ath_beacon_generate(struct ath_softc *sc * frames from a different vap. * XXX could be slow causing us to miss DBA */ + /* + * XXX TODO: this doesn't stop CABQ DMA - it assumes + * that since we're about to transmit a beacon, we've + * already stopped transmitting on the CABQ. But this + * doesn't at all mean that the CABQ DMA QCU will + * accept a new TXDP! So what, should we do a DMA + * stop? What if it fails? + * + * More thought is required here. + */ ath_tx_draintxq(sc, cabq); } } Modified: head/sys/dev/ath/if_ath_misc.h ============================================================================== --- head/sys/dev/ath/if_ath_misc.h Sat May 18 18:01:21 2013 (r250782) +++ head/sys/dev/ath/if_ath_misc.h Sat May 18 18:27:53 2013 (r250783) @@ -125,6 +125,8 @@ extern void ath_tx_update_tim(struct ath extern void ath_start(struct ifnet *ifp); extern void ath_start_task(void *arg, int npending); +extern void ath_tx_dump(struct ath_softc *sc, struct ath_txq *txq); + /* * Kick the frame TX task. */ Modified: head/sys/dev/ath/if_ath_tx.c ============================================================================== --- head/sys/dev/ath/if_ath_tx.c Sat May 18 18:01:21 2013 (r250782) +++ head/sys/dev/ath/if_ath_tx.c Sat May 18 18:27:53 2013 (r250783) @@ -744,6 +744,7 @@ ath_tx_handoff_hw(struct ath_softc *sc, struct ath_buf *bf) { struct ath_hal *ah = sc->sc_ah; + struct ath_buf *bf_first; /* * Insert the frame on the outbound list and pass it on @@ -759,6 +760,13 @@ ath_tx_handoff_hw(struct ath_softc *sc, KASSERT(txq->axq_qnum != ATH_TXQ_SWQ, ("ath_tx_handoff_hw called for mcast queue")); + /* + * XXX racy, should hold the PCU lock when checking this, + * and also should ensure that the TX counter is >0! + */ + KASSERT((sc->sc_inreset_cnt == 0), + ("%s: TX during reset?\n", __func__)); + #if 0 /* * This causes a LOR. Find out where the PCU lock is being @@ -776,161 +784,141 @@ ath_tx_handoff_hw(struct ath_softc *sc, __func__, txq->axq_qnum, (caddr_t)bf->bf_daddr, bf->bf_desc, txq->axq_depth); + /* XXX axq_link needs to be set and updated! */ ATH_TXQ_INSERT_TAIL(txq, bf, bf_list); if (bf->bf_state.bfs_aggr) txq->axq_aggr_depth++; - /* - * There's no need to update axq_link; the hardware - * is in reset and once the reset is complete, any - * non-empty queues will simply have DMA restarted. - */ return; } ATH_PCU_UNLOCK(sc); #endif - /* For now, so not to generate whitespace diffs */ - if (1) { - ATH_TXQ_LOCK(txq); -#ifdef IEEE80211_SUPPORT_TDMA - int qbusy; + ATH_TXQ_LOCK(txq); - ATH_TXQ_INSERT_TAIL(txq, bf, bf_list); - qbusy = ath_hal_txqenabled(ah, txq->axq_qnum); + /* + * XXX TODO: if there's a holdingbf, then + * ATH_TXQ_PUTRUNNING should be clear. + * + * If there is a holdingbf and the list is empty, + * then axq_link should be pointing to the holdingbf. + * + * Otherwise it should point to the last descriptor + * in the last ath_buf. + * + * In any case, we should really ensure that we + * update the previous descriptor link pointer to + * this descriptor, regardless of all of the above state. + * + * For now this is captured by having axq_link point + * to either the holdingbf (if the TXQ list is empty) + * or the end of the list (if the TXQ list isn't empty.) + * I'd rather just kill axq_link here and do it as above. + */ - ATH_KTR(sc, ATH_KTR_TX, 4, - "ath_tx_handoff: txq=%u, add bf=%p, qbusy=%d, depth=%d", - txq->axq_qnum, bf, qbusy, txq->axq_depth); - if (txq->axq_link == NULL) { - /* - * Be careful writing the address to TXDP. If - * the tx q is enabled then this write will be - * ignored. Normally this is not an issue but - * when tdma is in use and the q is beacon gated - * this race can occur. If the q is busy then - * defer the work to later--either when another - * packet comes along or when we prepare a beacon - * frame at SWBA. - */ - if (!qbusy) { - ath_hal_puttxbuf(ah, txq->axq_qnum, - bf->bf_daddr); - txq->axq_flags &= ~ATH_TXQ_PUTPENDING; - DPRINTF(sc, ATH_DEBUG_XMIT, - "%s: TXDP[%u] = %p (%p) lastds=%p depth %d\n", - __func__, txq->axq_qnum, - (caddr_t)bf->bf_daddr, bf->bf_desc, - bf->bf_lastds, - txq->axq_depth); - ATH_KTR(sc, ATH_KTR_TX, 5, - "ath_tx_handoff: TXDP[%u] = %p (%p) " - "lastds=%p depth %d", - txq->axq_qnum, - (caddr_t)bf->bf_daddr, bf->bf_desc, - bf->bf_lastds, - txq->axq_depth); - } else { - txq->axq_flags |= ATH_TXQ_PUTPENDING; - DPRINTF(sc, ATH_DEBUG_TDMA | ATH_DEBUG_XMIT, - "%s: Q%u busy, defer enable\n", __func__, - txq->axq_qnum); - ATH_KTR(sc, ATH_KTR_TX, 0, "defer enable"); - } - } else { - *txq->axq_link = bf->bf_daddr; - DPRINTF(sc, ATH_DEBUG_XMIT, - "%s: link[%u](%p)=%p (%p) depth %d\n", __func__, - txq->axq_qnum, txq->axq_link, - (caddr_t)bf->bf_daddr, bf->bf_desc, - txq->axq_depth); - ATH_KTR(sc, ATH_KTR_TX, 5, - "ath_tx_handoff: link[%u](%p)=%p (%p) lastds=%p", - txq->axq_qnum, txq->axq_link, - (caddr_t)bf->bf_daddr, bf->bf_desc, - bf->bf_lastds); - - if ((txq->axq_flags & ATH_TXQ_PUTPENDING) && !qbusy) { - /* - * The q was busy when we previously tried - * to write the address of the first buffer - * in the chain. Since it's not busy now - * handle this chore. We are certain the - * buffer at the front is the right one since - * axq_link is NULL only when the buffer list - * is/was empty. - */ - ath_hal_puttxbuf(ah, txq->axq_qnum, - TAILQ_FIRST(&txq->axq_q)->bf_daddr); - txq->axq_flags &= ~ATH_TXQ_PUTPENDING; - DPRINTF(sc, ATH_DEBUG_TDMA | ATH_DEBUG_XMIT, - "%s: Q%u restarted\n", __func__, - txq->axq_qnum); - ATH_KTR(sc, ATH_KTR_TX, 4, - "ath_tx_handoff: txq[%d] restarted, bf=%p " - "daddr=%p ds=%p", - txq->axq_qnum, - bf, - (caddr_t)bf->bf_daddr, - bf->bf_desc); - } - } -#else - ATH_TXQ_INSERT_TAIL(txq, bf, bf_list); - ATH_KTR(sc, ATH_KTR_TX, 3, - "ath_tx_handoff: non-tdma: txq=%u, add bf=%p " - "depth=%d", + /* + * Append the frame to the TX queue. + */ + ATH_TXQ_INSERT_TAIL(txq, bf, bf_list); + ATH_KTR(sc, ATH_KTR_TX, 3, + "ath_tx_handoff: non-tdma: txq=%u, add bf=%p " + "depth=%d", + txq->axq_qnum, + bf, + txq->axq_depth); + + /* + * If there's a link pointer, update it. + * + * XXX we should replace this with the above logic, just + * to kill axq_link with fire. + */ + if (txq->axq_link != NULL) { + *txq->axq_link = bf->bf_daddr; + DPRINTF(sc, ATH_DEBUG_XMIT, + "%s: link[%u](%p)=%p (%p) depth %d\n", __func__, + txq->axq_qnum, txq->axq_link, + (caddr_t)bf->bf_daddr, bf->bf_desc, + txq->axq_depth); + ATH_KTR(sc, ATH_KTR_TX, 5, + "ath_tx_handoff: non-tdma: link[%u](%p)=%p (%p) " + "lastds=%d", + txq->axq_qnum, txq->axq_link, + (caddr_t)bf->bf_daddr, bf->bf_desc, + bf->bf_lastds); + } + + /* + * If we've not pushed anything into the hardware yet, + * push the head of the queue into the TxDP. + * + * Once we've started DMA, there's no guarantee that + * updating the TxDP with a new value will actually work. + * So we just don't do that - if we hit the end of the list, + * we keep that buffer around (the "holding buffer") and + * re-start DMA by updating the link pointer of _that_ + * descriptor and then restart DMA. + */ + if (! (txq->axq_flags & ATH_TXQ_PUTRUNNING)) { + bf_first = TAILQ_FIRST(&txq->axq_q); + txq->axq_flags |= ATH_TXQ_PUTRUNNING; + ath_hal_puttxbuf(ah, txq->axq_qnum, bf_first->bf_daddr); + DPRINTF(sc, ATH_DEBUG_XMIT, + "%s: TXDP[%u] = %p (%p) depth %d\n", + __func__, txq->axq_qnum, + (caddr_t)bf_first->bf_daddr, bf_first->bf_desc, + txq->axq_depth); + ATH_KTR(sc, ATH_KTR_TX, 5, + "ath_tx_handoff: TXDP[%u] = %p (%p) " + "lastds=%p depth %d", txq->axq_qnum, - bf, + (caddr_t)bf_first->bf_daddr, bf_first->bf_desc, + bf_first->bf_lastds, txq->axq_depth); - if (txq->axq_link == NULL) { - ath_hal_puttxbuf(ah, txq->axq_qnum, bf->bf_daddr); - DPRINTF(sc, ATH_DEBUG_XMIT, - "%s: TXDP[%u] = %p (%p) depth %d\n", - __func__, txq->axq_qnum, - (caddr_t)bf->bf_daddr, bf->bf_desc, - txq->axq_depth); - ATH_KTR(sc, ATH_KTR_TX, 5, - "ath_tx_handoff: non-tdma: TXDP[%u] = %p (%p) " - "lastds=%p depth %d", - txq->axq_qnum, - (caddr_t)bf->bf_daddr, bf->bf_desc, - bf->bf_lastds, - txq->axq_depth); + } - } else { - *txq->axq_link = bf->bf_daddr; - DPRINTF(sc, ATH_DEBUG_XMIT, - "%s: link[%u](%p)=%p (%p) depth %d\n", __func__, - txq->axq_qnum, txq->axq_link, - (caddr_t)bf->bf_daddr, bf->bf_desc, - txq->axq_depth); - ATH_KTR(sc, ATH_KTR_TX, 5, - "ath_tx_handoff: non-tdma: link[%u](%p)=%p (%p) " - "lastds=%d", - txq->axq_qnum, txq->axq_link, - (caddr_t)bf->bf_daddr, bf->bf_desc, - bf->bf_lastds); + /* + * Ensure that the bf TXQ matches this TXQ, so later + * checking and holding buffer manipulation is sane. + */ + if (bf->bf_state.bfs_tx_queue != txq->axq_qnum) { + device_printf(sc->sc_dev, + "%s: bf=%p, bfs_tx_queue=%d, axq_qnum=%d\n", + __func__, + bf, + bf->bf_state.bfs_tx_queue, + txq->axq_qnum); + } - } -#endif /* IEEE80211_SUPPORT_TDMA */ + /* + * Track aggregate queue depth. + */ + if (bf->bf_state.bfs_aggr) + txq->axq_aggr_depth++; - if (bf->bf_state.bfs_tx_queue != txq->axq_qnum) { - device_printf(sc->sc_dev, - "%s: bf=%p, bfs_tx_queue=%d, axq_qnum=%d\n", - __func__, - bf, - bf->bf_state.bfs_tx_queue, - txq->axq_qnum); - } + /* + * Update the link pointer. + */ + ath_hal_gettxdesclinkptr(ah, bf->bf_lastds, &txq->axq_link); - if (bf->bf_state.bfs_aggr) - txq->axq_aggr_depth++; - ath_hal_gettxdesclinkptr(ah, bf->bf_lastds, &txq->axq_link); - ath_hal_txstart(ah, txq->axq_qnum); - ATH_TXQ_UNLOCK(txq); - ATH_KTR(sc, ATH_KTR_TX, 1, - "ath_tx_handoff: txq=%u, txstart", txq->axq_qnum); - } + /* + * Start DMA. + * + * If we wrote a TxDP above, DMA will start from here. + * + * If DMA is running, it'll do nothing. + * + * If the DMA engine hit the end of the QCU list (ie LINK=NULL, + * or VEOL) then it stops at the last transmitted write. + * We then append a new frame by updating the link pointer + * in that descriptor and then kick TxE here; it will re-read + * that last descriptor and find the new descriptor to transmit. + * + * This is why we keep the holding descriptor around. + */ + ath_hal_txstart(ah, txq->axq_qnum); + ATH_TXQ_UNLOCK(txq); + ATH_KTR(sc, ATH_KTR_TX, 1, + "ath_tx_handoff: txq=%u, txstart", txq->axq_qnum); } /* @@ -945,8 +933,6 @@ ath_legacy_tx_dma_restart(struct ath_sof struct ath_buf *bf, *bf_last; ATH_TXQ_LOCK_ASSERT(txq); - /* This is always going to be cleared, empty or not */ - txq->axq_flags &= ~ATH_TXQ_PUTPENDING; /* XXX make this ATH_TXQ_FIRST */ bf = TAILQ_FIRST(&txq->axq_q); @@ -955,7 +941,29 @@ ath_legacy_tx_dma_restart(struct ath_sof if (bf == NULL) return; - ath_hal_puttxbuf(ah, txq->axq_qnum, bf->bf_daddr); + DPRINTF(sc, ATH_DEBUG_RESET, + "%s: Q%d: bf=%p, bf_last=%p, daddr=0x%08x\n", + __func__, + txq->axq_qnum, + bf, + bf_last, + (uint32_t) bf->bf_daddr); + + if (sc->sc_debug & ATH_DEBUG_RESET) + ath_tx_dump(sc, txq); + + /* + * This is called from a restart, so DMA is known to be + * completely stopped. + */ + KASSERT((!(txq->axq_flags & ATH_TXQ_PUTRUNNING)), + ("%s: Q%d: called with PUTRUNNING=1\n", + __func__, + txq->axq_qnum)); + + ath_hal_puttxbuf(sc->sc_ah, txq->axq_qnum, bf->bf_daddr); + txq->axq_flags |= ATH_TXQ_PUTRUNNING; + ath_hal_gettxdesclinkptr(ah, bf_last->bf_lastds, &txq->axq_link); ath_hal_txstart(ah, txq->axq_qnum); } Modified: head/sys/dev/ath/if_athvar.h ============================================================================== --- head/sys/dev/ath/if_athvar.h Sat May 18 18:01:21 2013 (r250782) +++ head/sys/dev/ath/if_athvar.h Sat May 18 18:27:53 2013 (r250783) @@ -327,7 +327,8 @@ struct ath_txq { #define ATH_TXQ_SWQ (HAL_NUM_TX_QUEUES+1) /* qnum for s/w only queue */ u_int axq_ac; /* WME AC */ u_int axq_flags; -#define ATH_TXQ_PUTPENDING 0x0001 /* ath_hal_puttxbuf pending */ +//#define ATH_TXQ_PUTPENDING 0x0001 /* ath_hal_puttxbuf pending */ +#define ATH_TXQ_PUTRUNNING 0x0002 /* ath_hal_puttxbuf has been called */ u_int axq_depth; /* queue depth (stat only) */ u_int axq_aggr_depth; /* how many aggregates are queued */ u_int axq_intrcnt; /* interrupt count */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201305181827.r4IIRrOo069292>