RE: [PATCH v3] 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:

[...]

> @@ -166,16 +166,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 */

This comment is fine.

>         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)
> +               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;
> @@ -234,6 +243,10 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>                 break;
>         }
> 
> +       /* Decrement in-flight counter (skip firmware command channel) */

I think this is not necessary.

> +       if (txcb->txch != RTW89_TXCH_CH12)
> +               atomic_dec(&rtwusb->tx_inflight[txcb->txch]);
> +
>         kfree(txcb);
>  }
> 
> @@ -311,9 +324,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 */

This is not necessary too. Another thought is to remove this condition
(just increase/decrease the count for CH12), but skip to check the count
like this patch does in rtw89_usb_ops_check_and_reclaim_tx_resource().

> +               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 */

This comment is not necessary either.

> +                       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);

[...]

Ping-Ke