From: Peter Hessler Subject: Re: iwx: remove A-MPDU Rx reordering To: tech@openbsd.org Date: Tue, 10 Mar 2026 01:54:33 +0100 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