Date: Sat, 10 Sep 2011 20:24:28 +0800 From: Adrian Chadd <adrian@freebsd.org> To: freebsd-wireless@freebsd.org Subject: [patch] if_ath_tx: change interrupt scheduling deferral Message-ID: <CAJ-VmoknpF_RhiXWL07GSOqWoHpy3L5Ma4kCptHq9MtfbfPO8w@mail.gmail.com>
next in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
Hi,
This patch changes how the deferred interrupt handling works. It's
likely I'll have to change things a bit before I commit it so I won't
be (yet) committing this.
It brings the interrupt handling in line with how ath9k and the
reference driver works. It eliminates a possible race condition:
* ath_intr() and ath_hal_getisr() set AH5212(ah)->ah_intrTxqs, which
is a bitmap of TXQs which need servicing;
* The ath TX processes call txqactive() which check the above bitmap
and clear the bit that's been tested.
The interrupt handler ath_intr() can be called during the TX
completion task, so I think it's possible that the interrupt could
occur, setting a TXQ bit, in between txqactive()'s "check bit X" and
"clear bit X".
My testing was only showing up one queue hang every 20-50 million
packets where the TXQ had active packets in it which were completed,
but hadn't been processed.
I've only just begun testing this. I'll post updates as they're needed.
Thanks,
Adrian
[-- Attachment #2 --]
Index: if_athvar.h
===================================================================
--- if_athvar.h (revision 225467)
+++ if_athvar.h (working copy)
@@ -411,6 +411,7 @@
u_int sc_fftxqmax; /* max frames before drop */
u_int sc_txantenna; /* tx antenna (fixed or auto) */
HAL_INT sc_imask; /* interrupt mask copy */
+ HAL_INT sc_intrstatus; /* current interrupt status */
u_int sc_keymax; /* size of key cache */
u_int8_t sc_keymap[ATH_KEYBYTES];/* key use bit map */
@@ -429,7 +430,6 @@
ath_bufhead sc_rxbuf; /* receive buffer */
struct mbuf *sc_rxpending; /* pending receive data */
u_int32_t *sc_rxlink; /* link ptr in last RX desc */
- struct task sc_rxtask; /* rx int processing */
u_int8_t sc_defant; /* current default antenna */
u_int8_t sc_rxotherant; /* rx's on non-default antenna*/
u_int64_t sc_lastrx; /* tsf at last rx'd frame */
@@ -446,7 +446,6 @@
u_int sc_txintrperiod;/* tx interrupt batching */
struct ath_txq sc_txq[HAL_NUM_TX_QUEUES];
struct ath_txq *sc_ac2q[5]; /* WME AC -> h/w q map */
- struct task sc_txtask; /* tx int processing */
int sc_wd_timer; /* count down for wd timer */
struct callout sc_wd_ch; /* tx watchdog timer */
struct ath_tx_radiotap_header sc_tx_th;
@@ -460,6 +459,8 @@
struct ath_txq *sc_cabq; /* tx q for cab frames */
struct task sc_bmisstask; /* bmiss int processing */
struct task sc_bstucktask; /* stuck beacon processing */
+ struct task sc_intrtask; /* deferred interrupt processing */
+ struct task sc_fataltask; /* deferred reset processing */
enum {
OK, /* no change needed */
UPDATE, /* update pending */
Index: if_ath.c
===================================================================
--- if_ath.c (revision 225467)
+++ if_ath.c (working copy)
@@ -132,6 +132,7 @@
static void ath_watchdog(void *);
static int ath_ioctl(struct ifnet *, u_long, caddr_t);
static void ath_fatal_proc(void *, int);
+static void ath_handle_intr(void *, int);
static void ath_bmiss_vap(struct ieee80211vap *);
static void ath_bmiss_proc(void *, int);
static void ath_key_update_begin(struct ieee80211vap *);
@@ -173,8 +174,10 @@
static int ath_wme_update(struct ieee80211com *);
static void ath_tx_cleanupq(struct ath_softc *, struct ath_txq *);
static void ath_tx_cleanup(struct ath_softc *);
+#if 0
static void ath_tx_proc_q0(void *, int);
static void ath_tx_proc_q0123(void *, int);
+#endif
static void ath_tx_proc(void *, int);
static int ath_chan_set(struct ath_softc *, struct ieee80211_channel *);
static void ath_draintxq(struct ath_softc *);
@@ -395,9 +398,10 @@
taskqueue_start_threads(&sc->sc_tq, 1, PI_NET,
"%s taskq", ifp->if_xname);
- TASK_INIT(&sc->sc_rxtask, 0, ath_rx_proc, sc);
TASK_INIT(&sc->sc_bmisstask, 0, ath_bmiss_proc, sc);
TASK_INIT(&sc->sc_bstucktask,0, ath_bstuck_proc, sc);
+ TASK_INIT(&sc->sc_intrtask, 0, ath_handle_intr, sc);
+ TASK_INIT(&sc->sc_fataltask, 0, ath_fatal_proc, sc);
/*
* Allocate hardware transmit queues: one queue for
@@ -446,23 +450,6 @@
}
/*
- * Special case certain configurations. Note the
- * CAB queue is handled by these specially so don't
- * include them when checking the txq setup mask.
- */
- switch (sc->sc_txqsetup &~ (1<<sc->sc_cabq->axq_qnum)) {
- case 0x01:
- TASK_INIT(&sc->sc_txtask, 0, ath_tx_proc_q0, sc);
- break;
- case 0x0f:
- TASK_INIT(&sc->sc_txtask, 0, ath_tx_proc_q0123, sc);
- break;
- default:
- TASK_INIT(&sc->sc_txtask, 0, ath_tx_proc, sc);
- break;
- }
-
- /*
* Setup rate control. Some rate control modules
* call back to change the anntena state so expose
* the necessary entry points.
@@ -1345,6 +1332,63 @@
}
/*
+ * Do deferred interrupt processing; then re-enable interrupts
+ * if required.
+ */
+static void
+ath_handle_intr(void *arg, int npending)
+{
+ struct ath_softc *sc = (struct ath_softc *) arg;
+ HAL_INT status;
+ struct ath_hal *ah = sc->sc_ah;
+ struct ifnet *ifp = sc->sc_ifp;
+
+ if (sc->sc_invalid) {
+ /*
+ * The hardware is not ready/present, don't touch anything.
+ * Note this can happen early on if the IRQ is shared.
+ */
+ DPRINTF(sc, ATH_DEBUG_ANY, "%s: invalid; ignored\n", __func__);
+ return;
+ }
+
+ /*
+ * Fetch the current interrupt status from the interrupt
+ * handler. It's assumed that any interrupt which would lead
+ * us here won't happen until the interrupt is cleared.
+ *
+ * XXX Thus I'm not using a lock just yet.
+ */
+ status = sc->sc_intrstatus;
+
+ if (status & HAL_INT_FATAL) {
+ ath_hal_intrset(ah, 0); /* disable intr's until reset */
+ ath_fatal_proc(sc, 0);
+ } else {
+ if (status & (HAL_INT_RXEOL | HAL_INT_RX)) {
+ ath_rx_proc(sc, 1);
+ /* XXX this may reset the hardware? Should handle that! */
+ }
+ if (status & HAL_INT_TXURN) {
+ /* bump tx trigger level */
+ ath_hal_updatetxtriglevel(ah, AH_TRUE);
+ }
+ if (status & HAL_INT_TX) {
+ ath_tx_proc(sc, 1);
+ }
+ /*
+ * If TX or RX occured, call ath_start() if the interface
+ * can grab some packets.
+ */
+ if (!IFQ_IS_EMPTY(&ifp->if_snd))
+ ath_start(ifp);
+ }
+
+ /* re-enable interrupts */
+ ath_hal_intrset(sc->sc_ah, sc->sc_imask);
+}
+
+/*
* Interrupt handler. Most of the actual processing is deferred.
*/
void
@@ -1354,6 +1398,7 @@
struct ifnet *ifp = sc->sc_ifp;
struct ath_hal *ah = sc->sc_ah;
HAL_INT status = 0;
+ int sched = 0;
if (sc->sc_invalid) {
/*
@@ -1389,10 +1434,12 @@
if (status == 0x0)
return;
+ /* XXX locking? */
+ sc->sc_intrstatus = status;
+
if (status & HAL_INT_FATAL) {
sc->sc_stats.ast_hardware++;
- ath_hal_intrset(ah, 0); /* disable intr's until reset */
- ath_fatal_proc(sc, 0);
+ sched = 1;
} else {
if (status & HAL_INT_SWBA) {
/*
@@ -1422,12 +1469,12 @@
* traffic so any frames held on the staging
* queue are aged and potentially flushed.
*/
+ /* XXX there's no longer an rxtask? How to force? */
taskqueue_enqueue(sc->sc_tq, &sc->sc_rxtask);
#endif
}
}
if (status & HAL_INT_RXEOL) {
- int imask = sc->sc_imask;
/*
* NB: the hardware should re-read the link when
* RXE bit is written, but it doesn't work at
@@ -1443,29 +1490,23 @@
* by a call to ath_reset() somehow, the
* interrupt mask will be correctly reprogrammed.
*/
- imask &= ~(HAL_INT_RXEOL | HAL_INT_RXORN);
- ath_hal_intrset(ah, imask);
- /*
- * Enqueue an RX proc, to handled whatever
- * is in the RX queue.
- * This will then kick the PCU.
- */
- taskqueue_enqueue(sc->sc_tq, &sc->sc_rxtask);
+ /* XXX this is already reset in the sched call below */
sc->sc_rxlink = NULL;
sc->sc_kickpcu = 1;
+ sched = 1;
}
if (status & HAL_INT_TXURN) {
sc->sc_stats.ast_txurn++;
/* bump tx trigger level */
- ath_hal_updatetxtriglevel(ah, AH_TRUE);
+ sched = 1;
}
if (status & HAL_INT_RX) {
sc->sc_stats.ast_rx_intr++;
- taskqueue_enqueue(sc->sc_tq, &sc->sc_rxtask);
+ sched = 1;
}
if (status & HAL_INT_TX) {
sc->sc_stats.ast_tx_intr++;
- taskqueue_enqueue(sc->sc_tq, &sc->sc_txtask);
+ sched = 1;
}
if (status & HAL_INT_BMISS) {
sc->sc_stats.ast_bmiss++;
@@ -1494,6 +1535,18 @@
sc->sc_stats.ast_rxorn++;
}
}
+
+ /*
+ * If any of the above checks require an ISR schedule,
+ * enqueue the task and disable interrupts.
+ *
+ * Since beacon handling is done in interrupt context at the
+ * moment, always leave that enabled.
+ */
+ if (sched == 1) {
+ ath_hal_intrset(ah, (sc->sc_imask & HAL_INT_SWBA));
+ taskqueue_enqueue(sc->sc_tq, &sc->sc_intrtask);
+ }
}
static void
@@ -1826,6 +1879,9 @@
}
ath_hal_intrset(ah, sc->sc_imask);
+ /*
+ * XXX should this be done in-line?
+ */
ath_start(ifp); /* restart xmit */
return 0;
}
@@ -3950,23 +4006,20 @@
* XXX kick the PCU again to continue RXing?
*/
ath_stoprecv(sc);
- sc->sc_imask |= (HAL_INT_RXEOL | HAL_INT_RXORN);
if (ath_startrecv(sc) != 0) {
if_printf(ifp,
"%s: couldn't restart RX after RXEOL; resetting\n",
__func__);
+ /* XXX this must be scheduled! */
ath_reset(ifp);
return;
}
- ath_hal_intrset(ah, sc->sc_imask);
}
if ((ifp->if_drv_flags & IFF_DRV_OACTIVE) == 0) {
#ifdef IEEE80211_SUPPORT_SUPERG
ieee80211_ff_age_all(ic, 100);
#endif
- if (!IFQ_IS_EMPTY(&ifp->if_snd))
- ath_start(ifp);
}
#undef PA2DESC
}
@@ -4444,6 +4497,7 @@
return (txqs & (1<<qnum));
}
+#if 0
/*
* Deferred processing of transmit interrupt; special-cased
* for a single hardware transmit queue (e.g. 5210 and 5211).
@@ -4504,6 +4558,7 @@
ath_start(ifp);
}
+#endif
/*
* Deferred processing of transmit interrupt.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmoknpF_RhiXWL07GSOqWoHpy3L5Ma4kCptHq9MtfbfPO8w>