| Message ID | 20240329092432.873710-1-quic_nithp@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: ath12k: fix mac id extraction when MSDU spillover in rx error path | expand |
On 3/29/2024 2:24 AM, Nithyanantham P wrote: > From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > > Currently, in the rx error data path, mac id is extracted from the > last 64bits of MSDU end description tag for each entry received in > the WBM error ring. Then, each entry will be updated into the MSDU > list for further processing. The extracted mac id is valid when a > single MSDU is not fragmented and received in the WBM error ring. > > In scenarios where the size of a single MSDU received exceeds the > descriptor buffer size, resulting in fragmented or spillover MSDU > entries into the WBM error ring. In this case, the extracted mac id > from each spillover entry is invalid except the last spillover entry > of the MSDU. This invalid mac id leads to packet rejection. > > To address this issue, check if the MSDU continuation flag is set, > then extract the valid mac id from the last spillover entry. > Propagate the valid mac id to all the spillover entries of the single > MSDU in the temporary MSDU list(scatter_msdu_list). Then, update this > into the MSDU list (msdu_list) for further processing. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > Signed-off-by: Nithyanantham P <quic_nithp@quicinc.com> Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>
Nithyanantham P <quic_nithp@quicinc.com> writes: > From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > > Currently, in the rx error data path, mac id is extracted from the > last 64bits of MSDU end description tag for each entry received in > the WBM error ring. Then, each entry will be updated into the MSDU > list for further processing. The extracted mac id is valid when a > single MSDU is not fragmented and received in the WBM error ring. > > In scenarios where the size of a single MSDU received exceeds the > descriptor buffer size, resulting in fragmented or spillover MSDU > entries into the WBM error ring. In this case, the extracted mac id > from each spillover entry is invalid except the last spillover entry > of the MSDU. This invalid mac id leads to packet rejection. > > To address this issue, check if the MSDU continuation flag is set, > then extract the valid mac id from the last spillover entry. > Propagate the valid mac id to all the spillover entries of the single > MSDU in the temporary MSDU list(scatter_msdu_list). Then, update this > into the MSDU list (msdu_list) for further processing. > > Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> > Signed-off-by: Nithyanantham P <quic_nithp@quicinc.com> [...] > @@ -3807,16 +3816,50 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab, > continue; > } > > + msdu_data = (struct hal_rx_desc *)msdu->data; > rxcb->err_rel_src = err_info.err_rel_src; > rxcb->err_code = err_info.err_code; > - rxcb->rx_desc = (struct hal_rx_desc *)msdu->data; > - > - __skb_queue_tail(&msdu_list, msdu); > - > rxcb->is_first_msdu = err_info.first_msdu; > rxcb->is_last_msdu = err_info.last_msdu; > rxcb->is_continuation = err_info.continuation; > + rxcb->rx_desc = msdu_data; > + > + if (err_info.continuation) { > + __skb_queue_tail(&scatter_msdu_list, msdu); > + } else { > + mac_id = ath12k_dp_rx_get_msdu_src_link(ab, > + msdu_data); > + if (mac_id >= MAX_RADIOS) { > + dev_kfree_skb_any(msdu); > + > + /* In any case continuation bit is set > + * in the previous record, cleanup scatter_msdu_list > + */ > + ath12k_dp_clean_up_skb_list(&scatter_msdu_list); > + continue; > + } > + > + if (!skb_queue_empty(&scatter_msdu_list)) { > + struct sk_buff *msdu; > + > + skb_queue_walk(&scatter_msdu_list, msdu) { > + rxcb = ATH12K_SKB_RXCB(msdu); > + rxcb->mac_id = mac_id; > + } > + > + skb_queue_splice_tail_init(&scatter_msdu_list, > + &msdu_list); > + } > + > + rxcb = ATH12K_SKB_RXCB(msdu); > + rxcb->mac_id = mac_id; > + __skb_queue_tail(&msdu_list, msdu); > + } > } The else branch can be avoided with continue. I did that in the pending branch, please check my changes: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=6c677d91adad4817e7f6ef65a85331f52f0237ee
On 4/11/2024 2:33 AM, Kalle Valo wrote: [...] > The else branch can be avoided with continue. I did that in the pending > branch, please check my changes: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=6c677d91adad4817e7f6ef65a85331f52f0237ee > LGTM
On 4/11/2024 3:03 PM, Kalle Valo wrote: > Nithyanantham P <quic_nithp@quicinc.com> writes: > >> From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >> >> Currently, in the rx error data path, mac id is extracted from the >> last 64bits of MSDU end description tag for each entry received in >> the WBM error ring. Then, each entry will be updated into the MSDU >> list for further processing. The extracted mac id is valid when a >> single MSDU is not fragmented and received in the WBM error ring. >> >> In scenarios where the size of a single MSDU received exceeds the >> descriptor buffer size, resulting in fragmented or spillover MSDU >> entries into the WBM error ring. In this case, the extracted mac id >> from each spillover entry is invalid except the last spillover entry >> of the MSDU. This invalid mac id leads to packet rejection. >> >> To address this issue, check if the MSDU continuation flag is set, >> then extract the valid mac id from the last spillover entry. >> Propagate the valid mac id to all the spillover entries of the single >> MSDU in the temporary MSDU list(scatter_msdu_list). Then, update this >> into the MSDU list (msdu_list) for further processing. >> >> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1 >> >> Signed-off-by: Karthikeyan Periyasamy <quic_periyasa@quicinc.com> >> Signed-off-by: Nithyanantham P <quic_nithp@quicinc.com> > > [...] > >> @@ -3807,16 +3816,50 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab, >> continue; >> } >> >> + msdu_data = (struct hal_rx_desc *)msdu->data; >> rxcb->err_rel_src = err_info.err_rel_src; >> rxcb->err_code = err_info.err_code; >> - rxcb->rx_desc = (struct hal_rx_desc *)msdu->data; >> - >> - __skb_queue_tail(&msdu_list, msdu); >> - >> rxcb->is_first_msdu = err_info.first_msdu; >> rxcb->is_last_msdu = err_info.last_msdu; >> rxcb->is_continuation = err_info.continuation; >> + rxcb->rx_desc = msdu_data; >> + >> + if (err_info.continuation) { >> + __skb_queue_tail(&scatter_msdu_list, msdu); >> + } else { >> + mac_id = ath12k_dp_rx_get_msdu_src_link(ab, >> + msdu_data); >> + if (mac_id >= MAX_RADIOS) { >> + dev_kfree_skb_any(msdu); >> + >> + /* In any case continuation bit is set >> + * in the previous record, cleanup scatter_msdu_list >> + */ >> + ath12k_dp_clean_up_skb_list(&scatter_msdu_list); >> + continue; >> + } >> + >> + if (!skb_queue_empty(&scatter_msdu_list)) { >> + struct sk_buff *msdu; >> + >> + skb_queue_walk(&scatter_msdu_list, msdu) { >> + rxcb = ATH12K_SKB_RXCB(msdu); >> + rxcb->mac_id = mac_id; >> + } >> + >> + skb_queue_splice_tail_init(&scatter_msdu_list, >> + &msdu_list); >> + } >> + >> + rxcb = ATH12K_SKB_RXCB(msdu); >> + rxcb->mac_id = mac_id; >> + __skb_queue_tail(&msdu_list, msdu); >> + } >> } > > The else branch can be avoided with continue. I did that in the pending > branch, please check my changes: > > https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=6c677d91adad4817e7f6ef65a85331f52f0237ee > LGTM.
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c index a593beecdd12..b1b50e14a492 100644 --- a/drivers/net/wireless/ath/ath12k/dp_rx.c +++ b/drivers/net/wireless/ath/ath12k/dp_rx.c @@ -239,6 +239,14 @@ static inline u8 ath12k_dp_rx_get_msdu_src_link(struct ath12k_base *ab, return ab->hal_rx_ops->rx_desc_get_msdu_src_link_id(desc); } +static void ath12k_dp_clean_up_skb_list(struct sk_buff_head *skb_list) +{ + struct sk_buff *skb; + + while ((skb = __skb_dequeue(skb_list))) + dev_kfree_skb_any(skb); +} + static int ath12k_dp_purge_mon_ring(struct ath12k_base *ab) { int i, reaped = 0; @@ -3742,19 +3750,20 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab, struct hal_rx_wbm_rel_info err_info; struct hal_srng *srng; struct sk_buff *msdu; - struct sk_buff_head msdu_list; + struct sk_buff_head msdu_list, scatter_msdu_list; struct ath12k_skb_rxcb *rxcb; void *rx_desc; u8 mac_id; int num_buffs_reaped = 0; struct ath12k_rx_desc_info *desc_info; int ret, pdev_id; + struct hal_rx_desc *msdu_data; __skb_queue_head_init(&msdu_list); + __skb_queue_head_init(&scatter_msdu_list); srng = &ab->hal.srng_list[dp->rx_rel_ring.ring_id]; rx_ring = &dp->rx_refill_buf_ring; - spin_lock_bh(&srng->lock); ath12k_hal_srng_access_begin(ab, srng); @@ -3807,16 +3816,50 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab, continue; } + msdu_data = (struct hal_rx_desc *)msdu->data; rxcb->err_rel_src = err_info.err_rel_src; rxcb->err_code = err_info.err_code; - rxcb->rx_desc = (struct hal_rx_desc *)msdu->data; - - __skb_queue_tail(&msdu_list, msdu); - rxcb->is_first_msdu = err_info.first_msdu; rxcb->is_last_msdu = err_info.last_msdu; rxcb->is_continuation = err_info.continuation; + rxcb->rx_desc = msdu_data; + + if (err_info.continuation) { + __skb_queue_tail(&scatter_msdu_list, msdu); + } else { + mac_id = ath12k_dp_rx_get_msdu_src_link(ab, + msdu_data); + if (mac_id >= MAX_RADIOS) { + dev_kfree_skb_any(msdu); + + /* In any case continuation bit is set + * in the previous record, cleanup scatter_msdu_list + */ + ath12k_dp_clean_up_skb_list(&scatter_msdu_list); + continue; + } + + if (!skb_queue_empty(&scatter_msdu_list)) { + struct sk_buff *msdu; + + skb_queue_walk(&scatter_msdu_list, msdu) { + rxcb = ATH12K_SKB_RXCB(msdu); + rxcb->mac_id = mac_id; + } + + skb_queue_splice_tail_init(&scatter_msdu_list, + &msdu_list); + } + + rxcb = ATH12K_SKB_RXCB(msdu); + rxcb->mac_id = mac_id; + __skb_queue_tail(&msdu_list, msdu); + } } + /* In any case continuation bit is set in the + * last record, cleanup scatter_msdu_list + */ + ath12k_dp_clean_up_skb_list(&scatter_msdu_list); ath12k_hal_srng_access_end(ab, srng); @@ -3830,8 +3873,9 @@ int ath12k_dp_rx_process_wbm_err(struct ath12k_base *ab, rcu_read_lock(); while ((msdu = __skb_dequeue(&msdu_list))) { - mac_id = ath12k_dp_rx_get_msdu_src_link(ab, - (struct hal_rx_desc *)msdu->data); + rxcb = ATH12K_SKB_RXCB(msdu); + mac_id = rxcb->mac_id; + pdev_id = ath12k_hw_mac_id_to_pdev_id(ab->hw_params, mac_id); ar = ab->pdevs[pdev_id].ar;