| Message ID | 20240307181039.3219840-1-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wifi: mac80211: Improve bss-color configuration. | expand |
On 3/7/2024 10:10 AM, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > Always tell driver to apply bss color settings if beacon indicates > the bss coloring has been set. why? please describe the problem you are fixing > > And only enable bss coloring if beacon indicates bss color setting > is valid and also enabled. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > net/mac80211/cfg.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > index 327682995c92..aaa62c05428c 100644 > --- a/net/mac80211/cfg.c > +++ b/net/mac80211/cfg.c > @@ -1314,7 +1314,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->beacon.he_bss_color.enabled) > + if (params->beacon.he_bss_color_valid) > changed |= BSS_CHANGED_HE_BSS_COLOR; > } > > @@ -1494,6 +1494,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, > int err; > struct ieee80211_bss_conf *link_conf; > u64 changed = 0; > + bool color_en; > > lockdep_assert_wiphy(wiphy); > > @@ -1530,9 +1531,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, > if (err < 0) > return err; > > - if (beacon->he_bss_color_valid && > - beacon->he_bss_color.enabled != link_conf->he_bss_color.enabled) { > - link_conf->he_bss_color.enabled = beacon->he_bss_color.enabled; > + color_en = beacon->he_bss_color.enabled && beacon->he_bss_color_valid; > + if (color_en != link_conf->he_bss_color.enabled) { > + link_conf->he_bss_color.enabled = color_en; > changed |= BSS_CHANGED_HE_BSS_COLOR; > } >
On 3/8/24 09:40, Jeff Johnson wrote: > On 3/7/2024 10:10 AM, greearb@candelatech.com wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> Always tell driver to apply bss color settings if beacon indicates >> the bss coloring has been set. > > why? please describe the problem you are fixing To ensure driver is set to known state. This does not fix any known bug, but seems appropriate, and I noticed driver wasn't being set to disabled when BSS color was disabled while debugging some other issues (the driver in question defaulted to disabled). >> And only enable bss coloring if beacon indicates bss color setting >> is valid and also enabled. This seems more correct behaviour to me. It does not fix any known problem. Thanks, Ben >> >> Signed-off-by: Ben Greear <greearb@candelatech.com> >> --- >> net/mac80211/cfg.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c >> index 327682995c92..aaa62c05428c 100644 >> --- a/net/mac80211/cfg.c >> +++ b/net/mac80211/cfg.c >> @@ -1314,7 +1314,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->beacon.he_bss_color.enabled) >> + if (params->beacon.he_bss_color_valid) >> changed |= BSS_CHANGED_HE_BSS_COLOR; >> } >> >> @@ -1494,6 +1494,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, >> int err; >> struct ieee80211_bss_conf *link_conf; >> u64 changed = 0; >> + bool color_en; >> >> lockdep_assert_wiphy(wiphy); >> >> @@ -1530,9 +1531,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, >> if (err < 0) >> return err; >> >> - if (beacon->he_bss_color_valid && >> - beacon->he_bss_color.enabled != link_conf->he_bss_color.enabled) { >> - link_conf->he_bss_color.enabled = beacon->he_bss_color.enabled; >> + color_en = beacon->he_bss_color.enabled && beacon->he_bss_color_valid; >> + if (color_en != link_conf->he_bss_color.enabled) { >> + link_conf->he_bss_color.enabled = color_en; >> changed |= BSS_CHANGED_HE_BSS_COLOR; >> } >> >
> From: Ben Greear <greearb@candelatech.com> > > Always tell driver to apply bss color settings if beacon indicates > the bss coloring has been set. > > And only enable bss coloring if beacon indicates bss color setting > is valid and also enabled. > > Signed-off-by: Ben Greear <greearb@candelatech.com> > --- > net/mac80211/cfg.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c > index 327682995c92..aaa62c05428c 100644 > --- a/net/mac80211/cfg.c > +++ b/net/mac80211/cfg.c > @@ -1314,7 +1314,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->beacon.he_bss_color.enabled) > + if (params->beacon.he_bss_color_valid) > changed |= BSS_CHANGED_HE_BSS_COLOR; This change seems correct to me since he_bss_color_valid is set to true when userspace (e.g. hostapd) provides the bss color info. > } > > @@ -1494,6 +1494,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, > int err; > struct ieee80211_bss_conf *link_conf; > u64 changed = 0; > + bool color_en; > > lockdep_assert_wiphy(wiphy); > > @@ -1530,9 +1531,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, > if (err < 0) > return err; > > - if (beacon->he_bss_color_valid && > - beacon->he_bss_color.enabled != link_conf->he_bss_color.enabled) { > - link_conf->he_bss_color.enabled = beacon->he_bss_color.enabled; > + color_en = beacon->he_bss_color.enabled && beacon->he_bss_color_valid; > + if (color_en != link_conf->he_bss_color.enabled) { > + link_conf->he_bss_color.enabled = color_en; > changed |= BSS_CHANGED_HE_BSS_COLOR; > } Here both approaches seem fine to me (but I do not have a strong opinion). The one proposed by Ben disables by default bss coloring if color is not valid. Regards, Lorenzo > > -- > 2.42.0 > >
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 327682995c92..aaa62c05428c 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -1314,7 +1314,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->beacon.he_bss_color.enabled) + if (params->beacon.he_bss_color_valid) changed |= BSS_CHANGED_HE_BSS_COLOR; } @@ -1494,6 +1494,7 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, int err; struct ieee80211_bss_conf *link_conf; u64 changed = 0; + bool color_en; lockdep_assert_wiphy(wiphy); @@ -1530,9 +1531,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, if (err < 0) return err; - if (beacon->he_bss_color_valid && - beacon->he_bss_color.enabled != link_conf->he_bss_color.enabled) { - link_conf->he_bss_color.enabled = beacon->he_bss_color.enabled; + color_en = beacon->he_bss_color.enabled && beacon->he_bss_color_valid; + if (color_en != link_conf->he_bss_color.enabled) { + link_conf->he_bss_color.enabled = color_en; changed |= BSS_CHANGED_HE_BSS_COLOR; }