[PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Alastair D'Silva]

In rtw_sdio_handle_interrupt(), the HISR status register is cleared using
Write-1-to-Clear (W1C) semantics. However, the driver masks out the
REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before writing
it back, causing a 0 to be written to that bit.

This prevents the RX request interrupt from being acknowledged and
cleared by the hardware, trapping the CPU core in an infinite interrupt
storm loop upon receiving packets and triggering RCU stalls and system
lockups.

Remove the masking of REG_SDIO_HISR_RX_REQUEST so that the interrupt
status is correctly written back as a 1 and acknowledged.

Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---

Notes:
    Tested on Mellow Fly-C5 (Allwinner H618) with onboard Realtek RTL8821CS
    SDIO Wi-Fi under Armbian, resolving immediate RCU stalls upon packet arrival
    and achieving stable Wi-Fi throughput.

 drivers/net/wireless/realtek/rtw88/sdio.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 5b40d74b16ee..73686b462957 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1090,10 +1090,8 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 
 	if (hisr & REG_SDIO_HISR_TXERR)
 		rtw_sdio_tx_err_isr(rtwdev);
-	if (hisr & REG_SDIO_HISR_RX_REQUEST) {
-		hisr &= ~REG_SDIO_HISR_RX_REQUEST;
+	if (hisr & REG_SDIO_HISR_RX_REQUEST)
 		rtw_sdio_rx_isr(rtwdev);
-	}
 
 	rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
 
-- 
2.53.0

RE: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Ping-Ke Shih]

Hi Martin,

Alastair D'Silva <alastair@d-silva.org> wrote:
> In rtw_sdio_handle_interrupt(), the HISR status register is cleared using
> Write-1-to-Clear (W1C) semantics. However, the driver masks out the
> REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before writing
> it back, causing a 0 to be written to that bit.

I feel this patch makes sense.

Did you remember why you clear the bit locally? Can you share vendor
driver you referenced?

Ping-Ke

RE: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Ping-Ke Shih]

Ping-Ke Shih wrote:
> 
> Hi Martin,
> 
> Alastair D'Silva <alastair@d-silva.org> wrote:
> > In rtw_sdio_handle_interrupt(), the HISR status register is cleared using
> > Write-1-to-Clear (W1C) semantics. However, the driver masks out the
> > REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before writing
> > it back, causing a 0 to be written to that bit.
> 
> I feel this patch makes sense.
> 
> Did you remember why you clear the bit locally? Can you share vendor
> driver you referenced?
> 

I asked internal expert who said this bit will be cleared automatically
if all packets in RX buffer are received. If we clear this bit here,
the interrupt will not raise again if no newly incoming packet even
there are remaining packets in RX buffer. 

Checking rtw_sdio_rx_isr(), we can see it reads hisr for each iteration,
which this is also the evidence that the bit will be cleared automatically.

Therefore, we need to dig further why the RX buffer can't be empty
and get stuck. 

Ping-Ke

Re: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Alastair D'Silva]

On Thu, 2026-09-17 at 09:09 +0000, Ping-Ke Shih wrote:
> Ping-Ke Shih wrote:
> > 
> > Hi Martin,
> > 
> > Alastair D'Silva <alastair@d-silva.org> wrote:
> > > In rtw_sdio_handle_interrupt(), the HISR status register is
> > > cleared using
> > > Write-1-to-Clear (W1C) semantics. However, the driver masks out
> > > the
> > > REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before
> > > writing
> > > it back, causing a 0 to be written to that bit.
> > 
> > I feel this patch makes sense.
> > 
> > Did you remember why you clear the bit locally? Can you share
> > vendor
> > driver you referenced?
> > 
> 
> I asked internal expert who said this bit will be cleared
> automatically
> if all packets in RX buffer are received. If we clear this bit here,
> the interrupt will not raise again if no newly incoming packet even
> there are remaining packets in RX buffer. 
> 
> Checking rtw_sdio_rx_isr(), we can see it reads hisr for each
> iteration,
> which this is also the evidence that the bit will be cleared
> automatically.
> 
> Therefore, we need to dig further why the RX buffer can't be empty
> and get stuck. 
> 
> Ping-Ke
> 

Hi Ping-Ke and Martin,

Thank you for following up and consulting with the internal hardware
team. We have extensive test telemetry and logs from our bring-up of
the Mellow Fly-C5 board in Armbian that provide full context on this
issue.

1. Test Platform & Failure Telemetry:
-------------------------------------
- Hardware: Mellow Fly-C5 (Allwinner H618 SoC, sun50i-h618).

- Wi-Fi Chip: Onboard Realtek RTL8821CS connected over SDIO (mmc1 /
sunxi-mmc).

- Kernels: Tested on mainline Linux 6.18 and 7.x branches using
rtw88_8821cs.

- Reproduction: On stock mainline kernels, immediately upon interface
bring-up and association (wlan0: associated), the CPU core servicing
the SDIO IRQ became 100% pegged in an interrupt storm, triggering RCU
stalls:
    rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
            0-...0: (1 GPs behind) idle=... softirq=...
    rcu: rcu_preempt kthread starved for 5126 jiffies!

- Verification: With this patch applied (leaving 
REG_SDIO_HISR_RX_REQUEST set so W1C acknowledges it), the board
completed 100/100 consecutive reboots under active network traffic
(continuous ping, SSH, Moonraker HTTP) with zero RCU stalls and zero
packet loss.


2. Why the bit was cleared locally (Vendor driver reference):
-------------------------------------------------------------

In Realtek's vendor drivers (such as rtl8822bs and rtl8723ds):
- In hal/rtl8822b/sdio/rtl8822bs_io.c:
    if (phal->sdio_hisr & BIT_RX_REQUEST_8822B) {
        /* No need to write 1 clear for RX_REQUEST */
        phal->sdio_hisr ^= BIT_RX_REQUEST_8822B;
- In include/hal_com_reg.h:
    MASK_SDIO_HISR_CLEAR explicitly excludes SDIO_HISR_RX_REQUEST.

Martin appears to have ported this comment and software-clearing
convention directly into rtw88_sdio_handle_interrupt().


3. Difference between 8051 and 3081 chips in rtw_sdio_rx_isr():
---------------------------------------------------------------

Regarding the observation that rtw_sdio_rx_isr() re-reads HISR in each
iteration:

Notice that in rtw_sdio_rx_isr() (sdio.c:1057-1073):
    if (rtw_chip_wcpu_8051(rtwdev)) {
        hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
    } else {
        /* RTW_WCPU_3081 chips have improved hardware or
         * firmware and can use rx_len unconditionally.
         */
        hisr = REG_SDIO_HISR_RX_REQUEST;
    }

RTL8821CS (and RTL8822C/B) has wlan_cpu == RTW_WCPU_3081. For 3081
chips, rtw_sdio_rx_isr() never actually reads REG_SDIO_HISR! It sets
hisr to REG_SDIO_HISR_RX_REQUEST unconditionally and loops purely based
on REG_SDIO_RX0_REQ_LEN.


4. The RX buffer IS empty when the storm occurs:
------------------------------------------------

The buffer is not getting stuck with unread data. In our testing:

- rtw_sdio_rx_isr() drains all available packets until
REG_SDIO_RX0_REQ_LEN reads as 0, and then breaks out of the loop.

- rtw_sdio_handle_interrupt() then writes back to REG_SDIO_HISR with
  REG_SDIO_HISR_RX_REQUEST masked out (writing 0).

- Because the bit was not cleared in hardware via W1C (and was not
automatically de-asserted by hardware when rx_len reached 0), the SDIO
host controller sees the IRQ line still asserted and immediately re-
invokes the handler.

- On re-entry, REG_SDIO_HISR_RX_REQUEST is still 1, but
REG_SDIO_RX0_REQ_LEN is 0. rtw_sdio_rx_isr() immediately breaks out,
hisr writes 0 again, and the CPU is trapped in a 100% spin loop.


5. Effect on future interrupts:
-------------------------------

Regarding the internal expert's concern that clearing the bit prevents
future interrupts: in our testing, writing 1 to clear
REG_SDIO_HISR_RX_REQUEST after the FIFO is drained did NOT prevent
subsequent RX interrupts. When new packets arrived over the air, the
hardware asserted REG_SDIO_HISR_RX_REQUEST again normally.

If there is concern about edge cases (such as hitting the 64KB
total_rx_bytes limit before the FIFO is completely empty), would it be
acceptable to only clear REG_SDIO_HISR_RX_REQUEST if
REG_SDIO_RX0_REQ_LEN reads 0, or re-read REG_SDIO_HISR at the end of
rtw_sdio_rx_isr()?

Cheers,

-- 
Alastair D'Silva

RE: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
> On Thu, 2026-09-17 at 09:09 +0000, Ping-Ke Shih wrote:
> > Ping-Ke Shih wrote:
> > >
> > > Hi Martin,
> > >
> > > Alastair D'Silva <alastair@d-silva.org> wrote:
> > > > In rtw_sdio_handle_interrupt(), the HISR status register is
> > > > cleared using
> > > > Write-1-to-Clear (W1C) semantics. However, the driver masks out
> > > > the
> > > > REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before
> > > > writing
> > > > it back, causing a 0 to be written to that bit.
> > >
> > > I feel this patch makes sense.
> > >
> > > Did you remember why you clear the bit locally? Can you share
> > > vendor
> > > driver you referenced?
> > >
> >
> > I asked internal expert who said this bit will be cleared
> > automatically
> > if all packets in RX buffer are received. If we clear this bit here,
> > the interrupt will not raise again if no newly incoming packet even
> > there are remaining packets in RX buffer.
> >
> > Checking rtw_sdio_rx_isr(), we can see it reads hisr for each
> > iteration,
> > which this is also the evidence that the bit will be cleared
> > automatically.
> >
> > Therefore, we need to dig further why the RX buffer can't be empty
> > and get stuck.
> >
> > Ping-Ke
> >
> 
> Hi Ping-Ke and Martin,
> 
> Thank you for following up and consulting with the internal hardware
> team. We have extensive test telemetry and logs from our bring-up of
> the Mellow Fly-C5 board in Armbian that provide full context on this
> issue.
> 
> 1. Test Platform & Failure Telemetry:
> -------------------------------------
> - Hardware: Mellow Fly-C5 (Allwinner H618 SoC, sun50i-h618).
> 
> - Wi-Fi Chip: Onboard Realtek RTL8821CS connected over SDIO (mmc1 /
> sunxi-mmc).
> 
> - Kernels: Tested on mainline Linux 6.18 and 7.x branches using
> rtw88_8821cs.
> 
> - Reproduction: On stock mainline kernels, immediately upon interface
> bring-up and association (wlan0: associated), the CPU core servicing
> the SDIO IRQ became 100% pegged in an interrupt storm, triggering RCU
> stalls:
>     rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
>             0-...0: (1 GPs behind) idle=... softirq=...
>     rcu: rcu_preempt kthread starved for 5126 jiffies!

Does it mean even if RX buffer is empty, interrupt is still triggered?

> 
> - Verification: With this patch applied (leaving
> REG_SDIO_HISR_RX_REQUEST set so W1C acknowledges it), the board
> completed 100/100 consecutive reboots under active network traffic
> (continuous ping, SSH, Moonraker HTTP) with zero RCU stalls and zero
> packet loss.
> 
> 
> 2. Why the bit was cleared locally (Vendor driver reference):
> -------------------------------------------------------------
> 
> In Realtek's vendor drivers (such as rtl8822bs and rtl8723ds):
> - In hal/rtl8822b/sdio/rtl8822bs_io.c:
>     if (phal->sdio_hisr & BIT_RX_REQUEST_8822B) {
>         /* No need to write 1 clear for RX_REQUEST */
>         phal->sdio_hisr ^= BIT_RX_REQUEST_8822B;
> - In include/hal_com_reg.h:
>     MASK_SDIO_HISR_CLEAR explicitly excludes SDIO_HISR_RX_REQUEST.
> 
> Martin appears to have ported this comment and software-clearing
> convention directly into rtw88_sdio_handle_interrupt().

Thanks for the info. It looks like every SDIO chip does the same thing.

> 
> 
> 3. Difference between 8051 and 3081 chips in rtw_sdio_rx_isr():
> ---------------------------------------------------------------
> 
> Regarding the observation that rtw_sdio_rx_isr() re-reads HISR in each
> iteration:
> 
> Notice that in rtw_sdio_rx_isr() (sdio.c:1057-1073):
>     if (rtw_chip_wcpu_8051(rtwdev)) {
>         hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
>     } else {
>         /* RTW_WCPU_3081 chips have improved hardware or
>          * firmware and can use rx_len unconditionally.
>          */
>         hisr = REG_SDIO_HISR_RX_REQUEST;
>     }
> 
> RTL8821CS (and RTL8822C/B) has wlan_cpu == RTW_WCPU_3081. For 3081
> chips, rtw_sdio_rx_isr() never actually reads REG_SDIO_HISR! It sets
> hisr to REG_SDIO_HISR_RX_REQUEST unconditionally and loops purely based
> on REG_SDIO_RX0_REQ_LEN.

Indeed. I also found that after sending previous mail...

> 
> 
> 4. The RX buffer IS empty when the storm occurs:
> ------------------------------------------------
> 
> The buffer is not getting stuck with unread data. In our testing:
> 
> - rtw_sdio_rx_isr() drains all available packets until
> REG_SDIO_RX0_REQ_LEN reads as 0, and then breaks out of the loop.
> 
> - rtw_sdio_handle_interrupt() then writes back to REG_SDIO_HISR with
>   REG_SDIO_HISR_RX_REQUEST masked out (writing 0).
> 
> - Because the bit was not cleared in hardware via W1C (and was not
> automatically de-asserted by hardware when rx_len reached 0), the SDIO
> host controller sees the IRQ line still asserted and immediately re-
> invokes the handler.
> 
> - On re-entry, REG_SDIO_HISR_RX_REQUEST is still 1, but
> REG_SDIO_RX0_REQ_LEN is 0. rtw_sdio_rx_isr() immediately breaks out,
> hisr writes 0 again, and the CPU is trapped in a 100% spin loop.

This answers my question above.

> 
> 
> 5. Effect on future interrupts:
> -------------------------------
> 
> Regarding the internal expert's concern that clearing the bit prevents
> future interrupts: in our testing, writing 1 to clear
> REG_SDIO_HISR_RX_REQUEST after the FIFO is drained did NOT prevent
> subsequent RX interrupts. When new packets arrived over the air, the
> hardware asserted REG_SDIO_HISR_RX_REQUEST again normally.
> 
> If there is concern about edge cases (such as hitting the 64KB
> total_rx_bytes limit before the FIFO is completely empty), would it be
> acceptable to only clear REG_SDIO_HISR_RX_REQUEST if
> REG_SDIO_RX0_REQ_LEN reads 0, or re-read REG_SDIO_HISR at the end of
> rtw_sdio_rx_isr()?

I guess there is a racing between W1C REG_SDIO_HISR_RX_REQUEST and
REG_SDIO_RX0_REQ_LEN == 0.

With a suggestion from internal, if we want to disable the RX request,
the better way is to disable/enable it by IMR. The corresponding 
functions are:

   rtw_sdio_enable_interrupt()
   rtw_sdio_disable_interrupt()

To avoid interrupt storm, I personally suggest to combine NAPI, which
disable interrupt when it processes RX budget (I think we can W1C
REG_SDIO_HISR_RX_REQUEST by the way). If (RX) budget is full, it can
poll again by estimated time. Until budget is not full, it re-enable
interrupt. 

Ping-Ke

Re: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Alastair D'Silva]

On Mon, 2026-09-21 at 02:44 +0000, Ping-Ke Shih wrote:
> Alastair D'Silva <alastair@d-silva.org> wrote:
> > On Thu, 2026-09-17 at 09:09 +0000, Ping-Ke Shih wrote:
> > > Ping-Ke Shih wrote:
> > > > 
> > > > Hi Martin,
> > > > 
> > > > Alastair D'Silva <alastair@d-silva.org> wrote:
> > > > > In rtw_sdio_handle_interrupt(), the HISR status register is
> > > > > cleared using
> > > > > Write-1-to-Clear (W1C) semantics. However, the driver masks out
> > > > > the
> > > > > REG_SDIO_HISR_RX_REQUEST bit in the local 'hisr' variable before
> > > > > writing
> > > > > it back, causing a 0 to be written to that bit.
> > > > 
> > > > I feel this patch makes sense.
> > > > 
> > > > Did you remember why you clear the bit locally? Can you share
> > > > vendor
> > > > driver you referenced?
> > > > 
> > > 
> > > I asked internal expert who said this bit will be cleared
> > > automatically
> > > if all packets in RX buffer are received. If we clear this bit here,
> > > the interrupt will not raise again if no newly incoming packet even
> > > there are remaining packets in RX buffer.
> > > 
> > > Checking rtw_sdio_rx_isr(), we can see it reads hisr for each
> > > iteration,
> > > which this is also the evidence that the bit will be cleared
> > > automatically.
> > > 
> > > Therefore, we need to dig further why the RX buffer can't be empty
> > > and get stuck.
> > > 
> > > Ping-Ke
> > > 
> > 
> > Hi Ping-Ke and Martin,
> > 
> > Thank you for following up and consulting with the internal hardware
> > team. We have extensive test telemetry and logs from our bring-up of
> > the Mellow Fly-C5 board in Armbian that provide full context on this
> > issue.
> > 
> > 1. Test Platform & Failure Telemetry:
> > -------------------------------------
> > - Hardware: Mellow Fly-C5 (Allwinner H618 SoC, sun50i-h618).
> > 
> > - Wi-Fi Chip: Onboard Realtek RTL8821CS connected over SDIO (mmc1 /
> > sunxi-mmc).
> > 
> > - Kernels: Tested on mainline Linux 6.18 and 7.x branches using
> > rtw88_8821cs.
> > 
> > - Reproduction: On stock mainline kernels, immediately upon interface
> > bring-up and association (wlan0: associated), the CPU core servicing
> > the SDIO IRQ became 100% pegged in an interrupt storm, triggering RCU
> > stalls:
> >     rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
> >             0-...0: (1 GPs behind) idle=... softirq=...
> >     rcu: rcu_preempt kthread starved for 5126 jiffies!
> 
> Does it mean even if RX buffer is empty, interrupt is still triggered?
> 
> > 
> > - Verification: With this patch applied (leaving
> > REG_SDIO_HISR_RX_REQUEST set so W1C acknowledges it), the board
> > completed 100/100 consecutive reboots under active network traffic
> > (continuous ping, SSH, Moonraker HTTP) with zero RCU stalls and zero
> > packet loss.
> > 
> > 
> > 2. Why the bit was cleared locally (Vendor driver reference):
> > -------------------------------------------------------------
> > 
> > In Realtek's vendor drivers (such as rtl8822bs and rtl8723ds):
> > - In hal/rtl8822b/sdio/rtl8822bs_io.c:
> >     if (phal->sdio_hisr & BIT_RX_REQUEST_8822B) {
> >         /* No need to write 1 clear for RX_REQUEST */
> >         phal->sdio_hisr ^= BIT_RX_REQUEST_8822B;
> > - In include/hal_com_reg.h:
> >     MASK_SDIO_HISR_CLEAR explicitly excludes SDIO_HISR_RX_REQUEST.
> > 
> > Martin appears to have ported this comment and software-clearing
> > convention directly into rtw88_sdio_handle_interrupt().
> 
> Thanks for the info. It looks like every SDIO chip does the same thing.
> 
> > 
> > 
> > 3. Difference between 8051 and 3081 chips in rtw_sdio_rx_isr():
> > ---------------------------------------------------------------
> > 
> > Regarding the observation that rtw_sdio_rx_isr() re-reads HISR in each
> > iteration:
> > 
> > Notice that in rtw_sdio_rx_isr() (sdio.c:1057-1073):
> >     if (rtw_chip_wcpu_8051(rtwdev)) {
> >         hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
> >     } else {
> >         /* RTW_WCPU_3081 chips have improved hardware or
> >          * firmware and can use rx_len unconditionally.
> >          */
> >         hisr = REG_SDIO_HISR_RX_REQUEST;
> >     }
> > 
> > RTL8821CS (and RTL8822C/B) has wlan_cpu == RTW_WCPU_3081. For 3081
> > chips, rtw_sdio_rx_isr() never actually reads REG_SDIO_HISR! It sets
> > hisr to REG_SDIO_HISR_RX_REQUEST unconditionally and loops purely based
> > on REG_SDIO_RX0_REQ_LEN.
> 
> Indeed. I also found that after sending previous mail...
> 
> > 
> > 
> > 4. The RX buffer IS empty when the storm occurs:
> > ------------------------------------------------
> > 
> > The buffer is not getting stuck with unread data. In our testing:
> > 
> > - rtw_sdio_rx_isr() drains all available packets until
> > REG_SDIO_RX0_REQ_LEN reads as 0, and then breaks out of the loop.
> > 
> > - rtw_sdio_handle_interrupt() then writes back to REG_SDIO_HISR with
> >   REG_SDIO_HISR_RX_REQUEST masked out (writing 0).
> > 
> > - Because the bit was not cleared in hardware via W1C (and was not
> > automatically de-asserted by hardware when rx_len reached 0), the SDIO
> > host controller sees the IRQ line still asserted and immediately re-
> > invokes the handler.
> > 
> > - On re-entry, REG_SDIO_HISR_RX_REQUEST is still 1, but
> > REG_SDIO_RX0_REQ_LEN is 0. rtw_sdio_rx_isr() immediately breaks out,
> > hisr writes 0 again, and the CPU is trapped in a 100% spin loop.
> 
> This answers my question above.
> 
> > 
> > 
> > 5. Effect on future interrupts:
> > -------------------------------
> > 
> > Regarding the internal expert's concern that clearing the bit prevents
> > future interrupts: in our testing, writing 1 to clear
> > REG_SDIO_HISR_RX_REQUEST after the FIFO is drained did NOT prevent
> > subsequent RX interrupts. When new packets arrived over the air, the
> > hardware asserted REG_SDIO_HISR_RX_REQUEST again normally.
> > 
> > If there is concern about edge cases (such as hitting the 64KB
> > total_rx_bytes limit before the FIFO is completely empty), would it be
> > acceptable to only clear REG_SDIO_HISR_RX_REQUEST if
> > REG_SDIO_RX0_REQ_LEN reads 0, or re-read REG_SDIO_HISR at the end of
> > rtw_sdio_rx_isr()?
> 
> I guess there is a racing between W1C REG_SDIO_HISR_RX_REQUEST and
> REG_SDIO_RX0_REQ_LEN == 0.
> 
> With a suggestion from internal, if we want to disable the RX request,
> the better way is to disable/enable it by IMR. The corresponding 
> functions are:
> 
>    rtw_sdio_enable_interrupt()
>    rtw_sdio_disable_interrupt()
> 
> To avoid interrupt storm, I personally suggest to combine NAPI, which
> disable interrupt when it processes RX budget (I think we can W1C
> REG_SDIO_HISR_RX_REQUEST by the way). If (RX) budget is full, it can
> poll again by estimated time. Until budget is not full, it re-enable
> interrupt. 
> 
> Ping-Ke

Thanks for the feedback.

Regarding using NAPI and IMR: While NAPI and IMR masking is the standard approach for PCIe,
implementing true NAPI for the SDIO interface is problematic. napi_poll runs in NET_RX_SOFTIRQ
context (which cannot sleep), but reading from the SDIO bus requires sdio_claim_host(), which takes
a mutex and must be able to sleep.

Fortunately, the kernel's MMC core already runs sdio_irq_thread in process context specifically to
handle this. If we simply leave the REG_SDIO_HISR_RX_REQUEST bit asserted in hardware (by
conditionally skipping the W1C), the sdio_irq_thread acts exactly like a NAPI polling loop. It will
immediately re-invoke our handler in the next cycle, yielding to the scheduler as needed, but safely
in process context.

This achieves the budget-limited polling you suggested, but avoids the heavy overhead of extra SDIO
bus transactions to toggle the IMR on and off.


Before I spin a V2 and run through my testing, can you please confirm this aligns with what you were
expecting?

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 5b40d74b16ee..933e51a980a8 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1037,7 +1037,16 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 	}
 }
 
-static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
+/**
+ * rtw_sdio_rx_isr() - Process RX packets from SDIO hardware buffer
+ * @rtwdev: The rtw88 device context
+ *
+ * Reads packets from the hardware RX FIFO until the buffer is empty or the
+ * 64K packet budget is reached.
+ *
+ * Return: true if packets are still pending (budget reached), false otherwise.
+ */
+static bool rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 {
 	u32 rx_len, hisr, total_rx_bytes = 0;
 
@@ -1072,6 +1081,8 @@ static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 			hisr = REG_SDIO_HISR_RX_REQUEST;
 		}
 	} while (total_rx_bytes < SZ_64K && hisr & REG_SDIO_HISR_RX_REQUEST);
+
+	return rx_len > 0;
 }
 
 static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
@@ -1091,8 +1102,14 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 	if (hisr & REG_SDIO_HISR_TXERR)
 		rtw_sdio_tx_err_isr(rtwdev);
 	if (hisr & REG_SDIO_HISR_RX_REQUEST) {
-		hisr &= ~REG_SDIO_HISR_RX_REQUEST;
-		rtw_sdio_rx_isr(rtwdev);
+		bool rx_pending = rtw_sdio_rx_isr(rtwdev);
+
+		/* If budget was reached and packets are still pending, do not W1C
+		 * the RX_REQUEST bit. This leaves the hardware IRQ asserted,
+		 * causing sdio_irq_thread to poll again like a NAPI loop.
+		 */
+		if (rx_pending)
+			hisr &= ~REG_SDIO_HISR_RX_REQUEST;
 	}
 
 	rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
-- 
2.53.0

-- 
Alastair D'Silva

RE: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
 
> On Mon, 2026-09-21 at 02:44 +0000, Ping-Ke Shih wrote:
> > Alastair D'Silva <alastair@d-silva.org> wrote:
> > > 5. Effect on future interrupts:
> > > -------------------------------
> > >
> > > Regarding the internal expert's concern that clearing the bit prevents
> > > future interrupts: in our testing, writing 1 to clear
> > > REG_SDIO_HISR_RX_REQUEST after the FIFO is drained did NOT prevent
> > > subsequent RX interrupts. When new packets arrived over the air, the
> > > hardware asserted REG_SDIO_HISR_RX_REQUEST again normally.
> > >
> > > If there is concern about edge cases (such as hitting the 64KB
> > > total_rx_bytes limit before the FIFO is completely empty), would it be
> > > acceptable to only clear REG_SDIO_HISR_RX_REQUEST if
> > > REG_SDIO_RX0_REQ_LEN reads 0, or re-read REG_SDIO_HISR at the end of
> > > rtw_sdio_rx_isr()?
> >
> > I guess there is a racing between W1C REG_SDIO_HISR_RX_REQUEST and
> > REG_SDIO_RX0_REQ_LEN == 0.
> >
> > With a suggestion from internal, if we want to disable the RX request,
> > the better way is to disable/enable it by IMR. The corresponding
> > functions are:
> >
> >    rtw_sdio_enable_interrupt()
> >    rtw_sdio_disable_interrupt()
> >
> > To avoid interrupt storm, I personally suggest to combine NAPI, which
> > disable interrupt when it processes RX budget (I think we can W1C
> > REG_SDIO_HISR_RX_REQUEST by the way). If (RX) budget is full, it can
> > poll again by estimated time. Until budget is not full, it re-enable
> > interrupt.
> >
> > Ping-Ke
> 
> Thanks for the feedback.
> 
> Regarding using NAPI and IMR: While NAPI and IMR masking is the standard approach for PCIe,
> implementing true NAPI for the SDIO interface is problematic.

As I know, NAPI is a pure software mechanism, and should not depend on interfaces.

Quickly search for the terms 'napi' and 'sdio' in wireless drivers: 

$ git grep napi drivers/net/wireless/ | grep sdio
drivers/net/wireless/ath/ath10k/sdio.c:         napi_schedule(&ar->napi);

At least ath10k does. 

> napi_poll runs in NET_RX_SOFTIRQ
> context (which cannot sleep), but reading from the SDIO bus requires sdio_claim_host(), which takes
> a mutex and must be able to sleep.


How about setting NAPI to threaded mode?

> 
> Fortunately, the kernel's MMC core already runs sdio_irq_thread in process context specifically to
> handle this. If we simply leave the REG_SDIO_HISR_RX_REQUEST bit asserted in hardware (by
> conditionally skipping the W1C), the sdio_irq_thread acts exactly like a NAPI polling loop. It will
> immediately re-invoke our handler in the next cycle, yielding to the scheduler as needed, but safely
> in process context.
> 
> This achieves the budget-limited polling you suggested, but avoids the heavy overhead of extra SDIO
> bus transactions to toggle the IMR on and off.
> 
> 
> Before I spin a V2 and run through my testing, can you please confirm this aligns with what you were
> expecting?

There is a racing between reading rx_len (REG_SDIO_RX0_REQ_LEN) and
writing hisr. 

                   rx_len         REG_SDIO_RX0_REQ_LEN      hisr
T1: read reg          0                     0               hisr
T2: func return       0                     5               hisr
T3: W1C               0                     5               &=REG_SDIO_HISR_RX_REQUEST

This is the info I got from internal consulter. You can do real
experiments (add a long delay after reading register when rx_len == 0)
to verify if this is correct.

Ping-Ke

Re: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Alastair D'Silva]

On Tue, 2026-09-22 at 01:25 +0000, Ping-Ke Shih wrote:
> Alastair D'Silva <alastair@d-silva.org> wrote:
>  
> > On Mon, 2026-09-21 at 02:44 +0000, Ping-Ke Shih wrote:
> > > Alastair D'Silva <alastair@d-silva.org> wrote:
> > > > 5. Effect on future interrupts:
> > > > -------------------------------
> > > > 
> > > > Regarding the internal expert's concern that clearing the bit prevents
> > > > future interrupts: in our testing, writing 1 to clear
> > > > REG_SDIO_HISR_RX_REQUEST after the FIFO is drained did NOT prevent
> > > > subsequent RX interrupts. When new packets arrived over the air, the
> > > > hardware asserted REG_SDIO_HISR_RX_REQUEST again normally.
> > > > 
> > > > If there is concern about edge cases (such as hitting the 64KB
> > > > total_rx_bytes limit before the FIFO is completely empty), would it be
> > > > acceptable to only clear REG_SDIO_HISR_RX_REQUEST if
> > > > REG_SDIO_RX0_REQ_LEN reads 0, or re-read REG_SDIO_HISR at the end of
> > > > rtw_sdio_rx_isr()?
> > > 
> > > I guess there is a racing between W1C REG_SDIO_HISR_RX_REQUEST and
> > > REG_SDIO_RX0_REQ_LEN == 0.
> > > 
> > > With a suggestion from internal, if we want to disable the RX request,
> > > the better way is to disable/enable it by IMR. The corresponding
> > > functions are:
> > > 
> > >    rtw_sdio_enable_interrupt()
> > >    rtw_sdio_disable_interrupt()
> > > 
> > > To avoid interrupt storm, I personally suggest to combine NAPI, which
> > > disable interrupt when it processes RX budget (I think we can W1C
> > > REG_SDIO_HISR_RX_REQUEST by the way). If (RX) budget is full, it can
> > > poll again by estimated time. Until budget is not full, it re-enable
> > > interrupt.
> > > 
> > > Ping-Ke
> > 
> > Thanks for the feedback.
> > 
> > Regarding using NAPI and IMR: While NAPI and IMR masking is the standard approach for PCIe,
> > implementing true NAPI for the SDIO interface is problematic.
> 
> As I know, NAPI is a pure software mechanism, and should not depend on interfaces.
> 
> Quickly search for the terms 'napi' and 'sdio' in wireless drivers: 
> 
> $ git grep napi drivers/net/wireless/ | grep sdio
> drivers/net/wireless/ath/ath10k/sdio.c:         napi_schedule(&ar->napi);
> 
> At least ath10k does. 
> 

While NAPI is a software scheduling mechanism, what can be executed inside
the napi->poll() callback is strictly governed by the execution context.
We audited the entire wireless tree, and ath10k is indeed the only SDIO
driver that registers a NAPI instance. However, ath10k does NOT access the
SDIO bus from within NAPI.

In ath10k (ath10k/sdio.c), the SDIO IRQ handler and a dedicated workqueue
(async_work_rx) synchronously read the hardware mailbox over SDIO and buffer
skbs into an internal queue (rx_head). Only after packets are already in host
memory does it call napi_schedule(), and its napi_poll callback merely
dequeues those buffered skbs and passes them to mac80211.

No other SDIO drivers in the kernel (brcmfmac, mwifiex, wilc1000, rsi, cw1200,
or Realtek's staging rtl8723bs) use NAPI for SDIO. In fact, mt76 explicitly
checks `if (mt76_is_sdio(mdev))` to bypass NAPI entirely and dispatch to a
worker thread. Replicating ath10k's architecture in rtw88 would require adding
intermediate RX queues, an asynchronous worker thread, and flow-control
plumbing, which adds significant complexity to what is otherwise a lean SDIO HCI.

> > napi_poll runs in NET_RX_SOFTIRQ
> > context (which cannot sleep), but reading from the SDIO bus requires sdio_claim_host(), which
> > takes
> > a mutex and must be able to sleep.
> 
> 
> How about setting NAPI to threaded mode?

Even with threaded NAPI enabled (netif_threaded_enable()), napi->poll()
cannot sleep.

In net/core/dev.c:napi_threaded_poll_loop() (line 7900), the kernel explicitly
disables bottom halves around the poll execution:
    local_bh_disable();
    __napi_poll(napi, &repoll);
    local_bh_enable();

Because bottom halves are disabled, napi->poll() runs in atomic context. Any
attempt to perform SDIO operations will trigger kernel panics:
1. sdio_claim_host() calls __mmc_claim_host(), which has an explicit
   might_sleep() and calls schedule() if the host is contended (e.g. by TX).
2. CMD52/CMD53 transfers submit MMC requests and wait on completions
   (wait_for_completion()), which sleep.

Calling SDIO functions inside napi->poll(), even in threaded mode, triggers:
"BUG: scheduling while atomic: napi/wlan0-0/.../0x00000101".

Furthermore, the MMC core already provides a dedicated kernel thread for SDIO
IRQs: ksdioirqd/<host> (drivers/mmc/core/sdio_irq.c). When an SDIO interrupt
fires, ksdioirqd wakes up in true, sleepable process context, claims the host
on the driver's behalf, and invokes rtw_sdio_handle_interrupt(). Recreating a
kthread polling mechanism on top of NAPI would effectively duplicate what
ksdioirqd is already doing.

> 
> > 
> > Fortunately, the kernel's MMC core already runs sdio_irq_thread in process context specifically
> > to
> > handle this. If we simply leave the REG_SDIO_HISR_RX_REQUEST bit asserted in hardware (by
> > conditionally skipping the W1C), the sdio_irq_thread acts exactly like a NAPI polling loop. It
> > will
> > immediately re-invoke our handler in the next cycle, yielding to the scheduler as needed, but
> > safely
> > in process context.
> > 
> > This achieves the budget-limited polling you suggested, but avoids the heavy overhead of extra
> > SDIO
> > bus transactions to toggle the IMR on and off.
> > 
> > 
> > Before I spin a V2 and run through my testing, can you please confirm this aligns with what you
> > were
> > expecting?
> 
> There is a racing between reading rx_len (REG_SDIO_RX0_REQ_LEN) and
> writing hisr. 
> 
>                    rx_len         REG_SDIO_RX0_REQ_LEN      hisr
> T1: read reg          0                     0               hisr
> T2: func return       0                     5               hisr
> T3: W1C               0                     5               &=REG_SDIO_HISR_RX_REQUEST
> 
> This is the info I got from internal consulter. You can do real
> experiments (add a long delay after reading register when rx_len == 0)
> to verify if this is correct.
> 
> Ping-Ke

You and your internal consultant are completely right about this race window.
If a packet arrives between reading rx_len == 0 and performing the W1C on HISR,
clearing REG_SDIO_HISR_RX_REQUEST will leave the packet stranded in the FIFO
without an asserted interrupt line.

Instead of overhauling the driver for NAPI, this race condition can be solved
cleanly and robustly using the IMR masking suggestion from your internal team:
In rtw_sdio_handle_interrupt():
    hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
    if (!hisr)
        return;
    /* Mask interrupts and acknowledge pending status bits */
    rtw_sdio_disable_interrupt(rtwdev);
    rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
    if (hisr & REG_SDIO_HISR_TXERR)
        rtw_sdio_tx_err_isr(rtwdev);
    if (hisr & REG_SDIO_HISR_RX_REQUEST)
        rtw_sdio_rx_isr(rtwdev);
    /* Re-enable interrupts: if new packets arrived during rx_isr,
     * REG_SDIO_HISR_RX_REQUEST was asserted in hardware; unmasking HIMR
     * immediately asserts SDIO DAT[1], causing ksdioirqd to run another pass.
     */
    rtw_sdio_enable_interrupt(rtwdev);

I prefer this approach as:
1. It completely closes the race window: any packet arriving during FIFO drainage
   latches REG_SDIO_HISR_RX_REQUEST in hardware. When rtw_sdio_enable_interrupt()
   restores HIMR, the interrupt line is asserted and ksdioirqd immediately
   services the new packet.
2. The overhead of toggling HIMR is just two 4-byte CMD52/CMD53 writes
   (~1-2 microseconds total per interrupt burst), which is negligible (<0.1%)
   compared to transferring payload data over SDIO.
3. It uses the existing rtw_sdio_disable_interrupt() and rtw_sdio_enable_interrupt()
   helpers and requires only ~4 lines of code changes without touching rx_isr.

Alternatively, if we prefer not to touch HIMR, we can perform a drain-and-recheck
inside rtw_sdio_rx_isr():
  when rx_len reads 0, issue W1C to REG_SDIO_HISR_RX_REQUEST and immediately re-read
REG_SDIO_RX0_REQ_LEN.
  If it reads > 0, continue draining, otherwise break.

I'm not a fan of this approach though, it feels clunky and requires the next buffer
round to be completely drained, resulting in the code consuming more than the 64kb
budget it was allowed.

Toggling HIMR as outlined above is the cleanest and most standard pattern.
Please let me know if this approach is acceptable to you, and I will prepare
and submit v2.

-- 
Alastair D'Silva

RE: [PATCH] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
> Instead of overhauling the driver for NAPI, this race condition can be solved
> cleanly and robustly using the IMR masking suggestion from your internal team:
> In rtw_sdio_handle_interrupt():
>     hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
>     if (!hisr)
>         return;
>     /* Mask interrupts and acknowledge pending status bits */
>     rtw_sdio_disable_interrupt(rtwdev);
>     rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
>     if (hisr & REG_SDIO_HISR_TXERR)
>         rtw_sdio_tx_err_isr(rtwdev);
>     if (hisr & REG_SDIO_HISR_RX_REQUEST)
>         rtw_sdio_rx_isr(rtwdev);
>     /* Re-enable interrupts: if new packets arrived during rx_isr,
>      * REG_SDIO_HISR_RX_REQUEST was asserted in hardware; unmasking HIMR
>      * immediately asserts SDIO DAT[1], causing ksdioirqd to run another pass.
>      */
>     rtw_sdio_enable_interrupt(rtwdev);

Yes, I prefer this kind of flow as well. It looks very similar to what PCI does.

> 
> I prefer this approach as:
> 1. It completely closes the race window: any packet arriving during FIFO drainage
>    latches REG_SDIO_HISR_RX_REQUEST in hardware. When rtw_sdio_enable_interrupt()
>    restores HIMR, the interrupt line is asserted and ksdioirqd immediately
>    services the new packet.

Please do the same experiments to ensure it doesn't get stuck in FIFO.


> 2. The overhead of toggling HIMR is just two 4-byte CMD52/CMD53 writes
>    (~1-2 microseconds total per interrupt burst), which is negligible (<0.1%)
>    compared to transferring payload data over SDIO.

I have lack knowledge of SDIO, so I can't judge this.
Can you design experiments as evidence?


> 3. It uses the existing rtw_sdio_disable_interrupt() and rtw_sdio_enable_interrupt()
>    helpers and requires only ~4 lines of code changes without touching rx_isr.

Make sense. Only IMR is affected. 

> 
> Alternatively, if we prefer not to touch HIMR, we can perform a drain-and-recheck
> inside rtw_sdio_rx_isr():
>   when rx_len reads 0, issue W1C to REG_SDIO_HISR_RX_REQUEST and immediately re-read
> REG_SDIO_RX0_REQ_LEN.
>   If it reads > 0, continue draining, otherwise break.

Honestly I don't read and think this in detail, but I can't understand it can
avoid racing. As you are not a fan of this approach, just ignore this.

> 
> Toggling HIMR as outlined above is the cleanest and most standard pattern.
> Please let me know if this approach is acceptable to you, and I will prepare
> and submit v2.

I'm okay this the proposal. 

I heard from internal that some weak platforms might want this kind of 
"busy polling" to yield performance. Maybe, we can design another alternative
ways if sometime people encounter problem. Or you have another thought now?

Ping-Ke