* [PATCH 1/4] ath9k_hw: fix REG_SET_BIT and REG_CLR_BIT for multiple bits @ 2011-03-10 0:35 Felix Fietkau 2011-03-10 0:35 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Felix Fietkau 0 siblings, 1 reply; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 0:35 UTC (permalink / raw) To: linux-wireless; +Cc: linville, lrodriguez Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/wireless/ath/ath9k/hw.h | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/hw.h b/drivers/net/wireless/ath/ath9k/hw.h index ef79f4c..6650fd4 100644 --- a/drivers/net/wireless/ath/ath9k/hw.h +++ b/drivers/net/wireless/ath/ath9k/hw.h @@ -95,9 +95,9 @@ #define REG_READ_FIELD(_a, _r, _f) \ (((REG_READ(_a, _r) & _f) >> _f##_S)) #define REG_SET_BIT(_a, _r, _f) \ - REG_WRITE(_a, _r, REG_READ(_a, _r) | _f) + REG_WRITE(_a, _r, REG_READ(_a, _r) | (_f)) #define REG_CLR_BIT(_a, _r, _f) \ - REG_WRITE(_a, _r, REG_READ(_a, _r) & ~_f) + REG_WRITE(_a, _r, REG_READ(_a, _r) & ~(_f)) #define DO_DELAY(x) do { \ if ((++(x) % 64) == 0) \ -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/4] ath9k: fix stopping tx dma on reset 2011-03-10 0:35 [PATCH 1/4] ath9k_hw: fix REG_SET_BIT and REG_CLR_BIT for multiple bits Felix Fietkau @ 2011-03-10 0:35 ` Felix Fietkau 2011-03-10 0:35 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Felix Fietkau 2011-03-10 8:58 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Vasanthakumar Thiagarajan 0 siblings, 2 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 0:35 UTC (permalink / raw) To: linux-wireless; +Cc: linville, lrodriguez In some situations, stopping Tx DMA frequently fails, leading to messages like this: ath: Failed to stop TX DMA in 100 msec after killing last frame ath: Failed to stop TX DMA! This patch uses a few MAC features to abort DMA globally instead of iterating over all hardware queues and attempting to stop them individually. Not only is that faster and works with a shorter timeout, it also makes the process much more reliable. With this change, I can no longer trigger these messages on AR9380, and on AR9280 they become much more rare. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/wireless/ath/ath9k/mac.c | 31 +++++++++++++++++++++++++++++++ drivers/net/wireless/ath/ath9k/mac.h | 1 + drivers/net/wireless/ath/ath9k/xmit.c | 14 ++++++-------- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c index 5efc869..aa9f646 100644 --- a/drivers/net/wireless/ath/ath9k/mac.c +++ b/drivers/net/wireless/ath/ath9k/mac.c @@ -143,6 +143,37 @@ bool ath9k_hw_updatetxtriglevel(struct ath_hw *ah, bool bIncTrigLevel) } EXPORT_SYMBOL(ath9k_hw_updatetxtriglevel); +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) +{ + int i, q; + + REG_WRITE(ah, AR_Q_TXD, AR_Q_TXD_M); + + REG_SET_BIT(ah, AR_PCU_MISC, AR_PCU_FORCE_QUIET_COLL | AR_PCU_CLEAR_VMF); + REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); + REG_SET_BIT(ah, AR_D_GBL_IFS_MISC, AR_D_GBL_IFS_MISC_IGNORE_BACKOFF); + + for (q = 0; q < AR_NUM_QCU; q++) { + for (i = 1000; i > 0; i--) { + if (!ath9k_hw_numtxpending(ah, q)) + break; + + udelay(5); + } + } + if (!i) + return false; + + REG_CLR_BIT(ah, AR_PCU_MISC, AR_PCU_FORCE_QUIET_COLL | AR_PCU_CLEAR_VMF); + REG_CLR_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); + REG_CLR_BIT(ah, AR_D_GBL_IFS_MISC, AR_D_GBL_IFS_MISC_IGNORE_BACKOFF); + + REG_WRITE(ah, AR_Q_TXD, 0); + + return true; +} +EXPORT_SYMBOL(ath9k_hw_abort_tx_dma); + bool ath9k_hw_stoptxdma(struct ath_hw *ah, u32 q) { #define ATH9K_TX_STOP_DMA_TIMEOUT 4000 /* usec */ diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h index 04d58ae..347ee5f 100644 --- a/drivers/net/wireless/ath/ath9k/mac.h +++ b/drivers/net/wireless/ath/ath9k/mac.h @@ -677,6 +677,7 @@ void ath9k_hw_cleartxdesc(struct ath_hw *ah, void *ds); u32 ath9k_hw_numtxpending(struct ath_hw *ah, u32 q); bool ath9k_hw_updatetxtriglevel(struct ath_hw *ah, bool bIncTrigLevel); bool ath9k_hw_stoptxdma(struct ath_hw *ah, u32 q); +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah); void ath9k_hw_gettxintrtxqs(struct ath_hw *ah, u32 *txqs); bool ath9k_hw_set_txq_props(struct ath_hw *ah, int q, const struct ath9k_tx_queue_info *qinfo); diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c index e16136d..bb1d29e 100644 --- a/drivers/net/wireless/ath/ath9k/xmit.c +++ b/drivers/net/wireless/ath/ath9k/xmit.c @@ -1194,16 +1194,14 @@ bool ath_drain_all_txq(struct ath_softc *sc, bool retry_tx) if (sc->sc_flags & SC_OP_INVALID) return true; - /* Stop beacon queue */ - ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq); + ath9k_hw_abort_tx_dma(ah); - /* Stop data queues */ + /* Check if any queue remains active */ for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { - if (ATH_TXQ_SETUP(sc, i)) { - txq = &sc->tx.txq[i]; - ath9k_hw_stoptxdma(ah, txq->axq_qnum); - npend += ath9k_hw_numtxpending(ah, txq->axq_qnum); - } + if (!ATH_TXQ_SETUP(sc, i)) + continue; + + npend += ath9k_hw_numtxpending(ah, sc->tx.txq[i].axq_qnum); } if (npend) -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/4] ath9k: fix the .flush driver op implementation 2011-03-10 0:35 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Felix Fietkau @ 2011-03-10 0:35 ` Felix Fietkau 2011-03-10 0:35 ` [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling Felix Fietkau 2011-03-10 9:25 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Vasanthakumar Thiagarajan 2011-03-10 8:58 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Vasanthakumar Thiagarajan 1 sibling, 2 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 0:35 UTC (permalink / raw) To: linux-wireless; +Cc: linville, lrodriguez ath9k has a timeout of 60ms for the flush, but instead waiting 60 ms for all tx activity to finish, it resets it for every single queue. This patch simplifies the flush op and reuses ath_drain_all_txq for flushing out pending frames if necessary. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/wireless/ath/ath9k/main.c | 55 +++++++++++--------------------- 1 files changed, 19 insertions(+), 36 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c index 2e228aa..0e9f748 100644 --- a/drivers/net/wireless/ath/ath9k/main.c +++ b/drivers/net/wireless/ath/ath9k/main.c @@ -2128,54 +2128,37 @@ static void ath9k_set_coverage_class(struct ieee80211_hw *hw, u8 coverage_class) static void ath9k_flush(struct ieee80211_hw *hw, bool drop) { -#define ATH_FLUSH_TIMEOUT 60 /* ms */ struct ath_softc *sc = hw->priv; - struct ath_txq *txq = NULL; - struct ath_hw *ah = sc->sc_ah; - struct ath_common *common = ath9k_hw_common(ah); - int i, j, npend = 0; + int timeout = 60; /* ms */ + int i, j; mutex_lock(&sc->mutex); cancel_delayed_work_sync(&sc->tx_complete_work); - for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { - if (!ATH_TXQ_SETUP(sc, i)) - continue; - txq = &sc->tx.txq[i]; + if (drop) + timeout = 1; - if (!drop) { - for (j = 0; j < ATH_FLUSH_TIMEOUT; j++) { - if (!ath9k_has_pending_frames(sc, txq)) - break; - usleep_range(1000, 2000); - } - } + for (j = 0; j < timeout; j++) { + int npend = 0; + for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { + if (!ATH_TXQ_SETUP(sc, i)) + continue; - if (drop || ath9k_has_pending_frames(sc, txq)) { - ath_dbg(common, ATH_DBG_QUEUE, "Drop frames from hw queue:%d\n", - txq->axq_qnum); - spin_lock_bh(&txq->axq_lock); - txq->txq_flush_inprogress = true; - spin_unlock_bh(&txq->axq_lock); - - ath9k_ps_wakeup(sc); - ath9k_hw_stoptxdma(ah, txq->axq_qnum); - npend = ath9k_hw_numtxpending(ah, txq->axq_qnum); - ath9k_ps_restore(sc); - if (npend) - break; - - ath_draintxq(sc, txq, false); - txq->txq_flush_inprogress = false; + npend += ath9k_has_pending_frames(sc, &sc->tx.txq[i]); } - } - if (npend) { - ath_reset(sc, false); - txq->txq_flush_inprogress = false; + if (!npend) + goto out; + + usleep_range(1000, 2000); } + ath9k_ps_wakeup(sc); + ath_drain_all_txq(sc, false); + ath9k_ps_restore(sc); + +out: ieee80211_queue_delayed_work(hw, &sc->tx_complete_work, 0); mutex_unlock(&sc->mutex); } -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling 2011-03-10 0:35 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Felix Fietkau @ 2011-03-10 0:35 ` Felix Fietkau 2011-03-10 9:46 ` Vasanthakumar Thiagarajan 2011-03-10 9:25 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Vasanthakumar Thiagarajan 1 sibling, 1 reply; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 0:35 UTC (permalink / raw) To: linux-wireless; +Cc: linville, lrodriguez ath9k calls ath9k_hw_stoptxdma every time it sends a beacon, however there is not much point in doing that if the previous beacon and mcast traffic went out properly. On AR9380, calling that function too often can result in an increase of stuck beacons due to differences in the handling of the queue enable/disable functionality. With this patch, the queue will only be explicitly stopped if the previous data frames were not sent successfully. With the beacon code being the only remaining user of ath9k_hw_stoptxdma, this function can be simplified in order to remove the now pointless attempts at waiting for transmission completion, which would never happen at this point due to the different method of tx scheduling of the beacon queue. Signed-off-by: Felix Fietkau <nbd@openwrt.org> --- drivers/net/wireless/ath/ath9k/beacon.c | 13 +----- drivers/net/wireless/ath/ath9k/mac.c | 68 +++--------------------------- drivers/net/wireless/ath/ath9k/mac.h | 2 +- 3 files changed, 10 insertions(+), 73 deletions(-) diff --git a/drivers/net/wireless/ath/ath9k/beacon.c b/drivers/net/wireless/ath/ath9k/beacon.c index a4bdfdb..6d2a545f 100644 --- a/drivers/net/wireless/ath/ath9k/beacon.c +++ b/drivers/net/wireless/ath/ath9k/beacon.c @@ -373,6 +373,7 @@ void ath_beacon_tasklet(unsigned long data) ath_dbg(common, ATH_DBG_BSTUCK, "missed %u consecutive beacons\n", sc->beacon.bmisscnt); + ath9k_hw_stop_dma_queue(ah, sc->beacon.beaconq); ath9k_hw_bstuck_nfcal(ah); } else if (sc->beacon.bmisscnt >= BSTUCK_THRESH) { ath_dbg(common, ATH_DBG_BSTUCK, @@ -450,16 +451,6 @@ void ath_beacon_tasklet(unsigned long data) sc->beacon.updateslot = OK; } if (bfaddr != 0) { - /* - * Stop any current dma and put the new frame(s) on the queue. - * This should never fail since we check above that no frames - * are still pending on the queue. - */ - if (!ath9k_hw_stoptxdma(ah, sc->beacon.beaconq)) { - ath_err(common, "beacon queue %u did not stop?\n", - sc->beacon.beaconq); - } - /* NB: cabq traffic should already be queued and primed */ ath9k_hw_puttxbuf(ah, sc->beacon.beaconq, bfaddr); ath9k_hw_txstart(ah, sc->beacon.beaconq); @@ -780,7 +771,7 @@ void ath9k_set_beaconing_status(struct ath_softc *sc, bool status) ah->imask &= ~ATH9K_INT_SWBA; ath9k_hw_set_interrupts(ah, ah->imask); tasklet_kill(&sc->bcon_tasklet); - ath9k_hw_stoptxdma(ah, sc->beacon.beaconq); + ath9k_hw_stop_dma_queue(ah, sc->beacon.beaconq); } ath9k_ps_restore(sc); } diff --git a/drivers/net/wireless/ath/ath9k/mac.c b/drivers/net/wireless/ath/ath9k/mac.c index aa9f646..40373c5 100644 --- a/drivers/net/wireless/ath/ath9k/mac.c +++ b/drivers/net/wireless/ath/ath9k/mac.c @@ -174,84 +174,30 @@ bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) } EXPORT_SYMBOL(ath9k_hw_abort_tx_dma); -bool ath9k_hw_stoptxdma(struct ath_hw *ah, u32 q) +bool ath9k_hw_stop_dma_queue(struct ath_hw *ah, u32 q) { -#define ATH9K_TX_STOP_DMA_TIMEOUT 4000 /* usec */ +#define ATH9K_TX_STOP_DMA_TIMEOUT 1000 /* usec */ #define ATH9K_TIME_QUANTUM 100 /* usec */ - struct ath_common *common = ath9k_hw_common(ah); - struct ath9k_hw_capabilities *pCap = &ah->caps; - struct ath9k_tx_queue_info *qi; - u32 tsfLow, j, wait; - u32 wait_time = ATH9K_TX_STOP_DMA_TIMEOUT / ATH9K_TIME_QUANTUM; - - if (q >= pCap->total_queues) { - ath_dbg(common, ATH_DBG_QUEUE, - "Stopping TX DMA, invalid queue: %u\n", q); - return false; - } - - qi = &ah->txq[q]; - if (qi->tqi_type == ATH9K_TX_QUEUE_INACTIVE) { - ath_dbg(common, ATH_DBG_QUEUE, - "Stopping TX DMA, inactive queue: %u\n", q); - return false; - } + int wait_time = ATH9K_TX_STOP_DMA_TIMEOUT / ATH9K_TIME_QUANTUM; + int wait; REG_WRITE(ah, AR_Q_TXD, 1 << q); for (wait = wait_time; wait != 0; wait--) { if (ath9k_hw_numtxpending(ah, q) == 0) break; - udelay(ATH9K_TIME_QUANTUM); - } - - if (ath9k_hw_numtxpending(ah, q)) { - ath_dbg(common, ATH_DBG_QUEUE, - "%s: Num of pending TX Frames %d on Q %d\n", - __func__, ath9k_hw_numtxpending(ah, q), q); - - for (j = 0; j < 2; j++) { - tsfLow = REG_READ(ah, AR_TSF_L32); - REG_WRITE(ah, AR_QUIET2, - SM(10, AR_QUIET2_QUIET_DUR)); - REG_WRITE(ah, AR_QUIET_PERIOD, 100); - REG_WRITE(ah, AR_NEXT_QUIET_TIMER, tsfLow >> 10); - REG_SET_BIT(ah, AR_TIMER_MODE, - AR_QUIET_TIMER_EN); - - if ((REG_READ(ah, AR_TSF_L32) >> 10) == (tsfLow >> 10)) - break; - - ath_dbg(common, ATH_DBG_QUEUE, - "TSF has moved while trying to set quiet time TSF: 0x%08x\n", - tsfLow); - } - - REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); - udelay(200); - REG_CLR_BIT(ah, AR_TIMER_MODE, AR_QUIET_TIMER_EN); - - wait = wait_time; - while (ath9k_hw_numtxpending(ah, q)) { - if ((--wait) == 0) { - ath_err(common, - "Failed to stop TX DMA in 100 msec after killing last frame\n"); - break; - } - udelay(ATH9K_TIME_QUANTUM); - } - - REG_CLR_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); + udelay(ATH9K_TIME_QUANTUM); } REG_WRITE(ah, AR_Q_TXD, 0); + return wait != 0; #undef ATH9K_TX_STOP_DMA_TIMEOUT #undef ATH9K_TIME_QUANTUM } -EXPORT_SYMBOL(ath9k_hw_stoptxdma); +EXPORT_SYMBOL(ath9k_hw_stop_dma_queue); void ath9k_hw_gettxintrtxqs(struct ath_hw *ah, u32 *txqs) { diff --git a/drivers/net/wireless/ath/ath9k/mac.h b/drivers/net/wireless/ath/ath9k/mac.h index 347ee5f..bf8944a 100644 --- a/drivers/net/wireless/ath/ath9k/mac.h +++ b/drivers/net/wireless/ath/ath9k/mac.h @@ -676,7 +676,7 @@ void ath9k_hw_txstart(struct ath_hw *ah, u32 q); void ath9k_hw_cleartxdesc(struct ath_hw *ah, void *ds); u32 ath9k_hw_numtxpending(struct ath_hw *ah, u32 q); bool ath9k_hw_updatetxtriglevel(struct ath_hw *ah, bool bIncTrigLevel); -bool ath9k_hw_stoptxdma(struct ath_hw *ah, u32 q); +bool ath9k_hw_stop_dma_queue(struct ath_hw *ah, u32 q); bool ath9k_hw_abort_tx_dma(struct ath_hw *ah); void ath9k_hw_gettxintrtxqs(struct ath_hw *ah, u32 *txqs); bool ath9k_hw_set_txq_props(struct ath_hw *ah, int q, -- 1.7.3.2 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling 2011-03-10 0:35 ` [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling Felix Fietkau @ 2011-03-10 9:46 ` Vasanthakumar Thiagarajan 2011-03-10 13:04 ` Felix Fietkau 0 siblings, 1 reply; 14+ messages in thread From: Vasanthakumar Thiagarajan @ 2011-03-10 9:46 UTC (permalink / raw) To: Felix Fietkau Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On Thu, Mar 10, 2011 at 06:05:03AM +0530, Felix Fietkau wrote: > ath9k calls ath9k_hw_stoptxdma every time it sends a beacon, however there > is not much point in doing that if the previous beacon and mcast traffic > went out properly. On AR9380, calling that function too often can result > in an increase of stuck beacons due to differences in the handling of the > queue enable/disable functionality. > > With this patch, the queue will only be explicitly stopped if the previous > data frames were not sent successfully. With the beacon code being the > only remaining user of ath9k_hw_stoptxdma, this function can be simplified > in order to remove the now pointless attempts at waiting for transmission > completion, which would never happen at this point due to the different > method of tx scheduling of the beacon queue. Thanks. Can this patch be split into two, one which possibly fixes btsuck (a stable fix) and the other one which is a cleanup?. Vasanth ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling 2011-03-10 9:46 ` Vasanthakumar Thiagarajan @ 2011-03-10 13:04 ` Felix Fietkau 2011-03-10 13:15 ` Felix Fietkau 0 siblings, 1 reply; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 13:04 UTC (permalink / raw) To: Vasanthakumar Thiagarajan Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On 2011-03-10 10:46 AM, Vasanthakumar Thiagarajan wrote: > On Thu, Mar 10, 2011 at 06:05:03AM +0530, Felix Fietkau wrote: >> ath9k calls ath9k_hw_stoptxdma every time it sends a beacon, however there >> is not much point in doing that if the previous beacon and mcast traffic >> went out properly. On AR9380, calling that function too often can result >> in an increase of stuck beacons due to differences in the handling of the >> queue enable/disable functionality. >> >> With this patch, the queue will only be explicitly stopped if the previous >> data frames were not sent successfully. With the beacon code being the >> only remaining user of ath9k_hw_stoptxdma, this function can be simplified >> in order to remove the now pointless attempts at waiting for transmission >> completion, which would never happen at this point due to the different >> method of tx scheduling of the beacon queue. > > Thanks. Can this patch be split into two, one which possibly fixes btsuck > (a stable fix) and the other one which is a cleanup?. OK. I'll split it up, but won't propose it for stable yet, I want to see how well it works for users first. Thanks for the review of those patches. - Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling 2011-03-10 13:04 ` Felix Fietkau @ 2011-03-10 13:15 ` Felix Fietkau 0 siblings, 0 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 13:15 UTC (permalink / raw) To: Vasanthakumar Thiagarajan Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On 2011-03-10 2:04 PM, Felix Fietkau wrote: > On 2011-03-10 10:46 AM, Vasanthakumar Thiagarajan wrote: >> On Thu, Mar 10, 2011 at 06:05:03AM +0530, Felix Fietkau wrote: >>> ath9k calls ath9k_hw_stoptxdma every time it sends a beacon, however there >>> is not much point in doing that if the previous beacon and mcast traffic >>> went out properly. On AR9380, calling that function too often can result >>> in an increase of stuck beacons due to differences in the handling of the >>> queue enable/disable functionality. >>> >>> With this patch, the queue will only be explicitly stopped if the previous >>> data frames were not sent successfully. With the beacon code being the >>> only remaining user of ath9k_hw_stoptxdma, this function can be simplified >>> in order to remove the now pointless attempts at waiting for transmission >>> completion, which would never happen at this point due to the different >>> method of tx scheduling of the beacon queue. >> >> Thanks. Can this patch be split into two, one which possibly fixes btsuck >> (a stable fix) and the other one which is a cleanup?. > OK. I'll split it up, but won't propose it for stable yet, I want to see > how well it works for users first. Actually, now that I think about it, it can't easily be split up without introducing potential side-effects. The unmodified ath9k_hw_stoptxdma should not be called when a beacon is stuck, because then it will hit the code path that tries to kill pending frames by setting various flags and waiting for a while, which is absolutely counterproductive when being done to the beacon queue. I think it's better to leave the patch as a whole, the changes to ath9k_hw_stoptxdma do not fix anything without the changes to beacon.c. - Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] ath9k: fix the .flush driver op implementation 2011-03-10 0:35 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Felix Fietkau 2011-03-10 0:35 ` [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling Felix Fietkau @ 2011-03-10 9:25 ` Vasanthakumar Thiagarajan 2011-03-10 13:03 ` Felix Fietkau 1 sibling, 1 reply; 14+ messages in thread From: Vasanthakumar Thiagarajan @ 2011-03-10 9:25 UTC (permalink / raw) To: Felix Fietkau Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On Thu, Mar 10, 2011 at 06:05:02AM +0530, Felix Fietkau wrote: > ath9k has a timeout of 60ms for the flush, but instead waiting 60 ms for > all tx activity to finish, it resets it for every single queue. No, it does not do reset for every queue. > + for (j = 0; j < timeout; j++) { > + int npend = 0; > + for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { > + if (!ATH_TXQ_SETUP(sc, i)) > + continue; > > - if (drop || ath9k_has_pending_frames(sc, txq)) { > - ath_dbg(common, ATH_DBG_QUEUE, "Drop frames from hw queue:%d\n", > - txq->axq_qnum); > - spin_lock_bh(&txq->axq_lock); > - txq->txq_flush_inprogress = true; > - spin_unlock_bh(&txq->axq_lock); > - > - ath9k_ps_wakeup(sc); > - ath9k_hw_stoptxdma(ah, txq->axq_qnum); > - npend = ath9k_hw_numtxpending(ah, txq->axq_qnum); > - ath9k_ps_restore(sc); > - if (npend) > - break; > - > - ath_draintxq(sc, txq, false); > - txq->txq_flush_inprogress = false; > + npend += ath9k_has_pending_frames(sc, &sc->tx.txq[i]); > } > - } > > - if (npend) { > - ath_reset(sc, false); > - txq->txq_flush_inprogress = false; > + if (!npend) > + goto out; > + > + usleep_range(1000, 2000); Existing flush gives about 60ms for every q, that is reasonable time, but this 60ms for all the queues is too small. This flush is also called before sending null func in PS, so reset after hw queue stuck is necessary to find the hang early on. The usage of txq_flush_inprogress needs to be removed in other places in xmit. Vasanth ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] ath9k: fix the .flush driver op implementation 2011-03-10 9:25 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Vasanthakumar Thiagarajan @ 2011-03-10 13:03 ` Felix Fietkau 0 siblings, 0 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 13:03 UTC (permalink / raw) To: Vasanthakumar Thiagarajan Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On 2011-03-10 10:25 AM, Vasanthakumar Thiagarajan wrote: > On Thu, Mar 10, 2011 at 06:05:02AM +0530, Felix Fietkau wrote: >> ath9k has a timeout of 60ms for the flush, but instead waiting 60 ms for >> all tx activity to finish, it resets it for every single queue. > > No, it does not do reset for every queue. You're right, I got confused by the diff and my recollection of an old version of the flush op ;) >> + for (j = 0; j < timeout; j++) { >> + int npend = 0; >> + for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { >> + if (!ATH_TXQ_SETUP(sc, i)) >> + continue; >> >> - if (drop || ath9k_has_pending_frames(sc, txq)) { >> - ath_dbg(common, ATH_DBG_QUEUE, "Drop frames from hw queue:%d\n", >> - txq->axq_qnum); >> - spin_lock_bh(&txq->axq_lock); >> - txq->txq_flush_inprogress = true; >> - spin_unlock_bh(&txq->axq_lock); >> - >> - ath9k_ps_wakeup(sc); >> - ath9k_hw_stoptxdma(ah, txq->axq_qnum); >> - npend = ath9k_hw_numtxpending(ah, txq->axq_qnum); >> - ath9k_ps_restore(sc); >> - if (npend) >> - break; >> - >> - ath_draintxq(sc, txq, false); >> - txq->txq_flush_inprogress = false; >> + npend += ath9k_has_pending_frames(sc, &sc->tx.txq[i]); >> } >> - } >> >> - if (npend) { >> - ath_reset(sc, false); >> - txq->txq_flush_inprogress = false; >> + if (!npend) >> + goto out; >> + >> + usleep_range(1000, 2000); > > Existing flush gives about 60ms for every q, that is reasonable > time, but this 60ms for all the queues is too small. This flush > is also called before sending null func in PS, so reset after > hw queue stuck is necessary to find the hang early on. So maybe we should set it to something like 200ms for all? Anything more than that seems a bit excessive. I intend to add some more restrictions for the queue lengths soon, then we can reduce this timeout even further. > The usage of txq_flush_inprogress needs to be removed in other > places in xmit. OK - Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset 2011-03-10 0:35 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Felix Fietkau 2011-03-10 0:35 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Felix Fietkau @ 2011-03-10 8:58 ` Vasanthakumar Thiagarajan 2011-03-10 9:32 ` Vasanthakumar Thiagarajan 2011-03-10 12:47 ` Felix Fietkau 1 sibling, 2 replies; 14+ messages in thread From: Vasanthakumar Thiagarajan @ 2011-03-10 8:58 UTC (permalink / raw) To: Felix Fietkau Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On Thu, Mar 10, 2011 at 06:05:01AM +0530, Felix Fietkau wrote: > +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) > +{ > + int i, q; > + > + REG_WRITE(ah, AR_Q_TXD, AR_Q_TXD_M); > + > + REG_SET_BIT(ah, AR_PCU_MISC, AR_PCU_FORCE_QUIET_COLL | AR_PCU_CLEAR_VMF); > + REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); > + REG_SET_BIT(ah, AR_D_GBL_IFS_MISC, AR_D_GBL_IFS_MISC_IGNORE_BACKOFF); > + > + for (q = 0; q < AR_NUM_QCU; q++) { > + for (i = 1000; i > 0; i--) { > + if (!ath9k_hw_numtxpending(ah, q)) > + break; > + > + udelay(5); > + } > + } > + if (!i) > + return false; Here the assumption looks like a reset would follow to configure those registers back, may be some comment will be useful. > diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c > index e16136d..bb1d29e 100644 > --- a/drivers/net/wireless/ath/ath9k/xmit.c > +++ b/drivers/net/wireless/ath/ath9k/xmit.c > @@ -1194,16 +1194,14 @@ bool ath_drain_all_txq(struct ath_softc *sc, bool retry_tx) > if (sc->sc_flags & SC_OP_INVALID) > return true; > > - /* Stop beacon queue */ > - ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq); > + ath9k_hw_abort_tx_dma(ah); > > - /* Stop data queues */ > + /* Check if any queue remains active */ > for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { > - if (ATH_TXQ_SETUP(sc, i)) { > - txq = &sc->tx.txq[i]; > - ath9k_hw_stoptxdma(ah, txq->axq_qnum); > - npend += ath9k_hw_numtxpending(ah, txq->axq_qnum); > - } > + if (!ATH_TXQ_SETUP(sc, i)) > + continue; > + > + npend += ath9k_hw_numtxpending(ah, sc->tx.txq[i].axq_qnum); This can be moved to ath9k_hw_abort_tx_dma() and make it return npend as the current return value is unused anyway. Vasanth ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset 2011-03-10 8:58 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Vasanthakumar Thiagarajan @ 2011-03-10 9:32 ` Vasanthakumar Thiagarajan 2011-03-10 12:47 ` Felix Fietkau 1 sibling, 0 replies; 14+ messages in thread From: Vasanthakumar Thiagarajan @ 2011-03-10 9:32 UTC (permalink / raw) To: Felix Fietkau Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On Thu, Mar 10, 2011 at 02:28:47PM +0530, Vasanthakumar Thiagarajan wrote: > On Thu, Mar 10, 2011 at 06:05:01AM +0530, Felix Fietkau wrote: > > +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) > > +{ > > + int i, q; > > + > > + REG_WRITE(ah, AR_Q_TXD, AR_Q_TXD_M); > > + > > + REG_SET_BIT(ah, AR_PCU_MISC, AR_PCU_FORCE_QUIET_COLL | AR_PCU_CLEAR_VMF); > > + REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); > > + REG_SET_BIT(ah, AR_D_GBL_IFS_MISC, AR_D_GBL_IFS_MISC_IGNORE_BACKOFF); > > + > > + for (q = 0; q < AR_NUM_QCU; q++) { > > + for (i = 1000; i > 0; i--) { > > + if (!ath9k_hw_numtxpending(ah, q)) > > + break; > > + > > + udelay(5); > > + } > > + } > > + if (!i) > > + return false; > > Here the assumption looks like a reset would follow to configure > those registers back, may be some comment will be useful. > Also a hw reset is not guaranteed to follow this, call of ath_drain_all_txq() in .flush(), for ex. Vasanth ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset 2011-03-10 8:58 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Vasanthakumar Thiagarajan 2011-03-10 9:32 ` Vasanthakumar Thiagarajan @ 2011-03-10 12:47 ` Felix Fietkau 1 sibling, 0 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 12:47 UTC (permalink / raw) To: Vasanthakumar Thiagarajan Cc: linux-wireless@vger.kernel.org, linville@tuxdriver.com, Luis Rodriguez On 2011-03-10 9:58 AM, Vasanthakumar Thiagarajan wrote: > On Thu, Mar 10, 2011 at 06:05:01AM +0530, Felix Fietkau wrote: >> +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) >> +{ >> + int i, q; >> + >> + REG_WRITE(ah, AR_Q_TXD, AR_Q_TXD_M); >> + >> + REG_SET_BIT(ah, AR_PCU_MISC, AR_PCU_FORCE_QUIET_COLL | AR_PCU_CLEAR_VMF); >> + REG_SET_BIT(ah, AR_DIAG_SW, AR_DIAG_FORCE_CH_IDLE_HIGH); >> + REG_SET_BIT(ah, AR_D_GBL_IFS_MISC, AR_D_GBL_IFS_MISC_IGNORE_BACKOFF); >> + >> + for (q = 0; q < AR_NUM_QCU; q++) { >> + for (i = 1000; i > 0; i--) { >> + if (!ath9k_hw_numtxpending(ah, q)) >> + break; >> + >> + udelay(5); >> + } >> + } >> + if (!i) >> + return false; > > Here the assumption looks like a reset would follow to configure > those registers back, may be some comment will be useful. > >> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c >> index e16136d..bb1d29e 100644 >> --- a/drivers/net/wireless/ath/ath9k/xmit.c >> +++ b/drivers/net/wireless/ath/ath9k/xmit.c >> @@ -1194,16 +1194,14 @@ bool ath_drain_all_txq(struct ath_softc *sc, bool retry_tx) >> if (sc->sc_flags & SC_OP_INVALID) >> return true; >> >> - /* Stop beacon queue */ >> - ath9k_hw_stoptxdma(sc->sc_ah, sc->beacon.beaconq); >> + ath9k_hw_abort_tx_dma(ah); >> >> - /* Stop data queues */ >> + /* Check if any queue remains active */ >> for (i = 0; i < ATH9K_NUM_TX_QUEUES; i++) { >> - if (ATH_TXQ_SETUP(sc, i)) { >> - txq = &sc->tx.txq[i]; >> - ath9k_hw_stoptxdma(ah, txq->axq_qnum); >> - npend += ath9k_hw_numtxpending(ah, txq->axq_qnum); >> - } >> + if (!ATH_TXQ_SETUP(sc, i)) >> + continue; >> + >> + npend += ath9k_hw_numtxpending(ah, sc->tx.txq[i].axq_qnum); > > This can be moved to ath9k_hw_abort_tx_dma() and make it return > npend as the current return value is unused anyway. I think I'll drop the return code of ath9k_hw_abort_tx_dma. Since it checks the queues individually, queues that hit a timeout at first might recover later, so it would be useful if the caller checks them properly after the abort has been issued completely. - Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset @ 2011-03-10 4:21 Mark Mentovai 2011-03-10 13:20 ` Felix Fietkau 0 siblings, 1 reply; 14+ messages in thread From: Mark Mentovai @ 2011-03-10 4:21 UTC (permalink / raw) To: Felix Fietkau, linux-wireless; +Cc: linville, lrodriguez > --- a/drivers/net/wireless/ath/ath9k/mac.c [...] > +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) [...] > + for (q = 0; q < AR_NUM_QCU; q++) { > + for (i = 1000; i > 0; i--) { > + if (!ath9k_hw_numtxpending(ah, q)) > + break; > + > + udelay(5); > + } > + } > + if (!i) > + return false; Awesome-looking patch set. What’s the return value of this function supposed to mean? As written, it only returns false there’s anything left pending on the final queue. Notably, if the inner loop ran for all 999 iterations and ath9k_hw_numtxpending never returned false, the code above would effectively ignore that condition. I’m less concerned about the return value (because it actually seems that the function could be declared void, you don’t use the return value anywhere) than I am about the REG_CLR_BIT and REG_WRITE operations that follow. It seems that you might be either missing those in cases where they should happen, or performing them in cases where they shouldn’t. Did you mean to leave AR_Q_TXD set when something is left pending on the final hardware queue? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset 2011-03-10 4:21 Mark Mentovai @ 2011-03-10 13:20 ` Felix Fietkau 0 siblings, 0 replies; 14+ messages in thread From: Felix Fietkau @ 2011-03-10 13:20 UTC (permalink / raw) To: Mark Mentovai; +Cc: linux-wireless, linville, lrodriguez On 2011-03-10 5:21 AM, Mark Mentovai wrote: >> --- a/drivers/net/wireless/ath/ath9k/mac.c > [...] >> +bool ath9k_hw_abort_tx_dma(struct ath_hw *ah) > [...] >> + for (q = 0; q < AR_NUM_QCU; q++) { >> + for (i = 1000; i > 0; i--) { >> + if (!ath9k_hw_numtxpending(ah, q)) >> + break; >> + >> + udelay(5); >> + } >> + } >> + if (!i) >> + return false; > > Awesome-looking patch set. > > What’s the return value of this function supposed to mean? As > written, it only returns false there’s anything left pending on the > final queue. Notably, if the inner loop ran for all 999 iterations > and ath9k_hw_numtxpending never returned false, the code above would > effectively ignore that condition. Makes sense > I’m less concerned about the return value (because it actually seems > that the function could be declared void, you don’t use the return > value anywhere) than I am about the REG_CLR_BIT and REG_WRITE > operations that follow. It seems that you might be either missing > those in cases where they should happen, or performing them in cases > where they shouldn’t. Did you mean to leave AR_Q_TXD set when > something is left pending on the final hardware queue? I'll drop the return code and make the clearing of AR_Q_TXD and the other bits unconditional. - Felix ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-03-10 13:20 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-10 0:35 [PATCH 1/4] ath9k_hw: fix REG_SET_BIT and REG_CLR_BIT for multiple bits Felix Fietkau 2011-03-10 0:35 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Felix Fietkau 2011-03-10 0:35 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Felix Fietkau 2011-03-10 0:35 ` [PATCH 4/4] ath9k: improve reliability of beacon transmission and stuck beacon handling Felix Fietkau 2011-03-10 9:46 ` Vasanthakumar Thiagarajan 2011-03-10 13:04 ` Felix Fietkau 2011-03-10 13:15 ` Felix Fietkau 2011-03-10 9:25 ` [PATCH 3/4] ath9k: fix the .flush driver op implementation Vasanthakumar Thiagarajan 2011-03-10 13:03 ` Felix Fietkau 2011-03-10 8:58 ` [PATCH 2/4] ath9k: fix stopping tx dma on reset Vasanthakumar Thiagarajan 2011-03-10 9:32 ` Vasanthakumar Thiagarajan 2011-03-10 12:47 ` Felix Fietkau -- strict thread matches above, loose matches on Subject: below -- 2011-03-10 4:21 Mark Mentovai 2011-03-10 13:20 ` Felix Fietkau
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).