Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a3d73398fa51b66e77d98cf1e883c72b66d6a3f4.camel@sipsolutions.net>
Subject: Re: locking in wiphy_apply_custom_regulatory()
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Arend van Spriel <arend.vanspriel@broadcom.com>
Cc:     linux-wireless <linux-wireless@vger.kernel.org>
Date:   Fri, 11 Feb 2022 11:38:26 +0100
In-Reply-To: <6562d8c3-27f6-490a-7732-6c300eb3aa64@broadcom.com>
References: <6562d8c3-27f6-490a-7732-6c300eb3aa64@broadcom.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.3 (3.42.3-1.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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

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

> 51d62f2f2c501 added the rtnl locking.

That was the bugfix.

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


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

>  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? :)

johannes