diff mbox series

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

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

Commit Message

Rameshkumar Sundaram Dec. 3, 2021, 4:41 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>
---
 include/linux/ieee80211.h |  1 +
 net/mac80211/cfg.c        | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

Comments

Johannes Berg Dec. 3, 2021, 8:08 a.m. UTC | #1 
On Fri, 2021-12-03 at 10:11 +0530, Rameshkumar Sundaram wrote:
> 
> +++ b/net/mac80211/cfg.c
> @@ -995,6 +995,8 @@ 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;
> +	const u8 *cap;
> +	struct ieee80211_he_operation *he_oper = NULL;
>  	u32 changed = BSS_CHANGED_BEACON;
>  
>  	old = sdata_dereference(sdata->u.ap.beacon, sdata);
> @@ -1082,6 +1084,27 @@ static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
>  		changed |= BSS_CHANGED_FTM_RESPONDER;
>  	}
>  
> +	if (sdata->vif.bss_conf.he_support) {
> +		cap = cfg80211_find_ext_ie(WLAN_EID_EXT_HE_OPERATION,
> +					   params->tail, params->tail_len);
> +		if (cap && cap[1] >= sizeof(*he_oper) + 1)
> +			he_oper = (void *)(cap + 3);
> 

I'm not sure I like this mechanism - in ieee80211_start_ap() we
explicitly take it from the parameters given via nl80211, so it feels
the same should be true here. Why isn't it done that way?

(And if we decide it should be this way then you should be using a new
"const struct element *cap" instead of "const u8 *cap", with the better
helpers functions etc.)

johannes
Rameshkumar Sundaram Dec. 3, 2021, 11:42 a.m. UTC | #2 
> -----Original Message-----
> From: Johannes Berg <johannes@sipsolutions.net>
> Sent: Friday, December 3, 2021 1:39 PM
> To: Rameshkumar Sundaram (QUIC) <quic_ramess@quicinc.com>
> Cc: linux-wireless@vger.kernel.org; Lavanya Suresh
> <lavaks@codeaurora.org>
> Subject: Re: [PATCH] mac80211: disable BSS color collision detection in case
> of no free colors
> 
> On Fri, 2021-12-03 at 10:11 +0530, Rameshkumar Sundaram wrote:
> >
> > +++ b/net/mac80211/cfg.c
> > @@ -995,6 +995,8 @@ 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;
> > +	const u8 *cap;
> > +	struct ieee80211_he_operation *he_oper = NULL;
> >  	u32 changed = BSS_CHANGED_BEACON;
> >
> >  	old = sdata_dereference(sdata->u.ap.beacon, sdata); @@ -1082,6
> > +1084,27 @@ static int ieee80211_assign_beacon(struct
> ieee80211_sub_if_data *sdata,
> >  		changed |= BSS_CHANGED_FTM_RESPONDER;
> >  	}
> >
> > +	if (sdata->vif.bss_conf.he_support) {
> > +		cap = cfg80211_find_ext_ie(WLAN_EID_EXT_HE_OPERATION,
> > +					   params->tail, params->tail_len);
> > +		if (cap && cap[1] >= sizeof(*he_oper) + 1)
> > +			he_oper = (void *)(cap + 3);
> >
> 
> I'm not sure I like this mechanism - in ieee80211_start_ap() we explicitly take
> it from the parameters given via nl80211, so it feels the same should be true
> here. Why isn't it done that way?

This is because in this case only beacon will change and in nl80211_set_beacon()
we don’t parse NL80211_ATTR_HE_BSS_COLOR attribute or do nl80211_calculate_ap_params()
> 
> (And if we decide it should be this way then you should be using a new
> "const struct element *cap" instead of "const u8 *cap", with the better
> helpers functions etc.)
> 
Sure cfg80211_find_ext_ie() returns const u8 *, you want me to use cfg80211_find_ext_elem instead (i.e. like how nl80211_calculate_ap_params() does ?

> johannes
Johannes Berg Dec. 3, 2021, 11:48 a.m. UTC | #3 
On Fri, 2021-12-03 at 11:42 +0000, Rameshkumar Sundaram (QUIC) wrote:
> > > 
> > 
> > I'm not sure I like this mechanism - in ieee80211_start_ap() we
> > explicitly take
> > it from the parameters given via nl80211, so it feels the same
> > should be true
> > here. Why isn't it done that way?
> 
> This is because in this case only beacon will change and in
> nl80211_set_beacon()
> we don’t parse NL80211_ATTR_HE_BSS_COLOR attribute or do
> nl80211_calculate_ap_params()
> 

Yeah but we could change that? Why not?

johannes
kernel test robot Dec. 3, 2021, 1:37 p.m. UTC | #4 
Hi Rameshkumar,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on jberg-mac80211-next/master]
[also build test WARNING on jberg-mac80211/master v5.16-rc3 next-20211203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Rameshkumar-Sundaram/mac80211-disable-BSS-color-collision-detection-in-case-of-no-free-colors/20211203-124303
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git master
config: i386-randconfig-s001-20211203 (https://download.01.org/0day-ci/archive/20211203/202112032136.n3Acqu0R-lkp@intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.4-dirty
        # https://github.com/0day-ci/linux/commit/318efe87673a29286f893ea96b07869d9dce83bc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Rameshkumar-Sundaram/mac80211-disable-BSS-color-collision-detection-in-case-of-no-free-colors/20211203-124303
        git checkout 318efe87673a29286f893ea96b07869d9dce83bc
        # save the config file to linux build tree
        mkdir build_dir
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash net/mac80211/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)
>> net/mac80211/cfg.c:1096:36: sparse: sparse: restricted __le32 degrades to integer

vim +1096 net/mac80211/cfg.c

   991	
   992	static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
   993					   struct cfg80211_beacon_data *params,
   994					   const struct ieee80211_csa_settings *csa,
   995					   const struct ieee80211_color_change_settings *cca)
   996	{
   997		struct beacon_data *new, *old;
   998		int new_head_len, new_tail_len;
   999		int size, err;
  1000		const u8 *cap;
  1001		struct ieee80211_he_operation *he_oper = NULL;
  1002		u32 changed = BSS_CHANGED_BEACON;
  1003	
  1004		old = sdata_dereference(sdata->u.ap.beacon, sdata);
  1005	
  1006	
  1007		/* Need to have a beacon head if we don't have one yet */
  1008		if (!params->head && !old)
  1009			return -EINVAL;
  1010	
  1011		/* new or old head? */
  1012		if (params->head)
  1013			new_head_len = params->head_len;
  1014		else
  1015			new_head_len = old->head_len;
  1016	
  1017		/* new or old tail? */
  1018		if (params->tail || !old)
  1019			/* params->tail_len will be zero for !params->tail */
  1020			new_tail_len = params->tail_len;
  1021		else
  1022			new_tail_len = old->tail_len;
  1023	
  1024		size = sizeof(*new) + new_head_len + new_tail_len;
  1025	
  1026		new = kzalloc(size, GFP_KERNEL);
  1027		if (!new)
  1028			return -ENOMEM;
  1029	
  1030		/* start filling the new info now */
  1031	
  1032		/*
  1033		 * pointers go into the block we allocated,
  1034		 * memory is | beacon_data | head | tail |
  1035		 */
  1036		new->head = ((u8 *) new) + sizeof(*new);
  1037		new->tail = new->head + new_head_len;
  1038		new->head_len = new_head_len;
  1039		new->tail_len = new_tail_len;
  1040	
  1041		if (csa) {
  1042			new->cntdwn_current_counter = csa->count;
  1043			memcpy(new->cntdwn_counter_offsets, csa->counter_offsets_beacon,
  1044			       csa->n_counter_offsets_beacon *
  1045			       sizeof(new->cntdwn_counter_offsets[0]));
  1046		} else if (cca) {
  1047			new->cntdwn_current_counter = cca->count;
  1048			new->cntdwn_counter_offsets[0] = cca->counter_offset_beacon;
  1049		}
  1050	
  1051		/* copy in head */
  1052		if (params->head)
  1053			memcpy(new->head, params->head, new_head_len);
  1054		else
  1055			memcpy(new->head, old->head, new_head_len);
  1056	
  1057		/* copy in optional tail */
  1058		if (params->tail)
  1059			memcpy(new->tail, params->tail, new_tail_len);
  1060		else
  1061			if (old)
  1062				memcpy(new->tail, old->tail, new_tail_len);
  1063	
  1064		err = ieee80211_set_probe_resp(sdata, params->probe_resp,
  1065					       params->probe_resp_len, csa, cca);
  1066		if (err < 0) {
  1067			kfree(new);
  1068			return err;
  1069		}
  1070		if (err == 0)
  1071			changed |= BSS_CHANGED_AP_PROBE_RESP;
  1072	
  1073		if (params->ftm_responder != -1) {
  1074			sdata->vif.bss_conf.ftm_responder = params->ftm_responder;
  1075			err = ieee80211_set_ftm_responder_params(sdata,
  1076								 params->lci,
  1077								 params->lci_len,
  1078								 params->civicloc,
  1079								 params->civicloc_len);
  1080	
  1081			if (err < 0) {
  1082				kfree(new);
  1083				return err;
  1084			}
  1085	
  1086			changed |= BSS_CHANGED_FTM_RESPONDER;
  1087		}
  1088	
  1089		if (sdata->vif.bss_conf.he_support) {
  1090			cap = cfg80211_find_ext_ie(WLAN_EID_EXT_HE_OPERATION,
  1091						   params->tail, params->tail_len);
  1092			if (cap && cap[1] >= sizeof(*he_oper) + 1)
  1093				he_oper = (void *)(cap + 3);
  1094	
  1095			if (he_oper) {
> 1096				if (he_oper->he_oper_params & HE_OPERATION_BSS_COLOR_DISABLED) {
  1097					if (sdata->vif.bss_conf.he_bss_color.enabled) {
  1098						sdata->vif.bss_conf.he_bss_color.enabled = false;
  1099						changed |= BSS_CHANGED_HE_BSS_COLOR;
  1100					}
  1101				} else {
  1102					if (!sdata->vif.bss_conf.he_bss_color.enabled) {
  1103						sdata->vif.bss_conf.he_bss_color.enabled = true;
  1104						changed |= BSS_CHANGED_HE_BSS_COLOR;
  1105					}
  1106				}
  1107			}
  1108		}
  1109	
  1110		rcu_assign_pointer(sdata->u.ap.beacon, new);
  1111	
  1112		if (old)
  1113			kfree_rcu(old, rcu_head);
  1114	
  1115		return changed;
  1116	}
  1117	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
diff mbox series

Patch

diff --git a/include/linux/ieee80211.h b/include/linux/ieee80211.h
index 11d7af2..cc629d7 100644
--- a/include/linux/ieee80211.h
+++ b/include/linux/ieee80211.h
@@ -1874,6 +1874,7 @@  struct ieee80211_he_mcs_nss_supp {
 	__le16 tx_mcs_80p80;
 } __packed;
 
+#define HE_OPERATION_BSS_COLOR_DISABLED		((u32)BIT(31))
 /**
  * struct ieee80211_he_operation - HE capabilities element
  *
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index f36f249..eaa04b7 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -995,6 +995,8 @@  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;
+	const u8 *cap;
+	struct ieee80211_he_operation *he_oper = NULL;
 	u32 changed = BSS_CHANGED_BEACON;
 
 	old = sdata_dereference(sdata->u.ap.beacon, sdata);
@@ -1082,6 +1084,27 @@  static int ieee80211_assign_beacon(struct ieee80211_sub_if_data *sdata,
 		changed |= BSS_CHANGED_FTM_RESPONDER;
 	}
 
+	if (sdata->vif.bss_conf.he_support) {
+		cap = cfg80211_find_ext_ie(WLAN_EID_EXT_HE_OPERATION,
+					   params->tail, params->tail_len);
+		if (cap && cap[1] >= sizeof(*he_oper) + 1)
+			he_oper = (void *)(cap + 3);
+
+		if (he_oper) {
+			if (he_oper->he_oper_params & HE_OPERATION_BSS_COLOR_DISABLED) {
+				if (sdata->vif.bss_conf.he_bss_color.enabled) {
+					sdata->vif.bss_conf.he_bss_color.enabled = false;
+					changed |= BSS_CHANGED_HE_BSS_COLOR;
+				}
+			} else {
+				if (!sdata->vif.bss_conf.he_bss_color.enabled) {
+					sdata->vif.bss_conf.he_bss_color.enabled = true;
+					changed |= BSS_CHANGED_HE_BSS_COLOR;
+				}
+			}
+		}
+	}
+
 	rcu_assign_pointer(sdata->u.ap.beacon, new);
 
 	if (old)