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

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

Mehmet Fide <mehmet.fide@gmail.com> wrote:
> From: Mehmet Fide <mehmet.fide@screeningeagle.com>
> 
> hostapd's CHAN_SWITCH is refused because the driver does not announce
> channel switch support, so an AP on rtw88 can only change its channel
> by being torn down and started again.
> 
> Declare WIPHY_FLAG_HAS_CHANNEL_SWITCH and implement
> ieee80211_ops::channel_switch_beacon: the firmware repeats the beacon
> held in the first reserved page, so while a switch is announced the
> page is downloaded again every beacon interval to renew the countdown,
> and ieee80211_csa_finish() is called once it completes. IBSS, which
> the flag enables too, shares the page and the work.
> 
> The work is a wiphy delayed work of the device, like
> update_beacon_work: 

rtw88 doesn't switch to wiphy work yet. Mixing to use ieee80211 and wiphy
works with driver mutext still is harmless I think.

> rtw88 runs one beaconing interface, a hw restart
> replays add_interface without remove_interface, and the wiphy lock
> serializes it with the mac80211 state it reads. It is cancelled when
> the AP stops, when the vif goes away and on WoWLAN suspend, and a
> hardware scan is refused while a switch is announced, since it would
> take the AP off the channel its stations count down to.
> 
> Signed-off-by: Mehmet Fide <mehmet.fide@screeningeagle.com>
> ---
>  drivers/net/wireless/realtek/rtw88/fw.c       | 41 +++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/fw.h       |  1 +
>  drivers/net/wireless/realtek/rtw88/mac80211.c | 46 +++++++++++++++++++
>  drivers/net/wireless/realtek/rtw88/main.c     |  4 +-
>  drivers/net/wireless/realtek/rtw88/main.h     |  1 +
>  5 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw88/fw.c b/drivers/net/wireless/realtek/rtw88/fw.c
> index 49f09a9f4ed6..1ad25e0539c4 100644
> --- a/drivers/net/wireless/realtek/rtw88/fw.c
> +++ b/drivers/net/wireless/realtek/rtw88/fw.c
> @@ -1813,6 +1813,47 @@ void rtw_fw_update_beacon_work(struct work_struct *work)
>         mutex_unlock(&rtwdev->mutex);
>  }
> 
> +/* renew the countdown in the firmware's beacon page until it completes */

I think this is unnecessary. 

> +void rtw_fw_csa_beacon_work(struct wiphy *wiphy, struct wiphy_work *work)
> +{
> +       struct rtw_dev *rtwdev = container_of(work, struct rtw_dev,
> +                                             csa_beacon_work.work);
> +       struct rtw_rsvd_page *rsvd_pkt;
> +       struct ieee80211_vif *vif;
> +       unsigned int delay;
> +
> +       lockdep_assert_wiphy(wiphy);
> +
> +       mutex_lock(&rtwdev->mutex);
> +
> +       if (!test_bit(RTW_FLAG_RUNNING, rtwdev->flags))
> +               goto out;
> +
> +       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)
> +               goto out;
> +
> +       vif = rtwvif_to_vif(rsvd_pkt->rtwvif);

Should we define csa_beacon_work by vif? Then, here we can get rtwvif
(and vif) from work context.

> +       if (!vif->bss_conf.csa_active)
> +               goto out;
> +
> +       delay = ieee80211_tu_to_usec(vif->bss_conf.beacon_int);
> +
> +       if (!ieee80211_beacon_cntdwn_is_complete(vif, 0)) {
> +               rtw_fw_download_rsvd_page(rtwdev);
> +               rtw_send_rsvd_page_h2c(rtwdev);
> +
> +               wiphy_delayed_work_queue(wiphy, &rtwdev->csa_beacon_work,
> +                                        usecs_to_jiffies(delay));
> +       } else {
> +               ieee80211_csa_finish(vif, 0);
> +       }
> +
> +out:
> +       mutex_unlock(&rtwdev->mutex);
> +}
> +
>  static void rtw_fw_read_fifo_page(struct rtw_dev *rtwdev, u32 offset, u32 size,
>                                   u32 *buf, u32 residue, u16 start_pg)
>  {

[...]

> diff --git a/drivers/net/wireless/realtek/rtw88/mac80211.c
> b/drivers/net/wireless/realtek/rtw88/mac80211.c
> index 2a9b09fa76e7..7c2a373faff8 100644
> --- a/drivers/net/wireless/realtek/rtw88/mac80211.c
> +++ b/drivers/net/wireless/realtek/rtw88/mac80211.c
> @@ -235,6 +235,10 @@ 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);
> 
> +       if (rtwvif->net_type == RTW_NET_AP_MODE ||
> +           rtwvif->net_type == RTW_NET_AD_HOC)
> +               wiphy_delayed_work_cancel(hw->wiphy, &rtwdev->csa_beacon_work);
> +

Why should we need check Ad-hoc? How about just removing conditions?

As above comment, I'd move csa_beacon_work to vif.

>         mutex_lock(&rtwdev->mutex);
> 
>         rtw_leave_lps_deep(rtwdev);
> @@ -375,6 +379,13 @@ static void rtw_conf_tx(struct rtw_dev *rtwdev,
>                 __rtw_conf_tx(rtwdev, rtwvif, ac);
>  }
> 
> +/* renew the channel switch countdown one beacon interval from now */

No need this comment.

> +static void rtw_csa_beacon_queue(struct rtw_dev *rtwdev, u16 beacon_int)
> +{
> +       wiphy_delayed_work_queue(rtwdev->hw->wiphy, &rtwdev->csa_beacon_work,
> +                                usecs_to_jiffies(ieee80211_tu_to_usec(beacon_int)));

Just single one statement. Can't we just call wiphy_delayed_work_queue()
directly?

> +}
> +
>  static void rtw_ops_bss_info_changed(struct ieee80211_hw *hw,
>                                      struct ieee80211_vif *vif,
>                                      struct ieee80211_bss_conf *conf,
> @@ -438,6 +449,9 @@ 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);
> +               /* a hw restart replays the beacon, not channel_switch_beacon */

unnecessary comment. 

> +               if (conf->csa_active)
> +                       rtw_csa_beacon_queue(rtwdev, conf->beacon_int);
>         }
> 
>         if (changed & BSS_CHANGED_BEACON_ENABLED) {
> @@ -489,6 +503,8 @@ static void rtw_ops_stop_ap(struct ieee80211_hw *hw,
>  {
>         struct rtw_dev *rtwdev = hw->priv;
> 
> +       wiphy_delayed_work_cancel(hw->wiphy, &rtwdev->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 +572,16 @@ 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;
> +
> +       /* the beacon that starts the countdown was just downloaded */

unnecessary comment. 

> +       rtw_csa_beacon_queue(rtwdev, vif->bss_conf.beacon_int);
> +}
> +
>  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)

[...]

> @@ -900,6 +937,14 @@ static int rtw_ops_hw_scan(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
>                 return -EBUSY;
> 
>         mutex_lock(&rtwdev->mutex);
> +
> +       /* the stations count down to the new channel and expect the AP there */
> +       rtw_iterate_vifs(rtwdev, rtw_csa_active_iter, &csa_active);
> +       if (csa_active) {
> +               mutex_unlock(&rtwdev->mutex);
> +               return -EBUSY;
> +       }

Forgot to mention this by commit message?

> +
>         rtw_hw_scan_start(rtwdev, vif, req);
>         ret = rtw_hw_scan_offload(rtwdev, vif, true);
>         if (ret) {