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

["Luka Gejak" <luka.gejak@linux.dev>]

Hi Alastair,

Thanks for the new version. Two comments below.

November 5, 2026 at 3:56 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 172aa7ad4732a..e318fefe5b5e7 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c

[ ... ]

>  static void rtw_sdio_stop(struct rtw_dev *rtwdev)
>  {

[ ... ]

> +     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]);

ieee80211_purge_tx_queue() takes the frames off with __skb_dequeue(), so
without the queue lock. rtw_sdio_tx_write() appends with skb_queue_tail()
and does not look at running, so as far as I can see both can run on the
same queue at once.

That seems possible on the firmware recovery path, where the device is
stopped while mac80211 is still sending, since ieee80211_restart_hw()
only comes afterwards:

        rtw_fw_recovery_work()
          __fw_recovery_work()
            rtw_enter_ips()
              rtw_core_stop()
                rtw_power_off()
                  rtw_hci_stop()
                    rtw_sdio_stop()
          ieee80211_restart_hw()

rtw_core_stop() clears RTW_FLAG_RUNNING, but it does not cancel
rtwdev->tx_work, and __rtw_tx_work() does not check the flag. So a
tx_work that is already pushing frames can append while the purge walks
the queue, and the list and qlen get corrupted. On SDIO this path is
reachable since dc16902cad9c ("wifi: rtw88: ps: Trigger firmware
recovery when failing to leave LPS state"). If I read it right, that is
the only rtw_fw_recovery() caller there, because rtw_fw_c2h_cmd_isr() is
only called from pci.c.

A frame appended just after the purge also stays in tx_queue[] and goes
out after the restart with the TX descriptor from before it, like with
the rearm in v7.

Freeing with skb_dequeue() and ieee80211_free_txskb() in a loop would
keep the list intact. Maybe a second pass in rtw_sdio_start(), before
running is set, could drop the late frames too.

> +     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);
> +             }
> +     }
>  }

When the 8723bs back pressure has stopped an AC, mac80211 marks its txqs
with IEEE80211_TXQ_DIRTY the next time a dequeue is tried. On the
recovery path above, this wake runs before ieee80211_restart_hw() sets
in_reconfig. So if I read it right, the wake_txqs tasklet clears the flag
and calls rtw_ops_wake_tx_queue(), which returns early because
RTW_FLAG_RUNNING is already clear. After the restart those txqs only move
again once another frame is queued on them.

PCI does this reset in rtw_pci_start() since 6ac7db1cea43 ("wifi: rtw88:
pci: wake the TX queues after restart_hw and interface up"), through
rtw_pci_reset_tx_queue_state(). On restart_hw that runs while in_reconfig
is set and the SUSPEND reason still holds the queues, so the marked txqs
are woken at the end of the reconfig instead. I think moving the reset
and the wake to rtw_sdio_start() would work here too. It could also take
tx_queue[].lock there, like the 8723bs stop and wake helpers.

Best regards,
Luka Gejak