[PATCH 0/6] cfg80211: adds a workqueue for regulatory hints

[PATCH 0/6] cfg80211: adds a workqueue for regulatory hints

[Luis R. Rodriguez]

This series does prep work and then the actual work to move the
cfg80211 driver and userspace regulatory hints onto a workqueue.
It should cure some lockdep warnings and also pave the way for
more future reg work like beacon reg hints.

Luis R. Rodriguez (6):
  cfg80211: rename cfg80211_registered_device's idx to wiphy_idx
  cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity
  cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex
  cfg80211: add assert_cfg80211_lock() to ensure proper protection
  cfg80211: make regulatory_request use wiphy_idx instead of wiphy
  cfg80211: move regulatory hints to workqueue

 drivers/net/wireless/ath9k/main.c      |    8 +-
 drivers/net/wireless/zd1211rw/zd_mac.c |    6 +-
 include/net/cfg80211.h                 |    8 +-
 include/net/wireless.h                 |    9 +-
 net/wireless/core.c                    |   87 ++++++++-----
 net/wireless/core.h                    |   42 +++++-
 net/wireless/nl80211.c                 |   33 ++---
 net/wireless/reg.c                     |  240 ++++++++++++++++++++++++++------
 net/wireless/reg.h                     |    2 +
 net/wireless/sysfs.c                   |    2 +-
 10 files changed, 332 insertions(+), 105 deletions(-)

[PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx

[Luis R. Rodriguez]

Makes it clearer to read when comparing to ifidx

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/core.c    |   35 ++++++++++++++++++-----------------
 net/wireless/core.h    |    2 +-
 net/wireless/nl80211.c |    4 ++--
 net/wireless/sysfs.c   |    2 +-
 4 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 0668b2b..2b3e786 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -37,12 +37,13 @@ DEFINE_MUTEX(cfg80211_drv_mutex);
 static struct dentry *ieee80211_debugfs_dir;
 
 /* requires cfg80211_drv_mutex to be held! */
-static struct cfg80211_registered_device *cfg80211_drv_by_wiphy(int wiphy)
+static struct cfg80211_registered_device *
+cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 {
 	struct cfg80211_registered_device *result = NULL, *drv;
 
 	list_for_each_entry(drv, &cfg80211_drv_list, list) {
-		if (drv->idx == wiphy) {
+		if (drv->wiphy_idx == wiphy_idx) {
 			result = drv;
 			break;
 		}
@@ -56,12 +57,12 @@ static struct cfg80211_registered_device *
 __cfg80211_drv_from_info(struct genl_info *info)
 {
 	int ifindex;
-	struct cfg80211_registered_device *bywiphy = NULL, *byifidx = NULL;
+	struct cfg80211_registered_device *bywiphyidx = NULL, *byifidx = NULL;
 	struct net_device *dev;
 	int err = -EINVAL;
 
 	if (info->attrs[NL80211_ATTR_WIPHY]) {
-		bywiphy = cfg80211_drv_by_wiphy(
+		bywiphyidx = cfg80211_drv_by_wiphy_idx(
 				nla_get_u32(info->attrs[NL80211_ATTR_WIPHY]));
 		err = -ENODEV;
 	}
@@ -78,14 +79,14 @@ __cfg80211_drv_from_info(struct genl_info *info)
 		err = -ENODEV;
 	}
 
-	if (bywiphy && byifidx) {
-		if (bywiphy != byifidx)
+	if (bywiphyidx && byifidx) {
+		if (bywiphyidx != byifidx)
 			return ERR_PTR(-EINVAL);
 		else
-			return bywiphy; /* == byifidx */
+			return bywiphyidx; /* == byifidx */
 	}
-	if (bywiphy)
-		return bywiphy;
+	if (bywiphyidx)
+		return bywiphyidx;
 
 	if (byifidx)
 		return byifidx;
@@ -143,16 +144,16 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 			char *newname)
 {
 	struct cfg80211_registered_device *drv;
-	int idx, taken = -1, result, digits;
+	int wiphy_idx, taken = -1, result, digits;
 
 	mutex_lock(&cfg80211_drv_mutex);
 
 	/* prohibit calling the thing phy%d when %d is not its number */
-	sscanf(newname, PHY_NAME "%d%n", &idx, &taken);
-	if (taken == strlen(newname) && idx != rdev->idx) {
-		/* count number of places needed to print idx */
+	sscanf(newname, PHY_NAME "%d%n", &wiphy_idx, &taken);
+	if (taken == strlen(newname) && wiphy_idx != rdev->wiphy_idx) {
+		/* count number of places needed to print wiphy_idx */
 		digits = 1;
-		while (idx /= 10)
+		while (wiphy_idx /= 10)
 			digits++;
 		/*
 		 * deny the name if it is phy<idx> where <idx> is printed
@@ -222,9 +223,9 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv)
 
 	mutex_lock(&cfg80211_drv_mutex);
 
-	drv->idx = wiphy_counter++;
+	drv->wiphy_idx = wiphy_counter++;
 
-	if (unlikely(drv->idx < 0)) {
+	if (unlikely(drv->wiphy_idx < 0)) {
 		wiphy_counter--;
 		mutex_unlock(&cfg80211_drv_mutex);
 		/* ugh, wrapped! */
@@ -235,7 +236,7 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv)
 	mutex_unlock(&cfg80211_drv_mutex);
 
 	/* give it a proper name */
-	dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->idx);
+	dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->wiphy_idx);
 
 	mutex_init(&drv->mtx);
 	mutex_init(&drv->devlist_mtx);
diff --git a/net/wireless/core.h b/net/wireless/core.h
index e29ad4c..36e2397 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -37,7 +37,7 @@ struct cfg80211_registered_device {
 	enum environment_cap env;
 
 	/* wiphy index, internal only */
-	int idx;
+	int wiphy_idx;
 
 	/* associate netdev list */
 	struct mutex devlist_mtx;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 298a4de..47018bd 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -142,7 +142,7 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
 	if (!hdr)
 		return -1;
 
-	NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, dev->idx);
+	NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, dev->wiphy_idx);
 	NLA_PUT_STRING(msg, NL80211_ATTR_WIPHY_NAME, wiphy_name(&dev->wiphy));
 	NLA_PUT_U8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS,
 		   dev->wiphy.max_scan_ssids);
@@ -2739,7 +2739,7 @@ static int nl80211_send_scan_donemsg(struct sk_buff *msg,
 	if (!hdr)
 		return -1;
 
-	NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, rdev->idx);
+	NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx);
 	NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex);
 
 	/* XXX: we should probably bounce back the request? */
diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c
index 26a72b0..2e3fd91 100644
--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -31,7 +31,7 @@ static ssize_t name ## _show(struct device *dev,			\
 	return sprintf(buf, fmt "\n", dev_to_rdev(dev)->member);	\
 }
 
-SHOW_FMT(index, "%d", idx);
+SHOW_FMT(index, "%d", wiphy_idx);
 SHOW_FMT(macaddress, "%pM", wiphy.perm_addr);
 
 static struct device_attribute ieee80211_dev_attrs[] = {
-- 
1.6.1.2.253.ga34a

[PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity

[Luis R. Rodriguez]

This will later be used by others.

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

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 2b3e786..55be3b0 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -225,7 +225,7 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv)
 
 	drv->wiphy_idx = wiphy_counter++;
 
-	if (unlikely(drv->wiphy_idx < 0)) {
+	if (!wiphy_idx_valid(drv->wiphy_idx)) {
 		wiphy_counter--;
 		mutex_unlock(&cfg80211_drv_mutex);
 		/* ugh, wrapped! */
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 36e2397..e24bc2a 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -62,6 +62,13 @@ struct cfg80211_registered_device *wiphy_to_dev(struct wiphy *wiphy)
 	return container_of(wiphy, struct cfg80211_registered_device, wiphy);
 }
 
+/* Note 0 is valid, hence phy0 */
+static inline
+bool wiphy_idx_valid(int wiphy_idx)
+{
+	return (likely(wiphy_idx >= 0));
+}
+
 extern struct mutex cfg80211_drv_mutex;
 extern struct list_head cfg80211_drv_list;
 
-- 
1.6.1.2.253.ga34a

[PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex

[Luis R. Rodriguez]

cfg80211_drv_mutex is protecting more than the driver list,
this renames it and documents what its currently supposed to
protect.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/core.c    |   35 ++++++++++++++++++++---------------
 net/wireless/core.h    |    6 +++---
 net/wireless/nl80211.c |   20 ++++++++++----------
 net/wireless/reg.c     |   18 +++++++++---------
 4 files changed, 42 insertions(+), 37 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 55be3b0..f55040a 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -31,7 +31,12 @@ MODULE_DESCRIPTION("wireless configuration support");
  * only read the list, and that can happen quite
  * often because we need to do it for each command */
 LIST_HEAD(cfg80211_drv_list);
-DEFINE_MUTEX(cfg80211_drv_mutex);
+
+/*
+ * This is used to protect the cfg80211_drv_list, cfg80211_regdomain, and
+ * the last reguluatory request receipt in regd.c
+ */
+DEFINE_MUTEX(cfg80211_mutex);
 
 /* for debugfs */
 static struct dentry *ieee80211_debugfs_dir;
@@ -52,7 +57,7 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 	return result;
 }
 
-/* requires cfg80211_drv_mutex to be held! */
+/* requires cfg80211_mutex to be held! */
 static struct cfg80211_registered_device *
 __cfg80211_drv_from_info(struct genl_info *info)
 {
@@ -99,7 +104,7 @@ cfg80211_get_dev_from_info(struct genl_info *info)
 {
 	struct cfg80211_registered_device *drv;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	drv = __cfg80211_drv_from_info(info);
 
 	/* if it is not an error we grab the lock on
@@ -108,7 +113,7 @@ cfg80211_get_dev_from_info(struct genl_info *info)
 	if (!IS_ERR(drv))
 		mutex_lock(&drv->mtx);
 
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 
 	return drv;
 }
@@ -119,7 +124,7 @@ cfg80211_get_dev_from_ifindex(int ifindex)
 	struct cfg80211_registered_device *drv = ERR_PTR(-ENODEV);
 	struct net_device *dev;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	dev = dev_get_by_index(&init_net, ifindex);
 	if (!dev)
 		goto out;
@@ -130,7 +135,7 @@ cfg80211_get_dev_from_ifindex(int ifindex)
 		drv = ERR_PTR(-ENODEV);
 	dev_put(dev);
  out:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	return drv;
 }
 
@@ -146,7 +151,7 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 	struct cfg80211_registered_device *drv;
 	int wiphy_idx, taken = -1, result, digits;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	/* prohibit calling the thing phy%d when %d is not its number */
 	sscanf(newname, PHY_NAME "%d%n", &wiphy_idx, &taken);
@@ -194,7 +199,7 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev,
 
 	result = 0;
 out_unlock:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	if (result == 0)
 		nl80211_notify_dev_rename(rdev);
 
@@ -221,19 +226,19 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv)
 
 	drv->ops = ops;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	drv->wiphy_idx = wiphy_counter++;
 
 	if (!wiphy_idx_valid(drv->wiphy_idx)) {
 		wiphy_counter--;
-		mutex_unlock(&cfg80211_drv_mutex);
+		mutex_unlock(&cfg80211_mutex);
 		/* ugh, wrapped! */
 		kfree(drv);
 		return NULL;
 	}
 
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 
 	/* give it a proper name */
 	dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->wiphy_idx);
@@ -311,7 +316,7 @@ int wiphy_register(struct wiphy *wiphy)
 	/* check and set up bitrates */
 	ieee80211_set_bitrate_flags(wiphy);
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	/* set up regulatory info */
 	wiphy_update_regulatory(wiphy, REGDOM_SET_BY_CORE);
@@ -331,7 +336,7 @@ int wiphy_register(struct wiphy *wiphy)
 
 	res = 0;
 out_unlock:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	return res;
 }
 EXPORT_SYMBOL(wiphy_register);
@@ -341,7 +346,7 @@ void wiphy_unregister(struct wiphy *wiphy)
 	struct cfg80211_registered_device *drv = wiphy_to_dev(wiphy);
 
 	/* protect the device list */
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	BUG_ON(!list_empty(&drv->netdev_list));
 
@@ -367,7 +372,7 @@ void wiphy_unregister(struct wiphy *wiphy)
 	device_del(&drv->wiphy.dev);
 	debugfs_remove(drv->wiphy.debugfsdir);
 
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 }
 EXPORT_SYMBOL(wiphy_unregister);
 
diff --git a/net/wireless/core.h b/net/wireless/core.h
index e24bc2a..ba0531e 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -69,7 +69,7 @@ bool wiphy_idx_valid(int wiphy_idx)
 	return (likely(wiphy_idx >= 0));
 }
 
-extern struct mutex cfg80211_drv_mutex;
+extern struct mutex cfg80211_mutex;
 extern struct list_head cfg80211_drv_list;
 
 struct cfg80211_internal_bss {
@@ -88,13 +88,13 @@ struct cfg80211_internal_bss {
  * the driver's mutex!
  *
  * This means that you need to call cfg80211_put_dev()
- * before being allowed to acquire &cfg80211_drv_mutex!
+ * before being allowed to acquire &cfg80211_mutex!
  *
  * This is necessary because we need to lock the global
  * mutex to get an item off the list safely, and then
  * we lock the drv mutex so it doesn't go away under us.
  *
- * We don't want to keep cfg80211_drv_mutex locked
+ * We don't want to keep cfg80211_mutex locked
  * for all the time in order to allow requests on
  * other interfaces to go through at the same time.
  *
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 47018bd..f5c2c05 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -256,7 +256,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
 	int start = cb->args[0];
 	struct cfg80211_registered_device *dev;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	list_for_each_entry(dev, &cfg80211_drv_list, list) {
 		if (++idx <= start)
 			continue;
@@ -267,7 +267,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb)
 			break;
 		}
 	}
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 
 	cb->args[0] = idx;
 
@@ -470,7 +470,7 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback *
 	struct cfg80211_registered_device *dev;
 	struct wireless_dev *wdev;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	list_for_each_entry(dev, &cfg80211_drv_list, list) {
 		if (wp_idx < wp_start) {
 			wp_idx++;
@@ -497,7 +497,7 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback *
 		wp_idx++;
 	}
  out:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 
 	cb->args[0] = wp_idx;
 	cb->args[1] = if_idx;
@@ -1910,9 +1910,9 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info)
 	if (is_world_regdom(data))
 		return -EINVAL;
 #endif
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	r = __regulatory_hint(NULL, REGDOM_SET_BY_USER, data, 0, ENVIRON_ANY);
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	/* This means the regulatory domain was already set, however
 	 * we don't want to confuse userspace with a "successful error"
 	 * message so lets just treat it as a success */
@@ -2106,7 +2106,7 @@ static int nl80211_get_reg(struct sk_buff *skb, struct genl_info *info)
 	unsigned int i;
 	int err = -EINVAL;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	if (!cfg80211_regdomain)
 		goto out;
@@ -2169,7 +2169,7 @@ nla_put_failure:
 	genlmsg_cancel(msg, hdr);
 	err = -EMSGSIZE;
 out:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	return err;
 }
 
@@ -2228,9 +2228,9 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info)
 
 	BUG_ON(rule_idx != num_rules);
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	r = set_regdom(rd);
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	return r;
 
  bad_reg:
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 2323644..ba82312 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1116,7 +1116,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 	return -EINVAL;
 }
 
-/* Caller must hold &cfg80211_drv_mutex */
+/* Caller must hold &cfg80211_mutex */
 int __regulatory_hint(struct wiphy *wiphy, enum reg_set_by set_by,
 			const char *alpha2,
 			u32 country_ie_checksum,
@@ -1188,13 +1188,13 @@ void regulatory_hint(struct wiphy *wiphy, const char *alpha2)
 	int r;
 	BUG_ON(!alpha2);
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 	r = __regulatory_hint(wiphy, REGDOM_SET_BY_DRIVER,
 		alpha2, 0, ENVIRON_ANY);
 	/* This is required so that the orig_* parameters are saved */
 	if (r == -EALREADY && wiphy->strict_regulatory)
 		wiphy_update_regulatory(wiphy, REGDOM_SET_BY_DRIVER);
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 }
 EXPORT_SYMBOL(regulatory_hint);
 
@@ -1225,7 +1225,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	if (!last_request)
 		return;
 
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	/* IE len must be evenly divisible by 2 */
 	if (country_ie_len & 0x01)
@@ -1307,7 +1307,7 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 		country_ie_regdomain->alpha2, checksum, env);
 
 out:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 }
 EXPORT_SYMBOL(regulatory_hint_11d);
 
@@ -1562,7 +1562,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 
 /* Use this call to set the current regulatory domain. Conflicts with
  * multiple drivers can be ironed out later. Caller must've already
- * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */
+ * kmalloc'd the rd structure. Caller must hold cfg80211_mutex */
 int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
@@ -1586,7 +1586,7 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 	return r;
 }
 
-/* Caller must hold cfg80211_drv_mutex */
+/* Caller must hold cfg80211_mutex */
 void reg_device_remove(struct wiphy *wiphy)
 {
 	kfree(wiphy->regd);
@@ -1633,7 +1633,7 @@ int regulatory_init(void)
 
 void regulatory_exit(void)
 {
-	mutex_lock(&cfg80211_drv_mutex);
+	mutex_lock(&cfg80211_mutex);
 
 	reset_regdomains();
 
@@ -1644,5 +1644,5 @@ void regulatory_exit(void)
 
 	platform_device_unregister(reg_pdev);
 
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 }
-- 
1.6.1.2.253.ga34a

[PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection

[Luis R. Rodriguez]

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

diff --git a/net/wireless/core.c b/net/wireless/core.c
index f55040a..6807538 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -7,7 +7,6 @@
 #include <linux/if.h>
 #include <linux/module.h>
 #include <linux/err.h>
-#include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/nl80211.h>
 #include <linux/debugfs.h>
@@ -47,6 +46,8 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 {
 	struct cfg80211_registered_device *result = NULL, *drv;
 
+	assert_cfg80211_lock();
+
 	list_for_each_entry(drv, &cfg80211_drv_list, list) {
 		if (drv->wiphy_idx == wiphy_idx) {
 			result = drv;
@@ -66,6 +67,8 @@ __cfg80211_drv_from_info(struct genl_info *info)
 	struct net_device *dev;
 	int err = -EINVAL;
 
+	assert_cfg80211_lock();
+
 	if (info->attrs[NL80211_ATTR_WIPHY]) {
 		bywiphyidx = cfg80211_drv_by_wiphy_idx(
 				nla_get_u32(info->attrs[NL80211_ATTR_WIPHY]));
diff --git a/net/wireless/core.h b/net/wireless/core.h
index ba0531e..2500dbc 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -10,6 +10,7 @@
 #include <linux/netdevice.h>
 #include <linux/kref.h>
 #include <linux/rbtree.h>
+#include <linux/mutex.h>
 #include <net/genetlink.h>
 #include <net/wireless.h>
 #include <net/cfg80211.h>
@@ -72,6 +73,11 @@ bool wiphy_idx_valid(int wiphy_idx)
 extern struct mutex cfg80211_mutex;
 extern struct list_head cfg80211_drv_list;
 
+static inline void assert_cfg80211_lock(void)
+{
+	BUG_ON(!mutex_is_locked(&cfg80211_mutex));
+}
+
 struct cfg80211_internal_bss {
 	struct list_head list;
 	struct rb_node rbn;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index f5c2c05..843d26d 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -7,7 +7,6 @@
 #include <linux/if.h>
 #include <linux/module.h>
 #include <linux/err.h>
-#include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/if_ether.h>
 #include <linux/ieee80211.h>
@@ -138,6 +137,8 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags,
 	int i;
 	u16 ifmodes = dev->wiphy.interface_modes;
 
+	assert_cfg80211_lock();
+
 	hdr = nl80211hdr_put(msg, pid, seq, flags, NL80211_CMD_NEW_WIPHY);
 	if (!hdr)
 		return -1;
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index ba82312..aab1c69 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -830,6 +830,8 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_channel *chan;
 
+	assert_cfg80211_lock();
+
 	sband = wiphy->bands[band];
 	BUG_ON(chan_idx >= sband->n_channels);
 	chan = &sband->channels[chan_idx];
@@ -1042,6 +1044,10 @@ static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd,
 static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 			  const char *alpha2)
 {
+
+	if (set_by != REGDOM_SET_BY_CORE)
+		assert_cfg80211_lock();
+
 	/* All initial requests are respected */
 	if (!last_request)
 		return 0;
@@ -1126,6 +1132,9 @@ int __regulatory_hint(struct wiphy *wiphy, enum reg_set_by set_by,
 	bool intersect = false;
 	int r = 0;
 
+	if (set_by != REGDOM_SET_BY_CORE)
+		assert_cfg80211_lock();
+
 	r = ignore_request(wiphy, set_by, alpha2);
 
 	if (r == REG_INTERSECT) {
@@ -1201,6 +1210,8 @@ EXPORT_SYMBOL(regulatory_hint);
 static bool reg_same_country_ie_hint(struct wiphy *wiphy,
 			u32 country_ie_checksum)
 {
+	assert_cfg80211_lock();
+
 	if (!last_request->wiphy)
 		return false;
 	if (likely(last_request->wiphy != wiphy))
@@ -1567,6 +1578,8 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
 
+	assert_cfg80211_lock();
+
 	/* Note that this doesn't update the wiphys, this is done below */
 	r = __set_regdom(rd);
 	if (r) {
@@ -1589,6 +1602,8 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 /* Caller must hold cfg80211_mutex */
 void reg_device_remove(struct wiphy *wiphy)
 {
+	assert_cfg80211_lock();
+
 	kfree(wiphy->regd);
 	if (!last_request || !last_request->wiphy)
 		return;
-- 
1.6.1.2.253.ga34a

[PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

We do this so later on we can move the pending requests onto a
workqueue. By using the wiphy_idx instead of the wiphy we can
later easily check if the wiphy has disappeared or not.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 include/net/cfg80211.h |    6 ++--
 net/wireless/core.c    |   18 ++++++++++-
 net/wireless/core.h    |   21 ++++++++++++++
 net/wireless/reg.c     |   73 ++++++++++++++++++++++++++++++++----------------
 4 files changed, 89 insertions(+), 29 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index c0d1f5b..2a83510 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -375,9 +375,9 @@ enum environment_cap {
 };
 
 /**
- * struct regulatory_request - receipt of last regulatory request
+ * struct regulatory_request - used to keep track of regulatory requests
  *
- * @wiphy: this is set if this request's initiator is
+ * @wiphy_idx: this is set if this request's initiator is
  * 	%REGDOM_SET_BY_COUNTRY_IE or %REGDOM_SET_BY_DRIVER. This
  * 	can be used by the wireless core to deal with conflicts
  * 	and potentially inform users of which devices specifically
@@ -398,7 +398,7 @@ enum environment_cap {
  * 	indoor, or if it doesn't matter
  */
 struct regulatory_request {
-	struct wiphy *wiphy;
+	int wiphy_idx;
 	enum reg_set_by initiator;
 	char alpha2[2];
 	bool intersect;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 6807538..2024664 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -41,13 +41,14 @@ DEFINE_MUTEX(cfg80211_mutex);
 static struct dentry *ieee80211_debugfs_dir;
 
 /* requires cfg80211_drv_mutex to be held! */
-static struct cfg80211_registered_device *
-cfg80211_drv_by_wiphy_idx(int wiphy_idx)
+struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 {
 	struct cfg80211_registered_device *result = NULL, *drv;
 
 	assert_cfg80211_lock();
 
+	WARN_ON(!wiphy_idx_valid(wiphy_idx));
+
 	list_for_each_entry(drv, &cfg80211_drv_list, list) {
 		if (drv->wiphy_idx == wiphy_idx) {
 			result = drv;
@@ -58,6 +59,19 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 	return result;
 }
 
+/* requires cfg80211_drv_mutex to be held! */
+struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx)
+{
+	struct cfg80211_registered_device *drv;
+
+	assert_cfg80211_lock();
+
+	drv = cfg80211_drv_by_wiphy_idx(wiphy_idx);
+	if (!drv)
+		return NULL;
+	return &drv->wiphy;
+}
+
 /* requires cfg80211_mutex to be held! */
 static struct cfg80211_registered_device *
 __cfg80211_drv_from_info(struct genl_info *info)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 2500dbc..df1e9ff 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -78,6 +78,22 @@ static inline void assert_cfg80211_lock(void)
 	BUG_ON(!mutex_is_locked(&cfg80211_mutex));
 }
 
+/*
+ * You can use this to mark a wiphy_idx as not having an associated wiphy.
+ * It guarantees cfg80211_drv_by_wiphy_idx(wiphy_idx) will return NULL
+ */
+#define WIPHY_IDX_STALE -1
+
+static inline
+int wiphy_idx(struct wiphy *wiphy)
+{
+	struct cfg80211_registered_device *drv;
+	if (!wiphy)
+		return WIPHY_IDX_STALE;
+	drv = wiphy_to_dev(wiphy);
+	return drv->wiphy_idx;
+}
+
 struct cfg80211_internal_bss {
 	struct list_head list;
 	struct rb_node rbn;
@@ -87,6 +103,8 @@ struct cfg80211_internal_bss {
 	struct cfg80211_bss pub;
 };
 
+struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx);
+
 /*
  * This function returns a pointer to the driver
  * that the genl_info item that is passed refers to.
@@ -110,6 +128,9 @@ struct cfg80211_internal_bss {
 extern struct cfg80211_registered_device *
 cfg80211_get_dev_from_info(struct genl_info *info);
 
+/* requires cfg80211_drv_mutex to be held! */
+struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx);
+
 /* identical to cfg80211_get_dev_from_info but only operate on ifindex */
 extern struct cfg80211_registered_device *
 cfg80211_get_dev_from_ifindex(int ifindex);
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index aab1c69..316b2e4 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -829,9 +829,13 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 	const struct ieee80211_power_rule *power_rule = NULL;
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_channel *chan;
+	struct wiphy *request_wiphy;
 
 	assert_cfg80211_lock();
 
+	if (wiphy_idx_valid(last_request->wiphy_idx))
+		request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
 	sband = wiphy->bands[band];
 	BUG_ON(chan_idx >= sband->n_channels);
 	chan = &sband->channels[chan_idx];
@@ -879,8 +883,8 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 	power_rule = &reg_rule->power_rule;
 
 	if (last_request->initiator == REGDOM_SET_BY_DRIVER &&
-	    last_request->wiphy && last_request->wiphy == wiphy &&
-	    last_request->wiphy->strict_regulatory) {
+	    request_wiphy && request_wiphy == wiphy &&
+	    request_wiphy->strict_regulatory) {
 		/* This gaurantees the driver's requested regulatory domain
 		 * will always be used as a base for further regulatory
 		 * settings */
@@ -1044,6 +1048,11 @@ static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd,
 static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 			  const char *alpha2)
 {
+	/*
+	 * NB: there should be no need to use request_wiphy when in
+	 * REGDOM_SET_BY_CORE, as no wiphys should have sent hints by then
+	 */
+	struct wiphy *request_wiphy;
 
 	if (set_by != REGDOM_SET_BY_CORE)
 		assert_cfg80211_lock();
@@ -1062,10 +1071,15 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 		 */
 		return 0;
 	case REGDOM_SET_BY_COUNTRY_IE:
+
+		if (wiphy_idx_valid(last_request->wiphy_idx))
+			request_wiphy =
+				wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
 		if (unlikely(!is_an_alpha2(alpha2)))
 			return -EINVAL;
 		if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE) {
-			if (last_request->wiphy != wiphy) {
+			if (request_wiphy != wiphy) {
 				/*
 				 * Two cards with two APs claiming different
 				 * different Country IE alpha2s. We could
@@ -1167,7 +1181,7 @@ new_request:
 	request->alpha2[0] = alpha2[0];
 	request->alpha2[1] = alpha2[1];
 	request->initiator = set_by;
-	request->wiphy = wiphy;
+	request->wiphy_idx = wiphy_idx(wiphy);
 	request->intersect = intersect;
 	request->country_ie_checksum = country_ie_checksum;
 	request->country_ie_env = env;
@@ -1210,11 +1224,17 @@ EXPORT_SYMBOL(regulatory_hint);
 static bool reg_same_country_ie_hint(struct wiphy *wiphy,
 			u32 country_ie_checksum)
 {
+	struct wiphy *request_wiphy;
+
 	assert_cfg80211_lock();
 
-	if (!last_request->wiphy)
+	if (wiphy_idx_valid(last_request->wiphy_idx))
+		request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
+	if (!request_wiphy)
 		return false;
-	if (likely(last_request->wiphy != wiphy))
+
+	if (likely(request_wiphy != wiphy))
 		return !country_ie_integrity_changes(country_ie_checksum);
 	/* We should not have let these through at this point, they
 	 * should have been picked up earlier by the first alpha2 check
@@ -1262,14 +1282,15 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	/* We will run this for *every* beacon processed for the BSSID, so
 	 * we optimize an early check to exit out early if we don't have to
 	 * do anything */
-	if (likely(last_request->wiphy)) {
+	if (wiphy_idx_valid(last_request->wiphy_idx)) {
 		struct cfg80211_registered_device *drv_last_ie;
 
-		drv_last_ie = wiphy_to_dev(last_request->wiphy);
+		drv_last_ie =
+			cfg80211_drv_by_wiphy_idx(last_request->wiphy_idx);
 
 		/* Lets keep this simple -- we trust the first AP
 		 * after we intersect with CRDA */
-		if (likely(last_request->wiphy == wiphy)) {
+		if (likely(&drv_last_ie->wiphy == wiphy)) {
 			/* Ignore IEs coming in on this wiphy with
 			 * the same alpha2 and environment cap */
 			if (likely(alpha2_equal(drv_last_ie->country_ie_alpha2,
@@ -1361,13 +1382,12 @@ static void print_regdomain(const struct ieee80211_regdomain *rd)
 {
 
 	if (is_intersected_alpha2(rd->alpha2)) {
-		struct wiphy *wiphy = NULL;
-		struct cfg80211_registered_device *drv;
 
 		if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE) {
-			if (last_request->wiphy) {
-				wiphy = last_request->wiphy;
-				drv = wiphy_to_dev(wiphy);
+			struct cfg80211_registered_device *drv;
+			drv = cfg80211_drv_by_wiphy_idx(
+				last_request->wiphy_idx);
+			if (drv) {
 				printk(KERN_INFO "cfg80211: Current regulatory "
 					"domain updated by AP to: %c%c\n",
 					drv->country_ie_alpha2[0],
@@ -1433,7 +1453,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 {
 	const struct ieee80211_regdomain *intersected_rd = NULL;
 	struct cfg80211_registered_device *drv = NULL;
-	struct wiphy *wiphy = NULL;
+	struct wiphy *request_wiphy;
 	/* Some basic sanity checks first */
 
 	if (is_world_regdom(rd->alpha2)) {
@@ -1461,8 +1481,6 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 			return -EINVAL;
 	}
 
-	wiphy = last_request->wiphy;
-
 	/* Now lets set the regulatory domain, update all driver channels
 	 * and finally inform them of what we have done, in case they want
 	 * to review or adjust their own settings based on their own
@@ -1478,6 +1496,9 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		return -EINVAL;
 	}
 
+	if (wiphy_idx_valid(last_request->wiphy_idx))
+		request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
 	if (!last_request->intersect) {
 		int r;
 
@@ -1490,9 +1511,9 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		/* For a driver hint, lets copy the regulatory domain the
 		 * driver wanted to the wiphy to deal with conflicts */
 
-		BUG_ON(last_request->wiphy->regd);
+		BUG_ON(request_wiphy->regd);
 
-		r = reg_copy_regd(&last_request->wiphy->regd, rd);
+		r = reg_copy_regd(&request_wiphy->regd, rd);
 		if (r)
 			return r;
 
@@ -1513,7 +1534,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		 * However if a driver requested this specific regulatory
 		 * domain we keep it for its private use */
 		if (last_request->initiator == REGDOM_SET_BY_DRIVER)
-			last_request->wiphy->regd = rd;
+			request_wiphy->regd = rd;
 		else
 			kfree(rd);
 
@@ -1553,7 +1574,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 	if (!intersected_rd)
 		return -EINVAL;
 
-	drv = wiphy_to_dev(wiphy);
+	drv = wiphy_to_dev(request_wiphy);
 
 	drv->country_ie_alpha2[0] = rd->alpha2[0];
 	drv->country_ie_alpha2[1] = rd->alpha2[1];
@@ -1602,14 +1623,18 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 /* Caller must hold cfg80211_mutex */
 void reg_device_remove(struct wiphy *wiphy)
 {
+	struct wiphy *request_wiphy;
+
 	assert_cfg80211_lock();
 
+	request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
 	kfree(wiphy->regd);
-	if (!last_request || !last_request->wiphy)
+	if (!last_request || !request_wiphy)
 		return;
-	if (last_request->wiphy != wiphy)
+	if (request_wiphy != wiphy)
 		return;
-	last_request->wiphy = NULL;
+	last_request->wiphy_idx = WIPHY_IDX_STALE;
 	last_request->country_ie_env = ENVIRON_ANY;
 }
 
-- 
1.6.1.2.253.ga34a

[PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

Driver and userspace regulatory hints are now processed in
a workqueue. This should fix some lockdep warnings which complain
about possible circular locking dependencies such as:

1:

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.29-rc4-wl-13559-g0416c58-dirty #40
-------------------------------------------------------
iw/4598 is trying to acquire lock:
 (rtnl_mutex){--..}, at: [<ffffffff805013e2>] rtnl_lock+0x12/0x20
 but task is already holding lock:
  (&drv->mtx){--..}, at: [<ffffffffa0352991>] cfg80211_get_dev_from_ifindex+0x61/0xa0 [cfg80211]

2:

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.29-rc4-wl #7
-------------------------------------------------------
wpa_supplicant/3333 is trying to acquire lock:
 (&ifsta->work){--..}, at: [<ffffffff8025555d>] __cancel_work_timer+0x5d/0x1f0

but task is already holding lock:
 (rtnl_mutex){--..}, at: [<ffffffff804997a6>] devinet_ioctl+0x136/0x7a0

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath9k/main.c      |    8 ++-
 drivers/net/wireless/zd1211rw/zd_mac.c |    6 +-
 include/net/cfg80211.h                 |    2 +
 include/net/wireless.h                 |    9 ++-
 net/wireless/nl80211.c                 |   10 +--
 net/wireless/reg.c                     |  140 +++++++++++++++++++++++++++++---
 net/wireless/reg.h                     |    2 +
 7 files changed, 153 insertions(+), 24 deletions(-)

diff --git a/drivers/net/wireless/ath9k/main.c b/drivers/net/wireless/ath9k/main.c
index fc3460f..f51e7c8 100644
--- a/drivers/net/wireless/ath9k/main.c
+++ b/drivers/net/wireless/ath9k/main.c
@@ -1656,8 +1656,12 @@ int ath_attach(u16 devid, struct ath_softc *sc)
 
 	error = ieee80211_register_hw(hw);
 
-	if (!ath9k_is_world_regd(sc->sc_ah))
-		regulatory_hint(hw->wiphy, sc->sc_ah->regulatory.alpha2);
+	if (!ath9k_is_world_regd(sc->sc_ah)) {
+		error = regulatory_hint(hw->wiphy,
+			sc->sc_ah->regulatory.alpha2);
+		if (error)
+			goto detach;
+	}
 
 	/* Initialize LED control */
 	ath_init_leds(sc);
diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c
index 7579af2..da9214e 100644
--- a/drivers/net/wireless/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zd1211rw/zd_mac.c
@@ -170,10 +170,10 @@ int zd_mac_init_hw(struct ieee80211_hw *hw)
 		goto disable_int;
 
 	r = zd_reg2alpha2(mac->regdomain, alpha2);
-	if (!r)
-		regulatory_hint(hw->wiphy, alpha2);
+	if (r)
+		goto disable_int;
 
-	r = 0;
+	r = regulatory_hint(hw->wiphy, alpha2);
 disable_int:
 	zd_chip_disable_int(chip);
 out:
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 2a83510..ec29843 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -396,6 +396,7 @@ enum environment_cap {
  * 	country IE
  * @country_ie_env: lets us know if the AP is telling us we are outdoor,
  * 	indoor, or if it doesn't matter
+ * @list: used to insert into a linked list
  */
 struct regulatory_request {
 	int wiphy_idx;
@@ -404,6 +405,7 @@ struct regulatory_request {
 	bool intersect;
 	u32 country_ie_checksum;
 	enum environment_cap country_ie_env;
+	struct list_head list;
 };
 
 struct ieee80211_freq_range {
diff --git a/include/net/wireless.h b/include/net/wireless.h
index 1c6285e..afeade9 100644
--- a/include/net/wireless.h
+++ b/include/net/wireless.h
@@ -398,8 +398,15 @@ ieee80211_get_response_rate(struct ieee80211_supported_band *sband,
  * domain should be in or by providing a completely build regulatory domain.
  * If the driver provides an ISO/IEC 3166 alpha2 userspace will be queried
  * for a regulatory domain structure for the respective country.
+ *
+ * The wiphy must have been registered to cfg80211 prior to this call.
+ * For cfg80211 drivers this means you must first use wiphy_register(),
+ * for mac80211 drivers you must first use ieee80211_register_hw().
+ *
+ * Drivers should check the return value, its possible you can get
+ * an -ENOMEM.
  */
-extern void regulatory_hint(struct wiphy *wiphy, const char *alpha2);
+extern int regulatory_hint(struct wiphy *wiphy, const char *alpha2);
 
 /**
  * regulatory_hint_11d - hints a country IE as a regulatory domain
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 843d26d..fb1c99e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1911,14 +1911,8 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info)
 	if (is_world_regdom(data))
 		return -EINVAL;
 #endif
-	mutex_lock(&cfg80211_mutex);
-	r = __regulatory_hint(NULL, REGDOM_SET_BY_USER, data, 0, ENVIRON_ANY);
-	mutex_unlock(&cfg80211_mutex);
-	/* This means the regulatory domain was already set, however
-	 * we don't want to confuse userspace with a "successful error"
-	 * message so lets just treat it as a success */
-	if (r == -EALREADY)
-		r = 0;
+	r = regulatory_hint_user(data);
+
 	return r;
 }
 
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 316b2e4..78ff554 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -64,6 +64,18 @@ const struct ieee80211_regdomain *cfg80211_regdomain;
  * what it thinks should apply for the same country */
 static const struct ieee80211_regdomain *country_ie_regdomain;
 
+static LIST_HEAD(reg_requests_list);
+static DEFINE_MUTEX(reg_mutex);
+
+static void reg_process_pending_hints(void);
+
+static void reg_todo(struct work_struct *work)
+{
+	reg_process_pending_hints();
+}
+
+static DECLARE_WORK(reg_work, reg_todo);
+
 /* We keep a static world regulatory domain in case of the absence of CRDA */
 static const struct ieee80211_regdomain world_regdom = {
 	.n_reg_rules = 1,
@@ -829,7 +841,7 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 	const struct ieee80211_power_rule *power_rule = NULL;
 	struct ieee80211_supported_band *sband;
 	struct ieee80211_channel *chan;
-	struct wiphy *request_wiphy;
+	struct wiphy *request_wiphy = NULL;
 
 	assert_cfg80211_lock();
 
@@ -1206,21 +1218,66 @@ new_request:
 	return call_crda(alpha2);
 }
 
-void regulatory_hint(struct wiphy *wiphy, const char *alpha2)
+/* This currently queues driver and user hints */
+static void queue_regulatory_request(struct regulatory_request *request)
 {
-	int r;
+	mutex_lock(&reg_mutex);
+	list_add_tail(&request->list, &reg_requests_list);
+	mutex_unlock(&reg_mutex);
+
+	schedule_work(&reg_work);
+}
+
+/* Driver hints */
+int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
+{
+	struct regulatory_request *request;
+
 	BUG_ON(!alpha2);
+	BUG_ON(!wiphy);
 
-	mutex_lock(&cfg80211_mutex);
-	r = __regulatory_hint(wiphy, REGDOM_SET_BY_DRIVER,
-		alpha2, 0, ENVIRON_ANY);
-	/* This is required so that the orig_* parameters are saved */
-	if (r == -EALREADY && wiphy->strict_regulatory)
-		wiphy_update_regulatory(wiphy, REGDOM_SET_BY_DRIVER);
-	mutex_unlock(&cfg80211_mutex);
+	request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	request->wiphy_idx = wiphy_idx(wiphy);
+
+	/* Must have registered wiphy first */
+	BUG_ON(!wiphy_idx_valid(request->wiphy_idx));
+
+	request->alpha2[0] = alpha2[0];
+	request->alpha2[1] = alpha2[1];
+	request->initiator = REGDOM_SET_BY_DRIVER;
+	request->country_ie_env = ENVIRON_ANY;
+
+	queue_regulatory_request(request);
+
+	return 0;
 }
 EXPORT_SYMBOL(regulatory_hint);
 
+/* User hints */
+int regulatory_hint_user(const char *alpha2)
+{
+	struct regulatory_request *request;
+
+	BUG_ON(!alpha2);
+
+	request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	request->wiphy_idx = WIPHY_IDX_STALE;
+	request->alpha2[0] = alpha2[0];
+	request->alpha2[1] = alpha2[1];
+	request->initiator = REGDOM_SET_BY_USER,
+	request->country_ie_env = ENVIRON_ANY;
+
+	queue_regulatory_request(request);
+
+	return 0;
+}
+
 static bool reg_same_country_ie_hint(struct wiphy *wiphy,
 			u32 country_ie_checksum)
 {
@@ -1620,6 +1677,58 @@ int set_regdom(const struct ieee80211_regdomain *rd)
 	return r;
 }
 
+/* This currently only processes user and driver regulatory hints */
+static int reg_process_hint(struct regulatory_request *reg_request)
+{
+	int r = 0;
+	struct wiphy *wiphy = NULL;
+
+	BUG_ON(!reg_request->alpha2);
+
+	mutex_lock(&cfg80211_mutex);
+
+	if (wiphy_idx_valid(reg_request->wiphy_idx))
+		wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx);
+
+	if (reg_request->initiator == REGDOM_SET_BY_DRIVER &&
+	    !wiphy) {
+		r = -ENODEV;
+		goto out;
+	}
+
+	r = __regulatory_hint(wiphy, reg_request->initiator,
+		reg_request->alpha2, 0, reg_request->country_ie_env);
+	/* This is required so that the orig_* parameters are saved */
+	if (r == -EALREADY && wiphy->strict_regulatory)
+		wiphy_update_regulatory(wiphy, reg_request->initiator);
+out:
+	mutex_unlock(&cfg80211_mutex);
+
+	if (r == -EALREADY)
+		r = 0;
+
+	return r;
+}
+
+static void reg_process_pending_hints(void)
+	{
+	struct regulatory_request *reg_request, *tmp;
+	int r;
+
+	mutex_lock(&reg_mutex);
+	list_for_each_entry_safe(reg_request, tmp, &reg_requests_list, list) {
+		r = reg_process_hint(reg_request);
+		if (r)
+			printk(KERN_ERR "cfg80211: wiphy_idx %d sent a "
+				"regulatory hint but now has gone fishing, "
+				"ignoring request\n", reg_request->wiphy_idx);
+		list_del(&reg_request->list);
+		kfree(reg_request);
+	}
+	mutex_unlock(&reg_mutex);
+}
+
+
 /* Caller must hold cfg80211_mutex */
 void reg_device_remove(struct wiphy *wiphy)
 {
@@ -1673,6 +1782,11 @@ int regulatory_init(void)
 
 void regulatory_exit(void)
 {
+	struct regulatory_request *reg_request, *tmp;
+
+	cancel_work_sync(&reg_work);
+
+	mutex_lock(&reg_mutex);
 	mutex_lock(&cfg80211_mutex);
 
 	reset_regdomains();
@@ -1684,5 +1798,11 @@ void regulatory_exit(void)
 
 	platform_device_unregister(reg_pdev);
 
+	list_for_each_entry_safe(reg_request, tmp, &reg_requests_list, list) {
+		list_del(&reg_request->list);
+		kfree(reg_request);
+	}
+
 	mutex_unlock(&cfg80211_mutex);
+	mutex_unlock(&reg_mutex);
 }
diff --git a/net/wireless/reg.h b/net/wireless/reg.h
index fe8c83f..4730def 100644
--- a/net/wireless/reg.h
+++ b/net/wireless/reg.h
@@ -6,6 +6,8 @@ extern const struct ieee80211_regdomain *cfg80211_regdomain;
 bool is_world_regdom(const char *alpha2);
 bool reg_is_valid_request(const char *alpha2);
 
+int regulatory_hint_user(const char *alpha2);
+
 void reg_device_remove(struct wiphy *wiphy);
 
 int regulatory_init(void);
-- 
1.6.1.2.253.ga34a

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Johannes Berg]

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

On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
> Driver and userspace regulatory hints are now processed in
> a workqueue. This should fix some lockdep warnings which complain
> about possible circular locking dependencies such as:

Yes, but I need to look at the one Reinette posted and fix that one,
which will probably require a different solution.

johannes

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

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>> Driver and userspace regulatory hints are now processed in
>> a workqueue. This should fix some lockdep warnings which complain
>> about possible circular locking dependencies such as:
>
> Yes, but I need to look at the one Reinette posted and fix that one,
> which will probably require a different solution.

Oh I didn't see that one. How different?

  Luis

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 11:09 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>>> Driver and userspace regulatory hints are now processed in
>>> a workqueue. This should fix some lockdep warnings which complain
>>> about possible circular locking dependencies such as:
>>
>> Yes, but I need to look at the one Reinette posted and fix that one,
>> which will probably require a different solution.
>
> Oh I didn't see that one. How different?

I see an rtnl lock issue against scanning and the drv mutex. Doesn't
seem to be related to regulatory. So how would that affect a path to
move hints to a workqueue?

  Luis

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 11:14 PM, Luis R. Rodriguez
<lrodriguez@atheros.com> wrote:
> On Thu, Feb 12, 2009 at 11:09 PM, Luis R. Rodriguez
> <lrodriguez@atheros.com> wrote:
>> On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>>> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>>>> Driver and userspace regulatory hints are now processed in
>>>> a workqueue. This should fix some lockdep warnings which complain
>>>> about possible circular locking dependencies such as:
>>>
>>> Yes, but I need to look at the one Reinette posted and fix that one,
>>> which will probably require a different solution.
>>
>> Oh I didn't see that one. How different?
>
> I see an rtnl lock issue against scanning and the drv mutex. Doesn't
> seem to be related to regulatory. So how would that affect a path to
> move hints to a workqueue?

Oh you mean the rntl_lock() and drv mutex is _still_ an issue?

  Luis

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Johannes Berg]

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

On Thu, 2009-02-12 at 23:14 -0800, Luis R. Rodriguez wrote:

> >>> Yes, but I need to look at the one Reinette posted and fix that one,
> >>> which will probably require a different solution.
> >>
> >> Oh I didn't see that one. How different?
> >
> > I see an rtnl lock issue against scanning and the drv mutex. Doesn't
> > seem to be related to regulatory. So how would that affect a path to
> > move hints to a workqueue?
> 
> Oh you mean the rntl_lock() and drv mutex is _still_ an issue?

Well, regardless of regulatory, we now have two paths:

nl80211: cfg80211_mutex  -->  drv->mutex  -->  rtnl
wext:    rtnl  -->  cfg80211_mutex  -->  drv->mutex

which is clearly not a good plan.

Therefore, I'll probably have to implement solution (1) from what I sent
you, and invert the locking in nl80211. That would fix your the
immediate problem with regulatory as well, because that was:

rtnl  --->  (regulatory_hint) --->  cfg80211_mutex  -->  drv->mutex

johannes

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

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 11:17 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 23:14 -0800, Luis R. Rodriguez wrote:
>
>> >>> Yes, but I need to look at the one Reinette posted and fix that one,
>> >>> which will probably require a different solution.
>> >>
>> >> Oh I didn't see that one. How different?
>> >
>> > I see an rtnl lock issue against scanning and the drv mutex. Doesn't
>> > seem to be related to regulatory. So how would that affect a path to
>> > move hints to a workqueue?
>>
>> Oh you mean the rntl_lock() and drv mutex is _still_ an issue?
>
> Well, regardless of regulatory, we now have two paths:
>
> nl80211: cfg80211_mutex  -->  drv->mutex  -->  rtnl
> wext:    rtnl  -->  cfg80211_mutex  -->  drv->mutex
>
> which is clearly not a good plan.
>
> Therefore, I'll probably have to implement solution (1) from what I sent
> you, and invert the locking in nl80211. That would fix your the
> immediate problem with regulatory as well, because that was:
>
> rtnl  --->  (regulatory_hint) --->  cfg80211_mutex  -->  drv->mutex

Yup I see. So as it stands in this patch user reg hint now is:

reg_mutex --> cfg80211_mutex

Driver hint also does the same so it seems your fix on nl80211 would
probably just tap this series on the nl80211_req_set_reg().

I also think using a workqueue would be good here anyway, the only
negative thing I've seen is loading takes a bit longer.

  Luis

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Johannes Berg]

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

On Thu, 2009-02-12 at 23:35 -0800, Luis R. Rodriguez wrote:

> > Well, regardless of regulatory, we now have two paths:
> >
> > nl80211: cfg80211_mutex  -->  drv->mutex  -->  rtnl
> > wext:    rtnl  -->  cfg80211_mutex  -->  drv->mutex
> >
> > which is clearly not a good plan.
> >
> > Therefore, I'll probably have to implement solution (1) from what I sent
> > you, and invert the locking in nl80211. That would fix your the
> > immediate problem with regulatory as well, because that was:
> >
> > rtnl  --->  (regulatory_hint) --->  cfg80211_mutex  -->  drv->mutex
> 
> Yup I see. So as it stands in this patch user reg hint now is:
> 
> reg_mutex --> cfg80211_mutex

I don't really care about reg_mutex much, and it probably doesn't matter
as long as its use is limited. Question is whether we need it, but
that's a different question.

> Driver hint also does the same so it seems your fix on nl80211 would
> probably just tap this series on the nl80211_req_set_reg().

Yeah, I think I'll work on top of your series anyway so I don't clash
with the cleanups.

> I also think using a workqueue would be good here anyway, the only
> negative thing I've seen is loading takes a bit longer.

Indeed, it's a good thing because a path like this:

driver_open
 -> regulatory_hint
  -> cfg80211 stuff
   -> driver_reg_notifier

is always deadlock prone since you call into the driver from itself.

johannes

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

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 11:48 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 23:35 -0800, Luis R. Rodriguez wrote:
>
>> > Well, regardless of regulatory, we now have two paths:
>> >
>> > nl80211: cfg80211_mutex  -->  drv->mutex  -->  rtnl
>> > wext:    rtnl  -->  cfg80211_mutex  -->  drv->mutex
>> >
>> > which is clearly not a good plan.
>> >
>> > Therefore, I'll probably have to implement solution (1) from what I sent
>> > you, and invert the locking in nl80211. That would fix your the
>> > immediate problem with regulatory as well, because that was:
>> >
>> > rtnl  --->  (regulatory_hint) --->  cfg80211_mutex  -->  drv->mutex
>>
>> Yup I see. So as it stands in this patch user reg hint now is:
>>
>> reg_mutex --> cfg80211_mutex
>
> I don't really care about reg_mutex much, and it probably doesn't matter
> as long as its use is limited. Question is whether we need it, but
> that's a different question.

To answer that question -- reg_mutex just protects the
reg_requests_list linked list. That's it. Without that you could
potentially have two userspace reg hints while the workqueue runs.
Remember that the hints no longer lock except to just add a new
regulatory_request into the reg_requests_list.

>> Driver hint also does the same so it seems your fix on nl80211 would
>> probably just tap this series on the nl80211_req_set_reg().
>
> Yeah, I think I'll work on top of your series anyway so I don't clash
> with the cleanups.

Any other comments on the series?

>> I also think using a workqueue would be good here anyway, the only
>> negative thing I've seen is loading takes a bit longer.
>
> Indeed, it's a good thing because a path like this:
>
> driver_open
>  -> regulatory_hint
>  -> cfg80211 stuff
>   -> driver_reg_notifier
>
> is always deadlock prone since you call into the driver from itself.

I see -- good point.

  Luis

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Johannes Berg]

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

On Thu, 2009-02-12 at 23:57 -0800, Luis R. Rodriguez wrote:

> To answer that question -- reg_mutex just protects the
> reg_requests_list linked list. That's it. Without that you could
> potentially have two userspace reg hints while the workqueue runs.
> Remember that the hints no longer lock except to just add a new
> regulatory_request into the reg_requests_list.

Right, ok.

> >> Driver hint also does the same so it seems your fix on nl80211 would
> >> probably just tap this series on the nl80211_req_set_reg().
> >
> > Yeah, I think I'll work on top of your series anyway so I don't clash
> > with the cleanups.
> 
> Any other comments on the series?

Not really, looks pretty good to me. It does seem a little dubious to
return an error from a hint function and then not attach the hardware,
but that's up to you what you want to do in ath9k I guess -- seems it
should work ok even if the hint failed, though of course then you'll not
have memory for anything else either...

johannes

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

Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

On Fri, Feb 13, 2009 at 12:08 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 23:57 -0800, Luis R. Rodriguez wrote:
>
>> To answer that question -- reg_mutex just protects the
>> reg_requests_list linked list. That's it. Without that you could
>> potentially have two userspace reg hints while the workqueue runs.
>> Remember that the hints no longer lock except to just add a new
>> regulatory_request into the reg_requests_list.
>
> Right, ok.
>
>> >> Driver hint also does the same so it seems your fix on nl80211 would
>> >> probably just tap this series on the nl80211_req_set_reg().
>> >
>> > Yeah, I think I'll work on top of your series anyway so I don't clash
>> > with the cleanups.
>>
>> Any other comments on the series?
>
> Not really, looks pretty good to me. It does seem a little dubious to
> return an error from a hint function and then not attach the hardware,
> but that's up to you what you want to do in ath9k I guess -- seems it
> should work ok even if the hint failed, though of course then you'll not
> have memory for anything else either...

The only error is -ENOMEM so yeah we certainly should propagate that
down to the driver. An alternative is to use a few regulatory_requests
on the stack for queuing purposes but it just seemed silly on second
thought.

  Luis

Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Johannes Berg]

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

On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
> 
> +       if (wiphy_idx_valid(last_request->wiphy_idx))
> +               request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);

All this seems pointless, wiphy_idx_to_wiphy will just return NULL if
the index isn't valid. This is in a number of places, and it's not like
it being invalid will be happening often so we'd have to optimise for
it.

johannes

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

Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>>
>> +       if (wiphy_idx_valid(last_request->wiphy_idx))
>> +               request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
>
> All this seems pointless, wiphy_idx_to_wiphy will just return NULL if
> the index isn't valid. This is in a number of places, and it's not like
> it being invalid will be happening often so we'd have to optimise for
> it.

The check can be removed if the WARNING is removed on
wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the
warning, your call.

  Luis

Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Johannes Berg]

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

On Fri, 2009-02-13 at 10:21 -0800, Luis R. Rodriguez wrote:
> On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
> >>
> >> +       if (wiphy_idx_valid(last_request->wiphy_idx))
> >> +               request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
> >
> > All this seems pointless, wiphy_idx_to_wiphy will just return NULL if
> > the index isn't valid. This is in a number of places, and it's not like
> > it being invalid will be happening often so we'd have to optimise for
> > it.
> 
> The check can be removed if the WARNING is removed on
> wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the
> warning, your call.

Dunno, I think no warning is probably better if more than half the
callers would have to check first... Also, the warning seems like it
could spuriously trigger if a wiphy is removed?

One other thing I noticed - why is there a conditional assert on the
mutex? Shouldn't it always be locked?

johannes

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

Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

On Fri, Feb 13, 2009 at 1:24 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 10:21 -0800, Luis R. Rodriguez wrote:
>> On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>> >>
>> >> +       if (wiphy_idx_valid(last_request->wiphy_idx))
>> >> +               request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
>> >
>> > All this seems pointless, wiphy_idx_to_wiphy will just return NULL if
>> > the index isn't valid. This is in a number of places, and it's not like
>> > it being invalid will be happening often so we'd have to optimise for
>> > it.
>>
>> The check can be removed if the WARNING is removed on
>> wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the
>> warning, your call.
>
> Dunno, I think no warning is probably better if more than half the
> callers would have to check first... Also, the warning seems like it
> could spuriously trigger if a wiphy is removed?

Yeah I think you're right.

> One other thing I noticed - why is there a conditional assert on the
> mutex? Shouldn't it always be locked?

The conditional assert on the mutex is there because on cfg80211 you
don't want to hold a mutex as that would mean userspace gets stuck
with one. I believe lockdep complained to me about it as exactly that.

  Luis

Re: [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection

[Johannes Berg]

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

On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:

> +static inline void assert_cfg80211_lock(void)
> +{
> +	BUG_ON(!mutex_is_locked(&cfg80211_mutex));
> +}

That's a little heavy-handed for a small development mistake, can we
have a WARN_ON instead?

johannes

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

Re: [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 10:54 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote:
>
>> +static inline void assert_cfg80211_lock(void)
>> +{
>> +     BUG_ON(!mutex_is_locked(&cfg80211_mutex));
>> +}
>
> That's a little heavy-handed for a small development mistake, can we
> have a WARN_ON instead?

Sure, will change.

  Luis

Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity

[Johannes Berg]

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

On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote:

> -	if (unlikely(drv->wiphy_idx < 0)) {
> +	if (!wiphy_idx_valid(drv->wiphy_idx)) {

> +/* Note 0 is valid, hence phy0 */
> +static inline
> +bool wiphy_idx_valid(int wiphy_idx)
> +{
> +	return (likely(wiphy_idx >= 0));
> +}

Does that really work properly with the likely in there?

johannes

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

Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity

[Luis R. Rodriguez]

On Thu, Feb 12, 2009 at 10:53 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote:
>
>> -     if (unlikely(drv->wiphy_idx < 0)) {
>> +     if (!wiphy_idx_valid(drv->wiphy_idx)) {
>
>> +/* Note 0 is valid, hence phy0 */
>> +static inline
>> +bool wiphy_idx_valid(int wiphy_idx)
>> +{
>> +     return (likely(wiphy_idx >= 0));
>> +}
>
> Does that really work properly with the likely in there?

Oh you mean !likely() won't give me my unlikely() effect? I think so
as the path is meant to not be optimized so the optimized path would
be put first, no?

  Luis

Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity

[Johannes Berg]

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

On Thu, 2009-02-12 at 23:08 -0800, Luis R. Rodriguez wrote:
> On Thu, Feb 12, 2009 at 10:53 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote:
> >
> >> -     if (unlikely(drv->wiphy_idx < 0)) {
> >> +     if (!wiphy_idx_valid(drv->wiphy_idx)) {
> >
> >> +/* Note 0 is valid, hence phy0 */
> >> +static inline
> >> +bool wiphy_idx_valid(int wiphy_idx)
> >> +{
> >> +     return (likely(wiphy_idx >= 0));
> >> +}
> >
> > Does that really work properly with the likely in there?
> 
> Oh you mean !likely() won't give me my unlikely() effect? I think so
> as the path is meant to not be optimized so the optimized path would
> be put first, no?

Well keep in mind that likely() also affects branch prediction on some
CPUs where you can annotate that, and I think it wouldn't be possible
for the compiler to get it out of the inline there properly.

johannes

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

Re: [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints

[Johannes Berg]

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

On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote:
> This series does prep work and then the actual work to move the
> cfg80211 driver and userspace regulatory hints onto a workqueue.
> It should cure some lockdep warnings and also pave the way for
> more future reg work like beacon reg hints.
> 
> Luis R. Rodriguez (6):
>   cfg80211: rename cfg80211_registered_device's idx to wiphy_idx
>   cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity
>   cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex
>   cfg80211: add assert_cfg80211_lock() to ensure proper protection
>   cfg80211: make regulatory_request use wiphy_idx instead of wiphy
>   cfg80211: move regulatory hints to workqueue

net/wireless/reg.c: In function ‘ignore_request’:
net/wireless/reg.c:1067: warning: ‘request_wiphy’ may be used uninitialized in this function
net/wireless/reg.c: In function ‘__set_regdom’:
net/wireless/reg.c:1513: warning: ‘request_wiphy’ may be used uninitialized in this function

johannes

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