On 2/11/2022 11:38 AM, Johannes Berg wrote:
Hi Arend,I stumbled upon locking in wiphy_apply_custom_regulatory() and the history of commits make it a bit unclear to me if these locks are all really needed.Oh how I like the regulatory code ... ;)
Thanks for getting in to this in such detail...
beee246951571 added the rcu assignments without any locking.Well that was just plain wrong, right? You have to have some kind of writer-side locking with RCU. I think this probably was assuming we'd be under some locks already? In regulatory_set_wiphy_regd() vs. regulatory_set_wiphy_regd_sync() we later formalized this.
Yeah. That is clear.
51d62f2f2c501 added the rtnl locking.That was the bugfix.
Correct. Just not sure I understand why is has to be RTNL lock.
a05829a7222e9 added the wiphy lock.Yes that's because now get_wiphy_regdom() can be called as documented: /* * Returns the regulatory domain associated with the wiphy. * * Requires any of RTNL, wiphy mutex or RCU protection. */
So would wiphy mutex be sufficient. I guess my question is what is protected by these lock in wiphy_apply_custom_regulatory() and is it really necessary to have both.
My understanding of RCU operations was that locking is only needed for readers,No, readers are lockless (just "rcu_read_lock()" which enters a critical section, but no locking).
Understood. My bad.
but here we use not one but two mutex locks which makes me wonder. Especially, as this function is called before registering the wiphy. Could you clarify?Like I said above, that's because we want to dereference the pointer in three different contexts: * RCU * RTNL * wiphy mutex Maybe we can get rid of some of those readers, but e.g. iwlwifi is using wiphy_dereference(mvm->hw->wiphy, mvm->hw->wiphy->regd); which really should use get_wiphy_regdom(), but I'm not sure is holding the RTNL in all paths? Looking at them, it seems like it is, so perhaps we can get rid of the wiphy mutex locking. Why are you asking? :)
Just some experimental coding where I ended up calling wiphy_apply_custom_regulatory() upon IFF_UP and hit deadlock because RTNL was already taken. Anyway, that code already ended up in the garbage bin, but wanted to ask anyway. Learning by asking (stupid) questions ;-)
Regards, Arend
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
