This applies on top of a previous diff I sent, the one to make use of
the entire firmware retry table.
Recently I have spent a good amount of time trying to reverse-engineer
the sparsely documented behaviour of both iwn(4) and iwm(4) firmware with
respect to Tx aggregation. For details, see my comments in the diff.
I have also put some thought into how we can reconcile the firmware's
representation of relevant information with what MiRA expects to see.
This diff is the best I could come up with so far. There were at lot of
earlier attempts and I'm still not sure if this is the best possible
solution, but it seems more correct than what we have now.
In particular, this fixes wrong assumptions about what the data in the
firmware's "compressed block ack" notifications is supposed to represent,
and actually pieces information about individual subframes of aggregated
frames (A-MPDUs) back together when reporting to MiRA, rather than reporting
unrelated subframes to MiRA individually. (Recall that MiRA was primarily
designed to operate on units of entire A-MDPUs.)
diff 24c5886569df7f9428f24e6e74c71e02cba8c34c
636d6d139ade79b967dc9eb8b553e48dcd939227
blob - 8476916e4337cb6e681c11f26fc1fca5f4a48334
blob + f1a30eea896c8502e8f15bb3d1133b74120253f4
--- sys/dev/pci/if_iwn.c
+++ sys/dev/pci/if_iwn.c
@@ -2282,7 +2282,11 @@ iwn_mira_choose(struct iwn_softc *sc, struct ieee80211
iwn_set_link_quality(sc, ni);
}
-/* Process an incoming Compressed BlockAck. */
+/*
+ * Process an incoming Compressed BlockAck.
+ * Note that these block ack notifications are generated by firmware and do
+ * not necessarily correspond to contents of block ack frames seen on the air.
+ */
void
iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_rx_desc *desc,
struct iwn_rx_data *data)
@@ -2293,7 +2297,8 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
struct ieee80211_tx_ba *ba;
struct iwn_node *wn;
struct iwn_tx_ring *txq;
- uint16_t ssn, idx;
+ uint16_t seq, ssn, idx, end_idx;
+ int min_ampdu_id, max_ampdu_id, id;
int qid;
if (ic->ic_state != IEEE80211_S_RUN)
@@ -2322,66 +2327,98 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_
if (ba->ba_state != IEEE80211_BA_AGREED)
return;
- ssn = le16toh(cba->ssn); /* BA window starting sequence number */
- if (!SEQ_LT(ssn, ba->ba_winstart)) {
- ieee80211_output_ba_move_window(ic, ni, cba->tid, ssn);
+ /*
+ * The first bit in cba->bitmap corresponds to the sequence number
+ * stored in the sequence control field cba->seq.
+ * Any frames older than this can now be discarded; they should
+ * already have been reported as failures or been acknowledged.
+ *
+ * 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.
+ */
+ seq = le16toh(cba->seq) >> IEEE80211_SEQ_SEQ_SHIFT;
+ if (!SEQ_LT(seq, ba->ba_winstart)) {
+ ieee80211_output_ba_move_window(ic, ni, cba->tid, seq);
iwn_ampdu_txq_advance(sc, txq, qid,
- IWN_AGG_SSN_TO_TXQ_IDX(ssn));
+ IWN_AGG_SSN_TO_TXQ_IDX(seq));
iwn_clear_oactive(sc, txq);
}
+ /* Our BA window should now correspond to the bitmap. */
+ if (ba->ba_winstart != seq)
+ return;
- /* ba->ba_winstart should now correspond to cba->ssn */
- if (ba->ba_winstart != cba->ssn)
+ /* Skip rate control if our Tx rate is fixed. */
+ if (ic->ic_fixed_mcs != -1)
return;
/*
- * Update Tx rate statistics.
- * Skip rate control if our Tx rate is fixed.
+ * 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.
*/
- if (ic->ic_fixed_mcs == -1 && cba->nframes_sent > 0) {
- int end_idx = IWN_AGG_SSN_TO_TXQ_IDX(ba->ba_winend);
- int bit = 0, nsent = cba->nframes_sent;
+ ssn = le16toh(cba->ssn);
+ /* Determine the min/max IDs we assigned to AMPDUs in this range. */
+ idx = IWN_AGG_SSN_TO_TXQ_IDX(seq);
+ end_idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
+ min_ampdu_id = txq->data[idx].ampdu_id;
+ max_ampdu_id = min_ampdu_id;
+ while (idx != end_idx) {
+ struct iwn_tx_data *txdata = &txq->data[idx];
+
+ if (min_ampdu_id > txdata->ampdu_id)
+ min_ampdu_id = txdata->ampdu_id;
+ if (max_ampdu_id < txdata->ampdu_id)
+ max_ampdu_id = txdata->ampdu_id;
+
+ idx = (idx + 1) % IWN_TX_RING_COUNT;
+ }
+
+ /*
+ * Update Tx rate statistics for A-MPDUs before firmware's BA window.
+ */
+ for (id = min_ampdu_id; id <= max_ampdu_id; id++) {
+ int have_ack = 0, bit = 0;
+ idx = IWN_AGG_SSN_TO_TXQ_IDX(seq);
+ end_idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
wn->mn.agglen = 0;
wn->mn.ampdu_size = 0;
-
- ssn = le16toh(ba->ba_winstart);
- idx = IWN_AGG_SSN_TO_TXQ_IDX(ssn);
- while (nsent && idx != end_idx) {
+ while (idx != end_idx) {
struct iwn_tx_data *txdata = &txq->data[idx];
- int have_ack = (le64toh(cba->bitmap) & (1 << bit));
-
- if ((ba->ba_bitmap & (1 << bit)) == 0) {
- /*
- * Don't report frames to MiRA which were sent
- * at a different Tx rate than ni->ni_txmcs.
- */
- if (txdata->actual_txmcs == ni->ni_txmcs) {
- wn->mn.frames++;
- wn->mn.agglen++;
- wn->mn.ampdu_size += txdata->totlen +
- IEEE80211_CRC_LEN;
- if (txdata->retries > 0)
- wn->mn.retries++;
- if (!have_ack || txdata->txfail > 0)
- wn->mn.txfail++;
- }
- if (have_ack)
- ieee80211_output_ba_record_ack(ic,
- ni, cba->tid, ssn);
+ /*
+ * We can assume that this subframe has been ACKed
+ * because ACK failures come as single frames and
+ * before failing an A-MPDU subframe the firmware
+ * sends it as a single frame at least once.
+ *
+ * However, when this A-MPDU was transmitted we
+ * learned know how many subframes it contained.
+ * So if firmware isn't reporting all subframes now
+ * we can deduce an ACK failure for missing frames.
+ */
+ if (txdata->ampdu_id == id &&
+ txdata->ampdu_txmcs == ni->ni_txmcs &&
+ (ba->ba_bitmap & (1 << bit)) == 0) {
+ have_ack++;
+ wn->mn.frames = txdata->ampdu_nframes;
+ wn->mn.agglen = txdata->ampdu_nframes;
+ wn->mn.ampdu_size = txdata->ampdu_size;
+ if (txdata->retries > 1)
+ wn->mn.retries++;
+ ieee80211_output_ba_record_ack(ic, txdata->ni,
+ cba->tid, (seq + bit) & 0xfff);
}
idx = (idx + 1) % IWN_TX_RING_COUNT;
- ssn = (ssn + 1) % 0xfff;
- nsent--;
bit++;
}
- if (wn->mn.ampdu_size > 0)
- ieee80211_mira_choose(&wn->mn, ic, ni);
+ if (have_ack > 0) {
+ wn->mn.txfail = wn->mn.frames - have_ack;
+ iwn_mira_choose(sc, ni);
+ }
}
- if (wn->mn.ampdu_size > 0)
- iwn_mira_choose(sc, ni);
}
/*
@@ -2553,15 +2590,32 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_
if (ic->ic_state != IEEE80211_S_RUN)
return;
- /*
- * For each subframe collect Tx status, retries, and Tx rate used.
- * (The Tx rate is the same for all subframes in this batch.)
- */
if (nframes > 1) {
+ int ampdu_id, have_ampdu_id = 0, ampdu_size = 0;
int i;
+
+ /* Compute the size of this A-MPDU. */
for (i = 0; i < nframes; i++) {
uint8_t qid = agg_status[i].qid;
uint8_t idx = agg_status[i].idx;
+
+ if (qid != desc->qid)
+ continue;
+
+ txdata = &txq->data[idx];
+ if (txdata->ni == NULL)
+ continue;
+
+ ampdu_size += txdata->totlen + IEEE80211_CRC_LEN;
+ }
+
+ /*
+ * For each subframe collect Tx status, retries, and Tx rate.
+ * (The Tx rate is the same for all subframes in this batch.)
+ */
+ for (i = 0; i < nframes; i++) {
+ uint8_t qid = agg_status[i].qid;
+ uint8_t idx = agg_status[i].idx;
uint16_t txstatus = (le16toh(agg_status[i].status) &
IWN_AGG_TX_STATUS_MASK);
uint16_t trycnt = (le16toh(agg_status[i].status) &
@@ -2574,13 +2628,36 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_
if (txdata->ni == NULL)
continue;
- txdata->flags |= IWN_TXDATA_IS_AMPDU_SUBFRAME;
if (rflags & IWN_RFLAG_MCS)
- txdata->actual_txmcs = rate;
+ txdata->ampdu_txmcs = rate;
if (txstatus != IWN_AGG_TX_STATE_TRANSMITTED)
txdata->txfail++;
if (trycnt > 1)
- txdata->retries += trycnt - 1;
+ txdata->retries++;
+
+ /*
+ * Assign a common ID to all subframes of this A-MPDU.
+ * This ID will be used during Tx rate control to
+ * infer the ACK status of individual subframes.
+ */
+ if (!have_ampdu_id) {
+ wn = (void *)txdata->ni;
+ ampdu_id = wn->next_ampdu_id++;
+ have_ampdu_id = 1;
+ }
+ txdata->ampdu_id = ampdu_id;
+
+ /*
+ * We will also need to know the total number of
+ * subframes and the size of this A-MPDU. We store
+ * this redundantly on each subframe because firmware
+ * only reports acknowledged subframes via compressed
+ * block-ack notification. This way we will know what
+ * the total number of subframes and size were even if
+ * just one of these subframes gets acknowledged.
+ */
+ txdata->ampdu_nframes = nframes;
+ txdata->ampdu_size = ampdu_size;
}
return;
}
@@ -2599,14 +2676,10 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_
/*
* Skip rate control if our Tx rate is fixed.
- *
- * If this frame had Tx attempts as an A-MPDU subframe then
- * rate control is handled upon reception of a block ack frame.
- * Otherwise, this was the only Tx attempt for this frame and
- * handling rate control now makes sense.
+ * Don't report frames to MiRA which were sent at a different
+ * Tx rate than ni->ni_txmcs.
*/
- if (ic->ic_fixed_mcs == -1 && txdata->txmcs == ni->ni_txmcs &&
- (txdata->flags & IWN_TXDATA_IS_AMPDU_SUBFRAME) == 0) {
+ if (ic->ic_fixed_mcs == -1 && txdata->txmcs == ni->ni_txmcs) {
wn->mn.frames++;
wn->mn.agglen = 1;
wn->mn.ampdu_size = txdata->totlen + IEEE80211_CRC_LEN;
@@ -2617,32 +2690,18 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_
iwn_mira_choose(sc, ni);
}
+ /*
+ * SSN corresponds to the first (perhaps not yet transmitted) frame
+ * in firmware's BA window. Firmware is not going to retransmit any
+ * frames before its BA window so mark them all as done.
+ */
+ ieee80211_output_ba_move_window(ic, ni, tid, ssn);
+ iwn_ampdu_txq_advance(sc, txq, desc->qid, IWN_AGG_SSN_TO_TXQ_IDX(ssn));
+ iwn_clear_oactive(sc, txq);
+
+ /* Clear potential gap at the front of receiver's BA window. */
if (txfail)
ieee80211_tx_compressed_bar(ic, ni, tid, ssn);
- else if (!SEQ_LT(ssn, ba->ba_winstart)) {
- /*
- * Move window forward if SSN lies beyond end of window,
- * otherwise we can't record the ACK for this frame.
- * Non-acked frames which left holes in the bitmap near
- * the beginning of the window must be discarded.
- */
- uint16_t s = ssn;
- while (SEQ_LT(ba->ba_winend, s)) {
- ieee80211_output_ba_move_window(ic, ni, tid, s);
- iwn_ampdu_txq_advance(sc, txq, desc->qid,
- IWN_AGG_SSN_TO_TXQ_IDX(s));
- s = (s + 1) % 0xfff;
- }
- /* SSN should now be within window; set corresponding bit. */
- ieee80211_output_ba_record_ack(ic, ni, tid, ssn);
- }
-
- /* Move window forward up to the first hole in the bitmap. */
- ieee80211_output_ba_move_window_to_first_unacked(ic, ni, tid, ssn);
- iwn_ampdu_txq_advance(sc, txq, desc->qid,
- IWN_AGG_SSN_TO_TXQ_IDX(ba->ba_winstart));
-
- iwn_clear_oactive(sc, txq);
}
/*
@@ -2760,9 +2819,8 @@ iwn_tx_done_free_txdata(struct iwn_softc *sc, struct i
data->retries = 0;
data->txfail = 0;
data->txmcs = 0;
- data->actual_txmcs = 0;
+ data->ampdu_txmcs = 0;
data->txrate = 0;
- data->flags = 0;
}
void
@@ -3623,7 +3681,7 @@ iwn_tx(struct iwn_softc *sc, struct mbuf *m, struct ie
data->ni = ni;
data->txmcs = ni->ni_txmcs;
data->txrate = ni->ni_txrate;
- data->actual_txmcs = ni->ni_txmcs; /* updated upon Tx interrupt */
+ data->ampdu_txmcs = ni->ni_txmcs; /* updated upon Tx interrupt */
DPRINTFN(4, ("sending data: qid=%d idx=%d len=%d nsegs=%d\n",
ring->qid, ring->cur, m->m_pkthdr.len, data->map->dm_nsegs));
blob - 95020825899ecad2a2bf9366fd5411d2dc8df9bb
blob + aa9ec41c308afa34fed94d45eae7df4d36c5409e
--- sys/dev/pci/if_iwnvar.h
+++ sys/dev/pci/if_iwnvar.h
@@ -71,9 +71,12 @@ struct iwn_tx_data {
int txfail;
int txmcs;
int txrate;
- int flags;
-#define IWN_TXDATA_IS_AMPDU_SUBFRAME 0x01
- int actual_txmcs; /* for retried A-MPDU subframes */
+
+ /* A-MPDU subframes */
+ int ampdu_id;
+ int ampdu_txmcs;
+ int ampdu_nframes;
+ int ampdu_size;
};
struct iwn_tx_ring {
@@ -111,6 +114,7 @@ struct iwn_node {
uint16_t disable_tid;
uint8_t id;
uint8_t ridx[IEEE80211_RATE_MAXSIZE];
+ uint32_t next_ampdu_id;
};
struct iwn_calib_state {
