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;
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
