Driver for rtw8723ds

Driver for rtw8723ds

[Larry Finger]

Jernej,

Is there a reason that the driver for RTW8723DS was not included along with the 
other SDIO versions in rtw88?

I have a user of one of my GitHub repos that uses this device.

Thanks,

Larry

Re: Driver for rtw8723ds

[Jernej Škrabec]

Hi Larry,

/cc Martin

Dne nedelja, 30. april 2023 ob 18:51:15 CEST je Larry Finger napisal(a):
> Jernej,
> 
> Is there a reason that the driver for RTW8723DS was not included along with
> the other SDIO versions in rtw88?

We have no HW to test.

> 
> I have a user of one of my GitHub repos that uses this device.

Can you ask that user to test following commits?
https://github.com/xdarklight/linux/commit/
3866a7a3702f7f24557f2c065b7d4088f7027466
https://github.com/xdarklight/linux/commit/
66fd078556a6bf246337270b2e91d73c079fce2d

Patches are trivial, but some testing needs to be done to confirm the driver 
actually works as intended.

Best regards,
Jernej

> 
> Thanks,
> 
> Larry

Re: Driver for rtw8723ds

[Larry Finger]

On 5/2/23 14:27, Jernej Škrabec wrote:
> Hi Larry,
> 
> /cc Martin
> 
> Dne nedelja, 30. april 2023 ob 18:51:15 CEST je Larry Finger napisal(a):
>> Jernej,
>>
>> Is there a reason that the driver for RTW8723DS was not included along with
>> the other SDIO versions in rtw88?
> 
> We have no HW to test.
> 
>>
>> I have a user of one of my GitHub repos that uses this device.
> 
> Can you ask that user to test following commits?
> https://github.com/xdarklight/linux/commit/
> 3866a7a3702f7f24557f2c065b7d4088f7027466
> https://github.com/xdarklight/linux/commit/
> 66fd078556a6bf246337270b2e91d73c079fce2d
> 
> Patches are trivial, but some testing needs to be done to confirm the driver
> actually works as intended.

Jernej,

The user needs the rtw8723ds driver - the SDIO equivalent of rtw8723du.c that is 
used by the USB device. The riscv changes may be needed, but we are not quite 
that far yet.

Larry

Re: Driver for rtw8723ds

[Martin Blumenstingl]

Hi Larry,

On Thu, May 4, 2023 at 9:25 PM Larry Finger <Larry.Finger@lwfinger.net> wrote:
[...]
> > Can you ask that user to test following commits?
> > https://github.com/xdarklight/linux/commit/
> > 3866a7a3702f7f24557f2c065b7d4088f7027466
> > https://github.com/xdarklight/linux/commit/
> > 66fd078556a6bf246337270b2e91d73c079fce2d
> >
> > Patches are trivial, but some testing needs to be done to confirm the driver
> > actually works as intended.
>
> Jernej,
>
> The user needs the rtw8723ds driver - the SDIO equivalent of rtw8723du.c that is
> used by the USB device. The riscv changes may be needed, but we are not quite
> that far yet.
Strange, can you please elaborate what you are seeing in terms of riscv?
The two commits that Jernej shared are for an update to rtw8723d.c [0]
and the addition of a rtw8723ds driver [1]. None of these mentions
riscv (at least that's what my tired eyes are seeing today).


Best regards,
Martin


[0] https://github.com/xdarklight/linux/commit/3866a7a3702f7f24557f2c065b7d4088f7027466
[1] https://github.com/xdarklight/linux/commit/66fd078556a6bf246337270b2e91d73c079fce2d

Re: Driver for rtw8723ds

[Jernej Škrabec]

Dne četrtek, 04. maj 2023 ob 21:45:31 CEST je Martin Blumenstingl napisal(a):
> Hi Larry,
> 
> On Thu, May 4, 2023 at 9:25 PM Larry Finger <Larry.Finger@lwfinger.net>
> wrote: [...]
> 
> > > Can you ask that user to test following commits?
> > > https://github.com/xdarklight/linux/commit/
> > > 3866a7a3702f7f24557f2c065b7d4088f7027466
> > > https://github.com/xdarklight/linux/commit/
> > > 66fd078556a6bf246337270b2e91d73c079fce2d
> > > 
> > > Patches are trivial, but some testing needs to be done to confirm the
> > > driver actually works as intended.
> > 
> > Jernej,
> > 
> > The user needs the rtw8723ds driver - the SDIO equivalent of rtw8723du.c
> > that is used by the USB device. The riscv changes may be needed, but we
> > are not quite that far yet.
> 
> Strange, can you please elaborate what you are seeing in terms of riscv?
> The two commits that Jernej shared are for an update to rtw8723d.c [0]
> and the addition of a rtw8723ds driver [1]. None of these mentions
> riscv (at least that's what my tired eyes are seeing today).
> 
> 
> Best regards,
> Martin
> 
> 
> [0]
> https://github.com/xdarklight/linux/commit/3866a7a3702f7f24557f2c065b7d4088
> f7027466 [1]
> https://github.com/xdarklight/linux/commit/66fd078556a6bf246337270b2e91d73c
> 079fce2d

Problem is that my e-mail client split commit links in two parts. If you 
combine hashes with rest it opens correct commits. Currently, it opens head of 
your master branch, which is by chance risc-v merge commit.

Best regards,
Jernej

Re: Driver for rtw8723ds

[Larry Finger]

On 5/4/23 15:13, Jernej Škrabec wrote:
> Dne četrtek, 04. maj 2023 ob 21:45:31 CEST je Martin Blumenstingl napisal(a):
>> Hi Larry,
>>
>> On Thu, May 4, 2023 at 9:25 PM Larry Finger <Larry.Finger@lwfinger.net>
>> wrote: [...]
>>
>>>> Can you ask that user to test following commits?
>>>> https://github.com/xdarklight/linux/commit/
>>>> 3866a7a3702f7f24557f2c065b7d4088f7027466
>>>> https://github.com/xdarklight/linux/commit/
>>>> 66fd078556a6bf246337270b2e91d73c079fce2d
>>>>
>>>> Patches are trivial, but some testing needs to be done to confirm the
>>>> driver actually works as intended.
>>>
>>> Jernej,
>>>
>>> The user needs the rtw8723ds driver - the SDIO equivalent of rtw8723du.c
>>> that is used by the USB device. The riscv changes may be needed, but we
>>> are not quite that far yet.
>>
>> Strange, can you please elaborate what you are seeing in terms of riscv?
>> The two commits that Jernej shared are for an update to rtw8723d.c [0]
>> and the addition of a rtw8723ds driver [1]. None of these mentions
>> riscv (at least that's what my tired eyes are seeing today).
>>
>>
>> Best regards,
>> Martin
>>
>>
>> [0]
>> https://github.com/xdarklight/linux/commit/3866a7a3702f7f24557f2c065b7d4088
>> f7027466 [1]
>> https://github.com/xdarklight/linux/commit/66fd078556a6bf246337270b2e91d73c
>> 079fce2d
> 
> Problem is that my e-mail client split commit links in two parts. If you
> combine hashes with rest it opens correct commits. Currently, it opens head of
> your master branch, which is by chance risc-v merge commit.

After I compared the links that Martin sent with yours, I figured out the problem.

I downloaded the patches and applied them to my rtw88 repo. The user that wanted 
rtw8723ds support has been notified. We will see how it goes.

Thanks,

Larry

Re: Driver for rtw8723ds

[Larry Finger]

Martin and Jernej,

It took a bit of working through some problems, but the user now has the 
rtw8723ds loading and starting. The first problem other than the mechanics of 
building and installing the driver on his SoC was that there is a second SDIO 
vendor ID/device ID for his device, namely 0x024c:0xd724. As I am carrying a 
local copy of sdio_ids.h, that was relatively easy to fix.

At the moment, when the device starts, the log shows the following:
[    3.640866] rtw_8723ds mmc1:0001:1: Firmware version 48.0.0, H2C version 0
[    3.645302] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.654556] sunxi-mmc 4021000.mmc: map DMA failed
[    3.659268] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10040): -22
[    3.665710] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.672499] sunxi-mmc 4021000.mmc: map DMA failed
[    3.677208] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10040): -22
[    3.683739] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.690528] sunxi-mmc 4021000.mmc: map DMA failed
[    3.695236] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10100): -22
[    3.873173] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.879978] sunxi-mmc 4021000.mmc: map DMA failed
[    3.884690] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10002): -22
[    3.891141] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.897931] sunxi-mmc 4021000.mmc: map DMA failed
[    3.902640] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10002): -22
[    3.909172] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.915962] sunxi-mmc 4021000.mmc: map DMA failed
[    3.920672] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10008): -22
[    3.927116] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
[    3.933906] sunxi-mmc 4021000.mmc: map DMA failed
[    3.938616] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10008): -22
  [ ok ]

This problem is beyond my knowledge of SDIO.

Thanks for any help you can give.

Larry

Re: Driver for rtw8723ds

[Martin Blumenstingl]

[-- Attachment #1: Type: text/plain, Size: 2788 bytes --]

Hi Larry,

On Mon, May 8, 2023 at 8:55 PM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>
> Martin and Jernej,
>
> It took a bit of working through some problems, but the user now has the
> rtw8723ds loading and starting. The first problem other than the mechanics of
> building and installing the driver on his SoC was that there is a second SDIO
> vendor ID/device ID for his device, namely 0x024c:0xd724. As I am carrying a
> local copy of sdio_ids.h, that was relatively easy to fix.
Thanks for this hint - I guess we need to find out how to name that
0xd724 device and add it's ID to sdio_ids.h when I upstream the
patches then.
But let's look at the other problem first.

> At the moment, when the device starts, the log shows the following:
> [    3.640866] rtw_8723ds mmc1:0001:1: Firmware version 48.0.0, H2C version 0
> [    3.645302] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.654556] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.659268] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10040): -22
> [    3.665710] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.672499] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.677208] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10040): -22
> [    3.683739] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.690528] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.695236] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10100): -22
> [    3.873173] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.879978] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.884690] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10002): -22
> [    3.891141] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.897931] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.902640] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10002): -22
> [    3.909172] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.915962] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.920672] rtw_8723ds mmc1:0001:1: sdio read16 failed (0x10008): -22
> [    3.927116] sunxi-mmc 4021000.mmc: unaligned scatterlist: os b00 length 2
> [    3.933906] sunxi-mmc 4021000.mmc: map DMA failed
> [    3.938616] rtw_8723ds mmc1:0001:1: sdio write16 failed (0x10008): -22
This looks like an issue with the Allwinner SDIO controller. We can
try to work around this with the attached patch.
Please note that I've only compile-tested that patch.
In theory it can hurt 16-bit register access (read/write) performance
a bit (since we now require two MMC commands instead of one). Whether
this can be measured in the real world is unknown to me. Let's see if
it fixes the observed issue first.


Best regards,
Martin

[-- Attachment #2: rtw88-byte-wise-word-io.diff --]
[-- Type: text/x-patch, Size: 870 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index af0459a79899..06fce7c3adda 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -87,11 +87,6 @@ static void rtw_sdio_writew(struct rtw_dev *rtwdev, u16 val, u32 addr,
 	u8 buf[2];
 	int i;
 
-	if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2)) {
-		sdio_writew(rtwsdio->sdio_func, val, addr, err_ret);
-		return;
-	}
-
 	*(__le16 *)buf = cpu_to_le16(val);
 
 	for (i = 0; i < 2; i++) {
@@ -125,9 +120,6 @@ static u16 rtw_sdio_readw(struct rtw_dev *rtwdev, u32 addr, int *err_ret)
 	u8 buf[2];
 	int i;
 
-	if (rtw_sdio_use_memcpy_io(rtwdev, addr, 2))
-		return sdio_readw(rtwsdio->sdio_func, addr, err_ret);
-
 	for (i = 0; i < 2; i++) {
 		buf[i] = sdio_readb(rtwsdio->sdio_func, addr + i, err_ret);
 		if (*err_ret)

Re: Driver for rtw8723ds

[Larry Finger]

On 5/8/23 15:16, Martin Blumenstingl wrote:
> This looks like an issue with the Allwinner SDIO controller. We can
> try to work around this with the attached patch.
> Please note that I've only compile-tested that patch.
> In theory it can hurt 16-bit register access (read/write) performance
> a bit (since we now require two MMC commands instead of one). Whether
> this can be measured in the real world is unknown to me. Let's see if
> it fixes the observed issue first.
> 

Martin,

I added that patch to the driver. The user reports that he was able to do a ping 
and an nslookup before it crashed with the following in the log:

[    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341 
head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
[    8.714254] ------------[ cut here ]------------
[    8.718867] kernel BUG at net/core/skbuff.c:200!
[    8.723481] Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
[    8.730261] Modules linked in: rtw_8723ds rtw_8723d rtw_sdio rtw_core 
mac80211 libarc4 cfg80211 rfkill ipv6
[    8.740024] CPU: 1 PID: 222 Comm: ksdioirqd/mmc1 Not tainted 6.4.0-rc1-dirty #1
[    8.747327] Hardware name: MangoPi MQ Quad (DT)
[    8.751852] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.758808] pc : skb_panic+0x58/0x5c
[    8.762391] lr : skb_panic+0x58/0x5c
[    8.765964] sp : ffff80000aa3bc20
[    8.769274] x29: ffff80000aa3bc30 x28: ffff000003e16dd8 x27: 0000000000000055
[    8.776409] x26: 000000000000e001 x25: ffff000003e94200 x24: ffff80000119f408
[    8.783543] x23: 0000000000000018 x22: ffff000003e12060 x21: ffff80000aa3bd28
[    8.790677] x20: 000000000000003a x19: ffff000003e94200 x18: 0000000000000000
[    8.797811] x17: 010006000cc00100 x16: ffff80000a22ffc0 x15: 0000000000000030
[    8.804945] x14: ffff80000a241b20 x13: 0000000000000339 x12: 0000000000000113
[    8.812079] x11: 7265766f5f626b73 x10: ffff80000a299b20 x9 : 00000000fffff000
[    8.819212] x8 : ffff80000a241b20 x7 : ffff80000a299b20 x6 : 0000000000000000
[    8.826346] x5 : 000000000000bff4 x4 : 0000000000000000 x3 : 0000000000000000
[    8.833479] x2 : 0000000000000000 x1 : ffff000003d6d880 x0 : 000000000000008b
[    8.840614] Call trace:
[    8.843057]  skb_panic+0x58/0x5c
[    8.846286]  skb_find_text+0x0/0xc4
[    8.849773]  rtw_sdio_rx_skb+0x50/0xc8 [rtw_sdio]
[    8.854488]  rtw_sdio_rxfifo_recv+0x1a0/0x24c [rtw_sdio]
[    8.859803]  rtw_sdio_handle_interrupt+0xf0/0x124 [rtw_sdio]
[    8.865465]  process_sdio_pending_irqs+0x5c/0x1c4
[    8.870169]  sdio_irq_thread+0x84/0x178
[    8.874003]  kthread+0x118/0x11c
[    8.877231]  ret_from_fork+0x10/0x20
[    8.880814] Code: f90007e9 b940b108 f90003e8 97cb9ac2 (d4210000)
[    8.886903] ---[ end trace 0000000000000000 ]---
[    8.891515] note: ksdioirqd/mmc1[222] exited with irqs disabled
[    8.897478] note: ksdioirqd/mmc1[222] exited with preempt_count 1
[    8.903732] ------------[ cut here ]------------
[    8.908348] WARNING: CPU: 1 PID: 0 at kernel/context_tracking.c:128 
ct_kernel_exit.constprop.0+0x98/0xa0
[    8.917828] Modules linked in: rtw_8723ds rtw_8723d rtw_sdio rtw_core 
mac80211 libarc4 cfg80211 rfkill ipv6
[    8.927590] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D 
6.4.0-rc1-dirty #1
[    8.935758] Hardware name: MangoPi MQ Quad (DT)
[    8.940282] pstate: 200003c5 (nzCv DAIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.947237] pc : ct_kernel_exit.constprop.0+0x98/0xa0
[    8.952285] lr : ct_idle_enter+0x10/0x1c
[    8.956206] sp : ffff80000a76bdd0
[    8.959516] x29: ffff80000a76bdd0 x28: 0000000000000000 x27: 0000000000000000
[    8.966650] x26: 0000000000000000 x25: ffff000002d68000 x24: 0000000000000000
[    8.973784] x23: 0000000000000000 x22: ffff80000a229b48 x21: ffff800009b8daf8
[    8.980917] x20: ffff80000a229a40 x19: ffff00003fd95a20 x18: 0000000000000000
[    8.988051] x17: 3830303030303030 x16: 3030303030303020 x15: 0000000000000001
[    8.995185] x14: 00000000000000cb x13: 0000000000000000 x12: 0000000000000001
[    9.002317] x11: 0000000000000000 x10: 0000000000000a60 x9 : ffff80000a76bd30
[    9.009452] x8 : ffff000002d68ac0 x7 : 0000000000000000 x6 : 00000000106f30a4
[    9.016584] x5 : 00ffffffffffffff x4 : 4000000000000002 x3 : ffff80000a76bdd0
[    9.023717] x2 : 4000000000000000 x1 : ffff800009b8ba20 x0 : ffff800009b8ba20
[    9.030851] Call trace:
[    9.033295]  ct_kernel_exit.constprop.0+0x98/0xa0
[    9.037998]  ct_idle_enter+0x10/0x1c
[    9.041572]  default_idle_call+0x1c/0x3c
[    9.045494]  do_idle+0x214/0x270
[    9.048724]  cpu_startup_entry+0x24/0x2c
[    9.052646]  secondary_start_kernel+0x130/0x154
[    9.057179]  __secondary_switched+0xb8/0xbc
[    9.061363] ---[ end trace 0000000000000000 ]---
[   14.048126] platform leds: deferred probe pending

Somehow skb->tail was greater than skb->end. Unfortunately I do not have access 
to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi 
MQ Quad.

Larry

Re: Driver for rtw8723ds

[Martin Blumenstingl]

On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
[...]
> I added that patch to the driver. The user reports that he was able to do a ping
> and an nslookup before it crashed with the following in the log:
That's some positive news alongside the crash log: it seems that a
part of the driver works! :-)

> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
[...]
> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
> MQ Quad.
I need to have a closer look at the pkg_offset and struct
rtw_rx_pkt_stat which we receive.
Recently my own MangoPI MQ-Quad arrived but I did not have the time to
set it up yet. I'll try to do so during the weekend so I can debug
this on my own.

Please ping me next week in case I haven't provided any update until then.


Best regards,
Martin

Re: Driver for rtw8723ds

[Larry Finger]

On 5/10/23 16:07, Martin Blumenstingl wrote:
> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> [...]
>> I added that patch to the driver. The user reports that he was able to do a ping
>> and an nslookup before it crashed with the following in the log:
> That's some positive news alongside the crash log: it seems that a
> part of the driver works! :-)
> 
>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
> [...]
>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
>> MQ Quad.
> I need to have a closer look at the pkg_offset and struct
> rtw_rx_pkt_stat which we receive.
> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> set it up yet. I'll try to do so during the weekend so I can debug
> this on my own.
> 
> Please ping me next week in case I haven't provided any update until then.

I have some test prints in to check for skb overrun. My initial indication is 
that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run 
should verify that. My changes will do a pr_warn_once() when the problem 
happens, and then drop the skb.

My contact reported that he had one run of 3 minutes before the problem 
happened, which is good news for most of the driver.

Larry

Re: Driver for rtw8723ds

[Jernej Škrabec]

Larry,

Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
> On 5/10/23 16:07, Martin Blumenstingl wrote:
> > On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> > [...]
> >> I added that patch to the driver. The user reports that he was able to do a ping
> >> and an nslookup before it crashed with the following in the log:
> > That's some positive news alongside the crash log: it seems that a
> > part of the driver works! :-)
> > 
> >> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> >> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
> > [...]
> >> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
> >> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
> >> MQ Quad.
> > I need to have a closer look at the pkg_offset and struct
> > rtw_rx_pkt_stat which we receive.
> > Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> > set it up yet. I'll try to do so during the weekend so I can debug
> > this on my own.
> > 
> > Please ping me next week in case I haven't provided any update until then.
> 
> I have some test prints in to check for skb overrun. My initial indication is 
> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run 
> should verify that. My changes will do a pr_warn_once() when the problem 
> happens, and then drop the skb.
> 
> My contact reported that he had one run of 3 minutes before the problem 
> happened, which is good news for most of the driver.

I may have discovered something interesting. rtl8723ds vendor driver has
following checks in RX data parsing code:
https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99

Those checks are absent in rtl8822bs vendor driver, which was my original
development platform for SDIO. This may indicate some kind of bug in FW
and/or HW.

I think that at least second check, which checks for exactly the case your
client experience, can be easily added and tested.

Best regards,
Jernej

Re: Driver for rtw8723ds

[Larry Finger]

On 5/13/23 05:23, Jernej Škrabec wrote:
> Larry,
> 
> Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
>> On 5/10/23 16:07, Martin Blumenstingl wrote:
>>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>>> [...]
>>>> I added that patch to the driver. The user reports that he was able to do a ping
>>>> and an nslookup before it crashed with the following in the log:
>>> That's some positive news alongside the crash log: it seems that a
>>> part of the driver works! :-)
>>>
>>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
>>> [...]
>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
>>>> MQ Quad.
>>> I need to have a closer look at the pkg_offset and struct
>>> rtw_rx_pkt_stat which we receive.
>>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
>>> set it up yet. I'll try to do so during the weekend so I can debug
>>> this on my own.
>>>
>>> Please ping me next week in case I haven't provided any update until then.
>>
>> I have some test prints in to check for skb overrun. My initial indication is
>> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
>> should verify that. My changes will do a pr_warn_once() when the problem
>> happens, and then drop the skb.
>>
>> My contact reported that he had one run of 3 minutes before the problem
>> happened, which is good news for most of the driver.
> 
> I may have discovered something interesting. rtl8723ds vendor driver has
> following checks in RX data parsing code:
> https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
> 
> Those checks are absent in rtl8822bs vendor driver, which was my original
> development platform for SDIO. This may indicate some kind of bug in FW
> and/or HW.
> 
> I think that at least second check, which checks for exactly the case your
> client experience, can be easily added and tested.

Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
        /* fix Hardware RX data error, drop whole recv_buffer */
        if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
                kfree_skb(skb);
                return;
        }
I think that duplicates the code in the vendor driver.

I have not heard from my user as to whether it helps. My communications with him 
are at https://github.com/lwfinger/rtl8723ds/issues/37.

Larry

Re: Driver for rtw8723ds

[Jernej Škrabec]

[-- Attachment #1: Type: text/plain, Size: 3294 bytes --]

Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
> On 5/13/23 05:23, Jernej Škrabec wrote:
> > Larry,
> > 
> > Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
> >> On 5/10/23 16:07, Martin Blumenstingl wrote:
> >>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> >>> [...]
> >>>> I added that patch to the driver. The user reports that he was able to do a ping
> >>>> and an nslookup before it crashed with the following in the log:
> >>> That's some positive news alongside the crash log: it seems that a
> >>> part of the driver works! :-)
> >>>
> >>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> >>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
> >>> [...]
> >>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
> >>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
> >>>> MQ Quad.
> >>> I need to have a closer look at the pkg_offset and struct
> >>> rtw_rx_pkt_stat which we receive.
> >>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> >>> set it up yet. I'll try to do so during the weekend so I can debug
> >>> this on my own.
> >>>
> >>> Please ping me next week in case I haven't provided any update until then.
> >>
> >> I have some test prints in to check for skb overrun. My initial indication is
> >> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
> >> should verify that. My changes will do a pr_warn_once() when the problem
> >> happens, and then drop the skb.
> >>
> >> My contact reported that he had one run of 3 minutes before the problem
> >> happened, which is good news for most of the driver.
> > 
> > I may have discovered something interesting. rtl8723ds vendor driver has
> > following checks in RX data parsing code:
> > https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
> > 
> > Those checks are absent in rtl8822bs vendor driver, which was my original
> > development platform for SDIO. This may indicate some kind of bug in FW
> > and/or HW.
> > 
> > I think that at least second check, which checks for exactly the case your
> > client experience, can be easily added and tested.
> 
> Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
>         /* fix Hardware RX data error, drop whole recv_buffer */
>         if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
>                 kfree_skb(skb);
>                 return;
>         }
> I think that duplicates the code in the vendor driver.
> 
> I have not heard from my user as to whether it helps. My communications with him 
> are at https://github.com/lwfinger/rtl8723ds/issues/37.

I had second part in mind (see attachment), but this is IMO only sanity check
and it will mask the issue. At this point I'm not sure if this is something that
can happen occasionally or is there additional bug in rtw88 code. I'll check
rtl8723ds c2h code in greater detail.

In any case, I would argue that all 3 patches in this thread are valid and
should be submitted upstream.

Best regards,
Jernej


[-- Attachment #2: sdio-size-check.patch --]
[-- Type: text/x-patch, Size: 826 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index af0459a79899..d69ce76ad0f4 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -975,7 +975,13 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 		curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
 				     RTW_SDIO_DATA_PTR_ALIGN);
 
-		if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {
+		if ((curr_pkt_len + pkt_desc_sz) > rx_len) {
+			dev_warn(rtwdev->dev, "Invalid RX packet size!");
+			dev_kfree_skb_any(skb);
+			return;
+		}
+
+		if ((curr_pkt_len + pkt_desc_sz) == rx_len) {
 			/* Use the original skb (with it's adjusted offset)
 			 * when processing the last (or even the only) entry to
 			 * have it's memory freed automatically.

Re: Driver for rtw8723ds

[Larry Finger]

On 5/13/23 15:13, Jernej Škrabec wrote:
> Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
>> On 5/13/23 05:23, Jernej Škrabec wrote:
>>> Larry,
>>>
>>> Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
>>>> On 5/10/23 16:07, Martin Blumenstingl wrote:
>>>>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>>>>> [...]
>>>>>> I added that patch to the driver. The user reports that he was able to do a ping
>>>>>> and an nslookup before it crashed with the following in the log:
>>>>> That's some positive news alongside the crash log: it seems that a
>>>>> part of the driver works! :-)
>>>>>
>>>>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
>>>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
>>>>> [...]
>>>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
>>>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
>>>>>> MQ Quad.
>>>>> I need to have a closer look at the pkg_offset and struct
>>>>> rtw_rx_pkt_stat which we receive.
>>>>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
>>>>> set it up yet. I'll try to do so during the weekend so I can debug
>>>>> this on my own.
>>>>>
>>>>> Please ping me next week in case I haven't provided any update until then.
>>>>
>>>> I have some test prints in to check for skb overrun. My initial indication is
>>>> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
>>>> should verify that. My changes will do a pr_warn_once() when the problem
>>>> happens, and then drop the skb.
>>>>
>>>> My contact reported that he had one run of 3 minutes before the problem
>>>> happened, which is good news for most of the driver.
>>>
>>> I may have discovered something interesting. rtl8723ds vendor driver has
>>> following checks in RX data parsing code:
>>> https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
>>>
>>> Those checks are absent in rtl8822bs vendor driver, which was my original
>>> development platform for SDIO. This may indicate some kind of bug in FW
>>> and/or HW.
>>>
>>> I think that at least second check, which checks for exactly the case your
>>> client experience, can be easily added and tested.
>>
>> Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
>>          /* fix Hardware RX data error, drop whole recv_buffer */
>>          if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
>>                  kfree_skb(skb);
>>                  return;
>>          }
>> I think that duplicates the code in the vendor driver.
>>
>> I have not heard from my user as to whether it helps. My communications with him
>> are at https://github.com/lwfinger/rtl8723ds/issues/37.
> 
> I had second part in mind (see attachment), but this is IMO only sanity check
> and it will mask the issue. At this point I'm not sure if this is something that
> can happen occasionally or is there additional bug in rtw88 code. I'll check
> rtl8723ds c2h code in greater detail.
> 
> In any case, I would argue that all 3 patches in this thread are valid and
> should be submitted upstream.

That patch looks good. I ill apply it to my rtw88 repo.

Larry

Re: Driver for rtw8723ds

[Jernej Škrabec]

Dne sobota, 13. maj 2023 ob 23:21:47 CEST je Larry Finger napisal(a):
> On 5/13/23 15:13, Jernej Škrabec wrote:
> > Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
> >> On 5/13/23 05:23, Jernej Škrabec wrote:
> >>> Larry,
> >>>
> >>> Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
> >>>> On 5/10/23 16:07, Martin Blumenstingl wrote:
> >>>>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> >>>>> [...]
> >>>>>> I added that patch to the driver. The user reports that he was able to do a ping
> >>>>>> and an nslookup before it crashed with the following in the log:
> >>>>> That's some positive news alongside the crash log: it seems that a
> >>>>> part of the driver works! :-)
> >>>>>
> >>>>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> >>>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
> >>>>> [...]
> >>>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
> >>>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
> >>>>>> MQ Quad.
> >>>>> I need to have a closer look at the pkg_offset and struct
> >>>>> rtw_rx_pkt_stat which we receive.
> >>>>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> >>>>> set it up yet. I'll try to do so during the weekend so I can debug
> >>>>> this on my own.
> >>>>>
> >>>>> Please ping me next week in case I haven't provided any update until then.
> >>>>
> >>>> I have some test prints in to check for skb overrun. My initial indication is
> >>>> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
> >>>> should verify that. My changes will do a pr_warn_once() when the problem
> >>>> happens, and then drop the skb.
> >>>>
> >>>> My contact reported that he had one run of 3 minutes before the problem
> >>>> happened, which is good news for most of the driver.
> >>>
> >>> I may have discovered something interesting. rtl8723ds vendor driver has
> >>> following checks in RX data parsing code:
> >>> https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
> >>>
> >>> Those checks are absent in rtl8822bs vendor driver, which was my original
> >>> development platform for SDIO. This may indicate some kind of bug in FW
> >>> and/or HW.
> >>>
> >>> I think that at least second check, which checks for exactly the case your
> >>> client experience, can be easily added and tested.
> >>
> >> Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
> >>          /* fix Hardware RX data error, drop whole recv_buffer */
> >>          if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
> >>                  kfree_skb(skb);
> >>                  return;
> >>          }
> >> I think that duplicates the code in the vendor driver.
> >>
> >> I have not heard from my user as to whether it helps. My communications with him
> >> are at https://github.com/lwfinger/rtl8723ds/issues/37.
> > 
> > I had second part in mind (see attachment), but this is IMO only sanity check
> > and it will mask the issue. At this point I'm not sure if this is something that
> > can happen occasionally or is there additional bug in rtw88 code. I'll check
> > rtl8723ds c2h code in greater detail.
> > 
> > In any case, I would argue that all 3 patches in this thread are valid and
> > should be submitted upstream.
> 
> That patch looks good. I ill apply it to my rtw88 repo.

I see that issue is still there. Next idea would be to check if RX aggregation
is the problem. Just commenting out call to rtw_sdio_enable_rx_aggregation()
is enough to disable it.

Best regards,
Jernej

Re: Driver for rtw8723ds

[Larry Finger]

On 5/15/23 11:43, Jernej Škrabec wrote:
> Dne sobota, 13. maj 2023 ob 23:21:47 CEST je Larry Finger napisal(a):
>> On 5/13/23 15:13, Jernej Škrabec wrote:
>>> Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
>>>> On 5/13/23 05:23, Jernej Škrabec wrote:
>>>>> Larry,
>>>>>
>>>>> Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
>>>>>> On 5/10/23 16:07, Martin Blumenstingl wrote:
>>>>>>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
>>>>>>> [...]
>>>>>>>> I added that patch to the driver. The user reports that he was able to do a ping
>>>>>>>> and an nslookup before it crashed with the following in the log:
>>>>>>> That's some positive news alongside the crash log: it seems that a
>>>>>>> part of the driver works! :-)
>>>>>>>
>>>>>>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
>>>>>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
>>>>>>> [...]
>>>>>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
>>>>>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
>>>>>>>> MQ Quad.
>>>>>>> I need to have a closer look at the pkg_offset and struct
>>>>>>> rtw_rx_pkt_stat which we receive.
>>>>>>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
>>>>>>> set it up yet. I'll try to do so during the weekend so I can debug
>>>>>>> this on my own.
>>>>>>>
>>>>>>> Please ping me next week in case I haven't provided any update until then.
>>>>>>
>>>>>> I have some test prints in to check for skb overrun. My initial indication is
>>>>>> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
>>>>>> should verify that. My changes will do a pr_warn_once() when the problem
>>>>>> happens, and then drop the skb.
>>>>>>
>>>>>> My contact reported that he had one run of 3 minutes before the problem
>>>>>> happened, which is good news for most of the driver.
>>>>>
>>>>> I may have discovered something interesting. rtl8723ds vendor driver has
>>>>> following checks in RX data parsing code:
>>>>> https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
>>>>>
>>>>> Those checks are absent in rtl8822bs vendor driver, which was my original
>>>>> development platform for SDIO. This may indicate some kind of bug in FW
>>>>> and/or HW.
>>>>>
>>>>> I think that at least second check, which checks for exactly the case your
>>>>> client experience, can be easily added and tested.
>>>>
>>>> Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
>>>>           /* fix Hardware RX data error, drop whole recv_buffer */
>>>>           if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
>>>>                   kfree_skb(skb);
>>>>                   return;
>>>>           }
>>>> I think that duplicates the code in the vendor driver.
>>>>
>>>> I have not heard from my user as to whether it helps. My communications with him
>>>> are at https://github.com/lwfinger/rtl8723ds/issues/37.
>>>
>>> I had second part in mind (see attachment), but this is IMO only sanity check
>>> and it will mask the issue. At this point I'm not sure if this is something that
>>> can happen occasionally or is there additional bug in rtw88 code. I'll check
>>> rtl8723ds c2h code in greater detail.
>>>
>>> In any case, I would argue that all 3 patches in this thread are valid and
>>> should be submitted upstream.
>>
>> That patch looks good. I ill apply it to my rtw88 repo.
> 
> I see that issue is still there. Next idea would be to check if RX aggregation
> is the problem. Just commenting out call to rtw_sdio_enable_rx_aggregation()
> is enough to disable it.

Jernej,

With aggregation disabled, we still get "Invalid RX packet size!" messages. I am 
changing the statement to log (curr_pkt_len + pkt_desc_sz) > rx_len. I will let 
you know when the OP responds.

Larry

Re: Driver for rtw8723ds

[Jernej Škrabec]

[-- Attachment #1: Type: text/plain, Size: 4701 bytes --]

Dne ponedeljek, 15. maj 2023 ob 22:23:39 CEST je Larry Finger napisal(a):
> On 5/15/23 11:43, Jernej Škrabec wrote:
> > Dne sobota, 13. maj 2023 ob 23:21:47 CEST je Larry Finger napisal(a):
> >> On 5/13/23 15:13, Jernej Škrabec wrote:
> >>> Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
> >>>> On 5/13/23 05:23, Jernej Škrabec wrote:
> >>>>> Larry,
> >>>>>
> >>>>> Dne sreda, 10. maj 2023 ob 23:47:24 CEST je Larry Finger napisal(a):
> >>>>>> On 5/10/23 16:07, Martin Blumenstingl wrote:
> >>>>>>> On Wed, May 10, 2023 at 12:02 AM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> >>>>>>> [...]
> >>>>>>>> I added that patch to the driver. The user reports that he was able to do a ping
> >>>>>>>> and an nslookup before it crashed with the following in the log:
> >>>>>>> That's some positive news alongside the crash log: it seems that a
> >>>>>>> part of the driver works! :-)
> >>>>>>>
> >>>>>>>> [    8.700626] skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> >>>>>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 dev:<NULL>
> >>>>>>> [...]
> >>>>>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do not have access
> >>>>>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 on the MangoPi
> >>>>>>>> MQ Quad.
> >>>>>>> I need to have a closer look at the pkg_offset and struct
> >>>>>>> rtw_rx_pkt_stat which we receive.
> >>>>>>> Recently my own MangoPI MQ-Quad arrived but I did not have the time to
> >>>>>>> set it up yet. I'll try to do so during the weekend so I can debug
> >>>>>>> this on my own.
> >>>>>>>
> >>>>>>> Please ping me next week in case I haven't provided any update until then.
> >>>>>>
> >>>>>> I have some test prints in to check for skb overrun. My initial indication is
> >>>>>> that the problem was in the c2h branch of rtw_sdio_rx_skb(), but my next run
> >>>>>> should verify that. My changes will do a pr_warn_once() when the problem
> >>>>>> happens, and then drop the skb.
> >>>>>>
> >>>>>> My contact reported that he had one run of 3 minutes before the problem
> >>>>>> happened, which is good news for most of the driver.
> >>>>>
> >>>>> I may have discovered something interesting. rtl8723ds vendor driver has
> >>>>> following checks in RX data parsing code:
> >>>>> https://github.com/lwfinger/rtl8723ds/blob/master/hal/rtl8723d/sdio/rtl8723ds_recv.c#L83-L99
> >>>>>
> >>>>> Those checks are absent in rtl8822bs vendor driver, which was my original
> >>>>> development platform for SDIO. This may indicate some kind of bug in FW
> >>>>> and/or HW.
> >>>>>
> >>>>> I think that at least second check, which checks for exactly the case your
> >>>>> client experience, can be easily added and tested.
> >>>>
> >>>> Thanks for this update. I added the following to the start of rtw_sdio_rx_skb():
> >>>>           /* fix Hardware RX data error, drop whole recv_buffer */
> >>>>           if (!(rtwdev->hal.rcr & BIT_ACRC32) && pkt_stat->crc_err) {
> >>>>                   kfree_skb(skb);
> >>>>                   return;
> >>>>           }
> >>>> I think that duplicates the code in the vendor driver.
> >>>>
> >>>> I have not heard from my user as to whether it helps. My communications with him
> >>>> are at https://github.com/lwfinger/rtl8723ds/issues/37.
> >>>
> >>> I had second part in mind (see attachment), but this is IMO only sanity check
> >>> and it will mask the issue. At this point I'm not sure if this is something that
> >>> can happen occasionally or is there additional bug in rtw88 code. I'll check
> >>> rtl8723ds c2h code in greater detail.
> >>>
> >>> In any case, I would argue that all 3 patches in this thread are valid and
> >>> should be submitted upstream.
> >>
> >> That patch looks good. I ill apply it to my rtw88 repo.
> > 
> > I see that issue is still there. Next idea would be to check if RX aggregation
> > is the problem. Just commenting out call to rtw_sdio_enable_rx_aggregation()
> > is enough to disable it.
> 
> Jernej,
> 
> With aggregation disabled, we still get "Invalid RX packet size!" messages. I am 
> changing the statement to log (curr_pkt_len + pkt_desc_sz) > rx_len. I will let 
> you know when the OP responds.

Yeah, I saw. I just find another possible reason, which fits nicely in current
situation. Vendor driver parses drv_info_sz and shift fields only if packet
is normal, e.g. not c2h type. However, rtw88 always parses those fields. It's
possible that they have some value which should be ignored on 8723ds. I
appended another patch to test.

If this doesn't work, I'm out of ideas.

Best regards,
Jernej


[-- Attachment #2: sdio-size.patch --]
[-- Type: text/x-patch, Size: 1119 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index af0459a79899..6f9fda758eb7 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -969,13 +969,20 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 		rx_desc = skb->data;
 		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
 					 &rx_status);
-		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
-			     pkt_stat.shift;
+		pkt_offset = pkt_desc_sz;
+		if (!pkt_stat->is_c2h)
+			pkt_offset += pkt_stat.drv_info_sz + pkt_stat.shift;
 
 		curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
 				     RTW_SDIO_DATA_PTR_ALIGN);
 
-		if ((curr_pkt_len + pkt_desc_sz) >= rx_len) {
+		if ((curr_pkt_len + pkt_desc_sz) > rx_len) {
+			dev_warn(rtwdev->dev, "Invalid RX packet size!");
+			dev_kfree_skb_any(skb);
+			return;
+		}
+
+		if ((curr_pkt_len + pkt_desc_sz) == rx_len) {
 			/* Use the original skb (with it's adjusted offset)
 			 * when processing the last (or even the only) entry to
 			 * have it's memory freed automatically.

Re: Driver for rtw8723ds

[Martin Blumenstingl]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

Hi Jernej,

On Mon, May 15, 2023 at 10:37 PM Jernej Škrabec
<jernej.skrabec@gmail.com> wrote:
[...]
> > With aggregation disabled, we still get "Invalid RX packet size!" messages. I am
> > changing the statement to log (curr_pkt_len + pkt_desc_sz) > rx_len. I will let
> > you know when the OP responds.
>
> Yeah, I saw. I just find another possible reason, which fits nicely in current
> situation. Vendor driver parses drv_info_sz and shift fields only if packet
> is normal, e.g. not c2h type. However, rtw88 always parses those fields. It's
> possible that they have some value which should be ignored on 8723ds. I
> appended another patch to test.
I tried that patch and it didn't work for me (I can't get the card to
assoc to my AP with that patch).
Additionally I tried a simplified version (attached) and it didn't work.

I'm out of time for today though so I cannot continue testing.


Best regards,
Martin

[-- Attachment #2: sdio-check-is-c2h.diff --]
[-- Type: text/x-patch, Size: 709 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 06fce7c3adda..f96e98818184 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -961,8 +961,10 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 		rx_desc = skb->data;
 		chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat,
 					 &rx_status);
-		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
-			     pkt_stat.shift;
+
+		pkt_offset = pkt_desc_sz;
+		if (!pkt_stat.is_c2h)
+			pkt_offset += pkt_stat.drv_info_sz + pkt_stat.shift;
 
 		curr_pkt_len = ALIGN(pkt_offset + pkt_stat.pkt_len,
 				     RTW_SDIO_DATA_PTR_ALIGN);

Re: Driver for rtw8723ds

[Martin Blumenstingl]

[-- Attachment #1: Type: text/plain, Size: 1647 bytes --]

On Mon, May 15, 2023 at 11:11 PM Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Jernej,
>
> On Mon, May 15, 2023 at 10:37 PM Jernej Škrabec
> <jernej.skrabec@gmail.com> wrote:
> [...]
> > > With aggregation disabled, we still get "Invalid RX packet size!" messages. I am
> > > changing the statement to log (curr_pkt_len + pkt_desc_sz) > rx_len. I will let
> > > you know when the OP responds.
> >
> > Yeah, I saw. I just find another possible reason, which fits nicely in current
> > situation. Vendor driver parses drv_info_sz and shift fields only if packet
> > is normal, e.g. not c2h type. However, rtw88 always parses those fields. It's
> > possible that they have some value which should be ignored on 8723ds. I
> > appended another patch to test.
> I tried that patch and it didn't work for me (I can't get the card to
> assoc to my AP with that patch).
> Additionally I tried a simplified version (attached) and it didn't work.
>
> I'm out of time for today though so I cannot continue testing.
I took time during my lunch break for some more experiments and came
up with the attached patch (the vendor driver does something similar
and I only noticed that after I observed some rtw_rx_pkt_stat with
pkt_len being zero).
It survived 30 minutes of uptime, updating my system and several
iperf3 runs (in both directions).
iperf results:
- RX: 48 Mbit/s
- TX: 33 Mbit/s

And to be clear, those results are with:
- the word IO bugfix
- the initial two patches from this series
- Larry's addition of the second RTL8723DS SDIO ID
- the attached patch


Best regards,
Martin

[-- Attachment #2: rtw88-sdio-rx-isr-check-rx-request-bit.diff --]
[-- Type: text/x-patch, Size: 1193 bytes --]

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 06fce7c3adda..a94c4e15204b 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -998,7 +998,7 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 
 static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 {
-	u32 rx_len, total_rx_bytes = 0;
+	u32 rx_len, hisr, total_rx_bytes = 0;
 
 	while (total_rx_bytes < SZ_64K) {
 		if (rtw_chip_wcpu_11n(rtwdev))
@@ -1012,6 +1012,18 @@ static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 		rtw_sdio_rxfifo_recv(rtwdev, rx_len);
 
 		total_rx_bytes += rx_len;
+
+		/* Stop if no more RX requests are pending, even if rx_len
+		 * could be greater than zero in the next iteration. This is
+		 * needed because the RX buffer may already contain data while
+		 * the chip is not done filling that buffer yet. Still reading
+		 * the buffer can result in empty packets or reading packets
+		 * where rtw_rx_pkt_stat.pkt_len points beyond the end of the
+		 * buffer.
+		 */
+		hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
+		if (!(hisr & REG_SDIO_HISR_RX_REQUEST))
+			break;
 	}
 }
 

Re: Driver for rtw8723ds

[Larry Finger]

On 5/16/23 05:22, Martin Blumenstingl wrote:
> On Mon, May 15, 2023 at 11:11 PM Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>
>> Hi Jernej,
>>
>> On Mon, May 15, 2023 at 10:37 PM Jernej Škrabec
>> <jernej.skrabec@gmail.com> wrote:
>> [...]
>>>> With aggregation disabled, we still get "Invalid RX packet size!" messages. I am
>>>> changing the statement to log (curr_pkt_len + pkt_desc_sz) > rx_len. I will let
>>>> you know when the OP responds.
>>>
>>> Yeah, I saw. I just find another possible reason, which fits nicely in current
>>> situation. Vendor driver parses drv_info_sz and shift fields only if packet
>>> is normal, e.g. not c2h type. However, rtw88 always parses those fields. It's
>>> possible that they have some value which should be ignored on 8723ds. I
>>> appended another patch to test.
>> I tried that patch and it didn't work for me (I can't get the card to
>> assoc to my AP with that patch).
>> Additionally I tried a simplified version (attached) and it didn't work.
>>
>> I'm out of time for today though so I cannot continue testing.
> I took time during my lunch break for some more experiments and came
> up with the attached patch (the vendor driver does something similar
> and I only noticed that after I observed some rtw_rx_pkt_stat with
> pkt_len being zero).
> It survived 30 minutes of uptime, updating my system and several
> iperf3 runs (in both directions).
> iperf results:
> - RX: 48 Mbit/s
> - TX: 33 Mbit/s
> 
> And to be clear, those results are with:
> - the word IO bugfix
> - the initial two patches from this series
> - Larry's addition of the second RTL8723DS SDIO ID
> - the attached patch

Martin,

Please send me a copy of the version of sdio.c that works. It seems likely that 
I got mine all messed up as the OP at GitHub is getting lots of warnings from 
net/mac80211/rx.c:803.

I though I followed all the patches you and Jernej sent, but I must have messed 
something up.

Thanks,

Larry

Re: Driver for rtw8723ds

[Martin Blumenstingl]

Hi Larry,

On Tue, May 16, 2023 at 8:31 PM Larry Finger <Larry.Finger@lwfinger.net> wrote:
[...]
> >> I'm out of time for today though so I cannot continue testing.
> > I took time during my lunch break for some more experiments and came
> > up with the attached patch (the vendor driver does something similar
> > and I only noticed that after I observed some rtw_rx_pkt_stat with
> > pkt_len being zero).
> > It survived 30 minutes of uptime, updating my system and several
> > iperf3 runs (in both directions).
> > iperf results:
> > - RX: 48 Mbit/s
> > - TX: 33 Mbit/s
> >
> > And to be clear, those results are with:
> > - the word IO bugfix
> > - the initial two patches from this series
> > - Larry's addition of the second RTL8723DS SDIO ID
> > - the attached patch
>
> Martin,
>
> Please send me a copy of the version of sdio.c that works. It seems likely that
> I got mine all messed up as the OP at GitHub is getting lots of warnings from
> net/mac80211/rx.c:803.
Please take the file from [0] - this is how I successfully tested it.


Best regards,
Martin


[0] https://github.com/xdarklight/linux/commits/mangopi-mq-quad-20230516

Re: Driver for rtw8723ds

[Larry Finger]

On 5/16/23 13:48, Martin Blumenstingl wrote:
> Hi Larry,
> 
> On Tue, May 16, 2023 at 8:31 PM Larry Finger <Larry.Finger@lwfinger.net> wrote:
> [...]
>>>> I'm out of time for today though so I cannot continue testing.
>>> I took time during my lunch break for some more experiments and came
>>> up with the attached patch (the vendor driver does something similar
>>> and I only noticed that after I observed some rtw_rx_pkt_stat with
>>> pkt_len being zero).
>>> It survived 30 minutes of uptime, updating my system and several
>>> iperf3 runs (in both directions).
>>> iperf results:
>>> - RX: 48 Mbit/s
>>> - TX: 33 Mbit/s
>>>
>>> And to be clear, those results are with:
>>> - the word IO bugfix
>>> - the initial two patches from this series
>>> - Larry's addition of the second RTL8723DS SDIO ID
>>> - the attached patch
>>
>> Martin,
>>
>> Please send me a copy of the version of sdio.c that works. It seems likely that
>> I got mine all messed up as the OP at GitHub is getting lots of warnings from
>> net/mac80211/rx.c:803.
> Please take the file from [0] - this is how I successfully tested it.

Thanks. I got it and replaced the one in rtw88. Speedtest shows him getting a 
little over 40 Mbps.

Thanks for the help from both of you,

Larry