RE: [PATCH rtw-next v4 2/2] wifi: rtw88: support channel switch in AP mode

[Ping-Ke Shih <pkshih@realtek.com>]

Mehmet Fide <mehmet.fide@gmail.com> wrote:

[...]

> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -1802,14 +1802,76 @@ int rtw_fw_download_rsvd_page(struct rtw_dev *rtwdev)
>         return ret;
>  }
> 
> +bool rtw_fw_csa_active(struct rtw_dev *rtwdev)
> +{

add lockdep_assert_wiphy(wiphy) to rtw_fw_csa_{active,start,stop}?
Since they access rtwdev->csa_vif.

> +       return rtwdev->csa_vif && rtwdev->csa_vif->bss_conf.csa_active;
> +}
> +
> +void rtw_fw_csa_start(struct rtw_dev *rtwdev, struct ieee80211_vif *vif)
> +{
> +       u32 interval = ieee80211_tu_to_usec(vif->bss_conf.beacon_int);
> +
> +       rtwdev->csa_vif = vif;
> +       wiphy_delayed_work_queue(rtwdev->hw->wiphy, &rtwdev->csa_beacon_work,
> +                                usecs_to_jiffies(interval));
> +}
> +
> +void rtw_fw_csa_stop(struct rtw_dev *rtwdev, struct ieee80211_vif *vif)
> +{
> +       if (!vif || rtwdev->csa_vif != vif)
> +               return;
> +
> +       wiphy_delayed_work_cancel(rtwdev->hw->wiphy, &rtwdev->csa_beacon_work);
> +       rtwdev->csa_vif = NULL;
> +}
> +

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index 2a9b09fa76e7..087268909f5f 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -235,6 +235,8 @@ static void rtw_ops_remove_interface(struct ieee80211_hw *hw,
>         rtw_dbg(rtwdev, RTW_DBG_STATE, "stop vif %pM mac_id %d on port %d\n",
>                 vif->addr, rtwvif->mac_id, rtwvif->port);
> 
> +       rtw_fw_csa_stop(rtwdev, vif);
> +
>         mutex_lock(&rtwdev->mutex);
> 
>         rtw_leave_lps_deep(rtwdev);
> @@ -395,8 +397,10 @@ static void rtw_ops_bss_info_changed(struct ieee80211_hw *hw,
>                 if (vif->cfg.assoc) {
>                         rtw_coex_connect_notify(rtwdev, COEX_ASSOCIATE_FINISH);
> 
> -                       rtw_fw_download_rsvd_page(rtwdev);
> -                       rtw_send_rsvd_page_h2c(rtwdev);
> +                       if (!rtw_fw_csa_active(rtwdev)) {

Does it actually happen to AP mode?

If it could happen, check the condition by conf->csa_active (like below)?

> +                               rtw_fw_download_rsvd_page(rtwdev);
> +                               rtw_send_rsvd_page_h2c(rtwdev);
> +                       }
>                         rtw_fw_default_port(rtwdev, rtwvif);
>                         rtw_coex_media_status_notify(rtwdev, vif->cfg.assoc);
>                         if (rtw_bf_support)
> @@ -438,6 +442,8 @@ static void rtw_ops_bss_info_changed(struct ieee80211_hw *hw,
>                 rtw_set_dtim_period(rtwdev, conf->dtim_period);
>                 rtw_fw_download_rsvd_page(rtwdev);
>                 rtw_send_rsvd_page_h2c(rtwdev);
> +               if (conf->csa_active)
> +                       rtw_fw_csa_start(rtwdev, vif);
>         }
> 
>         if (changed & BSS_CHANGED_BEACON_ENABLED) {
> @@ -489,6 +495,8 @@ static void rtw_ops_stop_ap(struct ieee80211_hw *hw,
>  {
>         struct rtw_dev *rtwdev = hw->priv;
> 
> +       rtw_fw_csa_stop(rtwdev, vif);
> +
>         mutex_lock(&rtwdev->mutex);
>         rtw_write32_clr(rtwdev, REG_TCR, BIT_TCR_UPDATE_HGQMD);
>         rtw_write16(rtwdev, REG_ATIMWND, ATIMWND_DEFAULT);
> @@ -556,6 +564,15 @@ static int rtw_ops_set_tim(struct ieee80211_hw *hw, struct ieee80211_sta *sta,
>         return 0;
>  }
> 
> +static void rtw_ops_channel_switch_beacon(struct ieee80211_hw *hw,
> +                                         struct ieee80211_vif *vif,
> +                                         struct cfg80211_chan_def *chandef)
> +{
> +       struct rtw_dev *rtwdev = hw->priv;
> +
> +       rtw_fw_csa_start(rtwdev, vif);
> +}
> +
>  static int rtw_ops_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>                            struct ieee80211_vif *vif, struct ieee80211_sta *sta,
>                            struct ieee80211_key_conf *key)
> @@ -626,7 +643,8 @@ static int rtw_ops_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>         }
> 
>         /* download new cam settings for PG to backup */
> -       if (rtw_get_lps_deep_mode(rtwdev) == LPS_DEEP_MODE_PG)
> +       if (rtw_get_lps_deep_mode(rtwdev) == LPS_DEEP_MODE_PG &&
> +           !rtw_fw_csa_active(rtwdev))

Think of this deeper.... Is it existing race between set_key and !csa->active?

>                 rtw_fw_download_rsvd_page(rtwdev);
> 
>  out:
> @@ -846,6 +864,7 @@ static int rtw_ops_suspend(struct ieee80211_hw *hw,
>         int ret;
> 
>         mutex_lock(&rtwdev->mutex);
> +       rtw_fw_csa_stop(rtwdev, rtwdev->csa_vif);

Since you access rtwdev->csa_vif, should this take wiphy_lock?

>         ret = rtw_wow_suspend(rtwdev, wowlan);
>         if (ret)
>                 rtw_err(rtwdev, "failed to suspend for wow %d\n", ret);
> @@ -893,19 +912,30 @@ static int rtw_ops_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>         struct rtw_dev *rtwdev = hw->priv;
>         int ret;
> 
> -       if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_SCAN_OFFLOAD))
> -               return 1;
> +       mutex_lock(&rtwdev->mutex);

guard(mutex)( &rtwdev->mutex); ?

> 
> -       if (test_bit(RTW_FLAG_SCANNING, rtwdev->flags))
> -               return -EBUSY;
> +       if (rtw_fw_csa_active(rtwdev)) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
> +       if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_SCAN_OFFLOAD)) {
> +               ret = 1;
> +               goto out;
> +       }
> +
> +       if (test_bit(RTW_FLAG_SCANNING, rtwdev->flags)) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
> 
> -       mutex_lock(&rtwdev->mutex);
>         rtw_hw_scan_start(rtwdev, vif, req);
>         ret = rtw_hw_scan_offload(rtwdev, vif, true);
>         if (ret) {
>                 rtw_hw_scan_abort(rtwdev);
>                 rtw_err(rtwdev, "HW scan failed with status: %d\n", ret);
>         }
> +out:
>         mutex_unlock(&rtwdev->mutex);
> 
>         return ret;