Sharing Sashiko review, at the first part of it was something I wondered before
resending the v2.
I'm not sure what exactly is achieved with the msdu & rxd assignment, as it
seems to be really immediately discarded.
Please excuse my lack of knowledge in the wireless field, it's just patch which
we carry for long time and it seems helps others projects using WCN3990 too
> - 0016: WCN3990 splits A-MSDUs across multiple HTT in-order
> indications; mainline dropped them ("failed to extract amsdu: -11",
> rx_confused), collapsing TCP under load (49 kB/s). Persist the
> in-order MSDU list in driver state.
https://github.com/commaai/vamOS/commit/12d80d09e61188895e90d9e4245d513ad4d07f25
Thanks
David
--- Patch [1]: [PATCH RFC v2] wifi: ath10k: make in-order rx amsdu buffers
persistent ---
commit 10e41b4de22d818aaaa807183134d42a4da09825
Author: Richard Acayan <[email protected]>
wifi: ath10k: make in-order rx amsdu buffers persistent
This commit moves the list tracking unprocessed MSDUs to the driver state,
allowing it to handle MPDUs that are split among multiple in-order
indications.
Link:
https://lore.kernel.org/linux-wireless/[email protected]/
> 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
[ ... ]
> @@ -3161,6 +3165,10 @@ static int ath10k_htt_rx_extract_amsdu(struct
ath10k_hw_params *hw,
> 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);
> +
This isn't a bug, but are these assignments to msdu and rxd unused?
They appear to be unconditionally overwritten in the while loop condition and
the first statement of the loop body without ever being read.
> while ((msdu = __skb_dequeue(list))) {
> __skb_queue_tail(amsdu, msdu);
[ ... ]
> @@ -3339,12 +3364,15 @@ static int ath10k_htt_rx_in_ord_ind(struct ath10k
*ar, struct sk_buff *skb)
> 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;
Will returning -EIO here prematurely abort the NAPI polling loop?
When a split AMSDU is received from the hardware and
ath10k_htt_rx_extract_amsdu() returns -EAGAIN, this -EIO return value is
propagated up to ath10k_htt_txrx_compl_task(), which interprets it as a
signal to abort the rx_in_ord_compl_q loop and reschedule NAPI.
This seems to leave any subsequent queued packets, potentially including the
remainder of the split AMSDU, unprocessed until the next NAPI poll.
> default:
> /* Should not happen. */
> ath10k_warn(ar, "failed to extract amsdu: %d\n", ret);
