Received-SPF: pass (google.com: domain of linux-wireless+bounces-4678-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <de83c4fd-84de-432c-89c0-afc38b9adc77@quicinc.com>
Date: Wed, 13 Mar 2024 20:06:52 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 04/12] wifi: ath12k: vdev statemachine changes for
 single wiphy
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>, Sriram R <quic_srirrama@quicinc.com>
References: <20240312135557.1778379-1-quic_ramess@quicinc.com>
 <20240312135557.1778379-5-quic_ramess@quicinc.com>
 <4db3a534-42c6-4469-9ad1-764ab0018e8b@quicinc.com>
From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
In-Reply-To: <4db3a534-42c6-4469-9ad1-764ab0018e8b@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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@quicinc.com>
>>
>> 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@quicinc.com>
>> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>> ---
>>   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);
>