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

[Kalle Valo <kvalo@kernel.org>]

Rameshkumar Sundaram <quic_ramess@quicinc.com> writes:

> On 8/8/2024 4:27 PM, Kalle Valo wrote:
>> Kalle Valo <kvalo@kernel.org> writes:
>> 
>>> Rameshkumar Sundaram <quic_ramess@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;
	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
ar->conf_mutex. 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?

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;

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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches