From: kolya <[email protected]>
When tx agg is being stopped TID is flushed using ath_tx_flush_tid. It is possible that ath_tx_flush_tid completelly flushes TID (if all packets in this TID have already been retried). If this happened ath_tx_aggr_stop would leave TID in cleanup state permanently. Fix this by making ath_tx_flush_tid remove AGGR_ADDBA_COMPLETE and AGGR_CLEANUP flags from TID status if TID is empty.
---
drivers/net/wireless/ath/ath9k/xmit.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 55d077e..3574b0f 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -180,6 +180,11 @@ static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
}
spin_unlock_bh(&txq->axq_lock);
+
+ if (tid->baw_head == tid->baw_tail) {
+ tid->state &= ~AGGR_ADDBA_COMPLETE;
+ tid->state &= ~AGGR_CLEANUP;
+ }
}
static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
@@ -556,15 +561,9 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
spin_unlock_bh(&txq->axq_lock);
}
- if (tid->state & AGGR_CLEANUP) {
+ if (tid->state & AGGR_CLEANUP)
ath_tx_flush_tid(sc, tid);
- if (tid->baw_head == tid->baw_tail) {
- tid->state &= ~AGGR_ADDBA_COMPLETE;
- tid->state &= ~AGGR_CLEANUP;
- }
- }
-
rcu_read_unlock();
if (needreset) {
--
1.7.4.1
Hi Nikolay,
On Sat, Nov 19, 2011 at 12:11 PM, Nikolay Martynov <[email protected]> wrote:
> From: kolya <[email protected]>
>
> ?When tx agg is being stopped TID is flushed using ath_tx_flush_tid. It is possible that ath_tx_flush_tid completelly flushes TID (if all packets in this TID have already been retried). If this happened ath_tx_aggr_stop would leave TID in cleanup state permanently. Fix this by making ath_tx_flush_tid remove AGGR_ADDBA_COMPLETE and AGGR_CLEANUP flags from TID status if TID is empty.
> ---
> ?drivers/net/wireless/ath/ath9k/xmit.c | ? 13 ++++++-------
> ?1 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
> index 55d077e..3574b0f 100644
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -180,6 +180,11 @@ static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
> ? ? ? ?}
>
> ? ? ? ?spin_unlock_bh(&txq->axq_lock);
> +
> + ? ? ? if (tid->baw_head == tid->baw_tail) {
> + ? ? ? ? ? ? ? tid->state &= ~AGGR_ADDBA_COMPLETE;
> + ? ? ? ? ? ? ? tid->state &= ~AGGR_CLEANUP;
> + ? ? ? }
should this in be protected by axq_lock ?
> ?}
>
> ?static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
> @@ -556,15 +561,9 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
> ? ? ? ? ? ? ? ?spin_unlock_bh(&txq->axq_lock);
> ? ? ? ?}
>
> - ? ? ? if (tid->state & AGGR_CLEANUP) {
> + ? ? ? if (tid->state & AGGR_CLEANUP)
> ? ? ? ? ? ? ? ?ath_tx_flush_tid(sc, tid);
>
> - ? ? ? ? ? ? ? if (tid->baw_head == tid->baw_tail) {
> - ? ? ? ? ? ? ? ? ? ? ? tid->state &= ~AGGR_ADDBA_COMPLETE;
> - ? ? ? ? ? ? ? ? ? ? ? tid->state &= ~AGGR_CLEANUP;
> - ? ? ? ? ? ? ? }
> - ? ? ? }
> -
> ? ? ? ?rcu_read_unlock();
>
> ? ? ? ?if (needreset) {
based on my understanding if we don't clear this flag, i assume we
might have problem in ath_tx_aggr_start?
but should not this be properly cleared ath_txcomplete_aggr via tasklet path?
please let me know if i had missed somthing?
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at ?http://vger.kernel.org/majordomo-info.html
>
--
shafi
Nikolay Martynov <[email protected]> writes:
> From: kolya <[email protected]>
Please use your full name here.
> When tx agg is being stopped TID is flushed using ath_tx_flush_tid. It is possible that ath_tx_flush_tid completelly flushes TID (if all packets in this TID have already been retried). If this happened ath_tx_aggr_stop would leave TID in cleanup state permanently. Fix this by making ath_tx_flush_tid remove AGGR_ADDBA_COMPLETE and AGGR_CLEANUP flags from TID status if TID is empty.
And word wrap your commits to 72 chars or so.
--
Kalle Valo
When tx agg is being stopped TID is flushed using ath_tx_flush_tid. It
is possible that ath_tx_flush_tid completelly flushes TID (if all
packets in this TID have already been retried). If this happened
ath_tx_aggr_stop would leave TID in cleanup state permanently.
Fix this by making ath_tx_flush_tid remove AGGR_ADDBA_COMPLETE and
AGGR_CLEANUP flags from TID status if TID is empty.
Signed-off-by: Nikolay Martynov <[email protected]>
---
drivers/net/wireless/ath/ath9k/xmit.c | 13 ++++++-------
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 55d077e..80639e3 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -179,6 +179,11 @@ static void ath_tx_flush_tid(struct ath_softc *sc, struct ath_atx_tid *tid)
spin_lock_bh(&txq->axq_lock);
}
+ if (tid->baw_head == tid->baw_tail) {
+ tid->state &= ~AGGR_ADDBA_COMPLETE;
+ tid->state &= ~AGGR_CLEANUP;
+ }
+
spin_unlock_bh(&txq->axq_lock);
}
@@ -556,15 +561,9 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
spin_unlock_bh(&txq->axq_lock);
}
- if (tid->state & AGGR_CLEANUP) {
+ if (tid->state & AGGR_CLEANUP)
ath_tx_flush_tid(sc, tid);
- if (tid->baw_head == tid->baw_tail) {
- tid->state &= ~AGGR_ADDBA_COMPLETE;
- tid->state &= ~AGGR_CLEANUP;
- }
- }
-
rcu_read_unlock();
if (needreset) {
--
1.7.4.1
Hi,
2011/11/19 Mohammed Shafi <[email protected]>:
>> ? ? ? ?}
>>
>> ? ? ? ?spin_unlock_bh(&txq->axq_lock);
>> +
>> + ? ? ? if (tid->baw_head == tid->baw_tail) {
>> + ? ? ? ? ? ? ? tid->state &= ~AGGR_ADDBA_COMPLETE;
>> + ? ? ? ? ? ? ? tid->state &= ~AGGR_CLEANUP;
>> + ? ? ? }
>
> should this in be protected by axq_lock ?
In ath_tx_aggr_stop update of tid->state are protected by axq_lock.
In ath_tx_complete_aggr the updates of these flags were (before this
patch) not protected by axq_lock (unless I miss something here). So I
quess you are right - this actually should be protected by lock, I'll
send new version of this patch.
>
>> ?}
>>
>> ?static void ath_tx_update_baw(struct ath_softc *sc, struct ath_atx_tid *tid,
>> @@ -556,15 +561,9 @@ static void ath_tx_complete_aggr(struct ath_softc *sc, struct ath_txq *txq,
>> ? ? ? ? ? ? ? ?spin_unlock_bh(&txq->axq_lock);
>> ? ? ? ?}
>>
>> - ? ? ? if (tid->state & AGGR_CLEANUP) {
>> + ? ? ? if (tid->state & AGGR_CLEANUP)
>> ? ? ? ? ? ? ? ?ath_tx_flush_tid(sc, tid);
>>
>> - ? ? ? ? ? ? ? if (tid->baw_head == tid->baw_tail) {
>> - ? ? ? ? ? ? ? ? ? ? ? tid->state &= ~AGGR_ADDBA_COMPLETE;
>> - ? ? ? ? ? ? ? ? ? ? ? tid->state &= ~AGGR_CLEANUP;
>> - ? ? ? ? ? ? ? }
>> - ? ? ? }
>> -
>> ? ? ? ?rcu_read_unlock();
>>
>> ? ? ? ?if (needreset) {
>
> based on my understanding if we don't clear this flag, i assume we
> might have problem in ath_tx_aggr_start?
> but should not this be properly cleared ath_txcomplete_aggr via tasklet path?
> please let me know if i had ?missed somthing?
If AGGR_CLEANUP is not cleared up TID permanently stays in 'paused'
state and is never reused. To the best of my understanding this flag
is cleared in two places: ath_tx_aggr_stop and ath_tx_complete_aggr.
The flag should be removed when TID is empty.
ath_tx_aggr_stop had (before patch) a problem in which it didn't
clear the flag when TID was empty in some cases. Basically what
happened is that ath_tx_aggr_stop called ath_tx_flush_tid.
ath_tx_flush_tid tries to send packets that were not sent before and
if packet was already tried it removes it from TID. So under certain
conditions execution of ath_tx_flush_tid can result in empty TID.
ath_tx_aggr_stop didn't expect this and didn't clear the flag. When
TID is empty the ath_txcomplete_aggr is never called, so it doesn't
get a chance to cleanup the flag.
All I did is to move flag cleanup from ath_tx_complete_aggr to
ath_tx_flush_tid. ath_tx_flush_tid actually flushes TID and if TID is
empty after it was executed the flag should be removed. So, After my
patch both ath_tx_aggr_stop and ath_tx_complete_aggr clean up the flag
when appropriate.
I actually observed this problem (i.e. forever paused TID with no
packets and CLENUP flag up) and I think it can be recreated if one
tries to disable TID many times via debugfs when link is loaded with
traffic.
Please let me know if you have any more questions or suggestions.
Thanks!
--
Truthfully yours,
Martynov Nikolay.
Email: [email protected]