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

[Kalle Valo <kvalo@kernel.org>]

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.
>
> My first impressions:
>
> It's really confusing to have two locks with the same name (conf_mutex
> in struct ath12k_hw and struct ath12k).
>
> 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.

I didn't get any replies, and my own analysis is still ongoing, 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.

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

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