Download raw body.
iwx: remove A-MPDU Rx reordering
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?
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;
iwx: remove A-MPDU Rx reordering