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

[Toke Høiland-Jørgensen]

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