RE: [PATCH v7 4/6] wifi: rtw88: sdio: track free TX pages and OQT credits for RTL8723BS

[Luka Gejak]

Hi Ping-Ke,

On August 25, 2026 8:12:58 AM GMT+02:00, Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:

> I think you only need to note 'not free the skb on failure'.
> And move comment here to note __skb_pad().
> By the way, we can have a local variable 'pad_size'.

Done, all three:

        if (write_size > skb->len) {
                size_t pad_size = write_size - skb->len;

                /*
                 * __skb_pad() must not free the skb on failure: both callers
                 * still own it, one requeues it and the other frees it.
                 */
                ret = __skb_pad(skb, pad_size, false);
                if (ret)
                        return ret;
        }

> Not sure if the comment along declaration of tx_credit_lock is enough?
> If so, maybe we don't need this comment.

Agreed, that one is gone.

> guard(mutex)(&rtwsdio->tx_credit_lock);

The lock is only taken for this chip, and guard() is unconditional, so I
split the locked region into its own function instead of branching around
the lock:

        if (!rtw_is_8723bs(rtwdev)) {
                txsize = sdio_align_size(rtwsdio->sdio_func, skb->len);

                ret = rtw_sdio_check_free_txpg(rtwdev, queue, txsize);
                if (ret)
                        return ret;

                return rtw_sdio_write_to_port(rtwdev, skb, queue, txaddr,
                                              txsize);
        }
        ...
        guard(mutex)(&rtwsdio->tx_credit_lock);

        return rtw_sdio_8723bs_write_port(rtwdev, skb, queue, txaddr, txsize,
                                          write_size);

The transfer itself moved to rtw_sdio_write_to_port() so both paths share
it. That also removes the goto and the two rtl8723bs tests in the middle of
the function.

One thing I would like your opinion on while it is still cheap to change.
The unaligned SKB warning now sits in both callers rather than once inside
rtw_sdio_write_to_port(), because __func__ would otherwise report
rtw_sdio_write_to_port for every other SDIO chip where it reports
rtw_sdio_write_port today. Keeping that message identical costs three
duplicated lines. Putting it once in rtw_sdio_write_to_port() is the
tidier code and arguably the more accurate message, at the price of
changing a log line on chips this series is not about. I went with the
duplicate to leave the other parts alone, but I have no strong feeling
either way, so say which you prefer.

> I think you can add lockdep_assert_held() to the places the locks (mutex)
> must be held, and run test if somewhere throw warning (must not).

Added to rtw_sdio_8723bs_write_port(), rtw_sdio_8723bs_wait_tx_oqt() and
rtw_sdio_8723bs_consume_txpg(). Each has exactly one caller and all three
sit under the guard, so the call graph says they cannot be reached without
the lock.

I have to be straight about the test though: my test kernel is built
without CONFIG_PROVE_LOCKING, so those asserts compile to nothing there and
running it would prove nothing. I will build a kernel with lockdep enabled
and confirm before I would ask you to take the patch on that point, unless
you would rather I just drop the asserts.

Best regards,
Luka Gejak