Re: [Cerowrt-devel] Fwd: [bug #442] ath9k queue hang

[Dave Taht <dave.taht@gmail.com>]

Thx felix!

Given that there seems to be a potential race in the code
review I did at:

http://www.bufferbloat.net/issues/442#note-22

another thought is to make the increment and decrement of

txq->pending_frame atomic, or to do a flush before the unlock

What tree is this patch against?

On Tue, Apr 15, 2014 at 11:46 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2014-04-15 06:06, Dave Taht wrote:
>> regrettably I am too wiped to look this over further right now, but the patchset
>> seems very promising.
>>
>> I will review on a fresh brain in the morning. Other eyeballs desired
>> - this will have to get patched on top of 3.14 and then backported to
>> the 3.10 backport....
> The patch is a rather crude workaround which unfortunately will not
> help with narrowing down the cause. Also, doing a chip reset because a
> software queue is stuck is overkill.
>
> Please test if this patch helps. The tid->paused flag is no longer
> necessary since my rework of the tx path.
> ---
> --- a/drivers/net/wireless/ath/ath9k/ath9k.h
> +++ b/drivers/net/wireless/ath/ath9k/ath9k.h
> @@ -254,7 +254,6 @@ struct ath_atx_tid {
>
>         s8 bar_index;
>         bool sched;
> -       bool paused;
>         bool active;
>  };
>
> --- a/drivers/net/wireless/ath/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath/ath9k/xmit.c
> @@ -107,9 +107,6 @@ static void ath_tx_queue_tid(struct ath_
>  {
>         struct ath_atx_ac *ac = tid->ac;
>
> -       if (tid->paused)
> -               return;
> -
>         if (tid->sched)
>                 return;
>
> @@ -1407,7 +1404,6 @@ int ath_tx_aggr_start(struct ath_softc *
>         ath_tx_tid_change_state(sc, txtid);
>
>         txtid->active = true;
> -       txtid->paused = true;
>         *ssn = txtid->seq_start = txtid->seq_next;
>         txtid->bar_index = -1;
>
> @@ -1427,7 +1423,6 @@ void ath_tx_aggr_stop(struct ath_softc *
>
>         ath_txq_lock(sc, txq);
>         txtid->active = false;
> -       txtid->paused = false;
>         ath_tx_flush_tid(sc, txtid);
>         ath_tx_tid_change_state(sc, txtid);
>         ath_txq_unlock_complete(sc, txq);
> @@ -1487,7 +1482,7 @@ void ath_tx_aggr_wakeup(struct ath_softc
>                 ath_txq_lock(sc, txq);
>                 ac->clear_ps_filter = true;
>
> -               if (!tid->paused && ath_tid_has_buffered(tid)) {
> +               if (ath_tid_has_buffered(tid)) {
>                         ath_tx_queue_tid(txq, tid);
>                         ath_txq_schedule(sc, txq);
>                 }
> @@ -1510,7 +1505,6 @@ void ath_tx_aggr_resume(struct ath_softc
>         ath_txq_lock(sc, txq);
>
>         tid->baw_size = IEEE80211_MIN_AMPDU_BUF << sta->ht_cap.ampdu_factor;
> -       tid->paused = false;
>
>         if (ath_tid_has_buffered(tid)) {
>                 ath_tx_queue_tid(txq, tid);
> @@ -1544,8 +1538,6 @@ void ath9k_release_buffered_frames(struc
>                         continue;
>
>                 tid = ATH_AN_2_TID(an, i);
> -               if (tid->paused)
> -                       continue;
>
>                 ath_txq_lock(sc, tid->ac->txq);
>                 while (nframes > 0) {
> @@ -1844,9 +1836,6 @@ void ath_txq_schedule(struct ath_softc *
>                         list_del(&tid->list);
>                         tid->sched = false;
>
> -                       if (tid->paused)
> -                               continue;
> -
>                         if (ath_tx_sched_aggr(sc, txq, tid, &stop))
>                                 sent = true;
>
> @@ -2698,7 +2687,6 @@ void ath_tx_node_init(struct ath_softc *
>                 tid->baw_size  = WME_MAX_BA;
>                 tid->baw_head  = tid->baw_tail = 0;
>                 tid->sched     = false;
> -               tid->paused    = false;
>                 tid->active        = false;
>                 __skb_queue_head_init(&tid->buf_q);
>                 __skb_queue_head_init(&tid->retry_q);
>



-- 
Dave Täht

NSFW: https://w2.eff.org/Censorship/Internet_censorship_bills/russell_0296_indecent.article