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