[PATCH 0/2] wireless: fix 11d lockdep warning

[PATCH 0/2] wireless: fix 11d lockdep warning

[Luis R. Rodriguez]

This fixes the lockdep warning for me. We take a much simpler
approach than that mess I was doing before. cfg80211 stuff will
go later.

Luis R. Rodriguez (2):
  cfg80211: use goto out on 11d reg hint failure
  cfg80211: decouple regulatory variables from cfg80211_mutex

 net/wireless/core.c |    4 +---
 net/wireless/reg.c  |   32 +++++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 12 deletions(-)

[PATCH 1/2] cfg80211: use goto out on 11d reg hint failure

[Luis R. Rodriguez]

This does not change functionality.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index fb40428..2d1d183 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1774,10 +1774,8 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 
 	mutex_lock(&cfg80211_mutex);
 
-	if (unlikely(!last_request)) {
-		mutex_unlock(&cfg80211_mutex);
-		return;
-	}
+	if (unlikely(!last_request))
+		goto out;
 
 	/* IE len must be evenly divisible by 2 */
 	if (country_ie_len & 0x01)
-- 
1.6.0.4

[PATCH 2/2] cfg80211: decouple regulatory variables from cfg80211_mutex

[Luis R. Rodriguez]

We change regulatory code to be protected by its own regulatory
spinlock and alleviate cfg80211_mutex to only be used to protect
cfg80211_rdev_list, the registered device list.

By doing this we will be able to work on regulatory core components
without having to have hog up the cfg80211_mutex. An example here is
we no longer need to use the cfg80211_mutex during the country IE
regulatory hint; by doing so we end up curing this new lockdep warning:

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.31-rc4-wl #12
-------------------------------------------------------
phy1/1709 is trying to acquire lock:
 (cfg80211_mutex){+.+.+.}, at: [<ffffffffa00af852>] regulatory_hint_11d+0x32/0x3f0 [cfg80211]

but task is already holding lock:
 (&ifmgd->mtx){+.+.+.}, at: [<ffffffffa0144228>] ieee80211_sta_work+0x108/0x10f0 [mac80211]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #3 (&ifmgd->mtx){+.+.+.}:
       [<ffffffff810857b6>] __lock_acquire+0xd76/0x12b0
       [<ffffffff81085dd3>] lock_acquire+0xe3/0x120
       [<ffffffff814eeae4>] mutex_lock_nested+0x44/0x350
       [<ffffffffa0141bb8>] ieee80211_mgd_auth+0x108/0x1f0 [mac80211]
       [<ffffffffa0148563>] ieee80211_auth+0x13/0x20 [mac80211]
       [<ffffffffa00bc3a1>] __cfg80211_mlme_auth+0x1b1/0x2a0 [cfg80211]
       [<ffffffffa00bc516>] cfg80211_mlme_auth+0x86/0xc0 [cfg80211]
       [<ffffffffa00b368d>] nl80211_authenticate+0x21d/0x230 [cfg80211]
       [<ffffffff81416ba6>] genl_rcv_msg+0x1b6/0x1f0
       [<ffffffff81415c39>] netlink_rcv_skb+0x89/0xb0
       [<ffffffff814169d9>] genl_rcv+0x29/0x40
       [<ffffffff8141591d>] netlink_unicast+0x29d/0x2b0
       [<ffffffff81416514>] netlink_sendmsg+0x214/0x300
       [<ffffffff813e4407>] sock_sendmsg+0x107/0x130
       [<ffffffff813e45b9>] sys_sendmsg+0x189/0x320
       [<ffffffff81011f82>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #2 (&wdev->mtx){+.+.+.}:
       [<ffffffff810857b6>] __lock_acquire+0xd76/0x12b0
       [<ffffffff81085dd3>] lock_acquire+0xe3/0x120
       [<ffffffff814eeae4>] mutex_lock_nested+0x44/0x350
       [<ffffffffa00ab304>] cfg80211_netdev_notifier_call+0x1a4/0x390 [cfg80211]
       [<ffffffff814f3dff>] notifier_call_chain+0x3f/0x80
       [<ffffffff81075a91>] raw_notifier_call_chain+0x11/0x20
       [<ffffffff813f665a>] dev_open+0x10a/0x120
       [<ffffffff813f59bd>] dev_change_flags+0x9d/0x1e0
       [<ffffffff8144eb6e>] devinet_ioctl+0x6fe/0x760
       [<ffffffff81450204>] inet_ioctl+0x94/0xc0
       [<ffffffff813e25fa>] sock_ioctl+0x6a/0x290
       [<ffffffff8111e911>] vfs_ioctl+0x31/0xa0
       [<ffffffff8111ea9a>] do_vfs_ioctl+0x8a/0x5c0
       [<ffffffff8111f069>] sys_ioctl+0x99/0xa0
       [<ffffffff81011f82>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

-> #1 (&rdev->mtx){+.+.+.}:
       [<ffffffff810857b6>] __lock_acquire+0xd76/0x12b0
       [<ffffffff81085dd3>] lock_acquire+0xe3/0x120
       [<ffffffff814eeae4>] mutex_lock_nested+0x44/0x350
       [<ffffffffa00ac4d0>] cfg80211_get_dev_from_ifindex+0x60/0x90 [cfg80211]
       [<ffffffffa00b21ff>] get_rdev_dev_by_info_ifindex+0x6f/0xa0 [cfg80211]
       [<ffffffffa00b51eb>] nl80211_set_interface+0x3b/0x260 [cfg80211]
       [<ffffffff81416ba6>] genl_rcv_msg+0x1b6/0x1f0
       [<ffffffff81415c39>] netlink_rcv_skb+0x89/0xb0
       [<ffffffff814169d9>] genl_rcv+0x29/0x40
       [<ffffffff8141591d>] netlink_unicast+0x29d/0x2b0
       [<ffffffff81416514>] netlink_sendmsg+0x214/0x300
       [<ffffffff813e4407>] sock_sendmsg+0x107/0x130
       [<ffffffff813e45b9>] sys_sendmsg+0x189/0x320
       [<ffffffff81011f82>] system_call_fastpath+0x16/0x1b
       [<ffffffffffffffff>] 0xffffffffffffffff

other info that might help us debug this:

3 locks held by phy1/1709:
 #0:  ((wiphy_name(local->hw.wiphy))){+.+.+.}, at: [<ffffffff8106b45d>] worker_thread+0x19d/0x340
 #1:  (&ifmgd->work){+.+.+.}, at: [<ffffffff8106b45d>] worker_thread+0x19d/0x340
 #2:  (&ifmgd->mtx){+.+.+.}, at: [<ffffffffa0144228>] ieee80211_sta_work+0x108/0x10f0 [mac80211]

Reported-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/core.c |    4 +---
 net/wireless/reg.c  |   26 +++++++++++++++++++++-----
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 755cdf1..d8dbd7f 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -34,9 +34,7 @@ MODULE_DESCRIPTION("wireless configuration support");
 LIST_HEAD(cfg80211_rdev_list);
 
 /*
- * This is used to protect the cfg80211_rdev_list, cfg80211_regdomain,
- * country_ie_regdomain, the reg_beacon_list and the the last regulatory
- * request receipt (last_request).
+ * This is used to protect the cfg80211_rdev_list
  */
 DEFINE_MUTEX(cfg80211_mutex);
 
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 2d1d183..41c16fc 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -62,6 +62,15 @@ const struct ieee80211_regdomain *cfg80211_regdomain;
  */
 static const struct ieee80211_regdomain *country_ie_regdomain;
 
+/*
+ * Protects static reg.c components:
+ *     - cfg80211_world_regdom
+ *     - cfg80211_regdom
+ *     - country_ie_regdomain
+ *     - last_request
+ */
+static spinlock_t reg_lock;
+
 /* Used to queue up regulatory hints */
 static LIST_HEAD(reg_requests_list);
 static spinlock_t reg_requests_lock;
@@ -1570,6 +1579,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
 	BUG_ON(!reg_request->alpha2);
 
 	mutex_lock(&cfg80211_mutex);
+	spin_lock(&reg_lock);
 
 	if (wiphy_idx_valid(reg_request->wiphy_idx))
 		wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
@@ -1585,6 +1595,7 @@ static void reg_process_hint(struct regulatory_request *reg_request)
 	if (r == -EALREADY && wiphy && wiphy->strict_regulatory)
 		wiphy_update_regulatory(wiphy, reg_request->initiator);
 out:
+	spin_unlock(&reg_lock);
 	mutex_unlock(&cfg80211_mutex);
 }
 
@@ -1734,13 +1745,12 @@ int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
 }
 EXPORT_SYMBOL(regulatory_hint);
 
+/* Caller must hold reg_lock */
 static bool reg_same_country_ie_hint(struct wiphy *wiphy,
 			u32 country_ie_checksum)
 {
 	struct wiphy *request_wiphy;
 
-	assert_cfg80211_lock();
-
 	if (unlikely(last_request->initiator !=
 	    NL80211_REGDOM_SET_BY_COUNTRY_IE))
 		return false;
@@ -1772,7 +1782,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	enum environment_cap env = ENVIRON_ANY;
 	struct regulatory_request *request;
 
-	mutex_lock(&cfg80211_mutex);
+	spin_lock(&reg_lock);
 
 	if (unlikely(!last_request))
 		goto out;
@@ -1883,7 +1893,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	request->country_ie_checksum = checksum;
 	request->country_ie_env = env;
 
-	mutex_unlock(&cfg80211_mutex);
+	spin_unlock(&reg_lock);
 
 	queue_regulatory_request(request);
 
@@ -1892,7 +1902,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 free_rd_out:
 	kfree(rd);
 out:
-	mutex_unlock(&cfg80211_mutex);
+	spin_unlock(&reg_lock);
 }
 EXPORT_SYMBOL(regulatory_hint_11d);
 
@@ -2225,10 +2235,13 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 
 	assert_cfg80211_lock();
 
+	spin_lock(&reg_lock);
+
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
 	if (r) {
 		kfree(rd);
+		spin_unlock(&reg_lock);
 		return r;
 	}
 
@@ -2243,6 +2256,8 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 
 	nl80211_send_reg_change_event(last_request);
 
+	spin_unlock(&reg_lock);
+
 	return r;
 }
 
@@ -2275,6 +2290,7 @@ int regulatory_init(void)
 
 	spin_lock_init(&reg_requests_lock);
 	spin_lock_init(&reg_pending_beacons_lock);
+	spin_lock_init(&reg_lock);
 
 #ifdef CONFIG_WIRELESS_OLD_REGULATORY
 	cfg80211_regdomain = static_regdom(ieee80211_regdom);
-- 
1.6.0.4

Re: [PATCH 2/2] cfg80211: decouple regulatory variables from cfg80211_mutex

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 684 bytes --]

On Wed, 2009-07-29 at 17:46 -0700, Luis R. Rodriguez wrote:
> We change regulatory code to be protected by its own regulatory
> spinlock and alleviate cfg80211_mutex to only be used to protect
> cfg80211_rdev_list, the registered device list.
> 
> By doing this we will be able to work on regulatory core components
> without having to have hog up the cfg80211_mutex. An example here is
> we no longer need to use the cfg80211_mutex during the country IE
> regulatory hint; by doing so we end up curing this new lockdep warning:

This is ok with me, but be sure that it changes the API between drivers
and cfg80211 -- the reg notifier can no longer sleep now.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] cfg80211: decouple regulatory variables from cfg80211_mutex

[Luis R. Rodriguez]

On Thu, Jul 30, 2009 at 12:43 AM, Johannes
Berg<johannes@sipsolutions.net> wrote:
> On Wed, 2009-07-29 at 17:46 -0700, Luis R. Rodriguez wrote:
>> We change regulatory code to be protected by its own regulatory
>> spinlock and alleviate cfg80211_mutex to only be used to protect
>> cfg80211_rdev_list, the registered device list.
>>
>> By doing this we will be able to work on regulatory core components
>> without having to have hog up the cfg80211_mutex. An example here is
>> we no longer need to use the cfg80211_mutex during the country IE
>> regulatory hint; by doing so we end up curing this new lockdep warning:
>
> This is ok with me, but be sure that it changes the API between drivers
> and cfg80211 -- the reg notifier can no longer sleep now.

We could use a mutex as well.

  Luis

Re: [PATCH 2/2] cfg80211: decouple regulatory variables from cfg80211_mutex

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]

On Thu, 2009-07-30 at 07:54 -0700, Luis R. Rodriguez wrote:
> On Thu, Jul 30, 2009 at 12:43 AM, Johannes
> Berg<johannes@sipsolutions.net> wrote:
> > On Wed, 2009-07-29 at 17:46 -0700, Luis R. Rodriguez wrote:
> >> We change regulatory code to be protected by its own regulatory
> >> spinlock and alleviate cfg80211_mutex to only be used to protect
> >> cfg80211_rdev_list, the registered device list.
> >>
> >> By doing this we will be able to work on regulatory core components
> >> without having to have hog up the cfg80211_mutex. An example here is
> >> we no longer need to use the cfg80211_mutex during the country IE
> >> regulatory hint; by doing so we end up curing this new lockdep warning:
> >
> > This is ok with me, but be sure that it changes the API between drivers
> > and cfg80211 -- the reg notifier can no longer sleep now.
> 
> We could use a mutex as well.

Right. I don't care, just wanted you to be aware of the change. I don't
expect many drivers to really use the reg notifier stuff so if it
doesn't matter for ath.ko it probably just doesn't matter.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 2/2] cfg80211: decouple regulatory variables from cfg80211_mutex

[Luis R. Rodriguez]

On Thu, Jul 30, 2009 at 8:32 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Thu, 2009-07-30 at 07:54 -0700, Luis R. Rodriguez wrote:
>> On Thu, Jul 30, 2009 at 12:43 AM, Johannes
>> Berg<johannes@sipsolutions.net> wrote:
>> > On Wed, 2009-07-29 at 17:46 -0700, Luis R. Rodriguez wrote:
>> >> We change regulatory code to be protected by its own regulatory
>> >> spinlock and alleviate cfg80211_mutex to only be used to protect
>> >> cfg80211_rdev_list, the registered device list.
>> >>
>> >> By doing this we will be able to work on regulatory core components
>> >> without having to have hog up the cfg80211_mutex. An example here is
>> >> we no longer need to use the cfg80211_mutex during the country IE
>> >> regulatory hint; by doing so we end up curing this new lockdep warning:
>> >
>> > This is ok with me, but be sure that it changes the API between drivers
>> > and cfg80211 -- the reg notifier can no longer sleep now.
>>
>> We could use a mutex as well.
>
> Right. I don't care, just wanted you to be aware of the change. I don't
> expect many drivers to really use the reg notifier stuff so if it
> doesn't matter for ath.ko it probably just doesn't matter.

Yeah understood, I'll still respin with a mutex.

  Luis