Re: [PATCH] wifi: ath9k_htc: bound TX aggregation to MAX_TX_BUF_SIZE

[Toke Høiland-Jørgensen]

Georgios Karantzas <gck.kara@xxxxxxxxx> 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@xxxxxxxxx>

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