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

[Rameshkumar Sundaram]

On 8/9/2024 7:59 PM, Kalle Valo wrote:
> Rameshkumar Sundaram <quic_ramess at quicinc.com> writes:
> 
>> 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)
> 
> 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.
>