Received-SPF: pass (google.com: domain of linux-wireless+bounces-1839-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <bfdf62da-e2f3-4b59-971c-e10349e705f6@quicinc.com>
Date: Fri, 12 Jan 2024 10:25:29 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 10/12] wifi: ath12k: Modify set and get antenna mac ops
 for single wiphy
Content-Language: en-US
To: Sriram R <quic_srirrama@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>
References: <20240111045045.28377-1-quic_srirrama@quicinc.com>
 <20240111045045.28377-11-quic_srirrama@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240111045045.28377-11-quic_srirrama@quicinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit

On 1/10/2024 8:50 PM, Sriram R wrote:
> As multiple radios are combined into a single wiphy, and
> the current infrastructure supports only set/get antenna
> for the wiphy, the max Tx/Rx antenna capability is advertised
> during wiphy register.
> Hence, When antenna set/get is received we adjust the set/get
> based on max radio capability and set/get antenna accordingly.
> 
> Multi radio capability would get introduced with interface

Since this multi-radio capability is not part of the current series, I
don't think it is correct to say this "would get introduced". Rather, I
think it is more appropriate to say it "would need to be introduced in
the future" if you want per-radio configuration

> combination changes to support single wiphy model in cfg80211
> which would help extend the wiphy specific get/set configs similar
> to this to per hw level.
> 
> 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>
> ---
>  drivers/net/wireless/ath/ath12k/mac.c | 41 +++++++++++++++++++--------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
> index 2f19d6c5b741..00063c783e6c 100644
> --- a/drivers/net/wireless/ath/ath12k/mac.c
> +++ b/drivers/net/wireless/ath/ath12k/mac.c
> @@ -5064,6 +5064,13 @@ static int __ath12k_set_antenna(struct ath12k *ar, u32 tx_ant, u32 rx_ant)
>  	if (ath12k_check_chain_mask(ar, rx_ant, false))
>  		return -EINVAL;
>  
> +	/* Since we advertised the max cap of all radios combined during wiphy
> +	 * registration, ensure we don't set the antenna config higher than our
> +	 * limits

*specific radio* limits

> +	 */
> +	tx_ant = min_t(u32, tx_ant, ar->pdev->cap.tx_chain_mask);
> +	rx_ant = min_t(u32, rx_ant, ar->pdev->cap.rx_chain_mask);
> +
>  	ar->cfg_tx_chainmask = tx_ant;
>  	ar->cfg_rx_chainmask = rx_ant;
>  
> @@ -6380,21 +6387,25 @@ static void ath12k_mac_op_configure_filter(struct ieee80211_hw *hw,
>  static int ath12k_mac_op_get_antenna(struct ieee80211_hw *hw, u32 *tx_ant, u32 *rx_ant)
>  {
>  	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
> +	int antennas_rx = 0, antennas_tx = 0;
>  	struct ath12k *ar;
> +	int i;
>  
>  	mutex_lock(&ah->conf_mutex);
>  
>  	ar = ath12k_ah_to_ar(ah, 0);
>  
> -	mutex_lock(&ar->conf_mutex);
> -
> -	*tx_ant = ar->cfg_tx_chainmask;
> -	*rx_ant = ar->cfg_rx_chainmask;
> -
> -	mutex_unlock(&ar->conf_mutex);
> -
> +	for (i = 0; i < ah->num_radio; i++) {
> +		mutex_lock(&ar->conf_mutex);
> +		antennas_rx = max_t(u32, antennas_rx, ar->cfg_rx_chainmask);
> +		antennas_tx = max_t(u32, antennas_tx, ar->cfg_tx_chainmask);
> +		mutex_unlock(&ar->conf_mutex);
> +		ar++;
> +	}
>  	mutex_unlock(&ah->conf_mutex);
>  
> +	*tx_ant = antennas_rx;
> +	*rx_ant = antennas_tx;
>  	return 0;
>  }
>  
> @@ -6402,16 +6413,22 @@ static int ath12k_mac_op_set_antenna(struct ieee80211_hw *hw, u32 tx_ant, u32 rx
>  {
>  	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
>  	struct ath12k *ar;
> -	int ret;
> +	int ret = 0;
> +	int i;
>  
>  	mutex_lock(&ah->conf_mutex);
>  
>  	ar = ath12k_ah_to_ar(ah, 0);
>  
> -	mutex_lock(&ar->conf_mutex);
> -	ret = __ath12k_set_antenna(ar, tx_ant, rx_ant);
> -	mutex_unlock(&ar->conf_mutex);
> -
> +	for (i = 0; i < ah->num_radio; i++) {
> +		mutex_lock(&ar->conf_mutex);
> +		ret = __ath12k_set_antenna(ar, tx_ant, rx_ant);
> +		mutex_unlock(&ar->conf_mutex);
> +		if (ret)
> +			goto out;
> +		ar++;
> +	}
> +out:
>  	mutex_unlock(&ah->conf_mutex);
>  
>  	return ret;
Since I just have documentation nits:

Acked-by: Jeff Johnson <quic_jjohnson@quicinc.com>