Skip site navigation (1)Skip section navigation (2) 
Date:      Fri, 15 Mar 2013 02:52:38 +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: r248311 - head/sys/dev/ath
Message-ID:  <201303150252.r2F2qcpx030023@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help 
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" */

Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201303150252.r2F2qcpx030023>