From owner-svn-src-head@FreeBSD.ORG Tue Sep 18 20:33:04 2012 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id A30DA106566C; Tue, 18 Sep 2012 20:33:04 +0000 (UTC) (envelope-from adrian@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:4f8:fff6::2c]) by mx1.freebsd.org (Postfix) with ESMTP id 8E2528FC0C; Tue, 18 Sep 2012 20:33:04 +0000 (UTC) Received: from svn.freebsd.org (localhost [127.0.0.1]) by svn.freebsd.org (8.14.4/8.14.4) with ESMTP id q8IKX4QH057549; Tue, 18 Sep 2012 20:33:04 GMT (envelope-from adrian@svn.freebsd.org) Received: (from adrian@localhost) by svn.freebsd.org (8.14.4/8.14.4/Submit) id q8IKX4O5057547; Tue, 18 Sep 2012 20:33:04 GMT (envelope-from adrian@svn.freebsd.org) Message-Id: <201209182033.q8IKX4O5057547@svn.freebsd.org> From: Adrian Chadd Date: Tue, 18 Sep 2012 20:33:04 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r240677 - head/sys/dev/ath X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Sep 2012 20:33:04 -0000 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__);