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

[Ping-Ke Shih]

Alastair D'Silva <alastair@xxxxxxxxxxx> wrote:
> When rtw_sdio_stop() disables interrupts, in-flight interrupt handlers or
> delayed TX work may still run against a powered-down device.
> 
> Track the operational state in rtwsdio->running (similar to PCI), check it
> at the entry of rtw_sdio_handle_interrupt() and rtw_sdio_tx_handler(), and
> cancel the TX worker synchronously in rtw_sdio_stop().
> 
> Fixes: 65371a3f14e7 ("wifi: rtw88: sdio: Add HCI implementation for SDIO based chipsets")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Alastair D'Silva <alastair@xxxxxxxxxxx>
> ---
>  drivers/net/wireless/realtek/rtw88/sdio.c | 12 ++++++++++++
>  drivers/net/wireless/realtek/rtw88/sdio.h |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.c b/drivers/net/wireless/realtek/rtw88/sdio.c
> index e39284b71837..d2f4d7e8bc83 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.c
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.c
> @@ -1054,6 +1054,7 @@ static int rtw_sdio_8723bs_check_rqpn(struct rtw_dev *rtwdev)
> 
>  static int rtw_sdio_start(struct rtw_dev *rtwdev)
>  {
> +       struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
>         u32 clear;
> 
>         if (rtw_is_8723bs(rtwdev)) {
> @@ -1075,6 +1076,7 @@ static int rtw_sdio_start(struct rtw_dev *rtwdev)
>                         rtw_write32(rtwdev, REG_SDIO_HISR, clear);
>         }
> 
> +       rtwsdio->running = true;

Is there existing race between start/stop/interrupt? Need a lock?

>         rtw_sdio_enable_interrupt(rtwdev);
> 
>         return 0;
> @@ -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. 

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

>         if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_TX_WAKE))
>                 rtw_sdio_deep_ps_leave(rtwdev);
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/sdio.h b/drivers/net/wireless/realtek/rtw88/sdio.h
> index 6e7e6009744b..a3851d4a58e7 100644
> --- a/drivers/net/wireless/realtek/rtw88/sdio.h
> +++ b/drivers/net/wireless/realtek/rtw88/sdio.h
> @@ -168,6 +168,7 @@ struct rtw_sdio {
>         bool sdio3_bus_mode;
> 
>         void *irq_thread;
> +       bool running;
> 
>         struct workqueue_struct *txwq;
>         struct rtw_sdio_work_data *tx_handler_data;
> --
> 2.53.0