Re: [PATCH v5 0/6] wifi: rtw88: preparations for RTL8723B/RTL8723BS

[Luka Gejak]

Hi Peter,

On August 20, 2026 1:37:22 PM GMT+02:00, Peter Robinson
<pbrobinson@xxxxxxxxx> wrote:
>Hi Luka,
>
>On Thu, 20 Aug 2026 at 10:02, Luka Gejak <luka.gejak@xxxxxxxxx> wrote:
>>
>> On August 14, 2026 12:53:20 PM GMT+02:00, luka.gejak@xxxxxxxxx wrote:
>> >From: Luka Gejak <luka.gejak@xxxxxxxxx>
>> >
>> >This is the first of two series adding support for the Realtek RTL8723B
>> >802.11n chipset and its RTL8723BS SDIO variant to rtw88. It contains
>> >only the changes to the shared rtw88 core that the chip driver depends
>> >on. The chip itself, the build glue and the MAINTAINERS entry are a
>> >second series.
>> >
>>
>> ...
>>
>> Hi Ping-Ke,
>>
>> Please do not apply patches 4 and 6 of v6. I found real bugs in both
>> after posting and a v7 is on the way.
>>
>> Patch 6 touches the skb after handing it to the TX worker:
>>
>>         skb_queue_tail(&rtwsdio->tx_queue[queue], skb);
>>
>>         rtw_sdio_8723bs_stop_tx_queue(rtwdev, queue,
>>                                       skb_get_queue_mapping(skb));
>>
>> The worker can dequeue, transmit and free that skb before the second
>> statement runs, so reading it there is a use after free. v7 reads the
>> queue index into a local before the enqueue.
>>
>> Patch 4 concerns you more than me. It passed the padded size to
>> rtw_sdio_get_tx_addr(), where upstream passes skb->len. That value is
>> encoded into the CMD53 address as the transfer length, so on every other
>> SDIO chip the encoded length changed whenever sdio_align_size() padded.
>> For the RTL8723BS both expressions are the same value, so the change
>> bought this chip nothing and only affected the other parts. The padding
>> itself also ran on the generic path, giving those chips an allocation
>> and an -ENOMEM path they did not have. v7 restores skb->len and gates
>> the padding on the chip.
>>
>> The other v7 changes, all in patches 4 and 6:
>>
>>   - the free page check, the output queue wait and the accounting are
>>     serialised. The TX worker and the H2C path both reach
>>     rtw_sdio_write_port(), and two writers could each pass the checks
>>     and claim the same pages and output queue entry, after which the
>>     chip discards one transfer silently. The vendor driver has one
>>     transmit thread and never needed this.
>>   - the padding called skb_put_zero() after checking only tailroom, so a
>>     cloned skb had its shared buffer written. It uses __skb_pad() now.
>>   - the back-pressure stop could race the drain: the worker could empty
>>     the queue while the stop was still in progress, see the flag clear,
>>     and never wake it, leaving an access category stopped with nothing
>>     left to wake it.
>>
>> Patches 1, 2, 3 and 5 are unchanged and keep your acks; patch 5 shifts
>> only by hunk offsets because patch 4 grew above it. I dropped your ack
>> from patch 4, since the rework is too substantive to carry it.
>>
>> Most of these came from Sashiko's automated review of v6. I checked each
>> against the code before acting on it, and one of its eight findings did
>> not hold up.
>>
>> v7 follows shortly. It is hardware tested: 60 idle pings at 0% loss with
>> power save on, bidirectional load with no stall, 19.6 Mbit/s up and 36.8
>> down, UDP at 0% loss, three scans, three reconnects and a clean log.
>
>I had done some basic testing with v5, and had left a v6 to build
>overnight. I'll grab this shortly and do a build with v7 and do some
>testing.It was generally looking stable on the Pine64+ board I was
>using for testing.
>

Thanks a lot for testing. In my testing changes in v6 and v7 didn't impact
performance and v7 should be as stable if not more stable then v5 and v6.

>I think I may also have a device(s) with a 8723cs and/or a 8723ds I
>can possibly test with, I'll see if I can remember/find it over the
>weekend and get it running for some testing of that too.

That would be nice, so we can see if other chips are affected, as Ping-Ke
said.

>
>Peter

Best regards,
Luka Gejak