Received-SPF: pass (google.com: domain of linux-wireless+bounces-1960-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <839b56fa-9ce8-47f3-768d-b4f32a2041f1@quicinc.com>
Date: Tue, 16 Jan 2024 10:44:46 +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 v2 2/2] wifi: ath12k: Introduce the container for mac80211
 hw
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240112024214.3481840-1-quic_periyasa@quicinc.com>
 <20240112024214.3481840-3-quic_periyasa@quicinc.com>
 <87bk9m7f5g.fsf@kernel.org>
From: Karthikeyan Periyasamy <quic_periyasa@quicinc.com>
In-Reply-To: <87bk9m7f5g.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


On 1/15/2024 9:38 PM, Kalle Valo wrote:
> Karthikeyan Periyasamy <quic_periyasa@quicinc.com> writes:
>
>> To support multi link operation, we need to combine all the link/pdev
>> under a single wiphy. This avoids the overhead of synchronization
>> across multiple hardware instances in both the cfg80211 and mac80211
>> layers. Currently, each link/pdev is registered as separate wiphy,
>> tightly coupled with link/pdev/radio (ar) structure. To enable single
>> wiphy registration within the chip, we decouple the wiphy data entity from
>> the link/pdev/radio (ar) structure and move it under the chip (ab)
>> structure with a new data container (ath12k_hw) structure. This approach
>> improves scalability for future multi link operation support.
> What about struct ath12k_pdev? Do we need it still or should it be removed?


ath12k_pdev still holds the pdev related information within the SoC/chip 
(ab). I dont want it be remove.


>
>>   static void ath12k_mac_op_cancel_hw_scan(struct ieee80211_hw *hw,
>>   					 struct ieee80211_vif *vif)
>>   {
>> -	struct ath12k *ar = hw->priv;
>> +	struct ath12k_hw *ah = ath12k_hw_to_ah(hw);
>> +	struct ath12k *ar;
>> +
>> +	mutex_lock(&ah->conf_mutex);
>> +
>> +	ar = ath12k_ah_to_ar(ah);
>>   
>>   	mutex_lock(&ar->conf_mutex);
>>   	ath12k_scan_abort(ar);
>>   	mutex_unlock(&ar->conf_mutex);
>>   
>> +	mutex_unlock(&ah->conf_mutex);
>> +
>>   	cancel_delayed_work_sync(&ar->scan.timeout);
>>   }
> Do we really need two mutexes? I don't see any analysis about that. And
> even if we do, I feel that it should be added in a separate patch.


Yes, ah->conf_mutex protect the concurrent mac80211 operation. But there 
is other places like radio/link specific synchronous operation (ie MGMT 
tx wait for the vdev deletion) is needed. To fulfill this need, we also 
need radio/link specific (ar) mutex instead of all link (ah) mutex for 
efficient lock/unlock.


Thanks,

Karthikeyan