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

[PATCH rtw-next v4 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
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

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

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
> 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
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

[PATCH rtw-next v4 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 (similar to PCI), check it
at the entry of rtw_sdio_handle_interrupt() and rtw_sdio_tx_handler(), and
cancel the TX worker synchronously in rtw_sdio_stop().

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 | 12 ++++++++++++
 drivers/net/wireless/realtek/rtw88/sdio.h |  1 +
 2 files changed, 13 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index e39284b71837..d2f4d7e8bc83 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1054,6 +1054,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)) {
@@ -1075,6 +1076,7 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
 			rtw_write32(rtwdev, REG_SDIO_HISR, clear);
 	}
 
+	rtwsdio->running = true;
 	rtw_sdio_enable_interrupt(rtwdev);
 
 	return 0;
@@ -1082,7 +1084,11 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
 
 static void rtw_sdio_stop(struct rtw_dev *rtwdev)
 {
+	struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	rtwsdio->running = false;
 	rtw_sdio_disable_interrupt(rtwdev);
+	cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
 }
 
 static void rtw_sdio_deep_ps_enter(struct rtw_dev *rtwdev)
@@ -1470,6 +1476,9 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 	rtwdev = hw->priv;
 	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
+	if (!rtwsdio->running)
+		return;
+
 	rtwsdio->irq_thread = current;
 
 	hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
@@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
 	rtwdev = work_data->rtwdev;
 	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
 
+	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..a3851d4a58e7 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.h
+++ b/drivers/net/wireless/realtek/rtw88/sdio.h
@@ -168,6 +168,7 @@ struct rtw_sdio {
 	bool sdio3_bus_mode;
 
 	void *irq_thread;
+	bool running;
 
 	struct workqueue_struct *txwq;
 	struct rtw_sdio_work_data *tx_handler_data;
-- 
2.53.0

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

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
> 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 (similar to PCI), check it
> at the entry of rtw_sdio_handle_interrupt() and rtw_sdio_tx_handler(), and
> cancel the TX worker synchronously in rtw_sdio_stop().
> 
> 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 | 12 ++++++++++++
>  drivers/net/wireless/realtek/rtw88/sdio.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e39284b71837..d2f4d7e8bc83 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1054,6 +1054,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)) {
> @@ -1075,6 +1076,7 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
>                         rtw_write32(rtwdev, REG_SDIO_HISR, clear);
>         }
> 
> +       rtwsdio->running = true;

Is there existing race between start/stop/interrupt? Need a lock?

>         rtw_sdio_enable_interrupt(rtwdev);
> 
>         return 0;
> @@ -1082,7 +1084,11 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
> 
>  static void rtw_sdio_stop(struct rtw_dev *rtwdev)
>  {
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> +
> +       rtwsdio->running = false;
>         rtw_sdio_disable_interrupt(rtwdev);
> +       cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);

I think this is the major statement added by this patch, but I'm not sure
if this is actually needed. (Maybe, Luka can help this)

By 'git blame', the work is added by
28eaf7971a20 ("wifi: rtw88: sdio: add TX back-pressure and retry on page starvation")

Maybe, the fixes tag should be 28eaf7971a20. I think there is no way to apply this
to 65371a3f14e7 (your fixes tag)

But I think you need this because patch 3/4. 

>  }
> 
>  static void rtw_sdio_deep_ps_enter(struct rtw_dev *rtwdev)
> @@ -1470,6 +1476,9 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
>         rtwdev = hw->priv;
>         rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> 
> +       if (!rtwsdio->running)
> +               return;
> +
>         rtwsdio->irq_thread = current;
> 
>         hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
> @@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
>         rtwdev = work_data->rtwdev;
>         rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> 
> +       if (!rtwsdio->running)
> +               return;
> +

I don't think we need this, since you added cancel_delayed_work_sync() in
rtw_sdio_stop().

>         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..a3851d4a58e7 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.h
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.h
> @@ -168,6 +168,7 @@ struct rtw_sdio {
>         bool sdio3_bus_mode;
> 
>         void *irq_thread;
> +       bool running;
> 
>         struct workqueue_struct *txwq;
>         struct rtw_sdio_work_data *tx_handler_data;
> --
> 2.53.0

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

[Luka Gejak]

October 6, 2026 at 04:03, "Ping-Ke Shih" <pkshih@realtek.com mailto:pkshih@realtek.com?to=%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wrote:


> 
> Alastair D'Silva <alastair@d-silva.org> wrote:
> 
> > 
> > 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 (similar to PCI), check it
> >  at the entry of rtw_sdio_handle_interrupt() and rtw_sdio_tx_handler(), and
> >  cancel the TX worker synchronously in rtw_sdio_stop().
> >  
> >  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 | 12 ++++++++++++
> >  drivers/net/wireless/realtek/rtw88/sdio.h | 1 +
> >  2 files changed, 13 insertions(+)
> >  
> >  diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> >  index e39284b71837..d2f4d7e8bc83 100644
> >  --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> >  +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> >  @@ -1054,6 +1054,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)) {
> >  @@ -1075,6 +1076,7 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
> >  rtw_write32(rtwdev, REG_SDIO_HISR, clear);
> >  }
> >  
> >  + rtwsdio->running = true;
> > 
> Is there existing race between start/stop/interrupt? Need a lock?
> 
> > 
> > rtw_sdio_enable_interrupt(rtwdev);
> >  
> >  return 0;
> >  @@ -1082,7 +1084,11 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
> >  
> >  static void rtw_sdio_stop(struct rtw_dev *rtwdev)
> >  {
> >  + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  +
> >  + rtwsdio->running = false;
> >  rtw_sdio_disable_interrupt(rtwdev);
> >  + cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
> > 
> I think this is the major statement added by this patch, but I'm not sure
> if this is actually needed. (Maybe, Luka can help this)
> 

I will take a look at this series later during the day.

> By 'git blame', the work is added by
> 28eaf7971a20 ("wifi: rtw88: sdio: add TX back-pressure and retry on page starvation")
> 
> Maybe, the fixes tag should be 28eaf7971a20. I think there is no way to apply this
> to 65371a3f14e7 (your fixes tag)
> 
>

Best regards,
Luka Gejak

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

[Luka Gejak]

October 6, 2026 at 04:03, "Ping-Ke Shih" <pkshih@realtek.com mailto:pkshih@realtek.com?to=%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wrote:

> 
> Alastair D'Silva <alastair@d-silva.org> wrote:
> 
> > 
[...]
> >  @@ -1082,7 +1084,11 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
> >  
> >  static void rtw_sdio_stop(struct rtw_dev *rtwdev)
> >  {
> >  + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  +
> >  + rtwsdio->running = false;
> >  rtw_sdio_disable_interrupt(rtwdev);
> >  + cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
> > 
> I think this is the major statement added by this patch, but I'm not sure
> if this is actually needed. (Maybe, Luka can help this)
> 
> By 'git blame', the work is added by
> 28eaf7971a20 ("wifi: rtw88: sdio: add TX back-pressure and retry on page starvation")
> 
> Maybe, the fixes tag should be 28eaf7971a20. I think there is no way to apply this
> to 65371a3f14e7 (your fixes tag)
> 
> But I think you need this because patch 3/4. 
> 

The cancel is needed. Nothing else cancels this work before the
device goes down, the only other cancel is at remove:

	cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
	destroy_workqueue(rtwsdio->txwq);

The stop hook runs from the chip power off op, so the MAC is powered
off right after the HCI stops, but items can be pending at that point,
because the 8723bs retry re-arms the work with a delay:

	if (!rtw_is_8723bs(rtwdev))
		return false;

	queue_delayed_work(rtwsdio->txwq, &work_data->work, delay);

The delayed re-arm came with my back-pressure work, 28eaf7971a20.
So in my opinion cancel should stay with changed Fixes tag.

> > 
> > }
> >  
> >  static void rtw_sdio_deep_ps_enter(struct rtw_dev *rtwdev)
> >  @@ -1470,6 +1476,9 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
> >  rtwdev = hw->priv;
> >  rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  
> >  + if (!rtwsdio->running)
> >  + return;
> >  +

I think we should keep this check too, per my explanation below.

> >  rtwsdio->irq_thread = current;
> >  
> >  hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
> >  @@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
> >  rtwdev = work_data->rtwdev;
> >  rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  
> >  + if (!rtwsdio->running)
> >  + return;
> >  +
> > 
> I don't think we need this, since you added cancel_delayed_work_sync() in
> rtw_sdio_stop().

I think we should keep the check. The tx work arms the SDIO work at the end,
and stop does not cancel it:

	rtw_hci_tx_kick_off(rtwdev);

	cancel_work_sync(&rtwdev->c2h_work);
	cancel_work_sync(&rtwdev->update_beacon_work);
	cancel_delayed_work_sync(&rtwdev->watch_dog_work);
	cancel_delayed_work_sync(&coex->bt_relink_work);
	cancel_delayed_work_sync(&coex->bt_reenable_work);
	cancel_delayed_work_sync(&coex->defreeze_work);
	cancel_delayed_work_sync(&coex->wl_remain_work);
	cancel_delayed_work_sync(&coex->bt_remain_work);
	cancel_delayed_work_sync(&coex->wl_connecting_work);
	cancel_delayed_work_sync(&coex->bt_multi_link_remain_work);
	cancel_delayed_work_sync(&coex->wl_ccklock_work);

so a kick can land after the cancel has run, and only this flag
keeps the item off the powered-down device.

Also I believe this series should be tested on hardware to ensure
nothing breaks.

Best regards,
Luka Gejak

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

[Luka Gejak]

October 6, 2026 at 16:03, "Luka Gejak" <luka.gejak@linux.dev mailto:luka.gejak@linux.dev?to=%22Luka%20Gejak%22%20%3Cluka.gejak%40linux.dev%3E > wrote:


> 
> October 6, 2026 at 04:03, "Ping-Ke Shih" <pkshih@realtek.com mailto:pkshih@realtek.com?to=%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wrote:
> 
> > 
> > Alastair D'Silva <alastair@d-silva.org> wrote:
> > 
[...]
> >  
> >  static void rtw_sdio_deep_ps_enter(struct rtw_dev *rtwdev)
> >  @@ -1470,6 +1476,9 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
> >  rtwdev = hw->priv;
> >  rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  
> >  + if (!rtwsdio->running)
> >  + return;
> >  +
> > 
> I think we should keep this check too, per my explanation below.
> 

Correction, as I realized I didn't include explanation for this below.

Sorry for that, here is the explanation:

This check sits on the interrupt path, which the cancel does not
cover. rtw_sdio_stop() only masks HIMR. The SDIO IRQ is released in
rtw_sdio_free_irq():

sdio_release_irq(sdio_func);

and that only runs at remove. A request raised just before the mask
can still reach the handler once after a stop, and without the check
that run reads HISR and writes the ack back on a stopped device.

Best regards,
Luka Gejak

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

[Ping-Ke Shih]

Luka Gejak <luka.gejak@linux.dev> wrote:
> > >  @@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
> > >  rtwdev = work_data->rtwdev;
> > >  rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > >
> > >  + if (!rtwsdio->running)
> > >  + return;
> > >  +
> > >
> > I don't think we need this, since you added cancel_delayed_work_sync() in
> > rtw_sdio_stop().
> 
> I think we should keep the check. The tx work arms the SDIO work at the end,
> and stop does not cancel it:
> 
>         rtw_hci_tx_kick_off(rtwdev);
> 
>         cancel_work_sync(&rtwdev->c2h_work);
>         cancel_work_sync(&rtwdev->update_beacon_work);
>         cancel_delayed_work_sync(&rtwdev->watch_dog_work);
>         cancel_delayed_work_sync(&coex->bt_relink_work);
>         cancel_delayed_work_sync(&coex->bt_reenable_work);
>         cancel_delayed_work_sync(&coex->defreeze_work);
>         cancel_delayed_work_sync(&coex->wl_remain_work);
>         cancel_delayed_work_sync(&coex->bt_remain_work);
>         cancel_delayed_work_sync(&coex->wl_connecting_work);
>         cancel_delayed_work_sync(&coex->bt_multi_link_remain_work);
>         cancel_delayed_work_sync(&coex->wl_ccklock_work);

In rtw_sdio_stop(), it does cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
Is it not enough?

> 
> so a kick can land after the cancel has run, and only this flag
> keeps the item off the powered-down device.
> 
> Also I believe this series should be tested on hardware to ensure
> nothing breaks.

When calling rtw_sdio_stop(), I think mac80211 doesn't TX anymore
to driver, and then rtw_sdio_stop() does
 1. rtwsdio->running = false;
    // prevent to schedule rtwsdio->tx_handler_data->work again
 2. rtw_sdio_disable_interrupt(rtwdev);
    // prevent to run rtw_sdio_handle_interrupt again
 3. cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
    // flush/wait the pending rtw_sdio_tx_handler

Anything I miss? Could you point out a case this is needed?

Ping-Ke

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

[Luka Gejak]

October 7, 2026 at 03:26, "Ping-Ke Shih" <pkshih@realtek.com mailto:pkshih@realtek.com?to=%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wrote:


> 
> Luka Gejak <luka.gejak@linux.dev> wrote:
> 
> > 
> > > @@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work_struct *work)
> >  > rtwdev = work_data->rtwdev;
> >  > rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> >  >
> >  > + if (!rtwsdio->running)
> >  > + return;
> >  > +
> >  >
> >  I don't think we need this, since you added cancel_delayed_work_sync() in
> >  rtw_sdio_stop().
> >  
> >  I think we should keep the check. The tx work arms the SDIO work at the end,
> >  and stop does not cancel it:
> >  
> >  rtw_hci_tx_kick_off(rtwdev);
> >  
> >  cancel_work_sync(&rtwdev->c2h_work);
> >  cancel_work_sync(&rtwdev->update_beacon_work);
> >  cancel_delayed_work_sync(&rtwdev->watch_dog_work);
> >  cancel_delayed_work_sync(&coex->bt_relink_work);
> >  cancel_delayed_work_sync(&coex->bt_reenable_work);
> >  cancel_delayed_work_sync(&coex->defreeze_work);
> >  cancel_delayed_work_sync(&coex->wl_remain_work);
> >  cancel_delayed_work_sync(&coex->bt_remain_work);
> >  cancel_delayed_work_sync(&coex->wl_connecting_work);
> >  cancel_delayed_work_sync(&coex->bt_multi_link_remain_work);
> >  cancel_delayed_work_sync(&coex->wl_ccklock_work);
> > 
> In rtw_sdio_stop(), it does cancel_delayed_work_sync(&rtwsdio->tx_handler_data->work);
> Is it not enough?
> 

That is enough for a work item that is already armed. It does not cover an
arm that lands after the call, and that can happen:

[...]
>  1. rtwsdio->running = false;
>  // prevent to schedule rtwsdio->tx_handler_data->work again

Nothing reads rtwsdio->running on the arming path. wake_tx_queue() tests
the flag at the top and queues the work at the bottom:

	if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
		return;

	if (txq->ac == IEEE80211_AC_VO)
		__rtw_tx_work(rtwdev);
	else
		queue_work(rtwdev->tx_wq, &rtwdev->tx_work);

then __rtw_tx_work() kicks the SDIO work at the end:

	rtw_hci_tx_kick_off(rtwdev);

The sequence:

1. wake_tx_queue() reads the flag while it is still set and queues
   rtwdev->tx_work.
2. rtw_core_stop() clears the flag and cancels its list of works.
3. rtw_sdio_stop() sets running to false and disables the interrupts. The
   cancel_delayed_work_sync() finds nothing armed and returns at once.
4. rtw_tx_work() runs now, nothing cancelled it, and it ends in
   rtw_sdio_tx_kick_off(), which arms the delayed work again with
   mod_delayed_work().
5. rtw_sdio_tx_handler() runs, and without the check it calls
   rtw_sdio_deep_ps_leave() and then walks the queues on a MAC that is
   being powered off.

So the cancel only orders against a kick that came before it, and the
check covers the other order.

Best regards,
Luka Gejak

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

[Luka Gejak]

October 7, 2026 at 03:26, "Ping-Ke Shih" <pkshih@realtek.com mailto:pkshi=
h@realtek.com?to=3D%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wr=
ote:


> 
> Luka Gejak <luka.gejak@linux.dev> wrote:
> 
> > 
> > > @@ -1703,6 +1712,9 @@ static void rtw_sdio_tx_handler(struct work=
_struct *work)
> >  > rtwdev =3D work_data->rtwdev;
> >  > rtwsdio =3D (struct rtw_sdio *)rtwdev->priv;
> >  >
> >  > + if (!rtwsdio->running)
> >  > + return;
> >  > +
> >  >
> >  I don't think we need this, since you added cancel_delayed_work_sync=
() in
> >  rtw_sdio_stop().
> > 
> >  I think we should keep the check. The tx work arms the SDIO work a=
t the end,
> >  and stop does not cancel it:
> > 
> >  rtw_hci_tx_kick_off(rtwdev);
> > 
> >  cancel_work_sync(&rtwdev->c2h_work);
> >  cancel_work_sync(&rtwdev->update_beacon_work);
> >  cancel_delayed_work_sync(&rtwdev->watch_dog_work);
> >  cancel_delayed_work_sync(&coex->bt_relink_work);
> >  cancel_delayed_work_sync(&coex->bt_reenable_work);
> >  cancel_delayed_work_sync(&coex->defreeze_work);
> >  cancel_delayed_work_sync(&coex->wl_remain_work);
> >  cancel_delayed_work_sync(&coex->bt_remain_work);
> >  cancel_delayed_work_sync(&coex->wl_connecting_work);
> >  cancel_delayed_work_sync(&coex->bt_multi_link_remain_work);
> >  cancel_delayed_work_sync(&coex->wl_ccklock_work);
> > 
> In rtw_sdio_stop(), it does cancel_delayed_work_sync(&rtwsdio->tx_han=
dler_data->work);
> Is it not enough?
> 

That is enough for a work item that is already armed. It does not cover=
 an
arm that lands after the call, and that can happen:

[...]
>  1. rtwsdio->running =3D false;
>  // prevent to schedule rtwsdio->tx_handler_data->work again

Nothing reads rtwsdio->running on the arming path. wake_tx_queue() tests
the flag at the top and queues the work at the bottom:

	if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
		return;

	if (txq->ac =3D=3D IEEE80211_AC_VO)
		__rtw_tx_work(rtwdev);
	else
		queue_work(rtwdev->tx_wq, &rtwdev->tx_work);

then __rtw_tx_work() kicks the SDIO work at the end:

	rtw_hci_tx_kick_off(rtwdev);

The sequence:

1. wake_tx_queue() reads the flag while it is still set and queues
   rtwdev->tx_work.
2. rtw_core_stop() clears the flag and cancels its list of works.
3. rtw_sdio_stop() sets running to false and disables the interrupts. The
   cancel_delayed_work_sync() finds nothing armed and returns at once.
4. rtw_tx_work() runs now, nothing cancelled it, and it ends in
   rtw_sdio_tx_kick_off(), which arms the delayed work again with
   mod_delayed_work().
5. rtw_sdio_tx_handler() runs, and without the check it calls
   rtw_sdio_deep_ps_leave() and then walks the queues on a MAC that is
   being powered off.

So the cancel only orders against a kick that came before it, and the
check covers the other order.

Best regards,
Luka Gejak

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

[Luka Gejak]

October 7, 2026 at 07:21, "Luka Gejak" <luka.gejak@linux.dev mailto:luka.gejak@linux.dev?to=%22Luka%20Gejak%22%20%3Cluka.gejak%40linux.dev%3E > wrote:


[...]

Sorry for duplicated email, migadu was down and I thought email didn't send so I resent it.

Best regards,
Luka Gejak

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

[Alastair D'Silva]

On 3081-based SDIO chips (e.g. RTL8821CS), hardware does not automatically
clear REG_SDIO_HISR_RX_REQUEST when the RX buffer is empty. Masking this
bit out before writing back to HISR prevents it from being acknowledged,
causing an infinite interrupt storm loop that locks up the system.

8051-based chips instead rely on hardware to clear this bit automatically
once the buffer is empty, and software must not clear it.

Split rtw_sdio_handle_interrupt() into separate 8051 and 3081 handlers.
For 3081, disable interrupts via HIMR, acknowledge pending status bits
via W1C writeback, service pending events, and re-enable interrupts.
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 | 60 +++++++++++++++++------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
index d2f4d7e8bc83..c74644bf14ba 100644
--- a/drivers/net/wireless/realtek/rtw88/sdio.c
+++ b/drivers/net/wireless/realtek/rtw88/sdio.c
@@ -1466,23 +1466,8 @@ 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(struct sdio_func *sdio_func)
+static void rtw_sdio_handle_interrupt_8051(struct rtw_dev *rtwdev, u32 hisr)
 {
-	struct ieee80211_hw *hw = sdio_get_drvdata(sdio_func);
-	struct rtw_sdio *rtwsdio;
-	struct rtw_dev *rtwdev;
-	u32 hisr;
-
-	rtwdev = hw->priv;
-	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
-
-	if (!rtwsdio->running)
-		return;
-
-	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) {
@@ -1499,7 +1484,50 @@ static void rtw_sdio_handle_interrupt(struct sdio_func *sdio_func)
 		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);
+	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 */
+	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);
+	struct rtw_sdio *rtwsdio;
+	struct rtw_dev *rtwdev;
+	u32 hisr;
+
+	rtwdev = hw->priv;
+	rtwsdio = (struct rtw_sdio *)rtwdev->priv;
+
+	if (!rtwsdio->running)
+		return;
+
+	rtwsdio->irq_thread = current;
+
+	hisr = rtw_read32(rtwdev, REG_SDIO_HISR);
+	if (!hisr)
+		goto out;
+
+	if (rtw_chip_wcpu_8051(rtwdev))
+		rtw_sdio_handle_interrupt_8051(rtwdev, hisr);
+	else
+		rtw_sdio_handle_interrupt_3081(rtwdev, hisr);
 
+out:
 	rtwsdio->irq_thread = NULL;
 }
 
-- 
2.53.0

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

[Ping-Ke Shih]

Alastair D'Silva <alastair@d-silva.org> wrote:
> +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);
> +       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 */
> +       if (rtwsdio->running)

I think here needs a lock

> +               rtw_sdio_enable_interrupt(rtwdev);
> +}
> +