Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jernej =?utf-8?B?xaBrcmFiZWM=?= <jernej.skrabec@gmail.com>
To:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Larry Finger <Larry.Finger@lwfinger.net>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Subject: Re: Driver for rtw8723ds
Date:   Mon, 15 May 2023 18:43:37 +0200
Message-ID: <3162376.5fSG56mABF@jernej-laptop>
In-Reply-To: <fc20e35c-c7e6-cfa5-bdc5-f88ceee12f71@lwfinger.net>
References: <72a8eeb1-c91c-80a7-5a09-1b7963e0996b@lwfinger.net>
 <21872829.EfDdHjke4D@jernej-laptop>
 <fc20e35c-c7e6-cfa5-bdc5-f88ceee12f71@lwfinger.net>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Dne sobota, 13. maj 2023 ob 23:21:47 CEST je Larry Finger napisal(a):
> On 5/13/23 15:13, Jernej =C5=A0krabec wrote:
> > Dne sobota, 13. maj 2023 ob 21:55:51 CEST je Larry Finger napisal(a):
> >> On 5/13/23 05:23, Jernej =C5=A0krabec 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=E2=80=AFAM Larry Finger <Larry.Finger=
@lwfinger.net> wrote:
> >>>>> [...]
> >>>>>> I added that patch to the driver. The user reports that he was abl=
e 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:3=
341 put:3341
> >>>>>> head:ffff000003b3c000 data:ffff000003b3c040 tail:0xd4d end:0x2c0 d=
ev:<NULL>
> >>>>> [...]
> >>>>>> Somehow skb->tail was greater than skb->end. Unfortunately I do no=
t have access
> >>>>>> to gdb to tell you what line corresponds to rtw_sdio_rx_skb+0x50 o=
n 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 unti=
l then.
> >>>>
> >>>> I have some test prints in to check for skb overrun. My initial indi=
cation 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 pro=
blem
> >>>> happens, and then drop the skb.
> >>>>
> >>>> My contact reported that he had one run of 3 minutes before the prob=
lem
> >>>> 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/r=
tl8723ds_recv.c#L83-L99
> >>>
> >>> Those checks are absent in rtl8822bs vendor driver, which was my orig=
inal
> >>> development platform for SDIO. This may indicate some kind of bug in =
=46W
> >>> 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 communication=
s with him
> >> are at https://github.com/lwfinger/rtl8723ds/issues/37.
> >=20
> > 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 somet=
hing that
> > can happen occasionally or is there additional bug in rtw88 code. I'll =
check
> > rtl8723ds c2h code in greater detail.
> >=20
> > In any case, I would argue that all 3 patches in this thread are valid =
and
> > should be submitted upstream.
>=20
> 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 aggregat=
ion
is the problem. Just commenting out call to rtw_sdio_enable_rx_aggregation()
is enough to disable it.

Best regards,
Jernej