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;
MIME-Version: 1.0
References: <20230522202425.1827005-2-martin.blumenstingl@googlemail.com> <168503099830.22756.1444846830853755949.kvalo@kernel.org>
In-Reply-To: <168503099830.22756.1444846830853755949.kvalo@kernel.org>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri, 26 May 2023 15:11:24 +0200
Message-ID: <CAPDyKFr5X+rhx=0mUiy_w0Aik9re5WD71xn-H_hrbTDR1L08ww@mail.gmail.com>
Subject: Re: [PATCH wireless-next v2 1/4] wifi: rtw88: sdio: Check the HISR
 RX_REQUEST bit in rtw_sdio_rx_isr()
To:     Kalle Valo <kvalo@kernel.org>
Cc:     Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        linux-wireless@vger.kernel.org, linux-mmc@vger.kernel.org,
        linux-kernel@vger.kernel.org, tony0620emma@gmail.com,
        Peter Robinson <pbrobinson@gmail.com>,
        Ping-Ke Shih <pkshih@realtek.com>, jernej.skrabec@gmail.com,
        Larry Finger <Larry.Finger@lwfinger.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, 25 May 2023 at 18:10, Kalle Valo <kvalo@kernel.org> wrote:
>
> Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
>
> > rtw_sdio_rx_isr() is responsible for receiving data from the wifi chip
> > and is called from the SDIO interrupt handler when the interrupt status
> > register (HISR) has the RX_REQUEST bit set. After the first batch of
> > data has been processed by the driver the wifi chip may have more data
> > ready to be read, which is managed by a loop in rtw_sdio_rx_isr().
> >
> > It turns out that there are cases where the RX buffer length (from the
> > REG_SDIO_RX0_REQ_LEN register) does not match the data we receive. The
> > following two cases were observed with a RTL8723DS card:
> > - RX length is smaller than the total packet length including overhead
> >   and actual data bytes (whose length is part of the buffer we read from
> >   the wifi chip and is stored in rtw_rx_pkt_stat.pkt_len). This can
> >   result in errors like:
> >     skbuff: skb_over_panic: text:ffff8000011924ac len:3341 put:3341
> >   (one case observed was: RX buffer length = 1536 bytes but
> >    rtw_rx_pkt_stat.pkt_len = 1546 bytes, this is not valid as it means
> >    we need to read beyond the end of the buffer)
> > - RX length looks valid but rtw_rx_pkt_stat.pkt_len is zero
> >
> > Check if the RX_REQUEST is set in the HISR register for each iteration
> > inside rtw_sdio_rx_isr(). This mimics what the RTL8723DS vendor driver
> > does and makes the driver only read more data if the RX_REQUEST bit is
> > set (which seems to be a way for the card's hardware or firmware to
> > tell the host that data is ready to be processed).
> >
> > For RTW_WCPU_11AC chips this check is not needed. The RTL8822BS vendor
> > driver for example states that this check is unnecessary (but still uses
> > it) and the RTL8822CS drops this check entirely.
> >
> > Reviewed-by: Ping-Ke Shih <pkshih@realtek.com>
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
> 4 patches applied to wireless-next.git, thanks.
>
> e967229ead0e wifi: rtw88: sdio: Check the HISR RX_REQUEST bit in rtw_sdio_rx_isr()
> 9be20a822327 wifi: rtw88: rtw8723d: Implement RTL8723DS (SDIO) efuse parsing
> 09fcdbd28404 mmc: sdio: Add/rename SDIO ID of the RTL8723DS SDIO wifi cards
> a3b125ceb45e wifi: rtw88: Add support for the SDIO based RTL8723DS chipset

If it's not too late, feel free to add my ack for patch3. Nevermind,
if it adds additional work for you.

Kind regards
Uffe