RE: [PATCH v2] wifi: rtw89: usb: fix TX flow control by tracking in-flight URBs

[Ping-Ke Shih <pkshih@realtek.com>]

Lucid Duck <lucid_duck@justthetip.ca> wrote:
> From: Lucid Duck <lucid_duck@justthetip.ca>
> Date: Thu, 20 Mar 2026 20:00:00 -0700
> Subject: [PATCH v2] wifi: rtw89: usb: fix TX flow control by tracking
>  in-flight URBs

What is your mailer? Not 'git send-email'?
I'm not sure if I can apply this by patchwork tool.

> 
> rtw89_usb_ops_check_and_reclaim_tx_resource() returns a hardcoded
> placeholder value (42) instead of actual TX resource availability.
> This violates mac80211's flow control contract, preventing backpressure
> and causing uncontrolled URB accumulation under sustained TX load.
> 
> Fix by adding per-channel atomic counters (tx_inflight[]) that track
> in-flight URBs:
> 
> - Increment before usb_submit_urb() with rollback on failure
> - Decrement in completion callback
> - Return (MAX_URBS - inflight) to mac80211, or 0 when at capacity
> - Exclude firmware command channel (CH12) from tracking
> 
> The pre-increment pattern prevents a race where the USB core completes
> the URB (possibly on another CPU) before the submitting code increments
> the counter.
> 
> Tested on D-Link DWA-X1850 (RTL8832AU), kernel 6.18.3:
> 
>                      Unpatched -> Patched
>   USB3 5GHz DL:      494 -> 709 Mbps (+44%)
>   USB3 5GHz retx:    8 -> 1 (-88%)
>   USB3 2.4GHz DL:    54 -> 68 Mbps (+25%)
>   USB2 5GHz DL:      196 -> 225 Mbps (+15%)
>   USB2 2.4GHz DL:    123 -> 131 Mbps (+6%)

As this patch does TX flow control, could you share the uplink data
as well?

> 
> Signed-off-by: Lucid Duck <lucid_duck@justthetip.ca>
> ---
> Resending v2. This was prepared in late January after addressing v1
> review feedback, but the send failed silently (SMTP misconfiguration)
> and never appeared on lore.kernel.org. Apologies for the delay.
> 
> Changes since v1:
> - Removed duplicate "TX flow control" comment (Ping-Ke Shih)
> - Added test results to commit message (Ping-Ke Shih)
> 
> Bitterblue's CH12 question from v1: the CH12 guards in tx_kick_off()
> and write_port_complete() are a matched pair. tx_kick_off() skips
> atomic_inc for CH12, so the completion handler must skip atomic_dec
> to match. Removing only the completion side causes counter underflow.
> 
> Additional validation: 100-iteration stress test, 50-iteration
> teardown (rmmod/modprobe under load), 10x hot-unplug during active
> TX, and 30-minute soak -- all pass with counters balanced at idle.
> 
> The 32-URB-per-channel limit is based on similar USB wireless drivers
> (mt76, ath9k_htc). The fixed value works well for both USB2 and USB3.

Can increasing 32 get better performance? The stress test with small
packets might yield low throughput? (Just a question)

> 
>  drivers/net/wireless/realtek/rtw89/usb.c | 26 ++++++++++++++++++-----
>  drivers/net/wireless/realtek/rtw89/usb.h |  6 ++++++
>  2 files changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c
> b/drivers/net/wireless/realtek/rtw89/usb.c
> index eb489df..faafa3c 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -161,16 +161,25 @@ static u32
>  rtw89_usb_ops_check_and_reclaim_tx_resource(struct rtw89_dev *rtwdev,
>                                             u8 txch)
>  {
> +       struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
> +       int inflight;
> +
> +       /* Firmware command channel is not tracked */
>         if (txch == RTW89_TXCH_CH12)
>                 return 1;
> 
> -       return 42; /* TODO some kind of calculation? */
> +       inflight = atomic_read(&rtwusb->tx_inflight[txch]);
> +       if (inflight >= RTW89_USB_MAX_TX_URBS_PER_CH)

Out of curiosity. Is it possible inflight > RTW89_USB_MAX_TX_URBS_PER_CH?

> +               return 0;
> +
> +       return RTW89_USB_MAX_TX_URBS_PER_CH - inflight;
>  }
> 
>  static void rtw89_usb_write_port_complete(struct urb *urb)
>  {
>         struct rtw89_usb_tx_ctrl_block *txcb = urb->context;
>         struct rtw89_dev *rtwdev = txcb->rtwdev;
> +       struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
>         struct ieee80211_tx_info *info;
>         struct rtw89_txwd_body *txdesc;
>         struct sk_buff *skb;
> @@ -229,6 +238,10 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 break;
>         }
> 
> +       /* Decrement in-flight counter (skip firmware command channel) */
> +       if (txcb->txch != RTW89_TXCH_CH12)
> +               atomic_dec(&rtwusb->tx_inflight[txcb->txch]);
> +
>         kfree(txcb);
>  }
> 
> @@ -306,9 +319,17 @@ static void rtw89_usb_ops_tx_kick_off(struct rtw89_dev
> *rtwdev, u8 txch)
> 
>                 skb_queue_tail(&txcb->tx_ack_queue, skb);
> 
> +               /* Increment BEFORE submit to avoid race with completion */
> +               if (txch != RTW89_TXCH_CH12)
> +                       atomic_inc(&rtwusb->tx_inflight[txch]);
> +
>                 ret = rtw89_usb_write_port(rtwdev, txch, skb->data, skb->len,
>                                            txcb);
>                 if (ret) {
> +                       /* Rollback increment on failure */
> +                       if (txch != RTW89_TXCH_CH12)
> +                               atomic_dec(&rtwusb->tx_inflight[txch]);
> +
>                         if (ret != -ENODEV)
>                                 rtw89_err(rtwdev, "write port txch %d failed:
> %d\n",
>                                           txch, ret);
> @@ -666,8 +687,10 @@ static void rtw89_usb_init_tx(struct rtw89_dev *rtwdev)
>         struct rtw89_usb *rtwusb = rtw89_usb_priv(rtwdev);
>         int i;
> 
> -       for (i = 0; i < ARRAY_SIZE(rtwusb->tx_queue); i++)
> +       for (i = 0; i < ARRAY_SIZE(rtwusb->tx_queue); i++) {
>                 skb_queue_head_init(&rtwusb->tx_queue[i]);
> +               atomic_set(&rtwusb->tx_inflight[i], 0);
> +       }
>  }
> 
>  static void rtw89_usb_deinit_tx(struct rtw89_dev *rtwdev)
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.h
> b/drivers/net/wireless/realtek/rtw89/usb.h
> index 9f554b5..1459122 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.h
> +++ b/drivers/net/wireless/realtek/rtw89/usb.h
> @@ -20,6 +20,9 @@
>  #define RTW89_MAX_ENDPOINT_NUM         9
>  #define RTW89_MAX_BULKOUT_NUM          7
> 
> +/* TX flow control: max in-flight URBs per channel */

I think the code self-explain this already. No need this comment. 
It'd be helpful if you can explain how you decide 32. 
(In commit message, you mentioned this imitate other drivers, so
no need comment about that.)

> +#define RTW89_USB_MAX_TX_URBS_PER_CH   32
> +
>  struct rtw89_usb_info {
>         u32 usb_host_request_2;
>         u32 usb_wlan0_1;
> @@ -63,6 +66,9 @@ struct rtw89_usb {
>         struct usb_anchor tx_submitted;
> 
>         struct sk_buff_head tx_queue[RTW89_TXCH_NUM];
> +
> +       /* TX flow control: track in-flight URBs per channel */

I'd not prefer this comment neither. 

> +       atomic_t tx_inflight[RTW89_TXCH_NUM];
>  };
> 
>  static inline struct rtw89_usb *rtw89_usb_priv(struct rtw89_dev *rtwdev)
> --
> 2.53.0