[Cerowrt-devel] [bug #442] tearing apart ath_tx_start in search of the hang

[Cerowrt-devel] [bug #442] tearing apart ath_tx_start in search of the hang

[Dave Taht]

So what we are seeing here is some sort of problem with accounting
for pending_frames on a given queue. And since we've had a reminder
of how useful code review can be - and I'd rather like to understand
this logic anyway -

// my comments in //

/* Upon failure caller should free skb */
int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
                 struct ath_tx_control *txctl)
{
        struct ieee80211_hdr *hdr;
        struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
        struct ieee80211_sta *sta = txctl->sta;
        struct ieee80211_vif *vif = info->control.vif;
        struct ath_softc *sc = hw->priv;
        struct ath_txq *txq = txctl->txq;
        struct ath_atx_tid *tid = NULL;
        struct ath_buf *bf;
        int q;
        int ret;

        ret = ath_tx_prepare(hw, skb, txctl);
        if (ret)
            return ret;

        hdr = (struct ieee80211_hdr *) skb->data;
        /*
         * At this point, the vif, hw_key and sta pointers in the tx control
         * info are no longer valid (overwritten by the ath_frame_info data.
         */


// I haven't looked at what skb_get_queue_mapping can return yet

       q = skb_get_queue_mapping(skb);

        ath_txq_lock(sc, txq);
        if (txq == sc->tx.txq_map[q] &&
            ++txq->pending_frames > sc->tx.txq_max_pending[q] &&
            !txq->stopped) {
                ieee80211_stop_queue(sc->hw, q);
                txq->stopped = true;

// is there a difference between stopped and sleeping?

        }

// So if the queue is not mapped properly we don't increment pending
// frames. Also we are dependent on C processing the if left to right,
// which is a  good assumption, but it leaves the ++txq as a side effect

        if (txctl->an && ieee80211_is_data_present(hdr->frame_control))
                tid = ath_get_skb_tid(sc, txctl->an, skb);

        if (info->flags & IEEE80211_TX_CTL_PS_RESPONSE) {
                ath_txq_unlock(sc, txq);
                txq = sc->tx.uapsdq;

// So here we have a bit of state that changes after we've got some pending
// state above that's been changed. I imagine this lock could stay unlocked
// for a while and lead to races elsewhere.
// haven't a clue what tx.uapsdq is

                ath_txq_lock(sc, txq);
        } else if (txctl->an &&
                   ieee80211_is_data_present(hdr->frame_control)) {
                WARN_ON(tid->ac->txq != txctl->txq);

                if (info->flags & IEEE80211_TX_CTL_CLEAR_PS_FILT)
                        tid->ac->clear_ps_filter = true;

                /*
                 * Add this frame to software queue for scheduling later
                 * for aggregation.
                 */
                TX_STAT_INC(txq->axq_qnum, a_queued_sw);
                __skb_queue_tail(&tid->buf_q, skb);
                if (!txctl->an->sleeping)
                        ath_tx_queue_tid(txq, tid);
// so if we're not sleeping, queue it up
// and regardless if we're sleeping or not, schedule it
                ath_txq_schedule(sc, txq);
                goto out;
        }

// So if data is not present OR txctl->an is invalid OR
IEEE80211_TX_CTL_PS_RESPONSE is set in flags
/// we fall through to here.

       bf = ath_tx_setup_buffer(sc, txq, tid, skb);

// if we fell through to here, tid can be null unless data was present

        if (!bf) {
                ath_txq_skb_done(sc, txq, skb);
                if (txctl->paprd)
                        dev_kfree_skb_any(skb);
                else
                        ieee80211_free_txskb(sc->hw, skb);
                goto out;
        }

// Well, I note that we incremented the frames earlier in some cases
// should they be decremented above?

        bf->bf_state.bfs_paprd = txctl->paprd;

        if (txctl->paprd)
                bf->bf_state.bfs_paprd_timestamp = jiffies;

        ath_set_rates(vif, sta, bf);
        ath_tx_send_normal(sc, txq, tid, skb);

// Not clear as to why you set_rates here, and I assume tx_send_normal
// sends a non-aggregate

out:
        ath_txq_unlock(sc, txq);

        return 0;
}


-- 
Dave Täht

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

Re: [Cerowrt-devel] [bug #442] tearing apart ath_tx_start in search of the hang

[Robert Bradley]

On 15/04/14 00:08, Dave Taht wrote:
> So what we are seeing here is some sort of problem with accounting
> for pending_frames on a given queue. And since we've had a reminder
> of how useful code review can be - and I'd rather like to understand
> this logic anyway -
>
> // my comments in //
>
> /* Upon failure caller should free skb */
> int ath_tx_start(struct ieee80211_hw *hw, struct sk_buff *skb,
>                  struct ath_tx_control *txctl)
> {
<snip>
> // I haven't looked at what skb_get_queue_mapping can return yet
>
>        q = skb_get_queue_mapping(skb);
>
>         ath_txq_lock(sc, txq);
>         if (txq == sc->tx.txq_map[q] &&
>             ++txq->pending_frames > sc->tx.txq_max_pending[q] &&
>             !txq->stopped) {
>                 ieee80211_stop_queue(sc->hw, q);
>                 txq->stopped = true;
>
> // is there a difference between stopped and sleeping?
>
>         }
>

Not as far as I can tell from the documentation.  I did notice though
that the wakeup code in ath_tx_complete() lacks the
ath_txq_lock()/ath_txq_unlock() calls around it.  I don't know if that
helps or not...

-- 
Robert Bradley

Re: [Cerowrt-devel] [bug #442] tearing apart ath_tx_start in search of the hang

[Robert Bradley]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

On 15/04/2014 09:56, Robert Bradley wrote:
> Not as far as I can tell from the documentation.  I did notice though
> that the wakeup code in ath_tx_complete() lacks the
> ath_txq_lock()/ath_txq_unlock() calls around it.  I don't know if that
> helps or not...

Slight correction; make that ath_txq_skb_done()
(http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath9k/xmit.c#L150). 
It might not matter though if the queue's stopped at the time.

-- 
Robert Bradley



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]