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

> 
> Alastair D'Silva <alastair@xxxxxxxxxxx> 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