diff mbox series

[v4] mac80211: disable BSS color collision detection in case of no free colors

Message ID 1648195961-3811-1-git-send-email-quic_ramess@quicinc.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [v4] mac80211: disable BSS color collision detection in case of no free colors | expand

Commit Message

Rameshkumar Sundaram March 25, 2022, 8:12 a.m. UTC 
From: Lavanya Suresh <lavaks@codeaurora.org>

AP may run out of BSS color after color collision
detection event from driver.
Disable BSS color collision detection if no free colors are
available based on bss color disabled bit sent as a part of
NL80211_ATTR_HE_BSS_COLOR attribute sent in
NL80211_CMD_SET_BEACON.

It can be reenabled once new color is available.

Signed-off-by: Lavanya Suresh <lavaks@codeaurora.org>
Signed-off-by: Rameshkumar Sundaram <quic_ramess@quicinc.com>
---
Changes since v3:
 - Updated subject

Changes since v2:
 - Moved the logic to ieee80211_change_beacon() to have this
   BSS color enabled unset only during set_beacon case based
   on NL80211_ATTR_HE_BSS_COLOR attribute sent by hostapd.
 - Removed the relocation and parsing changes done on v1 and moved
   he_bss_color to cfg80211_beacon_data and parsed
   NL80211_ATTR_HE_BSS_COLOR in nl80211_parse_beacon()

Changes since v1:
 - Relocated ap params to cfg80211_beacon_data and
   nl80211_calculate_ap_params() to nl80211_parse_beacon()
   to have them parsed for all (start_ap and change beacon) commands.
---
 include/net/cfg80211.h |  4 ++--
 net/mac80211/cfg.c     | 11 +++++++--
 net/wireless/nl80211.c | 62 ++++++++++++++++++++++++--------------------------
 3 files changed, 41 insertions(+), 36 deletions(-)

Comments

Johannes Berg April 11, 2022, 1:31 p.m. UTC | #1 
On Fri, 2022-03-25 at 13:42 +0530, Rameshkumar Sundaram wrote:


> +	he_bss_color->color =
> +		nla_get_u8(tb[NL80211_HE_BSS_COLOR_ATTR_COLOR]);
> +	he_bss_color->enabled =
> +		!nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_DISABLED]);
> +	he_bss_color->partial =
> +		nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_PARTIAL]);
> +
> +	return 0;
> +}
> +
>  static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
>  				struct nlattr *attrs[],
>  				struct cfg80211_beacon_data *bcn)
> @@ -5204,6 +5228,12 @@ static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
>  		bcn->ftm_responder = -1;
>  	}
>  
> +	if (attrs[NL80211_ATTR_HE_BSS_COLOR]) {
> +		err = nl80211_parse_he_bss_color(attrs[NL80211_ATTR_HE_BSS_COLOR],
> +						 &bcn->he_bss_color);
> +		if (err)
> +			return err;
> +	}

Could use a newline after this.

But not sure I get it - doesn't that always disable BSS coloring now if
you do a beacon update that didn't include the attribute? Did hostapd
always include it in the past?

johannes
Johannes Berg April 11, 2022, 1:33 p.m. UTC | #2 
On Mon, 2022-04-11 at 15:31 +0200, Johannes Berg wrote:
> On Fri, 2022-03-25 at 13:42 +0530, Rameshkumar Sundaram wrote:
> 
> 
> > +	he_bss_color->color =
> > +		nla_get_u8(tb[NL80211_HE_BSS_COLOR_ATTR_COLOR]);
> > +	he_bss_color->enabled =
> > +		!nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_DISABLED]);
> > +	he_bss_color->partial =
> > +		nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_PARTIAL]);
> > +
> > +	return 0;
> > +}
> > +
> >  static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
> >  				struct nlattr *attrs[],
> >  				struct cfg80211_beacon_data *bcn)
> > @@ -5204,6 +5228,12 @@ static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
> >  		bcn->ftm_responder = -1;
> >  	}
> >  
> > +	if (attrs[NL80211_ATTR_HE_BSS_COLOR]) {
> > +		err = nl80211_parse_he_bss_color(attrs[NL80211_ATTR_HE_BSS_COLOR],
> > +						 &bcn->he_bss_color);
> > +		if (err)
> > +			return err;
> > +	}
> 
> Could use a newline after this.
> 
> But not sure I get it - doesn't that always disable BSS coloring now if
> you do a beacon update that didn't include the attribute? Did hostapd
> always include it in the past?
> 

Seems like we should track whether it was actually included, and only if
the attribute was included then mac80211 knows - ie. have something like

 struct cfg80211_beacon_data {
  ...
+	struct cfg80211_he_bss_color he_bss_color;
+	bool he_bss_color_valid;

or so?
Johannes Berg April 11, 2022, 1:34 p.m. UTC | #3 
>  include/net/cfg80211.h |  4 ++--
>  net/mac80211/cfg.c     | 11 +++++++--
>  net/wireless/nl80211.c | 62 ++++++++++++++++++++++++--------------------------
> 

Also, the mac80211 part is small, so maybe split the patch. Yes, the
mac80211 part is the key part that actually does something, but the
changes in the API could use a separate commit message explaining how
they work and how they are backward compatible.

johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index a887086..08b795d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1113,6 +1113,7 @@  struct cfg80211_mbssid_elems {
  *	Token (measurement type 11)
  * @lci_len: LCI data length
  * @civicloc_len: Civic location data length
+ * @he_bss_color: BSS Color settings
  */
 struct cfg80211_beacon_data {
 	const u8 *head, *tail;
@@ -1132,6 +1133,7 @@  struct cfg80211_beacon_data {
 	size_t probe_resp_len;
 	size_t lci_len;
 	size_t civicloc_len;
+	struct cfg80211_he_bss_color he_bss_color;
 };
 
 struct mac_address {
@@ -1233,7 +1235,6 @@  enum cfg80211_ap_settings_flags {
  * @sae_h2e_required: stations must support direct H2E technique in SAE
  * @flags: flags, as defined in enum cfg80211_ap_settings_flags
  * @he_obss_pd: OBSS Packet Detection settings
- * @he_bss_color: BSS Color settings
  * @he_oper: HE operation IE (or %NULL if HE isn't enabled)
  * @fils_discovery: FILS discovery transmission parameters
  * @unsol_bcast_probe_resp: Unsolicited broadcast probe response parameters
@@ -1267,7 +1268,6 @@  struct cfg80211_ap_settings {
 	bool twt_responder;
 	u32 flags;
 	struct ieee80211_he_obss_pd he_obss_pd;
-	struct cfg80211_he_bss_color he_bss_color;
 	struct cfg80211_fils_discovery fils_discovery;
 	struct cfg80211_unsol_bcast_probe_resp unsol_bcast_probe_resp;
 	struct cfg80211_mbssid_config mbssid_config;
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 26cc762..f54aaf1 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1133,7 +1133,7 @@  static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 			      IEEE80211_HE_OPERATION_RTS_THRESHOLD_MASK);
 		changed |= BSS_CHANGED_HE_OBSS_PD;
 
-		if (params->he_bss_color.enabled)
+		if (params->beacon.he_bss_color.enabled)
 			changed |= BSS_CHANGED_HE_BSS_COLOR;
 	}
 
@@ -1190,7 +1190,7 @@  static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	sdata->vif.bss_conf.allow_p2p_go_ps = sdata->vif.p2p;
 	sdata->vif.bss_conf.twt_responder = params->twt_responder;
 	sdata->vif.bss_conf.he_obss_pd = params->he_obss_pd;
-	sdata->vif.bss_conf.he_bss_color = params->he_bss_color;
+	sdata->vif.bss_conf.he_bss_color = params->beacon.he_bss_color;
 	sdata->vif.bss_conf.s1g = params->chandef.chan->band ==
 				  NL80211_BAND_S1GHZ;
 
@@ -1272,6 +1272,7 @@  static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 				   struct cfg80211_beacon_data *params)
 {
 	struct ieee80211_sub_if_data *sdata;
+	struct ieee80211_bss_conf *bss_conf;
 	struct beacon_data *old;
 	int err;
 
@@ -1291,6 +1292,12 @@  static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 	err = ieee80211_assign_beacon(sdata, params, NULL, NULL);
 	if (err < 0)
 		return err;
+
+	bss_conf = &sdata->vif.bss_conf;
+	if (params->he_bss_color.enabled != bss_conf->he_bss_color.enabled) {
+		bss_conf->he_bss_color.enabled = params->he_bss_color.enabled;
+		err |= BSS_CHANGED_HE_BSS_COLOR;
+	}
 	ieee80211_bss_info_change_notify(sdata, err);
 	return 0;
 }
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bfa5d74..df609e4 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5124,6 +5124,30 @@  nl80211_parse_mbssid_elems(struct wiphy *wiphy, struct nlattr *attrs)
 	return elems;
 }
 
+static int nl80211_parse_he_bss_color(struct nlattr *attrs,
+				      struct cfg80211_he_bss_color *he_bss_color)
+{
+	struct nlattr *tb[NL80211_HE_BSS_COLOR_ATTR_MAX + 1];
+	int err;
+
+	err = nla_parse_nested(tb, NL80211_HE_BSS_COLOR_ATTR_MAX, attrs,
+			       he_bss_color_policy, NULL);
+	if (err)
+		return err;
+
+	if (!tb[NL80211_HE_BSS_COLOR_ATTR_COLOR])
+		return -EINVAL;
+
+	he_bss_color->color =
+		nla_get_u8(tb[NL80211_HE_BSS_COLOR_ATTR_COLOR]);
+	he_bss_color->enabled =
+		!nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_DISABLED]);
+	he_bss_color->partial =
+		nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_PARTIAL]);
+
+	return 0;
+}
+
 static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
 				struct nlattr *attrs[],
 				struct cfg80211_beacon_data *bcn)
@@ -5204,6 +5228,12 @@  static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
 		bcn->ftm_responder = -1;
 	}
 
+	if (attrs[NL80211_ATTR_HE_BSS_COLOR]) {
+		err = nl80211_parse_he_bss_color(attrs[NL80211_ATTR_HE_BSS_COLOR],
+						 &bcn->he_bss_color);
+		if (err)
+			return err;
+	}
 	if (attrs[NL80211_ATTR_MBSSID_ELEMS]) {
 		struct cfg80211_mbssid_elems *mbssid =
 			nl80211_parse_mbssid_elems(&rdev->wiphy,
@@ -5262,30 +5292,6 @@  static int nl80211_parse_he_obss_pd(struct nlattr *attrs,
 	return 0;
 }
 
-static int nl80211_parse_he_bss_color(struct nlattr *attrs,
-				      struct cfg80211_he_bss_color *he_bss_color)
-{
-	struct nlattr *tb[NL80211_HE_BSS_COLOR_ATTR_MAX + 1];
-	int err;
-
-	err = nla_parse_nested(tb, NL80211_HE_BSS_COLOR_ATTR_MAX, attrs,
-			       he_bss_color_policy, NULL);
-	if (err)
-		return err;
-
-	if (!tb[NL80211_HE_BSS_COLOR_ATTR_COLOR])
-		return -EINVAL;
-
-	he_bss_color->color =
-		nla_get_u8(tb[NL80211_HE_BSS_COLOR_ATTR_COLOR]);
-	he_bss_color->enabled =
-		!nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_DISABLED]);
-	he_bss_color->partial =
-		nla_get_flag(tb[NL80211_HE_BSS_COLOR_ATTR_PARTIAL]);
-
-	return 0;
-}
-
 static int nl80211_parse_fils_discovery(struct cfg80211_registered_device *rdev,
 					struct nlattr *attrs,
 					struct cfg80211_ap_settings *params)
@@ -5677,14 +5683,6 @@  static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 			goto out;
 	}
 
-	if (info->attrs[NL80211_ATTR_HE_BSS_COLOR]) {
-		err = nl80211_parse_he_bss_color(
-					info->attrs[NL80211_ATTR_HE_BSS_COLOR],
-					&params->he_bss_color);
-		if (err)
-			goto out;
-	}
-
 	if (info->attrs[NL80211_ATTR_FILS_DISCOVERY]) {
 		err = nl80211_parse_fils_discovery(rdev,
 						   info->attrs[NL80211_ATTR_FILS_DISCOVERY],