Re: [PATCH v4 04/12] wifi: ath12k: vdev statemachine changes for single wiphy

[Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx>]

On 3/13/2024 3:55 AM, Jeff Johnson wrote:
> 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?
yeah, we can. Thanks for pointing out, will fix it in next version.
> > +
> > +	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
Sure, will remove this and below instance too.
> > +	}
> > +
> > +	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);