There's a logic bug in iwn(4) which means that automatic rate control
for A-MPDU runs while a fixed Tx MCS is configured with a command like
"ifconfig iwn0 media HT-MCS10 mode 11n".
The intention was of course the inverse: Use automatic rate control if
the Tx MCS is not fixed (i.e. if ic->ic_fixed_mcs == -1).
 
And there is a second bug which went unnoticed because of the above bug.
The value of 'seq' provided by firmware may be smaller than ba_winstart.
When this happens we trigger a KASSERT in ieee80211_output_ba_record_ack:
[[[
void
ieee80211_output_ba_record_ack(struct ieee80211com *ic,
    struct ieee80211_node *ni, uint8_t tid, uint16_t ssn)
{
        struct ieee80211_tx_ba *ba = &ni->ni_tx_ba[tid];
        int i = 0;
        uint16_t s = ba->ba_winstart;

        KASSERT(!SEQ_LT(ssn, ba->ba_winstart));
]]]

In fact, iwn_ampdu_rate_control() implicitly assumes that its 'seq'
parameter equals ba_winstart, so just pass that value directly.
Any frames before ba_winstart should already have been processed earlier.
They have been taken off the Tx ring already and any associated status
information about those frames has already been freed.

For clarity we could rename the 'seq' parameter of iwn_ampdu_rate_control()
to something like 'winstart'. But doing so would add a lot of noise to this
diff so I left that change out for now.

Technically, 'seq' is now unused, but I'd like to leave the code and its
comments intact because there is no information elsewhere about how this
part of the Intel firmware block ack mechanism works. I suppose the compiler
will optimize it away.

ok?

diff 595dd8c72381b17b942a17026a1ed6071864ca33 
3a86217247de0a2c279745d477ce0fd05e104135
blob - 7b3117d466e7d2ca798ddb1b01cef99e8b832dac
blob + 40a5316402f2ecc256b8ea777014595340cf7823
--- sys/dev/pci/if_iwn.c
+++ sys/dev/pci/if_iwn.c
@@ -2379,19 +2379,22 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
         * Multiple BA notifications in a row may be using this number, with
         * additional bits being set in cba->bitmap. It is unclear how the
         * firmware decides to shift this window forward.
+        * We rely on ba->ba_winstart instead.
         */
        seq = le16toh(cba->seq) >> IEEE80211_SEQ_SEQ_SHIFT;
 
        /*
         * The firmware's new BA window starting sequence number
         * corresponds to the first hole in cba->bitmap, implying
-        * that all frames between 'seq' and 'ssn' have been acked.
+        * that all frames between 'seq' and 'ssn' (non-inclusive)
+        * have been acked.
         */
        ssn = le16toh(cba->ssn);
 
        /* Skip rate control if our Tx rate is fixed. */
-       if (ic->ic_fixed_mcs != -1)
-               iwn_ampdu_rate_control(sc, ni, txq, cba->tid, seq, ssn);
+       if (ic->ic_fixed_mcs == -1)
+               iwn_ampdu_rate_control(sc, ni, txq, cba->tid, ba->ba_winstart,
+                   ssn);
 
        /*
         * SSN corresponds to the first (perhaps not yet transmitted) frame

Reply via email to