Re: [PATCH v4 01/12] wifi: ath12k: add multiple radio support in a single MAC HW un/register

From: Rameshkumar Sundaram <quic_ramess@quicinc.com>
To: Jeff Johnson <quic_jjohnson@quicinc.com>, <ath12k@lists.infradead.org>
Cc: <linux-wireless@vger.kernel.org>,
	Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
Subject: Re: [PATCH v4 01/12] wifi: ath12k: add multiple radio support in a single MAC HW un/register
Date: Wed, 13 Mar 2024 18:27:44 +0530	[thread overview]
Message-ID: <3178cf8c-d328-4fe2-b3be-f5c4c908f20f@quicinc.com> (raw)
In-Reply-To: <4a527729-2bf8-47ce-8275-d36b76bde95c@quicinc.com>


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