* [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints @ 2009-02-13 5:35 Luis R. Rodriguez 2009-02-13 5:35 ` [PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx Luis R. Rodriguez 2009-02-13 9:13 ` [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Johannes Berg 0 siblings, 2 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:35 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless This series does prep work and then the actual work to move the cfg80211 driver and userspace regulatory hints onto a workqueue. It should cure some lockdep warnings and also pave the way for more future reg work like beacon reg hints. Luis R. Rodriguez (6): cfg80211: rename cfg80211_registered_device's idx to wiphy_idx cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex cfg80211: add assert_cfg80211_lock() to ensure proper protection cfg80211: make regulatory_request use wiphy_idx instead of wiphy cfg80211: move regulatory hints to workqueue drivers/net/wireless/ath9k/main.c | 8 +- drivers/net/wireless/zd1211rw/zd_mac.c | 6 +- include/net/cfg80211.h | 8 +- include/net/wireless.h | 9 +- net/wireless/core.c | 87 ++++++++----- net/wireless/core.h | 42 +++++- net/wireless/nl80211.c | 33 ++--- net/wireless/reg.c | 240 ++++++++++++++++++++++++++------ net/wireless/reg.h | 2 + net/wireless/sysfs.c | 2 +- 10 files changed, 332 insertions(+), 105 deletions(-) ^ permalink raw reply [flat|nested] 27+ messages in thread
* [PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx 2009-02-13 5:35 [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Luis R. Rodriguez @ 2009-02-13 5:35 ` Luis R. Rodriguez 2009-02-13 5:35 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Luis R. Rodriguez 2009-02-13 9:13 ` [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Johannes Berg 1 sibling, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:35 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless Makes it clearer to read when comparing to ifidx Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- net/wireless/core.c | 35 ++++++++++++++++++----------------- net/wireless/core.h | 2 +- net/wireless/nl80211.c | 4 ++-- net/wireless/sysfs.c | 2 +- 4 files changed, 22 insertions(+), 21 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index 0668b2b..2b3e786 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -37,12 +37,13 @@ DEFINE_MUTEX(cfg80211_drv_mutex); static struct dentry *ieee80211_debugfs_dir; /* requires cfg80211_drv_mutex to be held! */ -static struct cfg80211_registered_device *cfg80211_drv_by_wiphy(int wiphy) +static struct cfg80211_registered_device * +cfg80211_drv_by_wiphy_idx(int wiphy_idx) { struct cfg80211_registered_device *result = NULL, *drv; list_for_each_entry(drv, &cfg80211_drv_list, list) { - if (drv->idx == wiphy) { + if (drv->wiphy_idx == wiphy_idx) { result = drv; break; } @@ -56,12 +57,12 @@ static struct cfg80211_registered_device * __cfg80211_drv_from_info(struct genl_info *info) { int ifindex; - struct cfg80211_registered_device *bywiphy = NULL, *byifidx = NULL; + struct cfg80211_registered_device *bywiphyidx = NULL, *byifidx = NULL; struct net_device *dev; int err = -EINVAL; if (info->attrs[NL80211_ATTR_WIPHY]) { - bywiphy = cfg80211_drv_by_wiphy( + bywiphyidx = cfg80211_drv_by_wiphy_idx( nla_get_u32(info->attrs[NL80211_ATTR_WIPHY])); err = -ENODEV; } @@ -78,14 +79,14 @@ __cfg80211_drv_from_info(struct genl_info *info) err = -ENODEV; } - if (bywiphy && byifidx) { - if (bywiphy != byifidx) + if (bywiphyidx && byifidx) { + if (bywiphyidx != byifidx) return ERR_PTR(-EINVAL); else - return bywiphy; /* == byifidx */ + return bywiphyidx; /* == byifidx */ } - if (bywiphy) - return bywiphy; + if (bywiphyidx) + return bywiphyidx; if (byifidx) return byifidx; @@ -143,16 +144,16 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, char *newname) { struct cfg80211_registered_device *drv; - int idx, taken = -1, result, digits; + int wiphy_idx, taken = -1, result, digits; mutex_lock(&cfg80211_drv_mutex); /* prohibit calling the thing phy%d when %d is not its number */ - sscanf(newname, PHY_NAME "%d%n", &idx, &taken); - if (taken == strlen(newname) && idx != rdev->idx) { - /* count number of places needed to print idx */ + sscanf(newname, PHY_NAME "%d%n", &wiphy_idx, &taken); + if (taken == strlen(newname) && wiphy_idx != rdev->wiphy_idx) { + /* count number of places needed to print wiphy_idx */ digits = 1; - while (idx /= 10) + while (wiphy_idx /= 10) digits++; /* * deny the name if it is phy<idx> where <idx> is printed @@ -222,9 +223,9 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv) mutex_lock(&cfg80211_drv_mutex); - drv->idx = wiphy_counter++; + drv->wiphy_idx = wiphy_counter++; - if (unlikely(drv->idx < 0)) { + if (unlikely(drv->wiphy_idx < 0)) { wiphy_counter--; mutex_unlock(&cfg80211_drv_mutex); /* ugh, wrapped! */ @@ -235,7 +236,7 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv) mutex_unlock(&cfg80211_drv_mutex); /* give it a proper name */ - dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->idx); + dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->wiphy_idx); mutex_init(&drv->mtx); mutex_init(&drv->devlist_mtx); diff --git a/net/wireless/core.h b/net/wireless/core.h index e29ad4c..36e2397 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -37,7 +37,7 @@ struct cfg80211_registered_device { enum environment_cap env; /* wiphy index, internal only */ - int idx; + int wiphy_idx; /* associate netdev list */ struct mutex devlist_mtx; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 298a4de..47018bd 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -142,7 +142,7 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags, if (!hdr) return -1; - NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, dev->idx); + NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, dev->wiphy_idx); NLA_PUT_STRING(msg, NL80211_ATTR_WIPHY_NAME, wiphy_name(&dev->wiphy)); NLA_PUT_U8(msg, NL80211_ATTR_MAX_NUM_SCAN_SSIDS, dev->wiphy.max_scan_ssids); @@ -2739,7 +2739,7 @@ static int nl80211_send_scan_donemsg(struct sk_buff *msg, if (!hdr) return -1; - NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, rdev->idx); + NLA_PUT_U32(msg, NL80211_ATTR_WIPHY, rdev->wiphy_idx); NLA_PUT_U32(msg, NL80211_ATTR_IFINDEX, netdev->ifindex); /* XXX: we should probably bounce back the request? */ diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c index 26a72b0..2e3fd91 100644 --- a/net/wireless/sysfs.c +++ b/net/wireless/sysfs.c @@ -31,7 +31,7 @@ static ssize_t name ## _show(struct device *dev, \ return sprintf(buf, fmt "\n", dev_to_rdev(dev)->member); \ } -SHOW_FMT(index, "%d", idx); +SHOW_FMT(index, "%d", wiphy_idx); SHOW_FMT(macaddress, "%pM", wiphy.perm_addr); static struct device_attribute ieee80211_dev_attrs[] = { -- 1.6.1.2.253.ga34a ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity 2009-02-13 5:35 ` [PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx Luis R. Rodriguez @ 2009-02-13 5:35 ` Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex Luis R. Rodriguez 2009-02-13 6:53 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Johannes Berg 0 siblings, 2 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:35 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless This will later be used by others. Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- net/wireless/core.c | 2 +- net/wireless/core.h | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index 2b3e786..55be3b0 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -225,7 +225,7 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv) drv->wiphy_idx = wiphy_counter++; - if (unlikely(drv->wiphy_idx < 0)) { + if (!wiphy_idx_valid(drv->wiphy_idx)) { wiphy_counter--; mutex_unlock(&cfg80211_drv_mutex); /* ugh, wrapped! */ diff --git a/net/wireless/core.h b/net/wireless/core.h index 36e2397..e24bc2a 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -62,6 +62,13 @@ struct cfg80211_registered_device *wiphy_to_dev(struct wiphy *wiphy) return container_of(wiphy, struct cfg80211_registered_device, wiphy); } +/* Note 0 is valid, hence phy0 */ +static inline +bool wiphy_idx_valid(int wiphy_idx) +{ + return (likely(wiphy_idx >= 0)); +} + extern struct mutex cfg80211_drv_mutex; extern struct list_head cfg80211_drv_list; -- 1.6.1.2.253.ga34a ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex 2009-02-13 5:35 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Luis R. Rodriguez @ 2009-02-13 5:36 ` Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Luis R. Rodriguez 2009-02-13 6:53 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Johannes Berg 1 sibling, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:36 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless cfg80211_drv_mutex is protecting more than the driver list, this renames it and documents what its currently supposed to protect. Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- net/wireless/core.c | 35 ++++++++++++++++++++--------------- net/wireless/core.h | 6 +++--- net/wireless/nl80211.c | 20 ++++++++++---------- net/wireless/reg.c | 18 +++++++++--------- 4 files changed, 42 insertions(+), 37 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index 55be3b0..f55040a 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -31,7 +31,12 @@ MODULE_DESCRIPTION("wireless configuration support"); * only read the list, and that can happen quite * often because we need to do it for each command */ LIST_HEAD(cfg80211_drv_list); -DEFINE_MUTEX(cfg80211_drv_mutex); + +/* + * This is used to protect the cfg80211_drv_list, cfg80211_regdomain, and + * the last reguluatory request receipt in regd.c + */ +DEFINE_MUTEX(cfg80211_mutex); /* for debugfs */ static struct dentry *ieee80211_debugfs_dir; @@ -52,7 +57,7 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx) return result; } -/* requires cfg80211_drv_mutex to be held! */ +/* requires cfg80211_mutex to be held! */ static struct cfg80211_registered_device * __cfg80211_drv_from_info(struct genl_info *info) { @@ -99,7 +104,7 @@ cfg80211_get_dev_from_info(struct genl_info *info) { struct cfg80211_registered_device *drv; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); drv = __cfg80211_drv_from_info(info); /* if it is not an error we grab the lock on @@ -108,7 +113,7 @@ cfg80211_get_dev_from_info(struct genl_info *info) if (!IS_ERR(drv)) mutex_lock(&drv->mtx); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); return drv; } @@ -119,7 +124,7 @@ cfg80211_get_dev_from_ifindex(int ifindex) struct cfg80211_registered_device *drv = ERR_PTR(-ENODEV); struct net_device *dev; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); dev = dev_get_by_index(&init_net, ifindex); if (!dev) goto out; @@ -130,7 +135,7 @@ cfg80211_get_dev_from_ifindex(int ifindex) drv = ERR_PTR(-ENODEV); dev_put(dev); out: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); return drv; } @@ -146,7 +151,7 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, struct cfg80211_registered_device *drv; int wiphy_idx, taken = -1, result, digits; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); /* prohibit calling the thing phy%d when %d is not its number */ sscanf(newname, PHY_NAME "%d%n", &wiphy_idx, &taken); @@ -194,7 +199,7 @@ int cfg80211_dev_rename(struct cfg80211_registered_device *rdev, result = 0; out_unlock: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); if (result == 0) nl80211_notify_dev_rename(rdev); @@ -221,19 +226,19 @@ struct wiphy *wiphy_new(struct cfg80211_ops *ops, int sizeof_priv) drv->ops = ops; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); drv->wiphy_idx = wiphy_counter++; if (!wiphy_idx_valid(drv->wiphy_idx)) { wiphy_counter--; - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); /* ugh, wrapped! */ kfree(drv); return NULL; } - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); /* give it a proper name */ dev_set_name(&drv->wiphy.dev, PHY_NAME "%d", drv->wiphy_idx); @@ -311,7 +316,7 @@ int wiphy_register(struct wiphy *wiphy) /* check and set up bitrates */ ieee80211_set_bitrate_flags(wiphy); - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); /* set up regulatory info */ wiphy_update_regulatory(wiphy, REGDOM_SET_BY_CORE); @@ -331,7 +336,7 @@ int wiphy_register(struct wiphy *wiphy) res = 0; out_unlock: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); return res; } EXPORT_SYMBOL(wiphy_register); @@ -341,7 +346,7 @@ void wiphy_unregister(struct wiphy *wiphy) struct cfg80211_registered_device *drv = wiphy_to_dev(wiphy); /* protect the device list */ - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); BUG_ON(!list_empty(&drv->netdev_list)); @@ -367,7 +372,7 @@ void wiphy_unregister(struct wiphy *wiphy) device_del(&drv->wiphy.dev); debugfs_remove(drv->wiphy.debugfsdir); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); } EXPORT_SYMBOL(wiphy_unregister); diff --git a/net/wireless/core.h b/net/wireless/core.h index e24bc2a..ba0531e 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -69,7 +69,7 @@ bool wiphy_idx_valid(int wiphy_idx) return (likely(wiphy_idx >= 0)); } -extern struct mutex cfg80211_drv_mutex; +extern struct mutex cfg80211_mutex; extern struct list_head cfg80211_drv_list; struct cfg80211_internal_bss { @@ -88,13 +88,13 @@ struct cfg80211_internal_bss { * the driver's mutex! * * This means that you need to call cfg80211_put_dev() - * before being allowed to acquire &cfg80211_drv_mutex! + * before being allowed to acquire &cfg80211_mutex! * * This is necessary because we need to lock the global * mutex to get an item off the list safely, and then * we lock the drv mutex so it doesn't go away under us. * - * We don't want to keep cfg80211_drv_mutex locked + * We don't want to keep cfg80211_mutex locked * for all the time in order to allow requests on * other interfaces to go through at the same time. * diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 47018bd..f5c2c05 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -256,7 +256,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb) int start = cb->args[0]; struct cfg80211_registered_device *dev; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); list_for_each_entry(dev, &cfg80211_drv_list, list) { if (++idx <= start) continue; @@ -267,7 +267,7 @@ static int nl80211_dump_wiphy(struct sk_buff *skb, struct netlink_callback *cb) break; } } - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); cb->args[0] = idx; @@ -470,7 +470,7 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback * struct cfg80211_registered_device *dev; struct wireless_dev *wdev; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); list_for_each_entry(dev, &cfg80211_drv_list, list) { if (wp_idx < wp_start) { wp_idx++; @@ -497,7 +497,7 @@ static int nl80211_dump_interface(struct sk_buff *skb, struct netlink_callback * wp_idx++; } out: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); cb->args[0] = wp_idx; cb->args[1] = if_idx; @@ -1910,9 +1910,9 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info) if (is_world_regdom(data)) return -EINVAL; #endif - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); r = __regulatory_hint(NULL, REGDOM_SET_BY_USER, data, 0, ENVIRON_ANY); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); /* This means the regulatory domain was already set, however * we don't want to confuse userspace with a "successful error" * message so lets just treat it as a success */ @@ -2106,7 +2106,7 @@ static int nl80211_get_reg(struct sk_buff *skb, struct genl_info *info) unsigned int i; int err = -EINVAL; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); if (!cfg80211_regdomain) goto out; @@ -2169,7 +2169,7 @@ nla_put_failure: genlmsg_cancel(msg, hdr); err = -EMSGSIZE; out: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); return err; } @@ -2228,9 +2228,9 @@ static int nl80211_set_reg(struct sk_buff *skb, struct genl_info *info) BUG_ON(rule_idx != num_rules); - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); r = set_regdom(rd); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); return r; bad_reg: diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 2323644..ba82312 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -1116,7 +1116,7 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by, return -EINVAL; } -/* Caller must hold &cfg80211_drv_mutex */ +/* Caller must hold &cfg80211_mutex */ int __regulatory_hint(struct wiphy *wiphy, enum reg_set_by set_by, const char *alpha2, u32 country_ie_checksum, @@ -1188,13 +1188,13 @@ void regulatory_hint(struct wiphy *wiphy, const char *alpha2) int r; BUG_ON(!alpha2); - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); r = __regulatory_hint(wiphy, REGDOM_SET_BY_DRIVER, alpha2, 0, ENVIRON_ANY); /* This is required so that the orig_* parameters are saved */ if (r == -EALREADY && wiphy->strict_regulatory) wiphy_update_regulatory(wiphy, REGDOM_SET_BY_DRIVER); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); } EXPORT_SYMBOL(regulatory_hint); @@ -1225,7 +1225,7 @@ void regulatory_hint_11d(struct wiphy *wiphy, if (!last_request) return; - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); /* IE len must be evenly divisible by 2 */ if (country_ie_len & 0x01) @@ -1307,7 +1307,7 @@ void regulatory_hint_11d(struct wiphy *wiphy, country_ie_regdomain->alpha2, checksum, env); out: - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); } EXPORT_SYMBOL(regulatory_hint_11d); @@ -1562,7 +1562,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) /* Use this call to set the current regulatory domain. Conflicts with * multiple drivers can be ironed out later. Caller must've already - * kmalloc'd the rd structure. Caller must hold cfg80211_drv_mutex */ + * kmalloc'd the rd structure. Caller must hold cfg80211_mutex */ int set_regdom(const struct ieee80211_regdomain *rd) { int r; @@ -1586,7 +1586,7 @@ int set_regdom(const struct ieee80211_regdomain *rd) return r; } -/* Caller must hold cfg80211_drv_mutex */ +/* Caller must hold cfg80211_mutex */ void reg_device_remove(struct wiphy *wiphy) { kfree(wiphy->regd); @@ -1633,7 +1633,7 @@ int regulatory_init(void) void regulatory_exit(void) { - mutex_lock(&cfg80211_drv_mutex); + mutex_lock(&cfg80211_mutex); reset_regdomains(); @@ -1644,5 +1644,5 @@ void regulatory_exit(void) platform_device_unregister(reg_pdev); - mutex_unlock(&cfg80211_drv_mutex); + mutex_unlock(&cfg80211_mutex); } -- 1.6.1.2.253.ga34a ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection 2009-02-13 5:36 ` [PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex Luis R. Rodriguez @ 2009-02-13 5:36 ` Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Luis R. Rodriguez 2009-02-13 6:54 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Johannes Berg 0 siblings, 2 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:36 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- net/wireless/core.c | 5 ++++- net/wireless/core.h | 6 ++++++ net/wireless/nl80211.c | 3 ++- net/wireless/reg.c | 15 +++++++++++++++ 4 files changed, 27 insertions(+), 2 deletions(-) diff --git a/net/wireless/core.c b/net/wireless/core.c index f55040a..6807538 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -7,7 +7,6 @@ #include <linux/if.h> #include <linux/module.h> #include <linux/err.h> -#include <linux/mutex.h> #include <linux/list.h> #include <linux/nl80211.h> #include <linux/debugfs.h> @@ -47,6 +46,8 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx) { struct cfg80211_registered_device *result = NULL, *drv; + assert_cfg80211_lock(); + list_for_each_entry(drv, &cfg80211_drv_list, list) { if (drv->wiphy_idx == wiphy_idx) { result = drv; @@ -66,6 +67,8 @@ __cfg80211_drv_from_info(struct genl_info *info) struct net_device *dev; int err = -EINVAL; + assert_cfg80211_lock(); + if (info->attrs[NL80211_ATTR_WIPHY]) { bywiphyidx = cfg80211_drv_by_wiphy_idx( nla_get_u32(info->attrs[NL80211_ATTR_WIPHY])); diff --git a/net/wireless/core.h b/net/wireless/core.h index ba0531e..2500dbc 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -10,6 +10,7 @@ #include <linux/netdevice.h> #include <linux/kref.h> #include <linux/rbtree.h> +#include <linux/mutex.h> #include <net/genetlink.h> #include <net/wireless.h> #include <net/cfg80211.h> @@ -72,6 +73,11 @@ bool wiphy_idx_valid(int wiphy_idx) extern struct mutex cfg80211_mutex; extern struct list_head cfg80211_drv_list; +static inline void assert_cfg80211_lock(void) +{ + BUG_ON(!mutex_is_locked(&cfg80211_mutex)); +} + struct cfg80211_internal_bss { struct list_head list; struct rb_node rbn; diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index f5c2c05..843d26d 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -7,7 +7,6 @@ #include <linux/if.h> #include <linux/module.h> #include <linux/err.h> -#include <linux/mutex.h> #include <linux/list.h> #include <linux/if_ether.h> #include <linux/ieee80211.h> @@ -138,6 +137,8 @@ static int nl80211_send_wiphy(struct sk_buff *msg, u32 pid, u32 seq, int flags, int i; u16 ifmodes = dev->wiphy.interface_modes; + assert_cfg80211_lock(); + hdr = nl80211hdr_put(msg, pid, seq, flags, NL80211_CMD_NEW_WIPHY); if (!hdr) return -1; diff --git a/net/wireless/reg.c b/net/wireless/reg.c index ba82312..aab1c69 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -830,6 +830,8 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band, struct ieee80211_supported_band *sband; struct ieee80211_channel *chan; + assert_cfg80211_lock(); + sband = wiphy->bands[band]; BUG_ON(chan_idx >= sband->n_channels); chan = &sband->channels[chan_idx]; @@ -1042,6 +1044,10 @@ static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd, static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by, const char *alpha2) { + + if (set_by != REGDOM_SET_BY_CORE) + assert_cfg80211_lock(); + /* All initial requests are respected */ if (!last_request) return 0; @@ -1126,6 +1132,9 @@ int __regulatory_hint(struct wiphy *wiphy, enum reg_set_by set_by, bool intersect = false; int r = 0; + if (set_by != REGDOM_SET_BY_CORE) + assert_cfg80211_lock(); + r = ignore_request(wiphy, set_by, alpha2); if (r == REG_INTERSECT) { @@ -1201,6 +1210,8 @@ EXPORT_SYMBOL(regulatory_hint); static bool reg_same_country_ie_hint(struct wiphy *wiphy, u32 country_ie_checksum) { + assert_cfg80211_lock(); + if (!last_request->wiphy) return false; if (likely(last_request->wiphy != wiphy)) @@ -1567,6 +1578,8 @@ int set_regdom(const struct ieee80211_regdomain *rd) { int r; + assert_cfg80211_lock(); + /* Note that this doesn't update the wiphys, this is done below */ r = __set_regdom(rd); if (r) { @@ -1589,6 +1602,8 @@ int set_regdom(const struct ieee80211_regdomain *rd) /* Caller must hold cfg80211_mutex */ void reg_device_remove(struct wiphy *wiphy) { + assert_cfg80211_lock(); + kfree(wiphy->regd); if (!last_request || !last_request->wiphy) return; -- 1.6.1.2.253.ga34a ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy 2009-02-13 5:36 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Luis R. Rodriguez @ 2009-02-13 5:36 ` Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 6/6] cfg80211: move regulatory hints to workqueue Luis R. Rodriguez 2009-02-13 11:04 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Johannes Berg 2009-02-13 6:54 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Johannes Berg 1 sibling, 2 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:36 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless We do this so later on we can move the pending requests onto a workqueue. By using the wiphy_idx instead of the wiphy we can later easily check if the wiphy has disappeared or not. Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- include/net/cfg80211.h | 6 ++-- net/wireless/core.c | 18 ++++++++++- net/wireless/core.h | 21 ++++++++++++++ net/wireless/reg.c | 73 ++++++++++++++++++++++++++++++++---------------- 4 files changed, 89 insertions(+), 29 deletions(-) diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index c0d1f5b..2a83510 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -375,9 +375,9 @@ enum environment_cap { }; /** - * struct regulatory_request - receipt of last regulatory request + * struct regulatory_request - used to keep track of regulatory requests * - * @wiphy: this is set if this request's initiator is + * @wiphy_idx: this is set if this request's initiator is * %REGDOM_SET_BY_COUNTRY_IE or %REGDOM_SET_BY_DRIVER. This * can be used by the wireless core to deal with conflicts * and potentially inform users of which devices specifically @@ -398,7 +398,7 @@ enum environment_cap { * indoor, or if it doesn't matter */ struct regulatory_request { - struct wiphy *wiphy; + int wiphy_idx; enum reg_set_by initiator; char alpha2[2]; bool intersect; diff --git a/net/wireless/core.c b/net/wireless/core.c index 6807538..2024664 100644 --- a/net/wireless/core.c +++ b/net/wireless/core.c @@ -41,13 +41,14 @@ DEFINE_MUTEX(cfg80211_mutex); static struct dentry *ieee80211_debugfs_dir; /* requires cfg80211_drv_mutex to be held! */ -static struct cfg80211_registered_device * -cfg80211_drv_by_wiphy_idx(int wiphy_idx) +struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx) { struct cfg80211_registered_device *result = NULL, *drv; assert_cfg80211_lock(); + WARN_ON(!wiphy_idx_valid(wiphy_idx)); + list_for_each_entry(drv, &cfg80211_drv_list, list) { if (drv->wiphy_idx == wiphy_idx) { result = drv; @@ -58,6 +59,19 @@ cfg80211_drv_by_wiphy_idx(int wiphy_idx) return result; } +/* requires cfg80211_drv_mutex to be held! */ +struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx) +{ + struct cfg80211_registered_device *drv; + + assert_cfg80211_lock(); + + drv = cfg80211_drv_by_wiphy_idx(wiphy_idx); + if (!drv) + return NULL; + return &drv->wiphy; +} + /* requires cfg80211_mutex to be held! */ static struct cfg80211_registered_device * __cfg80211_drv_from_info(struct genl_info *info) diff --git a/net/wireless/core.h b/net/wireless/core.h index 2500dbc..df1e9ff 100644 --- a/net/wireless/core.h +++ b/net/wireless/core.h @@ -78,6 +78,22 @@ static inline void assert_cfg80211_lock(void) BUG_ON(!mutex_is_locked(&cfg80211_mutex)); } +/* + * You can use this to mark a wiphy_idx as not having an associated wiphy. + * It guarantees cfg80211_drv_by_wiphy_idx(wiphy_idx) will return NULL + */ +#define WIPHY_IDX_STALE -1 + +static inline +int wiphy_idx(struct wiphy *wiphy) +{ + struct cfg80211_registered_device *drv; + if (!wiphy) + return WIPHY_IDX_STALE; + drv = wiphy_to_dev(wiphy); + return drv->wiphy_idx; +} + struct cfg80211_internal_bss { struct list_head list; struct rb_node rbn; @@ -87,6 +103,8 @@ struct cfg80211_internal_bss { struct cfg80211_bss pub; }; +struct cfg80211_registered_device *cfg80211_drv_by_wiphy_idx(int wiphy_idx); + /* * This function returns a pointer to the driver * that the genl_info item that is passed refers to. @@ -110,6 +128,9 @@ struct cfg80211_internal_bss { extern struct cfg80211_registered_device * cfg80211_get_dev_from_info(struct genl_info *info); +/* requires cfg80211_drv_mutex to be held! */ +struct wiphy *wiphy_idx_to_wiphy(int wiphy_idx); + /* identical to cfg80211_get_dev_from_info but only operate on ifindex */ extern struct cfg80211_registered_device * cfg80211_get_dev_from_ifindex(int ifindex); diff --git a/net/wireless/reg.c b/net/wireless/reg.c index aab1c69..316b2e4 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -829,9 +829,13 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band, const struct ieee80211_power_rule *power_rule = NULL; struct ieee80211_supported_band *sband; struct ieee80211_channel *chan; + struct wiphy *request_wiphy; assert_cfg80211_lock(); + if (wiphy_idx_valid(last_request->wiphy_idx)) + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); + sband = wiphy->bands[band]; BUG_ON(chan_idx >= sband->n_channels); chan = &sband->channels[chan_idx]; @@ -879,8 +883,8 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band, power_rule = ®_rule->power_rule; if (last_request->initiator == REGDOM_SET_BY_DRIVER && - last_request->wiphy && last_request->wiphy == wiphy && - last_request->wiphy->strict_regulatory) { + request_wiphy && request_wiphy == wiphy && + request_wiphy->strict_regulatory) { /* This gaurantees the driver's requested regulatory domain * will always be used as a base for further regulatory * settings */ @@ -1044,6 +1048,11 @@ static int reg_copy_regd(const struct ieee80211_regdomain **dst_regd, static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by, const char *alpha2) { + /* + * NB: there should be no need to use request_wiphy when in + * REGDOM_SET_BY_CORE, as no wiphys should have sent hints by then + */ + struct wiphy *request_wiphy; if (set_by != REGDOM_SET_BY_CORE) assert_cfg80211_lock(); @@ -1062,10 +1071,15 @@ static int ignore_request(struct wiphy *wiphy, enum reg_set_by set_by, */ return 0; case REGDOM_SET_BY_COUNTRY_IE: + + if (wiphy_idx_valid(last_request->wiphy_idx)) + request_wiphy = + wiphy_idx_to_wiphy(last_request->wiphy_idx); + if (unlikely(!is_an_alpha2(alpha2))) return -EINVAL; if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE) { - if (last_request->wiphy != wiphy) { + if (request_wiphy != wiphy) { /* * Two cards with two APs claiming different * different Country IE alpha2s. We could @@ -1167,7 +1181,7 @@ new_request: request->alpha2[0] = alpha2[0]; request->alpha2[1] = alpha2[1]; request->initiator = set_by; - request->wiphy = wiphy; + request->wiphy_idx = wiphy_idx(wiphy); request->intersect = intersect; request->country_ie_checksum = country_ie_checksum; request->country_ie_env = env; @@ -1210,11 +1224,17 @@ EXPORT_SYMBOL(regulatory_hint); static bool reg_same_country_ie_hint(struct wiphy *wiphy, u32 country_ie_checksum) { + struct wiphy *request_wiphy; + assert_cfg80211_lock(); - if (!last_request->wiphy) + if (wiphy_idx_valid(last_request->wiphy_idx)) + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); + + if (!request_wiphy) return false; - if (likely(last_request->wiphy != wiphy)) + + if (likely(request_wiphy != wiphy)) return !country_ie_integrity_changes(country_ie_checksum); /* We should not have let these through at this point, they * should have been picked up earlier by the first alpha2 check @@ -1262,14 +1282,15 @@ void regulatory_hint_11d(struct wiphy *wiphy, /* We will run this for *every* beacon processed for the BSSID, so * we optimize an early check to exit out early if we don't have to * do anything */ - if (likely(last_request->wiphy)) { + if (wiphy_idx_valid(last_request->wiphy_idx)) { struct cfg80211_registered_device *drv_last_ie; - drv_last_ie = wiphy_to_dev(last_request->wiphy); + drv_last_ie = + cfg80211_drv_by_wiphy_idx(last_request->wiphy_idx); /* Lets keep this simple -- we trust the first AP * after we intersect with CRDA */ - if (likely(last_request->wiphy == wiphy)) { + if (likely(&drv_last_ie->wiphy == wiphy)) { /* Ignore IEs coming in on this wiphy with * the same alpha2 and environment cap */ if (likely(alpha2_equal(drv_last_ie->country_ie_alpha2, @@ -1361,13 +1382,12 @@ static void print_regdomain(const struct ieee80211_regdomain *rd) { if (is_intersected_alpha2(rd->alpha2)) { - struct wiphy *wiphy = NULL; - struct cfg80211_registered_device *drv; if (last_request->initiator == REGDOM_SET_BY_COUNTRY_IE) { - if (last_request->wiphy) { - wiphy = last_request->wiphy; - drv = wiphy_to_dev(wiphy); + struct cfg80211_registered_device *drv; + drv = cfg80211_drv_by_wiphy_idx( + last_request->wiphy_idx); + if (drv) { printk(KERN_INFO "cfg80211: Current regulatory " "domain updated by AP to: %c%c\n", drv->country_ie_alpha2[0], @@ -1433,7 +1453,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) { const struct ieee80211_regdomain *intersected_rd = NULL; struct cfg80211_registered_device *drv = NULL; - struct wiphy *wiphy = NULL; + struct wiphy *request_wiphy; /* Some basic sanity checks first */ if (is_world_regdom(rd->alpha2)) { @@ -1461,8 +1481,6 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) return -EINVAL; } - wiphy = last_request->wiphy; - /* Now lets set the regulatory domain, update all driver channels * and finally inform them of what we have done, in case they want * to review or adjust their own settings based on their own @@ -1478,6 +1496,9 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) return -EINVAL; } + if (wiphy_idx_valid(last_request->wiphy_idx)) + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); + if (!last_request->intersect) { int r; @@ -1490,9 +1511,9 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) /* For a driver hint, lets copy the regulatory domain the * driver wanted to the wiphy to deal with conflicts */ - BUG_ON(last_request->wiphy->regd); + BUG_ON(request_wiphy->regd); - r = reg_copy_regd(&last_request->wiphy->regd, rd); + r = reg_copy_regd(&request_wiphy->regd, rd); if (r) return r; @@ -1513,7 +1534,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) * However if a driver requested this specific regulatory * domain we keep it for its private use */ if (last_request->initiator == REGDOM_SET_BY_DRIVER) - last_request->wiphy->regd = rd; + request_wiphy->regd = rd; else kfree(rd); @@ -1553,7 +1574,7 @@ static int __set_regdom(const struct ieee80211_regdomain *rd) if (!intersected_rd) return -EINVAL; - drv = wiphy_to_dev(wiphy); + drv = wiphy_to_dev(request_wiphy); drv->country_ie_alpha2[0] = rd->alpha2[0]; drv->country_ie_alpha2[1] = rd->alpha2[1]; @@ -1602,14 +1623,18 @@ int set_regdom(const struct ieee80211_regdomain *rd) /* Caller must hold cfg80211_mutex */ void reg_device_remove(struct wiphy *wiphy) { + struct wiphy *request_wiphy; + assert_cfg80211_lock(); + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); + kfree(wiphy->regd); - if (!last_request || !last_request->wiphy) + if (!last_request || !request_wiphy) return; - if (last_request->wiphy != wiphy) + if (request_wiphy != wiphy) return; - last_request->wiphy = NULL; + last_request->wiphy_idx = WIPHY_IDX_STALE; last_request->country_ie_env = ENVIRON_ANY; } -- 1.6.1.2.253.ga34a ^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 5:36 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Luis R. Rodriguez @ 2009-02-13 5:36 ` Luis R. Rodriguez 2009-02-13 6:56 ` Johannes Berg 2009-02-13 11:04 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Johannes Berg 1 sibling, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 5:36 UTC (permalink / raw) To: johannes, johannes, linville; +Cc: Luis R. Rodriguez, linux-wireless Driver and userspace regulatory hints are now processed in a workqueue. This should fix some lockdep warnings which complain about possible circular locking dependencies such as: 1: ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.29-rc4-wl-13559-g0416c58-dirty #40 ------------------------------------------------------- iw/4598 is trying to acquire lock: (rtnl_mutex){--..}, at: [<ffffffff805013e2>] rtnl_lock+0x12/0x20 but task is already holding lock: (&drv->mtx){--..}, at: [<ffffffffa0352991>] cfg80211_get_dev_from_ifindex+0x61/0xa0 [cfg80211] 2: ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.29-rc4-wl #7 ------------------------------------------------------- wpa_supplicant/3333 is trying to acquire lock: (&ifsta->work){--..}, at: [<ffffffff8025555d>] __cancel_work_timer+0x5d/0x1f0 but task is already holding lock: (rtnl_mutex){--..}, at: [<ffffffff804997a6>] devinet_ioctl+0x136/0x7a0 Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com> --- drivers/net/wireless/ath9k/main.c | 8 ++- drivers/net/wireless/zd1211rw/zd_mac.c | 6 +- include/net/cfg80211.h | 2 + include/net/wireless.h | 9 ++- net/wireless/nl80211.c | 10 +-- net/wireless/reg.c | 140 +++++++++++++++++++++++++++++--- net/wireless/reg.h | 2 + 7 files changed, 153 insertions(+), 24 deletions(-) diff --git a/drivers/net/wireless/ath9k/main.c b/drivers/net/wireless/ath9k/main.c index fc3460f..f51e7c8 100644 --- a/drivers/net/wireless/ath9k/main.c +++ b/drivers/net/wireless/ath9k/main.c @@ -1656,8 +1656,12 @@ int ath_attach(u16 devid, struct ath_softc *sc) error = ieee80211_register_hw(hw); - if (!ath9k_is_world_regd(sc->sc_ah)) - regulatory_hint(hw->wiphy, sc->sc_ah->regulatory.alpha2); + if (!ath9k_is_world_regd(sc->sc_ah)) { + error = regulatory_hint(hw->wiphy, + sc->sc_ah->regulatory.alpha2); + if (error) + goto detach; + } /* Initialize LED control */ ath_init_leds(sc); diff --git a/drivers/net/wireless/zd1211rw/zd_mac.c b/drivers/net/wireless/zd1211rw/zd_mac.c index 7579af2..da9214e 100644 --- a/drivers/net/wireless/zd1211rw/zd_mac.c +++ b/drivers/net/wireless/zd1211rw/zd_mac.c @@ -170,10 +170,10 @@ int zd_mac_init_hw(struct ieee80211_hw *hw) goto disable_int; r = zd_reg2alpha2(mac->regdomain, alpha2); - if (!r) - regulatory_hint(hw->wiphy, alpha2); + if (r) + goto disable_int; - r = 0; + r = regulatory_hint(hw->wiphy, alpha2); disable_int: zd_chip_disable_int(chip); out: diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h index 2a83510..ec29843 100644 --- a/include/net/cfg80211.h +++ b/include/net/cfg80211.h @@ -396,6 +396,7 @@ enum environment_cap { * country IE * @country_ie_env: lets us know if the AP is telling us we are outdoor, * indoor, or if it doesn't matter + * @list: used to insert into a linked list */ struct regulatory_request { int wiphy_idx; @@ -404,6 +405,7 @@ struct regulatory_request { bool intersect; u32 country_ie_checksum; enum environment_cap country_ie_env; + struct list_head list; }; struct ieee80211_freq_range { diff --git a/include/net/wireless.h b/include/net/wireless.h index 1c6285e..afeade9 100644 --- a/include/net/wireless.h +++ b/include/net/wireless.h @@ -398,8 +398,15 @@ ieee80211_get_response_rate(struct ieee80211_supported_band *sband, * domain should be in or by providing a completely build regulatory domain. * If the driver provides an ISO/IEC 3166 alpha2 userspace will be queried * for a regulatory domain structure for the respective country. + * + * The wiphy must have been registered to cfg80211 prior to this call. + * For cfg80211 drivers this means you must first use wiphy_register(), + * for mac80211 drivers you must first use ieee80211_register_hw(). + * + * Drivers should check the return value, its possible you can get + * an -ENOMEM. */ -extern void regulatory_hint(struct wiphy *wiphy, const char *alpha2); +extern int regulatory_hint(struct wiphy *wiphy, const char *alpha2); /** * regulatory_hint_11d - hints a country IE as a regulatory domain diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c index 843d26d..fb1c99e 100644 --- a/net/wireless/nl80211.c +++ b/net/wireless/nl80211.c @@ -1911,14 +1911,8 @@ static int nl80211_req_set_reg(struct sk_buff *skb, struct genl_info *info) if (is_world_regdom(data)) return -EINVAL; #endif - mutex_lock(&cfg80211_mutex); - r = __regulatory_hint(NULL, REGDOM_SET_BY_USER, data, 0, ENVIRON_ANY); - mutex_unlock(&cfg80211_mutex); - /* This means the regulatory domain was already set, however - * we don't want to confuse userspace with a "successful error" - * message so lets just treat it as a success */ - if (r == -EALREADY) - r = 0; + r = regulatory_hint_user(data); + return r; } diff --git a/net/wireless/reg.c b/net/wireless/reg.c index 316b2e4..78ff554 100644 --- a/net/wireless/reg.c +++ b/net/wireless/reg.c @@ -64,6 +64,18 @@ const struct ieee80211_regdomain *cfg80211_regdomain; * what it thinks should apply for the same country */ static const struct ieee80211_regdomain *country_ie_regdomain; +static LIST_HEAD(reg_requests_list); +static DEFINE_MUTEX(reg_mutex); + +static void reg_process_pending_hints(void); + +static void reg_todo(struct work_struct *work) +{ + reg_process_pending_hints(); +} + +static DECLARE_WORK(reg_work, reg_todo); + /* We keep a static world regulatory domain in case of the absence of CRDA */ static const struct ieee80211_regdomain world_regdom = { .n_reg_rules = 1, @@ -829,7 +841,7 @@ static void handle_channel(struct wiphy *wiphy, enum ieee80211_band band, const struct ieee80211_power_rule *power_rule = NULL; struct ieee80211_supported_band *sband; struct ieee80211_channel *chan; - struct wiphy *request_wiphy; + struct wiphy *request_wiphy = NULL; assert_cfg80211_lock(); @@ -1206,21 +1218,66 @@ new_request: return call_crda(alpha2); } -void regulatory_hint(struct wiphy *wiphy, const char *alpha2) +/* This currently queues driver and user hints */ +static void queue_regulatory_request(struct regulatory_request *request) { - int r; + mutex_lock(®_mutex); + list_add_tail(&request->list, ®_requests_list); + mutex_unlock(®_mutex); + + schedule_work(®_work); +} + +/* Driver hints */ +int regulatory_hint(struct wiphy *wiphy, const char *alpha2) +{ + struct regulatory_request *request; + BUG_ON(!alpha2); + BUG_ON(!wiphy); - mutex_lock(&cfg80211_mutex); - r = __regulatory_hint(wiphy, REGDOM_SET_BY_DRIVER, - alpha2, 0, ENVIRON_ANY); - /* This is required so that the orig_* parameters are saved */ - if (r == -EALREADY && wiphy->strict_regulatory) - wiphy_update_regulatory(wiphy, REGDOM_SET_BY_DRIVER); - mutex_unlock(&cfg80211_mutex); + request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL); + if (!request) + return -ENOMEM; + + request->wiphy_idx = wiphy_idx(wiphy); + + /* Must have registered wiphy first */ + BUG_ON(!wiphy_idx_valid(request->wiphy_idx)); + + request->alpha2[0] = alpha2[0]; + request->alpha2[1] = alpha2[1]; + request->initiator = REGDOM_SET_BY_DRIVER; + request->country_ie_env = ENVIRON_ANY; + + queue_regulatory_request(request); + + return 0; } EXPORT_SYMBOL(regulatory_hint); +/* User hints */ +int regulatory_hint_user(const char *alpha2) +{ + struct regulatory_request *request; + + BUG_ON(!alpha2); + + request = kzalloc(sizeof(struct regulatory_request), GFP_KERNEL); + if (!request) + return -ENOMEM; + + request->wiphy_idx = WIPHY_IDX_STALE; + request->alpha2[0] = alpha2[0]; + request->alpha2[1] = alpha2[1]; + request->initiator = REGDOM_SET_BY_USER, + request->country_ie_env = ENVIRON_ANY; + + queue_regulatory_request(request); + + return 0; +} + static bool reg_same_country_ie_hint(struct wiphy *wiphy, u32 country_ie_checksum) { @@ -1620,6 +1677,58 @@ int set_regdom(const struct ieee80211_regdomain *rd) return r; } +/* This currently only processes user and driver regulatory hints */ +static int reg_process_hint(struct regulatory_request *reg_request) +{ + int r = 0; + struct wiphy *wiphy = NULL; + + BUG_ON(!reg_request->alpha2); + + mutex_lock(&cfg80211_mutex); + + if (wiphy_idx_valid(reg_request->wiphy_idx)) + wiphy = wiphy_idx_to_wiphy(reg_request->wiphy_idx); + + if (reg_request->initiator == REGDOM_SET_BY_DRIVER && + !wiphy) { + r = -ENODEV; + goto out; + } + + r = __regulatory_hint(wiphy, reg_request->initiator, + reg_request->alpha2, 0, reg_request->country_ie_env); + /* This is required so that the orig_* parameters are saved */ + if (r == -EALREADY && wiphy->strict_regulatory) + wiphy_update_regulatory(wiphy, reg_request->initiator); +out: + mutex_unlock(&cfg80211_mutex); + + if (r == -EALREADY) + r = 0; + + return r; +} + +static void reg_process_pending_hints(void) + { + struct regulatory_request *reg_request, *tmp; + int r; + + mutex_lock(®_mutex); + list_for_each_entry_safe(reg_request, tmp, ®_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(®_request->list); + kfree(reg_request); + } + mutex_unlock(®_mutex); +} + + /* Caller must hold cfg80211_mutex */ void reg_device_remove(struct wiphy *wiphy) { @@ -1673,6 +1782,11 @@ int regulatory_init(void) void regulatory_exit(void) { + struct regulatory_request *reg_request, *tmp; + + cancel_work_sync(®_work); + + mutex_lock(®_mutex); mutex_lock(&cfg80211_mutex); reset_regdomains(); @@ -1684,5 +1798,11 @@ void regulatory_exit(void) platform_device_unregister(reg_pdev); + list_for_each_entry_safe(reg_request, tmp, ®_requests_list, list) { + list_del(®_request->list); + kfree(reg_request); + } + mutex_unlock(&cfg80211_mutex); + mutex_unlock(®_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 ^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 5:36 ` [PATCH 6/6] cfg80211: move regulatory hints to workqueue Luis R. Rodriguez @ 2009-02-13 6:56 ` Johannes Berg 2009-02-13 7:09 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 6:56 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 384 bytes --] On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: > Driver and userspace regulatory hints are now processed in > a workqueue. This should fix some lockdep warnings which complain > about possible circular locking dependencies such as: Yes, but I need to look at the one Reinette posted and fix that one, which will probably require a different solution. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 6:56 ` Johannes Berg @ 2009-02-13 7:09 ` Luis R. Rodriguez 2009-02-13 7:14 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:09 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: >> Driver and userspace regulatory hints are now processed in >> a workqueue. This should fix some lockdep warnings which complain >> about possible circular locking dependencies such as: > > Yes, but I need to look at the one Reinette posted and fix that one, > which will probably require a different solution. Oh I didn't see that one. How different? Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:09 ` Luis R. Rodriguez @ 2009-02-13 7:14 ` Luis R. Rodriguez 2009-02-13 7:14 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:14 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 11:09 PM, Luis R. Rodriguez <lrodriguez@atheros.com> wrote: > On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: >> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: >>> Driver and userspace regulatory hints are now processed in >>> a workqueue. This should fix some lockdep warnings which complain >>> about possible circular locking dependencies such as: >> >> Yes, but I need to look at the one Reinette posted and fix that one, >> which will probably require a different solution. > > Oh I didn't see that one. How different? I see an rtnl lock issue against scanning and the drv mutex. Doesn't seem to be related to regulatory. So how would that affect a path to move hints to a workqueue? Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:14 ` Luis R. Rodriguez @ 2009-02-13 7:14 ` Luis R. Rodriguez 2009-02-13 7:17 ` Johannes Berg 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:14 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 11:14 PM, Luis R. Rodriguez <lrodriguez@atheros.com> wrote: > On Thu, Feb 12, 2009 at 11:09 PM, Luis R. Rodriguez > <lrodriguez@atheros.com> wrote: >> On Thu, Feb 12, 2009 at 10:56 PM, Johannes Berg >> <johannes@sipsolutions.net> wrote: >>> On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: >>>> Driver and userspace regulatory hints are now processed in >>>> a workqueue. This should fix some lockdep warnings which complain >>>> about possible circular locking dependencies such as: >>> >>> Yes, but I need to look at the one Reinette posted and fix that one, >>> which will probably require a different solution. >> >> Oh I didn't see that one. How different? > > I see an rtnl lock issue against scanning and the drv mutex. Doesn't > seem to be related to regulatory. So how would that affect a path to > move hints to a workqueue? Oh you mean the rntl_lock() and drv mutex is _still_ an issue? Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:14 ` Luis R. Rodriguez @ 2009-02-13 7:17 ` Johannes Berg 2009-02-13 7:35 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 7:17 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 993 bytes --] On Thu, 2009-02-12 at 23:14 -0800, Luis R. Rodriguez wrote: > >>> Yes, but I need to look at the one Reinette posted and fix that one, > >>> which will probably require a different solution. > >> > >> Oh I didn't see that one. How different? > > > > I see an rtnl lock issue against scanning and the drv mutex. Doesn't > > seem to be related to regulatory. So how would that affect a path to > > move hints to a workqueue? > > Oh you mean the rntl_lock() and drv mutex is _still_ an issue? Well, regardless of regulatory, we now have two paths: nl80211: cfg80211_mutex --> drv->mutex --> rtnl wext: rtnl --> cfg80211_mutex --> drv->mutex which is clearly not a good plan. Therefore, I'll probably have to implement solution (1) from what I sent you, and invert the locking in nl80211. That would fix your the immediate problem with regulatory as well, because that was: rtnl ---> (regulatory_hint) ---> cfg80211_mutex --> drv->mutex johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:17 ` Johannes Berg @ 2009-02-13 7:35 ` Luis R. Rodriguez 2009-02-13 7:48 ` Johannes Berg 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:35 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 11:17 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 23:14 -0800, Luis R. Rodriguez wrote: > >> >>> Yes, but I need to look at the one Reinette posted and fix that one, >> >>> which will probably require a different solution. >> >> >> >> Oh I didn't see that one. How different? >> > >> > I see an rtnl lock issue against scanning and the drv mutex. Doesn't >> > seem to be related to regulatory. So how would that affect a path to >> > move hints to a workqueue? >> >> Oh you mean the rntl_lock() and drv mutex is _still_ an issue? > > Well, regardless of regulatory, we now have two paths: > > nl80211: cfg80211_mutex --> drv->mutex --> rtnl > wext: rtnl --> cfg80211_mutex --> drv->mutex > > which is clearly not a good plan. > > Therefore, I'll probably have to implement solution (1) from what I sent > you, and invert the locking in nl80211. That would fix your the > immediate problem with regulatory as well, because that was: > > rtnl ---> (regulatory_hint) ---> cfg80211_mutex --> drv->mutex Yup I see. So as it stands in this patch user reg hint now is: reg_mutex --> cfg80211_mutex Driver hint also does the same so it seems your fix on nl80211 would probably just tap this series on the nl80211_req_set_reg(). I also think using a workqueue would be good here anyway, the only negative thing I've seen is loading takes a bit longer. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:35 ` Luis R. Rodriguez @ 2009-02-13 7:48 ` Johannes Berg 2009-02-13 7:57 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 7:48 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1446 bytes --] On Thu, 2009-02-12 at 23:35 -0800, Luis R. Rodriguez wrote: > > Well, regardless of regulatory, we now have two paths: > > > > nl80211: cfg80211_mutex --> drv->mutex --> rtnl > > wext: rtnl --> cfg80211_mutex --> drv->mutex > > > > which is clearly not a good plan. > > > > Therefore, I'll probably have to implement solution (1) from what I sent > > you, and invert the locking in nl80211. That would fix your the > > immediate problem with regulatory as well, because that was: > > > > rtnl ---> (regulatory_hint) ---> cfg80211_mutex --> drv->mutex > > Yup I see. So as it stands in this patch user reg hint now is: > > reg_mutex --> cfg80211_mutex I don't really care about reg_mutex much, and it probably doesn't matter as long as its use is limited. Question is whether we need it, but that's a different question. > Driver hint also does the same so it seems your fix on nl80211 would > probably just tap this series on the nl80211_req_set_reg(). Yeah, I think I'll work on top of your series anyway so I don't clash with the cleanups. > I also think using a workqueue would be good here anyway, the only > negative thing I've seen is loading takes a bit longer. Indeed, it's a good thing because a path like this: driver_open -> regulatory_hint -> cfg80211 stuff -> driver_reg_notifier is always deadlock prone since you call into the driver from itself. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:48 ` Johannes Berg @ 2009-02-13 7:57 ` Luis R. Rodriguez 2009-02-13 8:08 ` Johannes Berg 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:57 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 11:48 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 23:35 -0800, Luis R. Rodriguez wrote: > >> > Well, regardless of regulatory, we now have two paths: >> > >> > nl80211: cfg80211_mutex --> drv->mutex --> rtnl >> > wext: rtnl --> cfg80211_mutex --> drv->mutex >> > >> > which is clearly not a good plan. >> > >> > Therefore, I'll probably have to implement solution (1) from what I sent >> > you, and invert the locking in nl80211. That would fix your the >> > immediate problem with regulatory as well, because that was: >> > >> > rtnl ---> (regulatory_hint) ---> cfg80211_mutex --> drv->mutex >> >> Yup I see. So as it stands in this patch user reg hint now is: >> >> reg_mutex --> cfg80211_mutex > > I don't really care about reg_mutex much, and it probably doesn't matter > as long as its use is limited. Question is whether we need it, but > that's a different question. To answer that question -- reg_mutex just protects the reg_requests_list linked list. That's it. Without that you could potentially have two userspace reg hints while the workqueue runs. Remember that the hints no longer lock except to just add a new regulatory_request into the reg_requests_list. >> Driver hint also does the same so it seems your fix on nl80211 would >> probably just tap this series on the nl80211_req_set_reg(). > > Yeah, I think I'll work on top of your series anyway so I don't clash > with the cleanups. Any other comments on the series? >> I also think using a workqueue would be good here anyway, the only >> negative thing I've seen is loading takes a bit longer. > > Indeed, it's a good thing because a path like this: > > driver_open > -> regulatory_hint > -> cfg80211 stuff > -> driver_reg_notifier > > is always deadlock prone since you call into the driver from itself. I see -- good point. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 7:57 ` Luis R. Rodriguez @ 2009-02-13 8:08 ` Johannes Berg 2009-02-13 8:19 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 8:08 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1021 bytes --] On Thu, 2009-02-12 at 23:57 -0800, Luis R. Rodriguez wrote: > To answer that question -- reg_mutex just protects the > reg_requests_list linked list. That's it. Without that you could > potentially have two userspace reg hints while the workqueue runs. > Remember that the hints no longer lock except to just add a new > regulatory_request into the reg_requests_list. Right, ok. > >> Driver hint also does the same so it seems your fix on nl80211 would > >> probably just tap this series on the nl80211_req_set_reg(). > > > > Yeah, I think I'll work on top of your series anyway so I don't clash > > with the cleanups. > > Any other comments on the series? Not really, looks pretty good to me. It does seem a little dubious to return an error from a hint function and then not attach the hardware, but that's up to you what you want to do in ath9k I guess -- seems it should work ok even if the hint failed, though of course then you'll not have memory for anything else either... johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 6/6] cfg80211: move regulatory hints to workqueue 2009-02-13 8:08 ` Johannes Berg @ 2009-02-13 8:19 ` Luis R. Rodriguez 0 siblings, 0 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 8:19 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Fri, Feb 13, 2009 at 12:08 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 23:57 -0800, Luis R. Rodriguez wrote: > >> To answer that question -- reg_mutex just protects the >> reg_requests_list linked list. That's it. Without that you could >> potentially have two userspace reg hints while the workqueue runs. >> Remember that the hints no longer lock except to just add a new >> regulatory_request into the reg_requests_list. > > Right, ok. > >> >> Driver hint also does the same so it seems your fix on nl80211 would >> >> probably just tap this series on the nl80211_req_set_reg(). >> > >> > Yeah, I think I'll work on top of your series anyway so I don't clash >> > with the cleanups. >> >> Any other comments on the series? > > Not really, looks pretty good to me. It does seem a little dubious to > return an error from a hint function and then not attach the hardware, > but that's up to you what you want to do in ath9k I guess -- seems it > should work ok even if the hint failed, though of course then you'll not > have memory for anything else either... The only error is -ENOMEM so yeah we certainly should propagate that down to the driver. An alternative is to use a few regulatory_requests on the stack for queuing purposes but it just seemed silly on second thought. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy 2009-02-13 5:36 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 6/6] cfg80211: move regulatory hints to workqueue Luis R. Rodriguez @ 2009-02-13 11:04 ` Johannes Berg 2009-02-13 18:21 ` Luis R. Rodriguez 1 sibling, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 11:04 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 436 bytes --] On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: > > + if (wiphy_idx_valid(last_request->wiphy_idx)) > + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); All this seems pointless, wiphy_idx_to_wiphy will just return NULL if the index isn't valid. This is in a number of places, and it's not like it being invalid will be happening often so we'd have to optimise for it. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy 2009-02-13 11:04 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Johannes Berg @ 2009-02-13 18:21 ` Luis R. Rodriguez 2009-02-13 21:24 ` Johannes Berg 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 18:21 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: >> >> + if (wiphy_idx_valid(last_request->wiphy_idx)) >> + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); > > All this seems pointless, wiphy_idx_to_wiphy will just return NULL if > the index isn't valid. This is in a number of places, and it's not like > it being invalid will be happening often so we'd have to optimise for > it. The check can be removed if the WARNING is removed on wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the warning, your call. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy 2009-02-13 18:21 ` Luis R. Rodriguez @ 2009-02-13 21:24 ` Johannes Berg 2009-02-13 21:40 ` Luis R. Rodriguez 0 siblings, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 21:24 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1074 bytes --] On Fri, 2009-02-13 at 10:21 -0800, Luis R. Rodriguez wrote: > On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg > <johannes@sipsolutions.net> wrote: > > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: > >> > >> + if (wiphy_idx_valid(last_request->wiphy_idx)) > >> + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); > > > > All this seems pointless, wiphy_idx_to_wiphy will just return NULL if > > the index isn't valid. This is in a number of places, and it's not like > > it being invalid will be happening often so we'd have to optimise for > > it. > > The check can be removed if the WARNING is removed on > wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the > warning, your call. Dunno, I think no warning is probably better if more than half the callers would have to check first... Also, the warning seems like it could spuriously trigger if a wiphy is removed? One other thing I noticed - why is there a conditional assert on the mutex? Shouldn't it always be locked? johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy 2009-02-13 21:24 ` Johannes Berg @ 2009-02-13 21:40 ` Luis R. Rodriguez 0 siblings, 0 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 21:40 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Fri, Feb 13, 2009 at 1:24 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Fri, 2009-02-13 at 10:21 -0800, Luis R. Rodriguez wrote: >> On Fri, Feb 13, 2009 at 3:04 AM, Johannes Berg >> <johannes@sipsolutions.net> wrote: >> > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: >> >> >> >> + if (wiphy_idx_valid(last_request->wiphy_idx)) >> >> + request_wiphy = wiphy_idx_to_wiphy(last_request->wiphy_idx); >> > >> > All this seems pointless, wiphy_idx_to_wiphy will just return NULL if >> > the index isn't valid. This is in a number of places, and it's not like >> > it being invalid will be happening often so we'd have to optimise for >> > it. >> >> The check can be removed if the WARNING is removed on >> wiphy_idx_to_wiphy(). I left it as I figured it'd be good to leave the >> warning, your call. > > Dunno, I think no warning is probably better if more than half the > callers would have to check first... Also, the warning seems like it > could spuriously trigger if a wiphy is removed? Yeah I think you're right. > One other thing I noticed - why is there a conditional assert on the > mutex? Shouldn't it always be locked? The conditional assert on the mutex is there because on cfg80211 you don't want to hold a mutex as that would mean userspace gets stuck with one. I believe lockdep complained to me about it as exactly that. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection 2009-02-13 5:36 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Luis R. Rodriguez @ 2009-02-13 6:54 ` Johannes Berg 2009-02-13 7:08 ` Luis R. Rodriguez 1 sibling, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 6:54 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 282 bytes --] On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: > +static inline void assert_cfg80211_lock(void) > +{ > + BUG_ON(!mutex_is_locked(&cfg80211_mutex)); > +} That's a little heavy-handed for a small development mistake, can we have a WARN_ON instead? johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection 2009-02-13 6:54 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Johannes Berg @ 2009-02-13 7:08 ` Luis R. Rodriguez 0 siblings, 0 replies; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:08 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 10:54 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 21:36 -0800, Luis R. Rodriguez wrote: > >> +static inline void assert_cfg80211_lock(void) >> +{ >> + BUG_ON(!mutex_is_locked(&cfg80211_mutex)); >> +} > > That's a little heavy-handed for a small development mistake, can we > have a WARN_ON instead? Sure, will change. Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity 2009-02-13 5:35 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex Luis R. Rodriguez @ 2009-02-13 6:53 ` Johannes Berg 2009-02-13 7:08 ` Luis R. Rodriguez 1 sibling, 1 reply; 27+ messages in thread From: Johannes Berg @ 2009-02-13 6:53 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 369 bytes --] On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote: > - if (unlikely(drv->wiphy_idx < 0)) { > + if (!wiphy_idx_valid(drv->wiphy_idx)) { > +/* Note 0 is valid, hence phy0 */ > +static inline > +bool wiphy_idx_valid(int wiphy_idx) > +{ > + return (likely(wiphy_idx >= 0)); > +} Does that really work properly with the likely in there? johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity 2009-02-13 6:53 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Johannes Berg @ 2009-02-13 7:08 ` Luis R. Rodriguez 2009-02-13 7:18 ` Johannes Berg 0 siblings, 1 reply; 27+ messages in thread From: Luis R. Rodriguez @ 2009-02-13 7:08 UTC (permalink / raw) To: Johannes Berg; +Cc: linville, linux-wireless On Thu, Feb 12, 2009 at 10:53 PM, Johannes Berg <johannes@sipsolutions.net> wrote: > On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote: > >> - if (unlikely(drv->wiphy_idx < 0)) { >> + if (!wiphy_idx_valid(drv->wiphy_idx)) { > >> +/* Note 0 is valid, hence phy0 */ >> +static inline >> +bool wiphy_idx_valid(int wiphy_idx) >> +{ >> + return (likely(wiphy_idx >= 0)); >> +} > > Does that really work properly with the likely in there? Oh you mean !likely() won't give me my unlikely() effect? I think so as the path is meant to not be optimized so the optimized path would be put first, no? Luis ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity 2009-02-13 7:08 ` Luis R. Rodriguez @ 2009-02-13 7:18 ` Johannes Berg 0 siblings, 0 replies; 27+ messages in thread From: Johannes Berg @ 2009-02-13 7:18 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 948 bytes --] On Thu, 2009-02-12 at 23:08 -0800, Luis R. Rodriguez wrote: > On Thu, Feb 12, 2009 at 10:53 PM, Johannes Berg > <johannes@sipsolutions.net> wrote: > > On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote: > > > >> - if (unlikely(drv->wiphy_idx < 0)) { > >> + if (!wiphy_idx_valid(drv->wiphy_idx)) { > > > >> +/* Note 0 is valid, hence phy0 */ > >> +static inline > >> +bool wiphy_idx_valid(int wiphy_idx) > >> +{ > >> + return (likely(wiphy_idx >= 0)); > >> +} > > > > Does that really work properly with the likely in there? > > Oh you mean !likely() won't give me my unlikely() effect? I think so > as the path is meant to not be optimized so the optimized path would > be put first, no? Well keep in mind that likely() also affects branch prediction on some CPUs where you can annotate that, and I think it wouldn't be possible for the compiler to get it out of the inline there properly. johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints 2009-02-13 5:35 [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Luis R. Rodriguez 2009-02-13 5:35 ` [PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx Luis R. Rodriguez @ 2009-02-13 9:13 ` Johannes Berg 1 sibling, 0 replies; 27+ messages in thread From: Johannes Berg @ 2009-02-13 9:13 UTC (permalink / raw) To: Luis R. Rodriguez; +Cc: linville, linux-wireless [-- Attachment #1: Type: text/plain, Size: 1039 bytes --] On Thu, 2009-02-12 at 21:35 -0800, Luis R. Rodriguez wrote: > This series does prep work and then the actual work to move the > cfg80211 driver and userspace regulatory hints onto a workqueue. > It should cure some lockdep warnings and also pave the way for > more future reg work like beacon reg hints. > > Luis R. Rodriguez (6): > cfg80211: rename cfg80211_registered_device's idx to wiphy_idx > cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity > cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex > cfg80211: add assert_cfg80211_lock() to ensure proper protection > cfg80211: make regulatory_request use wiphy_idx instead of wiphy > cfg80211: move regulatory hints to workqueue net/wireless/reg.c: In function ‘ignore_request’: net/wireless/reg.c:1067: warning: ‘request_wiphy’ may be used uninitialized in this function net/wireless/reg.c: In function ‘__set_regdom’: net/wireless/reg.c:1513: warning: ‘request_wiphy’ may be used uninitialized in this function johannes [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2009-02-13 21:40 UTC | newest] Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-13 5:35 [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Luis R. Rodriguez 2009-02-13 5:35 ` [PATCH 1/6] cfg80211: rename cfg80211_registered_device's idx to wiphy_idx Luis R. Rodriguez 2009-02-13 5:35 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 3/6] cfg80211: rename cfg80211_drv_mutex to cfg80211_mutex Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Luis R. Rodriguez 2009-02-13 5:36 ` [PATCH 6/6] cfg80211: move regulatory hints to workqueue Luis R. Rodriguez 2009-02-13 6:56 ` Johannes Berg 2009-02-13 7:09 ` Luis R. Rodriguez 2009-02-13 7:14 ` Luis R. Rodriguez 2009-02-13 7:14 ` Luis R. Rodriguez 2009-02-13 7:17 ` Johannes Berg 2009-02-13 7:35 ` Luis R. Rodriguez 2009-02-13 7:48 ` Johannes Berg 2009-02-13 7:57 ` Luis R. Rodriguez 2009-02-13 8:08 ` Johannes Berg 2009-02-13 8:19 ` Luis R. Rodriguez 2009-02-13 11:04 ` [PATCH 5/6] cfg80211: make regulatory_request use wiphy_idx instead of wiphy Johannes Berg 2009-02-13 18:21 ` Luis R. Rodriguez 2009-02-13 21:24 ` Johannes Berg 2009-02-13 21:40 ` Luis R. Rodriguez 2009-02-13 6:54 ` [PATCH 4/6] cfg80211: add assert_cfg80211_lock() to ensure proper protection Johannes Berg 2009-02-13 7:08 ` Luis R. Rodriguez 2009-02-13 6:53 ` [PATCH 2/6] cfg80211: add wiphy_idx_valid to check for wiphy_idx sanity Johannes Berg 2009-02-13 7:08 ` Luis R. Rodriguez 2009-02-13 7:18 ` Johannes Berg 2009-02-13 9:13 ` [PATCH 0/6] cfg80211: adds a workqueue for regulatory hints Johannes Berg
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.