[PATCH] wcn36xx: Add chained transfer support for AMSDU

[PATCH] wcn36xx: Add chained transfer support for AMSDU

[Loic Poulain]

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(-)

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,
-- 
2.7.4

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Bryan O'Donoghue]

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

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Kalle Valo]

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 :)

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/1634557705-11120-1-git-send-email-loic.poulain@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Loic Poulain]

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

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Bryan O'Donoghue]

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

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wcn36xx: Add chained transfer support for AMSDU

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/1634557705-11120-1-git-send-email-loic.poulain@linaro.org/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches