[RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Luis R. Rodriguez]

This series depends on my last posted series.

This set tries to address some issues I was with checking for DFS region
support on the cfg80211 path for starting radar detection. Apart from
checking for the DFS region when we get a request to start DFS radar
detection we should always check the DFS region also when we want to
use a mode of operation that requires DFS support in the kernel. IBSS
has a mode of operation, when NL80211_ATTR_HANDLE_DFS is set, which
should be taken into consideration for DFS support. My patch 5 titled
"cfg80211: DFS check dfs_region before usage" really is incomplete
and should check for NL80211_ATTR_HANDLE_DFS on IBSS wdevs when set
to make the appropriate exception.

While at it I considered the corner cases of changing regulatory domains,
this pushed me to add regulatory quiescing support. This is particularly
important when we switch regulatory domains and the DFS region changes but
should nevertheless still be done even in other cases. I think we should
also extend the IBSS NL80211_ATTR_HANDLE_DFS case to require newer
implementations to send their supported DFS regions, otherwise we assume
userespace is doing the right thing. I think we can take care of that better
in kernel space.

Janusz, please test, and likely feel free to take on the last patch.
We may need help from Johannes on the rtnl lock question now required
for checks on enabling beacon operation.

Jouni, thoughts on the approach? Sunit, Rakesh?

Luis R. Rodriguez (5):
  cfg80211: pass the wdev on the country IE regulatory hint
  cfg80211: make cfg80211_leave_all() available outside of sysfs
  cfg80211: add regulatory quiescing support
  cfg80211: add DFS region capability support
  cfg80211: DFS check dfs_region before usage

 include/net/cfg80211.h       | 57 +++++++++++++++++++++++++++++++++++++++++++
 include/net/regulatory.h     |  4 +++
 include/uapi/linux/nl80211.h | 21 +++++++++++++++-
 net/wireless/chan.c          |  8 ++++++
 net/wireless/core.c          | 47 +++++++++++++++++++++++++++++++++++
 net/wireless/core.h          |  2 ++
 net/wireless/nl80211.c       | 15 ++++++++++++
 net/wireless/reg.c           | 58 +++++++++++++++++++++++++++++++++++++++++++-
 net/wireless/reg.h           | 13 +++++-----
 net/wireless/sme.c           |  2 +-
 net/wireless/sysfs.c         |  8 ------
 11 files changed, 218 insertions(+), 17 deletions(-)

-- 
1.8.4.rc3

[RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Luis R. Rodriguez]

This also records the wdev on the last regulatory request,
this can be used later to help quiesce wdev's at appropriate
times by the regulatory core.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 include/net/regulatory.h |  4 ++++
 net/wireless/reg.c       |  5 ++++-
 net/wireless/reg.h       | 13 +++++++------
 net/wireless/sme.c       |  2 +-
 4 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/include/net/regulatory.h b/include/net/regulatory.h
index ab0bee0..40df080 100644
--- a/include/net/regulatory.h
+++ b/include/net/regulatory.h
@@ -42,6 +42,9 @@ enum environment_cap {
  * 	can be used by the wireless core to deal with conflicts
  * 	and potentially inform users of which devices specifically
  * 	cased the conflicts.
+ * @wdev: this is set for %REGDOM_SET_BY_COUNTRY_IE to help avoid
+ * 	trying to quiesce the same wdev if the country IE was
+ * 	issued through it.
  * @initiator: indicates who sent this request, could be any of
  * 	of those set in nl80211_reg_initiator (%NL80211_REGDOM_SET_BY_*)
  * @alpha2: the ISO / IEC 3166 alpha2 country code of the requested
@@ -76,6 +79,7 @@ enum environment_cap {
 struct regulatory_request {
 	struct rcu_head rcu_head;
 	int wiphy_idx;
+	struct wireless_dev *wdev;
 	enum nl80211_reg_initiator initiator;
 	enum nl80211_user_reg_hint_type user_reg_hint_type;
 	char alpha2[2];
diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 4bfbeaa..e5935c2 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1871,12 +1871,14 @@ int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
 }
 EXPORT_SYMBOL(regulatory_hint);
 
-void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
+void regulatory_hint_country_ie(struct wireless_dev *wdev,
+				enum ieee80211_band band,
 				const u8 *country_ie, u8 country_ie_len)
 {
 	char alpha2[2];
 	enum environment_cap env = ENVIRON_ANY;
 	struct regulatory_request *request = NULL, *lr;
+	struct wiphy *wiphy = wdev->wiphy;
 
 	/* IE len must be evenly divisible by 2 */
 	if (country_ie_len & 0x01)
@@ -1913,6 +1915,7 @@ void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
 		goto out;
 
 	request->wiphy_idx = get_wiphy_idx(wiphy);
+	request->wdev = wdev;
 	request->alpha2[0] = alpha2[0];
 	request->alpha2[1] = alpha2[1];
 	request->initiator = NL80211_REGDOM_SET_BY_COUNTRY_IE;
diff --git a/net/wireless/reg.h b/net/wireless/reg.h
index 02bd8f4..3b1e352 100644
--- a/net/wireless/reg.h
+++ b/net/wireless/reg.h
@@ -61,8 +61,9 @@ int regulatory_hint_found_beacon(struct wiphy *wiphy,
 
 /**
  * regulatory_hint_country_ie - hints a country IE as a regulatory domain
- * @wiphy: the wireless device giving the hint (used only for reporting
- *	conflicts)
+ * @wdev: the wireless device giving the hint, used for reporting
+ * 	conflicts and to quiesce devices which may disagree with the
+ * 	regulatory domain update.
  * @band: the band on which the country IE was received on. This determines
  *	the band we'll process the country IE channel triplets for.
  * @country_ie: pointer to the country IE
@@ -80,10 +81,10 @@ int regulatory_hint_found_beacon(struct wiphy *wiphy,
  * not observed. For this reason if a triplet is seen with channel
  * information for a band the BSS is not present in it will be ignored.
  */
-void regulatory_hint_country_ie(struct wiphy *wiphy,
-			 enum ieee80211_band band,
-			 const u8 *country_ie,
-			 u8 country_ie_len);
+void regulatory_hint_country_ie(struct wireless_dev *wdev,
+				enum ieee80211_band band,
+				const u8 *country_ie,
+				u8 country_ie_len);
 
 /**
  * regulatory_hint_disconnect - informs all devices have been disconneted
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 65f8008..0a5d304 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -682,7 +682,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	 * - country_ie + 2, the start of the country ie data, and
 	 * - and country_ie[1] which is the IE length
 	 */
-	regulatory_hint_country_ie(wdev->wiphy, bss->channel->band,
+	regulatory_hint_country_ie(wdev, bss->channel->band,
 				   country_ie + 2, country_ie[1]);
 	kfree(country_ie);
 }
-- 
1.8.4.rc3

[RFC 2/5] cfg80211: make cfg80211_leave_all() available outside of sysfs

[Luis R. Rodriguez]

This will be used later.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 net/wireless/core.c  | 8 ++++++++
 net/wireless/core.h  | 2 ++
 net/wireless/sysfs.c | 8 --------
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 5d53e49..818871e 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -783,6 +783,14 @@ void cfg80211_leave(struct cfg80211_registered_device *rdev,
 	wdev->beacon_interval = 0;
 }
 
+void cfg80211_leave_all(struct cfg80211_registered_device *rdev)
+{
+	struct wireless_dev *wdev;
+
+	list_for_each_entry(wdev, &rdev->wdev_list, list)
+		cfg80211_leave(rdev, wdev);
+}
+
 static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 					 unsigned long state, void *ptr)
 {
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 5390aeb..1ee0c01 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -459,6 +459,8 @@ void cfg80211_update_iface_num(struct cfg80211_registered_device *rdev,
 void cfg80211_leave(struct cfg80211_registered_device *rdev,
 		    struct wireless_dev *wdev);
 
+void cfg80211_leave_all(struct cfg80211_registered_device *rdev);
+
 void cfg80211_stop_p2p_device(struct cfg80211_registered_device *rdev,
 			      struct wireless_dev *wdev);
 
diff --git a/net/wireless/sysfs.c b/net/wireless/sysfs.c
index 9ee6bc1..de9adcf 100644
--- a/net/wireless/sysfs.c
+++ b/net/wireless/sysfs.c
@@ -87,14 +87,6 @@ static int wiphy_uevent(struct device *dev, struct kobj_uevent_env *env)
 }
 
 #ifdef CONFIG_PM
-static void cfg80211_leave_all(struct cfg80211_registered_device *rdev)
-{
-	struct wireless_dev *wdev;
-
-	list_for_each_entry(wdev, &rdev->wdev_list, list)
-		cfg80211_leave(rdev, wdev);
-}
-
 static int wiphy_suspend(struct device *dev, pm_message_t state)
 {
 	struct cfg80211_registered_device *rdev = dev_to_rdev(dev);
-- 
1.8.4.rc3

[RFC 3/5] cfg80211: add regulatory quiescing support

[Luis R. Rodriguez]

This quiesces devices when appropriate to ensure that
regulatory domain updates take effect and avoid having
devices initiate radiation when they should not.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 net/wireless/reg.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index e5935c2..3036093 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1287,12 +1287,65 @@ static void reg_call_notifier(struct wiphy *wiphy,
 		wiphy->reg_notifier(wiphy, request);
 }
 
+static void wiphy_regulatory_quiesce_all(struct wiphy *wiphy)
+{
+	struct cfg80211_registered_device *rdev;
+
+	rdev = wiphy_to_dev(wiphy);
+	if (!rdev)
+		return;
+
+	cfg80211_leave_all(rdev);
+}
+
+static void wiphy_regulatory_quiesce_skip_wdev(struct wiphy *wiphy,
+					       struct regulatory_request *lr)
+{
+	struct cfg80211_registered_device *rdev;
+	struct wiphy *last_wiphy = NULL;
+	struct wireless_dev *wdev;
+
+	rdev = wiphy_to_dev(wiphy);
+	if (!rdev)
+		return;
+
+	last_wiphy = wiphy_idx_to_wiphy(lr->wiphy_idx);
+	if (last_wiphy != wiphy) {
+		cfg80211_leave_all(rdev);
+		return;
+	}
+
+	list_for_each_entry(wdev, &rdev->wdev_list, list)
+		if (lr->wdev != wdev)
+			cfg80211_leave(rdev, wdev);
+}
+
+static void wiphy_regulatory_quiesce(struct wiphy *wiphy,
+				     struct regulatory_request *lr)
+{
+	switch (lr->initiator) {
+	case NL80211_REGDOM_SET_BY_CORE:
+		return;
+	case NL80211_REGDOM_SET_BY_USER:
+		wiphy_regulatory_quiesce_all(wiphy);
+		return;
+	case NL80211_REGDOM_SET_BY_DRIVER:
+		wiphy_regulatory_quiesce_all(wiphy);
+		return;
+	case NL80211_REGDOM_SET_BY_COUNTRY_IE:
+		wiphy_regulatory_quiesce_skip_wdev(wiphy, lr);
+		return;
+	}
+}
+
 static void wiphy_update_regulatory(struct wiphy *wiphy,
 				    enum nl80211_reg_initiator initiator)
 {
 	enum ieee80211_band band;
 	struct regulatory_request *lr = get_last_request();
 
+	wiphy_regulatory_quiesce(wiphy, lr);
+
 	if (ignore_reg_update(wiphy, initiator)) {
 		/*
 		 * Regulatory updates set by CORE are ignored for custom
-- 
1.8.4.rc3

[RFC 4/5] cfg80211: add DFS region capability support

[Luis R. Rodriguez]

This lets drivers explicitly enable specific DFS regions
and sends these to userspace when at least one is set.
This is to be used for modes of operation that require
DFS to be implemented in kernel space. cfg80211 will
only send the DFS regions to userspace for the wiphy
if the DFS region for the device agrees with the central
cfg80211 regulatory domain DFS region. In case of
regulatory updates the regulatory core already takes
care of quiescing devices when required. Userspace
should always query the support DFS region prior to
enabling usage of a DFS channel for a mode of operation
that requires DFS support in kernel.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 include/net/cfg80211.h       | 57 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/nl80211.h | 21 +++++++++++++++-
 net/wireless/core.c          | 39 ++++++++++++++++++++++++++++++
 net/wireless/nl80211.c       | 10 ++++++++
 4 files changed, 126 insertions(+), 1 deletion(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 817d813..0f43eb3 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -2701,6 +2701,19 @@ struct wiphy_coalesce_support {
  * @flags: wiphy flags, see &enum wiphy_flags
  * @regulatory_flags: wiphy regulatory flags, see
  * 	&enum ieee80211_regulatory_flags
+ * @dfs_regions: bitmask of the supported DFS regions that this device has
+ * 	support for on the driver. Having DFS region support means the
+ * 	device driver is capable of supporting radar detection as per that
+ * 	DFS region's rules. Radar detection is required prior to initiating
+ * 	radiation on channels that have the IEEE80211_CHAN_RADAR flag set.
+ * 	Modes of operation that require DFS support in kernel must ensure
+ * 	that initiating radiation will not happen if there is a mismatch
+ * 	of the supported DFS regions with wiphy_core_dfs_region_usable().
+ * 	Device drivers that have DFS support should pass each supported
+ * 	&enum nl80211_dfs_regions with wiphy_enable_dfs_region(). This will
+ * 	set the respective bitmask on dfs_regions. IBSS mode of operation
+ * 	that enables %NL80211_ATTR_HANDLE_DFS need not set the supported
+ * 	DFS regions, but should ensure it supports the core's set DFS region.
  * @features: features advertised to nl80211, see &enum nl80211_feature_flags.
  * @bss_priv_size: each BSS struct has private data allocated with it,
  *	this variable determines its size
@@ -2790,6 +2803,7 @@ struct wiphy {
 	u16 max_acl_mac_addrs;
 
 	u32 flags, regulatory_flags, features;
+	u32 dfs_regions;
 
 	u32 ap_sme_capa;
 
@@ -2988,6 +3002,49 @@ void wiphy_unregister(struct wiphy *wiphy);
  */
 void wiphy_free(struct wiphy *wiphy);
 
+/**
+ * wiphy_enable_dfs_region - enable a DFS region
+ *
+ * @wiphy: the wiphy to set the supported DFS regions for
+ * @dfs_region: an DFS region specified by an &enum nl80211_dfs_regions
+ * 	representing the DFS region to enable support on the wiphy for.
+ *
+ * This can be used to indicate to cfg80211 that the wiphy has DFS driver
+ * support for the specified DFS region for modes of operation that require
+ * DFS on the driver.
+ */
+void wiphy_enable_dfs_region(struct wiphy *wiphy,
+			     enum nl80211_dfs_regions dfs_region);
+
+/**
+ * wiphy_dfs_region_supported - checks if a DFS region is supported
+ *
+ * @wiphy: the wiphy to check the DFS region for
+ * @dfs_region: the DFS region we want to check support for
+ *
+ * This can be used to query if the wiphy's a specific DFS region.
+ * This should be checked before for initiating radiation on
+ * DFS channels for modes of operation that require DFS on the driver.
+ *
+ * Return: true if the dfs_region is supported by the device, returns
+ * false otherwise.
+ */
+bool wiphy_dfs_region_supported(struct wiphy *wiphy,
+				enum nl80211_dfs_regions dfs_region);
+
+/**
+ * wiphy_core_dfs_region_usable - checks if the current DFS region can be used
+ *
+ * @wiphy: the wiphy to check the DFS region against
+ *
+ * This can be used to query if the wiphy can use the currently set
+ * DFS region on the regulatory core.
+ *
+ * Return: true if the core's dfs_region is supported and usable by the device,
+ * returns false otherwise.
+ */
+bool wiphy_core_dfs_region_usable(struct wiphy *wiphy);
+
 /* internal structs */
 struct cfg80211_conn;
 struct cfg80211_internal_bss;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 7e25164..ecb3d88 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -1513,7 +1513,23 @@ enum nl80211_commands {
  *	%NL80211_CMD_JOIN_IBSS request, the driver will allow use of DFS
  *	channels and reports radar events to userspace. Userspace is required
  *	to react to radar events, e.g. initiate a channel switch or leave the
- *	IBSS network.
+ *	IBSS network. Userspace is also required to verify that the currently
+ *	programmed DFS region is supported by userspace and monitor it in case
+ *	of changes.
+ *
+ * @NL80211_ATTR_DFS_REGIONS: bitmask of all supported, tested, and
+ * 	certified &enum nl80211_dfs_regions that a wiphy has been declared to
+ *	support and that agrees with what is programmed currently on cfg80211.
+ *	This is used for modes of operation that require DFS support on the
+ *	driver. If %NL80211_ATTR_HANDLE_DFS is set userspace need not check
+ *	for this for IBSS as it will support DFS in userspace for IBSS.
+ *	Userspace can rely on %NL80211_ATTR_DFS_REGIONS to enable support for
+ *	modes of operation on channels flagged with %NL80211_RRF_DFS, the
+ *	kernel / driver will take care of DFS operation. If
+ *	%NL80211_ATTR_DFS_REGIONS is not set DFS is not supported or
+ *	does not match the DFS region currently set on cfg80211 and
+ *	userspace must not enable operation on channels with
+ *	%NL80211_RRF_DFS set.
  *
  * @NL80211_ATTR_MAX: highest attribute number currently defined
  * @__NL80211_ATTR_AFTER_LAST: internal use
@@ -1831,6 +1847,8 @@ enum nl80211_attrs {
 
 	NL80211_ATTR_HANDLE_DFS,
 
+	NL80211_ATTR_DFS_REGIONS,
+
 	/* add attributes here, update the policy in nl80211.c */
 
 	__NL80211_ATTR_AFTER_LAST,
@@ -1866,6 +1884,7 @@ enum nl80211_attrs {
 #define NL80211_ATTR_KEY NL80211_ATTR_KEY
 #define NL80211_ATTR_KEYS NL80211_ATTR_KEYS
 #define NL80211_ATTR_FEATURE_FLAGS NL80211_ATTR_FEATURE_FLAGS
+#define NL80211_ATTR_DFS_REGIONS NL80211_ATTR_DFS_REGIONS
 
 #define NL80211_MAX_SUPP_RATES			32
 #define NL80211_MAX_SUPP_HT_RATES		77
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 818871e..abd39c3 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -361,6 +361,45 @@ struct wiphy *wiphy_new(const struct cfg80211_ops *ops, int sizeof_priv)
 }
 EXPORT_SYMBOL(wiphy_new);
 
+void wiphy_enable_dfs_region(struct wiphy *wiphy,
+			     enum nl80211_dfs_regions dfs_region)
+{
+	if (!config_enabled(CONFIG_CFG80211_CERTIFICATION_ONUS))
+		return;
+	if (!reg_supported_dfs_region(dfs_region))
+		return;
+
+	wiphy->dfs_regions |= BIT(dfs_region);
+}
+EXPORT_SYMBOL_GPL(wiphy_enable_dfs_region);
+
+bool wiphy_dfs_region_supported(struct wiphy *wiphy,
+				enum nl80211_dfs_regions dfs_region)
+{
+	if (!config_enabled(CONFIG_CFG80211_CERTIFICATION_ONUS))
+		return false;
+	if (!reg_supported_dfs_region(dfs_region))
+		return false;
+	if (dfs_region == NL80211_DFS_UNSET)
+		return false;
+	if (BIT(dfs_region) & wiphy->dfs_regions)
+		return true;
+	return false;
+}
+
+bool wiphy_core_dfs_region_usable(struct wiphy *wiphy)
+{
+	enum nl80211_dfs_regions cfg80211_dfs_region;
+
+	ASSERT_RTNL();
+
+	cfg80211_dfs_region = reg_get_dfs_region(wiphy);
+	if (!wiphy_dfs_region_supported(wiphy, cfg80211_dfs_region))
+		return false;
+	return true;
+}
+EXPORT_SYMBOL_GPL(wiphy_core_dfs_region_usable);
+
 static int wiphy_verify_combinations(struct wiphy *wiphy)
 {
 	const struct ieee80211_iface_combination *c;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index c33b374..dcbc083 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -357,6 +357,7 @@ static const struct nla_policy nl80211_policy[NL80211_ATTR_MAX+1] = {
 	[NL80211_ATTR_STA_SUPPORTED_CHANNELS] = { .type = NLA_BINARY },
 	[NL80211_ATTR_STA_SUPPORTED_OPER_CLASSES] = { .type = NLA_BINARY },
 	[NL80211_ATTR_HANDLE_DFS] = { .type = NLA_FLAG },
+	[NL80211_ATTR_DFS_REGIONS] = { .type = NLA_U32 },
 };
 
 /* policy for the key attributes */
@@ -1559,6 +1560,15 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *dev,
 	case 10:
 		if (nl80211_send_coalesce(msg, dev))
 			goto nla_put_failure;
+		state->split_start++;
+		break;
+	case 11:
+		if (wiphy_core_dfs_region_usable(&dev->wiphy) &&
+		    dev->wiphy.dfs_regions &&
+		    nla_put_u32(msg,
+				NL80211_ATTR_DFS_REGIONS,
+				dev->wiphy.dfs_regions))
+			goto nla_put_failure;
 
 		/* done */
 		state->split_start = 0;
-- 
1.8.4.rc3

[RFC 5/5] cfg80211: DFS check dfs_region before usage

[Luis R. Rodriguez]

Check the DFS region before channel availability check
or declaring a channel as DFS usable.

Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 net/wireless/chan.c    | 8 ++++++++
 net/wireless/nl80211.c | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 78559b5..4e6eaa0 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
 	struct ieee80211_sta_ht_cap *ht_cap;
 	struct ieee80211_sta_vht_cap *vht_cap;
 	u32 width, control_freq;
+	enum nl80211_dfs_regions dfs_region;
 
 	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
 		return false;
 
+	rtnl_lock();
+	dfs_region = reg_get_dfs_region(wiphy);
+	rtnl_unlock();
+
+	if (dfs_region == NL80211_DFS_UNSET)
+		return false;
+
 	ht_cap = &wiphy->bands[chandef->chan->band]->ht_cap;
 	vht_cap = &wiphy->bands[chandef->chan->band]->vht_cap;
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index dcbc083..1acf45c 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5641,8 +5641,13 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	struct net_device *dev = info->user_ptr[1];
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_chan_def chandef;
+	enum nl80211_dfs_regions dfs_region;
 	int err;
 
+	dfs_region = reg_get_dfs_region(wdev->wiphy);
+	if (dfs_region == NL80211_DFS_UNSET)
+		 return -EINVAL;
+
 	err = nl80211_parse_chandef(rdev, info, &chandef);
 	if (err)
 		return err;
-- 
1.8.4.rc3

Re: [RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Johannes Berg]

On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:

> Janusz, please test, and likely feel free to take on the last patch.
> We may need help from Johannes on the rtnl lock question now required
> for checks on enabling beacon operation.

What's that rtnl thing about?

johannes

Re: [RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 7:48 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
>
>> Janusz, please test, and likely feel free to take on the last patch.
>> We may need help from Johannes on the rtnl lock question now required
>> for checks on enabling beacon operation.
>
> What's that rtnl thing about?

Whether or not callers of cfg80211_reg_can_beacon() have it properly
held already and if not best strategy to make this simple and concise.
The rtnl is required as we need to now query the cfg80211 regdom dfs
region for the case where a channel requires DFS.

  Luis

Re: [RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Johannes Berg]

On Wed, 2013-11-13 at 19:57 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 13, 2013 at 7:48 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> >
> >> Janusz, please test, and likely feel free to take on the last patch.
> >> We may need help from Johannes on the rtnl lock question now required
> >> for checks on enabling beacon operation.
> >
> > What's that rtnl thing about?
> 
> Whether or not callers of cfg80211_reg_can_beacon() have it properly
> held already and if not best strategy to make this simple and concise.
> The rtnl is required as we need to now query the cfg80211 regdom dfs
> region for the case where a channel requires DFS.

Can't you just use RCU for that part?

johannes

Re: [RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Johannes Berg]

On Wed, 2013-11-13 at 19:57 +0100, Luis R. Rodriguez wrote:
> On Wed, Nov 13, 2013 at 7:48 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> >
> >> Janusz, please test, and likely feel free to take on the last patch.
> >> We may need help from Johannes on the rtnl lock question now required
> >> for checks on enabling beacon operation.
> >
> > What's that rtnl thing about?
> 
> Whether or not callers of cfg80211_reg_can_beacon() have it properly
> held already and if not best strategy to make this simple and concise.
> The rtnl is required as we need to now query the cfg80211 regdom dfs
> region for the case where a channel requires DFS.

Can't you just use RCU for that part?

johannes

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Johannes Berg]

On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> This also records the wdev on the last regulatory request,
> this can be used later to help quiesce wdev's at appropriate
> times by the regulatory core.

> +++ b/net/wireless/reg.c
> @@ -1871,12 +1871,14 @@ int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
>  }
>  EXPORT_SYMBOL(regulatory_hint);
>  
> -void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
> +void regulatory_hint_country_ie(struct wireless_dev *wdev,
> +				enum ieee80211_band band,
>  				const u8 *country_ie, u8 country_ie_len)
...
> +       request->wdev = wdev;
> 
You have absolutely no validation of this pointer - the lifetime of the
request object and the wdev aren't necessarily the same.

At least you should very carefully document that this pointer is a
cookie (if it really is) and must never be dereferenced.

johannes

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Johannes Berg]

On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> This quiesces devices when appropriate to ensure that
> regulatory domain updates take effect and avoid having
> devices initiate radiation when they should not.

I'm not really sure this makes sense.

If we're staying connected, how can we be moving far enough to go
through regulatory domains to have totally different rules?

(Also, probably best to squash patches 2 and 3, 2 is pointless by itself
and the code move is obvious with this patch)

johannes

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Johannes Berg]

On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:

> +/**
> + * wiphy_enable_dfs_region - enable a DFS region
> + *
> + * @wiphy: the wiphy to set the supported DFS regions for
> + * @dfs_region: an DFS region specified by an &enum nl80211_dfs_regions
> + * 	representing the DFS region to enable support on the wiphy for.
> + *
> + * This can be used to indicate to cfg80211 that the wiphy has DFS driver
> + * support for the specified DFS region for modes of operation that require
> + * DFS on the driver.
> + */
> +void wiphy_enable_dfs_region(struct wiphy *wiphy,
> +			     enum nl80211_dfs_regions dfs_region);

What's the point of this function rather than the driver setting the
region bits before registration?

> +/**
> + * wiphy_dfs_region_supported - checks if a DFS region is supported
> + *
> + * @wiphy: the wiphy to check the DFS region for
> + * @dfs_region: the DFS region we want to check support for
> + *
> + * This can be used to query if the wiphy's a specific DFS region.
> + * This should be checked before for initiating radiation on
> + * DFS channels for modes of operation that require DFS on the driver.
> + *
> + * Return: true if the dfs_region is supported by the device, returns
> + * false otherwise.
> + */
> +bool wiphy_dfs_region_supported(struct wiphy *wiphy,
> +				enum nl80211_dfs_regions dfs_region);

Where would this function be used outside of cfg80211?

> +/**
> + * wiphy_core_dfs_region_usable - checks if the current DFS region can be used
> + *
> + * @wiphy: the wiphy to check the DFS region against
> + *
> + * This can be used to query if the wiphy can use the currently set
> + * DFS region on the regulatory core.
> + *
> + * Return: true if the core's dfs_region is supported and usable by the device,
> + * returns false otherwise.
> + */
> +bool wiphy_core_dfs_region_usable(struct wiphy *wiphy);

Ditto.

> + *	IBSS network. Userspace is also required to verify that the currently
> + *	programmed DFS region is supported by userspace and monitor it in case
> + *	of changes.

That seems like wishful thinking since userspace already exists for
this.

> + * @NL80211_ATTR_DFS_REGIONS: bitmask of all supported, tested, and
> + * 	certified &enum nl80211_dfs_regions that a wiphy has been declared to
> + *	support and that agrees with what is programmed currently on cfg80211.
> + *	This is used for modes of operation that require DFS support on the
> + *	driver. If %NL80211_ATTR_HANDLE_DFS is set userspace need not check
> + *	for this for IBSS as it will support DFS in userspace for IBSS.

???
You're confusing me. What's the point of these patches then if userspace
handles it?

The way I read the patch description was that here you were advertising
which pattern detectors a driver has - but that has nothing to do with
how userspace handles DFS in IBSS.

> +#define NL80211_ATTR_DFS_REGIONS NL80211_ATTR_DFS_REGIONS

Not needed.
 
> +void wiphy_enable_dfs_region(struct wiphy *wiphy,
> +			     enum nl80211_dfs_regions dfs_region)
> +{
> +	if (!config_enabled(CONFIG_CFG80211_CERTIFICATION_ONUS))
> +		return;
> +	if (!reg_supported_dfs_region(dfs_region))
> +		return;

Both of this is pointless, you can do it at build time.

> +	wiphy->dfs_regions |= BIT(dfs_region);

Just have the driver set this directly before registration and make
*using* and *advertising* it conditional.

johannes

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Johannes Berg]

On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> Check the DFS region before channel availability check
> or declaring a channel as DFS usable.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
> ---
>  net/wireless/chan.c    | 8 ++++++++
>  net/wireless/nl80211.c | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> index 78559b5..4e6eaa0 100644
> --- a/net/wireless/chan.c
> +++ b/net/wireless/chan.c
> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
>  	struct ieee80211_sta_ht_cap *ht_cap;
>  	struct ieee80211_sta_vht_cap *vht_cap;
>  	u32 width, control_freq;
> +	enum nl80211_dfs_regions dfs_region;
>  
>  	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
>  		return false;
>  
> +	rtnl_lock();
> +	dfs_region = reg_get_dfs_region(wiphy);
> +	rtnl_unlock();

This I'm fairly sure will just deadlock.

Anyway, this and patch 4 together can be done MUCH simpler by just
having the driver advertise the bitmap of what it supports before
registration, and then not using it if CERTIFICATION_ONUS isn't enabled
or something.

johannes

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Janusz Dziedzic]

On 13 November 2013 22:29, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
>> Check the DFS region before channel availability check
>> or declaring a channel as DFS usable.
>>
>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>> ---
>>  net/wireless/chan.c    | 8 ++++++++
>>  net/wireless/nl80211.c | 5 +++++
>>  2 files changed, 13 insertions(+)
>>
>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> index 78559b5..4e6eaa0 100644
>> --- a/net/wireless/chan.c
>> +++ b/net/wireless/chan.c
>> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
>>       struct ieee80211_sta_ht_cap *ht_cap;
>>       struct ieee80211_sta_vht_cap *vht_cap;
>>       u32 width, control_freq;
>> +     enum nl80211_dfs_regions dfs_region;
>>
>>       if (WARN_ON(!cfg80211_chandef_valid(chandef)))
>>               return false;
>>
>> +     rtnl_lock();
>> +     dfs_region = reg_get_dfs_region(wiphy);
>> +     rtnl_unlock();
>


Do we need check dfs_region in cfg80211_can_beacon() at all?
We already check first if all channels NL80211_DFS_AVAILABLE.
To be DFS_AVAILABLE we need pass CAC, and we will fail CAC if
dfs_region == UNSET.

Anyway we can do something like this in cfg80211_can_beacon()

        if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
-           cfg80211_chandef_dfs_available(wiphy, chandef)) {
+           cfg80211_chandef_dfs_available(wiphy, chandef) &&
+           reg_get_dfs_region(wiphy) != NL80211_DFS_UNSET) {
...
And change doc that cfg80211_can_beacon() require rtnl_lock.
But I think this is not required.

BR
Janusz

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Janusz Dziedzic]

On 14 November 2013 11:15, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote:
> On 13 November 2013 22:29, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
>>> Check the DFS region before channel availability check
>>> or declaring a channel as DFS usable.
>>>
>>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>>> ---
>>>  net/wireless/chan.c    | 8 ++++++++
>>>  net/wireless/nl80211.c | 5 +++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>>> index 78559b5..4e6eaa0 100644
>>> --- a/net/wireless/chan.c
>>> +++ b/net/wireless/chan.c
>>> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
>>>       struct ieee80211_sta_ht_cap *ht_cap;
>>>       struct ieee80211_sta_vht_cap *vht_cap;
>>>       u32 width, control_freq;
>>> +     enum nl80211_dfs_regions dfs_region;
>>>
>>>       if (WARN_ON(!cfg80211_chandef_valid(chandef)))
>>>               return false;
>>>
>>> +     rtnl_lock();
>>> +     dfs_region = reg_get_dfs_region(wiphy);
>>> +     rtnl_unlock();
>>
>
>
> Do we need check dfs_region in cfg80211_can_beacon() at all?
> We already check first if all channels NL80211_DFS_AVAILABLE.
> To be DFS_AVAILABLE we need pass CAC, and we will fail CAC if
> dfs_region == UNSET.
>
> Anyway we can do something like this in cfg80211_can_beacon()
>
>         if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
> -           cfg80211_chandef_dfs_available(wiphy, chandef)) {
> +           cfg80211_chandef_dfs_available(wiphy, chandef) &&
> +           reg_get_dfs_region(wiphy) != NL80211_DFS_UNSET) {
> ...
> And change doc that cfg80211_can_beacon() require rtnl_lock.
> But I think this is not required.
>
Or if we should handle dfs_region change after we start CAC and start beaconing,
we could add __cfg80211_can_beacon() without rtnl locking and
cfg80211_can_beacon() with locking.

BTW
Luis should we handle dfs_region change also during CAC? Who should
fail CAC in case we switch eg. from ETSI to FCC? Should we handle this
in driver?

BR
Janusz

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Johannes Berg]

On Thu, 2013-11-14 at 06:05 -0800, Luis R. Rodriguez wrote:

> > > -void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
> > > +void regulatory_hint_country_ie(struct wireless_dev *wdev,
> > > +				enum ieee80211_band band,
> > >  				const u8 *country_ie, u8 country_ie_len)
> > ...
> > > +       request->wdev = wdev;
> > > 
> > You have absolutely no validation of this pointer - the lifetime of the
> > request object and the wdev aren't necessarily the same.
> 
> Agreed.
> 
> > At least you should very carefully document that this pointer is a
> > cookie (if it really is) and must never be dereferenced.
> 
> OK, I could also validate it upon processing but we'd need to loop
> over the rdev wdev list for the country IE hint, if that is acceptable
> upon procesing it'd be valid and we'd avoid corner case issues.
> Thoughts?

As far as I can tell you already don't use it in any other way but a
cookie pointer, comparing while iterating the then-current list of
wdevs. But that's kinda unsafe. Might also be worth just making it a
void* to avoid people trying to use it.

johannes

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 10:20:37PM +0100, Johannes Berg wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > This also records the wdev on the last regulatory request,
> > this can be used later to help quiesce wdev's at appropriate
> > times by the regulatory core.
> 
> > +++ b/net/wireless/reg.c
> > @@ -1871,12 +1871,14 @@ int regulatory_hint(struct wiphy *wiphy, const char *alpha2)
> >  }
> >  EXPORT_SYMBOL(regulatory_hint);
> >  
> > -void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
> > +void regulatory_hint_country_ie(struct wireless_dev *wdev,
> > +				enum ieee80211_band band,
> >  				const u8 *country_ie, u8 country_ie_len)
> ...
> > +       request->wdev = wdev;
> > 
> You have absolutely no validation of this pointer - the lifetime of the
> request object and the wdev aren't necessarily the same.

Agreed.

> At least you should very carefully document that this pointer is a
> cookie (if it really is) and must never be dereferenced.

OK, I could also validate it upon processing but we'd need to loop
over the rdev wdev list for the country IE hint, if that is acceptable
upon procesing it'd be valid and we'd avoid corner case issues.
Thoughts?

  Luis

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 10:22:18PM +0100, Johannes Berg wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > This quiesces devices when appropriate to ensure that
> > regulatory domain updates take effect and avoid having
> > devices initiate radiation when they should not.
> 
> I'm not really sure this makes sense.
> 
> If we're staying connected, how can we be moving far enough to go
> through regulatory domains to have totally different rules?

Don't think of what makes sense, think of the corner cases that
could happen here, such as plugging in a card that disagrees
with regulatory settings, and creates a conflict on DFS regions.
One example might be someone plugging in a USB 802.11 card programmed
for JP in say a FR 802.11 AP, assuming the AP had the USB 802.11
driver. Another example may be if a user provides an input with
say 'iw reg set JP' on a FR AP. In such cases we want to stop IR,
even if the user was dumb, we'd be respecting the regulatory settings,
as silly as they may be.

Another more resonable case may be one AP creating a secondary
wdev as a STA, consider a P2P GO, with a P2P STA on another channel
and lets assume the P2P GO is misprogrammed for JP but the STA
wdev picks up association with an AP from FR.

> (Also, probably best to squash patches 2 and 3, 2 is pointless by itself
> and the code move is obvious with this patch)

OK.

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 10:28:16PM +0100, Johannes Berg wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> 
> > +/**
> > + * wiphy_enable_dfs_region - enable a DFS region
> > + *
> > + * @wiphy: the wiphy to set the supported DFS regions for
> > + * @dfs_region: an DFS region specified by an &enum nl80211_dfs_regions
> > + * 	representing the DFS region to enable support on the wiphy for.
> > + *
> > + * This can be used to indicate to cfg80211 that the wiphy has DFS driver
> > + * support for the specified DFS region for modes of operation that require
> > + * DFS on the driver.
> > + */
> > +void wiphy_enable_dfs_region(struct wiphy *wiphy,
> > +			     enum nl80211_dfs_regions dfs_region);
> 
> What's the point of this function rather than the driver setting the
> region bits before registration?

I don't trust driver developers, this provides a simple interface
which pushes driver developers to use CONFIG_CFG80211_CERTIFICATION_ONUS
for driver DFS features, it also validates the input and also does
the conversion to bitmasks for the driver developers. Without this
we'd assume driver developers would enable DFS under proper regulatory
configs but also require BIT() | BIT() settings.

> > +/**
> > + * wiphy_dfs_region_supported - checks if a DFS region is supported
> > + *
> > + * @wiphy: the wiphy to check the DFS region for
> > + * @dfs_region: the DFS region we want to check support for
> > + *
> > + * This can be used to query if the wiphy's a specific DFS region.
> > + * This should be checked before for initiating radiation on
> > + * DFS channels for modes of operation that require DFS on the driver.
> > + *
> > + * Return: true if the dfs_region is supported by the device, returns
> > + * false otherwise.
> > + */
> > +bool wiphy_dfs_region_supported(struct wiphy *wiphy,
> > +				enum nl80211_dfs_regions dfs_region);
> 
> Where would this function be used outside of cfg80211?

It would not, I can stuff that alone into core.c and make it static.

> > +/**
> > + * wiphy_core_dfs_region_usable - checks if the current DFS region can be used
> > + *
> > + * @wiphy: the wiphy to check the DFS region against
> > + *
> > + * This can be used to query if the wiphy can use the currently set
> > + * DFS region on the regulatory core.
> > + *
> > + * Return: true if the core's dfs_region is supported and usable by the device,
> > + * returns false otherwise.
> > + */
> > +bool wiphy_core_dfs_region_usable(struct wiphy *wiphy);
> 
> Ditto.

This was a question I had, but based on Januz' feedback it sounds like
we can keep this private.

> > + *	IBSS network. Userspace is also required to verify that the currently
> > + *	programmed DFS region is supported by userspace and monitor it in case
> > + *	of changes.
> 
> That seems like wishful thinking since userspace already exists for
> this.

It is, but its also a heads up as to perhaps what we should do to help
userspace, I think I mentioned elsewhere we could require userspace DFS
support to supply the supported DFS regions and then we'd deal with the
conflicts.

The way I'd envision support for that is to add a user_dfs_regions and
a respective cmd which userspace can set, if the kernel supports
it userspace can ifdef it (with the define trick on nl80211.h) and
then send this over for the supported DFS regions for new IBSS userspace.
We'd then get an enhancement moving forward, otherwise older kernels
would require userspace to handle DFS region mismatches / updates /
quiescing itself.

> > + * @NL80211_ATTR_DFS_REGIONS: bitmask of all supported, tested, and
> > + * 	certified &enum nl80211_dfs_regions that a wiphy has been declared to
> > + *	support and that agrees with what is programmed currently on cfg80211.
> > + *	This is used for modes of operation that require DFS support on the
> > + *	driver. If %NL80211_ATTR_HANDLE_DFS is set userspace need not check
> > + *	for this for IBSS as it will support DFS in userspace for IBSS.
> 
> ???
> You're confusing me. What's the point of these patches then if userspace
> handles it?

NL80211_ATTR_HANDLE_DFS seems to be only for IBSS userspace, I'm trying
to provide clarification that this is not required for
NL80211_ATTR_HANDLE_DFS.

> 
> The way I read the patch description was that here you were advertising
> which pattern detectors a driver has - but that has nothing to do with
> how userspace handles DFS in IBSS.

Agreed, I'm just providing clarification to the special
IBSS case when NL80211_ATTR_HANDLE_DFS is used, so that
useres of IBSS with DFS in userspace and NL80211_ATTR_HANDLE_DFS
don't set the DFS regions on their driver. As I mentioned
above though, a user_dfs_regions however is welcomed.

> > +#define NL80211_ATTR_DFS_REGIONS NL80211_ATTR_DFS_REGIONS
> 
> Not needed.

Why is that?

> > +void wiphy_enable_dfs_region(struct wiphy *wiphy,
> > +			     enum nl80211_dfs_regions dfs_region)
> > +{
> > +	if (!config_enabled(CONFIG_CFG80211_CERTIFICATION_ONUS))
> > +		return;
> > +	if (!reg_supported_dfs_region(dfs_region))
> > +		return;
> 
> Both of this is pointless, you can do it at build time.

Hrm.

> > +	wiphy->dfs_regions |= BIT(dfs_region);
> 
> Just have the driver set this directly before registration and make
> *using* and *advertising* it conditional.

OK yeah that works too, thanks for the feedback.

  Luis

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 10:29:33PM +0100, Johannes Berg wrote:
> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > Check the DFS region before channel availability check
> > or declaring a channel as DFS usable.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
> > ---
> >  net/wireless/chan.c    | 8 ++++++++
> >  net/wireless/nl80211.c | 5 +++++
> >  2 files changed, 13 insertions(+)
> > 
> > diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> > index 78559b5..4e6eaa0 100644
> > --- a/net/wireless/chan.c
> > +++ b/net/wireless/chan.c
> > @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
> >  	struct ieee80211_sta_ht_cap *ht_cap;
> >  	struct ieee80211_sta_vht_cap *vht_cap;
> >  	u32 width, control_freq;
> > +	enum nl80211_dfs_regions dfs_region;
> >  
> >  	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> >  		return false;
> >  
> > +	rtnl_lock();
> > +	dfs_region = reg_get_dfs_region(wiphy);
> > +	rtnl_unlock();
> 
> This I'm fairly sure will just deadlock.

OK :D

Januz seems to note this may not be required anyway too, so
hopefully that's true.

> Anyway, this and patch 4 together can be done MUCH simpler by just
> having the driver advertise the bitmap of what it supports before
> registration, and then not using it if CERTIFICATION_ONUS isn't enabled
> or something.

OK, good idea.

 Luis

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 11:15:25AM +0100, Janusz Dziedzic wrote:
> On 13 November 2013 22:29, Johannes Berg <johannes@sipsolutions.net> wrote:
> > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> >> Check the DFS region before channel availability check
> >> or declaring a channel as DFS usable.
> >>
> >> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
> >> ---
> >>  net/wireless/chan.c    | 8 ++++++++
> >>  net/wireless/nl80211.c | 5 +++++
> >>  2 files changed, 13 insertions(+)
> >>
> >> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> >> index 78559b5..4e6eaa0 100644
> >> --- a/net/wireless/chan.c
> >> +++ b/net/wireless/chan.c
> >> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
> >>       struct ieee80211_sta_ht_cap *ht_cap;
> >>       struct ieee80211_sta_vht_cap *vht_cap;
> >>       u32 width, control_freq;
> >> +     enum nl80211_dfs_regions dfs_region;
> >>
> >>       if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> >>               return false;
> >>
> >> +     rtnl_lock();
> >> +     dfs_region = reg_get_dfs_region(wiphy);
> >> +     rtnl_unlock();
> >
> 
> 
> Do we need check dfs_region in cfg80211_can_beacon() at all?
> We already check first if all channels NL80211_DFS_AVAILABLE.
> To be DFS_AVAILABLE we need pass CAC, and we will fail CAC if
> dfs_region == UNSET.

SWEET!

> Anyway we can do something like this in cfg80211_can_beacon()
> 
>         if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
> -           cfg80211_chandef_dfs_available(wiphy, chandef)) {
> +           cfg80211_chandef_dfs_available(wiphy, chandef) &&
> +           reg_get_dfs_region(wiphy) != NL80211_DFS_UNSET) {
> ...
> And change doc that cfg80211_can_beacon() require rtnl_lock.
> But I think this is not required.

If its not required I'd prefer to avoid this junk and just
clarify what you said as part of the documentation, it wasn't
clear to me. I'll just drop this patch in the series.

  Luis

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Johannes Berg]

On Thu, 2013-11-14 at 06:11 -0800, Luis R. Rodriguez wrote:
> On Wed, Nov 13, 2013 at 10:22:18PM +0100, Johannes Berg wrote:
> > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > > This quiesces devices when appropriate to ensure that
> > > regulatory domain updates take effect and avoid having
> > > devices initiate radiation when they should not.
> > 
> > I'm not really sure this makes sense.
> > 
> > If we're staying connected, how can we be moving far enough to go
> > through regulatory domains to have totally different rules?
> 
> Don't think of what makes sense, think of the corner cases that
> could happen here, such as plugging in a card that disagrees
> with regulatory settings, and creates a conflict on DFS regions.
> One example might be someone plugging in a USB 802.11 card programmed
> for JP in say a FR 802.11 AP, assuming the AP had the USB 802.11
> driver. Another example may be if a user provides an input with
> say 'iw reg set JP' on a FR AP. In such cases we want to stop IR,
> even if the user was dumb, we'd be respecting the regulatory settings,
> as silly as they may be.

Think of the other way around, such as plugging in a stupid USB NIC just
to see if it works - and suddenly finding your 5 GHz connection broken
because that USB NIC said it really needed some stupid country with 2.4
only. I'm not convinced.

johannes

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 12:52:26PM +0100, Janusz Dziedzic wrote:
> On 14 November 2013 11:15, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote:
> > On 13 November 2013 22:29, Johannes Berg <johannes@sipsolutions.net> wrote:
> >> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> >>> Check the DFS region before channel availability check
> >>> or declaring a channel as DFS usable.
> >>>
> >>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
> >>> ---
> >>>  net/wireless/chan.c    | 8 ++++++++
> >>>  net/wireless/nl80211.c | 5 +++++
> >>>  2 files changed, 13 insertions(+)
> >>>
> >>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
> >>> index 78559b5..4e6eaa0 100644
> >>> --- a/net/wireless/chan.c
> >>> +++ b/net/wireless/chan.c
> >>> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
> >>>       struct ieee80211_sta_ht_cap *ht_cap;
> >>>       struct ieee80211_sta_vht_cap *vht_cap;
> >>>       u32 width, control_freq;
> >>> +     enum nl80211_dfs_regions dfs_region;
> >>>
> >>>       if (WARN_ON(!cfg80211_chandef_valid(chandef)))
> >>>               return false;
> >>>
> >>> +     rtnl_lock();
> >>> +     dfs_region = reg_get_dfs_region(wiphy);
> >>> +     rtnl_unlock();
> >>
> >
> >
> > Do we need check dfs_region in cfg80211_can_beacon() at all?
> > We already check first if all channels NL80211_DFS_AVAILABLE.
> > To be DFS_AVAILABLE we need pass CAC, and we will fail CAC if
> > dfs_region == UNSET.
> >
> > Anyway we can do something like this in cfg80211_can_beacon()
> >
> >         if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
> > -           cfg80211_chandef_dfs_available(wiphy, chandef)) {
> > +           cfg80211_chandef_dfs_available(wiphy, chandef) &&
> > +           reg_get_dfs_region(wiphy) != NL80211_DFS_UNSET) {
> > ...
> > And change doc that cfg80211_can_beacon() require rtnl_lock.
> > But I think this is not required.
> >
> Or if we should handle dfs_region change after we start CAC and
> start beaconing, 

Can you clarify what you mean?

Right now update_all_wiphy_regulatory() contends on the rtnl_lock(),
I am not sure if starting CAC and beaconing does. I also just did
not get what you mean here.

> we could add __cfg80211_can_beacon() without rtnl
> locking and cfg80211_can_beacon() with locking.

If to use DFS we need DFS_AVAILABLE and if it is *never* set
if dfs region is unset I think we should be good *if* we handle
quiescing properly.

> BTW
> Luis should we handle dfs_region change also during CAC? Who should
> fail CAC in case we switch eg. from ETSI to FCC? Should we handle this
> in driver?

I gave this a lot of thought and decided to just do it in kernel
on cfg80211 by quescing upon regulatory domain changes where
appropriately (not just for DFS region change), *and* by having
wiphy_core_dfs_region_usable() always be checked before sending
to userspace the supported DFS regions. The trick here is that
since we are quiescing it means that userspace in theory (if
we do quescing properly) will always have to check query for the
DFS regions *anytime* it will want to start a new interface for
initiating radiation, ie just once upon init. If the DFS region
changes quescing would have turned off radiation and what I was
aiming for is for then userspace to have to get the DFS region
again and only if its set would it then again start radiation.

Doing it this way avoid us having to do anything on any driver.
It also allows us to not even have to deal with this in userspace,
which would have required userspace to provide support for handling
DFS region changes by monitoring the multicast regulatory group,
and upon a regulatory change, if a DFS region change, or any
regulatory change queisce itself.

Notice that upon a regulatory intersection we end up using DFS
unset if the DFS regions disagree, and that we now always use
the central cfg80211 DFS region. This ensures that DFS is tied
down to regulatory agreement on the system.

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Johannes Berg]

On Thu, 2013-11-14 at 06:31 -0800, Luis R. Rodriguez wrote:

> > What's the point of this function rather than the driver setting the
> > region bits before registration?
> 
> I don't trust driver developers, this provides a simple interface
> which pushes driver developers to use CONFIG_CFG80211_CERTIFICATION_ONUS
> for driver DFS features, it also validates the input and also does
> the conversion to bitmasks for the driver developers. Without this
> we'd assume driver developers would enable DFS under proper regulatory
> configs but also require BIT() | BIT() settings.

It has nothing to do with trust. If the driver author sets

	dfs_capability = BIT(FCC) | BIT(ETSI)

you still don't have to use the values if CERTIFICATION_ONUS isn't set.
Therefore, I'm not buying it.

> > > + *	IBSS network. Userspace is also required to verify that the currently
> > > + *	programmed DFS region is supported by userspace and monitor it in case
> > > + *	of changes.
> > 
> > That seems like wishful thinking since userspace already exists for
> > this.
> 
> It is, but its also a heads up as to perhaps what we should do to help
> userspace, I think I mentioned elsewhere we could require userspace DFS
> support to supply the supported DFS regions and then we'd deal with the
> conflicts.

I really don't get why userspace should have to deal with DFS regions.
You know what region you're in, you know which the hardware supports -
that's all userspace should care about. We can even make up the "is DFS
supported" flag on-the-fly based on all this information.

Pushing any of this to userspace just seems really stupid.

> The way I'd envision support for that is to add a user_dfs_regions and
> a respective cmd which userspace can set, if the kernel supports
> it userspace can ifdef it (with the define trick on nl80211.h) and
> then send this over for the supported DFS regions for new IBSS userspace.
> We'd then get an enhancement moving forward, otherwise older kernels
> would require userspace to handle DFS region mismatches / updates /
> quiescing itself.

See above. I see no reason for userspace to ever be really concerned
about the DFS region.

> > > + * @NL80211_ATTR_DFS_REGIONS: bitmask of all supported, tested, and
> > > + * 	certified &enum nl80211_dfs_regions that a wiphy has been declared to
> > > + *	support and that agrees with what is programmed currently on cfg80211.
> > > + *	This is used for modes of operation that require DFS support on the
> > > + *	driver. If %NL80211_ATTR_HANDLE_DFS is set userspace need not check
> > > + *	for this for IBSS as it will support DFS in userspace for IBSS.

> NL80211_ATTR_HANDLE_DFS seems to be only for IBSS userspace, I'm trying
> to provide clarification that this is not required for
> NL80211_ATTR_HANDLE_DFS.

Still doesn't make sense - HANDLE_DFS means "I'll react to radar
detection events and will do the CSA". It has absolutely nothing to do
with DFS regions. Please keep those things separate.

> > > +#define NL80211_ATTR_DFS_REGIONS NL80211_ATTR_DFS_REGIONS
> > 
> > Not needed.
> 
> Why is that?

Because nobody will do an #ifdef on it anyway

johannes

Re: [RFC 0/5] cfg80211: regulatory quiescing and exporting DFS regions

[Luis R. Rodriguez]

On Wed, Nov 13, 2013 at 08:13:44PM +0100, Johannes Berg wrote:
> On Wed, 2013-11-13 at 19:57 +0100, Luis R. Rodriguez wrote:
> > On Wed, Nov 13, 2013 at 7:48 PM, Johannes Berg
> > <johannes@sipsolutions.net> wrote:
> > > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > >
> > >> Janusz, please test, and likely feel free to take on the last patch.
> > >> We may need help from Johannes on the rtnl lock question now required
> > >> for checks on enabling beacon operation.
> > >
> > > What's that rtnl thing about?
> > 
> > Whether or not callers of cfg80211_reg_can_beacon() have it properly
> > held already and if not best strategy to make this simple and concise.
> > The rtnl is required as we need to now query the cfg80211 regdom dfs
> > region for the case where a channel requires DFS.
> 
> Can't you just use RCU for that part?

If Janusz is right then we don't even need that patch at all :D
The rtnl then would only be used for sending the wiphy, we'd
just require hostapd to always check for the DFS region prior to
starting IR for the first time, if the netdev was brought down
(quisced), we'd have to have it check again the DFS region.

This however does assume we can quiesce properly upon regulatory
domain changes.

  Luis

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Johannes Berg]

On Thu, 2013-11-14 at 07:12 -0800, Luis R. Rodriguez wrote:

> > But that's kinda unsafe. Might also be worth just making it a
> > void* to avoid people trying to use it.
> 
> OK. I can see the cookie in theory race against the allocator
> releasing and creating a new wdev and it magically being
> the same pointer, but chances are really low of that.

Hmm, good point, maybe it'd be better to clean it up on netdev
unregistration.

johannes

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 03:02:54PM +0100, Johannes Berg wrote:
> On Thu, 2013-11-14 at 06:05 -0800, Luis R. Rodriguez wrote:
> 
> > > > -void regulatory_hint_country_ie(struct wiphy *wiphy, enum ieee80211_band band,
> > > > +void regulatory_hint_country_ie(struct wireless_dev *wdev,
> > > > +				enum ieee80211_band band,
> > > >  				const u8 *country_ie, u8 country_ie_len)
> > > ...
> > > > +       request->wdev = wdev;
> > > > 
> > > You have absolutely no validation of this pointer - the lifetime of the
> > > request object and the wdev aren't necessarily the same.
> > 
> > Agreed.
> > 
> > > At least you should very carefully document that this pointer is a
> > > cookie (if it really is) and must never be dereferenced.
> > 
> > OK, I could also validate it upon processing but we'd need to loop
> > over the rdev wdev list for the country IE hint, if that is acceptable
> > upon procesing it'd be valid and we'd avoid corner case issues.
> > Thoughts?
> 
> As far as I can tell you already don't use it in any other way but a
> cookie pointer, comparing while iterating the then-current list of
> wdevs.

I confirm.

> But that's kinda unsafe. Might also be worth just making it a
> void* to avoid people trying to use it.

OK. I can see the cookie in theory race against the allocator
releasing and creating a new wdev and it magically being
the same pointer, but chances are really low of that.

 Luis

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 03:44:46PM +0100, Johannes Berg wrote:
> On Thu, 2013-11-14 at 06:11 -0800, Luis R. Rodriguez wrote:
> > On Wed, Nov 13, 2013 at 10:22:18PM +0100, Johannes Berg wrote:
> > > On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
> > > > This quiesces devices when appropriate to ensure that
> > > > regulatory domain updates take effect and avoid having
> > > > devices initiate radiation when they should not.
> > > 
> > > I'm not really sure this makes sense.
> > > 
> > > If we're staying connected, how can we be moving far enough to go
> > > through regulatory domains to have totally different rules?
> > 
> > Don't think of what makes sense, think of the corner cases that
> > could happen here, such as plugging in a card that disagrees
> > with regulatory settings, and creates a conflict on DFS regions.
> > One example might be someone plugging in a USB 802.11 card programmed
> > for JP in say a FR 802.11 AP, assuming the AP had the USB 802.11
> > driver. Another example may be if a user provides an input with
> > say 'iw reg set JP' on a FR AP. In such cases we want to stop IR,
> > even if the user was dumb, we'd be respecting the regulatory settings,
> > as silly as they may be.
> 
> Think of the other way around, such as plugging in a stupid USB NIC just
> to see if it works - and suddenly finding your 5 GHz connection broken
> because that USB NIC said it really needed some stupid country with 2.4
> only. I'm not convinced.

Actually your example is a good one that I'd use to make my case, but I
can also see this being like a sledge hammer. What if we only quiesce on
wdevs of types:

  o NL80211_IFTYPE_ADHOC
  o NL80211_IFTYPE_MESH_POINT
  o NL80211_IFTYPE_AP
  o NL80211_IFTYPE_P2P_GO

  Luis

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 04:05:23PM +0100, Johannes Berg wrote:
> On Thu, 2013-11-14 at 07:12 -0800, Luis R. Rodriguez wrote:
> 
> > > But that's kinda unsafe. Might also be worth just making it a
> > > void* to avoid people trying to use it.
> > 
> > OK. I can see the cookie in theory race against the allocator
> > releasing and creating a new wdev and it magically being
> > the same pointer, but chances are really low of that.
> 
> Hmm, good point, maybe it'd be better to clean it up on netdev
> unregistration.

Ah, the block notifier?

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Johannes Berg]

On Thu, 2013-11-14 at 07:33 -0800, Luis R. Rodriguez wrote:
> > Still doesn't make sense - HANDLE_DFS means "I'll react to radar
> > detection events and will do the CSA". It has absolutely nothing to do
> > with DFS regions. Please keep those things separate.
> 
> How you deal with radar detection is completely dependent on the DFS
> region, no?

You're missing *my* point :-)

Radar *detection* is region-dependent.

What happens *after* that (announce a channel switch, etc.) isn't at
all.

johannes

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 03:48:39PM +0100, Johannes Berg wrote:
> On Thu, 2013-11-14 at 06:31 -0800, Luis R. Rodriguez wrote:
> 
> > > What's the point of this function rather than the driver setting the
> > > region bits before registration?
> > 
> > I don't trust driver developers, this provides a simple interface
> > which pushes driver developers to use CONFIG_CFG80211_CERTIFICATION_ONUS
> > for driver DFS features, it also validates the input and also does
> > the conversion to bitmasks for the driver developers. Without this
> > we'd assume driver developers would enable DFS under proper regulatory
> > configs but also require BIT() | BIT() settings.
> 
> It has nothing to do with trust. If the driver author sets
> 
> 	dfs_capability = BIT(FCC) | BIT(ETSI)
> 
> you still don't have to use the values if CERTIFICATION_ONUS isn't set.
> Therefore, I'm not buying it.

True.

> > > > + *	IBSS network. Userspace is also required to verify that the currently
> > > > + *	programmed DFS region is supported by userspace and monitor it in case
> > > > + *	of changes.
> > > 
> > > That seems like wishful thinking since userspace already exists for
> > > this.
> > 
> > It is, but its also a heads up as to perhaps what we should do to help
> > userspace, I think I mentioned elsewhere we could require userspace DFS
> > support to supply the supported DFS regions and then we'd deal with the
> > conflicts.
> 
> I really don't get why userspace should have to deal with DFS regions.
> You know what region you're in, you know which the hardware supports -
> that's all userspace should care about. We can even make up the "is DFS
> supported" flag on-the-fly based on all this information.
> 
> Pushing any of this to userspace just seems really stupid.

You're missing my point that we are not sure if IBSS DFS userspace
supports DFS region changes as a cornercase, and I'm arguing userspace
should not have to deal with that and that I wish for us to deal with
that for userspace. Thinking about it though, if we quiesce for IBSS
upon regulatory change we'd cover this corner case.

> > The way I'd envision support for that is to add a user_dfs_regions and
> > a respective cmd which userspace can set, if the kernel supports
> > it userspace can ifdef it (with the define trick on nl80211.h) and
> > then send this over for the supported DFS regions for new IBSS userspace.
> > We'd then get an enhancement moving forward, otherwise older kernels
> > would require userspace to handle DFS region mismatches / updates /
> > quiescing itself.
> 
> See above. I see no reason for userspace to ever be really concerned
> about the DFS region.

Agreed, I was trying to deal with the corner case of quiescing upon
regulatory change for a DFS region change.

> > > > + * @NL80211_ATTR_DFS_REGIONS: bitmask of all supported, tested, and
> > > > + * 	certified &enum nl80211_dfs_regions that a wiphy has been declared to
> > > > + *	support and that agrees with what is programmed currently on cfg80211.
> > > > + *	This is used for modes of operation that require DFS support on the
> > > > + *	driver. If %NL80211_ATTR_HANDLE_DFS is set userspace need not check
> > > > + *	for this for IBSS as it will support DFS in userspace for IBSS.
> 
> > NL80211_ATTR_HANDLE_DFS seems to be only for IBSS userspace, I'm trying
> > to provide clarification that this is not required for
> > NL80211_ATTR_HANDLE_DFS.
> 
> Still doesn't make sense - HANDLE_DFS means "I'll react to radar
> detection events and will do the CSA". It has absolutely nothing to do
> with DFS regions. Please keep those things separate.

How you deal with radar detection is completely dependent on the DFS
region, no?

> > > > +#define NL80211_ATTR_DFS_REGIONS NL80211_ATTR_DFS_REGIONS
> > > 
> > > Not needed.
> > 
> > Why is that?
> 
> Because nobody will do an #ifdef on it anyway

OK

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 4:29 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-11-14 at 07:33 -0800, Luis R. Rodriguez wrote:
>> > Still doesn't make sense - HANDLE_DFS means "I'll react to radar
>> > detection events and will do the CSA". It has absolutely nothing to do
>> > with DFS regions. Please keep those things separate.
>>
>> How you deal with radar detection is completely dependent on the DFS
>> region, no?
>
> You're missing *my* point :-)
>
> Radar *detection* is region-dependent.
>
> What happens *after* that (announce a channel switch, etc.) isn't at
> all.

Sorry, I misread the documentation then, so HANDLE_DFS *does* not mean
userspace will implement the radar detection in userspace?

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Simon Wunderlich]

> On Thu, Nov 14, 2013 at 4:29 PM, Johannes Berg
> 
> <johannes@sipsolutions.net> wrote:
> > On Thu, 2013-11-14 at 07:33 -0800, Luis R. Rodriguez wrote:
> >> > Still doesn't make sense - HANDLE_DFS means "I'll react to radar
> >> > detection events and will do the CSA". It has absolutely nothing to do
> >> > with DFS regions. Please keep those things separate.
> >> 
> >> How you deal with radar detection is completely dependent on the DFS
> >> region, no?
> > 
> > You're missing *my* point :-)
> > 
> > Radar *detection* is region-dependent.
> > 
> > What happens *after* that (announce a channel switch, etc.) isn't at
> > all.
> 
> Sorry, I misread the documentation then, so HANDLE_DFS *does* not mean
> userspace will implement the radar detection in userspace?
> 

Right, it does not mean userspace implements radar detection/pattern 
matching/etc.

HANDLE_DFS only means that userspace lets the kernel know that it will react 
to nl80211-dfs-radar events (these only contain "i detected a radar on freq 
XXX" [1] and similar) and that it will do a channel switch (CSA) in this case 
to move away.

We still assume that the radar detection/pattern matching/etc, which is indeed 
region-specific, is done in the drivers (as done for ath9k now) or by other 
means independent of this HANDLE_DFS attribute.

Cheers,
    Simon

[1] http://lxr.free-electrons.com/source/include/uapi/linux/nl80211.h#L3836

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Janusz Dziedzic]

On 14 November 2013 15:46, Luis R. Rodriguez <mcgrof@do-not-panic.com> wrote:
> On Thu, Nov 14, 2013 at 12:52:26PM +0100, Janusz Dziedzic wrote:
>> On 14 November 2013 11:15, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote:
>> > On 13 November 2013 22:29, Johannes Berg <johannes@sipsolutions.net> wrote:
>> >> On Wed, 2013-11-13 at 19:12 +0100, Luis R. Rodriguez wrote:
>> >>> Check the DFS region before channel availability check
>> >>> or declaring a channel as DFS usable.
>> >>>
>> >>> Signed-off-by: Luis R. Rodriguez <mcgrof@do-not-panic.com>
>> >>> ---
>> >>>  net/wireless/chan.c    | 8 ++++++++
>> >>>  net/wireless/nl80211.c | 5 +++++
>> >>>  2 files changed, 13 insertions(+)
>> >>>
>> >>> diff --git a/net/wireless/chan.c b/net/wireless/chan.c
>> >>> index 78559b5..4e6eaa0 100644
>> >>> --- a/net/wireless/chan.c
>> >>> +++ b/net/wireless/chan.c
>> >>> @@ -517,10 +517,18 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
>> >>>       struct ieee80211_sta_ht_cap *ht_cap;
>> >>>       struct ieee80211_sta_vht_cap *vht_cap;
>> >>>       u32 width, control_freq;
>> >>> +     enum nl80211_dfs_regions dfs_region;
>> >>>
>> >>>       if (WARN_ON(!cfg80211_chandef_valid(chandef)))
>> >>>               return false;
>> >>>
>> >>> +     rtnl_lock();
>> >>> +     dfs_region = reg_get_dfs_region(wiphy);
>> >>> +     rtnl_unlock();
>> >>
>> >
>> >
>> > Do we need check dfs_region in cfg80211_can_beacon() at all?
>> > We already check first if all channels NL80211_DFS_AVAILABLE.
>> > To be DFS_AVAILABLE we need pass CAC, and we will fail CAC if
>> > dfs_region == UNSET.
>> >
>> > Anyway we can do something like this in cfg80211_can_beacon()
>> >
>> >         if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
>> > -           cfg80211_chandef_dfs_available(wiphy, chandef)) {
>> > +           cfg80211_chandef_dfs_available(wiphy, chandef) &&
>> > +           reg_get_dfs_region(wiphy) != NL80211_DFS_UNSET) {
>> > ...
>> > And change doc that cfg80211_can_beacon() require rtnl_lock.
>> > But I think this is not required.
>> >
>> Or if we should handle dfs_region change after we start CAC and
>> start beaconing,
>
> Can you clarify what you mean?
>
> Right now update_all_wiphy_regulatory() contends on the rtnl_lock(),
> I am not sure if starting CAC and beaconing does. I also just did
> not get what you mean here.
>
CAC is only a request - so as I understand correctly rtnl_lock() is
not held whole CAC time (1minute, 10 minutes)
So we could have something like this

1) request CAC
2) mac80211 will call driver radar detection (ETSI) and will start cac
timer (60s)
3) after 40 seconds we will change regdom and dfs_region (eg. from
ETSI -> FCC) - tested using iw reg set US

Currently we will not cancel CAC and will mark channels as
DFS_AVAILABLE after 1 minute.
So, probably we will need cancel_radar_detection() callback that will
cancel mac80211 timer here when cac_started and regulatory_update()
and different dfs_regions.

4) CAC end and we set DFS_AVAILABLE state
5) we start beaconning (even we do radar_detection for FCC only for
about 20 seconds)

This is probably rare case when we will change dfs_region between
start_cac() and can_beacon() - so I am not sure we have to handle
this?

BR
Janusz


>> we could add __cfg80211_can_beacon() without rtnl
>> locking and cfg80211_can_beacon() with locking.
>
> If to use DFS we need DFS_AVAILABLE and if it is *never* set
> if dfs region is unset I think we should be good *if* we handle
> quiescing properly.
>
>> BTW
>> Luis should we handle dfs_region change also during CAC? Who should
>> fail CAC in case we switch eg. from ETSI to FCC? Should we handle this
>> in driver?
>
> I gave this a lot of thought and decided to just do it in kernel
> on cfg80211 by quescing upon regulatory domain changes where
> appropriately (not just for DFS region change), *and* by having
> wiphy_core_dfs_region_usable() always be checked before sending
> to userspace the supported DFS regions. The trick here is that
> since we are quiescing it means that userspace in theory (if
> we do quescing properly) will always have to check query for the
> DFS regions *anytime* it will want to start a new interface for
> initiating radiation, ie just once upon init. If the DFS region
> changes quescing would have turned off radiation and what I was
> aiming for is for then userspace to have to get the DFS region
> again and only if its set would it then again start radiation.
>
> Doing it this way avoid us having to do anything on any driver.
> It also allows us to not even have to deal with this in userspace,
> which would have required userspace to provide support for handling
> DFS region changes by monitoring the multicast regulatory group,
> and upon a regulatory change, if a DFS region change, or any
> regulatory change queisce itself.
>
> Notice that upon a regulatory intersection we end up using DFS
> unset if the DFS regions disagree, and that we now always use
> the central cfg80211 DFS region. This ensures that DFS is tied
> down to regulatory agreement on the system.
>
>   Luis

Re: [RFC 5/5] cfg80211: DFS check dfs_region before usage

[Luis R. Rodriguez]

On Fri, Nov 15, 2013 at 10:37 AM, Janusz Dziedzic
<janusz.dziedzic@tieto.com> wrote:
> 1) request CAC
> 2) mac80211 will call driver radar detection (ETSI) and will start cac
> timer (60s)
> 3) after 40 seconds we will change regdom and dfs_region (eg. from
> ETSI -> FCC) - tested using iw reg set US
>
> Currently we will not cancel CAC and will mark channels as
> DFS_AVAILABLE after 1 minute.
> So, probably we will need cancel_radar_detection() callback that will
> cancel mac80211 timer here when cac_started and regulatory_update()
> and different dfs_regions.
>
> 4) CAC end and we set DFS_AVAILABLE state
> 5) we start beaconning (even we do radar_detection for FCC only for
> about 20 seconds)
>
> This is probably rare case when we will change dfs_region between
> start_cac() and can_beacon() - so I am not sure we have to handle
> this?

Ah great point. I had not considered this. The regulatory code I added
to quiesce would take care of this, it'd force wdev's to tear down any
active sessions they have. That is provided I convince Johannes this
is indeed necessary.

  Luis

Re: [RFC 4/5] cfg80211: add DFS region capability support

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 5:13 PM, Simon Wunderlich <sw@simonwunderlich.de> wrote:
> Right, it does not mean userspace implements radar detection/pattern
> matching/etc.
>
> HANDLE_DFS only means that userspace lets the kernel know that it will react
> to nl80211-dfs-radar events (these only contain "i detected a radar on freq
> XXX" [1] and similar) and that it will do a channel switch (CSA) in this case
> to move away.
>
> We still assume that the radar detection/pattern matching/etc, which is indeed
> region-specific, is done in the drivers (as done for ath9k now) or by other
> means independent of this HANDLE_DFS attribute.

OK thanks, I think I read too much into "userspace controls DFS
operation in IBSS mode". Some clarification on this would be useful on
the documentation.

In that case, yes everything would be handled properly now with the
series I posted and all that nonsense I mentioned about HANDLE_DFS can
be ignored.

  Luis

Re: [RFC 1/5] cfg80211: pass the wdev on the country IE regulatory hint

[Luis R. Rodriguez]

On Thu, Nov 14, 2013 at 4:21 PM, Luis R. Rodriguez
<mcgrof@do-not-panic.com> wrote:
> On Thu, Nov 14, 2013 at 04:05:23PM +0100, Johannes Berg wrote:
>> On Thu, 2013-11-14 at 07:12 -0800, Luis R. Rodriguez wrote:
>>
>> > > But that's kinda unsafe. Might also be worth just making it a
>> > > void* to avoid people trying to use it.
>> >
>> > OK. I can see the cookie in theory race against the allocator
>> > releasing and creating a new wdev and it magically being
>> > the same pointer, but chances are really low of that.
>>
>> Hmm, good point, maybe it'd be better to clean it up on netdev
>> unregistration.
>
> Ah, the block notifier?

Actually that would be good to remove stale unprocessed reguatory
hints too :D gonna go for that.

  Luis

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Johannes Berg]

On Thu, 2013-11-14 at 07:18 -0800, Luis R. Rodriguez wrote:

> > Think of the other way around, such as plugging in a stupid USB NIC just
> > to see if it works - and suddenly finding your 5 GHz connection broken
> > because that USB NIC said it really needed some stupid country with 2.4
> > only. I'm not convinced.
> 
> Actually your example is a good one that I'd use to make my case, 

But what's your case? Why trust a just-plugged-in USB NIC more than an
already-present PCIe NIC?

> but I
> can also see this being like a sledge hammer. What if we only quiesce on
> wdevs of types:
> 
>   o NL80211_IFTYPE_ADHOC
>   o NL80211_IFTYPE_MESH_POINT
>   o NL80211_IFTYPE_AP
>   o NL80211_IFTYPE_P2P_GO

At least we should then also have that patch that QCA has somewhere
about reporting to userspace that AP was stopped :P

johannes

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Johannes Berg]

On Thu, 2013-11-14 at 07:18 -0800, Luis R. Rodriguez wrote:

> > Think of the other way around, such as plugging in a stupid USB NIC just
> > to see if it works - and suddenly finding your 5 GHz connection broken
> > because that USB NIC said it really needed some stupid country with 2.4
> > only. I'm not convinced.
> 
> Actually your example is a good one that I'd use to make my case, 

But what's your case? Why trust a just-plugged-in USB NIC more than an
already-present PCIe NIC?

> but I
> can also see this being like a sledge hammer. What if we only quiesce on
> wdevs of types:
> 
>   o NL80211_IFTYPE_ADHOC
>   o NL80211_IFTYPE_MESH_POINT
>   o NL80211_IFTYPE_AP
>   o NL80211_IFTYPE_P2P_GO

At least we should then also have that patch that QCA has somewhere
about reporting to userspace that AP was stopped :P

johannes

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Luis R. Rodriguez]

On Mon, Nov 25, 2013 at 7:59 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> At least we should then also have that patch that QCA has somewhere
> about reporting to userspace that AP was stopped :P

I don't have time to resubmit these, so someone else can pick them up,
I'm happy to review though.

  Luis

Re: [RFC 3/5] cfg80211: add regulatory quiescing support

[Johannes Berg]

On Fri, 2014-01-24 at 15:35 -0800, Luis R. Rodriguez wrote:
> On Mon, Nov 25, 2013 at 7:59 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > At least we should then also have that patch that QCA has somewhere
> > about reporting to userspace that AP was stopped :P

FWIW, I did the above today.

johannes