Received-SPF: pass (google.com: domain of linux-wireless+bounces-5735-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <c5150881-7707-4b8f-b877-e5677039b0ed@quicinc.com>
Date: Mon, 1 Apr 2024 12:26:54 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v6 07/12] wifi: ath12k: Cache vdev configs before vdev
 create
Content-Language: en-US
To: Rameshkumar Sundaram <quic_ramess@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>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240401190409.2461819-8-quic_ramess@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

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

> +		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;