Re: [PATCH rtw v3 3/5] wifi: rtw89: perform tx_wait completions for USB part

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 29/08/2025 00:11, Fedor Pchelkin wrote:
> There is no completion signaling for tx_wait skbs on USB part. This means
> rtw89_core_tx_kick_off_and_wait() always returns with a timeout.
> 
> Moreover, recent rework of tx_wait objects lifecycle handling made the
> driver be responsible for freeing the associated skbs, not the core
> ieee80211 stack. Lack of completion signaling would cause those objects
> being kept in driver internal tx_waits queue until rtw89_hci_reset()
> occurs, and then a double free would happen.
> 
> Extract TX status handling into a separate function, like its
> rtw89_pci_tx_status() counterpart. Signal completion from there.
> 
> Found by Linux Verification Center (linuxtesting.org).
> 
> Fixes: 2135c28be6a8 ("wifi: rtw89: Add usb.{c,h}")
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> 
> New series iteration -> new nuances found.
> 
> It seems the two previous patches from the series would not be too great
> in USB case because there is no completion signaling for tx_wait skbs
> there.
> 
> I don't have this hardware so *the patch is compile tested only*. It'd
> be nice if someone gave it a run on top of two previous patches of the
> series, thanks!
> 

I tested your first three patches with RTL8851BU for a few hours. It's
looking good, no explosion yet.

The USB side doesn't have real TX ACK status reporting yet. I only
learned recently how to do that. It looks like it will work about the
same as in rtw88.

>  drivers/net/wireless/realtek/rtw89/usb.c | 36 +++++++++++++++---------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/usb.c b/drivers/net/wireless/realtek/rtw89/usb.c
> index 6cf89aee252e..10fe19bd5166 100644
> --- a/drivers/net/wireless/realtek/rtw89/usb.c
> +++ b/drivers/net/wireless/realtek/rtw89/usb.c
> @@ -188,11 +188,32 @@ static u8 rtw89_usb_get_bulkout_id(u8 ch_dma)
>  	}
>  }
>  
> +static void rtw89_usb_tx_status(struct rtw89_dev *rtwdev, struct sk_buff *skb,
> +				int status)
> +{
> +	struct rtw89_tx_skb_data *skb_data = RTW89_TX_SKB_CB(skb);
> +	struct ieee80211_tx_info *info;
> +
> +	if (rtw89_core_tx_wait_complete(rtwdev, skb_data, status == 0))
> +		return;
> +
> +	info = IEEE80211_SKB_CB(skb);
> +	ieee80211_tx_info_clear_status(info);
> +
> +	if (status == 0) {
> +		if (info->flags & IEEE80211_TX_CTL_NO_ACK)
> +			info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> +		else
> +			info->flags |= IEEE80211_TX_STAT_ACK;
> +	}
> +
> +	ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> +}
> +
>  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 ieee80211_tx_info *info;
>  	struct rtw89_txwd_body *txdesc;
>  	struct sk_buff *skb;
>  	u32 txdesc_size;
> @@ -214,18 +235,7 @@ static void rtw89_usb_write_port_complete(struct urb *urb)
>  			txdesc_size += rtwdev->chip->txwd_info_size;
>  
>  		skb_pull(skb, txdesc_size);
> -
> -		info = IEEE80211_SKB_CB(skb);
> -		ieee80211_tx_info_clear_status(info);
> -
> -		if (urb->status == 0) {
> -			if (info->flags & IEEE80211_TX_CTL_NO_ACK)
> -				info->flags |= IEEE80211_TX_STAT_NOACK_TRANSMITTED;
> -			else
> -				info->flags |= IEEE80211_TX_STAT_ACK;
> -		}
> -
> -		ieee80211_tx_status_irqsafe(rtwdev->hw, skb);
> +		rtw89_usb_tx_status(rtwdev, skb, urb->status);
>  	}
>  
>  	switch (urb->status) {