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

When iwx(4) takes a frame off the 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.

diff --git a/sys/dev/pci/if_iwx.c b/sys/dev/pci/if_iwx.c
index 21bc8be..8761374 100644 (file)
--- a/sys/dev/pci/if_iwx.c
+++ b/sys/dev/pci/if_iwx.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_iwx.c,v 1.59 2021/06/21 10:19:21 stsp Exp $        */
+/*     $OpenBSD: if_iwx.c,v 1.60 2021/06/30 09:46:46 stsp Exp $        */
 
 /*
  * Copyright (c) 2014, 2016 genua gmbh <info@genua.de>
@@ -4245,6 +4245,9 @@ iwx_rx_tx_cmd(struct iwx_softc *sc, struct iwx_rx_packet *pkt,
        int qid = cmd_hdr->qid;
        struct iwx_tx_ring *ring = &sc->txq[qid];
        struct iwx_tx_data *txd;
+       struct iwx_tx_resp *tx_resp = (void *)pkt->data;
+       uint32_t ssn;
+       uint32_t len = iwx_rx_packet_len(pkt);
 
        bus_dmamap_sync(sc->sc_dmat, data->map, 0, IWX_RBUF_SIZE,
            BUS_DMASYNC_POSTREAD);
@@ -4255,20 +4258,21 @@ iwx_rx_tx_cmd(struct iwx_softc *sc, struct iwx_rx_packet *pkt,
        if (txd->m == NULL)
                return;
 
+       if (sizeof(*tx_resp) + sizeof(ssn) +
+           tx_resp->frame_count * sizeof(tx_resp->status) > len)
+               return;
+
        iwx_rx_tx_cmd_single(sc, pkt, txd->in);
-       iwx_txd_done(sc, txd);
-       iwx_tx_update_byte_tbl(ring, idx, 0, 0);
 
        /*
-        * 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.
         */
-       while (ring->tail != idx) {
+       memcpy(&ssn, &tx_resp->status + tx_resp->frame_count, sizeof(ssn));
+       ssn = le32toh(ssn) & 0xfff;
+       while (ring->tail != IWX_AGG_SSN_TO_TXQ_IDX(ssn)) {
                txd = &ring->data[ring->tail];
                if (txd->m != NULL) {
-                       DPRINTF(("%s: missed Tx completion: tail=%d idx=%d\n",
-                           __func__, ring->tail, idx));
                        iwx_txd_done(sc, txd);
                        iwx_tx_update_byte_tbl(ring, idx, 0, 0);
                        ring->queued--;

diff --git a/sys/dev/pci/if_iwxvar.h b/sys/dev/pci/if_iwxvar.h
index 390b2fe..928b4d9 100644 (file)
--- a/sys/dev/pci/if_iwxvar.h
+++ b/sys/dev/pci/if_iwxvar.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if_iwxvar.h,v 1.16 2021/05/06 09:19:28 stsp Exp $     */
+/*     $OpenBSD: if_iwxvar.h,v 1.17 2021/06/30 09:46:46 stsp Exp $     */
 
 /*
  * Copyright (c) 2014 genua mbh <info@genua.de>
@@ -228,6 +228,9 @@ struct iwx_dma_info {
 #define IWX_TX_RING_LOMARK     192
 #define IWX_TX_RING_HIMARK     224
 
+/* For aggregation queues, index must be aligned to frame sequence number. */
+#define IWX_AGG_SSN_TO_TXQ_IDX(x)      ((x) & (IWX_TX_RING_COUNT - 1))
+
 struct iwx_tx_data {
        bus_dmamap_t    map;
        bus_addr_t      cmd_paddr;