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

[Ping-Ke Shih]

Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
> 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

Will/should you add this in v10 or v11 before your patch 4/6?

As your v10 didn't get ran by NIPA because of [1], you can send v10 with
RESEND or just send v11 (including the additional one).

(To answer the ordering point question below.) I prefer to have the patch
in advance, for two reasons. One is to backport easier, and another is to
clear to know it is existing problem.

[1] https://lore.kernel.org/linux-wireless/fe0d8a3c2951c051b321902582eaf95f230a754d.camel@xxxxxxxxxxxxxxxx/T/#u

> 
> > 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.

In v10, the beginning of rtw_sdio_write_port_8723bs() and
rtw_sdio_write_port_generic become the similar. And
I think ' txsize = round_up(skb->len, 4);' is not necessary to
rtw_sdio_write_port_8723bs() because it is 512-byte aligned.

Therefore, you can do the same thing in rtw_sdio_write_port() and then
call 
    rtw_sdio_write_port_8723bs()
or
    rtw_sdio_write_port_generic()
by condition. 

> 
> > 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.

Will this be needed by the additional patch

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

> 
> > 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.

I will check this by v11, because I'd like you can have v11 with

  1. "wifi: rtw88: sdio: zero the padding added to a TX transfer"
  2. move the same beginning of rtw_sdio_write_port_8723bs()
     and rtw_sdio_write_port_generic() to rtw_sdio_write_port()

Is it okay to you?

Ping-Ke