[PATCH v6 1/3] wifi: ath12k: prepare vif data structure for MLO handling

[Rameshkumar Sundaram]

On 8/8/2024 4:27 PM, Kalle Valo wrote:
> Kalle Valo <kvalo at kernel.org> writes:
> 
>> Rameshkumar Sundaram <quic_ramess at quicinc.com> writes:
>>
>>> Locking:
>>>   Currently modifications to members of arvif and arsta are protected by ar->conf_mutex
>>>   and it stays as such.
>>>   Now with these hw level structure (ahvif) being introduced, any modifications
>>>   to its members and link objects (i.e., arvifs[] which are dynamically allocated)
>>>   needs to be protected for writing and ah->conf_mutex is used for the same.
>>>   Also, atomic contexts(say WMI events and certain mac_ops) that we currently have in driver
>>>   will not(shouldn't be allowed) do any modifications but can read them and
>>>   rcu_read_lock() is used for the same.
>>
>> Please elaborate more about your locking design. Because of past bad
>> contributions from Qualcomm the bar is really high for adding any new
>> locks. I'm doing the locking analysis right now but it would help a lot
>> if you could provide your own analysis.
>>
The new ah->conf_mutex is particularly introduced to protect the members 
and dynamically allocated link objects of ahvif and ahsta 
(ahvif/sta->links[]) in process context (i.e. between call backs from 
mac80211 and ath12k's workers)
The same is protected by rcu in case of atomic contexts(tasklets of WMI 
and in datapath)

The usage ar->conf_mutex remains as such, and replacing ar->conf_mutex 
with ah->conf_mutex was not considered, as few of the workers(e.g, 
ath12k_sta_rc_update_wk) rely only on ar lock and wouldn't really need 
to wait for other ar's to complete the same task.

>> My first impressions:
>>
>> It's really confusing to have two locks with the same name (conf_mutex
>> in struct ath12k_hw and struct ath12k).
>>
Makes sense we could rename the new lock to distinguish.
>> struct ath12k_hw already has hw_mutex so I'm even more suspicious about
>> this locking design. That's not explained at all in commit messages.
> 
Ah yes, by the time i wrote initial version of the patch hw_mutex wasn't 
there so missed it, Seems we can use the same lock for this too, will 
check the feasibility further and update/send v7.

> I didn't get any replies, and my own analysis is still ongoing,
Sorry, was off for last couple of days and didn't get a chance to check 
this out.
  but the
> more I look at this, the more I feel using two overlapping mutexes is
> overkill. I'm starting to wonder if we would convert to using
> wiphy_lock()? That might simplify things significantly. I should have an
> old patchset doing that stored somewhere.
> 
Hmm, i remember usage of wiphy lock globally had lockup situations in 
ath12k last time around, if we could think of a solution for that i 
guess we could try to replace ah->conf_mutex with combination for 
wiphy_lock and rcu.