On Mon, May 15, 2023 at 11:11 PM Martin Blumenstingl
<martin.blumenstingl@xxxxxxxxxxxxxx> wrote:
>
> Hi Jernej,
>
> On Mon, May 15, 2023 at 10:37 PM Jernej Škrabec
> <jernej.skrabec@xxxxxxxxx> 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
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;
}
}
