From: stsp <stsp@openbsd.org>
Date: Wed, 30 Jun 2021 09:42:22 +0000 (+0000)
Subject: Make Tx-done interrupt processing in iwm(4) more similar to iwlwifi.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=0090cb9837062d43df254c97b9ce976e2aa2a0ea;p=openbsd

Make Tx-done interrupt processing in iwm(4) more similar to iwlwifi.

When iwm(4) takes a frame off a non-aggregation queue we used the ring index
reported by firmware to figure out which frame can be taken off the ring.
This logic was inherited from iwn(4).

We have sometimes seen an index get skipped and had a workaround in place
where we took all pending frames up to this index off the ring.

Apart from the ring index the firmware reports another value which is its
starting sequence number (SSN). An SSN is usually associated with a Tx
aggregation queue that uses block ack. On non-aggregation queues the SSN
matches the ring index most of the time and hence seems redundant at first
sight. But the values are not always the same.

We now always use the SSN as the upper bound which matches the Linux driver.

This seems to fix fatal firmware errors during Tx commands seen by jcs@
which suggests that we were sometimes taking frames off the ring too early.

The ring index is still used to feed information about a frame to the Tx
rate control algorithm but no longer determines when frames are taken off
the ring.

test and ok jcs@
---

diff --git a/sys/dev/pci/if_iwm.c b/sys/dev/pci/if_iwm.c
index e33fe39b978..026d797f5c9 100644
--- a/sys/dev/pci/if_iwm.c
+++ b/sys/dev/pci/if_iwm.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if_iwm.c,v 1.329 2021/06/21 10:19:21 stsp Exp $	*/
+/*	$OpenBSD: if_iwm.c,v 1.330 2021/06/30 09:42:22 stsp Exp $	*/
 
 /*
  * Copyright (c) 2014, 2016 genua gmbh <info@genua.de>
@@ -5460,7 +5460,7 @@ iwm_rx_tx_cmd(struct iwm_softc *sc, struct iwm_rx_packet *pkt,
 		return;
 	if (qid < IWM_FIRST_AGG_TX_QUEUE && tx_resp->frame_count > 1)
 		return;
-	if (qid >= IWM_FIRST_AGG_TX_QUEUE && sizeof(*tx_resp) + sizeof(ssn) +
+	if (sizeof(*tx_resp) + sizeof(ssn) +
 	    tx_resp->frame_count * sizeof(tx_resp->status) > len)
 		return;
 
@@ -5468,26 +5468,21 @@ iwm_rx_tx_cmd(struct iwm_softc *sc, struct iwm_rx_packet *pkt,
 	if (txd->m == NULL)
 		return;
 
+	memcpy(&ssn, &tx_resp->status + tx_resp->frame_count, sizeof(ssn));
+	ssn = le32toh(ssn) & 0xfff;
 	if (qid >= IWM_FIRST_AGG_TX_QUEUE) {
 		int status;
-		memcpy(&ssn, &tx_resp->status + tx_resp->frame_count, sizeof(ssn));
-		ssn = le32toh(ssn) & 0xfff;
 		status = le16toh(tx_resp->status.status) & IWM_TX_STATUS_MSK;
 		iwm_ampdu_tx_done(sc, cmd_hdr, txd->in, ring,
 		    le32toh(tx_resp->initial_rate), tx_resp->frame_count,
 		    tx_resp->failure_frame, ssn, status, &tx_resp->status);
 	} else {
-		iwm_rx_tx_cmd_single(sc, pkt, txd->in, txd->txmcs, txd->txrate);
-		iwm_txd_done(sc, txd);
-		ring->queued--;
-
 		/*
-		 * XXX Sometimes we miss Tx completion interrupts.
-		 * We cannot check Tx success/failure for affected frames;
-		 * just free the associated mbuf and release the associated
-		 * node reference.
+		 * Even though this is not an agg queue, we must only free
+		 * frames before the firmware's starting sequence number.
 		 */
-		iwm_txq_advance(sc, ring, idx);
+		iwm_rx_tx_cmd_single(sc, pkt, txd->in, txd->txmcs, txd->txrate);
+		iwm_txq_advance(sc, ring, IWM_AGG_SSN_TO_TXQ_IDX(ssn));
 		iwm_clear_oactive(sc, ring);
 	}
 }