On 8/8/2024 4:27 PM, Kalle Valo wrote:
Kalle Valo <kvalo@xxxxxxxxxx> writes:
Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx> 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.
