On 3/12/2024 6:55 AM, Rameshkumar Sundaram wrote:
> From: Sriram R <quic_srirrama@xxxxxxxxxxx>
>
> With single wiphy, multiple radios are combined into a single wiphy.
> Since any channel can be assigned to a vif being brought up,
> the vdev cannot be created during add_interface(). Hence defer the
> vdev creation till channel assignment.
>
> If only one radio is part of the wiphy, then the existing logic
> is maintained, i.e vdevs are created during add interface and
> started during channel assignment. This ensures no functional changes
> to single pdev devices which has only one radio in the SoC.
>
> Tested-on: QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>
> Signed-off-by: Sriram R <quic_srirrama@xxxxxxxxxxx>
> Signed-off-by: Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx>
> ---
> drivers/net/wireless/ath/ath12k/core.h | 1 +
> drivers/net/wireless/ath/ath12k/hw.h | 1 +
> drivers/net/wireless/ath/ath12k/mac.c | 203 +++++++++++++++++--------
> 3 files changed, 144 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 53bcf9416efd..70daec38d486 100644
> --- a/drivers/net/wireless/ath/ath12k/core.h
> +++ b/drivers/net/wireless/ath/ath12k/core.h
> @@ -251,6 +251,7 @@ struct ath12k_vif {
> } ap;
> } u;
>
> + bool is_created;
> bool is_started;
> bool is_up;
> u32 aid;
> diff --git a/drivers/net/wireless/ath/ath12k/hw.h b/drivers/net/wireless/ath/ath12k/hw.h
> index 87965980b938..e34c4f76c1ec 100644
> --- a/drivers/net/wireless/ath/ath12k/hw.h
> +++ b/drivers/net/wireless/ath/ath12k/hw.h
> @@ -80,6 +80,7 @@
> #define TARGET_RX_PEER_METADATA_VER_V1A 2
> #define TARGET_RX_PEER_METADATA_VER_V1B 3
>
> +#define ATH12K_HW_DEFAULT_QUEUE 0
> #define ATH12K_HW_MAX_QUEUES 4
> #define ATH12K_QUEUE_LEN 4096
>
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 4afaba3ba934..b6afef81a2d8 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -5780,64 +5780,24 @@ static void ath12k_mac_op_update_vif_offload(struct ieee80211_hw *hw,
> ath12k_mac_update_vif_offload(arvif);
> }
>
> -static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> - struct ieee80211_vif *vif)
> +static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif)
> {
> - struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> - struct ath12k *ar;
> - struct ath12k_base *ab;
> + struct ath12k_hw *ah = ar->ah;
> + struct ath12k_base *ab = ar->ab;
> + struct ieee80211_hw *hw = ah->hw;
> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> struct ath12k_wmi_vdev_create_arg vdev_arg = {0};
> struct ath12k_wmi_peer_create_arg peer_param;
> u32 param_id, param_value;
> u16 nss;
> int i;
> - int ret;
> - int bit;
> -
> - vif->driver_flags |= IEEE80211_VIF_SUPPORTS_UAPSD;
> + int ret, vdev_id;
>
> - ar = ath12k_ah_to_ar(ah, 0);
> - ab = ar->ab;
> -
> - mutex_lock(&ar->conf_mutex);
> -
> - if (vif->type == NL80211_IFTYPE_AP &&
> - ar->num_peers > (ar->max_num_peers - 1)) {
> - ath12k_warn(ab, "failed to create vdev due to insufficient peer entry resource in firmware\n");
> - ret = -ENOBUFS;
> - goto err;
> - }
> -
> - if (ar->num_created_vdevs > (TARGET_NUM_VDEVS - 1)) {
> - ath12k_warn(ab, "failed to create vdev, reached max vdev limit %d\n",
> - TARGET_NUM_VDEVS);
> - ret = -EBUSY;
> - goto err;
> - }
> -
> - memset(arvif, 0, sizeof(*arvif));
> + lockdep_assert_held(&ar->conf_mutex);
>
> arvif->ar = ar;
> - arvif->vif = vif;
> -
> - INIT_LIST_HEAD(&arvif->list);
> -
> - /* Should we initialize any worker to handle connection loss indication
> - * from firmware in sta mode?
> - */
> -
> - for (i = 0; i < ARRAY_SIZE(arvif->bitrate_mask.control); i++) {
> - arvif->bitrate_mask.control[i].legacy = 0xffffffff;
> - memset(arvif->bitrate_mask.control[i].ht_mcs, 0xff,
> - sizeof(arvif->bitrate_mask.control[i].ht_mcs));
> - memset(arvif->bitrate_mask.control[i].vht_mcs, 0xff,
> - sizeof(arvif->bitrate_mask.control[i].vht_mcs));
> - }
> -
> - bit = __ffs64(ab->free_vdev_map);
> -
> - arvif->vdev_id = bit;
> + vdev_id = __ffs64(ab->free_vdev_map);
> + arvif->vdev_id = vdev_id;
> arvif->vdev_subtype = WMI_VDEV_SUBTYPE_NONE;
>
> switch (vif->type) {
> @@ -5861,7 +5821,7 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> break;
> case NL80211_IFTYPE_MONITOR:
> arvif->vdev_type = WMI_VDEV_TYPE_MONITOR;
> - ar->monitor_vdev_id = bit;
> + ar->monitor_vdev_id = vdev_id;
> break;
> case NL80211_IFTYPE_P2P_DEVICE:
> arvif->vdev_type = WMI_VDEV_TYPE_STA;
> @@ -5872,7 +5832,7 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> break;
> }
>
> - ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "mac add interface id %d type %d subtype %d map %llx\n",
> + ath12k_dbg(ar->ab, ATH12K_DBG_MAC, "mac vdev create id %d type %d subtype %d map %llx\n",
> arvif->vdev_id, arvif->vdev_type, arvif->vdev_subtype,
> ab->free_vdev_map);
>
> @@ -5890,6 +5850,7 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> }
>
> ar->num_created_vdevs++;
> + arvif->is_created = true;
> ath12k_dbg(ab, ATH12K_DBG_MAC, "vdev %pM created, vdev_id %d\n",
> vif->addr, arvif->vdev_id);
> ar->allocated_vdev_map |= 1LL << arvif->vdev_id;
> @@ -5990,8 +5951,6 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> if (vif->type != NL80211_IFTYPE_MONITOR && ar->monitor_conf_enabled)
> ath12k_mac_monitor_vdev_create(ar);
>
> - mutex_unlock(&ar->conf_mutex);
> -
> return ret;
>
> err_peer_del:
> @@ -6017,6 +5976,7 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> err_vdev_del:
> ath12k_wmi_vdev_delete(ar, arvif->vdev_id);
> ar->num_created_vdevs--;
> + arvif->is_created = false;
> ar->allocated_vdev_map &= ~(1LL << arvif->vdev_id);
> ab->free_vdev_map |= 1LL << arvif->vdev_id;
> ab->free_vdev_stats_id_map &= ~(1LL << arvif->vdev_stats_id);
> @@ -6025,9 +5985,104 @@ static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> spin_unlock_bh(&ar->data_lock);
>
> err:
> + arvif->ar = NULL;
> + return ret;
> +}
> +
> +static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif,
> + struct ieee80211_chanctx_conf *ctx)
> +{
> + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> + struct ath12k_hw *ah = hw->priv;
> + struct ath12k_base *ab;
> + struct ath12k *ar;
> + int ret;
> + u8 bit;
> +
> + if (arvif->ar) {
> + WARN_ON(!arvif->is_created);
> + goto out;
> + }
> +
> + if (ah->num_radio == 1)
> + ar = ah->radio;
> + else if (ctx)
> + ar = ath12k_get_ar_by_ctx(hw, ctx);
> + else
> + return NULL;
> +
> + if (!ar)
> + goto out;
why does this goto out instead of just return NULL?
> +
> + ab = ar->ab;
> +
> + mutex_lock(&ar->conf_mutex);
> +
> + if (vif->type == NL80211_IFTYPE_AP &&
> + ar->num_peers > (ar->max_num_peers - 1)) {
> + ath12k_warn(ab, "failed to create vdev due to insufficient peer entry resource in firmware\n");
> + ret = -ENOBUFS;
> + goto unlock;
nothing is done with ret so setting it is pointless
> + }
> +
> + if (ar->num_created_vdevs > (TARGET_NUM_VDEVS - 1)) {
> + ath12k_warn(ab, "failed to create vdev, reached max vdev limit %d\n",
> + TARGET_NUM_VDEVS);
> + ret = -EBUSY;
> + goto unlock;
nothing is done with ret so setting it is pointless
> + }
> +
> + ret = ath12k_mac_vdev_create(ar, vif);
> + if (ret) {
> + ath12k_warn(ab, "failed to create vdev %d ret %d", bit, ret);
> + goto unlock;
> + }
> +
> + /* TODO If the vdev is created during channel assign and not during
> + * add_interface(), Apply any parameters for the vdev which were received
> + * after add_interface, corresponding to this vif.
> + */
> +unlock:
> mutex_unlock(&ar->conf_mutex);
> +out:
> + return arvif->ar;
> +}
>
> - return ret;
> +static int ath12k_mac_op_add_interface(struct ieee80211_hw *hw,
> + struct ieee80211_vif *vif)
> +{
> + struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> + int i;
> +
> + memset(arvif, 0, sizeof(*arvif));
> +
> + arvif->vif = vif;
> +
> + INIT_LIST_HEAD(&arvif->list);
> +
> + for (i = 0; i < ARRAY_SIZE(arvif->bitrate_mask.control); i++) {
> + arvif->bitrate_mask.control[i].legacy = 0xffffffff;
> + memset(arvif->bitrate_mask.control[i].ht_mcs, 0xff,
> + sizeof(arvif->bitrate_mask.control[i].ht_mcs));
> + memset(arvif->bitrate_mask.control[i].vht_mcs, 0xff,
> + sizeof(arvif->bitrate_mask.control[i].vht_mcs));
> + }
> +
> + /* Allocate Default Queue now and reassign during actual vdev create */
> + vif->cab_queue = ATH12K_HW_DEFAULT_QUEUE;
> + for (i = 0; i < ARRAY_SIZE(vif->hw_queue); i++)
> + vif->hw_queue[i] = ATH12K_HW_DEFAULT_QUEUE;
> +
> + vif->driver_flags |= IEEE80211_VIF_SUPPORTS_UAPSD;
> +
> + /* For single radio wiphy(i.e ah->num_radio is 1), create the vdev
> + * during add_interface itself, for multi radio wiphy, defer the vdev
> + * creation until channel_assign to determine the radio on which the
> + * vdev needs to be created
> + */
> + ath12k_mac_assign_vif_to_vdev(hw, vif, NULL);
> + return 0;
> }
>
> static void ath12k_mac_vif_unref(struct ath12k_dp *dp, struct ieee80211_vif *vif)
> @@ -6058,14 +6113,16 @@ static void ath12k_mac_vif_unref(struct ath12k_dp *dp, struct ieee80211_vif *vif
> static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
> struct ieee80211_vif *vif)
> {
> - struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> struct ath12k *ar;
> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> struct ath12k_base *ab;
> unsigned long time_left;
> int ret;
>
> - ar = ath12k_ah_to_ar(ah, 0);
> + if (!arvif->is_created)
> + return;
> +
> + ar = arvif->ar;
> ab = ar->ab;
>
> mutex_lock(&ar->conf_mutex);
> @@ -6107,6 +6164,7 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
> ar->allocated_vdev_map &= ~(1LL << arvif->vdev_id);
> ab->free_vdev_stats_id_map &= ~(1LL << arvif->vdev_stats_id);
> ar->num_created_vdevs--;
> + arvif->is_created = false;
>
> ath12k_dbg(ab, ATH12K_DBG_MAC, "vdev %pM deleted, vdev_id %d\n",
> vif->addr, arvif->vdev_id);
> @@ -6759,14 +6817,21 @@ ath12k_mac_op_assign_vif_chanctx(struct ieee80211_hw *hw,
> struct ieee80211_bss_conf *link_conf,
> struct ieee80211_chanctx_conf *ctx)
> {
> - struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> struct ath12k *ar;
> struct ath12k_base *ab;
> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> int ret;
> struct ath12k_wmi_peer_create_arg param;
>
> - ar = ath12k_ah_to_ar(ah, 0);
> + /* For multi radio wiphy, the vdev was not created during add_interface
> + * create now since we have a channel ctx now to assign to a specific ar/fw
> + */
> + ar = ath12k_mac_assign_vif_to_vdev(hw, vif, ctx);
> + if (!ar) {
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> ab = ar->ab;
>
> mutex_lock(&ar->conf_mutex);
> @@ -6842,13 +6907,22 @@ ath12k_mac_op_unassign_vif_chanctx(struct ieee80211_hw *hw,
> struct ieee80211_bss_conf *link_conf,
> struct ieee80211_chanctx_conf *ctx)
> {
> - struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> struct ath12k *ar;
> struct ath12k_base *ab;
> struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> int ret;
>
> - ar = ath12k_ah_to_ar(ah, 0);
> + /* The vif is expected to be attached to an ar's VDEV.
> + * We leave the vif/vdev in this function as is
> + * and not delete the vdev symmetric to assign_vif_chanctx()
> + * the VDEV will be deleted and unassigned either during
> + * remove_interface() or when there is a change in channel
> + * that moves the vif to a new ar
> + */
> + if (!arvif->is_created)
> + return;
> +
> + ar = arvif->ar;
> ab = ar->ab;
>
> mutex_lock(&ar->conf_mutex);
> @@ -6900,13 +6974,20 @@ ath12k_mac_op_switch_vif_chanctx(struct ieee80211_hw *hw,
> int n_vifs,
> enum ieee80211_chanctx_switch_mode mode)
> {
> - struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> struct ath12k *ar;
>
> - ar = ath12k_ah_to_ar(ah, 0);
> + ar = ath12k_get_ar_by_ctx(hw, vifs->old_ctx);
> + if (!ar)
> + return -EINVAL;
>
> mutex_lock(&ar->conf_mutex);
>
> + /* Switching channels across radio is not allowed */
> + if (ar != ath12k_get_ar_by_ctx(hw, vifs->new_ctx)) {
> + mutex_unlock(&ar->conf_mutex);
> + return -EINVAL;
> + }
> +
> ath12k_dbg(ar->ab, ATH12K_DBG_MAC,
> "mac chanctx switch n_vifs %d mode %d\n",
> n_vifs, mode);
