[PATCH rtw-next v7 1/4] wifi: rtw88: sdio: Handle allocation and read errors in rtw_sdio_rxfifo_recv

[PATCH rtw-next v7 1/4] wifi: rtw88: sdio: Handle allocation and read errors in rtw_sdio_rxfifo_recv

[Alastair D'Silva]

When dev_alloc_skb() or rtw_sdio_read_port() fails, rtw_sdio_rxfifo_recv()
returns without reading data. Because it returned void, the caller still
counted rx_len against the loop budget while leaving data stranded in the
FIFO.

Return an error code from rtw_sdio_rxfifo_recv() on failure and break out
of the receive loop in the caller immediately.

Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Acked-by: Ping-Ke Shih <pkshih@realtek.com>
Reviewed-by: Luka Gejak <luka.gejak@linux.dev>
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index dc2fd0f8f9ff..e39284b71837 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1349,7 +1349,7 @@ static void rtw_sdio_rx_skb(struct rtw_dev *rtwdev, struct sk_buff *skb,
 	ieee80211_rx_irqsafe(rtwdev->hw, skb);
 }
 
-static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
+static int rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 	const struct rtw_chip_info *chip = rtwdev->chip;
@@ -1366,12 +1366,12 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 
 	skb = dev_alloc_skb(bufsz);
 	if (!skb)
-		return;
+		return -ENOMEM;
 
 	ret = rtw_sdio_read_port(rtwdev, skb->data, bufsz);
 	if (ret) {
 		dev_kfree_skb_any(skb);
-		return;
+		return ret;
 	}
 
 	while (true) {
@@ -1380,7 +1380,7 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 					   &pkt_stat, &rx_status);
 		if (ret) {
 			dev_kfree_skb_any(skb);
-			return;
+			return ret;
 		}
 
 		pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
@@ -1416,11 +1416,14 @@ static void rtw_sdio_rxfifo_recv(struct rtw_dev *rtwdev, u32 rx_len)
 		skb_reserve(skb, curr_pkt_len);
 		rx_len -= curr_pkt_len;
 	}
+
+	return 0;
 }
 
 static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 {
 	u32 rx_len, hisr, total_rx_bytes = 0;
+	int ret;
 
 	do {
 		if (rtw_chip_wcpu_8051(rtwdev))
@@ -1431,7 +1434,9 @@ static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 		if (!rx_len)
 			break;
 
-		rtw_sdio_rxfifo_recv(rtwdev, rx_len);
+		ret = rtw_sdio_rxfifo_recv(rtwdev, rx_len);
+		if (ret)
+			break;
 
 		total_rx_bytes += rx_len;
 
-- 
2.53.0

[PATCH rtw-next v7 2/4] wifi: rtw88: sdio: Track running state and cancel TX worker on stop

[Alastair D'Silva]

When rtw_sdio_stop() disables interrupts, in-flight interrupt handlers or
delayed TX work may still run against a powered-down device.

Track the operational state in rtwsdio->running with an irq_lock mutex
(similar to PCI), check it at the entry of rtw_sdio_handle_interrupt()
and rtw_sdio_tx_handler(), serialize state transitions in rtw_sdio_start()
and rtw_sdio_stop() under sdio_claim_host() and irq_lock, cancel the TX
worker synchronously in rtw_sdio_stop(), and re-arm it in rtw_sdio_start()
so any frames left parked across a stop are drained and stopped queues are
woken.

Fixes: 28eaf7971a20 ("wifi: rtw88: sdio: add TX back-pressure and retry on page starvation")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 35 +++++++++++++++++++++--
 drivers/net/wireless/realtek/rtw88/sdio.h |  3 ++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index e39284b71837..253467250ee6 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -941,6 +941,8 @@ static void rtw_sdio_init(struct rtw_dev *rtwdev)
 {
 	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
+	mutex_init(&rtwsdio->irq_lock);
+
 	if (rtw_is_8723bs(rtwdev))
 		rtwsdio->irq_mask = REG_SDIO_HIMR_RX_REQUEST;
 	else
@@ -1054,6 +1056,7 @@ static int rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
 
 static int rtw_sdio_start(struct rtw_dev *rtwdev)
 {
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 	u32 clear;
 
 	if (rtw_is_8723bs(rtwdev)) {
@@ -1069,20 +1072,38 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
 
 	rtw_sdio_enable_rx_aggregation(rtwdev);
 
+	sdio_claim_host(rtwsdio->sdio_func);
+
 	if (rtw_is_8723bs(rtwdev)) {
 		clear = rtw_read32(rtwdev, REG_SDIO_HISR) & RTW_SDIO_HISR_CLEAR_MASK;
 		if (clear)
 			rtw_write32(rtwdev, REG_SDIO_HISR, clear);
 	}
 
-	rtw_sdio_enable_interrupt(rtwdev);
+	scoped_guard(mutex, &rtwsdio->irq_lock) {
+		rtwsdio->running = true;
+		rtw_sdio_enable_interrupt(rtwdev);
+	}
+
+	sdio_release_host(rtwsdio->sdio_func);
+
+	queue_delayed_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work, 0);
 
 	return 0;
 }
 
 static void rtw_sdio_stop(struct rtw_dev *rtwdev)
 {
-	rtw_sdio_disable_interrupt(rtwdev);
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	sdio_claim_host(rtwsdio->sdio_func);
+	scoped_guard(mutex, &rtwsdio->irq_lock) {
+		rtwsdio->running = false;
+		rtw_sdio_disable_interrupt(rtwdev);
+	}
+	sdio_release_host(rtwsdio->sdio_func);
+
+	cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
 }
 
 static void rtw_sdio_deep_ps_enter(struct rtw_dev *rtwdev)
@@ -1470,6 +1491,11 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 	rtwdev = hw->priv;
 	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
+	scoped_guard(mutex, &rtwsdio->irq_lock) {
+		if (!rtwsdio->running)
+			return;
+	}
+
 	rtwsdio->irq_thread = current;
 
 	hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
@@ -1703,6 +1729,11 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
 	rtwdev = work_data->rtwdev;
 	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
+	scoped_guard(mutex, &rtwsdio->irq_lock) {
+		if (!rtwsdio->running)
+			return;
+	}
+
 	if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_TX_WAKE))
 		rtw_sdio_deep_ps_leave(rtwdev);
 
diff --git a/drivers/net/wireless/realtek/rtw88/sdio.h b/drivers/net/wireless/realtek/rtw88/sdio.h
index 6e7e6009744b..64266b54f595 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.h
+++ b/drivers/net/wireless/realtek/rtw88/sdio.h
@@ -168,6 +168,9 @@ struct rtw_sdio {
 	bool sdio3_bus_mode;
 
 	void *irq_thread;
+	bool running;
+	/* protect running state and enabling interrupts */
+	struct mutex irq_lock;
 
 	struct workqueue_struct *txwq;
 	struct rtw_sdio_work_data *tx_handler_data;
-- 
2.53.0

Re: [PATCH rtw-next v7 2/4] wifi: rtw88: sdio: Track running state and cancel TX worker on stop

[Luka Gejak]

Hi Alastair,

Thanks for the new version. One comment below.

October 31, 2026 at 4:35 AM, "Alastair D'Silva" <alastair@d-silva.org> wrote:

> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e39284b71837..253467250ee6 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c

[ ... ]

> @@ -1069,20 +1072,38 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)

[ ... ]

> +     sdio_release_host(rtwsdio->sdio_func);
> +
> +     queue_delayed_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work, 0);
>
>       return 0;
>  }

I brought up this rearm on v6, but looking at the callers again I think it
may run a bit too early. rtw_sdio_start() is called from rtw_power_on(),
so the worker can start on whatever is parked before rtw_core_start() is
done with the device:

        rtw_core_start()
          rtw_power_on()
            rtw_hci_start()             <- worker queued here
            rtw_fw_send_general_info()
            rtw_coex_init_hw_config()
          rtw_sec_enable_sec_engine()
          set_bit(RTW_FLAG_RUNNING, rtwdev->flags)

The parked frames got their TX descriptor in rtw_sdio_tx_write() before
the stop, with the macid and sec type of that session. So if I read it
right, after an interface down/up or an idle IPS cycle they can go out for
a station that is gone, and maybe even before rtw_sec_enable_sec_engine()
has run. Resume from WoWLAN looks similar, rtw_wow_stop() starts the
HCI while the WoWLAN firmware is still loaded and rtw_wow_swap_fw()
follows right after.

PCI drops what is left in its rings in rtw_pci_stop(), through
rtw_pci_dma_release(). Maybe doing the same here would be safer, purging
rtwsdio->tx_queue[] with ieee80211_purge_tx_queue() and clearing
tx_queue_stopped[] with a wake of those queues, instead of the rearm.
What do you think?

The stop/start cycling in the cover letter was also run on 7.2.10, where
tx_handler_data->work is still a work_struct and the 8723BS HISR mask is
not there yet, so I guess the board ran a backport. Was this version run
on the board as posted?

Best regards,
Luka Gejak

Re: [PATCH rtw-next v7 2/4] wifi: rtw88: sdio: Track running state and cancel TX worker on stop

[Alastair D'Silva]

On Sat, 2026-10-31 at 07:10 +0000, Luka Gejak wrote:
> Hi Alastair,
> 
> Thanks for the new version. One comment below.
> 
> October 31, 2026 at 4:35 AM, "Alastair D'Silva" <alastair@d-silva.org> wrote:
> 
> > diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c
> > b/drivers/net/wireless/realtek/rtw88/sdio.c
> > index e39284b71837..253467250ee6 100644
> > --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> > +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> 
> [ ... ]
> 
> > @@ -1069,20 +1072,38 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
> 
> [ ... ]
> 
> > +     sdio_release_host(rtwsdio->sdio_func);
> > +
> > +     queue_delayed_work(rtwsdio->txwq, &rtwsdio->tx_handler_data->work, 0);
> > 
> >       return 0;
> >  }
> 
> I brought up this rearm on v6, but looking at the callers again I think it
> may run a bit too early. rtw_sdio_start() is called from rtw_power_on(),
> so the worker can start on whatever is parked before rtw_core_start() is
> done with the device:
> 
>         rtw_core_start()
>           rtw_power_on()
>             rtw_hci_start()             <- worker queued here
>             rtw_fw_send_general_info()
>             rtw_coex_init_hw_config()
>           rtw_sec_enable_sec_engine()
>           set_bit(RTW_FLAG_RUNNING, rtwdev->flags)
> 
> The parked frames got their TX descriptor in rtw_sdio_tx_write() before
> the stop, with the macid and sec type of that session. So if I read it
> right, after an interface down/up or an idle IPS cycle they can go out for
> a station that is gone, and maybe even before rtw_sec_enable_sec_engine()
> has run. Resume from WoWLAN looks similar, rtw_wow_stop() starts the
> HCI while the WoWLAN firmware is still loaded and rtw_wow_swap_fw()
> follows right after.
> 
> PCI drops what is left in its rings in rtw_pci_stop(), through
> rtw_pci_dma_release(). Maybe doing the same here would be safer, purging
> rtwsdio->tx_queue[] with ieee80211_purge_tx_queue() and clearing
> tx_queue_stopped[] with a wake of those queues, instead of the rearm.
> What do you think?

Agreed, your trace of the callers makes it clear that re-arming in
rtw_sdio_start() is too early and risks transmitting frames with stale
TX descriptors from the previous session (or while WoWLAN firmware is
still loaded).

Purging rtwsdio->tx_queue[] and waking any stopped AC queues in
rtw_sdio_stop() after cancel_delayed_work_sync() is much cleaner and
matches rtw_pci_stop(). Since rtwsdio->tx_queue_stopped[] is indexed by
enum rtw_tx_queue_type while ieee80211_wake_queue() takes the mac80211
AC index, I propose doing the following in rtw_sdio_stop() (and
dropping the re-arm from rtw_sdio_start()):

	cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
	for (i = 0; i < RTK_MAX_TX_QUEUE_NUM; i++)
		ieee80211_purge_tx_queue(rtwdev->hw, &rtwsdio->tx_queue[i]);
	for (i = 0; i < IEEE80211_NUM_ACS; i++) {
		queue = rtw_tx_ac_to_hwq(i);
		if (rtwsdio->tx_queue_stopped[queue]) {
			rtwsdio->tx_queue_stopped[queue] = false;
			ieee80211_wake_queue(rtwdev->hw, i);
		}
	}

> 
> The stop/start cycling in the cover letter was also run on 7.2.10, where
> tx_handler_data->work is still a work_struct and the 8723BS HISR mask is
> not there yet, so I guess the board ran a backport. Was this version run
> on the board as posted?
> 

The test board's base kernel is an Armbian sunxi64 build (versioned
7.2.10-edge-sunxi64 by Armbian). For the test run, rtw88_sdio.ko was
built directly from the rtw-next sdio.c/sdio.h (including delayed_work
and RTW_SDIO_HISR_CLEAR_MASK) with our patches applied, and only
rtw_is_8723bs() stubbed to false so rtw88_sdio.ko matched the struct
rtw_dev layout of the board's existing rtw88_core.ko. For v8 I will
build and load the full rtw-next drivers/net/wireless/realtek/rtw88/
module set on the board so it runs completely unmodified as posted.

-- 
Alastair D'Silva
0493 18 5566

[PATCH rtw-next v7 3/4] wifi: rtw88: sdio: Fix unhandled RX request interrupt storm

[Alastair D'Silva]

On 3081-based SDIO chips (observed on RTL8821CS; RTL8822BS and RTL8822CS
are inferred to be affected as they share the 3081 architecture), clearing
REG_SDIO_HISR_RX_REQUEST requires both a software W1C acknowledge and an
empty RX FIFO. Masking this bit out before writing back to HISR prevents
the software acknowledge from happening, leaving the bit asserted even
after the FIFO is drained and causing an infinite interrupt storm loop
that locks up the system.

8051-based chips instead clear this bit in hardware once the RX buffer is
empty without a W1C writeback, and software must not clear it.

Split rtw_sdio_handle_interrupt() into separate 8051 and 3081 handlers.
For 3081, disable interrupts via HIMR, acknowledge all pending status
bits via W1C writeback (preserving the existing full status word
writeback used on non-RTL8723BS chips, now including
REG_SDIO_HISR_RX_REQUEST), service pending events, and re-enable
interrupts under irq_lock if still running.

If unread packets remain in the FIFO when interrupts are re-enabled (or
if new packets arrived), REG_SDIO_HISR_RX_REQUEST remains asserted and
re-triggers the IRQ line. For 8051 chips, preserve the existing behavior.

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>
---
 drivers/net/wireless/realtek/rtw88/sdio.c | 73 ++++++++++++++++++-----
 1 file changed, 57 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index 253467250ee6..75a5f5a96169 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1481,6 +1481,58 @@ static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
 	} while (total_rx_bytes < SZ_64K && hisr & REG_SDIO_HISR_RX_REQUEST);
 }
 
+static void rtw_sdio_handle_interrupt_8051(struct rtw_dev *rtwdev, u32 hisr)
+{
+	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);
+	}
+
+	/*
+	 * RTL8723BS keeps raising the interrupt after resume if undefined
+	 * status bits are written back, so acknowledge only the bits this
+	 * driver defines. Other chips keep the existing behaviour.
+	 */
+	if (rtw_is_8723bs(rtwdev))
+		hisr &= RTW_SDIO_HISR_CLEAR_MASK;
+
+	rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
+}
+
+static void rtw_sdio_handle_interrupt_3081(struct rtw_dev *rtwdev, u32 hisr)
+{
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	rtw_sdio_disable_interrupt(rtwdev);
+
+	/*
+	 * Acknowledge all pending status bits via W1C writeback. Unlike 8051,
+	 * 3081 requires REG_SDIO_HISR_RX_REQUEST to be explicitly cleared by
+	 * software.
+	 */
+	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);
+
+	/*
+	 * Unmasking HIMR re-asserts the IRQ line if new packets arrived or if
+	 * unread data remains in the RX FIFO (e.g. if the 64KB budget was
+	 * reached or an RX error occurred). On 3081 hardware, clearing
+	 * REG_SDIO_HISR_RX_REQUEST requires both the W1C writeback above and
+	 * an empty RX FIFO, so the bit remains asserted as long as unread
+	 * data is in the buffer and re-triggers the interrupt when unmasked.
+	 */
+	scoped_guard(mutex, &rtwsdio->irq_lock) {
+		if (rtwsdio->running)
+			rtw_sdio_enable_interrupt(rtwdev);
+	}
+}
+
 static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 {
 	struct ieee80211_hw *hw = sdio_get_drvdata(sdio_func);
@@ -1499,24 +1551,13 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 	rtwsdio->irq_thread = current;
 
 	hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
-
-	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);
+	if (hisr) {
+		if (rtw_chip_wcpu_8051(rtwdev))
+			rtw_sdio_handle_interrupt_8051(rtwdev, hisr);
+		else
+			rtw_sdio_handle_interrupt_3081(rtwdev, hisr);
 	}
 
-	/*
-	 * RTL8723BS keeps raising the interrupt after resume if undefined
-	 * status bits are written back, so acknowledge only the bits this
-	 * driver defines. Other chips keep the existing behaviour.
-	 */
-	if (rtw_is_8723bs(rtwdev))
-		hisr &= RTW_SDIO_HISR_CLEAR_MASK;
-
-	rtw_write32(rtwdev, REG_SDIO_HISR, hisr);
-
 	rtwsdio->irq_thread = NULL;
 }
 
-- 
2.53.0