* [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
@ 2026-08-31 17:39 Georgios Karantzas
2026-09-09 11:49 ` Toke Høiland-Jørgensen
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Georgios Karantzas @ 2026-08-31 17:39 UTC (permalink / raw)
To: toke; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
__hif_usb_tx() dequeues up to MAX_TX_AGGR_NUM (20) frames into a single tx_buf of MAX_TX_BUF_SIZE (32768) bytes, limiting the batch by record count but never by cumulative byte length.
With large frames (MTU 2304), 20 aggregated frames of 2292 bytes each exceed the allocation (20 * 2296 = 45920 bytes), so the memcpy() in the loop writes up to 13152 bytes past tx_buf->buf before usb_submit_urb().
Peek the queue head and stop before copying any record that would cross MAX_TX_BUF_SIZE, then dispatch the current batch. Leftover skbs remain queued and are drained on the next URB completion.
Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
---
drivers/net/wireless/ath/ath9k/hif_usb.c | 29 +++++++++++++-----------
1 file changed, 16 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 0a3d2190b..3e9d0c59b 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -328,32 +328,35 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
for (i = 0; i < tx_skb_cnt; i++) {
- nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
+ nskb = skb_peek(&hif_dev->tx.tx_skb_queue);
+ if (!nskb)
+ break;
- /* Should never be NULL */
- BUG_ON(!nskb);
+ if (tx_buf->offset + nskb->len + 4 > MAX_TX_BUF_SIZE)
+ break;
+ nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
hif_dev->tx.tx_skb_cnt--;
- buf = tx_buf->buf;
- buf += tx_buf->offset;
+ buf = tx_buf->buf + tx_buf->offset;
hdr = (__le16 *)buf;
*hdr++ = cpu_to_le16(nskb->len);
*hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
- buf += 4;
- memcpy(buf, nskb->data, nskb->len);
- tx_buf->len = nskb->len + 4;
-
- if (i < (tx_skb_cnt - 1))
- tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
+ memcpy(buf + 4, nskb->data, nskb->len);
- if (i == (tx_skb_cnt - 1))
- tx_buf->len += tx_buf->offset;
+ tx_buf->len = tx_buf->offset + nskb->len + 4;
+ tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
__skb_queue_tail(&tx_buf->skb_queue, nskb);
TX_STAT_INC(hif_dev, skb_queued);
}
+ if (!i) {
+ list_move_tail(&tx_buf->list, &hif_dev->tx.tx_buf);
+ hif_dev->tx.tx_buf_cnt++;
+ return 0;
+ }
+
usb_fill_bulk_urb(tx_buf->urb, hif_dev->udev,
usb_sndbulkpipe(hif_dev->udev, USB_WLAN_TX_PIPE),
tx_buf->buf, tx_buf->len,
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
2026-08-31 17:39 [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE Georgios Karantzas
@ 2026-09-09 11:49 ` Toke Høiland-Jørgensen
2026-09-09 18:59 ` [PATCH v2] " Georgios Karantzas
2026-09-11 0:11 ` [PATCH v3] " Georgios Karantzas
2 siblings, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2026-09-09 11:49 UTC (permalink / raw)
To: Georgios Karantzas; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
Georgios Karantzas <gck.kara@gmail.com> writes:
> __hif_usb_tx() dequeues up to MAX_TX_AGGR_NUM (20) frames into a single tx_buf of MAX_TX_BUF_SIZE (32768) bytes, limiting the batch by record count but never by cumulative byte length.
>
> With large frames (MTU 2304), 20 aggregated frames of 2292 bytes each exceed the allocation (20 * 2296 = 45920 bytes), so the memcpy() in the loop writes up to 13152 bytes past tx_buf->buf before usb_submit_urb().
>
> Peek the queue head and stop before copying any record that would cross MAX_TX_BUF_SIZE, then dispatch the current batch. Leftover skbs remain queued and are drained on the next URB completion.
>
> Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
Please wrap the commit message at 72 characters.
> ---
> drivers/net/wireless/ath/ath9k/hif_usb.c | 29 +++++++++++++-----------
> 1 file changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 0a3d2190b..3e9d0c59b 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -328,32 +328,35 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
> tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
>
> for (i = 0; i < tx_skb_cnt; i++) {
> - nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
> + nskb = skb_peek(&hif_dev->tx.tx_skb_queue);
> + if (!nskb)
> + break;
>
> - /* Should never be NULL */
> - BUG_ON(!nskb);
> + if (tx_buf->offset + nskb->len + 4 > MAX_TX_BUF_SIZE)
> + break;
>
> + nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
> hif_dev->tx.tx_skb_cnt--;
This bit is fine...
> - buf = tx_buf->buf;
> - buf += tx_buf->offset;
> + buf = tx_buf->buf + tx_buf->offset;
> hdr = (__le16 *)buf;
> *hdr++ = cpu_to_le16(nskb->len);
> *hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
> - buf += 4;
> - memcpy(buf, nskb->data, nskb->len);
> - tx_buf->len = nskb->len + 4;
> -
> - if (i < (tx_skb_cnt - 1))
> - tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
> + memcpy(buf + 4, nskb->data, nskb->len);
>
> - if (i == (tx_skb_cnt - 1))
> - tx_buf->len += tx_buf->offset;
> + tx_buf->len = tx_buf->offset + nskb->len + 4;
> + tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
But this bit seems unrelated (and wrong?); please drop.
> __skb_queue_tail(&tx_buf->skb_queue, nskb);
> TX_STAT_INC(hif_dev, skb_queued);
> }
>
> + if (!i) {
> + list_move_tail(&tx_buf->list, &hif_dev->tx.tx_buf);
> + hif_dev->tx.tx_buf_cnt++;
> + return 0;
> + }
It's not possible for a single packet to overflow the buffer size, so
we'll never hit this. So let's not add a pointless check.
-Toke
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
2026-08-31 17:39 [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE Georgios Karantzas
2026-09-09 11:49 ` Toke Høiland-Jørgensen
@ 2026-09-09 18:59 ` Georgios Karantzas
2026-09-10 18:25 ` Toke Høiland-Jørgensen
2026-09-11 0:11 ` [PATCH v3] " Georgios Karantzas
2 siblings, 1 reply; 6+ messages in thread
From: Georgios Karantzas @ 2026-09-09 18:59 UTC (permalink / raw)
To: toke; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
Bound the TX batch by cumulative byte length, not just record count.
Fixes: fb9987d0f748c983 ("ath9k_htc: Support for AR9271 chipset.")
Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
---
v2:
- 72-col wrap + Fixes: tag.
- Accounting kept: deferred len += offset never fires on early break.
- (!i) guard dropped per review.
drivers/net/wireless/ath/ath9k/hif_usb.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 0a3d2190b..533e74565 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -328,11 +328,14 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
for (i = 0; i < tx_skb_cnt; i++) {
- nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
+ nskb = skb_peek(&hif_dev->tx.tx_skb_queue);
+ if (!nskb)
+ break;
- /* Should never be NULL */
- BUG_ON(!nskb);
+ if (tx_buf->offset + nskb->len + 4 > MAX_TX_BUF_SIZE)
+ break;
+ nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
hif_dev->tx.tx_skb_cnt--;
buf = tx_buf->buf;
@@ -342,13 +345,8 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
*hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
buf += 4;
memcpy(buf, nskb->data, nskb->len);
- tx_buf->len = nskb->len + 4;
-
- if (i < (tx_skb_cnt - 1))
- tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
-
- if (i == (tx_skb_cnt - 1))
- tx_buf->len += tx_buf->offset;
+ tx_buf->len = tx_buf->offset + nskb->len + 4;
+ tx_buf->offset += (((nskb->len + 3) / 4) + 1) * 4;
__skb_queue_tail(&tx_buf->skb_queue, nskb);
TX_STAT_INC(hif_dev, skb_queued);
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
2026-09-09 18:59 ` [PATCH v2] " Georgios Karantzas
@ 2026-09-10 18:25 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2026-09-10 18:25 UTC (permalink / raw)
To: Georgios Karantzas; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
Georgios Karantzas <gck.kara@gmail.com> writes:
> Bound the TX batch by cumulative byte length, not just record count.
>
> Fixes: fb9987d0f748c983 ("ath9k_htc: Support for AR9271 chipset.")
> Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
> ---
> v2:
> - 72-col wrap + Fixes: tag.
You seem to have dropped the commit message instead of just wrapping it.
Wrap does not mean "truncate", it just means "make sure each line stays
below 72 characters". The explanation in the commit message of v1 was
fine, the lines were just too long :)
> - Accounting kept: deferred len += offset never fires on early break.
Ah, right. Please explain this in the commit message. Also, see below:
> - (!i) guard dropped per review.
>
> drivers/net/wireless/ath/ath9k/hif_usb.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
> index 0a3d2190b..533e74565 100644
> --- a/drivers/net/wireless/ath/ath9k/hif_usb.c
> +++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
> @@ -328,11 +328,14 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
> tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
>
> for (i = 0; i < tx_skb_cnt; i++) {
> - nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
> + nskb = skb_peek(&hif_dev->tx.tx_skb_queue);
> + if (!nskb)
> + break;
>
> - /* Should never be NULL */
> - BUG_ON(!nskb);
> + if (tx_buf->offset + nskb->len + 4 > MAX_TX_BUF_SIZE)
> + break;
>
> + nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
> hif_dev->tx.tx_skb_cnt--;
>
> buf = tx_buf->buf;
> @@ -342,13 +345,8 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
> *hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
> buf += 4;
> memcpy(buf, nskb->data, nskb->len);
> - tx_buf->len = nskb->len + 4;
> -
> - if (i < (tx_skb_cnt - 1))
> - tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
> -
> - if (i == (tx_skb_cnt - 1))
> - tx_buf->len += tx_buf->offset;
> + tx_buf->len = tx_buf->offset + nskb->len + 4;
> + tx_buf->offset += (((nskb->len + 3) / 4) + 1) * 4;
While we're fixing this, let's make it readable as well - this could be
(with an #include <linux/math.h>):
tx_buf->offset += round_up(nskb->len + 4, 4);
-Toke
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
2026-08-31 17:39 [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE Georgios Karantzas
2026-09-09 11:49 ` Toke Høiland-Jørgensen
2026-09-09 18:59 ` [PATCH v2] " Georgios Karantzas
@ 2026-09-11 0:11 ` Georgios Karantzas
2026-09-11 10:20 ` Toke Høiland-Jørgensen
2 siblings, 1 reply; 6+ messages in thread
From: Georgios Karantzas @ 2026-09-11 0:11 UTC (permalink / raw)
To: toke; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
__hif_usb_tx() dequeues up to MAX_TX_AGGR_NUM (20) frames into a
single tx_buf of MAX_TX_BUF_SIZE (32768) bytes, limiting the batch
by record count but never by cumulative byte length.
With large frames (MTU 2304), 20 aggregated frames of 2292 bytes
each exceed the allocation (20 * 2296 = 45920 bytes), so the
memcpy() in the loop writes up to 13152 bytes past tx_buf->buf
before usb_submit_urb().
Peek the queue head and stop before copying any record that would
cross MAX_TX_BUF_SIZE, then dispatch the current batch. Leftover
skbs remain queued and are drained on the next URB completion.
The byte bound changes the loop's exit semantics: it can now exit
before i == tx_skb_cnt - 1. Stock only finalized tx_buf->len on
that last index (len += offset), so an early break would submit a
URB holding only the last record's length while every dequeued skb
is freed on completion, silently dropping frames. Make tx_buf->len
a running total and advance tx_buf->offset per record instead; the
stride round_up(nskb->len + 4, 4) is identical to the stock stride
when the loop runs to completion.
Tested on hardware with an MTU 2304 flood: the loop stops at
record 15 (len = 32144, offset = 32144, within 32768), no
oversized URB is submitted, and MTU 1500 pings pass 20/20.
Fixes: fb9987d0f748c983 ("ath9k_htc: Support for AR9271 chipset.")
Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
---
v3:
- Restore the full v1 commit message, wrapped at 72 columns.
- Explain the per-record len/offset accounting in the commit log.
- Use round_up() for the record stride and include <linux/math.h>.
v2:
- Add Fixes: tag.
- Keep immediate len/offset accounting (stock deferred
len += offset never executes when the loop breaks early).
- Drop the buf/memcpy reshuffle and the (!i) guard.
drivers/net/wireless/ath/ath9k/hif_usb.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/hif_usb.c b/drivers/net/wireless/ath/ath9k/hif_usb.c
index 0a3d2190b..a398c9b2d 100644
--- a/drivers/net/wireless/ath/ath9k/hif_usb.c
+++ b/drivers/net/wireless/ath/ath9k/hif_usb.c
@@ -14,6 +14,7 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/
+#include <linux/math.h>
#include <linux/unaligned.h>
#include "htc.h"
@@ -328,11 +329,14 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
tx_skb_cnt = min_t(u16, hif_dev->tx.tx_skb_cnt, MAX_TX_AGGR_NUM);
for (i = 0; i < tx_skb_cnt; i++) {
- nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
+ nskb = skb_peek(&hif_dev->tx.tx_skb_queue);
+ if (!nskb)
+ break;
- /* Should never be NULL */
- BUG_ON(!nskb);
+ if (tx_buf->offset + nskb->len + 4 > MAX_TX_BUF_SIZE)
+ break;
+ nskb = __skb_dequeue(&hif_dev->tx.tx_skb_queue);
hif_dev->tx.tx_skb_cnt--;
buf = tx_buf->buf;
@@ -342,13 +346,8 @@ static int __hif_usb_tx(struct hif_device_usb *hif_dev)
*hdr++ = cpu_to_le16(ATH_USB_TX_STREAM_MODE_TAG);
buf += 4;
memcpy(buf, nskb->data, nskb->len);
- tx_buf->len = nskb->len + 4;
-
- if (i < (tx_skb_cnt - 1))
- tx_buf->offset += (((tx_buf->len - 1) / 4) + 1) * 4;
-
- if (i == (tx_skb_cnt - 1))
- tx_buf->len += tx_buf->offset;
+ tx_buf->len = tx_buf->offset + nskb->len + 4;
+ tx_buf->offset += round_up(nskb->len + 4, 4);
__skb_queue_tail(&tx_buf->skb_queue, nskb);
TX_STAT_INC(hif_dev, skb_queued);
--
2.47.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE
2026-09-11 0:11 ` [PATCH v3] " Georgios Karantzas
@ 2026-09-11 10:20 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 6+ messages in thread
From: Toke Høiland-Jørgensen @ 2026-09-11 10:20 UTC (permalink / raw)
To: Georgios Karantzas; +Cc: linux-wireless, linux-kernel, Georgios Karantzas
Georgios Karantzas <gck.kara@gmail.com> writes:
> __hif_usb_tx() dequeues up to MAX_TX_AGGR_NUM (20) frames into a
> single tx_buf of MAX_TX_BUF_SIZE (32768) bytes, limiting the batch
> by record count but never by cumulative byte length.
>
> With large frames (MTU 2304), 20 aggregated frames of 2292 bytes
> each exceed the allocation (20 * 2296 = 45920 bytes), so the
> memcpy() in the loop writes up to 13152 bytes past tx_buf->buf
> before usb_submit_urb().
>
> Peek the queue head and stop before copying any record that would
> cross MAX_TX_BUF_SIZE, then dispatch the current batch. Leftover
> skbs remain queued and are drained on the next URB completion.
>
> The byte bound changes the loop's exit semantics: it can now exit
> before i == tx_skb_cnt - 1. Stock only finalized tx_buf->len on
> that last index (len += offset), so an early break would submit a
> URB holding only the last record's length while every dequeued skb
> is freed on completion, silently dropping frames. Make tx_buf->len
> a running total and advance tx_buf->offset per record instead; the
> stride round_up(nskb->len + 4, 4) is identical to the stock stride
> when the loop runs to completion.
>
> Tested on hardware with an MTU 2304 flood: the loop stops at
> record 15 (len = 32144, offset = 32144, within 32768), no
> oversized URB is submitted, and MTU 1500 pings pass 20/20.
>
> Fixes: fb9987d0f748c983 ("ath9k_htc: Support for AR9271 chipset.")
> Signed-off-by: Georgios Karantzas <gck.kara@gmail.com>
Looks good now, thanks!
Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-09-11 10:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-31 17:39 [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE Georgios Karantzas
2026-09-09 11:49 ` Toke Høiland-Jørgensen
2026-09-09 18:59 ` [PATCH v2] " Georgios Karantzas
2026-09-10 18:25 ` Toke Høiland-Jørgensen
2026-09-11 0:11 ` [PATCH v3] " Georgios Karantzas
2026-09-11 10:20 ` Toke Høiland-Jørgensen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®