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

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@xxxxxxxxx> wrote:
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> index 3cd17a3bb494..5e68ba73548a 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -1802,14 +1802,73 @@ int rtw_fw_download_rsvd_page(struct rtw_dev *rtwdev)
>         return ret;
>  }
> 
> +static struct ieee80211_vif *rtw_fw_beacon_vif(struct rtw_dev *rtwdev)

There is single one caller, so just squash into rtw_fw_csa_active().

I asked to move CS work to vif, because I want to avoid this kind of
getting vif from rsvd_pkt->rtwvif. Is there any way to avoid this?

> +{
> +       struct rtw_rsvd_page *rsvd_pkt;
> +
> +       rsvd_pkt = list_first_entry_or_null(&rtwdev->rsvd_page_list,
> +                                           struct rtw_rsvd_page, build_list);
> +       if (!rsvd_pkt || rsvd_pkt->type != RSVD_BEACON)
> +               return NULL;
> +
> +       return rtwvif_to_vif(rsvd_pkt->rtwvif);
> +}
> +
> +bool rtw_fw_csa_active(struct rtw_dev *rtwdev)
> +{
> +       struct ieee80211_vif *vif = rtw_fw_beacon_vif(rtwdev);
> +
> +       return vif && vif->bss_conf.csa_active;
> +}
> +
>  void rtw_fw_update_beacon_work(struct work_struct *work)
>  {
>         struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
>                                               update_beacon_work);
> 
>         mutex_lock(&rtwdev->mutex);
> +
> +       if (rtw_fw_csa_active(rtwdev))
> +               goto out;
> +
>         rtw_fw_download_rsvd_page(rtwdev);
>         rtw_send_rsvd_page_h2c(rtwdev);
> +
> +out:
> +       mutex_unlock(&rtwdev->mutex);
> +}
> +

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/fw.h b/drivers/net/wireless/realtek/rtw88/fw.h
> index 48ad9ceab6ea..a0a1dcbf93de 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.h
> +++ b/drivers/net/wireless/realtek/rtw88/fw.h
> @@ -863,7 +863,9 @@ void rtw_add_rsvd_page_pno(struct rtw_dev *rtwdev,
>  void rtw_add_rsvd_page_sta(struct rtw_dev *rtwdev,
>                            struct rtw_vif *rtwvif);
>  int rtw_fw_download_rsvd_page(struct rtw_dev *rtwdev);
> +bool rtw_fw_csa_active(struct rtw_dev *rtwdev);
>  void rtw_fw_update_beacon_work(struct work_struct *work);
> +void rtw_fw_csa_beacon_work(struct wiphy *wiphy, struct wiphy_work *work);
>  void rtw_send_rsvd_page_h2c(struct rtw_dev *rtwdev);
>  int rtw_dump_drv_rsvd_page(struct rtw_dev *rtwdev,
>                            u32 offset, u32 size, u32 *buf);
> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index 2a9b09fa76e7..4c5be1bd9875 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -164,6 +164,9 @@ static int rtw_ops_add_interface(struct ieee80211_hw *hw,
>         memset(&rtwvif->bfee, 0, sizeof(struct rtw_bfee));
>         rtw_txq_init(rtwdev, vif->txq);
>         INIT_LIST_HEAD(&rtwvif->rsvd_page_list);
> +       if (!test_bit(RTW_FLAG_RESTARTING, rtwdev->flags))

Will it be a problem just unconditionally initializing csa work?

> +               wiphy_delayed_work_init(&rtwvif->csa_beacon_work,
> +                                       rtw_fw_csa_beacon_work);
> 
>         mutex_lock(&rtwdev->mutex);
> 
> @@ -235,6 +238,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);
> 
> +       wiphy_delayed_work_cancel(hw->wiphy, &rtwvif->csa_beacon_work);
> +
>         mutex_lock(&rtwdev->mutex);
> 
>         rtw_leave_lps_deep(rtwdev);
> @@ -438,6 +443,13 @@ 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) {
> +                       u32 interval = ieee80211_tu_to_usec(conf->beacon_int);
> +
> +                       wiphy_delayed_work_queue(hw->wiphy,
> +                                                &rtwvif->csa_beacon_work,
> +                                                usecs_to_jiffies(interval));
> +               }
>         }
> 
>         if (changed & BSS_CHANGED_BEACON_ENABLED) {
> @@ -487,8 +499,11 @@ static void rtw_ops_stop_ap(struct ieee80211_hw *hw,
>                             struct ieee80211_vif *vif,
>                             struct ieee80211_bss_conf *link_conf)
>  {
> +       struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
>         struct rtw_dev *rtwdev = hw->priv;
> 
> +       wiphy_delayed_work_cancel(hw->wiphy, &rtwvif->csa_beacon_work);
> +
>         mutex_lock(&rtwdev->mutex);
>         rtw_write32_clr(rtwdev, REG_TCR, BIT_TCR_UPDATE_HGQMD);
>         rtw_write16(rtwdev, REG_ATIMWND, ATIMWND_DEFAULT);
> @@ -556,6 +571,17 @@ 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)
> +{
> +       u32 interval = ieee80211_tu_to_usec(vif->bss_conf.beacon_int);
> +       struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
> +
> +       wiphy_delayed_work_queue(hw->wiphy, &rtwvif->csa_beacon_work,
> +                                usecs_to_jiffies(interval));
> +}
> +
>  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 +652,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))

As the comment, this is to download new CAM settings. If CSA is ongoning,
you ignore the download. Then, my question is when will you download
this properly?

>                 rtw_fw_download_rsvd_page(rtwdev);
> 
>  out:
> @@ -839,6 +866,14 @@ static int rtw_ops_get_antenna(struct ieee80211_hw *hw,
>  }
> 
>  #ifdef CONFIG_PM
> +static void rtw_csa_cancel_iter(void *data, struct ieee80211_vif *vif)
> +{
> +       struct rtw_vif *rtwvif = (struct rtw_vif *)vif->drv_priv;
> +       struct rtw_dev *rtwdev = data;
> +
> +       wiphy_delayed_work_cancel(rtwdev->hw->wiphy, &rtwvif->csa_beacon_work);
> +}
> +
>  static int rtw_ops_suspend(struct ieee80211_hw *hw,
>                            struct cfg80211_wowlan *wowlan)
>  {
> @@ -846,6 +881,7 @@ static int rtw_ops_suspend(struct ieee80211_hw *hw,
>         int ret;
> 
>         mutex_lock(&rtwdev->mutex);
> +       rtw_iterate_vifs(rtwdev, rtw_csa_cancel_iter, rtwdev);

I'm thinking if we move back csa work to rtwdev (like v2) to avoid this kind of
iterative. 

>         ret = rtw_wow_suspend(rtwdev, wowlan);
>         if (ret)
>                 rtw_err(rtwdev, "failed to suspend for wow %d\n", ret);
> @@ -887,10 +923,19 @@ static void rtw_reconfig_complete(struct ieee80211_hw *hw,
>         mutex_unlock(&rtwdev->mutex);
>  }
> 
> +static void rtw_csa_active_iter(void *data, struct ieee80211_vif *vif)
> +{
> +       bool *csa_active = data;
> +
> +       if (vif->bss_conf.csa_active)
> +               *csa_active = true;
> +}
> +
>  static int rtw_ops_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                            struct ieee80211_scan_request *req)
>  {
>         struct rtw_dev *rtwdev = hw->priv;
> +       bool csa_active = false;
>         int ret;
> 
>         if (!rtw_fw_feature_check(&rtwdev->fw, FW_FEATURE_SCAN_OFFLOAD))
> @@ -900,6 +945,13 @@ static int rtw_ops_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                 return -EBUSY;
> 
>         mutex_lock(&rtwdev->mutex);
> +
> +       rtw_iterate_vifs(rtwdev, rtw_csa_active_iter, &csa_active);

and avoid this iterative. 

> +       if (csa_active) {
> +               mutex_unlock(&rtwdev->mutex);
> +               return -EBUSY;
> +       }
> +
>         rtw_hw_scan_start(rtwdev, vif, req);
>         ret = rtw_hw_scan_offload(rtwdev, vif, true);
>         if (ret) {