Download raw body.
iwx: remove A-MPDU Rx reordering
On 2026 Mar 09 (Mon) at 14:55:30 +0100 (+0100), Stefan Sperling wrote:
:iwx inherited code from iwm which handles reordering of received A-MPDU
:subframes at the driver level. In Linux iwlwifi this code was added for
:9k devices and multi-queue Rx operation.
:Our drivers only use a single Rx queue, but keeping the code close enough
:to Linux seemed reasonable the time when 9k devices were new, and it didn't
:hurt.
:
:This code made its way into iwx because iwx started out as a copy of iwm.
:But it turns out AX200 and later devices perform Rx reordering in firmware
:already and set flags on Rx descriptors which the driver can use to check
:for duplicate frames which can be discarded.
:
:I have recently become aware of two related changes the iwlwifi developers
:made in 2023:
:
:1) Multi-queue Rx has been disabled in Linux on iwm 9000 devices.
:(linux commit 29fa9a984b6d1075020f12071a89897fd62ed27f)
:This was done to allow for change number 2:
:
:2) The Rx-reordering code has been removed from iwiwifi, because it was
:only needed for 9k devices which do not provide the needed information in
:Rx descriptors. And the code was found to occasionally drop legit frames.
:(linux commit ff8e3a40d78bc414213b2724ad775adf98780a5a)
:
:I am not sure to which degree the dropped legit frames are affecting
:our implementation of this in iwm. It seems to be working fine for us?
:Maybe we should look at potential packet loss on iwm 9k devices at some
:point, even though I do not recall any complaints about such an issue.
:
:In any case, this means the Rx reordering code in iwx is pointless.
:We can rely on Rx descriptor flags instead.
:
:Testing on any iwx devices would be appreciated. I have tested this diff
:on AX200 and BZ with no visible change in behaviour.
:
:ok?
:
tested on AX211, on 11ac and 11g networks.
OK
:M sys/dev/pci/if_iwx.c | 6+ 199-
:M sys/dev/pci/if_iwxreg.h | 1+ 0-
:M sys/dev/pci/if_iwxvar.h | 0+ 15-
:
:3 files changed, 7 insertions(+), 214 deletions(-)
:
:path + /usr/src
:commit - bf3713f7dfbf19ede85da952a0d01b849aa8a252
:blob - e731cf032ef66ed7a7eab46a3669fc5d80d9e8d3
:file + sys/dev/pci/if_iwx.c
:--- sys/dev/pci/if_iwx.c
:+++ sys/dev/pci/if_iwx.c
:@@ -329,7 +329,6 @@ int iwx_ampdu_tx_start(struct ieee80211com *, struct i
: void iwx_rx_ba_session_expired(void *);
: void iwx_rx_bar_frame_release(struct iwx_softc *, struct iwx_rx_packet *,
: struct mbuf_list *);
:-void iwx_reorder_timer_expired(void *);
: void iwx_sta_rx_agg(struct iwx_softc *, struct ieee80211_node *, uint8_t,
: uint16_t, uint16_t, int, int);
: void iwx_sta_tx_agg_start(struct iwx_softc *, struct ieee80211_node *,
:@@ -494,12 +493,9 @@ void iwx_dump_driver_status(struct iwx_softc *);
: void iwx_nic_umac_error(struct iwx_softc *);
: int iwx_detect_duplicate(struct iwx_softc *, struct mbuf *,
: struct iwx_rx_mpdu_desc *, struct ieee80211_rxinfo *);
:-int iwx_is_sn_less(uint16_t, uint16_t, uint16_t);
: void iwx_release_frames(struct iwx_softc *, struct ieee80211_node *,
: struct iwx_rxba_data *, struct iwx_reorder_buffer *, uint16_t,
: struct mbuf_list *);
:-int iwx_oldsn_workaround(struct iwx_softc *, struct ieee80211_node *,
:- int, struct iwx_reorder_buffer *, uint32_t, uint32_t);
: int iwx_rx_reorder(struct iwx_softc *, struct mbuf *, int,
: struct iwx_rx_mpdu_desc *, int, int, uint32_t,
: struct ieee80211_rxinfo *, struct mbuf_list *);
:@@ -3192,13 +3188,7 @@ iwx_init_reorder_buffer(struct iwx_reorder_buffer *reo
: reorder_buf->head_sn = ssn;
: reorder_buf->num_stored = 0;
: reorder_buf->buf_size = buf_size;
:- reorder_buf->last_amsdu = 0;
:- reorder_buf->last_sub_index = 0;
:- reorder_buf->removed = 0;
: reorder_buf->valid = 0;
:- reorder_buf->consec_oldsn_drops = 0;
:- reorder_buf->consec_oldsn_ampdu_gp2 = 0;
:- reorder_buf->consec_oldsn_prev_drop = 0;
: }
:
: void
:@@ -3211,11 +3201,8 @@ iwx_clear_reorder_buffer(struct iwx_softc *sc, struct
: for (i = 0; i < reorder_buf->buf_size; i++) {
: entry = &rxba->entries[i];
: ml_purge(&entry->frames);
:- timerclear(&entry->reorder_time);
: }
:
:- reorder_buf->removed = 1;
:- timeout_del(&reorder_buf->reorder_timer);
: timerclear(&rxba->last_rx);
: timeout_del(&rxba->session_timer);
: rxba->baid = IWX_RX_REORDER_DATA_INVALID_BAID;
:@@ -3286,68 +3273,6 @@ iwx_rx_bar_frame_release(struct iwx_softc *sc, struct
: iwx_release_frames(sc, ni, rxba, buf, nssn, ml);
: }
:
:-void
:-iwx_reorder_timer_expired(void *arg)
:-{
:- struct mbuf_list ml = MBUF_LIST_INITIALIZER();
:- struct iwx_reorder_buffer *buf = arg;
:- struct iwx_rxba_data *rxba = iwx_rxba_data_from_reorder_buf(buf);
:- struct iwx_reorder_buf_entry *entries = &rxba->entries[0];
:- struct iwx_softc *sc = rxba->sc;
:- struct ieee80211com *ic = &sc->sc_ic;
:- struct ieee80211_node *ni = ic->ic_bss;
:- int i, s;
:- uint16_t sn = 0, index = 0;
:- int expired = 0;
:- int cont = 0;
:- struct timeval now, timeout, expiry;
:-
:- if (!buf->num_stored || buf->removed)
:- return;
:-
:- s = splnet();
:- getmicrouptime(&now);
:- USEC_TO_TIMEVAL(RX_REORDER_BUF_TIMEOUT_MQ_USEC, &timeout);
:-
:- for (i = 0; i < buf->buf_size ; i++) {
:- index = (buf->head_sn + i) % buf->buf_size;
:-
:- if (ml_empty(&entries[index].frames)) {
:- /*
:- * If there is a hole and the next frame didn't expire
:- * we want to break and not advance SN.
:- */
:- cont = 0;
:- continue;
:- }
:- timeradd(&entries[index].reorder_time, &timeout, &expiry);
:- if (!cont && timercmp(&now, &expiry, <))
:- break;
:-
:- expired = 1;
:- /* continue until next hole after this expired frame */
:- cont = 1;
:- sn = (buf->head_sn + (i + 1)) & 0xfff;
:- }
:-
:- if (expired) {
:- /* SN is set to the last expired frame + 1 */
:- iwx_release_frames(sc, ni, rxba, buf, sn, &ml);
:- if_input(&sc->sc_ic.ic_if, &ml);
:- ic->ic_stats.is_ht_rx_ba_window_gap_timeout++;
:- } else {
:- /*
:- * If no frame expired and there are stored frames, index is now
:- * pointing to the first unexpired frame - modify reorder timeout
:- * accordingly.
:- */
:- timeout_add_usec(&buf->reorder_timer,
:- RX_REORDER_BUF_TIMEOUT_MQ_USEC);
:- }
:-
:- splx(s);
:-}
:-
: #define IWX_MAX_RX_BA_SESSIONS 16
:
: struct iwx_rxba_data *
:@@ -4895,18 +4820,6 @@ iwx_detect_duplicate(struct iwx_softc *sc, struct mbuf
: return 0;
: }
:
:-/*
:- * Returns true if sn2 - buffer_size < sn1 < sn2.
:- * To be used only in order to compare reorder buffer head with NSSN.
:- * We fully trust NSSN unless it is behind us due to reorder timeout.
:- * Reorder timeout can only bring us up to buffer_size SNs ahead of NSSN.
:- */
:-int
:-iwx_is_sn_less(uint16_t sn1, uint16_t sn2, uint16_t buffer_size)
:-{
:- return SEQ_LT(sn1, sn2) && !SEQ_LT(sn1, sn2 - buffer_size);
:-}
:-
: void
: iwx_release_frames(struct iwx_softc *sc, struct ieee80211_node *ni,
: struct iwx_rxba_data *rxba, struct iwx_reorder_buffer *reorder_buf,
:@@ -4915,11 +4828,7 @@ iwx_release_frames(struct iwx_softc *sc, struct ieee80
: struct iwx_reorder_buf_entry *entries = &rxba->entries[0];
: uint16_t ssn = reorder_buf->head_sn;
:
:- /* ignore nssn smaller than head sn - this can happen due to timeout */
:- if (iwx_is_sn_less(nssn, ssn, reorder_buf->buf_size))
:- goto set_timer;
:-
:- while (iwx_is_sn_less(ssn, nssn, reorder_buf->buf_size)) {
:+ while (SEQ_LT(ssn, nssn)) {
: int index = ssn % reorder_buf->buf_size;
: struct mbuf *m;
: int chanidx, is_shortpre;
:@@ -4956,63 +4865,8 @@ iwx_release_frames(struct iwx_softc *sc, struct ieee80
: ssn = (ssn + 1) & 0xfff;
: }
: reorder_buf->head_sn = nssn;
:-
:-set_timer:
:- if (reorder_buf->num_stored && !reorder_buf->removed) {
:- timeout_add_usec(&reorder_buf->reorder_timer,
:- RX_REORDER_BUF_TIMEOUT_MQ_USEC);
:- } else
:- timeout_del(&reorder_buf->reorder_timer);
: }
:
:-int
:-iwx_oldsn_workaround(struct iwx_softc *sc, struct ieee80211_node *ni, int tid,
:- struct iwx_reorder_buffer *buffer, uint32_t reorder_data, uint32_t gp2)
:-{
:- struct ieee80211com *ic = &sc->sc_ic;
:-
:- if (gp2 != buffer->consec_oldsn_ampdu_gp2) {
:- /* we have a new (A-)MPDU ... */
:-
:- /*
:- * reset counter to 0 if we didn't have any oldsn in
:- * the last A-MPDU (as detected by GP2 being identical)
:- */
:- if (!buffer->consec_oldsn_prev_drop)
:- buffer->consec_oldsn_drops = 0;
:-
:- /* either way, update our tracking state */
:- buffer->consec_oldsn_ampdu_gp2 = gp2;
:- } else if (buffer->consec_oldsn_prev_drop) {
:- /*
:- * tracking state didn't change, and we had an old SN
:- * indication before - do nothing in this case, we
:- * already noted this one down and are waiting for the
:- * next A-MPDU (by GP2)
:- */
:- return 0;
:- }
:-
:- /* return unless this MPDU has old SN */
:- if (!(reorder_data & IWX_RX_MPDU_REORDER_BA_OLD_SN))
:- return 0;
:-
:- /* update state */
:- buffer->consec_oldsn_prev_drop = 1;
:- buffer->consec_oldsn_drops++;
:-
:- /* if limit is reached, send del BA and reset state */
:- if (buffer->consec_oldsn_drops == IWX_AMPDU_CONSEC_DROPS_DELBA) {
:- ieee80211_delba_request(ic, ni, IEEE80211_REASON_UNSPECIFIED,
:- 0, tid);
:- buffer->consec_oldsn_prev_drop = 0;
:- buffer->consec_oldsn_drops = 0;
:- return 1;
:- }
:-
:- return 0;
:-}
:-
: /*
: * Handle re-ordering of frames which were de-aggregated in hardware.
: * Returns 1 if the MPDU was consumed (buffered or dropped).
:@@ -5034,8 +4888,6 @@ iwx_rx_reorder(struct iwx_softc *sc, struct mbuf *m, i
: int last_subframe =
: (desc->amsdu_info & IWX_RX_MPDU_AMSDU_LAST_SUBFRAME);
: uint8_t tid;
:- uint8_t subframe_idx = (desc->amsdu_info &
:- IWX_RX_MPDU_AMSDU_SUBFRAME_IDX_MASK);
: struct iwx_reorder_buf_entry *entries;
: int index;
: uint16_t nssn, sn;
:@@ -5100,39 +4952,19 @@ iwx_rx_reorder(struct iwx_softc *sc, struct mbuf *m, i
: goto drop;
: }
:
:- /*
:- * If there was a significant jump in the nssn - adjust.
:- * If the SN is smaller than the NSSN it might need to first go into
:- * the reorder buffer, in which case we just release up to it and the
:- * rest of the function will take care of storing it and releasing up to
:- * the nssn.
:- */
:- if (!iwx_is_sn_less(nssn, buffer->head_sn + buffer->buf_size,
:- buffer->buf_size) ||
:- !SEQ_LT(sn, buffer->head_sn + buffer->buf_size)) {
:- uint16_t min_sn = SEQ_LT(sn, nssn) ? sn : nssn;
:- ic->ic_stats.is_ht_rx_frame_above_ba_winend++;
:- iwx_release_frames(sc, ni, rxba, buffer, min_sn, ml);
:- }
:-
:- if (iwx_oldsn_workaround(sc, ni, tid, buffer, reorder_data,
:- device_timestamp)) {
:- /* BA session will be torn down. */
:- ic->ic_stats.is_ht_rx_ba_window_jump++;
:+ /* drop any duplicate packets */
:+ if (desc->status & htole32(IWX_RX_MPDU_RES_STATUS_DUPLICATE))
: goto drop;
:
:- }
:-
: /* drop any outdated packets */
:- if (SEQ_LT(sn, buffer->head_sn)) {
:+ if (reorder_data & IWX_RX_MPDU_REORDER_BA_OLD_SN) {
: ic->ic_stats.is_ht_rx_frame_below_ba_winstart++;
: goto drop;
: }
:
: /* release immediately if allowed by nssn and no stored frames */
: if (!buffer->num_stored && SEQ_LT(sn, nssn)) {
:- if (iwx_is_sn_less(buffer->head_sn, nssn, buffer->buf_size) &&
:- (!is_amsdu || last_subframe))
:+ if (!is_amsdu || last_subframe)
: buffer->head_sn = nssn;
: ieee80211_release_node(ic, ni);
: return 0;
:@@ -5155,24 +4987,7 @@ iwx_rx_reorder(struct iwx_softc *sc, struct mbuf *m, i
:
: index = sn % buffer->buf_size;
:
:- /*
:- * Check if we already stored this frame
:- * As AMSDU is either received or not as whole, logic is simple:
:- * If we have frames in that position in the buffer and the last frame
:- * originated from AMSDU had a different SN then it is a retransmission.
:- * If it is the same SN then if the subframe index is incrementing it
:- * is the same AMSDU - otherwise it is a retransmission.
:- */
:- if (!ml_empty(&entries[index].frames)) {
:- if (!is_amsdu) {
:- ic->ic_stats.is_ht_rx_ba_no_buf++;
:- goto drop;
:- } else if (sn != buffer->last_amsdu ||
:- buffer->last_sub_index >= subframe_idx) {
:- ic->ic_stats.is_ht_rx_ba_no_buf++;
:- goto drop;
:- }
:- } else {
:+ if (ml_empty(&entries[index].frames)) {
: /* This data is the same for all A-MSDU subframes. */
: entries[index].chanidx = chanidx;
: entries[index].is_shortpre = is_shortpre;
:@@ -5184,13 +4999,7 @@ iwx_rx_reorder(struct iwx_softc *sc, struct mbuf *m, i
: /* put in reorder buffer */
: ml_enqueue(&entries[index].frames, m);
: buffer->num_stored++;
:- getmicrouptime(&entries[index].reorder_time);
:
:- if (is_amsdu) {
:- buffer->last_amsdu = sn;
:- buffer->last_sub_index = subframe_idx;
:- }
:-
: /*
: * We cannot trust NSSN for AMSDU sub-frames that are not the last.
: * The reason is that NSSN advances on the first sub-frame, and may
:@@ -12308,8 +12117,6 @@ iwx_attach(struct device *parent, struct device *self,
: rxba->sc = sc;
: timeout_set(&rxba->session_timer, iwx_rx_ba_session_expired,
: rxba);
:- timeout_set(&rxba->reorder_buf.reorder_timer,
:- iwx_reorder_timer_expired, &rxba->reorder_buf);
: for (j = 0; j < nitems(rxba->entries); j++)
: ml_init(&rxba->entries[j].frames);
: }
:commit - bf3713f7dfbf19ede85da952a0d01b849aa8a252
:blob - b6d027fedb110fa27bf4696ba713e099273adddc
:file + sys/dev/pci/if_iwxreg.h
:--- sys/dev/pci/if_iwxreg.h
:+++ sys/dev/pci/if_iwxreg.h
:@@ -3632,6 +3632,7 @@ struct iwx_rx_mpdu_res_start {
: #define IWX_RX_MPDU_RES_STATUS_ROBUST_MNG_FRAME (1 << 15)
: #define IWX_RX_MPDU_RES_STATUS_HASH_INDEX_MSK (0x3F0000)
: #define IWX_RX_MPDU_RES_STATUS_STA_ID_MSK (0x1f000000)
:+#define IWX_RX_MPDU_RES_STATUS_DUPLICATE (1 << 22)
: #define IWX_RX_MPDU_RES_STATUS_RRF_KILL (1 << 29)
: #define IWX_RX_MPDU_RES_STATUS_FILTERING_MSK (0xc00000)
: #define IWX_RX_MPDU_RES_STATUS2_FILTERING_MSK (0xc0000000)
:commit - bf3713f7dfbf19ede85da952a0d01b849aa8a252
:blob - 7be82818bc54423e510492e7b7e8974a0f63000d
:file + sys/dev/pci/if_iwxvar.h
:--- sys/dev/pci/if_iwxvar.h
:+++ sys/dev/pci/if_iwxvar.h
:@@ -358,11 +358,6 @@ struct iwx_self_init_dram {
: * @num_stored: number of mpdus stored in the buffer
: * @buf_size: the reorder buffer size as set by the last addba request
: * @queue: queue of this reorder buffer
:- * @last_amsdu: track last ASMDU SN for duplication detection
:- * @last_sub_index: track ASMDU sub frame index for duplication detection
:- * @reorder_timer: timer for frames are in the reorder buffer. For AMSDU
:- * it is the time of last received sub-frame
:- * @removed: prevent timer re-arming
: * @valid: reordering is valid for this queue
: * @consec_oldsn_drops: consecutive drops due to old SN
: * @consec_oldsn_ampdu_gp2: A-MPDU GP2 timestamp to track
:@@ -375,25 +370,15 @@ struct iwx_reorder_buffer {
: uint16_t head_sn;
: uint16_t num_stored;
: uint16_t buf_size;
:- uint16_t last_amsdu;
:- uint8_t last_sub_index;
:- struct timeout reorder_timer;
:- int removed;
: int valid;
:- unsigned int consec_oldsn_drops;
:- uint32_t consec_oldsn_ampdu_gp2;
:- unsigned int consec_oldsn_prev_drop;
:-#define IWX_AMPDU_CONSEC_DROPS_DELBA 10
: };
:
: /**
: * struct iwx_reorder_buf_entry - reorder buffer entry per frame sequence number
: * @frames: list of mbufs stored (A-MSDU subframes share a sequence number)
:- * @reorder_time: time the packet was stored in the reorder buffer
: */
: struct iwx_reorder_buf_entry {
: struct mbuf_list frames;
:- struct timeval reorder_time;
: uint32_t rx_pkt_status;
: int chanidx;
: int is_shortpre;
:
--
If I had a plantation in Georgia and a home in Hell,
I'd sell the plantation and go home.
-- Eugene P. Gallagher
iwx: remove A-MPDU Rx reordering