From: Lavanya Suresh <[email protected]>
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 <[email protected]>
Signed-off-by: Rameshkumar Sundaram <[email protected]>
---
include/linux/ieee80211.h | 1 +
net/mac80211/cfg.c | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+)
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)
--
2.7.4
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
> -----Original Message-----
> From: Johannes Berg <[email protected]>
> Sent: Friday, December 3, 2021 1:39 PM
> To: Rameshkumar Sundaram (QUIC) <[email protected]>
> Cc: [email protected]; Lavanya Suresh
> <[email protected]>
> 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
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
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/[email protected]/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 <[email protected]>
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/[email protected]