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

[Luka Gejak]

On Sun Sep 6, 2026 at 4:22 AM CEST, Ping-Ke Shih wrote:
[...]
> If the __skb_padd() is necessary for original (generic) part, please
> add another patch to fix it. (see below comment)
[...]
> Why doesn't rtw_sdio_write_port_generic() need __skb_pad() as well?
> Is there an existing problem by original?

There is. rtw_sdio_write_port() rounds the transfer up with
sdio_align_size() and passes that length to sdio_memcpy_toio() while the
skb still holds only skb->len bytes, so between one and 511 bytes from
past the end of the frame get transmitted. Whether that stays inside the
skb's allocation depends on how much tailroom it happens to have.

It gets its own patch, no chip condition:

  wifi: rtw88: sdio: zero the padding added to a TX transfer

Which also answers the question the other way round: the RTL8723BS does
not need the padding for its own sake. I tested that rather than assume
it, with a local patch that transfers unpadded exactly as the generic
path does. Uplink is 20.9 to 21.1 Mbit/s unpadded against 19.5 to 19.8
padded, in an interleaved A/B in one session, and scans, reconnection
and a UDP flood are clean either way. So the padding is a correctness
fix that belongs in the shared path, and patch 4 no longer carries it.

There is an ordering point in that, which I would rather raise with you.
The separate fix patches rtw_sdio_write_port(), which after this series
is rtw_sdio_write_port_generic(), so it applies cleanly on top but only
covers the generic path. The RTL8723BS would then be the one part still
transmitting the padding unzeroed, which is not where I want to leave it.

The shared place is rtw_sdio_write_to_port(), which both paths call, but
that only exists once this series is in. So I plan to send it once this
series is applied, against that function, where a single patch covers
every SDIO part including this one and nothing here needs respinning.

There is one more thing. With the padding gone from patch 4, nothing on
that path returns -ENOMEM any more, so retrying it in patch 6 was dead
code and I removed it. If the padding fix is applied then -ENOMEM becomes
reachable again, for every chip, and that retry has to come back with it:
without it a stopped access category has nothing left to kick it.

> As we have sdio_set_block_size(sdio_func, RTW_SDIO_BLOCK_SIZE),
> can here use sdio_align_size() to get the writ_size?

Yes, and v10 does. I checked it is really the same rather than only
looking the same: on this card sdio_align_size() matches the open coded
rounding at the block boundaries, and over a full session it agreed on
every frame, 80000 of them with no difference.

It is equivalent because the card reports multi block support and a 512
byte block size, which puts the byte mode limit at 512. Without those
two properties sdio_align_size() falls into chunked byte transfers that
are not block multiples, so the equivalence is a property of this card
and host rather than something unconditional. It is still the right call:
it is the core helper and it is what the generic path already uses.

> I think write_size must be large or equal to skb->len, right?
> Therefore, declare size_t pad_size at top of function, and then
[...]
> I think this is easier to read.

You are right that it can never be negative. The whole block is gone in
v10 with the padding, so the question does not arise any more, but it
would have been the better shape.

> In this patchset, you add many comments. Please review them across whole
> patchset to see if they actually need. Like this one, do you think the code
> can't explain itself?

I went through the series and removed redundant comments: v10 adds 62 comment
lines where v9 added 103.

What I kept is the things the code cannot show: which page pool a queue
draws from and how that was measured on hardware, why the counters are
atomic_t, why only the defined interrupt status bits are acknowledged,
and the two places where an skb has already been handed on and must not
be touched. Two more are there because they were asked for in earlier
review: the comment in patch 2 and the mod_delayed_work() one in patch 6.
Say the word if any of those should go too.

Best regards,
Luka Gejak