Received-SPF: pass (google.com: domain of linux-wireless+bounces-4674-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <3178cf8c-d328-4fe2-b3be-f5c4c908f20f@quicinc.com>
Date: Wed, 13 Mar 2024 18:27:44 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4 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>, <ath12k@lists.infradead.org>
CC: <linux-wireless@vger.kernel.org>,
        Karthikeyan Periyasamy
	<quic_periyasa@quicinc.com>
References: <20240312135557.1778379-1-quic_ramess@quicinc.com>
 <20240312135557.1778379-2-quic_ramess@quicinc.com>
 <4a527729-2bf8-47ce-8275-d36b76bde95c@quicinc.com>
From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
In-Reply-To: <4a527729-2bf8-47ce-8275-d36b76bde95c@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 3/13/2024 3:23 AM, Jeff Johnson wrote:
> On 3/12/2024 6:55 AM, Rameshkumar Sundaram wrote:
>> From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
>>
>> Currently MAC HW un/register helper function support the single radio.
>> To enable single/multi link operation in the future, the following helper
>> functions need to be refactored to accommodate multiple radios under a
>> single MAC HW un/register:
>>
>> 	* ath12k_ah_to_ar()
>> 	* ath12k_mac_hw_allocate()
>> 	* ath12k_mac_hw_register()
>> 	* ath12k_mac_hw_unregister()
>>
>> This refactoring will make it easier to scale these functionalities and
>> support Multi link operation.
>>
>>                           Current Multi wiphy Model
>>
>>   +---------------+          +---------------+            +---------------+
>>   |  Mac80211 hw  |          | Mac80211 hw   |            | Mac80211 hw   |
>>   |  private data |          | private data  |            | private data  |
>>   |               |          |               |            |               |
>>   |ath12k_hw (ah) |          |ath12k_hw (ah) |            |ath12k_hw (ah) |
>>   |               |          |               |            |               |
>>   | +-----------+ |          | +-----------+ |            | +-----------+ |
>>   | | ar (2GHz) | |          | | ar (5GHz) | |            | | ar (6GHz) | |
>>   | +-----------+ |          | +-----------+ |            | +-----------+ |
>>   |               |          |               |            |               |
>>   +---------------+          +---------------+            +---------------+
>>
>>                             Single wiphy Model
>>
>>                              +--------------+
>>                              | Mac80211 hw  |
>>                              | private data |
>>                              |              |
>>                              |ath12k hw (ah)|
>>                              | +----------+ |
>>                              | |ar (2GHz) | |
>>                              | +----------+ |
>>                              | |ar (5GHz) | |
>>                              | +----------+ |
>>                              | |ar (6GHz) | |
>>                              | +----------+ |
>>                              +--------------+
>>
>> 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: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
>> Signed-off-by: Sriram R <quic_srirrama@quicinc.com>
>> Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
>> ---
>>   drivers/net/wireless/ath/ath12k/core.h |  12 +-
>>   drivers/net/wireless/ath/ath12k/mac.c  | 184 ++++++++++++++++---------
>>   drivers/net/wireless/ath/ath12k/reg.c  |   2 +-
>>   3 files changed, 127 insertions(+), 71 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
>> index 97e5a0ccd233..ff831faa4945 100644
>> --- a/drivers/net/wireless/ath/ath12k/core.h
>> +++ b/drivers/net/wireless/ath/ath12k/core.h
>> @@ -951,13 +951,21 @@ static inline struct ath12k_hw *ath12k_hw_to_ah(struct ieee80211_hw  *hw)
>>   	return hw->priv;
>>   }
>>   
>> -static inline struct ath12k *ath12k_ah_to_ar(struct ath12k_hw *ah)
>> +static inline struct ath12k *ath12k_ah_to_ar(struct ath12k_hw *ah, u8 hw_link_id)
>>   {
>> -	return ah->radio;
>> +	if (WARN(hw_link_id >= ah->num_radio,
>> +		 "bad hw link id %d, so switch to default link\n", hw_link_id))
>> +		hw_link_id = 0;
>> +
>> +	return &ah->radio[hw_link_id];
>>   }
>>   
>>   static inline struct ieee80211_hw *ath12k_ar_to_hw(struct ath12k *ar)
>>   {
>>   	return ar->ah->hw;
>>   }
>> +
>> +#define for_each_ar(index, ah, ar) \
>> +	for ((index) = 0; ((index) < (ah)->num_radio && \
>> +	     ((ar) = &(ah)->radio[(index)])); (index)++)
> 
> 
> this seems like logically the wrong order of operands
> this is an operation on the ah object so IMO that should be first
> the actual iterators i and ar should follow that
> 
Sure Jeff, will rearrange the operands as suggested.
> and guess we have to figure out how to suppress the ath12k-check issues with
> this macro
ath12k-check complains about the reuse of ah and index arguments which 
may get evaluated multiple times if its an arithmetic expression, But 
areas where we use the macro in our code aren't doing so.
Do you have any suggestions here ? or shall we go back and use this 
for-loop inline.
> 
> that's my only comments on this patch.
> 
> /jeff