Re: [PATCH v2 07/11] wifi: rtw88: sdio: track free TX pages and OQT credits for RTL8723BS

[luka.gejak@linux.dev]

From: Luka Gejak <luka.gejak@linux.dev>

On 27/07/2026 08:52, Ping-Ke Shih wrote:
>> output queue token count that rtw88 does not track at all.
> 
> Does OQT mean Output Queue Token? Could you explain a little bit?

Yes. REG_SDIO_OQT_FREE_PG is how many more descriptors the chip's SDIO
output queue can still accept. On this part the chip discards a write
that arrives when the count is zero, silently, so the frame is lost
rather than retried. The vendor driver polls the register before every
port write; this patch caches the count and only re-reads when the
cached value is exhausted. I have rewritten that paragraph of the
commit message to spell it out and dropped the "OQT" abbreviation from
the prose.

> I think this can largely improve SDIO. Please share the performance
> numbers before/after this patch.

Added to the commit message. Measured against an iperf3 server one hop
behind the AP, using the wlan0 byte counters as ground truth because
eth0 on the test box shares the same subnet:

  without this patch: TCP and UDP both 0 bit/s in either direction
  with this patch:    TCP 25.3 Mbit/s up, 37.3 Mbit/s down
                      UDP 25.0 Mbit/s up at 0% loss

To be clear about the "before" number: on the generic path the
association itself still completes, but no data gets through at all, so
this is not a tuning patch. The transfer sizing in the third hunk is
the part that matters most there; without the block size padding the
chip stops accepting writes entirely.

>> +static void rtw_sdio_init_free_txpg(struct rtw_dev *rtwdev)
>> +static void rtw_sdio_sync_free_txpg(struct rtw_dev *rtwdev)
> 
> _8723bs_ specific so add to the function name

Renamed, along with rtw_sdio_wait_tx_oqt() ->
rtw_sdio_8723bs_wait_tx_oqt(). The shared field decoding is now
rtw_sdio_8723bs_store_free_txpg().

> It is clear that bit masks of REG_SDIO_FREE_TXPG are PG_{HIGH, NORMAL, LOW,
> PUB}. Let's define them in reg.h, and use u32_get_bits() to read out the
> values.

Done, in sdio.h next to REG_SDIO_FREE_TXPG since that is where the
register lives:

  #define BIT_FREE_TXPG_HIGH	GENMASK(7, 0)
  #define BIT_FREE_TXPG_NORMAL	GENMASK(15, 8)
  #define BIT_FREE_TXPG_LOW	GENMASK(23, 16)
  #define BIT_FREE_TXPG_PUB	GENMASK(31, 24)

and read with u32_get_bits(). That also removed the duplicated decode,
which is what let me answer your next question properly.

>> +               dedicated = rtw_sdio_8723bs_free_txpg(rtwdev, queue);
> 
> Why do you check ` dedicated < 0` for this case?

There was no good reason; the queue does not change between the two
calls so the second result could not have been an error. Rather than
add a second check, rtw_sdio_8723bs_free_txpg() now returns the
atomic_t * for the queue, so the mapping is resolved once, NULL is
checked once with a warning, and the resync path just re-reads the same
counter.

>> +       bool rtl8723bs = rtw_is_8723bs(rtwdev);
> 
> reverse X'mas tree

Fixed.

>> +       if (!ret && rtl8723bs) {
> 
> move to else-branch below?

Done, it is now if (ret) { warn } else if (rtl8723bs) { account }.

>> +       if (fifo->acq_pg_num <= pg_tbl->hq_num + pg_tbl->lq_num +
> 
> second and third lines don't align.

The sum was computed twice with different formatting, so it is now a
reserved_num local used in both places.

>> +       rtw_sdio_8723bs_check_rqpn(rtwdev);
> 
> Should we check chip is RTL8723BS before calling?
> I'd have consistent the calling rule -- callers check?

Agreed, and applied across the series. rtw_sdio_start() now has the
test and both helpers assume it.

>> +       atomic_t free_pg_high;
> 
> Could I know the reason why these should be atomic_t? Which context are
> they running?

Two contexts. The usual one is the TX work, which is single threaded
(create_singlethread_workqueue), so on its own it would not need them.
But rtw_sdio_write_port() is also reached from rtw_sdio_write_data()
for H2C commands and reserved page uploads, which runs in process
context under rtwdev->mutex and does not go through the TX workqueue.
Those two share no lock.

The counters are a fast path hint rather than an authority: whenever
they claim there is not enough room, rtw_sdio_8723bs_sync_free_txpg()
re-reads REG_SDIO_FREE_TXPG before the caller gives up, so a lost
update costs one extra register read and nothing else. I have put that
reasoning in a comment above the helpers. If you would rather have a
spinlock around the group I can do that instead, it is not a hot path
in the sense that would make the lock cost anything.

Best regards,
Luka Gejak