Date: Sun, 2 Oct 2011 14:08:56 +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: r225925 - head/sys/dev/ath/ath_hal/ar5416 Message-ID: <201110021408.p92E8uQE066719@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: adrian Date: Sun Oct 2 14:08:56 2011 New Revision: 225925 URL: http://svn.freebsd.org/changeset/base/225925 Log: Various interrupt handling and RX interrupt mitigation fixes. * The AR_ISR_RAC interrupt processing method has a subtle bug in all the MAC revisions (including pre-11n NICs) until AR9300v2. If you're unlucky, the clear phase clears an update to one of the secondary registers, which includes TX status. This shows up as a "watchdog timeout" if you're doing very low levels of TX traffic. If you're doing a lot of non-11n TX traffic, you'll end up receiving a TX interrupt from some later traffic anyway. But when TX'ing 11n aggregation session traffic (which -HEAD isn't yet doing), you may find that you're only able to TX one frame (due to BAW restrictions) and this may end up hitting this race condition. The only solution is to not use RAC and instead use AR_ISR and the AR_ISR_Sx registers. The bit in AR_ISR which represents the secondary registers are not cleared; only the AR_ISR_Sx bits are. This way any updates which occur between the read and subsequent write will stay asserted and (correctly) trigger a subsequent interrupt. I've tested this on the AR5416, AR9160, AR9280. I will soon test the AR9285 and AR9287. * The AR_ISR TX and RX bits (and all others!) are set regardless of whether the contents of the AR_IMR register. So if RX mitigation is enabled, RXOK is going to be set in AR_ISR and it would normally set HAL_INT_RX. Fix the code to not set HAL_INT_RX when RXOK is set and RX mitigation is compiled in. That way the RX path isn't prematurely called. I would see: * An interrupt would come in (eg a beacon, or TX completion) where RXOK was set but RXINTM/RXMINT wasn't; * ath_rx_proc() be called - completing RX frames; * RXINTM/RXMINT would then fire; * ath_rx_proc() would then be called again but find no frames in the queue. This fixes the RX mitigation behaviour to not overly call ath_rx_proc(). * Start to flesh out more correct timer interrupt handling - it isn't kite/merlin specific. It's actually based on whether autosleep support is enabled or not. This is sourced from my 11n TX branch and has been tested for a few weeks. Finally, the interrupt handling change should likely be implemented for AR5210, AR5211 and AR5212. Modified: head/sys/dev/ath/ath_hal/ar5416/ar5416_interrupts.c head/sys/dev/ath/ath_hal/ar5416/ar5416reg.h Modified: head/sys/dev/ath/ath_hal/ar5416/ar5416_interrupts.c ============================================================================== --- head/sys/dev/ath/ath_hal/ar5416/ar5416_interrupts.c Sun Oct 2 13:51:26 2011 (r225924) +++ head/sys/dev/ath/ath_hal/ar5416/ar5416_interrupts.c Sun Oct 2 14:08:56 2011 (r225925) @@ -68,6 +68,7 @@ HAL_BOOL ar5416GetPendingInterrupts(struct ath_hal *ah, HAL_INT *masked) { uint32_t isr, isr0, isr1, sync_cause = 0; + HAL_CAPABILITIES *pCap = &AH_PRIVATE(ah)->ah_caps; /* * Verify there's a mac interrupt and the RTC is on. @@ -110,42 +111,95 @@ ar5416GetPendingInterrupts(struct ath_ha mask2 |= HAL_INT_CST; if (isr2 & AR_ISR_S2_TSFOOR) mask2 |= HAL_INT_TSFOOR; + + /* + * Don't mask out AR_BCNMISC; instead mask + * out what causes it. + */ + OS_REG_WRITE(ah, AR_ISR_S2, isr2); + isr &= ~AR_ISR_BCNMISC; } - isr = OS_REG_READ(ah, AR_ISR_RAC); if (isr == 0xffffffff) { *masked = 0; return AH_FALSE; } *masked = isr & HAL_INT_COMMON; + + if (isr & (AR_ISR_RXMINTR | AR_ISR_RXINTM)) + *masked |= HAL_INT_RX; + if (isr & (AR_ISR_TXMINTR | AR_ISR_TXINTM)) + *masked |= HAL_INT_TX; + + /* + * When doing RX interrupt mitigation, the RXOK bit is set + * in AR_ISR even if the relevant bit in AR_IMR is clear. + * Since this interrupt may be due to another source, don't + * just automatically set HAL_INT_RX if it's set, otherwise + * we could prematurely service the RX queue. + * + * In some cases, the driver can even handle all the RX + * frames just before the mitigation interrupt fires. + * The subsequent RX processing trip will then end up + * processing 0 frames. + */ +#ifdef AH_AR5416_INTERRUPT_MITIGATION + if (isr & AR_ISR_RXERR) + *masked |= HAL_INT_RX; +#else if (isr & (AR_ISR_RXOK | AR_ISR_RXERR)) *masked |= HAL_INT_RX; - if (isr & (AR_ISR_TXOK | AR_ISR_TXDESC | AR_ISR_TXERR | AR_ISR_TXEOL)) { +#endif + + if (isr & (AR_ISR_TXOK | AR_ISR_TXDESC | AR_ISR_TXERR | + AR_ISR_TXEOL)) { *masked |= HAL_INT_TX; - isr0 = OS_REG_READ(ah, AR_ISR_S0_S); + + isr0 = OS_REG_READ(ah, AR_ISR_S0); + OS_REG_WRITE(ah, AR_ISR_S0, isr0); + isr1 = OS_REG_READ(ah, AR_ISR_S1); + OS_REG_WRITE(ah, AR_ISR_S1, isr1); + + /* + * Don't clear the primary ISR TX bits, clear + * what causes them (S0/S1.) + */ + isr &= ~(AR_ISR_TXOK | AR_ISR_TXDESC | + AR_ISR_TXERR | AR_ISR_TXEOL); + ahp->ah_intrTxqs |= MS(isr0, AR_ISR_S0_QCU_TXOK); ahp->ah_intrTxqs |= MS(isr0, AR_ISR_S0_QCU_TXDESC); - isr1 = OS_REG_READ(ah, AR_ISR_S1_S); ahp->ah_intrTxqs |= MS(isr1, AR_ISR_S1_QCU_TXERR); ahp->ah_intrTxqs |= MS(isr1, AR_ISR_S1_QCU_TXEOL); } - if (AR_SREV_MERLIN(ah) || AR_SREV_KITE(ah)) { + if ((isr & AR_ISR_GENTMR) || (! pCap->halAutoSleepSupport)) { uint32_t isr5; - isr5 = OS_REG_READ(ah, AR_ISR_S5_S); - if (isr5 & AR_ISR_S5_TIM_TIMER) - *masked |= HAL_INT_TIM_TIMER; + isr5 = OS_REG_READ(ah, AR_ISR_S5); + OS_REG_WRITE(ah, AR_ISR_S5, isr5); + isr &= ~AR_ISR_GENTMR; + + if (! pCap->halAutoSleepSupport) + if (isr5 & AR_ISR_S5_TIM_TIMER) + *masked |= HAL_INT_TIM_TIMER; } - - /* Interrupt Mitigation on AR5416 */ -#ifdef AH_AR5416_INTERRUPT_MITIGATION - if (isr & (AR_ISR_RXMINTR | AR_ISR_RXINTM)) - *masked |= HAL_INT_RX; -#endif *masked |= mask2; } + /* + * Since we're not using AR_ISR_RAC, clear the status bits + * for handled interrupts here. For bits whose interrupt + * source is a secondary register, those bits should've been + * masked out - instead of those bits being written back, + * their source (ie, the secondary status registers) should + * be cleared. That way there are no race conditions with + * new triggers coming in whilst they've been read/cleared. + */ + OS_REG_WRITE(ah, AR_ISR, isr); + /* Flush previous write */ + OS_REG_READ(ah, AR_ISR); + if (AR_SREV_HOWL(ah)) return AH_TRUE; Modified: head/sys/dev/ath/ath_hal/ar5416/ar5416reg.h ============================================================================== --- head/sys/dev/ath/ath_hal/ar5416/ar5416reg.h Sun Oct 2 13:51:26 2011 (r225924) +++ head/sys/dev/ath/ath_hal/ar5416/ar5416reg.h Sun Oct 2 14:08:56 2011 (r225925) @@ -250,6 +250,7 @@ /* Interrupts */ #define AR_ISR_TXMINTR 0x00080000 /* Maximum interrupt tx rate */ #define AR_ISR_RXMINTR 0x01000000 /* Maximum interrupt rx rate */ +#define AR_ISR_GENTMR 0x10000000 /* OR of generic timer bits in S5 */ #define AR_ISR_TXINTM 0x40000000 /* Tx int after mitigation */ #define AR_ISR_RXINTM 0x80000000 /* Rx int after mitigation */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201110021408.p92E8uQE066719>