[PATCH v10 0/5] cfg80211/mac80211: move interface combinations check to mac80211

[PATCH v10 0/5] cfg80211/mac80211: move interface combinations check to mac80211

[Luciano Coelho]

Hi,

The history is getting too big, so drop it... *sigh*

Adressed some comment by Eliad (thanks!)

In v10:

   * ignore software iftypes when checking used_iftypes;
   * ignore NL80211_IFTYPE_UNSPECIFIED instead of WARNing in
     cfg80211_dfs_required();

Luca.

Luciano Coelho (5):
  cfg80211: refactor cfg80211_can_use_iftype_chan()
  cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()
  cfg80211/mac80211: move interface counting for combination check to
    mac80211
  cfg80211/mac80211: move combination check to mac80211 for ibss
  cfg80211/mac80211: move more combination checks to mac80211

 Documentation/DocBook/80211.tmpl |   1 +
 include/net/cfg80211.h           |  31 +++++++--
 net/mac80211/cfg.c               |  11 +++-
 net/mac80211/chan.c              |  17 +++++
 net/mac80211/ibss.c              |  64 ++++++++++++------
 net/mac80211/ieee80211_i.h       |   4 ++
 net/mac80211/iface.c             |   6 +-
 net/mac80211/mesh.c              |   9 +--
 net/mac80211/util.c              |  76 +++++++++++++++++++++
 net/wireless/chan.c              |  74 ++++++++++++++++-----
 net/wireless/core.c              |  11 +---
 net/wireless/core.h              |  29 --------
 net/wireless/ibss.c              |  24 -------
 net/wireless/mesh.c              |  25 -------
 net/wireless/mlme.c              |  14 +---
 net/wireless/nl80211.c           |  59 +++++------------
 net/wireless/util.c              | 139 ++++++++++++++++++++++-----------------
 17 files changed, 344 insertions(+), 250 deletions(-)

-- 
1.9.0

[PATCH v10 1/5] cfg80211: refactor cfg80211_can_use_iftype_chan()

[Luciano Coelho]

Separate the code that counts the interface types and channels from
the code that check the interface combinations.  The new function that
checks for combinations is exported so it can be called by the
drivers.

This is done in preparation for moving the interface combinations
checks out of cfg80211.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v4:

   * rebased on top of slightly modified applied patches
   * removed the parenthesis from the comment (not needed for docbook)
   * use num[NUM_NL80211_IFTYPES] in cfg80211_check_combinations()

In v7:

   * removed redundant arguments (num_interfaces and used_iftypes)
     from cfg80211_check_combinations();

In v10:

   * ignore sw_interfaces when counting used_types
---
 Documentation/DocBook/80211.tmpl |   1 +
 include/net/cfg80211.h           |  22 +++++++
 net/wireless/util.c              | 129 ++++++++++++++++++++++-----------------
 3 files changed, 96 insertions(+), 56 deletions(-)

diff --git a/Documentation/DocBook/80211.tmpl b/Documentation/DocBook/80211.tmpl
index 044b764..d9b9416 100644
--- a/Documentation/DocBook/80211.tmpl
+++ b/Documentation/DocBook/80211.tmpl
@@ -100,6 +100,7 @@
 !Finclude/net/cfg80211.h wdev_priv
 !Finclude/net/cfg80211.h ieee80211_iface_limit
 !Finclude/net/cfg80211.h ieee80211_iface_combination
+!Finclude/net/cfg80211.h cfg80211_check_combinations
       </chapter>
       <chapter>
       <title>Actions and configuration</title>
diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index ff3af16..768fcc0 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -4682,6 +4682,28 @@ void cfg80211_crit_proto_stopped(struct wireless_dev *wdev, gfp_t gfp);
  */
 unsigned int ieee80211_get_num_supported_channels(struct wiphy *wiphy);
 
+/**
+ * cfg80211_check_combinations - check interface combinations
+ *
+ * @wiphy: the wiphy
+ * @num_different_channels: the number of different channels we want
+ *	to use for verification
+ * @radar_detect: a bitmap where each bit corresponds to a channel
+ *	width where radar detection is needed, as in the definition of
+ *	&struct ieee80211_iface_combination.@radar_detect_widths
+ * @iftype_num: array with the numbers of interfaces of each interface
+ *	type.  The index is the interface type as specified in &enum
+ *	nl80211_iftype.
+ *
+ * This function can be called by the driver to check whether a
+ * combination of interfaces and their types are allowed according to
+ * the interface combinations.
+ */
+int cfg80211_check_combinations(struct wiphy *wiphy,
+				const int num_different_channels,
+				const u8 radar_detect,
+				const int iftype_num[NUM_NL80211_IFTYPES]);
+
 /* Logging, debugging and troubleshooting/diagnostic helpers. */
 
 /* wiphy_printk helpers, similar to dev_printk */
diff --git a/net/wireless/util.c b/net/wireless/util.c
index dadc934..a24448b 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1253,6 +1253,76 @@ int cfg80211_validate_beacon_int(struct cfg80211_registered_device *rdev,
 	return res;
 }
 
+int cfg80211_check_combinations(struct wiphy *wiphy,
+				const int num_different_channels,
+				const u8 radar_detect,
+				const int iftype_num[NUM_NL80211_IFTYPES])
+{
+	int i, j, iftype;
+	int num_interfaces = 0;
+	u32 used_iftypes = 0;
+
+	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
+		num_interfaces += iftype_num[iftype];
+		if (iftype_num[iftype] > 0 &&
+		    !(wiphy->software_iftypes & BIT(iftype)))
+			used_iftypes |= BIT(iftype);
+	}
+
+	for (i = 0; i < wiphy->n_iface_combinations; i++) {
+		const struct ieee80211_iface_combination *c;
+		struct ieee80211_iface_limit *limits;
+		u32 all_iftypes = 0;
+
+		c = &wiphy->iface_combinations[i];
+
+		if (num_interfaces > c->max_interfaces)
+			continue;
+		if (num_different_channels > c->num_different_channels)
+			continue;
+
+		limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
+				 GFP_KERNEL);
+		if (!limits)
+			return -ENOMEM;
+
+		for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
+			if (wiphy->software_iftypes & BIT(iftype))
+				continue;
+			for (j = 0; j < c->n_limits; j++) {
+				all_iftypes |= limits[j].types;
+				if (!(limits[j].types & BIT(iftype)))
+					continue;
+				if (limits[j].max < iftype_num[iftype])
+					goto cont;
+				limits[j].max -= iftype_num[iftype];
+			}
+		}
+
+		if (radar_detect && !(c->radar_detect_widths & radar_detect))
+			goto cont;
+
+		/* Finally check that all iftypes that we're currently
+		 * using are actually part of this combination. If they
+		 * aren't then we can't use this combination and have
+		 * to continue to the next.
+		 */
+		if ((all_iftypes & used_iftypes) != used_iftypes)
+			goto cont;
+
+		/* This combination covered all interface types and
+		 * supported the requested numbers, so we're good.
+		 */
+		kfree(limits);
+		return 0;
+ cont:
+		kfree(limits);
+	}
+
+	return -EBUSY;
+}
+EXPORT_SYMBOL(cfg80211_check_combinations);
+
 int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
 				 struct wireless_dev *wdev,
 				 enum nl80211_iftype iftype,
@@ -1261,7 +1331,6 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
 				 u8 radar_detect)
 {
 	struct wireless_dev *wdev_iter;
-	u32 used_iftypes = BIT(iftype);
 	int num[NUM_NL80211_IFTYPES];
 	struct ieee80211_channel
 			*used_channels[CFG80211_MAX_NUM_DIFFERENT_CHANNELS];
@@ -1269,7 +1338,7 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
 	enum cfg80211_chan_mode chmode;
 	int num_different_channels = 0;
 	int total = 1;
-	int i, j;
+	int i;
 
 	ASSERT_RTNL();
 
@@ -1354,65 +1423,13 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
 
 		num[wdev_iter->iftype]++;
 		total++;
-		used_iftypes |= BIT(wdev_iter->iftype);
 	}
 
 	if (total == 1 && !radar_detect)
 		return 0;
 
-	for (i = 0; i < rdev->wiphy.n_iface_combinations; i++) {
-		const struct ieee80211_iface_combination *c;
-		struct ieee80211_iface_limit *limits;
-		u32 all_iftypes = 0;
-
-		c = &rdev->wiphy.iface_combinations[i];
-
-		if (total > c->max_interfaces)
-			continue;
-		if (num_different_channels > c->num_different_channels)
-			continue;
-
-		limits = kmemdup(c->limits, sizeof(limits[0]) * c->n_limits,
-				 GFP_KERNEL);
-		if (!limits)
-			return -ENOMEM;
-
-		for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
-			if (rdev->wiphy.software_iftypes & BIT(iftype))
-				continue;
-			for (j = 0; j < c->n_limits; j++) {
-				all_iftypes |= limits[j].types;
-				if (!(limits[j].types & BIT(iftype)))
-					continue;
-				if (limits[j].max < num[iftype])
-					goto cont;
-				limits[j].max -= num[iftype];
-			}
-		}
-
-		if (radar_detect && !(c->radar_detect_widths & radar_detect))
-			goto cont;
-
-		/*
-		 * Finally check that all iftypes that we're currently
-		 * using are actually part of this combination. If they
-		 * aren't then we can't use this combination and have
-		 * to continue to the next.
-		 */
-		if ((all_iftypes & used_iftypes) != used_iftypes)
-			goto cont;
-
-		/*
-		 * This combination covered all interface types and
-		 * supported the requested numbers, so we're good.
-		 */
-		kfree(limits);
-		return 0;
- cont:
-		kfree(limits);
-	}
-
-	return -EBUSY;
+	return cfg80211_check_combinations(&rdev->wiphy, num_different_channels,
+					   radar_detect, num);
 }
 
 int ieee80211_get_ratemask(struct ieee80211_supported_band *sband,
-- 
1.9.0

[PATCH v10 2/5] cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()

[Luciano Coelho]

Some interface types don't require DFS (such as STATION, P2P_CLIENT
etc).  In order to centralize these decisions, make
cfg80211_chandef_dfs_required() take the iftype into consideration.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v4:

   * rebased on top of slightly modified applied patches
   * removed the double negations when assigning err to a boolean;

In v7:

   * change cfg80211_chandef_dfs_required() back to returning 1 if
     radar detection is needed instead of a bitmap with the width
     required.

In v9:

   * fixed commit message;
   * added braces in a couple of if's that had comments and a
     statement;
   * move NL80211_IFTYPE_UNSPECIFIED to the WARN part of the switch in
     dfs_required) and remove default;

In v10:

   * move NL80211_IFTYPE_UNSPECIFIED up again, so we don't warn, just
     ignore it.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 include/net/cfg80211.h |  7 +++--
 net/mac80211/ibss.c    | 32 +++++++++++-----------
 net/mac80211/mesh.c    |  9 +++---
 net/wireless/chan.c    | 74 ++++++++++++++++++++++++++++++++++++++------------
 net/wireless/mesh.c    |  7 +++--
 net/wireless/nl80211.c | 37 ++++++++++++-------------
 6 files changed, 104 insertions(+), 62 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 768fcc0..51f278a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -441,10 +441,13 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
  * cfg80211_chandef_dfs_required - checks if radar detection is required
  * @wiphy: the wiphy to validate against
  * @chandef: the channel definition to check
- * Return: 1 if radar detection is required, 0 if it is not, < 0 on error
+ * @iftype: the interface type as specified in &enum nl80211_iftype
+ * Returns:
+ *	1 if radar detection is required, 0 if it is not, < 0 on error
  */
 int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
-				  const struct cfg80211_chan_def *chandef);
+				  const struct cfg80211_chan_def *chandef,
+				  enum nl80211_iftype);
 
 /**
  * ieee80211_chandef_rate_flags - returns rate flags for a channel
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index e458ca0..12057fd 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -228,7 +228,7 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	struct beacon_data *presp;
 	enum nl80211_bss_scan_width scan_width;
 	bool have_higher_than_11mbit;
-	bool radar_required = false;
+	bool radar_required;
 	int err;
 
 	sdata_assert_lock(sdata);
@@ -282,21 +282,20 @@ static void __ieee80211_sta_join_ibss(struct ieee80211_sub_if_data *sdata,
 	}
 
 	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &chandef);
+					    &chandef, NL80211_IFTYPE_ADHOC);
 	if (err < 0) {
 		sdata_info(sdata,
 			   "Failed to join IBSS, invalid chandef\n");
 		return;
 	}
-	if (err > 0) {
-		if (!ifibss->userspace_handles_dfs) {
-			sdata_info(sdata,
-				   "Failed to join IBSS, DFS channel without control program\n");
-			return;
-		}
-		radar_required = true;
+	if (err > 0 && !ifibss->userspace_handles_dfs) {
+		sdata_info(sdata,
+			   "Failed to join IBSS, DFS channel without control program\n");
+		return;
 	}
 
+	radar_required = err;
+
 	mutex_lock(&local->mtx);
 	if (ieee80211_vif_use_channel(sdata, &chandef,
 				      ifibss->fixed_channel ?
@@ -775,7 +774,8 @@ static void ieee80211_ibss_csa_mark_radar(struct ieee80211_sub_if_data *sdata)
 	 * unavailable.
 	 */
 	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &ifibss->chandef);
+					    &ifibss->chandef,
+					    NL80211_IFTYPE_ADHOC);
 	if (err > 0)
 		cfg80211_radar_event(sdata->local->hw.wiphy, &ifibss->chandef,
 				     GFP_ATOMIC);
@@ -873,17 +873,17 @@ ieee80211_ibss_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	}
 
 	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &params.chandef);
+					    &params.chandef,
+					    NL80211_IFTYPE_ADHOC);
 	if (err < 0)
 		goto disconnect;
-	if (err) {
+	if (err > 0 && !ifibss->userspace_handles_dfs) {
 		/* IBSS-DFS only allowed with a control program */
-		if (!ifibss->userspace_handles_dfs)
-			goto disconnect;
-
-		params.radar_required = true;
+		goto disconnect;
 	}
 
+	params.radar_required = err;
+
 	if (cfg80211_chandef_identical(&params.chandef,
 				       &sdata->vif.bss_conf.chandef)) {
 		ibss_dbg(sdata,
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index f70e9cd..ea0b628 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -903,14 +903,15 @@ ieee80211_mesh_process_chnswitch(struct ieee80211_sub_if_data *sdata,
 	}
 
 	err = cfg80211_chandef_dfs_required(sdata->local->hw.wiphy,
-					    &params.chandef);
+					    &params.chandef,
+					    NL80211_IFTYPE_MESH_POINT);
 	if (err < 0)
 		return false;
-	if (err) {
-		params.radar_required = true;
+	if (err > 0)
 		/* TODO: DFS not (yet) supported */
 		return false;
-	}
+
+	params.radar_required = err;
 
 	if (cfg80211_chandef_identical(&params.chandef,
 				       &sdata->vif.bss_conf.chandef)) {
diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 8659d5c..07bb239 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -326,28 +326,57 @@ static int cfg80211_get_chans_dfs_required(struct wiphy *wiphy,
 
 
 int cfg80211_chandef_dfs_required(struct wiphy *wiphy,
-				  const struct cfg80211_chan_def *chandef)
+				  const struct cfg80211_chan_def *chandef,
+				  enum nl80211_iftype iftype)
 {
 	int width;
-	int r;
+	int ret;
 
 	if (WARN_ON(!cfg80211_chandef_valid(chandef)))
 		return -EINVAL;
 
-	width = cfg80211_chandef_get_width(chandef);
-	if (width < 0)
-		return -EINVAL;
+	switch (iftype) {
+	case NL80211_IFTYPE_ADHOC:
+	case NL80211_IFTYPE_AP:
+	case NL80211_IFTYPE_P2P_GO:
+	case NL80211_IFTYPE_MESH_POINT:
+		width = cfg80211_chandef_get_width(chandef);
+		if (width < 0)
+			return -EINVAL;
 
-	r = cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq1,
-					    width);
-	if (r)
-		return r;
+		ret = cfg80211_get_chans_dfs_required(wiphy,
+						      chandef->center_freq1,
+						      width);
+		if (ret < 0)
+			return ret;
+		else if (ret > 0)
+			return BIT(chandef->width);
 
-	if (!chandef->center_freq2)
-		return 0;
+		if (!chandef->center_freq2)
+			return 0;
+
+		ret = cfg80211_get_chans_dfs_required(wiphy,
+						      chandef->center_freq2,
+						      width);
+		if (ret < 0)
+			return ret;
+		else if (ret > 0)
+			return BIT(chandef->width);
 
-	return cfg80211_get_chans_dfs_required(wiphy, chandef->center_freq2,
-					       width);
+		break;
+	case NL80211_IFTYPE_STATION:
+	case NL80211_IFTYPE_P2P_CLIENT:
+	case NL80211_IFTYPE_MONITOR:
+	case NL80211_IFTYPE_AP_VLAN:
+	case NL80211_IFTYPE_WDS:
+	case NL80211_IFTYPE_P2P_DEVICE:
+	case NL80211_IFTYPE_UNSPECIFIED:
+		break;
+	case NUM_NL80211_IFTYPES:
+		WARN_ON(1);
+	}
+
+	return 0;
 }
 EXPORT_SYMBOL(cfg80211_chandef_dfs_required);
 
@@ -671,7 +700,8 @@ bool cfg80211_reg_can_beacon(struct wiphy *wiphy,
 
 	trace_cfg80211_reg_can_beacon(wiphy, chandef);
 
-	if (cfg80211_chandef_dfs_required(wiphy, chandef) > 0 &&
+	if (cfg80211_chandef_dfs_required(wiphy, chandef,
+					  NL80211_IFTYPE_UNSPECIFIED) > 0 &&
 	    cfg80211_chandef_dfs_available(wiphy, chandef)) {
 		/* We can skip IEEE80211_CHAN_NO_IR if chandef dfs available */
 		prohibited_flags = IEEE80211_CHAN_DISABLED;
@@ -701,6 +731,8 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 		        enum cfg80211_chan_mode *chanmode,
 		        u8 *radar_detect)
 {
+	int ret;
+
 	*chan = NULL;
 	*chanmode = CHAN_MODE_UNDEFINED;
 
@@ -743,8 +775,11 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chan = wdev->chandef.chan;
 			*chanmode = CHAN_MODE_SHARED;
 
-			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef))
+			ret = cfg80211_chandef_dfs_required(wdev->wiphy,
+							    &wdev->chandef,
+							    wdev->iftype);
+			WARN_ON(ret < 0);
+			if (ret > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;
@@ -753,8 +788,11 @@ cfg80211_get_chan_state(struct wireless_dev *wdev,
 			*chan = wdev->chandef.chan;
 			*chanmode = CHAN_MODE_SHARED;
 
-			if (cfg80211_chandef_dfs_required(wdev->wiphy,
-							  &wdev->chandef))
+			ret = cfg80211_chandef_dfs_required(wdev->wiphy,
+							    &wdev->chandef,
+							    wdev->iftype);
+			WARN_ON(ret < 0);
+			if (ret > 0)
 				*radar_detect |= BIT(wdev->chandef.width);
 		}
 		return;
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index 5af5cc6..a95212b 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -178,10 +178,13 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &setup->chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &setup->chandef);
+	err = cfg80211_chandef_dfs_required(wdev->wiphy,
+					    &setup->chandef,
+					    NL80211_IFTYPE_MESH_POINT);
 	if (err < 0)
 		return err;
-	if (err)
+
+	if (err > 0)
 		radar_detect_width = BIT(setup->chandef.width);
 
 	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 052c1bf..9fe7746 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3261,12 +3261,15 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &params.chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &params.chandef);
+	err = cfg80211_chandef_dfs_required(wdev->wiphy,
+					    &params.chandef,
+					    NL80211_IFTYPE_AP);
 	if (err < 0)
 		return err;
-	if (err) {
-		radar_detect_width = BIT(params.chandef.width);
+
+	if (err > 0) {
 		params.radar_required = true;
+		radar_detect_width = BIT(params.chandef.width);
 	}
 
 	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
@@ -5796,7 +5799,8 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	if (wdev->cac_started)
 		return -EBUSY;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef);
+	err = cfg80211_chandef_dfs_required(wdev->wiphy, &chandef,
+					    NL80211_IFTYPE_UNSPECIFIED);
 	if (err < 0)
 		return err;
 
@@ -5931,22 +5935,15 @@ skip_beacons:
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &params.chandef))
 		return -EINVAL;
 
-	switch (dev->ieee80211_ptr->iftype) {
-	case NL80211_IFTYPE_AP:
-	case NL80211_IFTYPE_P2P_GO:
-	case NL80211_IFTYPE_ADHOC:
-	case NL80211_IFTYPE_MESH_POINT:
-		err = cfg80211_chandef_dfs_required(wdev->wiphy,
-						    &params.chandef);
-		if (err < 0)
-			return err;
-		if (err) {
-			radar_detect_width = BIT(params.chandef.width);
-			params.radar_required = true;
-		}
-		break;
-	default:
-		break;
+	err = cfg80211_chandef_dfs_required(wdev->wiphy,
+					    &params.chandef,
+					    wdev->iftype);
+	if (err < 0)
+		return err;
+
+	if (err > 0) {
+		radar_detect_width = BIT(params.chandef.width);
+		params.radar_required = true;
 	}
 
 	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-- 
1.9.0

[PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Luciano Coelho]

Move the counting part of the interface combination check from
cfg80211 to mac80211.

This is needed to simplify locking when the driver has to perform a
combination check by itself (eg. with channel-switch).

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v3:

   * pass the mode argument instead of IEEE80211_CHANCTX_SHARED to
     ieee80211_check_combinations() in ieee80211_vif_use_channel();

In v4:

   * rebased on top of slightly modified applied patches;
   * removed WARN_ON in ieee80211_start_radar_detection();
   * removed unnecessary TODOs in ieee80211_mgd_auth() and
     ieee80211_mgd_assoc();
   * use local and sdata instead of wiphy and wdev in
     ieee80211_check_combinations();
   * check if the vif has a chanctx instead of checking the type and
     if it is running in the interfaces iterator in
     ieee80211_check_combinations();
   * combined the "continue" cases in the iteration into a single if;
   * removed cfg80211_can_use_chan() since libertas mesh, the only
     remaining caller, doesn't really need it.

In v5:

   * use the radar detection widths from all existing contexts,
     regardless of whether we can share an existing context or if the
     context is exclusive.

In v6:

   * fix typo in the interface counting code
     s/sdata-vif.chanctx_conf/sdata_iter-vif.chanctx_conf/

In v7:

   * use new version of cfg80211_check_combinations()
   * remove unnecessary TODO from the documentation
   * remove the local argument in ieee80211_check_combinations, since
     it can be dereferenced from sdata anyway;

In v8:

   * fix radar_detect_width before calling
     ieee80211_check_combinations() -- rebase bug;

In v9:

   * pass sdata->wdev.iftype instead of sdata->vif.type when calling
     cfg80211_chandef_dfs_required();
---
 include/net/cfg80211.h     |  2 --
 net/mac80211/cfg.c         |  2 --
 net/mac80211/chan.c        | 17 +++++++++++
 net/mac80211/ieee80211_i.h |  4 +++
 net/mac80211/util.c        | 72 ++++++++++++++++++++++++++++++++++++++++++++++
 net/wireless/core.h        | 13 ++-------
 net/wireless/ibss.c        |  4 +++
 net/wireless/mesh.c        | 28 ------------------
 net/wireless/mlme.c        | 14 +--------
 net/wireless/nl80211.c     | 29 +++----------------
 net/wireless/util.c        |  5 ++++
 11 files changed, 110 insertions(+), 80 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index 51f278a..23a72e2 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -657,7 +657,6 @@ struct cfg80211_acl_data {
  * @p2p_opp_ps: P2P opportunistic PS
  * @acl: ACL configuration used by the drivers which has support for
  *	MAC address based access control
- * @radar_required: set if radar detection is required
  */
 struct cfg80211_ap_settings {
 	struct cfg80211_chan_def chandef;
@@ -675,7 +674,6 @@ struct cfg80211_ap_settings {
 	u8 p2p_ctwindow;
 	bool p2p_opp_ps;
 	const struct cfg80211_acl_data *acl;
-	bool radar_required;
 };
 
 /**
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index aaa59d7..9b555e0 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -972,7 +972,6 @@ static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	sdata->needed_rx_chains = sdata->local->rx_chains;
 
 	mutex_lock(&local->mtx);
-	sdata->radar_required = params->radar_required;
 	err = ieee80211_vif_use_channel(sdata, &params->chandef,
 					IEEE80211_CHANCTX_SHARED);
 	mutex_unlock(&local->mtx);
@@ -2930,7 +2929,6 @@ static int ieee80211_start_radar_detection(struct wiphy *wiphy,
 	/* whatever, but channel contexts should not complain about that one */
 	sdata->smps_mode = IEEE80211_SMPS_OFF;
 	sdata->needed_rx_chains = local->rx_chains;
-	sdata->radar_required = true;
 
 	err = ieee80211_vif_use_channel(sdata, chandef,
 					IEEE80211_CHANCTX_SHARED);
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 42c6592..384f3c7 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -513,6 +513,7 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 {
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_chanctx *ctx;
+	u8 radar_detect_width = 0;
 	int ret;
 
 	lockdep_assert_held(&local->mtx);
@@ -520,6 +521,22 @@ int ieee80211_vif_use_channel(struct ieee80211_sub_if_data *sdata,
 	WARN_ON(sdata->dev && netif_carrier_ok(sdata->dev));
 
 	mutex_lock(&local->chanctx_mtx);
+
+	ret = cfg80211_chandef_dfs_required(local->hw.wiphy,
+					    chandef,
+					    sdata->wdev.iftype);
+	if (ret < 0)
+		goto out;
+	if (ret > 0)
+		radar_detect_width = BIT(chandef->width);
+
+	sdata->radar_required = ret;
+
+	ret = ieee80211_check_combinations(sdata, chandef, mode,
+					   radar_detect_width);
+	if (ret < 0)
+		goto out;
+
 	__ieee80211_vif_release_channel(sdata);
 
 	ctx = ieee80211_find_chanctx(local, chandef, mode);
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 8603dfb..d3ebe7f 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1810,6 +1810,10 @@ int ieee80211_cs_headroom(struct ieee80211_local *local,
 			  enum nl80211_iftype iftype);
 void ieee80211_recalc_dtim(struct ieee80211_local *local,
 			   struct ieee80211_sub_if_data *sdata);
+int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
+				 const struct cfg80211_chan_def *chandef,
+				 enum ieee80211_chanctx_mode chanmode,
+				 u8 radar_detect);
 
 #ifdef CONFIG_MAC80211_NOINLINE
 #define debug_noinline noinline
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index d842af5..3972b64 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2801,3 +2801,75 @@ void ieee80211_recalc_dtim(struct ieee80211_local *local,
 
 	ps->dtim_count = dtim_count;
 }
+
+int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
+				 const struct cfg80211_chan_def *chandef,
+				 enum ieee80211_chanctx_mode chanmode,
+				 u8 radar_detect)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct ieee80211_sub_if_data *sdata_iter;
+	enum nl80211_iftype iftype = sdata->wdev.iftype;
+	int num[NUM_NL80211_IFTYPES];
+	struct ieee80211_chanctx *ctx;
+	int num_different_channels = 1;
+	int total = 1;
+
+	lockdep_assert_held(&local->chanctx_mtx);
+
+	if (WARN_ON(hweight32(radar_detect) > 1))
+		return -EINVAL;
+
+	if (WARN_ON(chanmode == IEEE80211_CHANCTX_SHARED && !chandef->chan))
+		return -EINVAL;
+
+	if (WARN_ON(iftype >= NUM_NL80211_IFTYPES))
+		return -EINVAL;
+
+	/* Always allow software iftypes */
+	if (local->hw.wiphy->software_iftypes & BIT(iftype)) {
+		if (radar_detect)
+			return -EINVAL;
+		return 0;
+	}
+
+	memset(num, 0, sizeof(num));
+
+	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
+		num[iftype] = 1;
+
+	list_for_each_entry(ctx, &local->chanctx_list, list) {
+		if (ctx->conf.radar_enabled)
+			radar_detect |= BIT(ctx->conf.def.width);
+		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) {
+			num_different_channels++;
+			continue;
+		}
+		if ((chanmode == IEEE80211_CHANCTX_SHARED) &&
+		    cfg80211_chandef_compatible(chandef,
+						&ctx->conf.def))
+			continue;
+		num_different_channels++;
+	}
+
+	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
+		struct wireless_dev *wdev_iter;
+
+		wdev_iter = &sdata_iter->wdev;
+
+		if (sdata_iter == sdata ||
+		    rcu_access_pointer(sdata_iter->vif.chanctx_conf) == NULL ||
+		    local->hw.wiphy->software_iftypes & BIT(wdev_iter->iftype))
+			continue;
+
+		num[wdev_iter->iftype]++;
+		total++;
+	}
+
+	if (total == 1 && !radar_detect)
+		return 0;
+
+	return cfg80211_check_combinations(local->hw.wiphy,
+					   num_different_channels,
+					   radar_detect, num);
+}
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 3975ffa..4a930ef 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -411,6 +411,9 @@ cfg80211_can_change_interface(struct cfg80211_registered_device *rdev,
 			      struct wireless_dev *wdev,
 			      enum nl80211_iftype iftype)
 {
+	/* TODO: For this function, we'll probably need to keep some
+	 * kind of interface combination check in cfg80211...
+	 */
 	return cfg80211_can_use_iftype_chan(rdev, wdev, iftype, NULL,
 					    CHAN_MODE_UNDEFINED, 0);
 }
@@ -425,16 +428,6 @@ cfg80211_can_add_interface(struct cfg80211_registered_device *rdev,
 	return cfg80211_can_change_interface(rdev, NULL, iftype);
 }
 
-static inline int
-cfg80211_can_use_chan(struct cfg80211_registered_device *rdev,
-		      struct wireless_dev *wdev,
-		      struct ieee80211_channel *chan,
-		      enum cfg80211_chan_mode chanmode)
-{
-	return cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					    chan, chanmode, 0);
-}
-
 static inline unsigned int elapsed_jiffies_msecs(unsigned long start)
 {
 	unsigned long end = jiffies;
diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index 349db9d..d81cb68 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -135,6 +135,10 @@ int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
 		radar_detect_width = BIT(params->chandef.width);
 	}
 
+	/* TODO: We need to check the combinations at this point, we
+	 * probably must move this call down to join_ibss() in
+	 * mac80211.
+	 */
 	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
 					   check_chan,
 					   (params->channel_fixed &&
diff --git a/net/wireless/mesh.c b/net/wireless/mesh.c
index a95212b..b8a7a35 100644
--- a/net/wireless/mesh.c
+++ b/net/wireless/mesh.c
@@ -99,7 +99,6 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 			 const struct mesh_config *conf)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
-	u8 radar_detect_width = 0;
 	int err;
 
 	BUILD_BUG_ON(IEEE80211_MAX_SSID_LEN != IEEE80211_MAX_MESH_ID_LEN);
@@ -178,22 +177,6 @@ int __cfg80211_join_mesh(struct cfg80211_registered_device *rdev,
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &setup->chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy,
-					    &setup->chandef,
-					    NL80211_IFTYPE_MESH_POINT);
-	if (err < 0)
-		return err;
-
-	if (err > 0)
-		radar_detect_width = BIT(setup->chandef.width);
-
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					   setup->chandef.chan,
-					   CHAN_MODE_SHARED,
-					   radar_detect_width);
-	if (err)
-		return err;
-
 	err = rdev_join_mesh(rdev, dev, conf, setup);
 	if (!err) {
 		memcpy(wdev->ssid, setup->mesh_id, setup->mesh_id_len);
@@ -239,17 +222,6 @@ int cfg80211_set_mesh_channel(struct cfg80211_registered_device *rdev,
 		if (!netif_running(wdev->netdev))
 			return -ENETDOWN;
 
-		/* cfg80211_can_use_chan() calls
-		 * cfg80211_can_use_iftype_chan() with no radar
-		 * detection, so if we're trying to use a radar
-		 * channel here, something is wrong.
-		 */
-		WARN_ON_ONCE(chandef->chan->flags & IEEE80211_CHAN_RADAR);
-		err = cfg80211_can_use_chan(rdev, wdev, chandef->chan,
-					    CHAN_MODE_SHARED);
-		if (err)
-			return err;
-
 		err = rdev_libertas_set_mesh_channel(rdev, wdev->netdev,
 						     chandef->chan);
 		if (!err)
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index c52ff59..4b4ba70 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -233,14 +233,8 @@ int cfg80211_mlme_auth(struct cfg80211_registered_device *rdev,
 	if (!req.bss)
 		return -ENOENT;
 
-	err = cfg80211_can_use_chan(rdev, wdev, req.bss->channel,
-				    CHAN_MODE_SHARED);
-	if (err)
-		goto out;
-
 	err = rdev_auth(rdev, dev, &req);
 
-out:
 	cfg80211_put_bss(&rdev->wiphy, req.bss);
 	return err;
 }
@@ -306,16 +300,10 @@ int cfg80211_mlme_assoc(struct cfg80211_registered_device *rdev,
 	if (!req->bss)
 		return -ENOENT;
 
-	err = cfg80211_can_use_chan(rdev, wdev, chan, CHAN_MODE_SHARED);
-	if (err)
-		goto out;
-
 	err = rdev_assoc(rdev, dev, req);
 	if (!err)
 		cfg80211_hold_bss(bss_from_pub(req->bss));
-
-out:
-	if (err)
+	else
 		cfg80211_put_bss(&rdev->wiphy, req->bss);
 
 	return err;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 9fe7746..07253bf 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -3142,7 +3142,6 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_ap_settings params;
 	int err;
-	u8 radar_detect_width = 0;
 
 	if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_AP &&
 	    dev->ieee80211_ptr->iftype != NL80211_IFTYPE_P2P_GO)
@@ -3261,24 +3260,6 @@ static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 	if (!cfg80211_reg_can_beacon(&rdev->wiphy, &params.chandef))
 		return -EINVAL;
 
-	err = cfg80211_chandef_dfs_required(wdev->wiphy,
-					    &params.chandef,
-					    NL80211_IFTYPE_AP);
-	if (err < 0)
-		return err;
-
-	if (err > 0) {
-		params.radar_required = true;
-		radar_detect_width = BIT(params.chandef.width);
-	}
-
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					   params.chandef.chan,
-					   CHAN_MODE_SHARED,
-					   radar_detect_width);
-	if (err)
-		return err;
-
 	if (info->attrs[NL80211_ATTR_ACL_POLICY]) {
 		params.acl = parse_acl_data(&rdev->wiphy, info);
 		if (IS_ERR(params.acl))
@@ -5813,12 +5794,6 @@ static int nl80211_start_radar_detection(struct sk_buff *skb,
 	if (!rdev->ops->start_radar_detection)
 		return -EOPNOTSUPP;
 
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					   chandef.chan, CHAN_MODE_SHARED,
-					   BIT(chandef.width));
-	if (err)
-		return err;
-
 	cac_time_ms = cfg80211_chandef_dfs_cac_time(&rdev->wiphy, &chandef);
 	if (WARN_ON(!cac_time_ms))
 		cac_time_ms = IEEE80211_DFS_MIN_CAC_TIME_MS;
@@ -5946,6 +5921,10 @@ skip_beacons:
 		params.radar_required = true;
 	}
 
+	/* TODO: I left this here for now.  With channel switch, the
+	 * verification is a bit more complicated, because we only do
+	 * it later when the channel switch really happens.
+	 */
 	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
 					   params.chandef.chan,
 					   CHAN_MODE_SHARED,
diff --git a/net/wireless/util.c b/net/wireless/util.c
index a24448b..1069a24 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -1360,6 +1360,11 @@ int cfg80211_can_use_iftype_chan(struct cfg80211_registered_device *rdev,
 
 	num[iftype] = 1;
 
+	/* TODO: We'll probably not need this anymore, since this
+	 * should only be called with CHAN_MODE_UNDEFINED. There are
+	 * still a couple of pending calls where other chanmodes are
+	 * used, but we should get rid of them.
+	 */
 	switch (chanmode) {
 	case CHAN_MODE_UNDEFINED:
 		break;
-- 
1.9.0

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Michal Kazior]

On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@intel.com> wrote:

[...]

> +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> +                                const struct cfg80211_chan_def *chandef,
> +                                enum ieee80211_chanctx_mode chanmode,
> +                                u8 radar_detect)
> +{
> +       struct ieee80211_local *local = sdata->local;
> +       struct ieee80211_sub_if_data *sdata_iter;
> +       enum nl80211_iftype iftype = sdata->wdev.iftype;
> +       int num[NUM_NL80211_IFTYPES];
> +       struct ieee80211_chanctx *ctx;
> +       int num_different_channels = 1;
> +       int total = 1;
> +
> +       lockdep_assert_held(&local->chanctx_mtx);
> +
> +       if (WARN_ON(hweight32(radar_detect) > 1))
> +               return -EINVAL;
> +
> +       if (WARN_ON(chanmode == IEEE80211_CHANCTX_SHARED && !chandef->chan))
> +               return -EINVAL;
> +
> +       if (WARN_ON(iftype >= NUM_NL80211_IFTYPES))
> +               return -EINVAL;
> +
> +       /* Always allow software iftypes */
> +       if (local->hw.wiphy->software_iftypes & BIT(iftype)) {
> +               if (radar_detect)
> +                       return -EINVAL;
> +               return 0;
> +       }
> +
> +       memset(num, 0, sizeof(num));
> +
> +       if (iftype != NL80211_IFTYPE_UNSPECIFIED)
> +               num[iftype] = 1;
> +

> +       list_for_each_entry(ctx, &local->chanctx_list, list) {
> +               if (ctx->conf.radar_enabled)
> +                       radar_detect |= BIT(ctx->conf.def.width);

Is this really correct?

The original behaviour was to:

 for each wdev:
   get_chan_state(..., &radar_width)

where get_chan_state() ORs BIT(width) accordingly to iftype, etc.

For 2 APs with different operational widths (e.g. 20 and 40) this means:

a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)

I think radar_detect should be computed:

 radar_detect = 0 // **
 for each vif:
  if vif.ctx == ctx && vif.radar_required:
   radar_detect = BIT(vif.bss.width)

** you could argue to initialize with BIT(ctx->width), but all things
seem to use ieee80211_vif_use_channel() (including CAC/radar
detection) so bss_conf.chandef.width should be set correctly and
iteration alone should suffice.


Michał

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Coelho, Luciano]

T24gRnJpLCAyMDE0LTAzLTE0IGF0IDA5OjQwICswMTAwLCBNaWNoYWwgS2F6aW9yIHdyb3RlOg0K
PiBPbiAxMSBNYXJjaCAyMDE0IDE1OjE2LCBMdWNpYW5vIENvZWxobyA8bHVjaWFuby5jb2VsaG9A
aW50ZWwuY29tPiB3cm90ZToNCj4gDQo+IFsuLi5dDQo+IA0KPiA+ICtpbnQgaWVlZTgwMjExX2No
ZWNrX2NvbWJpbmF0aW9ucyhzdHJ1Y3QgaWVlZTgwMjExX3N1Yl9pZl9kYXRhICpzZGF0YSwNCj4g
PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjb25zdCBzdHJ1Y3QgY2ZnODAyMTFf
Y2hhbl9kZWYgKmNoYW5kZWYsDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg
ZW51bSBpZWVlODAyMTFfY2hhbmN0eF9tb2RlIGNoYW5tb2RlLA0KPiA+ICsgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICAgIHU4IHJhZGFyX2RldGVjdCkNCj4gPiArew0KPiA+ICsgICAgICAg
c3RydWN0IGllZWU4MDIxMV9sb2NhbCAqbG9jYWwgPSBzZGF0YS0+bG9jYWw7DQo+ID4gKyAgICAg
ICBzdHJ1Y3QgaWVlZTgwMjExX3N1Yl9pZl9kYXRhICpzZGF0YV9pdGVyOw0KPiA+ICsgICAgICAg
ZW51bSBubDgwMjExX2lmdHlwZSBpZnR5cGUgPSBzZGF0YS0+d2Rldi5pZnR5cGU7DQo+ID4gKyAg
ICAgICBpbnQgbnVtW05VTV9OTDgwMjExX0lGVFlQRVNdOw0KPiA+ICsgICAgICAgc3RydWN0IGll
ZWU4MDIxMV9jaGFuY3R4ICpjdHg7DQo+ID4gKyAgICAgICBpbnQgbnVtX2RpZmZlcmVudF9jaGFu
bmVscyA9IDE7DQo+ID4gKyAgICAgICBpbnQgdG90YWwgPSAxOw0KPiA+ICsNCj4gPiArICAgICAg
IGxvY2tkZXBfYXNzZXJ0X2hlbGQoJmxvY2FsLT5jaGFuY3R4X210eCk7DQo+ID4gKw0KPiA+ICsg
ICAgICAgaWYgKFdBUk5fT04oaHdlaWdodDMyKHJhZGFyX2RldGVjdCkgPiAxKSkNCj4gPiArICAg
ICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7DQo+ID4gKw0KPiA+ICsgICAgICAgaWYgKFdBUk5f
T04oY2hhbm1vZGUgPT0gSUVFRTgwMjExX0NIQU5DVFhfU0hBUkVEICYmICFjaGFuZGVmLT5jaGFu
KSkNCj4gPiArICAgICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7DQo+ID4gKw0KPiA+ICsgICAg
ICAgaWYgKFdBUk5fT04oaWZ0eXBlID49IE5VTV9OTDgwMjExX0lGVFlQRVMpKQ0KPiA+ICsgICAg
ICAgICAgICAgICByZXR1cm4gLUVJTlZBTDsNCj4gPiArDQo+ID4gKyAgICAgICAvKiBBbHdheXMg
YWxsb3cgc29mdHdhcmUgaWZ0eXBlcyAqLw0KPiA+ICsgICAgICAgaWYgKGxvY2FsLT5ody53aXBo
eS0+c29mdHdhcmVfaWZ0eXBlcyAmIEJJVChpZnR5cGUpKSB7DQo+ID4gKyAgICAgICAgICAgICAg
IGlmIChyYWRhcl9kZXRlY3QpDQo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJuIC1F
SU5WQUw7DQo+ID4gKyAgICAgICAgICAgICAgIHJldHVybiAwOw0KPiA+ICsgICAgICAgfQ0KPiA+
ICsNCj4gPiArICAgICAgIG1lbXNldChudW0sIDAsIHNpemVvZihudW0pKTsNCj4gPiArDQo+ID4g
KyAgICAgICBpZiAoaWZ0eXBlICE9IE5MODAyMTFfSUZUWVBFX1VOU1BFQ0lGSUVEKQ0KPiA+ICsg
ICAgICAgICAgICAgICBudW1baWZ0eXBlXSA9IDE7DQo+ID4gKw0KPiANCj4gPiArICAgICAgIGxp
c3RfZm9yX2VhY2hfZW50cnkoY3R4LCAmbG9jYWwtPmNoYW5jdHhfbGlzdCwgbGlzdCkgew0KPiA+
ICsgICAgICAgICAgICAgICBpZiAoY3R4LT5jb25mLnJhZGFyX2VuYWJsZWQpDQo+ID4gKyAgICAg
ICAgICAgICAgICAgICAgICAgcmFkYXJfZGV0ZWN0IHw9IEJJVChjdHgtPmNvbmYuZGVmLndpZHRo
KTsNCj4gDQo+IElzIHRoaXMgcmVhbGx5IGNvcnJlY3Q/DQoNCkknbSBub3Qgc3VyZSBhbnltb3Jl
LiA6KQ0KDQoNCj4gVGhlIG9yaWdpbmFsIGJlaGF2aW91ciB3YXMgdG86DQo+IA0KPiAgZm9yIGVh
Y2ggd2RldjoNCj4gICAgZ2V0X2NoYW5fc3RhdGUoLi4uLCAmcmFkYXJfd2lkdGgpDQo+IA0KPiB3
aGVyZSBnZXRfY2hhbl9zdGF0ZSgpIE9ScyBCSVQod2lkdGgpIGFjY29yZGluZ2x5IHRvIGlmdHlw
ZSwgZXRjLg0KPiANCj4gRm9yIDIgQVBzIHdpdGggZGlmZmVyZW50IG9wZXJhdGlvbmFsIHdpZHRo
cyAoZS5nLiAyMCBhbmQgNDApIHRoaXMgbWVhbnM6DQo+IA0KPiBhKSBvbGQgY29kZTogd2lkdGhf
ZGV0ZWN0ID0gQklUKFdJRFRIXzIwKSB8IEJJVChXSURUSF80MCkNCj4gYikgbmV3IGNvZGU6IHdp
ZHRoX2RldGVjdCA9IEJJVChjaGFuY3R4LT53aWR0aCkgd2hpY2ggaXMgQklUKFdJRFRIXzQwKQ0K
PiANCj4gSSB0aGluayByYWRhcl9kZXRlY3Qgc2hvdWxkIGJlIGNvbXB1dGVkOg0KPiANCj4gIHJh
ZGFyX2RldGVjdCA9IDAgLy8gKioNCj4gIGZvciBlYWNoIHZpZjoNCj4gICBpZiB2aWYuY3R4ID09
IGN0eCAmJiB2aWYucmFkYXJfcmVxdWlyZWQ6DQo+ICAgIHJhZGFyX2RldGVjdCA9IEJJVCh2aWYu
YnNzLndpZHRoKQ0KPiANCj4gKiogeW91IGNvdWxkIGFyZ3VlIHRvIGluaXRpYWxpemUgd2l0aCBC
SVQoY3R4LT53aWR0aCksIGJ1dCBhbGwgdGhpbmdzDQo+IHNlZW0gdG8gdXNlIGllZWU4MDIxMV92
aWZfdXNlX2NoYW5uZWwoKSAoaW5jbHVkaW5nIENBQy9yYWRhcg0KPiBkZXRlY3Rpb24pIHNvIGJz
c19jb25mLmNoYW5kZWYud2lkdGggc2hvdWxkIGJlIHNldCBjb3JyZWN0bHkgYW5kDQo+IGl0ZXJh
dGlvbiBhbG9uZSBzaG91bGQgc3VmZmljZS4NCg0KQnV0IGRvZXMgdGhpcyByZWFsbHkgbWFrZSBz
ZW5zZT8gVGhpcyB3YXMgbW9yZSBvciBsZXNzIHRoZSBpZGVhIEkgaGFkDQp3aGVuIEkgY2hhbmdl
ZCB0aGUgY2ZnODAyMTFfY2hhbmRlZl9kZnNfcmVxdWlyZWQoKSB0byByZXR1cm4gYSBiaXQgbWFz
aw0KKHdoaWNoIEkgbGF0ZXIgcmV2ZXJ0ZWQpLg0KDQpJIHRoaW5rIHRoZSBjdXJyZW50IGJlaGF2
aW9yIG1heSBiZSB3cm9uZy4gIEluIHRoZSBvbGQNCmNmZzgwMjExX2Nhbl91c2VfaWZ0eXBlX2No
YW4oKSB3ZSBtYXRjaCAqYW55KiBvZiB0aGUgYml0cyBpbg0KcmFkYXJfZGV0ZWN0IHdpdGggdGhl
IGJpdHMgc3VwcG9ydGVkIGluIHRoZSBpbnRlcmZhY2UgY29tYmluYXRpb25zLiAgU28sDQppbiB5
b3VyIGV4YW1wbGUsIHdlIHdvdWxkIGFjY2VwdCB0aGUgY29tYmluYXRpb24gZXZlbiBpZiB0aGUg
ZHJpdmVyIGRpZA0Kbm90IHN1cHBvcnQgV0lEVEhfNDAgKGJlY2F1c2UgaXQgd291bGQgbWF0Y2gg
V0lEVEhfMjApLiAgSWYgdGhlIGNvbnRleHQNCmlzIHNldCB0byBXSURUSF80MCwgZG9uJ3Qgd2Ug
aGF2ZSB0byBtYWtlIHN1cmUgdGhlIGRyaXZlciBjYW4gZGV0ZWN0DQpyYWRhcnMgb24gdGhhdCB3
aWR0aD8NCg0KLS0NCkNoZWVycywNCkx1Y2EuDQo=

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Michal Kazior]

On 18 March 2014 09:13, Coelho, Luciano <luciano.coelho@intel.com> wrote:
> On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote:
>> On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@intel.com> wrote:

[...]

>> > +       list_for_each_entry(ctx, &local->chanctx_list, list) {
>> > +               if (ctx->conf.radar_enabled)
>> > +                       radar_detect |= BIT(ctx->conf.def.width);
>>
>> Is this really correct?
>
> I'm not sure anymore. :)
>
>
>> The original behaviour was to:
>>
>>  for each wdev:
>>    get_chan_state(..., &radar_width)
>>
>> where get_chan_state() ORs BIT(width) accordingly to iftype, etc.
>>
>> For 2 APs with different operational widths (e.g. 20 and 40) this means:
>>
>> a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
>> b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)
>>
>> I think radar_detect should be computed:
>>
>>  radar_detect = 0 // **
>>  for each vif:
>>   if vif.ctx == ctx && vif.radar_required:
>>    radar_detect = BIT(vif.bss.width)
>>
>> ** you could argue to initialize with BIT(ctx->width), but all things
>> seem to use ieee80211_vif_use_channel() (including CAC/radar
>> detection) so bss_conf.chandef.width should be set correctly and
>> iteration alone should suffice.
>
> But does this really make sense? This was more or less the idea I had
> when I changed the cfg80211_chandef_dfs_required() to return a bit mask
> (which I later reverted).
>
> I think the current behavior may be wrong.  In the old
> cfg80211_can_use_iftype_chan() we match *any* of the bits in
> radar_detect with the bits supported in the interface combinations.  So,
> in your example, we would accept the combination even if the driver did
> not support WIDTH_40 (because it would match WIDTH_20).  If the context
> is set to WIDTH_40, don't we have to make sure the driver can detect
> radars on that width?

Fair point, but current behaviour of cfg80211_can_use_iftype_chan()
doesn't seem like a correct one to me. It shouldn't match any but
*all* of the radar_detect bits, no?


Michał

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Luca Coelho]

On Tue, 2014-03-18 at 09:36 +0100, Michal Kazior wrote:
> On 18 March 2014 09:13, Coelho, Luciano <luciano.coelho@intel.com> wrote:
> > On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote:
> >> On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@intel.com> wrote:
> 
> [...]
> 
> >> > +       list_for_each_entry(ctx, &local->chanctx_list, list) {
> >> > +               if (ctx->conf.radar_enabled)
> >> > +                       radar_detect |= BIT(ctx->conf.def.width);
> >>
> >> Is this really correct?
> >
> > I'm not sure anymore. :)
> >
> >
> >> The original behaviour was to:
> >>
> >>  for each wdev:
> >>    get_chan_state(..., &radar_width)
> >>
> >> where get_chan_state() ORs BIT(width) accordingly to iftype, etc.
> >>
> >> For 2 APs with different operational widths (e.g. 20 and 40) this means:
> >>
> >> a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
> >> b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)
> >>
> >> I think radar_detect should be computed:
> >>
> >>  radar_detect = 0 // **
> >>  for each vif:
> >>   if vif.ctx == ctx && vif.radar_required:
> >>    radar_detect = BIT(vif.bss.width)
> >>
> >> ** you could argue to initialize with BIT(ctx->width), but all things
> >> seem to use ieee80211_vif_use_channel() (including CAC/radar
> >> detection) so bss_conf.chandef.width should be set correctly and
> >> iteration alone should suffice.
> >
> > But does this really make sense? This was more or less the idea I had
> > when I changed the cfg80211_chandef_dfs_required() to return a bit mask
> > (which I later reverted).
> >
> > I think the current behavior may be wrong.  In the old
> > cfg80211_can_use_iftype_chan() we match *any* of the bits in
> > radar_detect with the bits supported in the interface combinations.  So,
> > in your example, we would accept the combination even if the driver did
> > not support WIDTH_40 (because it would match WIDTH_20).  If the context
> > is set to WIDTH_40, don't we have to make sure the driver can detect
> > radars on that width?
> 
> Fair point, but current behaviour of cfg80211_can_use_iftype_chan()
> doesn't seem like a correct one to me. It shouldn't match any but
> *all* of the radar_detect bits, no?

I don't really know.  I think that if we're using 40MHz, we need to be
able to detect on the entire 40MHz, and that includes the 20MHz part as
well.  So being able to detect on 20MHz in that same channel context is
irrelevant, isn't it?

I think the current cfg80211_can_use_iftype_chan() is wrong because it
allows narrow bandwidths to pass the test, even if we're using wider
ones.

--
Luca.

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Michal Kazior]

On 18 March 2014 09:44, Luca Coelho <luca@coelho.fi> wrote:
> On Tue, 2014-03-18 at 09:36 +0100, Michal Kazior wrote:
>> On 18 March 2014 09:13, Coelho, Luciano <luciano.coelho@intel.com> wrote:
>> > On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote:
>> >> On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@intel.com> wrote:
>>
>> [...]
>>
>> >> > +       list_for_each_entry(ctx, &local->chanctx_list, list) {
>> >> > +               if (ctx->conf.radar_enabled)
>> >> > +                       radar_detect |= BIT(ctx->conf.def.width);
>> >>
>> >> Is this really correct?
>> >
>> > I'm not sure anymore. :)
>> >
>> >
>> >> The original behaviour was to:
>> >>
>> >>  for each wdev:
>> >>    get_chan_state(..., &radar_width)
>> >>
>> >> where get_chan_state() ORs BIT(width) accordingly to iftype, etc.
>> >>
>> >> For 2 APs with different operational widths (e.g. 20 and 40) this means:
>> >>
>> >> a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
>> >> b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)
>> >>
>> >> I think radar_detect should be computed:
>> >>
>> >>  radar_detect = 0 // **
>> >>  for each vif:
>> >>   if vif.ctx == ctx && vif.radar_required:
>> >>    radar_detect = BIT(vif.bss.width)
>> >>
>> >> ** you could argue to initialize with BIT(ctx->width), but all things
>> >> seem to use ieee80211_vif_use_channel() (including CAC/radar
>> >> detection) so bss_conf.chandef.width should be set correctly and
>> >> iteration alone should suffice.
>> >
>> > But does this really make sense? This was more or less the idea I had
>> > when I changed the cfg80211_chandef_dfs_required() to return a bit mask
>> > (which I later reverted).
>> >
>> > I think the current behavior may be wrong.  In the old
>> > cfg80211_can_use_iftype_chan() we match *any* of the bits in
>> > radar_detect with the bits supported in the interface combinations.  So,
>> > in your example, we would accept the combination even if the driver did
>> > not support WIDTH_40 (because it would match WIDTH_20).  If the context
>> > is set to WIDTH_40, don't we have to make sure the driver can detect
>> > radars on that width?
>>
>> Fair point, but current behaviour of cfg80211_can_use_iftype_chan()
>> doesn't seem like a correct one to me. It shouldn't match any but
>> *all* of the radar_detect bits, no?
>
> I don't really know.  I think that if we're using 40MHz, we need to be
> able to detect on the entire 40MHz, and that includes the 20MHz part as
> well.  So being able to detect on 20MHz in that same channel context is
> irrelevant, isn't it?

True but take this hypothetical case:

You have a driver combination that supports radar_detect only at
40MHz. You start AP @ 40MHz with radar. You match the radar_detect.
You start AP @ 20MHz with radar. You match again radar_detect (because
there's the AP @ 40MHz running). You stop AP @ 40Mhz. You now have
only an AP running at 20MHz which requires radar_detect for 20MHz but
there's no combination to match that. Oops. We should've never allowed
the AP @ 20MHz to start.

I doubt there's hardware that needs a combination like that but it
just bothers me current structure and logic allows it.


Michał

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Luca Coelho]

On Tue, 2014-03-18 at 09:58 +0100, Michal Kazior wrote:
> On 18 March 2014 09:44, Luca Coelho <luca@coelho.fi> wrote:
> > On Tue, 2014-03-18 at 09:36 +0100, Michal Kazior wrote:
> >> On 18 March 2014 09:13, Coelho, Luciano <luciano.coelho@intel.com> wrote:
> >> > On Fri, 2014-03-14 at 09:40 +0100, Michal Kazior wrote:
> >> >> On 11 March 2014 15:16, Luciano Coelho <luciano.coelho@intel.com> wrote:
> >>
> >> [...]
> >>
> >> >> > +       list_for_each_entry(ctx, &local->chanctx_list, list) {
> >> >> > +               if (ctx->conf.radar_enabled)
> >> >> > +                       radar_detect |= BIT(ctx->conf.def.width);
> >> >>
> >> >> Is this really correct?
> >> >
> >> > I'm not sure anymore. :)
> >> >
> >> >
> >> >> The original behaviour was to:
> >> >>
> >> >>  for each wdev:
> >> >>    get_chan_state(..., &radar_width)
> >> >>
> >> >> where get_chan_state() ORs BIT(width) accordingly to iftype, etc.
> >> >>
> >> >> For 2 APs with different operational widths (e.g. 20 and 40) this means:
> >> >>
> >> >> a) old code: width_detect = BIT(WIDTH_20) | BIT(WIDTH_40)
> >> >> b) new code: width_detect = BIT(chanctx->width) which is BIT(WIDTH_40)
> >> >>
> >> >> I think radar_detect should be computed:
> >> >>
> >> >>  radar_detect = 0 // **
> >> >>  for each vif:
> >> >>   if vif.ctx == ctx && vif.radar_required:
> >> >>    radar_detect = BIT(vif.bss.width)
> >> >>
> >> >> ** you could argue to initialize with BIT(ctx->width), but all things
> >> >> seem to use ieee80211_vif_use_channel() (including CAC/radar
> >> >> detection) so bss_conf.chandef.width should be set correctly and
> >> >> iteration alone should suffice.
> >> >
> >> > But does this really make sense? This was more or less the idea I had
> >> > when I changed the cfg80211_chandef_dfs_required() to return a bit mask
> >> > (which I later reverted).
> >> >
> >> > I think the current behavior may be wrong.  In the old
> >> > cfg80211_can_use_iftype_chan() we match *any* of the bits in
> >> > radar_detect with the bits supported in the interface combinations.  So,
> >> > in your example, we would accept the combination even if the driver did
> >> > not support WIDTH_40 (because it would match WIDTH_20).  If the context
> >> > is set to WIDTH_40, don't we have to make sure the driver can detect
> >> > radars on that width?
> >>
> >> Fair point, but current behaviour of cfg80211_can_use_iftype_chan()
> >> doesn't seem like a correct one to me. It shouldn't match any but
> >> *all* of the radar_detect bits, no?
> >
> > I don't really know.  I think that if we're using 40MHz, we need to be
> > able to detect on the entire 40MHz, and that includes the 20MHz part as
> > well.  So being able to detect on 20MHz in that same channel context is
> > irrelevant, isn't it?
> 
> True but take this hypothetical case:
> 
> You have a driver combination that supports radar_detect only at
> 40MHz. You start AP @ 40MHz with radar. You match the radar_detect.
> You start AP @ 20MHz with radar. You match again radar_detect (because
> there's the AP @ 40MHz running). You stop AP @ 40Mhz. You now have
> only an AP running at 20MHz which requires radar_detect for 20MHz but
> there's no combination to match that. Oops. We should've never allowed
> the AP @ 20MHz to start.

Yeah, this scenario is broken.  My patch series doesn't break it any
more than it already is. :) Actually it improves this a bit, because the
opposite scenario would work better:

A hardware that supports radar detect only at 20MHz.  You start AP at
20MHz, everything is fine.  Now you start another AP at 40MHz, the
current code would allow it (because it checks if *any* width matches).
Oops! With my patches we wouldn't allow it, because the new chanctx
would require 40MHz when you start the second AP and it wouldn't be
allowed.


> I doubt there's hardware that needs a combination like that but it
> just bothers me current structure and logic allows it.

Yeah, if you keep peeking into the DFS code you are probably going to
find even worse things. :(

I guess the only good way to fix the scenario you described would be to
recheck combinations whenever a vif gets removed from a chanctx as well.

But maybe a better thing to do would be to change the
radar_detect_widths bitmask into max_radar_detect_width or something.  I
don't think it really makes sense to be able to detect radars on, say,
40MHz but not on 20MHz.

Anyway, this is a separate issue and I don't think it should block my
series from going in.  Someone should fix DFS separately.  As Johannes
would probably say, "patches welcome". :)

--
Cheers,
Luca.

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Wen Gong]

On 3/11/2014 10:16 PM, Luciano Coelho wrote:
> ...
> +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> +				 const struct cfg80211_chan_def *chandef,
> +				 enum ieee80211_chanctx_mode chanmode,
> +				 u8 radar_detect)
> +{
> +	struct ieee80211_local *local = sdata->local;
> +	struct ieee80211_sub_if_data *sdata_iter;
> +	enum nl80211_iftype iftype = sdata->wdev.iftype;
> +	int num[NUM_NL80211_IFTYPES];
> +	struct ieee80211_chanctx *ctx;
> +	int num_different_channels = 1;
> +	int total = 1;
> +
> +	lockdep_assert_held(&local->chanctx_mtx);
> +
> +	if (WARN_ON(hweight32(radar_detect) > 1))
> +		return -EINVAL;
> +
> +	if (WARN_ON(chanmode == IEEE80211_CHANCTX_SHARED && !chandef->chan))
> +		return -EINVAL;
> +
> +	if (WARN_ON(iftype >= NUM_NL80211_IFTYPES))
> +		return -EINVAL;
> +
> +	/* Always allow software iftypes */
> +	if (local->hw.wiphy->software_iftypes & BIT(iftype)) {
> +		if (radar_detect)
> +			return -EINVAL;
> +		return 0;
> +	}
> +
> +	memset(num, 0, sizeof(num));
> +
> +	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
> +		num[iftype] = 1;
> +
> +	list_for_each_entry(ctx, &local->chanctx_list, list) {
> +		if (ctx->conf.radar_enabled)
> +			radar_detect |= BIT(ctx->conf.def.width);
> +		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) {
> +			num_different_channels++;
> +			continue;
> +		}
> +		if ((chanmode == IEEE80211_CHANCTX_SHARED) &&
> +		    cfg80211_chandef_compatible(chandef,
> +						&ctx->conf.def))
> +			continue;
> +		num_different_channels++;
> +	}
> +
> +	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> +		struct wireless_dev *wdev_iter;
> +
> +		wdev_iter = &sdata_iter->wdev;
> +
> +		if (sdata_iter == sdata ||
> +		    rcu_access_pointer(sdata_iter->vif.chanctx_conf) == NULL ||
> +		    local->hw.wiphy->software_iftypes & BIT(wdev_iter->iftype))
> +			continue;
> +
> +		num[wdev_iter->iftype]++;
> +		total++;
> +	}
> +
> +	if (total == 1 && !radar_detect)
> +		return 0;
> +

should also check with cfg80211_check_combinations() when total == 1 and 
num_different_channels > 1 ?

When MLO is enabled, it could have 2 channels for one ieee80211_sub_if_data.

> +	return cfg80211_check_combinations(local->hw.wiphy,
> +					   num_different_channels,
> +					   radar_detect, num);
> +}
> ...

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Johannes Berg]

On Mon, 2023-01-09 at 17:39 +0800, Wen Gong wrote:
> On 3/11/2014 10:16 PM, Luciano Coelho wrote:
> > ...
> > +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> > +				 const struct cfg80211_chan_def *chandef,
> > +				 enum ieee80211_chanctx_mode chanmode,
> > +				 u8 radar_detect)
> > +{
> > +	struct ieee80211_local *local = sdata->local;
> > +	struct ieee80211_sub_if_data *sdata_iter;
> > +	enum nl80211_iftype iftype = sdata->wdev.iftype;
> > +	int num[NUM_NL80211_IFTYPES];
> > +	struct ieee80211_chanctx *ctx;
> > +	int num_different_channels = 1;
> > +	int total = 1;
> > +
> > +	lockdep_assert_held(&local->chanctx_mtx);
> > +
> > +	if (WARN_ON(hweight32(radar_detect) > 1))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(chanmode == IEEE80211_CHANCTX_SHARED && !chandef->chan))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(iftype >= NUM_NL80211_IFTYPES))
> > +		return -EINVAL;
> > +
> > +	/* Always allow software iftypes */
> > +	if (local->hw.wiphy->software_iftypes & BIT(iftype)) {
> > +		if (radar_detect)
> > +			return -EINVAL;
> > +		return 0;
> > +	}
> > +
> > +	memset(num, 0, sizeof(num));
> > +
> > +	if (iftype != NL80211_IFTYPE_UNSPECIFIED)
> > +		num[iftype] = 1;
> > +
> > +	list_for_each_entry(ctx, &local->chanctx_list, list) {
> > +		if (ctx->conf.radar_enabled)
> > +			radar_detect |= BIT(ctx->conf.def.width);
> > +		if (ctx->mode == IEEE80211_CHANCTX_EXCLUSIVE) {
> > +			num_different_channels++;
> > +			continue;
> > +		}
> > +		if ((chanmode == IEEE80211_CHANCTX_SHARED) &&
> > +		    cfg80211_chandef_compatible(chandef,
> > +						&ctx->conf.def))
> > +			continue;
> > +		num_different_channels++;
> > +	}
> > +
> > +	list_for_each_entry_rcu(sdata_iter, &local->interfaces, list) {
> > +		struct wireless_dev *wdev_iter;
> > +
> > +		wdev_iter = &sdata_iter->wdev;
> > +
> > +		if (sdata_iter == sdata ||
> > +		    rcu_access_pointer(sdata_iter->vif.chanctx_conf) == NULL ||
> > +		    local->hw.wiphy->software_iftypes & BIT(wdev_iter->iftype))
> > +			continue;
> > +
> > +		num[wdev_iter->iftype]++;
> > +		total++;
> > +	}
> > +
> > +	if (total == 1 && !radar_detect)
> > +		return 0;
> > +
> 
> should also check with cfg80211_check_combinations() when total == 1 and 
> num_different_channels > 1 ?
> 
> When MLO is enabled, it could have 2 channels for one ieee80211_sub_if_data.
> 

Heh. You're commenting on a patch from 2014, well before MLO :-)

Not sure what happens in the code now?

johannes

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Wen Gong]

On 1/9/2023 6:05 PM, Johannes Berg wrote:
> On Mon, 2023-01-09 at 17:39 +0800, Wen Gong wrote:
>> On 3/11/2014 10:16 PM, Luciano Coelho wrote:
>>> ...
>>> +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>>> +				 const struct cfg80211_chan_def *chandef,
>>> +				 enum ieee80211_chanctx_mode chanmode,
>>> +				 u8 radar_detect)
...
>>> +
>>> +	if (total == 1 && !radar_detect)
>>> +		return 0;
>>> +
>> should also check with cfg80211_check_combinations() when total == 1 and
>> num_different_channels > 1 ?
>>
>> When MLO is enabled, it could have 2 channels for one ieee80211_sub_if_data.
>>
> Heh. You're commenting on a patch from 2014, well before MLO :-)
>
> Not sure what happens in the code now?
>
> johannes
Yes, it is 2014. Each interface only has one channel at 2014.
I did not hit issue for the code.

the story is like this:
When station interface and p2p device interface both running.
the station connect to MLO AP which has 2 links.
The ieee80211_link_use_channel()/ieee80211_check_combinations() call 
cfg80211_check_combinations()
for the channel1 and channel2 because total==2.
When only station interface is running, not called for channel1/channel2 
because total==1.
That is the little thing I hit.

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Johannes Berg]

On Tue, 2023-01-10 at 15:23 +0800, Wen Gong wrote:
> On 1/9/2023 6:05 PM, Johannes Berg wrote:
> > On Mon, 2023-01-09 at 17:39 +0800, Wen Gong wrote:
> > > On 3/11/2014 10:16 PM, Luciano Coelho wrote:
> > > > ...
> > > > +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
> > > > +				 const struct cfg80211_chan_def *chandef,
> > > > +				 enum ieee80211_chanctx_mode chanmode,
> > > > +				 u8 radar_detect)
> ...
> > > > +
> > > > +	if (total == 1 && !radar_detect)
> > > > +		return 0;
> > > > +
> > > should also check with cfg80211_check_combinations() when total == 1 and
> > > num_different_channels > 1 ?
> > > 
> > > When MLO is enabled, it could have 2 channels for one ieee80211_sub_if_data.
> > > 
> > Heh. You're commenting on a patch from 2014, well before MLO :-)
> > 
> > Not sure what happens in the code now?
> > 
> > johannes
> Yes, it is 2014. Each interface only has one channel at 2014.
> I did not hit issue for the code.
> 
> the story is like this:
> When station interface and p2p device interface both running.
> the station connect to MLO AP which has 2 links.
> The ieee80211_link_use_channel()/ieee80211_check_combinations() call 
> cfg80211_check_combinations()
> for the channel1 and channel2 because total==2.
> When only station interface is running, not called for channel1/channel2 
> because total==1.
> That is the little thing I hit.
> 

So ... I guess you found a bug? Not sure what I'm supposed to say here.
Send a fix?

johannes

Re: [PATCH v10 3/5] cfg80211/mac80211: move interface counting for combination check to mac80211

[Wen Gong]

On 2/14/2023 10:41 PM, Johannes Berg wrote:
> On Tue, 2023-01-10 at 15:23 +0800, Wen Gong wrote:
>> On 1/9/2023 6:05 PM, Johannes Berg wrote:
>>> On Mon, 2023-01-09 at 17:39 +0800, Wen Gong wrote:
>>>> On 3/11/2014 10:16 PM, Luciano Coelho wrote:
>>>>> ...
>>>>> +int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
>>>>> +				 const struct cfg80211_chan_def *chandef,
>>>>> +				 enum ieee80211_chanctx_mode chanmode,
>>>>> +				 u8 radar_detect)
>> ...
>>>>> +
>>>>> +	if (total == 1 && !radar_detect)
>>>>> +		return 0;
>>>>> +
>>>> should also check with cfg80211_check_combinations() when total == 1 and
>>>> num_different_channels > 1 ?
>>>>
>>>> When MLO is enabled, it could have 2 channels for one ieee80211_sub_if_data.
>>>>
>>> Heh. You're commenting on a patch from 2014, well before MLO :-)
>>>
>>> Not sure what happens in the code now?
>>>
>>> johannes
>> Yes, it is 2014. Each interface only has one channel at 2014.
>> I did not hit issue for the code.
>>
>> the story is like this:
>> When station interface and p2p device interface both running.
>> the station connect to MLO AP which has 2 links.
>> The ieee80211_link_use_channel()/ieee80211_check_combinations() call
>> cfg80211_check_combinations()
>> for the channel1 and channel2 because total==2.
>> When only station interface is running, not called for channel1/channel2
>> because total==1.
>> That is the little thing I hit.
>>
> So ... I guess you found a bug? Not sure what I'm supposed to say here.
> Send a fix?
I consider it NOT a bug. It is only the check is not strict here.
>
> johannes

[PATCH v10 4/5] cfg80211/mac80211: move combination check to mac80211 for ibss

[Luciano Coelho]

Now that mac80211 can check the interface combinations itself, move
the combinations check from cfg80211 to mac80211 when joining an IBSS.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
In v2:

   * lock the chanctx mutex in ieee80211_ibss_join() before calling
     ieee80211_check_combinations(). (Thanks Michal);

   * pass the mode argument instead of IEEE80211_CHANCTX_SHARED to
     ieee80211_check_combinations() in ieee80211_vif_use_channel();

In v3:

   * moved the second change from v2 (pass the mode argument...) to
     the previous patch, where it should be;

In v4:

   * rebased on top of slightly modified applied patches

In v5:

   * use local and sdata in ibss when calling
     ieee80211_check_combinations()

In v7:

   * don't pass local in ieee80211_check_combinations() anymore;

In v8:

   * fix radar_detect_width before calling
     ieee80211_check_combinations() -- rebase bug;

In v9:

   * pass sdata->wdev.iftype instead of sdata->vif.type when calling
     cfg80211_chandef_dfs_required();
---
 net/mac80211/ibss.c | 32 +++++++++++++++++++++++++++++---
 net/wireless/ibss.c | 28 ----------------------------
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 12057fd..72b9337 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -1644,7 +1644,33 @@ int ieee80211_ibss_join(struct ieee80211_sub_if_data *sdata,
 	u32 changed = 0;
 	u32 rate_flags;
 	struct ieee80211_supported_band *sband;
+	enum ieee80211_chanctx_mode chanmode;
+	struct ieee80211_local *local = sdata->local;
+	int radar_detect_width = 0;
 	int i;
+	int ret;
+
+	ret = cfg80211_chandef_dfs_required(local->hw.wiphy,
+					    &params->chandef,
+					    sdata->wdev.iftype);
+	if (ret < 0)
+		return ret;
+
+	if (ret > 0) {
+		if (!params->userspace_handles_dfs)
+			return -EINVAL;
+		radar_detect_width = BIT(params->chandef.width);
+	}
+
+	chanmode = (params->channel_fixed && !ret) ?
+		IEEE80211_CHANCTX_SHARED : IEEE80211_CHANCTX_EXCLUSIVE;
+
+	mutex_lock(&local->chanctx_mtx);
+	ret = ieee80211_check_combinations(sdata, &params->chandef, chanmode,
+					   radar_detect_width);
+	mutex_unlock(&local->chanctx_mtx);
+	if (ret < 0)
+		return ret;
 
 	if (params->bssid) {
 		memcpy(sdata->u.ibss.bssid, params->bssid, ETH_ALEN);
@@ -1659,7 +1685,7 @@ int ieee80211_ibss_join(struct ieee80211_sub_if_data *sdata,
 
 	/* fix basic_rates if channel does not support these rates */
 	rate_flags = ieee80211_chandef_rate_flags(&params->chandef);
-	sband = sdata->local->hw.wiphy->bands[params->chandef.chan->band];
+	sband = local->hw.wiphy->bands[params->chandef.chan->band];
 	for (i = 0; i < sband->n_bitrates; i++) {
 		if ((rate_flags & sband->bitrates[i].flags) != rate_flags)
 			sdata->u.ibss.basic_rates &= ~BIT(i);
@@ -1708,9 +1734,9 @@ int ieee80211_ibss_join(struct ieee80211_sub_if_data *sdata,
 	ieee80211_bss_info_change_notify(sdata, changed);
 
 	sdata->smps_mode = IEEE80211_SMPS_OFF;
-	sdata->needed_rx_chains = sdata->local->rx_chains;
+	sdata->needed_rx_chains = local->rx_chains;
 
-	ieee80211_queue_work(&sdata->local->hw, &sdata->work);
+	ieee80211_queue_work(&local->hw, &sdata->work);
 
 	return 0;
 }
diff --git a/net/wireless/ibss.c b/net/wireless/ibss.c
index d81cb68..8282de8 100644
--- a/net/wireless/ibss.c
+++ b/net/wireless/ibss.c
@@ -88,8 +88,6 @@ int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
 			 struct cfg80211_cached_keys *connkeys)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
-	struct ieee80211_channel *check_chan;
-	u8 radar_detect_width = 0;
 	int err;
 
 	ASSERT_WDEV_LOCK(wdev);
@@ -126,32 +124,6 @@ int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
 #ifdef CONFIG_CFG80211_WEXT
 	wdev->wext.ibss.chandef = params->chandef;
 #endif
-	check_chan = params->chandef.chan;
-	if (params->userspace_handles_dfs) {
-		/* Check for radar even if the current channel is not
-		 * a radar channel - it might decide to change to DFS
-		 * channel later.
-		 */
-		radar_detect_width = BIT(params->chandef.width);
-	}
-
-	/* TODO: We need to check the combinations at this point, we
-	 * probably must move this call down to join_ibss() in
-	 * mac80211.
-	 */
-	err = cfg80211_can_use_iftype_chan(rdev, wdev, wdev->iftype,
-					   check_chan,
-					   (params->channel_fixed &&
-					    !radar_detect_width)
-					   ? CHAN_MODE_SHARED
-					   : CHAN_MODE_EXCLUSIVE,
-					   radar_detect_width);
-
-	if (err) {
-		wdev->connect_keys = NULL;
-		return err;
-	}
-
 	err = rdev_join_ibss(rdev, dev, params);
 	if (err) {
 		wdev->connect_keys = NULL;
-- 
1.9.0

[PATCH v10 5/5] cfg80211/mac80211: move more combination checks to mac80211

[Luciano Coelho]

Get rid of the cfg80211_can_add_interface() and
cfg80211_can_change_interface() functions by moving that functionality
to mac80211.  With this patch all interface combination checks are now
out of cfg80211 (except for the channel switch case which will be
addressed in a future commit).

Additionally, modify the ieee80211_check_combinations() function so
that an undefined chandef can be passed, in order to use it before a
channel is defined.

Signed-off-by: Luciano Coelho <luciano.coelho@intel.com>
---
 net/mac80211/cfg.c     |  9 +++++++++
 net/mac80211/iface.c   |  6 +++++-
 net/mac80211/util.c    | 10 +++++++---
 net/wireless/core.c    | 11 +++--------
 net/wireless/core.h    | 22 ----------------------
 net/wireless/nl80211.c |  5 ++---
 net/wireless/util.c    |  5 -----
 7 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 9b555e0..fae76f7 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -109,6 +109,15 @@ static int ieee80211_change_iface(struct wiphy *wiphy,
 static int ieee80211_start_p2p_device(struct wiphy *wiphy,
 				      struct wireless_dev *wdev)
 {
+	struct ieee80211_sub_if_data *sdata = IEEE80211_WDEV_TO_SUB_IF(wdev);
+	int ret;
+
+	mutex_lock(&sdata->local->chanctx_mtx);
+	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
+	mutex_unlock(&sdata->local->chanctx_mtx);
+	if (ret < 0)
+		return ret;
+
 	return ieee80211_do_open(wdev, true);
 }
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index be198f4..4c55481 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -250,6 +250,7 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 {
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_sub_if_data *nsdata;
+	int ret;
 
 	ASSERT_RTNL();
 
@@ -300,7 +301,10 @@ static int ieee80211_check_concurrent_iface(struct ieee80211_sub_if_data *sdata,
 		}
 	}
 
-	return 0;
+	mutex_lock(&local->chanctx_mtx);
+	ret = ieee80211_check_combinations(sdata, NULL, 0, 0);
+	mutex_unlock(&local->chanctx_mtx);
+	return ret;
 }
 
 static int ieee80211_check_queues(struct ieee80211_sub_if_data *sdata,
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 3972b64..955b043 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2812,7 +2812,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 	enum nl80211_iftype iftype = sdata->wdev.iftype;
 	int num[NUM_NL80211_IFTYPES];
 	struct ieee80211_chanctx *ctx;
-	int num_different_channels = 1;
+	int num_different_channels = 0;
 	int total = 1;
 
 	lockdep_assert_held(&local->chanctx_mtx);
@@ -2820,9 +2820,13 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 	if (WARN_ON(hweight32(radar_detect) > 1))
 		return -EINVAL;
 
-	if (WARN_ON(chanmode == IEEE80211_CHANCTX_SHARED && !chandef->chan))
+	if (WARN_ON(chandef && chanmode == IEEE80211_CHANCTX_SHARED &&
+		    !chandef->chan))
 		return -EINVAL;
 
+	if (chandef)
+		num_different_channels = 1;
+
 	if (WARN_ON(iftype >= NUM_NL80211_IFTYPES))
 		return -EINVAL;
 
@@ -2845,7 +2849,7 @@ int ieee80211_check_combinations(struct ieee80211_sub_if_data *sdata,
 			num_different_channels++;
 			continue;
 		}
-		if ((chanmode == IEEE80211_CHANCTX_SHARED) &&
+		if (chandef && chanmode == IEEE80211_CHANCTX_SHARED &&
 		    cfg80211_chandef_compatible(chandef,
 						&ctx->conf.def))
 			continue;
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 276cf93..7fca03e 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -396,10 +396,7 @@ static int wiphy_verify_combinations(struct wiphy *wiphy)
 		for (j = 0; j < c->n_limits; j++) {
 			u16 types = c->limits[j].types;
 
-			/*
-			 * interface types shouldn't overlap, this is
-			 * used in cfg80211_can_change_interface()
-			 */
+			/* interface types shouldn't overlap */
 			if (WARN_ON(types & all_iftypes))
 				return -EINVAL;
 			all_iftypes |= types;
@@ -798,7 +795,6 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct cfg80211_registered_device *rdev;
-	int ret;
 
 	if (!wdev)
 		return NOTIFY_DONE;
@@ -961,9 +957,8 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 	case NETDEV_PRE_UP:
 		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
 			return notifier_from_errno(-EOPNOTSUPP);
-		ret = cfg80211_can_add_interface(rdev, wdev->iftype);
-		if (ret)
-			return notifier_from_errno(ret);
+		if (rfkill_blocked(rdev->rfkill))
+			return notifier_from_errno(-ERFKILL);
 		break;
 	}
 
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 4a930ef..53f1996 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -406,28 +406,6 @@ unsigned int
 cfg80211_chandef_dfs_cac_time(struct wiphy *wiphy,
 			      const struct cfg80211_chan_def *chandef);
 
-static inline int
-cfg80211_can_change_interface(struct cfg80211_registered_device *rdev,
-			      struct wireless_dev *wdev,
-			      enum nl80211_iftype iftype)
-{
-	/* TODO: For this function, we'll probably need to keep some
-	 * kind of interface combination check in cfg80211...
-	 */
-	return cfg80211_can_use_iftype_chan(rdev, wdev, iftype, NULL,
-					    CHAN_MODE_UNDEFINED, 0);
-}
-
-static inline int
-cfg80211_can_add_interface(struct cfg80211_registered_device *rdev,
-			   enum nl80211_iftype iftype)
-{
-	if (rfkill_blocked(rdev->rfkill))
-		return -ERFKILL;
-
-	return cfg80211_can_change_interface(rdev, NULL, iftype);
-}
-
 static inline unsigned int elapsed_jiffies_msecs(unsigned long start)
 {
 	unsigned long end = jiffies;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 07253bf..5465e0e 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -8957,9 +8957,8 @@ static int nl80211_start_p2p_device(struct sk_buff *skb, struct genl_info *info)
 	if (wdev->p2p_started)
 		return 0;
 
-	err = cfg80211_can_add_interface(rdev, wdev->iftype);
-	if (err)
-		return err;
+	if (rfkill_blocked(rdev->rfkill))
+		return -ERFKILL;
 
 	err = rdev_start_p2p_device(rdev, wdev);
 	if (err)
diff --git a/net/wireless/util.c b/net/wireless/util.c
index 1069a24..b9deec2 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -873,11 +873,6 @@ int cfg80211_change_iface(struct cfg80211_registered_device *rdev,
 		return -EBUSY;
 
 	if (ntype != otype && netif_running(dev)) {
-		err = cfg80211_can_change_interface(rdev, dev->ieee80211_ptr,
-						    ntype);
-		if (err)
-			return err;
-
 		dev->ieee80211_ptr->use_4addr = false;
 		dev->ieee80211_ptr->mesh_id_up_len = 0;
 		wdev_lock(dev->ieee80211_ptr);
-- 
1.9.0

Re: [PATCH v10 0/5] cfg80211/mac80211: move interface combinations check to mac80211

[Luca Coelho]

On Tue, 2014-03-11 at 16:16 +0200, Luciano Coelho wrote:
> Hi,
> 
> The history is getting too big, so drop it... *sigh*
> 
> Adressed some comment by Eliad (thanks!)
> 
> In v10:
> 
>    * ignore software iftypes when checking used_iftypes;
>    * ignore NL80211_IFTYPE_UNSPECIFIED instead of WARNing in
>      cfg80211_dfs_required();
> 
> Luca.
> 
> Luciano Coelho (5):
>   cfg80211: refactor cfg80211_can_use_iftype_chan()
>   cfg80211/mac80211: refactor cfg80211_chandef_dfs_required()
>   cfg80211/mac80211: move interface counting for combination check to
>     mac80211
>   cfg80211/mac80211: move combination check to mac80211 for ibss
>   cfg80211/mac80211: move more combination checks to mac80211

I have applied this series to my mac80211-next-csa.git [1] tree.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/luca/mac80211-next-csa.git

--
Luca.