| Message ID | 1634557705-11120-1-git-send-email-loic.poulain@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a224b47ab36d7db5fb5d410622777fd10794f4cd |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wcn36xx: Add chained transfer support for AMSDU | expand |
On 18/10/2021 12:48, Loic Poulain wrote: > WCNSS RX DMA transfer support is limited to 3872 bytes, which is > enough for simple MPDUs (single MSDU), but not enough for cases > with A-MSDU (depending on max AMSDU size or max MPDU size). > > In that case the MPDU is spread ove multiple transfers, with the > first transfer containing the MPDU header and (at least) the first > A-MSDU subframe and additional transfer(s) containing the following > A-MSDUs. This can be handled with a series of flags to tagging the > first and last A-MSDU transfers. > > In that case we have to bufferize and re-linearize the A-MSDU buffers > into a proper MPDU skb before forwarding to mac80211 (in the same way > as it is done in ath10k). > > This change also includes sanity check of the buffer descriptor to > prevent skb overflow. I like this patch and I think it plugs a gap we have but, my question is, have we seen this code be triggered at least once ? I'm a bit reticent about apply it upstream unless/until we have. --- bod
Loic Poulain <loic.poulain@linaro.org> wrote: > WCNSS RX DMA transfer support is limited to 3872 bytes, which is > enough for simple MPDUs (single MSDU), but not enough for cases > with A-MSDU (depending on max AMSDU size or max MPDU size). > > In that case the MPDU is spread over multiple transfers, with the > first transfer containing the MPDU header and (at least) the first > A-MSDU subframe and additional transfer(s) containing the following > A-MSDUs. This can be handled with a series of flags to tagging the > first and last A-MSDU transfers. > > In that case we have to bufferize and re-linearize the A-MSDU buffers > into a proper MPDU skb before forwarding to mac80211 (in the same way > as it is done in ath10k). > > This change also includes sanity check of the buffer descriptor to > prevent skb overflow. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Loic, what should I do with this patch? Bryan is hesitant but I would be fine to take this. You have to decide :)
On 25/10/2021 16:46, Loic Poulain wrote: > But it's better than the previous situation IMHO, since we check the > buffer descriptor to prevent skb breakage. Let's apply it and see how we go.. --- bod
Hi Kalle, On Mon, 25 Oct 2021 at 15:22, Kalle Valo <kvalo@codeaurora.org> wrote: > > Loic Poulain <loic.poulain@linaro.org> wrote: > > > WCNSS RX DMA transfer support is limited to 3872 bytes, which is > > enough for simple MPDUs (single MSDU), but not enough for cases > > with A-MSDU (depending on max AMSDU size or max MPDU size). > > > > In that case the MPDU is spread over multiple transfers, with the > > first transfer containing the MPDU header and (at least) the first > > A-MSDU subframe and additional transfer(s) containing the following > > A-MSDUs. This can be handled with a series of flags to tagging the > > first and last A-MSDU transfers. > > > > In that case we have to bufferize and re-linearize the A-MSDU buffers > > into a proper MPDU skb before forwarding to mac80211 (in the same way > > as it is done in ath10k). > > > > This change also includes sanity check of the buffer descriptor to > > prevent skb overflow. > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> > > Loic, what should I do with this patch? Bryan is hesitant but I > would be fine to take this. You have to decide :) So the point is that we had some skb overflows reported in the field, likely due to wcn36xx_rx buffer descriptor parsing, and possibly due to the A-MSDU split. No such issues have been reported since we applied that change. But I've not validated myself that the A-MSDU re-aggregation procedure is functional (though it's mostly copied from ath10k), since it can happen only in very specific cases, i.e when the MPDU (including the A-MSDU) size is larger than the 3872-byte DMA buffer and smaller than 3895 bytes (which is our max VHT MPDU length). But it's better than the previous situation IMHO, since we check the buffer descriptor to prevent skb breakage. Regards, Loic
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes: > On 25/10/2021 16:46, Loic Poulain wrote: >> But it's better than the previous situation IMHO, since we check the >> buffer descriptor to prevent skb breakage. > > Let's apply it and see how we go.. Sounds good to me. And we can revert it if doesn't go well.
Loic Poulain <loic.poulain@linaro.org> wrote: > WCNSS RX DMA transfer support is limited to 3872 bytes, which is > enough for simple MPDUs (single MSDU), but not enough for cases > with A-MSDU (depending on max AMSDU size or max MPDU size). > > In that case the MPDU is spread over multiple transfers, with the > first transfer containing the MPDU header and (at least) the first > A-MSDU subframe and additional transfer(s) containing the following > A-MSDUs. This can be handled with a series of flags to tagging the > first and last A-MSDU transfers. > > In that case we have to bufferize and re-linearize the A-MSDU buffers > into a proper MPDU skb before forwarding to mac80211 (in the same way > as it is done in ath10k). > > This change also includes sanity check of the buffer descriptor to > prevent skb overflow. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Patch applied to ath-next branch of ath.git, thanks. a224b47ab36d wcn36xx: Add chained transfer support for AMSDU
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c index b5aa20e..96bd99a 100644 --- a/drivers/net/wireless/ath/wcn36xx/main.c +++ b/drivers/net/wireless/ath/wcn36xx/main.c @@ -1501,6 +1501,7 @@ static int wcn36xx_probe(struct platform_device *pdev) mutex_init(&wcn->conf_mutex); mutex_init(&wcn->hal_mutex); mutex_init(&wcn->scan_lock); + __skb_queue_head_init(&wcn->amsdu); wcn->hal_buf = devm_kmalloc(wcn->dev, WCN36XX_HAL_BUF_SIZE, GFP_KERNEL); if (!wcn->hal_buf) { @@ -1578,6 +1579,8 @@ static int wcn36xx_remove(struct platform_device *pdev) iounmap(wcn->dxe_base); iounmap(wcn->ccu_base); + __skb_queue_purge(&wcn->amsdu); + mutex_destroy(&wcn->hal_mutex); ieee80211_free_hw(hw); diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c index 3979171..3cecc8f 100644 --- a/drivers/net/wireless/ath/wcn36xx/smd.c +++ b/drivers/net/wireless/ath/wcn36xx/smd.c @@ -266,7 +266,8 @@ static void wcn36xx_smd_set_sta_ht_params(struct ieee80211_sta *sta, sta_params->max_ampdu_size = sta->ht_cap.ampdu_factor; sta_params->max_ampdu_density = sta->ht_cap.ampdu_density; - sta_params->max_amsdu_size = is_cap_supported(caps, + /* max_amsdu_size: 1 : 3839 bytes, 0 : 7935 bytes (max) */ + sta_params->max_amsdu_size = !is_cap_supported(caps, IEEE80211_HT_CAP_MAX_AMSDU); sta_params->sgi_20Mhz = is_cap_supported(caps, IEEE80211_HT_CAP_SGI_20); diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c index a3ef497..1edc703 100644 --- a/drivers/net/wireless/ath/wcn36xx/txrx.c +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c @@ -231,6 +231,41 @@ static const struct wcn36xx_rate wcn36xx_rate_table[] = { { 4333, 9, RX_ENC_VHT, RX_ENC_FLAG_SHORT_GI, RATE_INFO_BW_80 }, }; +static struct sk_buff *wcn36xx_unchain_msdu(struct sk_buff_head *amsdu) +{ + struct sk_buff *skb, *first; + int total_len = 0; + int space; + + first = __skb_dequeue(amsdu); + + skb_queue_walk(amsdu, skb) + total_len += skb->len; + + space = total_len - skb_tailroom(first); + if (space > 0 && pskb_expand_head(first, 0, space, GFP_ATOMIC) < 0) { + __skb_queue_head(amsdu, first); + return NULL; + } + + /* Walk list again, copying contents into msdu_head */ + while ((skb = __skb_dequeue(amsdu))) { + skb_copy_from_linear_data(skb, skb_put(first, skb->len), + skb->len); + dev_kfree_skb_irq(skb); + } + + return first; +} + +static void __skb_queue_purge_irq(struct sk_buff_head *list) +{ + struct sk_buff *skb; + + while ((skb = __skb_dequeue(list)) != NULL) + dev_kfree_skb_irq(skb); +} + int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb) { struct ieee80211_rx_status status; @@ -252,6 +287,26 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb) "BD <<< ", (char *)bd, sizeof(struct wcn36xx_rx_bd)); + if (bd->pdu.mpdu_data_off <= bd->pdu.mpdu_header_off || + bd->pdu.mpdu_len < bd->pdu.mpdu_header_len) + goto drop; + + if (bd->asf && !bd->esf) { /* chained A-MSDU chunks */ + /* Sanity check */ + if (bd->pdu.mpdu_data_off + bd->pdu.mpdu_len > WCN36XX_PKT_SIZE) + goto drop; + + skb_put(skb, bd->pdu.mpdu_data_off + bd->pdu.mpdu_len); + skb_pull(skb, bd->pdu.mpdu_data_off); + + /* Only set status for first chained BD (with mac header) */ + goto done; + } + + if (bd->pdu.mpdu_header_off < sizeof(*bd) || + bd->pdu.mpdu_header_off + bd->pdu.mpdu_len > WCN36XX_PKT_SIZE) + goto drop; + skb_put(skb, bd->pdu.mpdu_header_off + bd->pdu.mpdu_len); skb_pull(skb, bd->pdu.mpdu_header_off); @@ -328,9 +383,37 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb) (char *)skb->data, skb->len); } +done: + /* Chained AMSDU ? slow path */ + if (unlikely(bd->asf && !(bd->lsf && bd->esf))) { + if (bd->esf && !skb_queue_empty(&wcn->amsdu)) { + wcn36xx_err("Discarding non complete chain"); + __skb_queue_purge_irq(&wcn->amsdu); + } + + __skb_queue_tail(&wcn->amsdu, skb); + + if (!bd->lsf) + return 0; /* Not the last AMSDU, wait for more */ + + skb = wcn36xx_unchain_msdu(&wcn->amsdu); + if (!skb) + goto drop; + } + ieee80211_rx_irqsafe(wcn->hw, skb); return 0; + +drop: /* drop everything */ + wcn36xx_err("Drop frame! skb:%p len:%u hoff:%u doff:%u asf=%u esf=%u lsf=%u\n", + skb, bd->pdu.mpdu_len, bd->pdu.mpdu_header_off, + bd->pdu.mpdu_data_off, bd->asf, bd->esf, bd->lsf); + + dev_kfree_skb_irq(skb); + __skb_queue_purge_irq(&wcn->amsdu); + + return -EINVAL; } static void wcn36xx_set_tx_pdu(struct wcn36xx_tx_bd *bd, diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h index add6e52..ae63bc6 100644 --- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h +++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h @@ -269,6 +269,9 @@ struct wcn36xx { struct sk_buff *tx_ack_skb; struct timer_list tx_ack_timer; + /* For A-MSDU re-aggregation */ + struct sk_buff_head amsdu; + /* RF module */ unsigned rf_id; @@ -276,7 +279,6 @@ struct wcn36xx { /* Debug file system entry */ struct wcn36xx_dfs_entry dfs; #endif /* CONFIG_WCN36XX_DEBUGFS */ - }; static inline bool wcn36xx_is_fw_version(struct wcn36xx *wcn,
WCNSS RX DMA transfer support is limited to 3872 bytes, which is enough for simple MPDUs (single MSDU), but not enough for cases with A-MSDU (depending on max AMSDU size or max MPDU size). In that case the MPDU is spread ove multiple transfers, with the first transfer containing the MPDU header and (at least) the first A-MSDU subframe and additional transfer(s) containing the following A-MSDUs. This can be handled with a series of flags to tagging the first and last A-MSDU transfers. In that case we have to bufferize and re-linearize the A-MSDU buffers into a proper MPDU skb before forwarding to mac80211 (in the same way as it is done in ath10k). This change also includes sanity check of the buffer descriptor to prevent skb overflow. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- drivers/net/wireless/ath/wcn36xx/main.c | 3 ++ drivers/net/wireless/ath/wcn36xx/smd.c | 3 +- drivers/net/wireless/ath/wcn36xx/txrx.c | 83 ++++++++++++++++++++++++++++++ drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 4 +- 4 files changed, 91 insertions(+), 2 deletions(-)