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@xxxxxxxxxxx mailto:pkshih@xxxxxxxxxxx?to=%22Ping-Ke%20Shih%22%20%3Cpkshih%40realtek.com%3E > wrote:


> 
> Luka Gejak <luka.gejak@xxxxxxxxx> 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