[PATCH 00/10] cfg80211: move reg hints to workqueue

[PATCH 00/10] cfg80211: move reg hints to workqueue

[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.
This also cleans up the core reg hint onto its own routine which
addresses some of Johannes comments about the branching on the
assert_cfg80211_mutex(). We also now do a cleanup the comment
style on reg.c.

Luis R. Rodriguez (10):
  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
  nl80211: disallow user requests prior to regulatory_init()
  cfg80211: add regulatory_hint_core() to separate the core reg hint
  cfg80211: propagate -ENOMEM during regulatory_init()
  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
  cfg80211: comments style cleanup

 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                    |   93 ++++--
 net/wireless/core.h                    |   42 ++-
 net/wireless/nl80211.c                 |   42 ++-
 net/wireless/reg.c                     |  578 +++++++++++++++++++++++---------
 net/wireless/reg.h                     |    2 +
 net/wireless/sysfs.c                   |    2 +-
 10 files changed, 572 insertions(+), 218 deletions(-)

[PATCH 01/10] 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 fd46b18..80741c4 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -163,7 +163,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);
@@ -2705,7 +2705,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 02/10] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity

[Luis R. Rodriguez]

This will later be used by others, for now make use of it in
cfg80211_drv_by_wiphy_idx() to return early if an invalid
wiphy_idx has been provided.

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

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 2b3e786..35d457b 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -42,6 +42,9 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 {
 	struct cfg80211_registered_device *result = NULL, *drv;
 
+	if (!wiphy_idx_valid(wiphy_idx))
+		return NULL;
+
 	list_for_each_entry(drv, &cfg80211_drv_list, list) {
 		if (drv->wiphy_idx == wiphy_idx) {
 			result = drv;
@@ -225,7 +228,7 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv)
 
 	drv->wiphy_idx = wiphy_counter++;
 
-	if (unlikely(drv->wiphy_idx < 0)) {
+	if (unlikely(!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..505fc10 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 (wiphy_idx >= 0);
+}
+
 extern struct mutex cfg80211_drv_mutex;
 extern struct list_head cfg80211_drv_list;
 
-- 
1.6.1.2.253.ga34a

[PATCH 03/10] 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 35d457b..39d40d1 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;
@@ -55,7 +60,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)
 {
@@ -102,7 +107,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
@@ -111,7 +116,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;
 }
@@ -122,7 +127,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;
@@ -133,7 +138,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;
 }
 
@@ -149,7 +154,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);
@@ -197,7 +202,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);
 
@@ -224,19 +229,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 (unlikely(!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);
@@ -314,7 +319,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);
@@ -334,7 +339,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);
@@ -344,7 +349,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));
 
@@ -370,7 +375,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 505fc10..e12eab5 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -69,7 +69,7 @@ bool wiphy_idx_valid(int wiphy_idx)
 	return (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 80741c4..db973f2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -277,7 +277,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;
@@ -288,7 +288,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;
 
@@ -491,7 +491,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++;
@@ -518,7 +518,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;
@@ -1886,9 +1886,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 */
@@ -2078,7 +2078,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;
@@ -2141,7 +2141,7 @@ nla_put_failure:
 	genlmsg_cancel(msg, hdr);
 	err = -EMSGSIZE;
 out:
-	mutex_unlock(&cfg80211_drv_mutex);
+	mutex_unlock(&cfg80211_mutex);
 	return err;
 }
 
@@ -2200,9 +2200,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 04/10] nl80211: disallow user requests prior to regulatory_init()

[Luis R. Rodriguez]

If cfg80211 is built into the kernel there is perhaps a small
time window betwen nl80211_init() and regulatory_init() where
cfg80211_regdomain hasn't yet been initialized to let the
wireless core do its work. During that rare case and time
frame (if its even possible) we don't allow user regulatory
changes as cfg80211 is working on enabling its first regulatory
domain.

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

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index db973f2..6f4dedb 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1876,6 +1876,15 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info)
 	int r;
 	char *data = NULL;
 
+	/*
+	 * You should only get this when cfg80211 hasn't yet initialized
+	 * completely when built-in to the kernel right between the time
+	 * window between nl80211_init() and regulatory_init(), if that is
+	 * even possible.
+	 */
+	if (!cfg80211_regdomain)
+		return -EINPROGRESS;
+
 	if (!info->attrs[NL80211_ATTR_REG_ALPHA2])
 		return -EINVAL;
 
-- 
1.6.1.2.253.ga34a

[PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Luis R. Rodriguez]

This will allow us to clean up and make distinctions of who
needs locking or not.

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

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index ba82312..679fded 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1050,11 +1050,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 	case REGDOM_SET_BY_INIT:
 		return -EINVAL;
 	case REGDOM_SET_BY_CORE:
-		/*
-		 * Always respect new wireless core hints, should only happen
-		 * when updating the world regulatory domain at init.
-		 */
-		return 0;
+		return -EINVAL;
 	case REGDOM_SET_BY_COUNTRY_IE:
 		if (unlikely(!is_an_alpha2(alpha2)))
 			return -EINVAL;
@@ -1183,6 +1179,26 @@ new_request:
 	return call_crda(alpha2);
 }
 
+static int regulatory_hint_core(const char *alpha2)
+{
+	struct regulatory_request *request;
+
+	BUG_ON(last_request);
+
+	request = kzalloc(sizeof(struct regulatory_request),
+			  GFP_KERNEL);
+	if (!request)
+		return -ENOMEM;
+
+	request->alpha2[0] = alpha2[0];
+	request->alpha2[1] = alpha2[1];
+	request->initiator = REGDOM_SET_BY_CORE;
+
+	last_request = request;
+
+	return call_crda(alpha2);
+}
+
 void regulatory_hint(struct wiphy *wiphy, const char *alpha2)
 {
 	int r;
@@ -1616,16 +1632,18 @@ int regulatory_init(void)
 	 * stuck with the static values. We ignore "EU" code as
 	 * that is not a valid ISO / IEC 3166 alpha2 */
 	if (ieee80211_regdom[0] != 'E' || ieee80211_regdom[1] != 'U')
-		err = __regulatory_hint(NULL, REGDOM_SET_BY_CORE,
-					ieee80211_regdom, 0, ENVIRON_ANY);
+		err = regulatory_hint_core(ieee80211_regdom);
 #else
 	cfg80211_regdomain = cfg80211_world_regdom;
 
-	err = __regulatory_hint(NULL, REGDOM_SET_BY_CORE, "00", 0, ENVIRON_ANY);
-	if (err)
+	err = regulatory_hint_core("00");
+	if (err) {
+		if (err == -ENOMEM)
+			return err;
 		printk(KERN_ERR "cfg80211: calling CRDA failed - "
 		       "unable to update world regulatory domain, "
 		       "using static definition\n");
+	}
 #endif
 
 	return 0;
-- 
1.6.1.2.253.ga34a

[PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Luis R. Rodriguez]

Calling kobject_uevent_env() can fail mainly due to out of
memory conditions. We do not want to continue during such
conditions so propagate that as well instead of letting
cfg80211 load as if everything is peachy.

Additionally lets clarify that when CRDA is not called during
cfg80211's initialization _and_ if the error is not an -ENOMEM
its because kobject_uevent_env() failed to call CRDA, not because
CRDA failed. For those who want to find out why we also let you
do so by enabling the kernel config CONFIG_CFG80211_REG_DEBUG --
you'll get an actual stack trace.

So for now we'll treat non -ENOMEM kobject_uevent_env() failures as
non fatal during cfg80211's initialization.

CC: Greg KH <greg@kroah.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 net/wireless/reg.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 679fded..47d5056 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1616,7 +1616,7 @@ void reg_device_remove(struct wiphy *wiphy)
 
 int regulatory_init(void)
 {
-	int err;
+	int err = 0;
 
 	reg_pdev = platform_device_register_simple("regulatory", 0, NULL, 0);
 	if (IS_ERR(reg_pdev))
@@ -1637,14 +1637,24 @@ int regulatory_init(void)
 	cfg80211_regdomain = cfg80211_world_regdom;
 
 	err = regulatory_hint_core("00");
+#endif
 	if (err) {
 		if (err == -ENOMEM)
 			return err;
-		printk(KERN_ERR "cfg80211: calling CRDA failed - "
-		       "unable to update world regulatory domain, "
-		       "using static definition\n");
-	}
+		/*
+		 * N.B. kobject_uevent_env() can fail mainly for when we're out
+		 * memory which is handled and propagated appropriately above
+		 * but it can also fail during a netlink_broadcast() or during
+		 * early boot for call_usermodehelper(). For now treat these
+		 * errors as non-fatal.
+		 */
+		printk(KERN_ERR "cfg80211: kobject_uevent_env() was unable "
+			"to call CRDA during init");
+#ifdef CONFIG_CFG80211_REG_DEBUG
+		/* We want to find out exactly why when debugging */
+		WARN_ON(err);
 #endif
+	}
 
 	return 0;
 }
-- 
1.6.1.2.253.ga34a

[PATCH 07/10] 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 39d40d1..e347093 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>
@@ -50,6 +49,8 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 	if (!wiphy_idx_valid(wiphy_idx))
 		return NULL;
 
+	assert_cfg80211_lock();
+
 	list_for_each_entry(drv, &cfg80211_drv_list, list) {
 		if (drv->wiphy_idx == wiphy_idx) {
 			result = drv;
@@ -69,6 +70,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 e12eab5..1f036f3 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 6f4dedb..89593d3 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>
@@ -159,6 +158,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 47d5056..93c197c 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;
@@ -1122,6 +1128,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) {
@@ -1217,6 +1226,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))
@@ -1583,6 +1594,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) {
@@ -1605,6 +1618,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 08/10] 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    |   21 ++++++++++++--
 net/wireless/core.h    |   21 ++++++++++++++
 net/wireless/reg.c     |   70 ++++++++++++++++++++++++++++-------------------
 4 files changed, 84 insertions(+), 34 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 e347093..f6b612d 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -40,9 +40,8 @@ DEFINE_MUTEX(cfg80211_mutex);
 /* for debugfs */
 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)
+/* requires cfg80211_mutex to be held! */
+struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx)
 {
 	struct cfg80211_registered_device *result = NULL, *drv;
 
@@ -61,6 +60,22 @@ 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;
+
+	if (!wiphy_idx_valid(wiphy_idx))
+		return NULL;
+
+	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 1f036f3..24f98c3 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 93c197c..098e416 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -829,9 +829,12 @@ 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();
 
+	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 +882,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,9 +1047,9 @@ 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)
 {
+	struct wiphy *last_wiphy = NULL;
 
-	if (set_by != REGDOM_SET_BY_CORE)
-		assert_cfg80211_lock();
+	assert_cfg80211_lock();
 
 	/* All initial requests are respected */
 	if (!last_request)
@@ -1058,10 +1061,13 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 	case REGDOM_SET_BY_CORE:
 		return -EINVAL;
 	case REGDOM_SET_BY_COUNTRY_IE:
+
+		last_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 (last_wiphy != wiphy) {
 				/*
 				 * Two cards with two APs claiming different
 				 * different Country IE alpha2s. We could
@@ -1128,8 +1134,7 @@ 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();
+	assert_cfg80211_lock();
 
 	r = ignore_request(wiphy, set_by, alpha2);
 
@@ -1163,7 +1168,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;
@@ -1226,11 +1231,16 @@ 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)
+	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
@@ -1278,14 +1288,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 (likely(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,
@@ -1377,13 +1388,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],
@@ -1449,7 +1459,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)) {
@@ -1477,8 +1487,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
@@ -1494,6 +1502,8 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		return -EINVAL;
 	}
 
+	request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx);
+
 	if (!last_request->intersect) {
 		int r;
 
@@ -1506,9 +1516,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;
 
@@ -1529,7 +1539,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);
 
@@ -1569,7 +1579,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];
@@ -1618,14 +1628,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 09/10] cfg80211: move regulatory hints to workqueue

[Luis R. Rodriguez]

Driver and userspace regulatory hints are now processed in
a workqueue. This will allow us to later add more work into
the workqueue such as doing AP beacon regulatory hints.

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                     |  138 +++++++++++++++++++++++++++++---
 net/wireless/reg.h                     |    2 +
 7 files changed, 151 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 89593d3..3a9ecb2 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1896,14 +1896,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 098e416..6c8c719 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();
 
@@ -1213,18 +1225,61 @@ static int regulatory_hint_core(const char *alpha2)
 	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);
+}
+
+/* User hints */
+int regulatory_hint_user(const char *alpha2)
+{
+	struct regulatory_request *request;
+
 	BUG_ON(!alpha2);
 
-	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_STALE;
+	request->alpha2[0] = alpha2[0];
+	request->alpha2[1] = alpha2[1];
+	request->initiator = REGDOM_SET_BY_USER,
+
+	queue_regulatory_request(request);
+
+	return 0;
+}
+
+/* Driver hints */
+int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
+{
+	struct regulatory_request *request;
+
+	BUG_ON(!alpha2);
+	BUG_ON(!wiphy);
+
+	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;
+
+	queue_regulatory_request(request);
+
+	return 0;
 }
 EXPORT_SYMBOL(regulatory_hint);
 
@@ -1625,6 +1680,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)
 {
@@ -1690,6 +1797,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();
@@ -1701,5 +1813,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

[PATCH 10/10] cfg80211: comments style cleanup

[Luis R. Rodriguez]

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

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 6c8c719..9dce86a 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -54,14 +54,18 @@ static u32 supported_bandwidths[] = {
 	MHZ_TO_KHZ(20),
 };
 
-/* Central wireless core regulatory domains, we only need two,
+/*
+ * Central wireless core regulatory domains, we only need two,
  * the current one and a world regulatory domain in case we have no
- * information to give us an alpha2 */
+ * information to give us an alpha2
+ */
 const struct ieee80211_regdomain *cfg80211_regdomain;
 
-/* We use this as a place for the rd structure built from the
+/*
+ * We use this as a place for the rd structure built from the
  * last parsed country IE to rest until CRDA gets back to us with
- * what it thinks should apply for the same country */
+ * what it thinks should apply for the same country
+ */
 static const struct ieee80211_regdomain *country_ie_regdomain;
 
 static LIST_HEAD(reg_requests_list);
@@ -95,9 +99,11 @@ static char *ieee80211_regdom = "US";
 module_param(ieee80211_regdom, charp, 0444);
 MODULE_PARM_DESC(ieee80211_regdom, "IEEE 802.11 regulatory domain code");
 
-/* We assume 40 MHz bandwidth for the old regulatory work.
+/*
+ * We assume 40 MHz bandwidth for the old regulatory work.
  * We make emphasis we are using the exact same frequencies
- * as before */
+ * as before
+ */
 
 static const struct ieee80211_regdomain us_regdom = {
 	.n_reg_rules = 6,
@@ -136,8 +142,10 @@ static const struct ieee80211_regdomain jp_regdom = {
 
 static const struct ieee80211_regdomain eu_regdom = {
 	.n_reg_rules = 6,
-	/* This alpha2 is bogus, we leave it here just for stupid
-	 * backward compatibility */
+	/*
+	 * This alpha2 is bogus, we leave it here just for stupid
+	 * backward compatibility
+	 */
 	.alpha2 =  "EU",
 	.reg_rules = {
 		/* IEEE 802.11b/g, channels 1..13 */
@@ -206,8 +214,10 @@ static void reset_regdomains(void)
 	cfg80211_regdomain = NULL;
 }
 
-/* Dynamic world regulatory domain requested by the wireless
- * core upon initialization */
+/*
+ * Dynamic world regulatory domain requested by the wireless
+ * core upon initialization
+ */
 static void update_world_regdomain(const struct ieee80211_regdomain *rd)
 {
 	BUG_ON(!last_request);
@@ -248,8 +258,10 @@ static bool is_unknown_alpha2(const char *alpha2)
 {
 	if (!alpha2)
 		return false;
-	/* Special case where regulatory domain was built by driver
-	 * but a specific alpha2 cannot be determined */
+	/*
+	 * Special case where regulatory domain was built by driver
+	 * but a specific alpha2 cannot be determined
+	 */
 	if (alpha2[0] == '9' && alpha2[1] == '9')
 		return true;
 	return false;
@@ -259,9 +271,11 @@ static bool is_intersected_alpha2(const char *alpha2)
 {
 	if (!alpha2)
 		return false;
-	/* Special case where regulatory domain is the
+	/*
+	 * Special case where regulatory domain is the
 	 * result of an intersection between two regulatory domain
-	 * structures */
+	 * structures
+	 */
 	if (alpha2[0] == '9' && alpha2[1] == '8')
 		return true;
 	return false;
@@ -314,8 +328,10 @@ static bool country_ie_integrity_changes(u32 checksum)
 	return false;
 }
 
-/* This lets us keep regulatory code which is updated on a regulatory
- * basis in userspace. */
+/*
+ * This lets us keep regulatory code which is updated on a regulatory
+ * basis in userspace.
+ */
 static int call_crda(const char *alpha2)
 {
 	char country_env[9 + 2] = "COUNTRY=";
@@ -426,10 +442,12 @@ static bool freq_in_rule_band(const struct ieee80211_freq_range *freq_range,
 #undef ONE_GHZ_IN_KHZ
 }
 
-/* Converts a country IE to a regulatory domain. A regulatory domain
+/*
+ * Converts a country IE to a regulatory domain. A regulatory domain
  * structure has a lot of information which the IE doesn't yet have,
  * so for the other values we use upper max values as we will intersect
- * with our userspace regulatory agent to get lower bounds. */
+ * with our userspace regulatory agent to get lower bounds.
+ */
 static struct ieee80211_regdomain *country_ie_2_rd(
 				u8 *country_ie,
 				u8 country_ie_len,
@@ -474,9 +492,11 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 
 	*checksum ^= ((flags ^ alpha2[0] ^ alpha2[1]) << 8);
 
-	/* We need to build a reg rule for each triplet, but first we must
+	/*
+	 * We need to build a reg rule for each triplet, but first we must
 	 * calculate the number of reg rules we will need. We will need one
-	 * for each channel subband */
+	 * for each channel subband
+	 */
 	while (country_ie_len >= 3) {
 		int end_channel = 0;
 		struct ieee80211_country_ie_triplet *triplet =
@@ -514,9 +534,11 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		if (cur_sub_max_channel < cur_channel)
 			return NULL;
 
-		/* Do not allow overlapping channels. Also channels
+		/*
+		 * Do not allow overlapping channels. Also channels
 		 * passed in each subband must be monotonically
-		 * increasing */
+		 * increasing
+		 */
 		if (last_sub_max_channel) {
 			if (cur_channel <= last_sub_max_channel)
 				return NULL;
@@ -524,10 +546,12 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 				return NULL;
 		}
 
-		/* When dot11RegulatoryClassesRequired is supported
+		/*
+		 * When dot11RegulatoryClassesRequired is supported
 		 * we can throw ext triplets as part of this soup,
 		 * for now we don't care when those change as we
-		 * don't support them */
+		 * don't support them
+		 */
 		*checksum ^= ((cur_channel ^ cur_sub_max_channel) << 8) |
 		  ((cur_sub_max_channel ^ cur_sub_max_channel) << 16) |
 		  ((triplet->chans.max_power ^ cur_sub_max_channel) << 24);
@@ -538,8 +562,10 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		country_ie_len -= 3;
 		num_rules++;
 
-		/* Note: this is not a IEEE requirement but
-		 * simply a memory requirement */
+		/*
+		 * Note: this is not a IEEE requirement but
+		 * simply a memory requirement
+		 */
 		if (num_rules > NL80211_MAX_SUPP_REG_RULES)
 			return NULL;
 	}
@@ -567,8 +593,10 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 		struct ieee80211_freq_range *freq_range = NULL;
 		struct ieee80211_power_rule *power_rule = NULL;
 
-		/* Must parse if dot11RegulatoryClassesRequired is true,
-		 * we don't support this yet */
+		/*
+		 * Must parse if dot11RegulatoryClassesRequired is true,
+		 * we don't support this yet
+		 */
 		if (triplet->ext.reg_extension_id >=
 				IEEE80211_COUNTRY_EXTENSION_ID) {
 			country_ie += 3;
@@ -590,10 +618,12 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 			end_channel =  triplet->chans.first_channel +
 				(4 * (triplet->chans.num_channels - 1));
 
-		/* The +10 is since the regulatory domain expects
+		/*
+		 * The +10 is since the regulatory domain expects
 		 * the actual band edge, not the center of freq for
 		 * its start and end freqs, assuming 20 MHz bandwidth on
-		 * the channels passed */
+		 * the channels passed
+		 */
 		freq_range->start_freq_khz =
 			MHZ_TO_KHZ(ieee80211_channel_to_frequency(
 				triplet->chans.first_channel) - 10);
@@ -601,9 +631,11 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 			MHZ_TO_KHZ(ieee80211_channel_to_frequency(
 				end_channel) + 10);
 
-		/* Large arbitrary values, we intersect later */
-		/* Increment this if we ever support >= 40 MHz channels
-		 * in IEEE 802.11 */
+		/*
+		 * These are large arbitrary values we use to intersect later.
+		 * Increment this if we ever support >= 40 MHz channels
+		 * in IEEE 802.11
+		 */
 		freq_range->max_bandwidth_khz = MHZ_TO_KHZ(40);
 		power_rule->max_antenna_gain = DBI_TO_MBI(100);
 		power_rule->max_eirp = DBM_TO_MBM(100);
@@ -619,8 +651,10 @@ static struct ieee80211_regdomain *country_ie_2_rd(
 }
 
 
-/* Helper for regdom_intersect(), this does the real
- * mathematical intersection fun */
+/*
+ * Helper for regdom_intersect(), this does the real
+ * mathematical intersection fun
+ */
 static int reg_rules_intersect(
 	const struct ieee80211_reg_rule *rule1,
 	const struct ieee80211_reg_rule *rule2,
@@ -698,11 +732,13 @@ static struct ieee80211_regdomain *regdom_intersect(
 	if (!rd1 || !rd2)
 		return NULL;
 
-	/* First we get a count of the rules we'll need, then we actually
+	/*
+	 * First we get a count of the rules we'll need, then we actually
 	 * build them. This is to so we can malloc() and free() a
 	 * regdomain once. The reason we use reg_rules_intersect() here
 	 * is it will return -EINVAL if the rule computed makes no sense.
-	 * All rules that do check out OK are valid. */
+	 * All rules that do check out OK are valid.
+	 */
 
 	for (x = 0; x < rd1->n_reg_rules; x++) {
 		rule1 = &rd1->reg_rules[x];
@@ -730,14 +766,18 @@ static struct ieee80211_regdomain *regdom_intersect(
 		rule1 = &rd1->reg_rules[x];
 		for (y = 0; y < rd2->n_reg_rules; y++) {
 			rule2 = &rd2->reg_rules[y];
-			/* This time around instead of using the stack lets
+			/*
+			 * This time around instead of using the stack lets
 			 * write to the target rule directly saving ourselves
-			 * a memcpy() */
+			 * a memcpy()
+			 */
 			intersected_rule = &rd->reg_rules[rule_idx];
 			r = reg_rules_intersect(rule1, rule2,
 				intersected_rule);
-			/* No need to memset here the intersected rule here as
-			 * we're not using the stack anymore */
+			/*
+			 * No need to memset here the intersected rule here as
+			 * we're not using the stack anymore
+			 */
 			if (r)
 				continue;
 			rule_idx++;
@@ -756,8 +796,10 @@ static struct ieee80211_regdomain *regdom_intersect(
 	return rd;
 }
 
-/* XXX: add support for the rest of enum nl80211_reg_rule_flags, we may
- * want to just have the channel structure use these */
+/*
+ * XXX: add support for the rest of enum nl80211_reg_rule_flags, we may
+ * want to just have the channel structure use these
+ */
 static u32 map_regdom_flags(u32 rd_flags)
 {
 	u32 channel_flags = 0;
@@ -783,8 +825,10 @@ static int freq_reg_info_regd(struct wiphy *wiphy,
 
 	regd = custom_regd ? custom_regd : cfg80211_regdomain;
 
-	/* Follow the driver's regulatory domain, if present, unless a country
-	 * IE has been processed or a user wants to help complaince further */
+	/*
+	 * Follow the driver's regulatory domain, if present, unless a country
+	 * IE has been processed or a user wants to help complaince further
+	 */
 	if (last_request->initiator != REGDOM_SET_BY_COUNTRY_IE &&
 	    last_request->initiator != REGDOM_SET_BY_USER &&
 	    wiphy->regd)
@@ -802,9 +846,11 @@ static int freq_reg_info_regd(struct wiphy *wiphy,
 		fr = &rr->freq_range;
 		pr = &rr->power_rule;
 
-		/* We only need to know if one frequency rule was
+		/*
+		 * We only need to know if one frequency rule was
 		 * was in center_freq's band, that's enough, so lets
-		 * not overwrite it once found */
+		 * not overwrite it once found
+		 */
 		if (!band_rule_found)
 			band_rule_found = freq_in_rule_band(fr, center_freq);
 
@@ -857,7 +903,8 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 		&max_bandwidth, &reg_rule);
 
 	if (r) {
-		/* This means no regulatory rule was found in the country IE
+		/*
+		 * This means no regulatory rule was found in the country IE
 		 * with a frequency range on the center_freq's band, since
 		 * IEEE-802.11 allows for a country IE to have a subset of the
 		 * regulatory information provided in a country we ignore
@@ -876,8 +923,10 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 				chan->center_freq, wiphy_name(wiphy));
 #endif
 		} else {
-		/* In this case we know the country IE has at least one reg rule
-		 * for the band so we respect its band definitions */
+		/*
+		 * In this case we know the country IE has at least one reg rule
+		 * for the band so we respect its band definitions
+		 */
 #ifdef CONFIG_CFG80211_REG_DEBUG
 			if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE)
 				printk(KERN_DEBUG "cfg80211: Disabling "
@@ -896,9 +945,11 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band,
 	if (last_request->initiator == REGDOM_SET_BY_DRIVER &&
 	    request_wiphy && request_wiphy == wiphy &&
 	    request_wiphy->strict_regulatory) {
-		/* This gaurantees the driver's requested regulatory domain
+		/*
+		 * This gaurantees the driver's requested regulatory domain
 		 * will always be used as a base for further regulatory
-		 * settings */
+		 * settings
+		 */
 		chan->flags = chan->orig_flags =
 			map_regdom_flags(reg_rule->flags);
 		chan->max_antenna_gain = chan->orig_mag =
@@ -939,8 +990,10 @@ static bool ignore_reg_update(struct wiphy *wiphy, enum reg_set_by setby)
 	if (setby == REGDOM_SET_BY_CORE &&
 		  wiphy->custom_regulatory)
 		return true;
-	/* wiphy->regd will be set once the device has its own
-	 * desired regulatory domain set */
+	/*
+	 * wiphy->regd will be set once the device has its own
+	 * desired regulatory domain set
+	 */
 	if (wiphy->strict_regulatory && !wiphy->regd &&
 	    !is_world_regdom(last_request->alpha2))
 		return true;
@@ -1050,8 +1103,10 @@ static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd,
 	return 0;
 }
 
-/* Return value which can be used by ignore_request() to indicate
- * it has been determined we should intersect two regulatory domains */
+/*
+ * Return value which can be used by ignore_request() to indicate
+ * it has been determined we should intersect two regulatory domains
+ */
 #define REG_INTERSECT	1
 
 /* This has the logic which determines when a new request
@@ -1091,8 +1146,10 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 					return -EOPNOTSUPP;
 				return -EALREADY;
 			}
-			/* Two consecutive Country IE hints on the same wiphy.
-			 * This should be picked up early by the driver/stack */
+			/*
+			 * Two consecutive Country IE hints on the same wiphy.
+			 * This should be picked up early by the driver/stack
+			 */
 			if (WARN_ON(!alpha2_equal(cfg80211_regdomain->alpha2,
 				  alpha2)))
 				return 0;
@@ -1111,13 +1168,17 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
 	case REGDOM_SET_BY_USER:
 		if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE)
 			return REG_INTERSECT;
-		/* If the user knows better the user should set the regdom
-		 * to their country before the IE is picked up */
+		/*
+		 * If the user knows better the user should set the regdom
+		 * to their country before the IE is picked up
+		 */
 		if (last_request->initiator == REGDOM_SET_BY_USER &&
 			  last_request->intersect)
 			return -EOPNOTSUPP;
-		/* Process user requests only after previous user/driver/core
-		 * requests have been processed */
+		/*
+		 * Process user requests only after previous user/driver/core
+		 * requests have been processed
+		 */
 		if (last_request->initiator == REGDOM_SET_BY_CORE ||
 		    last_request->initiator == REGDOM_SET_BY_DRIVER ||
 		    last_request->initiator == REGDOM_SET_BY_USER) {
@@ -1158,9 +1219,11 @@ int __regulatory_hint(struct wiphy *wiphy, enum reg_set_by set_by,
 		}
 		intersect = true;
 	} else if (r) {
-		/* If the regulatory domain being requested by the
+		/*
+		 * If the regulatory domain being requested by the
 		 * driver has already been set just copy it to the
-		 * wiphy */
+		 * wiphy
+		 */
 		if (r == -EALREADY && set_by == REGDOM_SET_BY_DRIVER) {
 			r = reg_copy_regd(&wiphy->regd, cfg80211_regdomain);
 			if (r)
@@ -1297,9 +1360,11 @@ static bool reg_same_country_ie_hint(struct 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
+	/*
+	 * We should not have let these through at this point, they
 	 * should have been picked up earlier by the first alpha2 check
-	 * on the device */
+	 * on the device
+	 */
 	if (WARN_ON(!country_ie_integrity_changes(country_ie_checksum)))
 		return true;
 	return false;
@@ -1326,9 +1391,11 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	if (country_ie_len < IEEE80211_COUNTRY_IE_MIN_LEN)
 		goto out;
 
-	/* Pending country IE processing, this can happen after we
+	/*
+	 * Pending country IE processing, this can happen after we
 	 * call CRDA and wait for a response if a beacon was received before
-	 * we were able to process the last regulatory_hint_11d() call */
+	 * we were able to process the last regulatory_hint_11d() call
+	 */
 	if (country_ie_regdomain)
 		goto out;
 
@@ -1340,34 +1407,44 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	else if (country_ie[2] == 'O')
 		env = ENVIRON_OUTDOOR;
 
-	/* We will run this for *every* beacon processed for the BSSID, so
+	/*
+	 * 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 */
+	 * do anything
+	 */
 	if (likely(wiphy_idx_valid(last_request->wiphy_idx))) {
 		struct cfg80211_registered_device *drv_last_ie;
 
 		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 */
+		/*
+		 * Lets keep this simple -- we trust the first AP
+		 * after we intersect with CRDA
+		 */
 		if (likely(&drv_last_ie->wiphy == wiphy)) {
-			/* Ignore IEs coming in on this wiphy with
-			 * the same alpha2 and environment cap */
+			/*
+			 * Ignore IEs coming in on this wiphy with
+			 * the same alpha2 and environment cap
+			 */
 			if (likely(alpha2_equal(drv_last_ie->country_ie_alpha2,
 				  alpha2) &&
 				  env == drv_last_ie->env)) {
 				goto out;
 			}
-			/* the wiphy moved on to another BSSID or the AP
+			/*
+			 * the wiphy moved on to another BSSID or the AP
 			 * was reconfigured. XXX: We need to deal with the
 			 * case where the user suspends and goes to goes
 			 * to another country, and then gets IEs from an
-			 * AP with different settings */
+			 * AP with different settings
+			 */
 			goto out;
 		} else {
-			/* Ignore IEs coming in on two separate wiphys with
-			 * the same alpha2 and environment cap */
+			/*
+			 * Ignore IEs coming in on two separate wiphys with
+			 * the same alpha2 and environment cap
+			 */
 			if (likely(alpha2_equal(drv_last_ie->country_ie_alpha2,
 				  alpha2) &&
 				  env == drv_last_ie->env)) {
@@ -1382,18 +1459,22 @@ void regulatory_hint_11d(struct wiphy *wiphy,
 	if (!rd)
 		goto out;
 
-	/* This will not happen right now but we leave it here for the
+	/*
+	 * This will not happen right now but we leave it here for the
 	 * the future when we want to add suspend/resume support and having
 	 * the user move to another country after doing so, or having the user
 	 * move to another AP. Right now we just trust the first AP. This is why
 	 * this is marked as likley(). If we hit this before we add this support
 	 * we want to be informed of it as it would indicate a mistake in the
-	 * current design  */
+	 * current design
+	 */
 	if (likely(WARN_ON(reg_same_country_ie_hint(wiphy, checksum))))
 		goto out;
 
-	/* We keep this around for when CRDA comes back with a response so
-	 * we can intersect with that */
+	/*
+	 * We keep this around for when CRDA comes back with a response so
+	 * we can intersect with that
+	 */
 	country_ie_regdomain = rd;
 
 	__regulatory_hint(wiphy, REGDOM_SET_BY_COUNTRY_IE,
@@ -1419,8 +1500,10 @@ static void print_rd_rules(const struct ieee80211_regdomain *rd)
 		freq_range = &reg_rule->freq_range;
 		power_rule = &reg_rule->power_rule;
 
-		/* There may not be documentation for max antenna gain
-		 * in certain regions */
+		/*
+		 * There may not be documentation for max antenna gain
+		 * in certain regions
+		 */
 		if (power_rule->max_antenna_gain)
 			printk(KERN_INFO "\t(%d KHz - %d KHz @ %d KHz), "
 				"(%d mBi, %d mBm)\n",
@@ -1531,9 +1614,11 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 	if (!last_request)
 		return -EINVAL;
 
-	/* Lets only bother proceeding on the same alpha2 if the current
+	/*
+	 * Lets only bother proceeding on the same alpha2 if the current
 	 * rd is non static (it means CRDA was present and was used last)
-	 * and the pending request came in from a country IE */
+	 * and the pending request came in from a country IE
+	 */
 	if (last_request->initiator != REGDOM_SET_BY_COUNTRY_IE) {
 		/* If someone else asked us to change the rd lets only bother
 		 * checking if the alpha2 changes if CRDA was already called */
@@ -1542,10 +1627,12 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 			return -EINVAL;
 	}
 
-	/* Now lets set the regulatory domain, update all driver channels
+	/*
+	 * 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
-	 * internal EEPROM data */
+	 * internal EEPROM data
+	 */
 
 	if (WARN_ON(!reg_is_valid_request(rd->alpha2)))
 		return -EINVAL;
@@ -1568,8 +1655,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 			return 0;
 		}
 
-		/* For a driver hint, lets copy the regulatory domain the
-		 * driver wanted to the wiphy to deal with conflicts */
+		/*
+		 * For a driver hint, lets copy the regulatory domain the
+		 * driver wanted to the wiphy to deal with conflicts
+		 */
 
 		BUG_ON(request_wiphy->regd);
 
@@ -1590,9 +1679,11 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		if (!intersected_rd)
 			return -EINVAL;
 
-		/* We can trash what CRDA provided now.
+		/*
+		 * We can trash what CRDA provided now.
 		 * However if a driver requested this specific regulatory
-		 * domain we keep it for its private use */
+		 * domain we keep it for its private use
+		 */
 		if (last_request->initiator == REGDOM_SET_BY_DRIVER)
 			request_wiphy->regd = rd;
 		else
@@ -1614,8 +1705,10 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 	BUG_ON(!country_ie_regdomain);
 
 	if (rd != country_ie_regdomain) {
-		/* Intersect what CRDA returned and our what we
-		 * had built from the Country IE received */
+		/*
+		 * Intersect what CRDA returned and our what we
+		 * had built from the Country IE received
+		 */
 
 		intersected_rd = regdom_intersect(rd, country_ie_regdomain);
 
@@ -1625,9 +1718,11 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 		kfree(country_ie_regdomain);
 		country_ie_regdomain = NULL;
 	} else {
-		/* This would happen when CRDA was not present and
+		/*
+		 * This would happen when CRDA was not present and
 		 * OLD_REGULATORY was enabled. We intersect our Country
-		 * IE rd and what was set on cfg80211 originally */
+		 * IE rd and what was set on cfg80211 originally
+		 */
 		intersected_rd = regdom_intersect(rd, cfg80211_regdomain);
 	}
 
@@ -1652,9 +1747,11 @@ static int __set_regdom(const struct ieee80211_regdomain *rd)
 }
 
 
-/* Use this call to set the current regulatory domain. Conflicts with
+/*
+ * 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_mutex */
+ * kmalloc'd the rd structure. Caller must hold cfg80211_mutex
+ */
 int set_regdom(const struct ieee80211_regdomain *rd)
 {
 	int r;
@@ -1763,10 +1860,12 @@ int regulatory_init(void)
 
 	printk(KERN_INFO "cfg80211: Using static regulatory domain info\n");
 	print_regdomain_info(cfg80211_regdomain);
-	/* The old code still requests for a new regdomain and if
+	/*
+	 * The old code still requests for a new regdomain and if
 	 * you have CRDA you get it updated, otherwise you get
 	 * stuck with the static values. We ignore "EU" code as
-	 * that is not a valid ISO / IEC 3166 alpha2 */
+	 * that is not a valid ISO / IEC 3166 alpha2
+	 */
 	if (ieee80211_regdom[0] != 'E' || ieee80211_regdom[1] != 'U')
 		err = regulatory_hint_core(ieee80211_regdom);
 #else
-- 
1.6.1.2.253.ga34a

Re: [PATCH 00/10] cfg80211: move reg hints to workqueue

[Luis R. Rodriguez]

On Fri, Feb 13, 2009 at 09:33:37PM -0800, Luis Rodriguez wrote:
> This series does prep work and then the actual work to move the
> cfg80211 driver and userspace regulatory hints onto a workqueue.
> This also cleans up the core reg hint onto its own routine which
> addresses some of Johannes comments about the branching on the
> assert_cfg80211_mutex(). We also now do a cleanup the comment
> style on reg.c.

I forgot to mention this is now rebased on top of Johannes' two
nl80211 patches:

nl80211: lock rtnl around all operations
genetlink: introduce pre/post methods

  Luis

Re: [PATCH 04/10] nl80211: disallow user requests prior to regulatory_init()

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
> If cfg80211 is built into the kernel there is perhaps a small
> time window betwen nl80211_init() and regulatory_init() where
> cfg80211_regdomain hasn't yet been initialized to let the
> wireless core do its work. During that rare case and time
> frame (if its even possible) we don't allow user regulatory
> changes as cfg80211 is working on enabling its first regulatory
> domain.
> 
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  net/wireless/nl80211.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)
> 
> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
> index db973f2..6f4dedb 100644
> --- a/net/wireless/nl80211.c
> +++ b/net/wireless/nl80211.c
> @@ -1876,6 +1876,15 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info)
>  	int r;
>  	char *data = NULL;
>  
> +	/*
> +	 * You should only get this when cfg80211 hasn't yet initialized
> +	 * completely when built-in to the kernel right between the time
> +	 * window between nl80211_init() and regulatory_init(), if that is
> +	 * even possible.
> +	 */
> +	if (!cfg80211_regdomain)
> +		return -EINPROGRESS;
> +

But that variable access here is racy too. It might be ok anyway because
the variable can never be NULL again after the first assignment, but in
that case the assignment needs to take care to assign something fully
created ...

johannes

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

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
> This will allow us to clean up and make distinctions of who
> needs locking or not.
> 
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  net/wireless/reg.c |   36 +++++++++++++++++++++++++++---------
>  1 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index ba82312..679fded 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -1050,11 +1050,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
>  	case REGDOM_SET_BY_INIT:
>  		return -EINVAL;
>  	case REGDOM_SET_BY_CORE:
> -		/*
> -		 * Always respect new wireless core hints, should only happen
> -		 * when updating the world regulatory domain at init.
> -		 */
> -		return 0;
> +		return -EINVAL;
>  	case REGDOM_SET_BY_COUNTRY_IE:
>  		if (unlikely(!is_an_alpha2(alpha2)))
>  			return -EINVAL;
> @@ -1183,6 +1179,26 @@ new_request:
>  	return call_crda(alpha2);
>  }
>  
> +static int regulatory_hint_core(const char *alpha2)
> +{
> +	struct regulatory_request *request;
> +
> +	BUG_ON(last_request);
> +
> +	request = kzalloc(sizeof(struct regulatory_request),
> +			  GFP_KERNEL);
> +	if (!request)
> +		return -ENOMEM;
> +
> +	request->alpha2[0] = alpha2[0];
> +	request->alpha2[1] = alpha2[1];
> +	request->initiator = REGDOM_SET_BY_CORE;
> +
> +	last_request = request;

So before you documented that cfg80211_mutex is used to protect this
variable, but it's not used here.

johannes

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

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
> Calling kobject_uevent_env() can fail mainly due to out of
> memory conditions. We do not want to continue during such
> conditions so propagate that as well instead of letting
> cfg80211 load as if everything is peachy.
> 
> Additionally lets clarify that when CRDA is not called during
> cfg80211's initialization _and_ if the error is not an -ENOMEM
> its because kobject_uevent_env() failed to call CRDA, not because
> CRDA failed. For those who want to find out why we also let you
> do so by enabling the kernel config CONFIG_CFG80211_REG_DEBUG --
> you'll get an actual stack trace.
> 
> So for now we'll treat non -ENOMEM kobject_uevent_env() failures as
> non fatal during cfg80211's initialization.

I disagree with this patch -- there's so much that can go wrong even if
allocating the message here is ok that imho it's hardly useful to check
for errors here. crda could fail, not be installed, etc.

johannes

> CC: Greg KH <greg@kroah.com>
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
>  net/wireless/reg.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> index 679fded..47d5056 100644
> --- a/net/wireless/reg.c
> +++ b/net/wireless/reg.c
> @@ -1616,7 +1616,7 @@ void reg_device_remove(struct wiphy *wiphy)
>  
>  int regulatory_init(void)
>  {
> -	int err;
> +	int err = 0;
>  
>  	reg_pdev = platform_device_register_simple("regulatory", 0, NULL, 0);
>  	if (IS_ERR(reg_pdev))
> @@ -1637,14 +1637,24 @@ int regulatory_init(void)
>  	cfg80211_regdomain = cfg80211_world_regdom;
>  
>  	err = regulatory_hint_core("00");
> +#endif
>  	if (err) {
>  		if (err == -ENOMEM)
>  			return err;
> -		printk(KERN_ERR "cfg80211: calling CRDA failed - "
> -		       "unable to update world regulatory domain, "
> -		       "using static definition\n");
> -	}
> +		/*
> +		 * N.B. kobject_uevent_env() can fail mainly for when we're out
> +		 * memory which is handled and propagated appropriately above
> +		 * but it can also fail during a netlink_broadcast() or during
> +		 * early boot for call_usermodehelper(). For now treat these
> +		 * errors as non-fatal.
> +		 */
> +		printk(KERN_ERR "cfg80211: kobject_uevent_env() was unable "
> +			"to call CRDA during init");
> +#ifdef CONFIG_CFG80211_REG_DEBUG
> +		/* We want to find out exactly why when debugging */
> +		WARN_ON(err);
>  #endif
> +	}
>  
>  	return 0;
>  }

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

Re: [PATCH 07/10] cfg80211: add assert_cfg80211_lock() to ensure proper protection

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:

> @@ -1122,6 +1128,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) {

still conditional locking?

johannes

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

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:

> +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;
> +}

I neither like the name of this function (same as the variable name) nor
the function -- you're using it in exactly one place where you cannot
ever pass NULL afaict.

johannes

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

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

[Johannes Berg]

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

On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:

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

I wouldn't mind it explaining which list ;)

> +static LIST_HEAD(reg_requests_list);
> +static DEFINE_MUTEX(reg_mutex);
> +
> +static void reg_process_pending_hints(void);

can you reorder the code in a way that doesn't require forward
declarations?

> +/* This currently only processes user and driver regulatory hints */
> +static int reg_process_hint(struct regulatory_request *reg_request)
> +{

why bother offloading _user_ hints to a workqueue? That seems a little
pointless since we can sleep there and do whatever. OTOH, maybe _all_
should be offloaded so the order is correct?

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

pointless warning, it'll happen if somebody just inserts/unplugs very
quickly on a loaded system.

> +		list_del(&reg_request->list);
> +		kfree(reg_request);
> +	}
> +	mutex_unlock(&reg_mutex);
> +}

Are you sure the mutex order reg_mutex --> cfg80211_mutex is a good
idea? Is there a need for two locks at all?

johannes

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

Re: [PATCH 04/10] nl80211: disallow user requests prior to regulatory_init()

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 2:58 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>> If cfg80211 is built into the kernel there is perhaps a small
>> time window betwen nl80211_init() and regulatory_init() where
>> cfg80211_regdomain hasn't yet been initialized to let the
>> wireless core do its work. During that rare case and time
>> frame (if its even possible) we don't allow user regulatory
>> changes as cfg80211 is working on enabling its first regulatory
>> domain.
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  net/wireless/nl80211.c |    9 +++++++++
>>  1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
>> index db973f2..6f4dedb 100644
>> --- a/net/wireless/nl80211.c
>> +++ b/net/wireless/nl80211.c
>> @@ -1876,6 +1876,15 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info)
>>       int r;
>>       char *data = NULL;
>>
>> +     /*
>> +      * You should only get this when cfg80211 hasn't yet initialized
>> +      * completely when built-in to the kernel right between the time
>> +      * window between nl80211_init() and regulatory_init(), if that is
>> +      * even possible.
>> +      */
>> +     if (!cfg80211_regdomain)
>> +             return -EINPROGRESS;
>> +
>
> But that variable access here is racy too.

Right.

> It might be ok anyway because
> the variable can never be NULL again after the first assignment

That's what I was going for.

>  but in
> that case the assignment needs to take care to assign something fully
> created ...

Can you elaborate on what you mean?

  Luis

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 2:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>> This will allow us to clean up and make distinctions of who
>> needs locking or not.
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>  net/wireless/reg.c |   36 +++++++++++++++++++++++++++---------
>>  1 files changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> index ba82312..679fded 100644
>> --- a/net/wireless/reg.c
>> +++ b/net/wireless/reg.c
>> @@ -1050,11 +1050,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
>>       case REGDOM_SET_BY_INIT:
>>               return -EINVAL;
>>       case REGDOM_SET_BY_CORE:
>> -             /*
>> -              * Always respect new wireless core hints, should only happen
>> -              * when updating the world regulatory domain at init.
>> -              */
>> -             return 0;
>> +             return -EINVAL;
>>       case REGDOM_SET_BY_COUNTRY_IE:
>>               if (unlikely(!is_an_alpha2(alpha2)))
>>                       return -EINVAL;
>> @@ -1183,6 +1179,26 @@ new_request:
>>       return call_crda(alpha2);
>>  }
>>
>> +static int regulatory_hint_core(const char *alpha2)
>> +{
>> +     struct regulatory_request *request;
>> +
>> +     BUG_ON(last_request);
>> +
>> +     request = kzalloc(sizeof(struct regulatory_request),
>> +                       GFP_KERNEL);
>> +     if (!request)
>> +             return -ENOMEM;
>> +
>> +     request->alpha2[0] = alpha2[0];
>> +     request->alpha2[1] = alpha2[1];
>> +     request->initiator = REGDOM_SET_BY_CORE;
>> +
>> +     last_request = request;
>
> So before you documented that cfg80211_mutex is used to protect this
> variable, but it's not used here.

This is only used during initialization, have any better ideas?

  Luis

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 3:01 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>> Calling kobject_uevent_env() can fail mainly due to out of
>> memory conditions. We do not want to continue during such
>> conditions so propagate that as well instead of letting
>> cfg80211 load as if everything is peachy.
>>
>> Additionally lets clarify that when CRDA is not called during
>> cfg80211's initialization _and_ if the error is not an -ENOMEM
>> its because kobject_uevent_env() failed to call CRDA, not because
>> CRDA failed. For those who want to find out why we also let you
>> do so by enabling the kernel config CONFIG_CFG80211_REG_DEBUG --
>> you'll get an actual stack trace.
>>
>> So for now we'll treat non -ENOMEM kobject_uevent_env() failures as
>> non fatal during cfg80211's initialization.
>
> I disagree with this patch -- there's so much that can go wrong even if
> allocating the message here is ok that imho it's hardly useful to check
> for errors here.

Huh?? ENOMEM is the only error we are propagating  -- that itself
seems reasonable to propagate.

> crda could fail, not be installed, etc.

This is never propagated to the kernel, that is not what this patch
does. Those failures you mentioned should not prevent cfg80211 from
moving on.

All this patch does is propagate -ENOMEMs.

  Luis

Re: [PATCH 07/10] cfg80211: add assert_cfg80211_lock() to ensure proper protection

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 3:01 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>
>> @@ -1122,6 +1128,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) {
>
> still conditional locking?

WTF, that was a typo the idea with this new series was to remove that.
Sorry I'll resend with that removed.

  Luis

Re: [PATCH 04/10] nl80211: disallow user requests prior to regulatory_init()

[Johannes Berg]

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

On Mon, 2009-02-16 at 00:13 -0800, Luis R. Rodriguez wrote:

> >> +     if (!cfg80211_regdomain)
> >> +             return -EINPROGRESS;
> >> +
> >
> > But that variable access here is racy too.
> 
> Right.
> 
> > It might be ok anyway because
> > the variable can never be NULL again after the first assignment
> 
> That's what I was going for.
> 
> >  but in
> > that case the assignment needs to take care to assign something fully
> > created ...
> 
> Can you elaborate on what you mean?

It'd have to be done in a RCU-like fashion, unless we only ever use it
as a bool here?

johannes

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

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Johannes Berg]

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

On Mon, 2009-02-16 at 00:14 -0800, Luis R. Rodriguez wrote:
> On Sun, Feb 15, 2009 at 2:59 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
> >> This will allow us to clean up and make distinctions of who
> >> needs locking or not.
> >>
> >> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> >> ---
> >>  net/wireless/reg.c |   36 +++++++++++++++++++++++++++---------
> >>  1 files changed, 27 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
> >> index ba82312..679fded 100644
> >> --- a/net/wireless/reg.c
> >> +++ b/net/wireless/reg.c
> >> @@ -1050,11 +1050,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
> >>       case REGDOM_SET_BY_INIT:
> >>               return -EINVAL;
> >>       case REGDOM_SET_BY_CORE:
> >> -             /*
> >> -              * Always respect new wireless core hints, should only happen
> >> -              * when updating the world regulatory domain at init.
> >> -              */
> >> -             return 0;
> >> +             return -EINVAL;
> >>       case REGDOM_SET_BY_COUNTRY_IE:
> >>               if (unlikely(!is_an_alpha2(alpha2)))
> >>                       return -EINVAL;
> >> @@ -1183,6 +1179,26 @@ new_request:
> >>       return call_crda(alpha2);
> >>  }
> >>
> >> +static int regulatory_hint_core(const char *alpha2)
> >> +{
> >> +     struct regulatory_request *request;
> >> +
> >> +     BUG_ON(last_request);
> >> +
> >> +     request = kzalloc(sizeof(struct regulatory_request),
> >> +                       GFP_KERNEL);
> >> +     if (!request)
> >> +             return -ENOMEM;
> >> +
> >> +     request->alpha2[0] = alpha2[0];
> >> +     request->alpha2[1] = alpha2[1];
> >> +     request->initiator = REGDOM_SET_BY_CORE;
> >> +
> >> +     last_request = request;
> >
> > So before you documented that cfg80211_mutex is used to protect this
> > variable, but it's not used here.
> 
> This is only used during initialization, have any better ideas?

If regulatory is initialised before netlink that is probably fine, is
it?

johannes

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

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Johannes Berg]

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

On Mon, 2009-02-16 at 00:18 -0800, Luis R. Rodriguez wrote:

> > I disagree with this patch -- there's so much that can go wrong even if
> > allocating the message here is ok that imho it's hardly useful to check
> > for errors here.
> 
> Huh?? ENOMEM is the only error we are propagating  -- that itself
> seems reasonable to propagate.
> 
> > crda could fail, not be installed, etc.
> 
> This is never propagated to the kernel, that is not what this patch
> does. Those failures you mentioned should not prevent cfg80211 from
> moving on.
> 
> All this patch does is propagate -ENOMEMs.

Yes, all those other possible crda failures do not prevent cfg80211 from
moving on. But being unable to create a uevent could very well also be
treated as a crda failure, and then how is it different from crda
failing in userspace to warrant stopping cfg80211 from moving on?

johannes

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

Re: [PATCH 04/10] nl80211: disallow user requests prior to regulatory_init()

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 12:49 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 00:13 -0800, Luis R. Rodriguez wrote:
>
>> >> +     if (!cfg80211_regdomain)
>> >> +             return -EINPROGRESS;
>> >> +
>> >
>> > But that variable access here is racy too.
>>
>> Right.
>>
>> > It might be ok anyway because
>> > the variable can never be NULL again after the first assignment
>>
>> That's what I was going for.
>>
>> >  but in
>> > that case the assignment needs to take care to assign something fully
>> > created ...
>>
>> Can you elaborate on what you mean?
>
> It'd have to be done in a RCU-like fashion,

I see what you're saying.

> unless we only ever use it
> as a bool here?

Its only used as a bool here, mind you __regulatory_hint() does do
mucking with cfg80211 but my patches move the hint to a workqueue so
that locking is now done later through the workqueue so under this new
patch series -- yes cfg80211_regdomain will only be used as a bool.

But come to think of it this is still racy -- think of the cases where
reset_regdomains() is called and the small race between the end of of
those calls and another CPU getting a new user reg hint. So we should
just lock.

  Luis

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 12:52 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 00:18 -0800, Luis R. Rodriguez wrote:
>
>> > I disagree with this patch -- there's so much that can go wrong even if
>> > allocating the message here is ok that imho it's hardly useful to check
>> > for errors here.
>>
>> Huh?? ENOMEM is the only error we are propagating  -- that itself
>> seems reasonable to propagate.
>>
>> > crda could fail, not be installed, etc.
>>
>> This is never propagated to the kernel, that is not what this patch
>> does. Those failures you mentioned should not prevent cfg80211 from
>> moving on.
>>
>> All this patch does is propagate -ENOMEMs.
>
> Yes, all those other possible crda failures do not prevent cfg80211 from
> moving on. But being unable to create a uevent could very well also be
> treated as a crda failure

We're strictly checking just for ENOMEMs which if you are getting will
get you into problems anyway so might as well propagate that
information.

kobject_uevent_env() can fail for a short set of other non-memory
related issues and those are the cases we are ignoring here including
the possible call to call_usermodehelper() during early boot (which
I'm not even sure how likely it is, Greg?).

>, and then how is it different from crda
> failing in userspace to warrant stopping cfg80211 from moving on?

This is based on the premise that we are simply bailing out for all
conditions on the creation of a udev event but that is not the case --
prior to this we were disregarding -ENOMEMs which I do not believe is
correct. Why wouldn't you want to propagate that?

  Luis

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Johannes Berg]

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

On Mon, 2009-02-16 at 01:09 -0800, Luis R. Rodriguez wrote:

> We're strictly checking just for ENOMEMs which if you are getting will
> get you into problems anyway so might as well propagate that
> information.
> 
> kobject_uevent_env() can fail for a short set of other non-memory
> related issues and those are the cases we are ignoring here including
> the possible call to call_usermodehelper() during early boot (which
> I'm not even sure how likely it is, Greg?).
> 
> >, and then how is it different from crda
> > failing in userspace to warrant stopping cfg80211 from moving on?
> 
> This is based on the premise that we are simply bailing out for all
> conditions on the creation of a udev event but that is not the case --
> prior to this we were disregarding -ENOMEMs which I do not believe is
> correct. Why wouldn't you want to propagate that?

Dunno, it just seems pointless to check for an error condition that
covers so few of the possible ways to fail here.

johannes

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

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 12:50 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 00:14 -0800, Luis R. Rodriguez wrote:
>> On Sun, Feb 15, 2009 at 2:59 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>> >> This will allow us to clean up and make distinctions of who
>> >> needs locking or not.
>> >>
>> >> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> >> ---
>> >>  net/wireless/reg.c |   36 +++++++++++++++++++++++++++---------
>> >>  1 files changed, 27 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/net/wireless/reg.c b/net/wireless/reg.c
>> >> index ba82312..679fded 100644
>> >> --- a/net/wireless/reg.c
>> >> +++ b/net/wireless/reg.c
>> >> @@ -1050,11 +1050,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by,
>> >>       case REGDOM_SET_BY_INIT:
>> >>               return -EINVAL;
>> >>       case REGDOM_SET_BY_CORE:
>> >> -             /*
>> >> -              * Always respect new wireless core hints, should only happen
>> >> -              * when updating the world regulatory domain at init.
>> >> -              */
>> >> -             return 0;
>> >> +             return -EINVAL;
>> >>       case REGDOM_SET_BY_COUNTRY_IE:
>> >>               if (unlikely(!is_an_alpha2(alpha2)))
>> >>                       return -EINVAL;
>> >> @@ -1183,6 +1179,26 @@ new_request:
>> >>       return call_crda(alpha2);
>> >>  }
>> >>
>> >> +static int regulatory_hint_core(const char *alpha2)
>> >> +{
>> >> +     struct regulatory_request *request;
>> >> +
>> >> +     BUG_ON(last_request);
>> >> +
>> >> +     request = kzalloc(sizeof(struct regulatory_request),
>> >> +                       GFP_KERNEL);
>> >> +     if (!request)
>> >> +             return -ENOMEM;
>> >> +
>> >> +     request->alpha2[0] = alpha2[0];
>> >> +     request->alpha2[1] = alpha2[1];
>> >> +     request->initiator = REGDOM_SET_BY_CORE;
>> >> +
>> >> +     last_request = request;
>> >
>> > So before you documented that cfg80211_mutex is used to protect this
>> > variable, but it's not used here.
>>
>> This is only used during initialization, have any better ideas?
>
> If regulatory is initialised before netlink that is probably fine, is
> it?

Yeah I was hoping that would be the case but then I realized that we
also end up creating a udev event to call crda and we need nl80211
initialized in order to process these hints so technically we have a
race between regulatory_init() finishing and nl80211_init() get done
before we can let nl80211 process the first CORE call to crda so it
may be dropped -- unless we are sure udev will do some sort of
retries.

An alternative is to add the regulatory_request() for core to the
queue and then call a reg_post_init() on core.c  that would just
schedule the reg workqueue after nl80211_init().

Thoughts?

  Luis

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Johannes Berg]

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

On Mon, 2009-02-16 at 01:17 -0800, Luis R. Rodriguez wrote:

> >> > So before you documented that cfg80211_mutex is used to protect this
> >> > variable, but it's not used here.
> >>
> >> This is only used during initialization, have any better ideas?
> >
> > If regulatory is initialised before netlink that is probably fine, is
> > it?
> 
> Yeah I was hoping that would be the case but then I realized that we
> also end up creating a udev event to call crda and we need nl80211
> initialized in order to process these hints so technically we have a
> race between regulatory_init() finishing and nl80211_init() get done
> before we can let nl80211 process the first CORE call to crda so it
> may be dropped -- unless we are sure udev will do some sort of
> retries.
> 
> An alternative is to add the regulatory_request() for core to the
> queue and then call a reg_post_init() on core.c  that would just
> schedule the reg workqueue after nl80211_init().
> 
> Thoughts?

Can't you just do the proper locking and initialise regulatory after
nl80211?

johannes

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

Re: [PATCH 06/10] cfg80211: propagate -ENOMEM during regulatory_init()

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 1:15 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 01:09 -0800, Luis R. Rodriguez wrote:
>
>> We're strictly checking just for ENOMEMs which if you are getting will
>> get you into problems anyway so might as well propagate that
>> information.
>>
>> kobject_uevent_env() can fail for a short set of other non-memory
>> related issues and those are the cases we are ignoring here including
>> the possible call to call_usermodehelper() during early boot (which
>> I'm not even sure how likely it is, Greg?).
>>
>> >, and then how is it different from crda
>> > failing in userspace to warrant stopping cfg80211 from moving on?
>>
>> This is based on the premise that we are simply bailing out for all
>> conditions on the creation of a udev event but that is not the case --
>> prior to this we were disregarding -ENOMEMs which I do not believe is
>> correct. Why wouldn't you want to propagate that?
>
> Dunno, it just seems pointless to check for an error condition that
> covers so few of the possible ways to fail here.

RIght -- I hear ya, I am not sure how critical the other failures in
kobject_uevent_env() are compared to -ENOMEM so this is why I didn't
just propagate all errors and since one user already reported
kobject_uevent_env() failing during initialization I was hesitant to
simply let cfg80211 not be loaded because of this routine's failure.

On the other hand, if we let -ENOMEM be propagated I believe it would
be easier to narrow down failures of kobject_uevent_env() during
intialization but I have to understand exactly how common those other
possible failure are and for what reason.

  Luis

Re: [PATCH 05/10] cfg80211: add regulatory_hint_core() to separate the core reg hint

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 1:22 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 01:17 -0800, Luis R. Rodriguez wrote:
>
>> >> > So before you documented that cfg80211_mutex is used to protect this
>> >> > variable, but it's not used here.
>> >>
>> >> This is only used during initialization, have any better ideas?
>> >
>> > If regulatory is initialised before netlink that is probably fine, is
>> > it?
>>
>> Yeah I was hoping that would be the case but then I realized that we
>> also end up creating a udev event to call crda and we need nl80211
>> initialized in order to process these hints so technically we have a
>> race between regulatory_init() finishing and nl80211_init() get done
>> before we can let nl80211 process the first CORE call to crda so it
>> may be dropped -- unless we are sure udev will do some sort of
>> retries.
>>
>> An alternative is to add the regulatory_request() for core to the
>> queue and then call a reg_post_init() on core.c  that would just
>> schedule the reg workqueue after nl80211_init().
>>
>> Thoughts?
>
> Can't you just do the proper locking and initialise regulatory after
> nl80211?

Yes if we figure out a way to not do a mutex_lock() from the module's
init, we cannot call a mutex_lock() from cfg80211_init(). I think
sending core's request to the workqueue is one way -- we'd then have
to schedule the queue towards the end of regulatory_init().

  Luis

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

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 3:10 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>
>> --- 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
>
> I wouldn't mind it explaining which list ;)
>
>> +static LIST_HEAD(reg_requests_list);
>> +static DEFINE_MUTEX(reg_mutex);
>> +
>> +static void reg_process_pending_hints(void);
>
> can you reorder the code in a way that doesn't require forward
> declarations?
>
>> +/* This currently only processes user and driver regulatory hints */
>> +static int reg_process_hint(struct regulatory_request *reg_request)
>> +{
>
> why bother offloading _user_ hints to a workqueue? That seems a little
> pointless since we can sleep there and do whatever.

True, I'd like to use the workqueue for all reg requests though.

> OTOH, maybe _all_
> should be offloaded so the order is correct?

Yeah I think this would be better but regulatory_hint_11d() wasn't so
trivial to move to it so I think as a first step moving these two is a
good start. You do have a good point about the order but since the
only other case that could be out of order is when we process 11d I
don't see it that bad to process a reg hint during association over a
user hint -- for now. Unless that trade off is unacceptable.

>> +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);
>
> pointless warning, it'll happen if somebody just inserts/unplugs very
> quickly on a loaded system.

How about under REG_DEBUG?

>> +             list_del(&reg_request->list);
>> +             kfree(reg_request);
>> +     }
>> +     mutex_unlock(&reg_mutex);
>> +}
>
> Are you sure the mutex order reg_mutex --> cfg80211_mutex is a good
> idea? Is there a need for two locks at all?

I think so. So reg_mutex would prevent any new requests to be added
into the queue, that's the only thing that mutex protects. Once you
are sure you have no one adding requests to the queue you then need to
ensure no one is processing a regulatory request and hence the
cfg80211_mutex.

We need to protect the queue from additions to the queue from having
the workqueue process it.

  Luis

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

[Johannes Berg]

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

On Mon, 2009-02-16 at 01:48 -0800, Luis R. Rodriguez wrote:

> > Are you sure the mutex order reg_mutex --> cfg80211_mutex is a good
> > idea? Is there a need for two locks at all?
> 
> I think so. So reg_mutex would prevent any new requests to be added
> into the queue, that's the only thing that mutex protects. Once you
> are sure you have no one adding requests to the queue you then need to
> ensure no one is processing a regulatory request and hence the
> cfg80211_mutex.
> 
> We need to protect the queue from additions to the queue from having
> the workqueue process it.

Maybe just use a spinlock for the list, and do the loop manually like in
mac80211/key.c then?

        spin_lock(&todo_lock);
        while (!list_empty(&todo_list)) {
                key = list_first_entry(&todo_list, struct ieee80211_key, todo);
                list_del_init(&key->todo);
		spin_unlock(&todo_lock);
........
		spin_lock(&todo_lock);
	}
	spin_unlock(&todo_lock);

johannes

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

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

On Sun, Feb 15, 2009 at 3:04 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2009-02-13 at 21:33 -0800, Luis R. Rodriguez wrote:
>
>> +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;
>> +}
>
> I neither like the name of this function (same as the variable name) nor
> the function -- you're using it in exactly one place where you cannot
> ever pass NULL afaict.

Nope, I'm calling it from 2 places:

* Driver hints which never have the wiphy NULL
* __regulatory_hint: called from 11d hints, and from the workqueue
that processes the pending requests (which can come from userspace or
from drivers right now).

Do you want me to move it into reg.c?

  Luis

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Johannes Berg]

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

On Mon, 2009-02-16 at 01:55 -0800, Luis R. Rodriguez wrote:

> >> +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;
> >> +}
> >
> > I neither like the name of this function (same as the variable name) nor
> > the function -- you're using it in exactly one place where you cannot
> > ever pass NULL afaict.
> 
> Nope, I'm calling it from 2 places:
> 
> * Driver hints which never have the wiphy NULL
> * __regulatory_hint: called from 11d hints, and from the workqueue
> that processes the pending requests (which can come from userspace or
> from drivers right now).

Ok, but in the first case you can't have NULL, and in the second you
could just inline it. It's not an operation that requires an inline here
for it.

johannes

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

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

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 1:55 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 01:48 -0800, Luis R. Rodriguez wrote:
>
>> > Are you sure the mutex order reg_mutex --> cfg80211_mutex is a good
>> > idea? Is there a need for two locks at all?
>>
>> I think so. So reg_mutex would prevent any new requests to be added
>> into the queue, that's the only thing that mutex protects. Once you
>> are sure you have no one adding requests to the queue you then need to
>> ensure no one is processing a regulatory request and hence the
>> cfg80211_mutex.
>>
>> We need to protect the queue from additions to the queue from having
>> the workqueue process it.
>
> Maybe just use a spinlock for the list, and do the loop manually like in
> mac80211/key.c then?
>
>        spin_lock(&todo_lock);
>        while (!list_empty(&todo_list)) {
>                key = list_first_entry(&todo_list, struct ieee80211_key, todo);
>                list_del_init(&key->todo);
>                spin_unlock(&todo_lock);
> ........
>                spin_lock(&todo_lock);
>        }
>        spin_unlock(&todo_lock);

Sure, is the benefit you see that we won't contend userspace longer if
the workqueue is busy?

  Luis

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 2:02 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 01:55 -0800, Luis R. Rodriguez wrote:
>
>> >> +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;
>> >> +}
>> >
>> > I neither like the name of this function (same as the variable name) nor
>> > the function -- you're using it in exactly one place where you cannot
>> > ever pass NULL afaict.
>>
>> Nope, I'm calling it from 2 places:
>>
>> * Driver hints which never have the wiphy NULL
>> * __regulatory_hint: called from 11d hints, and from the workqueue
>> that processes the pending requests (which can come from userspace or
>> from drivers right now).
>
> Ok, but in the first case you can't have NULL, and in the second you
> could just inline it. It's not an operation that requires an inline here
> for it.

Sure, its just called twice and I think it makes the code more
readable to use a routine, what if I just move it to core.c or reg.c.

  Luis

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

[Johannes Berg]

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

On Mon, 2009-02-16 at 02:09 -0800, Luis R. Rodriguez wrote:

> > Maybe just use a spinlock for the list, and do the loop manually like in
> > mac80211/key.c then?
> >
> >        spin_lock(&todo_lock);
> >        while (!list_empty(&todo_list)) {
> >                key = list_first_entry(&todo_list, struct ieee80211_key, todo);
> >                list_del_init(&key->todo);
> >                spin_unlock(&todo_lock);
> > ........
> >                spin_lock(&todo_lock);
> >        }
> >        spin_unlock(&todo_lock);
> 
> Sure, is the benefit you see that we won't contend userspace longer if
> the workqueue is busy?

No, just that it's simpler because we truly only use the spinlock for
list accesses rather than holding a reg_mutex around the cfg80211 mutex.

Thus we can queue things without needing the cfg80211 mutex, but dequeue
needs to run under it.

johannes

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

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Johannes Berg]

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

On Mon, 2009-02-16 at 02:11 -0800, Luis R. Rodriguez wrote:

> >> * Driver hints which never have the wiphy NULL
> >> * __regulatory_hint: called from 11d hints, and from the workqueue
> >> that processes the pending requests (which can come from userspace or
> >> from drivers right now).
> >
> > Ok, but in the first case you can't have NULL, and in the second you
> > could just inline it. It's not an operation that requires an inline here
> > for it.
> 
> Sure, its just called twice

Yeah but the first call is entirely pointless since wiphy cannot be NULL
there. I don't want to encourage random error-checking accessors where
writing wiphy->wiphy_idx is perfectly fine.

>  and I think it makes the code more
> readable to use a routine, what if I just move it to core.c or reg.c.

If you really think it makes the code that much more readable then
please just use it once in the case where a NULL argument can happen
(the workqueue), and move it to that code.

johannes

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

Re: [PATCH 08/10] cfg80211: make regulatory_request use wiphy_idx instead of wiphy

[Luis R. Rodriguez]

On Mon, Feb 16, 2009 at 2:20 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2009-02-16 at 02:11 -0800, Luis R. Rodriguez wrote:
>
>> >> * Driver hints which never have the wiphy NULL
>> >> * __regulatory_hint: called from 11d hints, and from the workqueue
>> >> that processes the pending requests (which can come from userspace or
>> >> from drivers right now).
>> >
>> > Ok, but in the first case you can't have NULL, and in the second you
>> > could just inline it. It's not an operation that requires an inline here
>> > for it.
>>
>> Sure, its just called twice
>
> Yeah but the first call is entirely pointless since wiphy cannot be NULL
> there. I don't want to encourage random error-checking accessors where
> writing wiphy->wiphy_idx is perfectly fine.

That's the thing wiphy does not have a wiphy_idx, the
cfg80211_registered_device does.

>>  and I think it makes the code more
>> readable to use a routine, what if I just move it to core.c or reg.c.
>
> If you really think it makes the code that much more readable then
> please just use it once in the case where a NULL argument can happen
> (the workqueue), and move it to that code.

OK

  Luis