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

[Ping-Ke Shih]

Alastair D'Silva <alastair@xxxxxxxxxxx> 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