From owner-svn-src-user@FreeBSD.ORG Sun Oct 2 05:11:02 2011 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 90A07106566C; Sun, 2 Oct 2011 05:11:02 +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 80B3C8FC0C; Sun, 2 Oct 2011 05:11:02 +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 p925B2dM048730; Sun, 2 Oct 2011 05:11:02 GMT (envelope-from adrian@svn.freebsd.org) Received: (from adrian@localhost) by svn.freebsd.org (8.14.4/8.14.4/Submit) id p925B2oq048727; Sun, 2 Oct 2011 05:11:02 GMT (envelope-from adrian@svn.freebsd.org) Message-Id: <201110020511.p925B2oq048727@svn.freebsd.org> From: Adrian Chadd Date: Sun, 2 Oct 2011 05:11:02 +0000 (UTC) To: src-committers@freebsd.org, svn-src-user@freebsd.org X-SVN-Group: user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cc: Subject: svn commit: r225914 - user/adrian/if_ath_tx/sys/dev/ath X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Oct 2011 05:11:02 -0000 Author: adrian Date: Sun Oct 2 05:11:02 2011 New Revision: 225914 URL: http://svn.freebsd.org/changeset/base/225914 Log: Put the txqactive and kickpcu flags behind ath_lock for now, to avoid racey access to them from different threads. Since the ath_intr() code can be called at the same time as a taskqueue, we need to ensure that these don't clash. It doesn't happen on my testing on single-core MIPS boards w/out PREEMPTION enabled. It's likely to happen on PREEMPTION kernels and with multi-core CPUs. The real solution is to do what the reference driver does - create a pcu lock and serialise all PCU access behind that. I'll look at doing this at a later time. Modified: user/adrian/if_ath_tx/sys/dev/ath/if_ath.c user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h Modified: user/adrian/if_ath_tx/sys/dev/ath/if_ath.c ============================================================================== --- user/adrian/if_ath_tx/sys/dev/ath/if_ath.c Sun Oct 2 02:42:31 2011 (r225913) +++ user/adrian/if_ath_tx/sys/dev/ath/if_ath.c Sun Oct 2 05:11:02 2011 (r225914) @@ -1342,6 +1342,7 @@ ath_intr(void *arg) struct ifnet *ifp = sc->sc_ifp; struct ath_hal *ah = sc->sc_ah; HAL_INT status = 0; + uint32_t txqs; if (sc->sc_invalid) { /* @@ -1381,6 +1382,29 @@ ath_intr(void *arg) ah->ah_intrstate[6]); status &= sc->sc_imask; /* discard unasked for bits */ + /* + * This has now updated the txqactive bits, so + * we should fetch them from the HAL and merge them + * into sc->sc_txq_active. That way we won't miss out + * where one CPU clears the txq bit whilst the other CPU + * sets it. + * + * The HAL updates it if the relevant TX status bits are set + * in the status registers, regardless of whether the status + * caused the interrupt and/or is set in sc_imask. + * Hence we update the bits before we check for status == 0. + */ + ATH_LOCK(sc); + /* + * This returns the txq bits in the given mask and blanks them. + * Since it's only ever set and cleared by the HAL and we are now + * doing it in ath_intr(), it's effectively non-racey. + */ + txqs = 0xffffffff; + ath_hal_gettxintrtxqs(sc->sc_ah, &txqs); + sc->sc_txq_active |= txqs; + ATH_UNLOCK(sc); + /* Short-circuit un-handled interrupts */ if (status == 0x0) return; @@ -1440,15 +1464,29 @@ ath_intr(void *arg) * by a call to ath_reset() somehow, the * interrupt mask will be correctly reprogrammed. */ + ATH_LOCK(sc); imask &= ~(HAL_INT_RXEOL | HAL_INT_RXORN); ath_hal_intrset(ah, imask); /* + * Only blank sc_rxlink if we've not yet kicked + * the PCU. + * + * This isn't entirely correct - the correct solution + * would be to have a PCU lock and engage that for + * the duration of the PCU fiddling; which would include + * running the RX process. Otherwise we could end up + * messing up the RX descriptor chain and making the + * RX desc list much shorter. + */ + if (! sc->sc_kickpcu) + sc->sc_rxlink = NULL; + sc->sc_kickpcu = 1; + ATH_UNLOCK(sc); + /* * Enqueue an RX proc, to handled whatever * is in the RX queue. * This will then kick the PCU. */ - sc->sc_rxlink = NULL; - sc->sc_kickpcu = 1; taskqueue_enqueue_fast(sc->sc_tq, &sc->sc_rxtask); } if (status & HAL_INT_TXURN) { @@ -1490,8 +1528,10 @@ ath_intr(void *arg) * RXEOL before the rxproc has had a chance * to run. */ + ATH_LOCK(sc); if (sc->sc_kickpcu == 0) ath_hal_intrset(ah, sc->sc_imask); + ATH_UNLOCK(sc); } if (status & HAL_INT_RXORN) { /* NB: hal marks HAL_INT_FATAL when RXORN is fatal */ @@ -4103,8 +4143,10 @@ rx_next: ath_mode_init(sc); /* set filters, etc. */ ath_hal_startpcurecv(ah); /* re-enable PCU/DMA engine */ + ATH_LOCK(sc); ath_hal_intrset(ah, sc->sc_imask); sc->sc_kickpcu = 0; + ATH_UNLOCK(sc); } if ((ifp->if_drv_flags & IFF_DRV_OACTIVE) == 0) { @@ -4590,12 +4632,29 @@ ath_tx_processq(struct ath_softc *sc, st return nacked; } +/* + * This is inefficient - it's going to be a lot better + * if the ath_tx_proc* functions took a private snapshot + * of sc_txq_active once, inside the lock, rather than + * calling it multiple times. + * + * So before this makes it into -HEAD, that should be + * implemented. + */ static __inline int -txqactive(struct ath_hal *ah, int qnum) +txqactive(struct ath_softc *sc, int qnum) { u_int32_t txqs = 1<sc_txq_active & txqs; + sc->sc_txq_active &= ~txqs; + ATH_UNLOCK(sc); + return (!! r); } /* @@ -4608,10 +4667,10 @@ ath_tx_proc_q0(void *arg, int npending) struct ath_softc *sc = arg; struct ifnet *ifp = sc->sc_ifp; - if (txqactive(sc->sc_ah, 0) && ath_tx_processq(sc, &sc->sc_txq[0])) + if (txqactive(sc, 0) && ath_tx_processq(sc, &sc->sc_txq[0])) /* XXX why is lastrx updated in tx code? */ sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); - if (txqactive(sc->sc_ah, sc->sc_cabq->axq_qnum)) + if (txqactive(sc, sc->sc_cabq->axq_qnum)) ath_tx_processq(sc, sc->sc_cabq); ifp->if_drv_flags &= ~IFF_DRV_OACTIVE; sc->sc_wd_timer = 0; @@ -4637,15 +4696,15 @@ ath_tx_proc_q0123(void *arg, int npendin * Process each active queue. */ nacked = 0; - if (txqactive(sc->sc_ah, 0)) + if (txqactive(sc, 0)) nacked += ath_tx_processq(sc, &sc->sc_txq[0]); - if (txqactive(sc->sc_ah, 1)) + if (txqactive(sc, 1)) nacked += ath_tx_processq(sc, &sc->sc_txq[1]); - if (txqactive(sc->sc_ah, 2)) + if (txqactive(sc, 2)) nacked += ath_tx_processq(sc, &sc->sc_txq[2]); - if (txqactive(sc->sc_ah, 3)) + if (txqactive(sc, 3)) nacked += ath_tx_processq(sc, &sc->sc_txq[3]); - if (txqactive(sc->sc_ah, sc->sc_cabq->axq_qnum)) + if (txqactive(sc, sc->sc_cabq->axq_qnum)) ath_tx_processq(sc, sc->sc_cabq); if (nacked) sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); @@ -4674,7 +4733,7 @@ ath_tx_proc(void *arg, int npending) */ nacked = 0; for (i = 0; i < HAL_NUM_TX_QUEUES; i++) - if (ATH_TXQ_SETUP(sc, i) && txqactive(sc->sc_ah, i)) + if (ATH_TXQ_SETUP(sc, i) && txqactive(sc, i)) nacked += ath_tx_processq(sc, &sc->sc_txq[i]); if (nacked) sc->sc_lastrx = ath_hal_gettsf64(sc->sc_ah); Modified: user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h ============================================================================== --- user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h Sun Oct 2 02:42:31 2011 (r225913) +++ user/adrian/if_ath_tx/sys/dev/ath/if_athvar.h Sun Oct 2 05:11:02 2011 (r225914) @@ -384,7 +384,6 @@ struct ath_softc { sc_setcca : 1,/* set/clr CCA with TDMA */ sc_resetcal : 1,/* reset cal state next trip */ sc_rxslink : 1,/* do self-linked final descriptor */ - sc_kickpcu : 1,/* kick PCU RX on next RX proc */ sc_rxtsf32 : 1;/* RX dec TSF is 32 bits */ uint32_t sc_eerd; /* regdomain from EEPROM */ uint32_t sc_eecc; /* country code from EEPROM */ @@ -411,7 +410,19 @@ struct ath_softc { u_int sc_fftxqmin; /* min frames before staging */ u_int sc_fftxqmax; /* max frames before drop */ u_int sc_txantenna; /* tx antenna (fixed or auto) */ + HAL_INT sc_imask; /* interrupt mask copy */ + /* + * These are modified in the interrupt handler as well as + * the task queues and other contexts. Thus these must be + * protected by a mutex, or they could clash. + * + * For now, access to these is behind the ATH_LOCK, + * just to save time. + */ + uint32_t sc_txq_active; /* bitmap of active TXQs */ + uint32_t sc_kickpcu; /* whether to kick the PCU */ + u_int sc_keymax; /* size of key cache */ u_int8_t sc_keymap[ATH_KEYBYTES];/* key use bit map */