diff mbox series

[v5,07/12] wifi: ath12k: Cache vdev configs before vdev create

Message ID 20240320190943.3850106-8-quic_ramess@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series wifi: ath12k: Add single wiphy support | expand

Commit Message

Rameshkumar Sundaram March 20, 2024, 7:09 p.m. UTC 
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>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
---
 drivers/net/wireless/ath/ath12k/core.h |  19 ++++
 drivers/net/wireless/ath/ath12k/mac.c  | 149 ++++++++++++++++++++-----
 2 files changed, 140 insertions(+), 28 deletions(-)

Comments

Jeff Johnson March 21, 2024, 9:04 p.m. UTC | #1 
On 3/20/2024 12:09 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>

in any of these patches where you are performing significant rework please
feel free to add a Co-developed-by: tag for yourself

> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath12k/core.h |  19 ++++
>  drivers/net/wireless/ath/ath12k/mac.c  | 149 ++++++++++++++++++++-----
>  2 files changed, 140 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> index 507f08ab3ad5..fa0606c460c6 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;

in my v4 comment I had suggested that this cache be a pointer.
instead you chose to add pointers to the subordinate records within the cache.
why take that approach?

>  };
>  
>  struct ath12k_vif_iter {
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 50c8c6ddc167..0274eac33b1f 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -3021,12 +3021,14 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
>  
>  	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) {
> +		arvif->cache.bss_conf_changed |= changed;
>  		return;
> +	}
>  
>  	mutex_lock(&ar->conf_mutex);
>  
> @@ -3517,12 +3519,11 @@ static int ath12k_clear_peer_keys(struct ath12k_vif *arvif,
>  	return first_errno;
>  }
>  
> -static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> -				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> -				 struct ieee80211_key_conf *key)
> +static int ath12k_mac_set_key(struct ath12k *ar, enum set_key_cmd cmd,
> +			      struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> +			      struct ieee80211_key_conf *key)
>  {
> -	struct ath12k *ar;
> -	struct ath12k_base *ab;
> +	struct ath12k_base *ab = ar->ab;
>  	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>  	struct ath12k_peer *peer;
>  	struct ath12k_sta *arsta;
> @@ -3530,28 +3531,11 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  	int ret = 0;
>  	u32 flags = 0;
>  
> -	/* BIP needs to be done in software */
> -	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
> -		return 1;
> -
> -	ar = ath12k_get_ar_by_vif(hw, vif);
> -	if (!ar) {
> -		WARN_ON_ONCE(1);
> -		return -EINVAL;
> -	}
> -	ab = ar->ab;
> +	lockdep_assert_held(&ar->conf_mutex);
>  
> -	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ar->ab->dev_flags))
> +	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ab->dev_flags))
>  		return 1;
>  
> -	if (key->keyidx > WMI_MAX_KEY_INDEX)
> -		return -ENOSPC;
> -
> -	mutex_lock(&ar->conf_mutex);
> -
>  	if (sta)
>  		peer_addr = sta->addr;
>  	else if (arvif->vdev_type == WMI_VDEV_TYPE_STA)
> @@ -3643,6 +3627,51 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>  	spin_unlock_bh(&ab->base_lock);
>  
>  exit:
> +	return ret;
> +}
> +
> +static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> +				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> +				 struct ieee80211_key_conf *key)
> +{
> +	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> +	struct ath12k_key_conf *key_conf;
> +	struct ath12k *ar;
> +	int ret;
> +
> +	/* BIP needs to be done in software */
> +	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
> +		return 1;
> +
> +	if (key->keyidx > WMI_MAX_KEY_INDEX)
> +		return -ENOSPC;
> +
> +	ar = ath12k_get_ar_by_vif(hw, vif);
> +	if (!ar) {
> +		/* ar is expected to be valid when sta ptr is available */
> +		if (sta) {
> +			WARN_ON_ONCE(1);
> +			return -EINVAL;
> +		}
> +
> +		key_conf = arvif->cache.key_conf;
> +		if (!key_conf) {
> +			key_conf = kzalloc(sizeof(*key_conf), GFP_KERNEL);
> +			if (!key_conf)
> +				return -ENOSPC;
> +			arvif->cache.key_conf = key_conf;
> +		}
> +		key_conf->cmd = cmd;
> +		key_conf->key = key;
> +		key_conf->changed = true;

if you are allocating the individual structs do you need this flag? isn't the
presence of the cache.<foo>_conf pointer itself sufficient?

> +		return 0;
> +	}
> +
> +	mutex_lock(&ar->conf_mutex);
> +	ret = ath12k_mac_set_key(ar, cmd, vif, sta, key);
>  	mutex_unlock(&ar->conf_mutex);
>  	return ret;
>  }
> @@ -4473,11 +4502,22 @@ static int ath12k_mac_op_conf_tx(struct ieee80211_hw *hw,
>  {
>  	struct ath12k *ar;
>  	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> +	struct ath12k_tx_conf *tx_conf;
>  	int ret;
>  
>  	ar = ath12k_get_ar_by_vif(hw, vif);
>  	if (!ar) {
> -		/* TODO cache the info and apply after vdev is created */
> +		/* cache the info and apply after vdev is created */
> +		tx_conf = arvif->cache.tx_conf;
> +		if (!tx_conf) {
> +			tx_conf = kzalloc(sizeof(*tx_conf), GFP_KERNEL);
> +			if (!tx_conf)
> +				return -ENOSPC;
> +			arvif->cache.tx_conf = tx_conf;
> +		}
> +		tx_conf->changed = true;
> +		tx_conf->ac = ac;
> +		tx_conf->tx_queue_params = *params;
>  		return -EINVAL;
>  	}
>  
> @@ -6121,6 +6161,57 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif)
>  	return ret;
>  }
>  
> +static void ath12k_mac_vif_cache_free(struct ath12k *ar, struct ath12k_vif *arvif)
> +{
> +	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
> +	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
> +
> +	lockdep_assert_held(&ar->conf_mutex);
> +
> +	kfree(tx_conf);
> +	arvif->cache.tx_conf = NULL;
> +	kfree(key_conf);
> +	arvif->cache.key_conf = NULL;
> +	arvif->cache.bss_conf_changed = 0;
> +}
> +
> +static void ath12k_mac_vif_cache_flush(struct ath12k *ar,  struct ieee80211_vif *vif)
> +{
> +	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
> +	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
> +	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
> +	struct ath12k_base *ab = ar->ab;
> +
> +	int ret;
> +
> +	lockdep_assert_held(&ar->conf_mutex);
> +
> +	if (tx_conf && tx_conf->changed) {

as mentioned above if you are allocating the per-op cach structs individually
then you should not need the changed flag, this can just become:
	if (txconf) {

> +		ret = ath12k_mac_conf_tx(arvif, 0, tx_conf->ac,
> +					 &tx_conf->tx_queue_params);
> +		if (ret)
> +			ath12k_warn(ab,
> +				    "unable to apply tx config parameters to vdev %d\n",
> +				    ret);
> +	}
> +
> +	if (arvif->cache.bss_conf_changed) {
> +		ath12k_mac_bss_info_changed(ar, arvif, &vif->bss_conf,
> +					    arvif->cache.bss_conf_changed);
> +	}
> +
> +	if (key_conf && key_conf->changed) {

	if (key_conf) {

> +		ret = ath12k_mac_set_key(ar, key_conf->cmd, vif, NULL,
> +					 key_conf->key);
> +		if (ret) {
> +			WARN_ON_ONCE(1);

why have this when you're using ath12k_warn()?

> +			ath12k_warn(ab, "unable to apply set key param to vdev %d ret %d\n",
> +				    arvif->vdev_id, ret);
> +		}
> +	}
> +	ath12k_mac_vif_cache_free(ar, arvif);
> +}
> +
>  static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>  						    struct ieee80211_vif *vif,
>  						    struct ieee80211_chanctx_conf *ctx)
> @@ -6201,10 +6292,11 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>  		goto unlock;
>  	}
>  
> -	/* TODO If the vdev is created during channel assign and not during
> +	/* 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.
>  	 */
> +	ath12k_mac_vif_cache_flush(ar, vif);
>  unlock:
>  	mutex_unlock(&ar->conf_mutex);
>  out:
> @@ -6316,6 +6408,7 @@ static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif)
>  	spin_unlock_bh(&ar->data_lock);
>  
>  	ath12k_peer_cleanup(ar, arvif->vdev_id);
> +	ath12k_mac_vif_cache_free(ar, arvif);
>  
>  	idr_for_each(&ar->txmgmt_idr,
>  		     ath12k_mac_vif_txmgmt_idr_remove, vif);
Rameshkumar Sundaram March 25, 2024, 5:49 p.m. UTC | #2 
On 3/22/2024 2:34 AM, Jeff Johnson wrote:
> On 3/20/2024 12:09 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>
> 
> in any of these patches where you are performing significant rework please
> feel free to add a Co-developed-by: tag for yourself
Sure, Will add it in next version.
> 
>> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/core.h |  19 ++++
>>   drivers/net/wireless/ath/ath12k/mac.c  | 149 ++++++++++++++++++++-----
>>   2 files changed, 140 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
>> index 507f08ab3ad5..fa0606c460c6 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;
> 
> in my v4 comment I had suggested that this cache be a pointer.
> instead you chose to add pointers to the subordinate records within the cache.
> why take that approach?
> 
Thought of making the actual cache objects as pointers, instead of 
allocating memory for all type(key,tx) of caches during vdev create itself.
So that well allocate required cache alone if needed. Thoughts ?
>>   };
>>   
>>   struct ath12k_vif_iter {
>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>> index 50c8c6ddc167..0274eac33b1f 100644
>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>> @@ -3021,12 +3021,14 @@ static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
>>   
>>   	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) {
>> +		arvif->cache.bss_conf_changed |= changed;
>>   		return;
>> +	}
>>   
>>   	mutex_lock(&ar->conf_mutex);
>>   
>> @@ -3517,12 +3519,11 @@ static int ath12k_clear_peer_keys(struct ath12k_vif *arvif,
>>   	return first_errno;
>>   }
>>   
>> -static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>> -				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
>> -				 struct ieee80211_key_conf *key)
>> +static int ath12k_mac_set_key(struct ath12k *ar, enum set_key_cmd cmd,
>> +			      struct ieee80211_vif *vif, struct ieee80211_sta *sta,
>> +			      struct ieee80211_key_conf *key)
>>   {
>> -	struct ath12k *ar;
>> -	struct ath12k_base *ab;
>> +	struct ath12k_base *ab = ar->ab;
>>   	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>>   	struct ath12k_peer *peer;
>>   	struct ath12k_sta *arsta;
>> @@ -3530,28 +3531,11 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>>   	int ret = 0;
>>   	u32 flags = 0;
>>   
>> -	/* BIP needs to be done in software */
>> -	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
>> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
>> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
>> -	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
>> -		return 1;
>> -
>> -	ar = ath12k_get_ar_by_vif(hw, vif);
>> -	if (!ar) {
>> -		WARN_ON_ONCE(1);
>> -		return -EINVAL;
>> -	}
>> -	ab = ar->ab;
>> +	lockdep_assert_held(&ar->conf_mutex);
>>   
>> -	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ar->ab->dev_flags))
>> +	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ab->dev_flags))
>>   		return 1;
>>   
>> -	if (key->keyidx > WMI_MAX_KEY_INDEX)
>> -		return -ENOSPC;
>> -
>> -	mutex_lock(&ar->conf_mutex);
>> -
>>   	if (sta)
>>   		peer_addr = sta->addr;
>>   	else if (arvif->vdev_type == WMI_VDEV_TYPE_STA)
>> @@ -3643,6 +3627,51 @@ static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>>   	spin_unlock_bh(&ab->base_lock);
>>   
>>   exit:
>> +	return ret;
>> +}
>> +
>> +static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
>> +				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
>> +				 struct ieee80211_key_conf *key)
>> +{
>> +	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>> +	struct ath12k_key_conf *key_conf;
>> +	struct ath12k *ar;
>> +	int ret;
>> +
>> +	/* BIP needs to be done in software */
>> +	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
>> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
>> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
>> +	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
>> +		return 1;
>> +
>> +	if (key->keyidx > WMI_MAX_KEY_INDEX)
>> +		return -ENOSPC;
>> +
>> +	ar = ath12k_get_ar_by_vif(hw, vif);
>> +	if (!ar) {
>> +		/* ar is expected to be valid when sta ptr is available */
>> +		if (sta) {
>> +			WARN_ON_ONCE(1);
>> +			return -EINVAL;
>> +		}
>> +
>> +		key_conf = arvif->cache.key_conf;
>> +		if (!key_conf) {
>> +			key_conf = kzalloc(sizeof(*key_conf), GFP_KERNEL);
>> +			if (!key_conf)
>> +				return -ENOSPC;
>> +			arvif->cache.key_conf = key_conf;
>> +		}
>> +		key_conf->cmd = cmd;
>> +		key_conf->key = key;
>> +		key_conf->changed = true;
> 
> if you are allocating the individual structs do you need this flag? isn't the
> presence of the cache.<foo>_conf pointer itself sufficient?
Yeah, that's correct. Thanks for pointing it out. will fix this as well 
as the other instance below.
> 
>> +		return 0;
>> +	}
>> +
>> +	mutex_lock(&ar->conf_mutex);
>> +	ret = ath12k_mac_set_key(ar, cmd, vif, sta, key);
>>   	mutex_unlock(&ar->conf_mutex);
>>   	return ret;
>>   }
>> @@ -4473,11 +4502,22 @@ static int ath12k_mac_op_conf_tx(struct ieee80211_hw *hw,
>>   {
>>   	struct ath12k *ar;
>>   	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>> +	struct ath12k_tx_conf *tx_conf;
>>   	int ret;
>>   
>>   	ar = ath12k_get_ar_by_vif(hw, vif);
>>   	if (!ar) {
>> -		/* TODO cache the info and apply after vdev is created */
>> +		/* cache the info and apply after vdev is created */
>> +		tx_conf = arvif->cache.tx_conf;
>> +		if (!tx_conf) {
>> +			tx_conf = kzalloc(sizeof(*tx_conf), GFP_KERNEL);
>> +			if (!tx_conf)
>> +				return -ENOSPC;
>> +			arvif->cache.tx_conf = tx_conf;
>> +		}
>> +		tx_conf->changed = true;
>> +		tx_conf->ac = ac;
>> +		tx_conf->tx_queue_params = *params;
>>   		return -EINVAL;
>>   	}
>>   
>> @@ -6121,6 +6161,57 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif)
>>   	return ret;
>>   }
>>   
>> +static void ath12k_mac_vif_cache_free(struct ath12k *ar, struct ath12k_vif *arvif)
>> +{
>> +	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
>> +	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
>> +
>> +	lockdep_assert_held(&ar->conf_mutex);
>> +
>> +	kfree(tx_conf);
>> +	arvif->cache.tx_conf = NULL;
>> +	kfree(key_conf);
>> +	arvif->cache.key_conf = NULL;
>> +	arvif->cache.bss_conf_changed = 0;
>> +}
>> +
>> +static void ath12k_mac_vif_cache_flush(struct ath12k *ar,  struct ieee80211_vif *vif)
>> +{
>> +	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
>> +	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
>> +	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
>> +	struct ath12k_base *ab = ar->ab;
>> +
>> +	int ret;
>> +
>> +	lockdep_assert_held(&ar->conf_mutex);
>> +
>> +	if (tx_conf && tx_conf->changed) {
> 
> as mentioned above if you are allocating the per-op cach structs individually
> then you should not need the changed flag, this can just become:
> 	if (txconf) {
> 
>> +		ret = ath12k_mac_conf_tx(arvif, 0, tx_conf->ac,
>> +					 &tx_conf->tx_queue_params);
>> +		if (ret)
>> +			ath12k_warn(ab,
>> +				    "unable to apply tx config parameters to vdev %d\n",
>> +				    ret);
>> +	}
>> +
>> +	if (arvif->cache.bss_conf_changed) {
>> +		ath12k_mac_bss_info_changed(ar, arvif, &vif->bss_conf,
>> +					    arvif->cache.bss_conf_changed);
>> +	}
>> +
>> +	if (key_conf && key_conf->changed) {
> 
> 	if (key_conf) {
> 
>> +		ret = ath12k_mac_set_key(ar, key_conf->cmd, vif, NULL,
>> +					 key_conf->key);
>> +		if (ret) {
>> +			WARN_ON_ONCE(1);
> 
> why have this when you're using ath12k_warn()?
The intention was get the current call stack dumped(to know how we 
landed this undesired scenario) along with vdev id information below.
But, Thinking again, this is called only during vdev create and the call 
stack is not really going to help much. Will probably remove it and add 
it later if needed.
> 
>> +			ath12k_warn(ab, "unable to apply set key param to vdev %d ret %d\n",
>> +				    arvif->vdev_id, ret);
>> +		}
>> +	}
>> +	ath12k_mac_vif_cache_free(ar, arvif);
>> +}
>> +
>>   static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>>   						    struct ieee80211_vif *vif,
>>   						    struct ieee80211_chanctx_conf *ctx)
>> @@ -6201,10 +6292,11 @@ static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
>>   		goto unlock;
>>   	}
>>   
>> -	/* TODO If the vdev is created during channel assign and not during
>> +	/* 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.
>>   	 */
>> +	ath12k_mac_vif_cache_flush(ar, vif);
>>   unlock:
>>   	mutex_unlock(&ar->conf_mutex);
>>   out:
>> @@ -6316,6 +6408,7 @@ static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif)
>>   	spin_unlock_bh(&ar->data_lock);
>>   
>>   	ath12k_peer_cleanup(ar, arvif->vdev_id);
>> +	ath12k_mac_vif_cache_free(ar, arvif);
>>   
>>   	idr_for_each(&ar->txmgmt_idr,
>>   		     ath12k_mac_vif_txmgmt_idr_remove, vif);
>
Jeff Johnson March 25, 2024, 6:13 p.m. UTC | #3 
On 3/25/2024 10:49 AM, Rameshkumar Sundaram wrote:
> On 3/22/2024 2:34 AM, Jeff Johnson wrote:
>> On 3/20/2024 12:09 PM, Rameshkumar Sundaram wrote:
>>> +	struct ath12k_vif_cache cache;
>>
>> in my v4 comment I had suggested that this cache be a pointer.
>> instead you chose to add pointers to the subordinate records within the cache.
>> why take that approach?
>>
> Thought of making the actual cache objects as pointers, instead of 
> allocating memory for all type(key,tx) of caches during vdev create itself.
> So that well allocate required cache alone if needed. Thoughts ?

There is a tradeoff here.

This cache is temporary, but the allocation exists for the lifetime of the
struct. So by having a single pointer here you only waste one pointer, but at
the expense that for each object type you have to see if the cache is already
present, and if not, then allocate the cache.

I'm OK either way, but just wanted to point out that the current approach
wastes more memory when the cache isn't needed.

/jeff
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 507f08ab3ad5..fa0606c460c6 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 50c8c6ddc167..0274eac33b1f 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -3021,12 +3021,14 @@  static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
 
 	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) {
+		arvif->cache.bss_conf_changed |= changed;
 		return;
+	}
 
 	mutex_lock(&ar->conf_mutex);
 
@@ -3517,12 +3519,11 @@  static int ath12k_clear_peer_keys(struct ath12k_vif *arvif,
 	return first_errno;
 }
 
-static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
-				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
-				 struct ieee80211_key_conf *key)
+static int ath12k_mac_set_key(struct ath12k *ar, enum set_key_cmd cmd,
+			      struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+			      struct ieee80211_key_conf *key)
 {
-	struct ath12k *ar;
-	struct ath12k_base *ab;
+	struct ath12k_base *ab = ar->ab;
 	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
 	struct ath12k_peer *peer;
 	struct ath12k_sta *arsta;
@@ -3530,28 +3531,11 @@  static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 	int ret = 0;
 	u32 flags = 0;
 
-	/* BIP needs to be done in software */
-	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
-	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
-	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
-	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
-		return 1;
-
-	ar = ath12k_get_ar_by_vif(hw, vif);
-	if (!ar) {
-		WARN_ON_ONCE(1);
-		return -EINVAL;
-	}
-	ab = ar->ab;
+	lockdep_assert_held(&ar->conf_mutex);
 
-	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ar->ab->dev_flags))
+	if (test_bit(ATH12K_FLAG_HW_CRYPTO_DISABLED, &ab->dev_flags))
 		return 1;
 
-	if (key->keyidx > WMI_MAX_KEY_INDEX)
-		return -ENOSPC;
-
-	mutex_lock(&ar->conf_mutex);
-
 	if (sta)
 		peer_addr = sta->addr;
 	else if (arvif->vdev_type == WMI_VDEV_TYPE_STA)
@@ -3643,6 +3627,51 @@  static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
 	spin_unlock_bh(&ab->base_lock);
 
 exit:
+	return ret;
+}
+
+static int ath12k_mac_op_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
+				 struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+				 struct ieee80211_key_conf *key)
+{
+	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
+	struct ath12k_key_conf *key_conf;
+	struct ath12k *ar;
+	int ret;
+
+	/* BIP needs to be done in software */
+	if (key->cipher == WLAN_CIPHER_SUITE_AES_CMAC ||
+	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_128 ||
+	    key->cipher == WLAN_CIPHER_SUITE_BIP_GMAC_256 ||
+	    key->cipher == WLAN_CIPHER_SUITE_BIP_CMAC_256)
+		return 1;
+
+	if (key->keyidx > WMI_MAX_KEY_INDEX)
+		return -ENOSPC;
+
+	ar = ath12k_get_ar_by_vif(hw, vif);
+	if (!ar) {
+		/* ar is expected to be valid when sta ptr is available */
+		if (sta) {
+			WARN_ON_ONCE(1);
+			return -EINVAL;
+		}
+
+		key_conf = arvif->cache.key_conf;
+		if (!key_conf) {
+			key_conf = kzalloc(sizeof(*key_conf), GFP_KERNEL);
+			if (!key_conf)
+				return -ENOSPC;
+			arvif->cache.key_conf = key_conf;
+		}
+		key_conf->cmd = cmd;
+		key_conf->key = key;
+		key_conf->changed = true;
+		return 0;
+	}
+
+	mutex_lock(&ar->conf_mutex);
+	ret = ath12k_mac_set_key(ar, cmd, vif, sta, key);
 	mutex_unlock(&ar->conf_mutex);
 	return ret;
 }
@@ -4473,11 +4502,22 @@  static int ath12k_mac_op_conf_tx(struct ieee80211_hw *hw,
 {
 	struct ath12k *ar;
 	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
+	struct ath12k_tx_conf *tx_conf;
 	int ret;
 
 	ar = ath12k_get_ar_by_vif(hw, vif);
 	if (!ar) {
-		/* TODO cache the info and apply after vdev is created */
+		/* cache the info and apply after vdev is created */
+		tx_conf = arvif->cache.tx_conf;
+		if (!tx_conf) {
+			tx_conf = kzalloc(sizeof(*tx_conf), GFP_KERNEL);
+			if (!tx_conf)
+				return -ENOSPC;
+			arvif->cache.tx_conf = tx_conf;
+		}
+		tx_conf->changed = true;
+		tx_conf->ac = ac;
+		tx_conf->tx_queue_params = *params;
 		return -EINVAL;
 	}
 
@@ -6121,6 +6161,57 @@  static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif)
 	return ret;
 }
 
+static void ath12k_mac_vif_cache_free(struct ath12k *ar, struct ath12k_vif *arvif)
+{
+	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
+	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	kfree(tx_conf);
+	arvif->cache.tx_conf = NULL;
+	kfree(key_conf);
+	arvif->cache.key_conf = NULL;
+	arvif->cache.bss_conf_changed = 0;
+}
+
+static void ath12k_mac_vif_cache_flush(struct ath12k *ar,  struct ieee80211_vif *vif)
+{
+	struct ath12k_vif *arvif = ath12k_vif_to_arvif(vif);
+	struct ath12k_key_conf *key_conf = arvif->cache.key_conf;
+	struct ath12k_tx_conf *tx_conf = arvif->cache.tx_conf;
+	struct ath12k_base *ab = ar->ab;
+
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (tx_conf && tx_conf->changed) {
+		ret = ath12k_mac_conf_tx(arvif, 0, tx_conf->ac,
+					 &tx_conf->tx_queue_params);
+		if (ret)
+			ath12k_warn(ab,
+				    "unable to apply tx config parameters to vdev %d\n",
+				    ret);
+	}
+
+	if (arvif->cache.bss_conf_changed) {
+		ath12k_mac_bss_info_changed(ar, arvif, &vif->bss_conf,
+					    arvif->cache.bss_conf_changed);
+	}
+
+	if (key_conf && key_conf->changed) {
+		ret = ath12k_mac_set_key(ar, key_conf->cmd, vif, NULL,
+					 key_conf->key);
+		if (ret) {
+			WARN_ON_ONCE(1);
+			ath12k_warn(ab, "unable to apply set key param to vdev %d ret %d\n",
+				    arvif->vdev_id, ret);
+		}
+	}
+	ath12k_mac_vif_cache_free(ar, arvif);
+}
+
 static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
 						    struct ieee80211_vif *vif,
 						    struct ieee80211_chanctx_conf *ctx)
@@ -6201,10 +6292,11 @@  static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
 		goto unlock;
 	}
 
-	/* TODO If the vdev is created during channel assign and not during
+	/* 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.
 	 */
+	ath12k_mac_vif_cache_flush(ar, vif);
 unlock:
 	mutex_unlock(&ar->conf_mutex);
 out:
@@ -6316,6 +6408,7 @@  static int ath12k_mac_vdev_delete(struct ath12k *ar, struct ieee80211_vif *vif)
 	spin_unlock_bh(&ar->data_lock);
 
 	ath12k_peer_cleanup(ar, arvif->vdev_id);
+	ath12k_mac_vif_cache_free(ar, arvif);
 
 	idr_for_each(&ar->txmgmt_idr,
 		     ath12k_mac_vif_txmgmt_idr_remove, vif);