diff mbox series

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

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

Commit Message

Rameshkumar Sundaram Dec. 12, 2021, 11:11 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 of he_oper_params in 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 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 |  28 +++++++------
 net/mac80211/cfg.c     |  16 ++++++--
 net/wireless/nl80211.c | 109 ++++++++++++++++++++++++-------------------------
 3 files changed, 82 insertions(+), 71 deletions(-)

Comments

Johannes Berg Dec. 20, 2021, 10:01 a.m. UTC | #1 
Hi,


>  include/net/cfg80211.h |  28 +++++++------
>  net/mac80211/cfg.c     |  16 ++++++--
>  net/wireless/nl80211.c | 109 ++++++++++++++++++++++++-------------------------
>  3 files changed, 82 insertions(+), 71 deletions(-)
> 

This is now a fairly big cfg80211 change, and not much mac80211. Can you
please split it?

But you didn't really address why we need to do this via element change
detection, rather than letting hostapd do this via the
NL80211_ATTR_HE_BSS_COLOR attribute even in change_beacon?

johannes
Rameshkumar Sundaram Dec. 27, 2021, 4:19 a.m. UTC | #2 
> -----Original Message-----
> From: Johannes Berg <johannes@sipsolutions.net>
> Sent: Monday, December 20, 2021 3:32 PM
> To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>
> Cc: linux-wireless@vger.kernel.org; Lavanya Suresh
> <lavaks@codeaurora.org>
> Subject: Re: [PATCH v2] mac80211: disable BSS color collision detection in
> case of no free colors
> 
> Hi,
> 
> 
> >  include/net/cfg80211.h |  28 +++++++------
> >  net/mac80211/cfg.c     |  16 ++++++--
> >  net/wireless/nl80211.c | 109
> > ++++++++++++++++++++++++-------------------------
> >  3 files changed, 82 insertions(+), 71 deletions(-)
> >
> 
> This is now a fairly big cfg80211 change, and not much mac80211. Can you
> please split it?
> 
> But you didn't really address why we need to do this via element change
> detection, rather than letting hostapd do this via the
> NL80211_ATTR_HE_BSS_COLOR attribute even in change_beacon?
> 

Ah! I totally missed this attribute parsing option and went with ap_params from
last discussion.

Yes, hostapd would send NL80211_ATTR_HE_BSS_COLOR for change_beacon too,
maybe we should relocate cfg80211_he_bss_color to beacon data 
and do nl80211_parse_he_bss_color() in nl80211_parse_beacon() in that case to
have this data for both commands.

> johannes
Rameshkumar Sundaram Jan. 3, 2022, 4:22 a.m. UTC | #3 
> -----Original Message-----
> From: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>
> Sent: Monday, December 27, 2021 9:50 AM
> To: Johannes Berg <johannes@sipsolutions.net>
> Cc: linux-wireless@vger.kernel.org; Rameshkumar Sundaram (QUIC)
> <quic_ramess@quicinc.com>; Lavanya Suresh <lavaks@codeaurora.org>
> Subject: RE: [PATCH v2] mac80211: disable BSS color collision detection in
> case of no free colors
> 
> 
> > -----Original Message-----
> > From: Johannes Berg <johannes@sipsolutions.net>
> > Sent: Monday, December 20, 2021 3:32 PM
> > To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>
> > Cc: linux-wireless@vger.kernel.org; Lavanya Suresh
> > <lavaks@codeaurora.org>
> > Subject: Re: [PATCH v2] mac80211: disable BSS color collision
> > detection in case of no free colors
> >
> > Hi,
> >
> >
> > >  include/net/cfg80211.h |  28 +++++++------
> > >  net/mac80211/cfg.c     |  16 ++++++--
> > >  net/wireless/nl80211.c | 109
> > > ++++++++++++++++++++++++-------------------------
> > >  3 files changed, 82 insertions(+), 71 deletions(-)
> > >
> >
> > This is now a fairly big cfg80211 change, and not much mac80211. Can
> > you please split it?
> >
> > But you didn't really address why we need to do this via element
> > change detection, rather than letting hostapd do this via the
> > NL80211_ATTR_HE_BSS_COLOR attribute even in change_beacon?
> >
> 
> Ah! I totally missed this attribute parsing option and went with ap_params
> from last discussion.
> 
> Yes, hostapd would send NL80211_ATTR_HE_BSS_COLOR for change_beacon
> too, maybe we should relocate cfg80211_he_bss_color to beacon data and
> do nl80211_parse_he_bss_color() in nl80211_parse_beacon() in that case to
> have this data for both commands.
> 
But channel_switch and color_change commands won't have this attribute set and
We may disable color there.
We will have to have a flag and set if NL80211_ATTR_HE_BSS_COLOR is present in the 
NL command and check that flag in assign_beacon() before using params->he_bss_color
struct data to prevent that then.

So, can we go with this IE checking itself or we can do above change, please suggest.
> > johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index a887086..fc17c9d 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1113,7 +1113,16 @@  struct cfg80211_mbssid_elems {
  *	Token (measurement type 11)
  * @lci_len: LCI data length
  * @civicloc_len: Civic location data length
+ * @ht_cap: HT capabilities (or %NULL if HT isn't enabled)
+ * @vht_cap: VHT capabilities (or %NULL if VHT isn't enabled)
+ * @he_cap: HE capabilities (or %NULL if HE isn't enabled)
+ * @he_oper: HE operation IE (or %NULL if HE isn't enabled)
+ * @ht_required: stations must support HT
+ * @vht_required: stations must support VHT
+ * @he_required: stations must support HE
+ * @sae_h2e_required: stations must support direct H2E technique in SAE
  */
+
 struct cfg80211_beacon_data {
 	const u8 *head, *tail;
 	const u8 *beacon_ies;
@@ -1132,6 +1141,12 @@  struct cfg80211_beacon_data {
 	size_t probe_resp_len;
 	size_t lci_len;
 	size_t civicloc_len;
+
+	const struct ieee80211_ht_cap *ht_cap;
+	const struct ieee80211_vht_cap *vht_cap;
+	const struct ieee80211_he_cap_elem *he_cap;
+	const struct ieee80211_he_operation *he_oper;
+	bool ht_required, vht_required, he_required, sae_h2e_required;
 };
 
 struct mac_address {
@@ -1223,18 +1238,10 @@  enum cfg80211_ap_settings_flags {
  * @pbss: If set, start as a PCP instead of AP. Relevant for DMG
  *	networks.
  * @beacon_rate: bitrate to be used for beacons
- * @ht_cap: HT capabilities (or %NULL if HT isn't enabled)
- * @vht_cap: VHT capabilities (or %NULL if VHT isn't enabled)
- * @he_cap: HE capabilities (or %NULL if HE isn't enabled)
- * @ht_required: stations must support HT
- * @vht_required: stations must support VHT
  * @twt_responder: Enable Target Wait Time
- * @he_required: stations must support HE
- * @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
  * @mbssid_config: AP settings for multiple bssid
@@ -1259,11 +1266,6 @@  struct cfg80211_ap_settings {
 	bool pbss;
 	struct cfg80211_bitrate_mask beacon_rate;
 
-	const struct ieee80211_ht_cap *ht_cap;
-	const struct ieee80211_vht_cap *vht_cap;
-	const struct ieee80211_he_cap_elem *he_cap;
-	const struct ieee80211_he_operation *he_oper;
-	bool ht_required, vht_required, he_required, sae_h2e_required;
 	bool twt_responder;
 	u32 flags;
 	struct ieee80211_he_obss_pd he_obss_pd;
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 26cc762..bc99be0 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -997,6 +997,7 @@  static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 	struct beacon_data *new, *old;
 	int new_head_len, new_tail_len;
 	int size, err;
+	bool bss_clr_enabled;
 	u32 changed = BSS_CHANGED_BEACON;
 
 	old = sdata_dereference(sdata->u.ap.beacon, sdata);
@@ -1084,6 +1085,15 @@  static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 		changed |= BSS_CHANGED_FTM_RESPONDER;
 	}
 
+	if (params->he_cap && params->he_oper) {
+		bss_clr_enabled = !le32_get_bits(params->he_oper->he_oper_params,
+						 IEEE80211_HE_OPERATION_BSS_COLOR_DISABLED);
+		if (sdata->vif.bss_conf.he_bss_color.enabled != bss_clr_enabled) {
+			sdata->vif.bss_conf.he_bss_color.enabled = bss_clr_enabled;
+			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		}
+	}
+
 	rcu_assign_pointer(sdata->u.ap.beacon, new);
 
 	if (old)
@@ -1123,13 +1133,13 @@  static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev,
 	prev_beacon_int = sdata->vif.bss_conf.beacon_int;
 	sdata->vif.bss_conf.beacon_int = params->beacon_interval;
 
-	if (params->he_cap && params->he_oper) {
+	if (params->beacon.he_cap && params->beacon.he_oper) {
 		sdata->vif.bss_conf.he_support = true;
 		sdata->vif.bss_conf.htc_trig_based_pkt_ext =
-			le32_get_bits(params->he_oper->he_oper_params,
+			le32_get_bits(params->beacon.he_oper->he_oper_params,
 			      IEEE80211_HE_OPERATION_DFLT_PE_DURATION_MASK);
 		sdata->vif.bss_conf.frame_time_rts_th =
-			le32_get_bits(params->he_oper->he_oper_params,
+			le32_get_bits(params->beacon.he_oper->he_oper_params,
 			      IEEE80211_HE_OPERATION_RTS_THRESHOLD_MASK);
 		changed |= BSS_CHANGED_HE_OBSS_PD;
 
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bfa5d74..3881219 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -5124,6 +5124,58 @@  nl80211_parse_mbssid_elems(struct wiphy *wiphy, struct nlattr *attrs)
 	return elems;
 }
 
+static void nl80211_check_ap_rate_selectors(struct cfg80211_beacon_data *bcn,
+					    const struct element *rates)
+{
+	int i;
+
+	if (!rates)
+		return;
+
+	for (i = 0; i < rates->datalen; i++) {
+		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_HT_PHY)
+			bcn->ht_required = true;
+		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_VHT_PHY)
+			bcn->vht_required = true;
+		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_HE_PHY)
+			bcn->he_required = true;
+		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_SAE_H2E)
+			bcn->sae_h2e_required = true;
+	}
+}
+
+/*
+ * Since the nl80211 API didn't include, from the beginning, attributes about
+ * HT/VHT requirements/capabilities, we parse them out of the IEs for the
+ * benefit of drivers that rebuild IEs in the firmware.
+ */
+static void nl80211_calculate_ap_params(struct cfg80211_beacon_data *bcn)
+{
+	size_t ies_len = bcn->tail_len;
+	const u8 *ies = bcn->tail;
+	const struct element *rates;
+	const struct element *cap;
+
+	rates = cfg80211_find_elem(WLAN_EID_SUPP_RATES, ies, ies_len);
+	nl80211_check_ap_rate_selectors(bcn, rates);
+
+	rates = cfg80211_find_elem(WLAN_EID_EXT_SUPP_RATES, ies, ies_len);
+	nl80211_check_ap_rate_selectors(bcn, rates);
+
+	cap = cfg80211_find_elem(WLAN_EID_HT_CAPABILITY, ies, ies_len);
+	if (cap && cap->datalen >= sizeof(*bcn->ht_cap))
+		bcn->ht_cap = (void *)cap->data;
+	cap = cfg80211_find_elem(WLAN_EID_VHT_CAPABILITY, ies, ies_len);
+	if (cap && cap->datalen >= sizeof(*bcn->vht_cap))
+		bcn->vht_cap = (void *)cap->data;
+	cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_CAPABILITY, ies, ies_len);
+	if (cap && cap->datalen >= sizeof(*bcn->he_cap) + 1)
+		bcn->he_cap = (void *)(cap->data + 1);
+	cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_OPERATION, ies, ies_len);
+	if (cap && cap->datalen >= sizeof(*bcn->he_oper) + 1)
+		bcn->he_oper = (void *)(cap->data + 1);
+}
+
 static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
 				struct nlattr *attrs[],
 				struct cfg80211_beacon_data *bcn)
@@ -5215,6 +5267,8 @@  static int nl80211_parse_beacon(struct cfg80211_registered_device *rdev,
 		bcn->mbssid_ies = mbssid;
 	}
 
+	nl80211_calculate_ap_params(bcn);
+
 	return 0;
 }
 
@@ -5345,59 +5399,6 @@  nl80211_parse_unsol_bcast_probe_resp(struct cfg80211_registered_device *rdev,
 	return 0;
 }
 
-static void nl80211_check_ap_rate_selectors(struct cfg80211_ap_settings *params,
-					    const struct element *rates)
-{
-	int i;
-
-	if (!rates)
-		return;
-
-	for (i = 0; i < rates->datalen; i++) {
-		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_HT_PHY)
-			params->ht_required = true;
-		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_VHT_PHY)
-			params->vht_required = true;
-		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_HE_PHY)
-			params->he_required = true;
-		if (rates->data[i] == BSS_MEMBERSHIP_SELECTOR_SAE_H2E)
-			params->sae_h2e_required = true;
-	}
-}
-
-/*
- * Since the nl80211 API didn't include, from the beginning, attributes about
- * HT/VHT requirements/capabilities, we parse them out of the IEs for the
- * benefit of drivers that rebuild IEs in the firmware.
- */
-static void nl80211_calculate_ap_params(struct cfg80211_ap_settings *params)
-{
-	const struct cfg80211_beacon_data *bcn = &params->beacon;
-	size_t ies_len = bcn->tail_len;
-	const u8 *ies = bcn->tail;
-	const struct element *rates;
-	const struct element *cap;
-
-	rates = cfg80211_find_elem(WLAN_EID_SUPP_RATES, ies, ies_len);
-	nl80211_check_ap_rate_selectors(params, rates);
-
-	rates = cfg80211_find_elem(WLAN_EID_EXT_SUPP_RATES, ies, ies_len);
-	nl80211_check_ap_rate_selectors(params, rates);
-
-	cap = cfg80211_find_elem(WLAN_EID_HT_CAPABILITY, ies, ies_len);
-	if (cap && cap->datalen >= sizeof(*params->ht_cap))
-		params->ht_cap = (void *)cap->data;
-	cap = cfg80211_find_elem(WLAN_EID_VHT_CAPABILITY, ies, ies_len);
-	if (cap && cap->datalen >= sizeof(*params->vht_cap))
-		params->vht_cap = (void *)cap->data;
-	cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_CAPABILITY, ies, ies_len);
-	if (cap && cap->datalen >= sizeof(*params->he_cap) + 1)
-		params->he_cap = (void *)(cap->data + 1);
-	cap = cfg80211_find_ext_elem(WLAN_EID_EXT_HE_OPERATION, ies, ies_len);
-	if (cap && cap->datalen >= sizeof(*params->he_oper) + 1)
-		params->he_oper = (void *)(cap->data + 1);
-}
-
 static bool nl80211_get_ap_channel(struct cfg80211_registered_device *rdev,
 				   struct cfg80211_ap_settings *params)
 {
@@ -5712,8 +5713,6 @@  static int nl80211_start_ap(struct sk_buff *skb, struct genl_info *info)
 			goto out;
 	}
 
-	nl80211_calculate_ap_params(params);
-
 	if (info->attrs[NL80211_ATTR_EXTERNAL_AUTH_SUPPORT])
 		params->flags |= AP_SETTINGS_EXTERNAL_AUTH_SUPPORT;