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@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?

> 
> 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