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

[Ping-Ke Shih]

Alastair D'Silva <alastair@xxxxxxxxxxx> wrote:
 
> On Mon, 2026-09-21 at 02:44 +0000, Ping-Ke Shih wrote:
> > Alastair D'Silva <alastair@xxxxxxxxxxx> 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