Re: [PATCH v11 4/7] wifi: rtw88: sdio: zero the padding added to a TX transfer

[Luka Gejak]

On Thu Sep 10, 2026 at 4:24 AM CEST, Ping-Ke Shih wrote:
> luka.gejak@xxxxxxxxx <luka.gejak@xxxxxxxxx> wrote:
>> From: Luka Gejak <luka.gejak@xxxxxxxxx>
>> 
>> rtw_sdio_write_port() rounds the transfer up with sdio_align_size() and
>> then hands that length to sdio_memcpy_toio() while the skb still only
>> holds skb->len bytes. The difference, between one and 511 bytes, is read
>> from beyond the end of the frame and transmitted. Whether it stays
>> inside the skb's allocation depends on how much tailroom the skb happens
>> to have, so this is at best sending uninitialised memory over the air.
>> 
>> Pad the skb up to the transfer size first. __skb_pad() zeroes the added
>> bytes, reallocates a cloned skb rather than writing into a buffer a
>> clone still shares, and leaves skb->len alone, so nothing else in the
>> transmit path has to change.
>
> With __skb_pad(), it might increase CPU usage.
> Could you roughly measure that?
>

Measured with the ftrace function profiler over a 30 second saturating
uplink transfer, 18.6 Mbit/s, four cores:

  function              hits      time(us)   share of one core
  rtw_sdio_write_port  48358      26839023        89.2%
  sdio_memcpy_toio     47855      11783994        39.2%
  __skb_pad            48344        616489         2.1%
  pskb_expand_head     47869        466036         1.6%

__skb_pad() is about 2% of one core with the link saturated. The whole
transfer takes 14.8% of the four cores against 0.77% idle, so the
padding is roughly 3.5% of the CPU the transfer already uses and 2.3%
of the driver's write path. On throughput it is about 6%, which is the
A/B in the commit message.

pskb_expand_head() runs on 99% of the calls, so nearly every frame takes
the reallocating path rather than the memset. That is three quarters of
the cost; without it the padding would be around 0.5% of one core. I
have not worked out yet whether the skb is cloned or short of tailroom.
If it is tailroom it is probably avoidable, and I can chase it as a
follow up.

I do not think this is a blocker. A few percent seems a fair price for
not putting uninitialised memory on the air.

One thing to decide, though. The commit message says the padding "costs
nothing observable", which is too strong given the numbers in that same
paragraph. Do you think it needs rewording? If so, would you be willing
to amend it yourself when applying? That seems better than a resend of
seven patches for one line.

>> 
>> It must not free the skb on failure: rtw_sdio_write_data() frees the skb
>> itself and rtw_sdio_process_tx_queue() requeues it, so both callers
>> still own it and would double free.
>> 
>> Found while reworking this path for the RTL8723BS. Measured on RTL8723BS
>> hardware, padding the transfer costs nothing observable: uplink is
>> 19.5 to 19.8 Mbit/s padded against 20.9 to 21.1 Mbit/s unpadded in an
>> interleaved A/B, with scans, reconnection and a UDP flood clean in both.
>> The other SDIO parts sharing this path are untested; I have only the
>> RTL8723BS.
>> 
>> Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
>> Signed-off-by: Luka Gejak <luka.gejak@xxxxxxxxx>
>
> Acked-by: Ping-Ke Shih <pkshih@xxxxxxxxxxx>

Thanks for the Acked-by tag, one both patches, I will send 2nd series for 
the actual driver code after this prep series is merged to rtw-next.

Best regards,
Luka Gejak