Received-SPF: pass (google.com: domain of linux-wireless+bounces-5743-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <71c8c3e2-290a-4866-b4ed-3c7d9f101c77@quicinc.com>
Date: Tue, 2 Apr 2024 15:44:59 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 07/12] wifi: ath12k: Cache vdev configs before vdev
 create
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>, Sriram R <quic_srirrama@quicinc.com>
References: <20240401190409.2461819-1-quic_ramess@quicinc.com>
 <20240401190409.2461819-8-quic_ramess@quicinc.com>
 <c5150881-7707-4b8f-b877-e5677039b0ed@quicinc.com>
Content-Language: en-US
From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
In-Reply-To: <c5150881-7707-4b8f-b877-e5677039b0ed@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 4/2/2024 12:56 AM, Jeff Johnson wrote:
> On 4/1/2024 12:04 PM, Rameshkumar Sundaram wrote:
>> From: Sriram R <quic_srirrama@quicinc.com>
>>
>> Since the vdev create for a corresponding vif is deferred
>> until a channel is assigned, cache the information which
>> are received through mac80211 ops between add_interface()
>> and assign_vif_chanctx() and set them once the vdev is
>> created on one of the ath12k radios as the channel gets
>> assigned via assign_vif_chanctx().
>>
>> 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>
>> Co-developed-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/core.h |  19 +++
>>   drivers/net/wireless/ath/ath12k/mac.c  | 160 +++++++++++++++++++------
>>   2 files changed, 145 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
>> index 507f08ab3ad5..dd6c474bd507 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.h
>> +++ b/drivers/net/wireless/ath/ath12k/core.h
>> @@ -214,6 +214,24 @@ enum ath12k_monitor_flags {
>>   	ATH12K_FLAG_MONITOR_ENABLED,
>>   };
>>   
>> +struct ath12k_tx_conf {
>> +	bool changed;
>> +	u16 ac;
>> +	struct ieee80211_tx_queue_params tx_queue_params;
>> +};
>> +
>> +struct ath12k_key_conf {
>> +	bool changed;
>> +	enum set_key_cmd cmd;
>> +	struct ieee80211_key_conf *key;
>> +};
>> +
>> +struct ath12k_vif_cache {
>> +	struct ath12k_tx_conf tx_conf;
>> +	struct ath12k_key_conf key_conf;
>> +	u32 bss_conf_changed;
>> +};
>> +
>>   struct ath12k_vif {
>>   	u32 vdev_id;
>>   	enum wmi_vdev_type vdev_type;
>> @@ -268,6 +286,7 @@ struct ath12k_vif {
>>   	u8 vdev_stats_id;
>>   	u32 punct_bitmap;
>>   	bool ps;
>> +	struct ath12k_vif_cache *cache;
>>   };
>>   
>>   struct ath12k_vif_iter {
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 60095d876a43..3f2d1b00ae24 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -3018,15 +3018,24 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
>>   {
>>   	struct ath12k *ar;
>>   	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>> +	struct ath12k_vif_cache *cache = arvif->cache;
>>   
>>   	ar = ath12k_get_ar_by_vif(hw, vif);
>>   
>> -	/* TODO if the vdev is not created on a certain radio,
>> +	/* if the vdev is not created on a certain radio,
>>   	 * cache the info to be updated later on vdev creation
>>   	 */
>>   
>> -	if (!ar)
>> +	if (!ar) {
>> +		if (!cache) {
>> +			cache = kzalloc(sizeof(*cache), GFP_KERNEL);
>> +			if (!cache)
>> +				return;
>> +			arvif->cache = cache;
>> +		}
> 
> since you have this core logic in several places, i'd refactor this into a
> separate function, i.e.
> ath12k_arvif_get_cache(arvif)
> 	if (!arvif->cache)
> 		arvif->cache = kzalloc(sizeof(*arvif->cache), GFP_KERNEL)
> 	return arvif->cache;
> 
> then all of the callers would have simply:
> 	cache = ath12k_arvif_get_cache(arvif);
> 	if (!cache) handle error;
> 	use cache
> 
Sure, will add get and put routines for cache and use them.
>> +		arvif->cache->bss_conf_changed |= changed;
>>   		return;
>> +	}
>>   
> 
> [...]
> 
>> @@ -6339,8 +6425,14 @@ static void ath12k_mac_op_remove_interface(struct ieee80211_hw *hw,
>>   	struct ath12k *ar;
>>   	int ret;
>>   
>> -	if (!arvif->is_created)
>> +	if (!arvif->is_created) {
>> +		/* if we cached some config but never received assign chanctx,
>> +		 * free the allocated cache.
>> +		 */
>> +		kfree(arvif->cache);
>> +		arvif->cache = NULL;
> 
> perhaps for the free code also have:
> ath12k_arvif_put_cache(arvif)
> 	kfree(arvif->cache);
> 	arvif->cache = NULL;
> 
>
>>   		return;
>> +	}
>>   
>>   	ar = arvif->ar;
>>   	ab = ar->ab;
>