Received-SPF: pass (google.com: domain of linux-wireless+bounces-1825-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <aee0419e-c443-4b5d-957c-6c88dd3d4a71@quicinc.com>
Date: Fri, 12 Jan 2024 09:03:01 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 01/12] wifi: ath12k: add multiple radio support in a
 single MAC HW un/register
Content-Language: en-US
To: Sriram R <quic_srirrama@quicinc.com>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Karthikeyan Periyasamy
	<quic_periyasa@quicinc.com>
References: <20240111045045.28377-1-quic_srirrama@quicinc.com>
 <20240111045045.28377-2-quic_srirrama@quicinc.com>
From: Jeff Johnson <quic_jjohnson@quicinc.com>
In-Reply-To: <20240111045045.28377-2-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:
...

> @@ -7553,10 +7553,12 @@ static int ath12k_mac_setup_channels_rates(struct ath12k *ar,
>  
>  static u16 ath12k_mac_get_ifmodes(struct ath12k_hw *ah)
>  {
> -	struct ath12k *ar = ath12k_ah_to_ar(ah);
> +	struct ath12k *ar;
> +	int i;
>  	u16 interface_modes = U16_MAX;
>  
> -	interface_modes &= ar->ab->hw_params->interface_modes;
> +	for_each_ar(i, ah, ar)
> +		interface_modes &= ar->ab->hw_params->interface_modes;

This logic sets interface_modes to be the modes that are supported on
every radio (starts with all modes supported and then masks off the ones
that are not supported on a given radio). That means that if radio 0
supports a mode that radio 1 does not support, then we don't advertise
support for that mode. Just want to verify that is the correct logic,
and that instead we don't want to start with no modes supported and then
OR in the ones that are supported so that we get the list of modes that
are supported on ANY radio instead of ALL radios.

>  
>  	return interface_modes == U16_MAX ? 0 : interface_modes;
>  }
> @@ -7564,15 +7566,19 @@ static u16 ath12k_mac_get_ifmodes(struct ath12k_hw *ah)
>  static bool ath12k_mac_is_iface_mode_enable(struct ath12k_hw *ah,
>  					    enum nl80211_iftype type)
>  {
> -	struct ath12k *ar = ath12k_ah_to_ar(ah);
> +	struct ath12k *ar;
> +	int i;
>  	u16 interface_modes, mode;
>  	bool is_enable = true;
>  
>  	mode = BIT(type);
> -
> -	interface_modes = ar->ab->hw_params->interface_modes;
> -	if (!(interface_modes & mode))
> -		is_enable = false;
> +	for_each_ar(i, ah, ar) {
> +		interface_modes = ar->ab->hw_params->interface_modes;
> +		if (!(interface_modes & mode)) {
> +			is_enable = false;

note that in the example I mentioned above, this logic could say a given
mode is enabled even though the bit isn't set in the interface_modes
listed above

> +			break;
> +		}
> +	}
>  
>  	return is_enable;