Received-SPF: pass (google.com: domain of linux-wireless+bounces-1957-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <de5a4bbc-3533-7666-c079-9164ea98dd27@quicinc.com>
Date: Tue, 16 Jan 2024 10:15:52 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH 01/12] wifi: ath12k: add multiple radio support in a
 single MAC HW un/register
Content-Language: en-US
To: Jeff Johnson <quic_jjohnson@quicinc.com>,
        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-2-quic_srirrama@quicinc.com>
 <aee0419e-c443-4b5d-957c-6c88dd3d4a71@quicinc.com>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <aee0419e-c443-4b5d-957c-6c88dd3d4a71@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/12/2024 10:33 PM, Jeff Johnson wrote:
> 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.


Yes, interface_modes to be the modes that are supported on every radio 
not any radio.

Any radio support can be add in the future ath12k driver once the hw 
specific interface mode support is added in mac80211/cfg80211.


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


The condition check "if (!(interface_modes & mode))" become true when 
the the bit isn't set in the interface_modes. So no issues.


Thanks,

Karthikeyan