On Fri, Apr 17, 2020 at 06:30:22PM +0200, Stefan Sperling wrote:
> On Fri, Apr 17, 2020 at 04:03:30PM +0200, Stefan Sperling wrote:
> > 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.)
>
> Unfortunately, this diff can trigger a page fault. I am looking for a fix.
This new version has been tested by cwen and Josh Grosse and seems to
fix the problems they found with the previous diff. Thanks!
diff 24c5886569df7f9428f24e6e74c71e02cba8c34c
719fff35cc167c0814926c547e92b317aad32553
blob - 8476916e4337cb6e681c11f26fc1fca5f4a48334
blob + b8beb56aa1112e65c2907b2955694b6a8907d9ae
--- 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,103 @@ 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 (txdata->m != NULL) {
+ 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);
+ uint16_t s = (seq + bit) & 0xfff;
+ /*
+ * 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 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->m != NULL && txdata->ampdu_id == id &&
+ txdata->ampdu_txmcs == ni->ni_txmcs &&
+ (SEQ_LT(ba->ba_winend, s) ||
+ (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++;
+ if (!SEQ_LT(ba->ba_winend, s))
+ ieee80211_output_ba_record_ack(ic, ni,
+ cba->tid, s);
}
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 +2595,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 +2633,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 +2681,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 +2695,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 +2824,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 +3686,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 {
