Re: [PATCH 3/4] wifi: rtw88: usb: Support RX aggregation

[Sascha Hauer <sha@pengutronix.de>]

On Sun, Jul 28, 2024 at 10:42:32PM +0300, Bitterblue Smith wrote:
> The chips can be configured to aggregate several frames into a single
> USB transfer. Modify rtw_usb_rx_handler() to support this case.
> 
> RX aggregation improves the RX speed on certain ARM systems, like the
> NanoPi NEO Core2.
> 
> Currently none of the chips are configured to aggregate frames.
> 
> Tested with RTL8811CU and RTL8723DU.
> 
> Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
> ---
>  drivers/net/wireless/realtek/rtw88/usb.c | 57 +++++++++++++++---------
>  1 file changed, 37 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/usb.c b/drivers/net/wireless/realtek/rtw88/usb.c
> index 73948078068f..d61be1029a7b 100644
> --- a/drivers/net/wireless/realtek/rtw88/usb.c
> +++ b/drivers/net/wireless/realtek/rtw88/usb.c
> @@ -546,11 +546,12 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>  	struct rtw_usb *rtwusb = container_of(work, struct rtw_usb, rx_work);
>  	struct rtw_dev *rtwdev = rtwusb->rtwdev;
>  	const struct rtw_chip_info *chip = rtwdev->chip;
> -	struct rtw_rx_pkt_stat pkt_stat;
> +	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
>  	struct ieee80211_rx_status rx_status;
> +	u32 pkt_offset, next_pkt, urb_len;
> +	struct rtw_rx_pkt_stat pkt_stat;
> +	struct sk_buff *next_skb = NULL;
>  	struct sk_buff *skb;
> -	u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> -	u32 pkt_offset;
>  	u8 *rx_desc;
>  	int limit;
>  
> @@ -559,29 +560,44 @@ static void rtw_usb_rx_handler(struct work_struct *work)
>  		if (!skb)
>  			break;
>  
> -		rx_desc = skb->data;
> -		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> -					 &rx_status);
> -		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> -			     pkt_stat.shift;
> -
> -		if (pkt_stat.is_c2h) {
> -			skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> -			rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> -			continue;
> -		}
> -
>  		if (skb_queue_len(&rtwusb->rx_queue) >= RTW_USB_MAX_RXQ_LEN) {
>  			dev_dbg_ratelimited(rtwdev->dev, "failed to get rx_queue, overflow\n");
>  			dev_kfree_skb_any(skb);
>  			continue;
>  		}
>  
> -		skb_put(skb, pkt_stat.pkt_len);
> -		skb_reserve(skb, pkt_offset);
> -		rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> -		memcpy(skb->cb, &rx_status, sizeof(rx_status));
> -		ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +		urb_len = skb->len;
> +
> +		do {
> +			rx_desc = skb->data;
> +			chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
> +						 &rx_status);
> +			pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> +				     pkt_stat.shift;
> +
> +			next_pkt = round_up(pkt_stat.pkt_len + pkt_offset, 8);
> +
> +			if (urb_len >= next_pkt + pkt_desc_sz)
> +				next_skb = skb_clone(skb, GFP_KERNEL);

You could add a:
			else
				next_skb = NULL;

here and drop the next_skb = NULL from the end of the loop. No
functional change, but easier to read.

> +
> +			if (pkt_stat.is_c2h) {
> +				skb_trim(skb, pkt_stat.pkt_len + pkt_offset);
> +				rtw_fw_c2h_cmd_rx_irqsafe(rtwdev, pkt_offset, skb);
> +			} else {
> +				skb_pull(skb, pkt_offset);
> +				skb_trim(skb, pkt_stat.pkt_len);
> +				rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> +				memcpy(skb->cb, &rx_status, sizeof(rx_status));
> +				ieee80211_rx_irqsafe(rtwdev->hw, skb);
> +			}
> +
> +			skb = next_skb;
> +			if (skb)
> +				skb_pull(next_skb, next_pkt);

You could use skb instead of next_skb here. Both are the same, so no
functional change, just makes it a bit easier to read when you use the
same variable that you just tested for validity.

> +
> +			urb_len -= next_pkt;
> +			next_skb = NULL;
> +		} while (skb && urb_len >= pkt_desc_sz);

You can drop the urb_len >= pkt_desc_sz check. It will be exactly true
when skb is non NULL as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |