Date: Mon, 24 Sep 2012 06:42:20 +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: r240883 - head/sys/dev/ath Message-ID: <201209240642.q8O6gK5u061707@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Mon Sep 24 06:42:20 2012 New Revision: 240883 URL: http://svn.freebsd.org/changeset/base/240883 Log: Prepare for software retransmission of non-aggregate frames but ensure it's disabled. The previous commit to enable CLRDMASK setting didn't do it at all correctly for non-aggregate sessions - so the CLRDMASK bit would be cleared and never re-set. * move ath_tx_update_clrdmask() to be called by functions that setup descriptors and queue frames to the hardware, rather than scattered everywhere. * Force CLRDMASK to be set on all non-aggregate session frames being transmitted. * Use ath_tx_normal_comp() now on non-aggregate sessoin frames that are queued via ath_tx_xmit_normal(). That way the TID hwq is updated and they can trigger (eventual) filter frame queue resets and software retransmits. There's still a bit more work to do in this area to reverse the silly short-sightedness on my part, however it's likely going to be better to fix this now than just reverting the patch. Thanks to people on the freebsd-wireless@ mailing list for promptly pointing this out. Modified: head/sys/dev/ath/if_ath_tx.c Modified: head/sys/dev/ath/if_ath_tx.c ============================================================================== --- head/sys/dev/ath/if_ath_tx.c Mon Sep 24 06:00:51 2012 (r240882) +++ head/sys/dev/ath/if_ath_tx.c Mon Sep 24 06:42:20 2012 (r240883) @@ -1260,6 +1260,22 @@ ath_tx_do_ratelookup(struct ath_softc *s } /* + * Update the CLRDMASK bit in the ath_buf if it needs to be set. + */ +static void +ath_tx_update_clrdmask(struct ath_softc *sc, struct ath_tid *tid, + struct ath_buf *bf) +{ + + ATH_TID_LOCK_ASSERT(sc, tid); + + if (tid->clrdmask == 1) { + bf->bf_state.bfs_txflags |= HAL_TXDESC_CLRDMASK; + tid->clrdmask = 0; + } +} + +/* * Transmit the given frame to the hardware. * * The frame must already be setup; rate control must already have @@ -1274,9 +1290,25 @@ static void ath_tx_xmit_normal(struct ath_softc *sc, struct ath_txq *txq, struct ath_buf *bf) { + struct ath_node *an = ATH_NODE(bf->bf_node); + struct ath_tid *tid = &an->an_tid[bf->bf_state.bfs_tid]; ATH_TXQ_LOCK_ASSERT(txq); + /* + * For now, just enable CLRDMASK. ath_tx_xmit_normal() does + * set a completion handler however it doesn't (yet) properly + * handle the strict ordering requirements needed for normal, + * non-aggregate session frames. + * + * Once this is implemented, only set CLRDMASK like this for + * frames that must go out - eg management/raw frames. + */ + bf->bf_state.bfs_txflags |= HAL_TXDESC_CLRDMASK; + + /* See if clrdmask needs to be set */ + ath_tx_update_clrdmask(sc, tid, bf); + /* Setup the descriptor before handoff */ ath_tx_do_ratelookup(sc, bf); ath_tx_calc_duration(sc, bf); @@ -1285,8 +1317,11 @@ ath_tx_xmit_normal(struct ath_softc *sc, ath_tx_rate_fill_rcflags(sc, bf); ath_tx_setds(sc, bf); - /* XXX Bump TID HWQ counter */ - /* XXX Assign a completion handler */ + /* Track per-TID hardware queue depth correctly */ + tid->hwq_depth++; + + /* Assign the completion handler */ + bf->bf_comp = ath_tx_normal_comp; /* Hand off to hardware */ ath_tx_handoff(sc, txq, bf); @@ -2485,22 +2520,6 @@ ath_tx_tid_unsched(struct ath_softc *sc, } /* - * Update the CLRDMASK bit in the ath_buf if it needs to be set. - */ -static void -ath_tx_update_clrdmask(struct ath_softc *sc, struct ath_tid *tid, - struct ath_buf *bf) -{ - - ATH_TID_LOCK_ASSERT(sc, tid); - - if (tid->clrdmask == 1) { - bf->bf_state.bfs_txflags |= HAL_TXDESC_CLRDMASK; - tid->clrdmask = 0; - } -} - -/* * Assign a sequence number manually to the given frame. * * This should only be called for A-MPDU TX frames. @@ -2738,7 +2757,6 @@ ath_tx_swq(struct ath_softc *sc, struct } else if (txq->axq_depth < sc->sc_hwq_limit) { /* AMPDU not running, attempt direct dispatch */ DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: xmit_normal\n", __func__); - ath_tx_update_clrdmask(sc, atid, bf); ath_tx_xmit_normal(sc, txq, bf); } else { /* Busy; queue */ @@ -3432,6 +3450,19 @@ ath_tx_txq_drain(struct ath_softc *sc, s /* * Handle completion of non-aggregate session frames. + * + * This (currently) doesn't implement software retransmission of + * non-aggregate frames! + * + * Software retransmission of non-aggregate frames needs to obey + * the strict sequence number ordering, and drop any frames that + * will fail this. + * + * For now, filtered frames and frame transmission will cause + * all kinds of issues. So we don't support them. + * + * So anyone queuing frames via ath_tx_normal_xmit() or + * ath_tx_hw_queue_norm() must override and set CLRDMASK. */ void ath_tx_normal_comp(struct ath_softc *sc, struct ath_buf *bf, int fail) @@ -3450,10 +3481,23 @@ ath_tx_normal_comp(struct ath_softc *sc, atid->hwq_depth--; +#if 0 + /* + * If the frame was filtered, stick it on the filter frame + * queue and complain about it. It shouldn't happen! + */ + if ((ts->ts_status & HAL_TXERR_FILT) || + (ts->ts_status != 0 && atid->isfiltered)) { + device_printf(sc->sc_dev, + "%s: isfiltered=%d, ts_status=%d: huh?\n", + __func__, + atid->isfiltered, + ts->ts_status); + ath_tx_tid_filt_comp_buf(sc, atid, bf); + } +#endif if (atid->isfiltered) - device_printf(sc->sc_dev, "%s: isfiltered=1, normal_comp?\n", - __func__); - + device_printf(sc->sc_dev, "%s: filtered?!\n", __func__); if (atid->hwq_depth < 0) device_printf(sc->sc_dev, "%s: hwq_depth < 0: %d\n", __func__, atid->hwq_depth); @@ -3471,7 +3515,6 @@ ath_tx_normal_comp(struct ath_softc *sc, */ if (atid->isfiltered) ath_tx_tid_filt_comp_complete(sc, atid); - ATH_TXQ_UNLOCK(sc->sc_ac2q[atid->ac]); /* @@ -4766,6 +4809,12 @@ ath_tx_tid_hw_queue_norm(struct ath_soft /* Normal completion handler */ bf->bf_comp = ath_tx_normal_comp; + /* + * Override this for now, until the non-aggregate + * completion handler correctly handles software retransmits. + */ + bf->bf_state.bfs_txflags |= HAL_TXDESC_CLRDMASK; + /* Update CLRDMASK just before this frame is queued */ ath_tx_update_clrdmask(sc, tid, bf);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209240642.q8O6gK5u061707>