On 8/9/2024 7:59 PM, Kalle Valo wrote:
Rameshkumar Sundaram <quic_ramess@xxxxxxxxxxx> writes:
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)
I need more info than that. I can't understand which conf_mutex protects
what data exactly, currently it just looks random to me.
Let's take an example:
static void ath12k_mac_op_bss_info_changed(struct ieee80211_hw *hw,
...
mutex_lock(&ah->conf_mutex);
arvif = &ahvif->deflink;
Note that currently only deflink is referred here, but when MLO is
enabled, info->link_id will be used to de-refer corresponding link
i.e, arvif = rcu_dereference(ahvif->link[info->link_id]);
ar = ath12k_get_ar_by_vif(hw, vif > if (!ar) {
cache = ath12k_arvif_get_cache(arvif);
...
mutex_lock(&ar->conf_mutex);
ath12k_mac_bss_info_changed(ar, arvif, info, changed);
So first mac80211 calls ath12k_mac_op_bss_info_changed() with wiphy
mutex held. Then ath12k takes ah->conf_mutex and soon after also
ah->conf_mutex is taken since link object is referenced, and yes
currently only deflink is referred, but once MLO is enabled,
info->link_id from mac80211 is used to refer the corresponding link[] arvif.
ar->conf_mutex.
ar conf_mutex is existing lock which is taken for synchronization of
configuration between this mac80211 callback and workers of the same ar
(say ath12k_sta_rc_update_wk()) since this config is applied on an arvif
belonging to that ar.
So we are basically holding three locks and it's not
clear for me the difference between ah and ar mutexes. For example, do
ath12k_get_ar_by_vif() & ath12k_arvif_get_cache() require ah->conf_mutex
to be held? Or why are we taking it here?
No, above two functions doesn't really need ah->conf_mutex at this
point, but ath12k_arvif_get_cache() is being modified for MLO (check
[v2] wifi: ath12k: prepare vif config caching for MLO) where caching is
done in ahvif and there it will be needed.
I guess ahvif->deflink access does not require any protection because in
ath12k_mac_op_tx() we access ahvif->deflink without any protection:
struct ath12k_link_vif *arvif = &ahvif->deflink;
ath12k_mac_op_tx() will always be in atomic context as mac80211 ensures
it to be under rcu_read_lock() hence referring and reading link objects
of ahvif is safe, but cannot modify it.
Anyway, I just could not understand this locking design and besides it
just looks uncessarily complex. I propose dropping the new conf_mutex in
this patchset altogether and handle the locking in a separate patchset
later on.
AFAICS removing ah->conf_mutex from this patchset should be safe as
mac80211 is holding the wiphy mutex already. Of course I might have
missed something but at least that's what it looks like.
You're right wiphy/sdata lock is held on all callbacks from mac80211 and
synchronized naturally, but these callbacks running along with workers
which will be referring link objects of ahvif (e.g,
ath12k_mgmt_over_wmi_tx_work() referring deflink as of now will use
skb_cb->link_id to fetch corresponding link arvif) will still need some
sort of synchronization.
