From owner-svn-src-all@FreeBSD.ORG Fri Mar 15 02:52:38 2013 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 936B445D; Fri, 15 Mar 2013 02:52:38 +0000 (UTC) (envelope-from adrian@FreeBSD.org) Received: from svn.freebsd.org (svn.freebsd.org [IPv6:2001:1900:2254:2068::e6a:0]) by mx1.freebsd.org (Postfix) with ESMTP id 76C8FABC; Fri, 15 Mar 2013 02:52:38 +0000 (UTC) Received: from svn.freebsd.org ([127.0.1.70]) by svn.freebsd.org (8.14.6/8.14.6) with ESMTP id r2F2qcmU030025; Fri, 15 Mar 2013 02:52:38 GMT (envelope-from adrian@svn.freebsd.org) Received: (from adrian@localhost) by svn.freebsd.org (8.14.6/8.14.5/Submit) id r2F2qcpx030023; Fri, 15 Mar 2013 02:52:38 GMT (envelope-from adrian@svn.freebsd.org) Message-Id: <201303150252.r2F2qcpx030023@svn.freebsd.org> From: Adrian Chadd Date: Fri, 15 Mar 2013 02:52:38 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r248311 - head/sys/dev/ath X-SVN-Group: head MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Mar 2013 02:52:38 -0000 Author: adrian Date: Fri Mar 15 02:52:37 2013 New Revision: 248311 URL: http://svnweb.freebsd.org/changeset/base/248311 Log: Add locking around the new holdingbf code. Since this is being done during buffer free, it's a crap shoot whether the TX path lock is held or not. I tried putting the ath_freebuf() code inside the TX lock and I got all kinds of locking issues - it turns out that the buffer free path sometimes is called with the lock held and sometimes isn't. So I'll go and fix that soon. Hence for now the holdingbf buffers are protected by the TXBUF lock. Modified: head/sys/dev/ath/if_ath.c head/sys/dev/ath/if_athvar.h Modified: head/sys/dev/ath/if_ath.c ============================================================================== --- head/sys/dev/ath/if_ath.c Fri Mar 15 01:02:35 2013 (r248310) +++ head/sys/dev/ath/if_ath.c Fri Mar 15 02:52:37 2013 (r248311) @@ -4156,14 +4156,13 @@ ath_returnbuf_head(struct ath_softc *sc, static void ath_txq_freeholdingbuf(struct ath_softc *sc, struct ath_txq *txq) { + ATH_TXBUF_LOCK_ASSERT(sc); if (txq->axq_holdingbf == NULL) return; txq->axq_holdingbf->bf_flags &= ~ATH_BUF_BUSY; - ATH_TXBUF_LOCK(sc); ath_returnbuf_tail(sc, txq->axq_holdingbf); - ATH_TXBUF_UNLOCK(sc); txq->axq_holdingbf = NULL; } @@ -4176,6 +4175,8 @@ ath_txq_addholdingbuf(struct ath_softc * { struct ath_txq *txq; + ATH_TXBUF_LOCK_ASSERT(sc); + /* XXX assert ATH_BUF_BUSY is set */ /* XXX assert the tx queue is under the max number */ @@ -4188,10 +4189,8 @@ ath_txq_addholdingbuf(struct ath_softc * ath_returnbuf_tail(sc, bf); return; } - txq = &sc->sc_txq[bf->bf_state.bfs_tx_queue]; ath_txq_freeholdingbuf(sc, txq); - txq->axq_holdingbf = bf; } @@ -4219,7 +4218,9 @@ ath_freebuf(struct ath_softc *sc, struct * If this buffer is busy, push it onto the holding queue */ if (bf->bf_flags & ATH_BUF_BUSY) { + ATH_TXBUF_LOCK(sc); ath_txq_addholdingbuf(sc, bf); + ATH_TXBUF_UNLOCK(sc); return; } @@ -4342,7 +4343,9 @@ ath_tx_draintxq(struct ath_softc *sc, st /* * Free the holding buffer if it exists */ + ATH_TXBUF_LOCK(sc); ath_txq_freeholdingbuf(sc, txq); + ATH_TXBUF_UNLOCK(sc); /* * Drain software queued frames which are on Modified: head/sys/dev/ath/if_athvar.h ============================================================================== --- head/sys/dev/ath/if_athvar.h Fri Mar 15 01:02:35 2013 (r248310) +++ head/sys/dev/ath/if_athvar.h Fri Mar 15 02:52:37 2013 (r248311) @@ -329,6 +329,15 @@ struct ath_txq { u_int axq_intrcnt; /* interrupt count */ u_int32_t *axq_link; /* link ptr in last TX desc */ TAILQ_HEAD(axq_q_s, ath_buf) axq_q; /* transmit queue */ + /* + * XXX the holdingbf field is protected by the TXBUF lock + * for now, NOT the TX lock. + * + * Architecturally, it would likely be better to move + * the holdingbf field to a separate array in ath_softc + * just to highlight that it's not protected by the normal + * TX path lock. + */ struct ath_buf *axq_holdingbf; /* holding TX buffer */ char axq_name[12]; /* e.g. "ath0_txq4" */