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

[Bitterblue Smith <rtl8821cerfe2@gmail.com>]

On 01/09/2025 20:45, Fedor Pchelkin wrote:
> On Fri, 29. Aug 22:57, Bitterblue Smith wrote:
>> 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.
> 
> Hello Bitterblue,
> 
> thank you!  Just in case, rtw89_core_send_nullfunc() has to be called in
> order to trigger all the tx_wait activity touched with the patches, please
> make sure it's called during the tests - it should be done after scan
> complete, 47a498b84f01 ("wifi: rtw89: TX nulldata 0 after scan complete").
> 
> There is one more issue we'd also need to solve: perform tx_wait
> completion signaling inside rtw89_usb_ops_reset() (driver shutdown stage
> should probably also be handled with the case).  This'd require having an
> ability to track TX URBs and kill them.  I'm just throwing these thoughts
> now, maybe you have some ideas.  I'm still exploring the USB-part source
> code and hopefully will have a chance to get hands on the USB chip soon.
> 
>>
>> 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.
> 
> Do you mean similar pattern already exists in rtw88?  Could you give a
> hint on how USB side TX ACK status reporting works there?  At a quick
> glance, I don't see how those TX URB complete callbacks differ from what
> rtw89 has.

Well, I assume we are talking about ACK status reporting. For example,
when mac80211 detects beacon loss it sends a null frame, or a probe
request (I'm not sure which is used when). It flags the frame with
IEEE80211_TX_CTL_REQ_TX_STATUS, which means the driver has to report
whether the AP sent ACK for the null frame/probe request or not. If
the AP doesn't reply for a while, the connection is considered lost.

A URB status of 0 only means that the URB was submitted successfully.
It doesn't mean the chip actually transmitted anything, and it doesn't
mean the chip received ACK from the AP.

In order to receive these ACK status reports rtw89 will have to set
a bit in the TX descriptor and place the skb in a queue to wait for
a message from the firmware. ieee80211_tx_status_irqsafe() can be
called when the firmware sends the message. 

This is for USB. It seems to work differently for PCIE in rtw89
(rtw89_pci_release_rpp()). In rtw88 it's one mechanism for PCIE, USB,
and SDIO.

These are some functions to look at in rtw88:

rtw_tx_report_enable()
rtw_usb_write_port_tx_complete()
rtw_tx_report_enqueue()

rtw_usb_rx_handler()
rtw_fw_c2h_cmd_rx_irqsafe()
rtw_fw_c2h_cmd_handle() / rtw_fw_c2h_cmd_handle_ext()
rtw_tx_report_handle()