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

[Alastair D'Silva <alastair@d-silva.org>]

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