Fix processing of compressed block ack notifications sent by iwn(4) firmware.

Fix wrong assumptions about what the data in these notifications is supposed
to represent, and actually piece information about individual subframes of
aggregated frames (A-MPDUs) back together when reporting to MiRA, rather than
reporting unrelated subframes to MiRA individually.

Testing by cwen@, Josh Grosse, f.holop, benno@
ok jmatthew@

2 files changed, 155 insertions, 86 deletions

diff --git a/sys/dev/pci/if_iwn.c b/sys/dev/pci/if_iwn.c
index 0b3c73fd0f2..b0fb1ec2174 100644
--- a/sys/dev/pci/if_iwn.c
+++ b/sys/dev/pci/if_iwn.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_iwn.c,v 1.226 2020/04/27 08:01:50 stsp Exp $	*/
+/*	$OpenBSD: if_iwn.c,v 1.227 2020/04/27 08:02:24 stsp Exp $	*/
 
 /*-
  * Copyright (c) 2007-2010 Damien Bergamini <damien.bergamini@free.fr>
@@ -2282,7 +2282,11 @@ iwn_mira_choose(struct iwn_softc *sc, struct ieee80211_node *ni)
 		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_rx_desc *desc,
 	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,63 +2327,102 @@ iwn_rx_compressed_ba(struct iwn_softc *sc, struct iwn_rx_desc *desc,
 	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)
+		if (have_ack > 0) {
+			wn->mn.txfail = wn->mn.frames - have_ack;
 			iwn_mira_choose(sc, ni);
+		}
 	}
 }
 
@@ -2551,12 +2595,29 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_ring *txq,
 	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;
@@ -2572,13 +2633,36 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_ring *txq,
 			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;
 	}
@@ -2597,14 +2681,10 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_ring *txq,
 
 	/*
 	 * 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;
@@ -2615,32 +2695,18 @@ iwn_ampdu_tx_done(struct iwn_softc *sc, struct iwn_tx_ring *txq,
 		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);
 }
 
 /*
@@ -2758,9 +2824,8 @@ iwn_tx_done_free_txdata(struct iwn_softc *sc, struct iwn_tx_data *data)
 	data->retries = 0;
 	data->txfail = 0;
 	data->txmcs = 0;
-	data->actual_txmcs = 0;
+	data->ampdu_txmcs = 0;
 	data->txrate = 0;
-	data->flags = 0;
 }
 
 void
@@ -3621,7 +3686,7 @@ iwn_tx(struct iwn_softc *sc, struct mbuf *m, struct ieee80211_node *ni)
 	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));
diff --git a/sys/dev/pci/if_iwnvar.h b/sys/dev/pci/if_iwnvar.h
index 95020825899..20991a04e16 100644
--- a/sys/dev/pci/if_iwnvar.h
+++ b/sys/dev/pci/if_iwnvar.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_iwnvar.h,v 1.36 2020/04/09 07:20:08 stsp Exp $	*/
+/*	$OpenBSD: if_iwnvar.h,v 1.37 2020/04/27 08:02:24 stsp Exp $	*/
 
 /*-
  * Copyright (c) 2007, 2008
@@ -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 {