RE: [PATCH v8 0/6] wifi: rtw88: preparations for RTL8723B/RTL8723BS

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> On August 25, 2026 6:33:08 PM GMT+02:00, luka.gejak@xxxxxxxxx wrote:
> >From: Luka Gejak <luka.gejak@xxxxxxxxx>
> >
> >This is the first of two series adding support for the Realtek RTL8723B
> >802.11n chipset and its RTL8723BS SDIO variant to rtw88. It contains
> >only the changes to the shared rtw88 core that the chip driver depends
> >on. The chip itself, the build glue and the MAINTAINERS entry are a
> >second series.
> >
> 
> ...
> 
> Hi Ping-Ke,
> 
> A tester has been running v8 on a slow ARM SDIO board. In a good signal
> environment it passes a thousand iterations of his stress test with
> nothing in the log. In a poor one he still gets "failed to get tx report
> from firmware", and a reconnection along with it. Power save was off for
> both.
> 
> Looking at why, the two ends of the tx report path are not symmetric:
> 
>   rtw_tx_report_handle() -> rtw_tx_report_tx_status()
>                          -> ieee80211_tx_status_irqsafe()
> 
>   rtw_tx_report_purge_timer() -> skb_queue_purge()

I think it should use ieee80211_purge_tx_queue() instead of skb_queue_purge().

> 
> When the report arrives mac80211 gets a verdict. When it does not, the
> frames are freed with kfree_skb() and mac80211 is told nothing at all.
> rtw_tx_report_enqueue() also re-arms the timer on every frame, so it
> fires once, 500 ms after the last enqueue, and purges everything
> outstanding, including frames queued a moment earlier.

I guess it'd simplify the design that it extends lifetime of queued frames
once a new one is queued. 

> 
> One of the frames that asks for a report is the nullfunc mac80211 sends
> to poll a link it suspects is dead. If no status comes back,
> ieee80211_sta_tx_notify() is never called, the poll counts as
> unanswered, and mac80211 tears the connection down. That fits what the
> tester sees: the warning and the reconnection arriving together, and
> only when the signal is poor enough for reports to go missing.
> 
> Raising the timeout further does not look like the answer. Patch 3 of
> this series already takes it to 2500 ms for the RTL8723BS, five times
> the default, and he still reaches it. A longer wait only delays the
> status.

I think the timeout relies on 
   static int probe_wait_ms = 500;
   module_param(probe_wait_ms, int, 0644);

So, if you enlarge the timeout in rtw_tx_report_enqueue() over 500ms,
it can't help the case.

Maybe, you can set larger number to module parameter probe_wait_ms to
see if it can help.

> 
> What looks right to me is to report the frames instead of dropping
> them: walk the queue on timeout and hand each one back with
> rtw_tx_report_tx_status(rtwdev, cur, false). That tells mac80211 the
> frame was not acknowledged, which is true, and lets it act rather than
> wait for a status that will never come. It would also stop mac80211 TX
> skbs being freed with kfree_skb() instead of being returned through
> ieee80211_tx_status().

As the comment:

 * Calls to this function, ieee80211_tx_status_skb() and
 * ieee80211_tx_status_ni() may not be mixed for a single hardware.

Should use ieee80211_tx_status_irqsafe(). 

But I feel this case, using ieee80211_purge_tx_queue() is equivalent?

> 
> That is core behaviour for every chip, not just this one, so I would
> rather ask than send it. Would you want it as a separate patch outside
> this series? 

This looks an independent patch. If you want to go faster, a separate
patch is better. If you don't concern the time, putting together with
your patchset is okay to me.

> And is reporting "not acked" what you would want on the
> other parts, or would you rather the timeout stayed silent for them and
> only this chip changed?

If using ieee80211_purge_tx_queue() can get positive result, no need
special condition for certain chips.