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:   Kalle Valo <kvalo@kernel.org>
To:     Ulf Hansson <ulf.hansson@linaro.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>
Subject: Re: [PATCH wireless-next v2 1/4] wifi: rtw88: sdio: Check the HISR RX_REQUEST bit in rtw_sdio_rx_isr()
References: <20230522202425.1827005-2-martin.blumenstingl@googlemail.com>
        <168503099830.22756.1444846830853755949.kvalo@kernel.org>
        <CAPDyKFr5X+rhx=0mUiy_w0Aik9re5WD71xn-H_hrbTDR1L08ww@mail.gmail.com>
Date:   Fri, 26 May 2023 17:14:07 +0300
In-Reply-To: <CAPDyKFr5X+rhx=0mUiy_w0Aik9re5WD71xn-H_hrbTDR1L08ww@mail.gmail.com>
        (Ulf Hansson's message of "Fri, 26 May 2023 15:11:24 +0200")
Message-ID: <87ttvzji34.fsf@kernel.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Ulf Hansson <ulf.hansson@linaro.org> writes:

> 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.

We don't rebase wireless trees so I can't add it anymore, sorry. But
thanks for reviewing it.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches