Date: Tue, 18 Sep 2012 20:33:04 +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: r240677 - head/sys/dev/ath Message-ID: <201209182033.q8IKX4O5057547@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Tue Sep 18 20:33:04 2012 New Revision: 240677 URL: http://svn.freebsd.org/changeset/base/240677 Log: Oops - take a copy of ath_tx_status from the buffer before the TX processing is done. The aggregate path was definitely accessing 'ts' before it was actually being assigned. This had the side effect of over-filtering frames, since occasionally that bit would be '1'. Whilst here, do the same thing in the non-aggregate completion function - as calling the filter function may also invalidate bf. Pointy hat to: adrian, for not noticing this over many, many code reviews. 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 Tue Sep 18 20:28:55 2012 (r240676) +++ head/sys/dev/ath/if_ath_tx.c Tue Sep 18 20:33:04 2012 (r240677) @@ -3955,6 +3955,12 @@ ath_tx_aggr_comp_aggr(struct ath_softc * DPRINTF(sc, ATH_DEBUG_SW_TX_AGGR, "%s: called; hwq_depth=%d\n", __func__, atid->hwq_depth); + /* + * Take a copy; this may be needed -after- bf_first + * has been completed and freed. + */ + ts = bf_first->bf_status.ds_txstat; + TAILQ_INIT(&bf_q); TAILQ_INIT(&bf_cq); @@ -3970,6 +3976,8 @@ ath_tx_aggr_comp_aggr(struct ath_softc * * If the TID is filtered, handle completing the filter * transition before potentially kicking it to the cleanup * function. + * + * XXX this is duplicate work, ew. */ if (atid->isfiltered) ath_tx_tid_filt_comp_complete(sc, atid); @@ -4030,11 +4038,6 @@ ath_tx_aggr_comp_aggr(struct ath_softc * } /* - * Take a copy; this may be needed -after- bf_first - * has been completed and freed. - */ - ts = bf_first->bf_status.ds_txstat; - /* * XXX for now, use the first frame in the aggregate for * XXX rate control completion; it's at least consistent. */ @@ -4255,10 +4258,16 @@ ath_tx_aggr_comp_unaggr(struct ath_softc struct ath_node *an = ATH_NODE(ni); int tid = bf->bf_state.bfs_tid; struct ath_tid *atid = &an->an_tid[tid]; - struct ath_tx_status *ts = &bf->bf_status.ds_txstat; + struct ath_tx_status ts; int drops = 0; /* + * Take a copy of this; filtering/cloning the frame may free the + * bf pointer. + */ + ts = bf->bf_status.ds_txstat; + + /* * Update rate control status here, before we possibly * punt to retry or cleanup. * @@ -4268,7 +4277,7 @@ ath_tx_aggr_comp_unaggr(struct ath_softc ath_tx_update_ratectrl(sc, ni, bf->bf_state.bfs_rc, &bf->bf_status.ds_txstat, bf->bf_state.bfs_pktlen, - 1, (ts->ts_status == 0) ? 0 : 1); + 1, (ts.ts_status == 0) ? 0 : 1); /* * This is called early so atid->hwq_depth can be tracked. @@ -4328,8 +4337,8 @@ ath_tx_aggr_comp_unaggr(struct ath_softc * list as it will end up being recycled without having * been made available for the hardware. */ - if ((ts->ts_status & HAL_TXERR_FILT) || - (ts->ts_status != 0 && atid->isfiltered)) { + if ((ts.ts_status & HAL_TXERR_FILT) || + (ts.ts_status != 0 && atid->isfiltered)) { int freeframe; if (fail != 0) @@ -4383,7 +4392,7 @@ ath_tx_aggr_comp_unaggr(struct ath_softc #if 0 if (fail == 0 && ts->ts_status & HAL_TXERR_XRETRY) { #endif - if (fail == 0 && ts->ts_status != 0) { + if (fail == 0 && ts.ts_status != 0) { ATH_TXQ_UNLOCK(sc->sc_ac2q[atid->ac]); DPRINTF(sc, ATH_DEBUG_SW_TX, "%s: retry_unaggr\n", __func__);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201209182033.q8IKX4O5057547>