* 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; 6+ 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] 6+ messages in thread* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset
2011-03-10 4:21 [PATCH 2/4] ath9k: fix stopping tx dma on reset Mark Mentovai
@ 2011-03-10 13:20 ` Felix Fietkau
0 siblings, 0 replies; 6+ 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] 6+ messages in thread
* [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; 6+ 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] 6+ 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 8:58 ` Vasanthakumar Thiagarajan
0 siblings, 1 reply; 6+ 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] 6+ 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 8:58 ` Vasanthakumar Thiagarajan
2011-03-10 9:32 ` Vasanthakumar Thiagarajan
2011-03-10 12:47 ` Felix Fietkau
0 siblings, 2 replies; 6+ 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] 6+ messages in thread* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset
2011-03-10 8:58 ` Vasanthakumar Thiagarajan
@ 2011-03-10 9:32 ` Vasanthakumar Thiagarajan
2011-03-10 12:47 ` Felix Fietkau
1 sibling, 0 replies; 6+ 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] 6+ messages in thread* Re: [PATCH 2/4] ath9k: fix stopping tx dma on reset
2011-03-10 8:58 ` Vasanthakumar Thiagarajan
2011-03-10 9:32 ` Vasanthakumar Thiagarajan
@ 2011-03-10 12:47 ` Felix Fietkau
1 sibling, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2011-03-10 13:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-03-10 4:21 [PATCH 2/4] ath9k: fix stopping tx dma on reset Mark Mentovai
2011-03-10 13:20 ` Felix Fietkau
-- strict thread matches above, loose matches on Subject: below --
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 8:58 ` Vasanthakumar Thiagarajan
2011-03-10 9:32 ` Vasanthakumar Thiagarajan
2011-03-10 12:47 ` 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).