Re: [PATCH v4 07/12] wifi: ath12k: Cache vdev configs before vdev create

[Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx>]

On 3/13/2024 4:31 AM, Jeff Johnson wrote:
> On 3/12/2024 6:55 AM, Rameshkumar Sundaram wrote:
> > From: Sriram R <quic_srirrama@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Signed-off-by: Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx>
> > ---
> >   drivers/net/wireless/ath/ath12k/core.h |  19 ++++
> >   drivers/net/wireless/ath/ath12k/mac.c  | 116 +++++++++++++++++++------
> >   2 files changed, 107 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
> > index 70daec38d486..fe151aa90687 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;
>
> is there a reason this isn't a *cache?
> this allocation is only needed for the time between when a vdev is created and
> when a channel is assigned, so why not have a dynamic allocation that is only
> around for that time? otherwise for every vdev you waste this memory for the
> lifetime of the vdev.
There is no specific reason behind it.
Your point makes sense, we need not have this for lifetime of the vdev. 
Shall we have cache as such and have *tx_conf and *key_conf as 
dynamically allocated members ?
That way we can allocate them whenever corresponding config is received 
and have it in cache.
> >   };
> >   
> >   struct ath12k_vif_iter {
> > diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> > index 6d2176b0a556..fab92f08fdb7 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;
>
> why don't you need to save the actual *info as well?
info(ieee80211_bss_conf) can be obtained from arvif->vif(ieee80211_vif) 
whenever needed.
> >   		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,43 @@ 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 *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;
> > +		}
> > +		arvif->cache.key_conf.cmd = cmd;
> > +		arvif->cache.key_conf.key = key;
> > +		arvif->cache.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;
> >   }
> > @@ -4477,7 +4498,10 @@ static int ath12k_mac_op_conf_tx(struct ieee80211_hw *hw,
> >   
> >   	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 */
> > +		arvif->cache.tx_conf.changed = true;
> > +		arvif->cache.tx_conf.ac = ac;
> > +		arvif->cache.tx_conf.tx_queue_params = *params;
> >   		return -EINVAL;
> >   	}
> >   
> > @@ -6121,6 +6145,41 @@ static int ath12k_mac_vdev_create(struct ath12k *ar, struct ieee80211_vif *vif)
> >   	return ret;
> >   }
> >   
> > +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_base *ab = ar->ab;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&ar->conf_mutex);
> > +
> > +	if (arvif->cache.tx_conf.changed) {
> > +		ret = ath12k_mac_conf_tx(arvif, 0, arvif->cache.tx_conf.ac,
> > +					 &arvif->cache.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 (arvif->cache.key_conf.changed) {
> > +		ret = ath12k_mac_set_key(ar, arvif->cache.key_conf.cmd,
> > +					 vif, NULL,
> > +					 arvif->cache.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);
> > +		}
> > +	}
> > +
> > +	memset(&arvif->cache, 0, sizeof(struct ath12k_vif_cache));
>
> sizeof(arvif->cache)
>
> > +}
> > +
> >   static struct ath12k *ath12k_mac_assign_vif_to_vdev(struct ieee80211_hw *hw,
> >   						    struct ieee80211_vif *vif,
> >   						    struct ieee80211_chanctx_conf *ctx)
> > @@ -6203,10 +6262,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: