On 7/19/2026 2:45 PM, David Heidelberg via B4 Relay wrote: > From: Richard Acayan <[email protected]> > > The WCN3990 might split MSDUs among multiple "in-order" indications. The > driver needs information from previous indications to handle MPDUs that > are not started by the same indications that complete them. Move the > list that tracks unprocessed MSDUs to the driver state so the driver can > handle MPDUs that are split in this way and be less confused. > > Fixes: c545070e404b ("ath10k: implement rx reorder support") > Signed-off-by: Richard Acayan <[email protected]> > Co-developed-by: David Heidelberg <[email protected]> > Signed-off-by: David Heidelberg <[email protected]> > --- > This one we have in-tree forever.
I'm transcribing a few comments from my review agent (which may overlap Sashiko). I have not vetted them for correctness. Hopefully I placed them at the correct spots! > > Changes in v2: > - checkpatch & style. (Jeff) > - Improve comments. > - Link to v1: > https://lore.kernel.org/linux-wireless/[email protected]/ > --- > drivers/net/wireless/ath/ath10k/htt.h | 4 +++ > drivers/net/wireless/ath/ath10k/htt_rx.c | 50 > +++++++++++++++++++++++++------- > 2 files changed, 43 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/wireless/ath/ath10k/htt.h > b/drivers/net/wireless/ath/ath10k/htt.h > index 25c6b2e2f81c8..07c3dc088f07b 100644 > --- a/drivers/net/wireless/ath/ath10k/htt.h > +++ b/drivers/net/wireless/ath/ath10k/htt.h > @@ -1924,16 +1924,20 @@ struct ath10k_htt { > > bool tx_mem_allocated; > const struct ath10k_htt_tx_ops *tx_ops; > const struct ath10k_htt_rx_ops *rx_ops; > bool disable_tx_comp; > bool bundle_tx; > struct sk_buff_head tx_req_head; > struct sk_buff_head tx_complete_head; > + > + u8 rx_in_ord_split_tid; > + u16 rx_in_ord_split_peer_id; > + struct sk_buff_head rx_in_ord_split; > }; > > struct ath10k_htt_tx_ops { > int (*htt_send_rx_ring_cfg)(struct ath10k_htt *htt); > int (*htt_send_frag_desc_bank_cfg)(struct ath10k_htt *htt); > int (*htt_alloc_frag_desc)(struct ath10k_htt *htt); > void (*htt_free_frag_desc)(struct ath10k_htt *htt); > int (*htt_tx)(struct ath10k_htt *htt, enum ath10k_hw_txrx_mode txmode, > diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c > b/drivers/net/wireless/ath/ath10k/htt_rx.c > index ab2d373b4750d..732bd3a2f9992 100644 > --- a/drivers/net/wireless/ath/ath10k/htt_rx.c > +++ b/drivers/net/wireless/ath/ath10k/htt_rx.c > @@ -291,16 +291,18 @@ void ath10k_htt_rx_free(struct ath10k_htt *htt) > return; > > timer_delete_sync(&htt->rx_ring.refill_retry_timer); > > skb_queue_purge(&htt->rx_msdus_q); > skb_queue_purge(&htt->rx_in_ord_compl_q); > skb_queue_purge(&htt->tx_fetch_ind_q); > > + skb_queue_purge(&htt->rx_in_ord_split); > + > spin_lock_bh(&htt->rx_ring.lock); > ath10k_htt_rx_ring_free(htt); > spin_unlock_bh(&htt->rx_ring.lock); > > dma_free_coherent(htt->ar->dev, > ath10k_htt_get_rx_ring_size(htt), > ath10k_htt_get_vaddr_ring(htt), > htt->rx_ring.base_paddr); > @@ -841,16 +843,18 @@ int ath10k_htt_rx_alloc(struct ath10k_htt *htt) > htt->rx_ring.sw_rd_idx.msdu_payld = 0; > hash_init(htt->rx_ring.skb_table); > > skb_queue_head_init(&htt->rx_msdus_q); > skb_queue_head_init(&htt->rx_in_ord_compl_q); > skb_queue_head_init(&htt->tx_fetch_ind_q); > atomic_set(&htt->num_mpdus_ready, 0); > > + skb_queue_head_init(&htt->rx_in_ord_split); > + > ath10k_dbg(ar, ATH10K_DBG_BOOT, "htt rx ring size %d fill_level %d\n", > htt->rx_ring.size, htt->rx_ring.fill_level); > return 0; > > err_dma_idx: > dma_free_coherent(htt->ar->dev, > ath10k_htt_get_rx_ring_size(htt), > vaddr_ring, > @@ -3156,16 +3160,20 @@ static int ath10k_htt_rx_extract_amsdu(struct > ath10k_hw_params *hw, > struct rx_msdu_end_common *rxd_msdu_end_common; > > if (skb_queue_empty(list)) > return -ENOBUFS; > > if (WARN_ON(!skb_queue_empty(amsdu))) > return -EINVAL; > > + msdu = skb_peek(list); > + rxd = HTT_RX_BUF_TO_RX_DESC(hw, > + (void *)msdu->data - > hw->rx_desc_ops->rx_desc_size); Dead rxd computation before the loop — VALID, MINOR Lines 3168–3170 compute rxd via skb_peek(), but the while loop at 3172 immediately dequeues the same SKB and unconditionally recomputes rxd at lines 3175–3177. The pre-loop assignment is never read. It's dead code. The original patch must have introduced this when restructuring (the pre-existing code likely used rxd from before the loop). It should be removed. > + > while ((msdu = __skb_dequeue(list))) { > __skb_queue_tail(amsdu, msdu); > > rxd = HTT_RX_BUF_TO_RX_DESC(hw, > (void *)msdu->data - > hw->rx_desc_ops->rx_desc_size); > > rxd_msdu_end_common = ath10k_htt_rx_desc_get_msdu_end(hw, rxd); > @@ -3257,17 +3265,16 @@ static void ath10k_htt_rx_h_rx_offload(struct ath10k > *ar, > } > } > > static int ath10k_htt_rx_in_ord_ind(struct ath10k *ar, struct sk_buff *skb) > { > struct ath10k_htt *htt = &ar->htt; > struct htt_resp *resp = (void *)skb->data; > struct ieee80211_rx_status *status = &htt->rx_status; > - struct sk_buff_head list; > struct sk_buff_head amsdu; > u16 peer_id; > u16 msdu_count; > u8 vdev_id; > u8 tid; > bool offload; > bool frag; > int ret; > @@ -3292,64 +3299,85 @@ static int ath10k_htt_rx_in_ord_ind(struct ath10k > *ar, struct sk_buff *skb) > "htt rx in ord vdev %i peer %i tid %i offload %i frag %i > msdu count %i\n", > vdev_id, peer_id, tid, offload, frag, msdu_count); > > if (skb->len < msdu_count * sizeof(*resp->rx_in_ord_ind.msdu_descs32)) { > ath10k_warn(ar, "dropping invalid in order rx indication\n"); > return -EINVAL; > } > > - /* The event can deliver more than 1 A-MSDU. Each A-MSDU is later > - * extracted and processed. > + if (!skb_queue_empty(&htt->rx_in_ord_split)) { > + /* > + * It might still be possible to handle this case if there is > + * only one peer that splits at each given moment. We are > + * bailing out because we should have a test case for this > + * before trying to fix it. > + */ > + if (tid != htt->rx_in_ord_split_tid || > + peer_id != htt->rx_in_ord_split_peer_id || > + offload) { Dead offload disjunct — VALID, MINOR ath10k_htt_rx_h_rx_offload() at line 3346 drains rx_in_ord_split entirely (via __skb_dequeue loop). After it runs, the while (!skb_queue_empty(...)) loop at 3348 never executes. Therefore a split (-EAGAIN from ath10k_htt_rx_extract_amsdu) can never occur for offload frames. The || offload condition at line 3316 in the split-validation guard is unreachable. It should be removed for clarity. > + ath10k_warn(ar, "split amsdu did not resume > immediately\n"); > + htt->rx_confused = true; > + ath10k_core_start_recovery(ar); > + return -EIO; > + } > + } > + > + /* > + * The event can deliver more than 1 A-MSDU or continue a previous one. > + * Each A-MSDU is later extracted and processed. > */ > - __skb_queue_head_init(&list); > if (ar->hw_params.target_64bit) > ret = ath10k_htt_rx_pop_paddr64_list(htt, &resp->rx_in_ord_ind, > - &list); > + &htt->rx_in_ord_split); > else > ret = ath10k_htt_rx_pop_paddr32_list(htt, &resp->rx_in_ord_ind, > - &list); > + &htt->rx_in_ord_split); > > if (ret < 0) { > ath10k_warn(ar, "failed to pop paddr list: %d\n", ret); > htt->rx_confused = true; > ath10k_core_start_recovery(ar); > return -EIO; rx_in_ord_split not purged on pop_paddr error — VALID BUG Lines 3335–3339: on pop_paddr error, htt->rx_confused = true and ath10k_core_start_recovery() are called, but __skb_queue_purge(&htt->rx_in_ord_split) is missing. If a split is already in progress (queue is non-empty) when pop_paddr fails on the continuation event, the existing SKBs are stranded. The default: path at lines 3371–3377 does purge the queue, making this an inconsistency. This will leak DMA-mapped SKBs. > } > > /* Offloaded frames are very different and need to be handled > * separately. > */ > if (offload) > - ath10k_htt_rx_h_rx_offload(ar, &list); > + ath10k_htt_rx_h_rx_offload(ar, &htt->rx_in_ord_split); > > - while (!skb_queue_empty(&list)) { > + while (!skb_queue_empty(&htt->rx_in_ord_split)) { > __skb_queue_head_init(&amsdu); > - ret = ath10k_htt_rx_extract_amsdu(&ar->hw_params, &list, > &amsdu); > + ret = ath10k_htt_rx_extract_amsdu(&ar->hw_params, > + &htt->rx_in_ord_split, > &amsdu); > switch (ret) { > case 0: > /* Note: The in-order indication may report interleaved > * frames from different PPDUs meaning reported rx rate > * to mac80211 isn't accurate/reliable. It's still > * better to report something than nothing though. This > * should still give an idea about rx rate to the user. > */ > ath10k_htt_rx_h_ppdu(ar, &amsdu, status, vdev_id); > ath10k_htt_rx_h_filter(ar, &amsdu, status, NULL); > ath10k_htt_rx_h_mpdu(ar, &amsdu, status, false, NULL, > NULL, peer_id, frag); > ath10k_htt_rx_h_enqueue(ar, &amsdu, status); > break; > case -EAGAIN: > - fallthrough; > + htt->rx_in_ord_split_tid = tid; > + htt->rx_in_ord_split_peer_id = peer_id; > + > + return -EIO; frag flag not saved across -EAGAIN — VALID BUG frag is a local extracted from the current event's header (line 3296). On -EAGAIN (line 3366–3370), only tid and peer_id are saved. When the continuation event arrives, ath10k_htt_rx_h_mpdu() at line 3362 is called with the frag from the new event, not the original split event. If the original said frag=1, the fragments will be misprocessed by mac80211 on continuation. This needs rx_in_ord_split_frag added alongside _tid and _peer_id. > default: > /* Should not happen. */ > ath10k_warn(ar, "failed to extract amsdu: %d\n", ret); > htt->rx_confused = true; > - __skb_queue_purge(&list); > + __skb_queue_purge(&htt->rx_in_ord_split); > ath10k_core_start_recovery(ar); > return -EIO; > } > } > return ret; > } > > static void ath10k_htt_rx_tx_fetch_resp_id_confirm(struct ath10k *ar, > > --- > base-commit: 0718283ab28bc3907e10b61a6b4be6fefa1cbb2f > change-id: 20260719-ath10k-a-msdu-c49334eb091d > > Best regards,
