Skip to contentSkip to conversation

mac80211: Ensure bss-coloring is always configured

PATCH In review [email protected] 3 participants 7 messages Jan 30, 2024 – Feb 8, 2024 mac80211

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 mac80211: Ensure bss-coloring is always configured

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

And I think the beacon-change logic was slightly wrong, so adjust that as well.

Signed-off-by Ben Greear <[email protected]>
---
 net/mac80211/cfg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/mac80211/cfg.c +5 -5
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;
 	}
 
 	if (params->he_cap) {

@@ -1512,6 +1511,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);
 

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 	changed |= 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;
 	}
Signature · 1 line
2.41.0
maintainer
#1 lore raw
Quoted 5 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 30 lines
Signed-off-by Ben Greear <[email protected]>
---
 net/mac80211/cfg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/mac80211/cfg.c +2 -2
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;
 	}
 
 	if (params->he_cap) {

@@ -1512,6 +1511,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);
 

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 	changed |= err;

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1557

Quoted 10 lines
net/mac80211/cfg.c +3 -3
 
-	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;
 	}

Not sure how this isn't updating the color itself, Lorenzo?

johannes

#2 lore raw
Quoted 29 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:
Quoted 5 lines

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 13 lines
Signed-off-by Ben Greear <[email protected]>
---
 net/mac80211/cfg.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
net/mac80211/cfg.c +0 -0
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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;
Quoted 3 lines

net/mac80211/cfg.c static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, · line 1334

Quoted 3 lines
net/mac80211/cfg.c +1 -2
-		if (params->beacon.he_bss_color.enabled)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;

I think we should use beacon->he_bss_color_valid here instead of params->beacon.he_bss_color.enabled, agree?

Quoted 15 lines
Quoted 15 lines
net/mac80211/cfg.c +1 -0
 	}
 
 	if (params->he_cap) {

@@ -1512,6 +1511,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);
 

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
 		return err;
 	changed |= err;
Quoted 8 lines

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1556

Quoted 8 lines
net/mac80211/cfg.c +3 -3
-	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;
 	}

this seems fine to me.

Regards,
Lorenzo

Quoted 6 lines
Quoted 1 line

Not sure how this isn't updating the color itself, Lorenzo?

johannes

author
#3 lore raw
Quoted 29 lines · Lorenzo Bianconi
Quoted 29 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:
Quoted 5 lines

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 13 lines
Signed-off-by Ben Greear <[email protected]>
---
  net/mac80211/cfg.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)
net/mac80211/cfg.c +0 -0
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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;
Quoted 6 lines
Quoted 3 lines

net/mac80211/cfg.c static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, · line 1334

Quoted 3 lines
net/mac80211/cfg.c +1 -2
-		if (params->beacon.he_bss_color.enabled)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;

I think we should use beacon->he_bss_color_valid here instead of params->beacon.he_bss_color.enabled, agree?

Either way, the value changed from un-set/un-known to some value, so why not just mark it changed and flush to the driver?

Thanks,
Ben

Quoted 35 lines
Quoted 16 lines
Quoted 16 lines
net/mac80211/cfg.c +1 -0
  	}
 
  	if (params->he_cap) {

@@ -1512,6 +1511,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);
 

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
  		return err;
  	changed |= err;
Quoted 8 lines

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1556

Quoted 8 lines
net/mac80211/cfg.c +3 -3
-	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;
  	}

this seems fine to me. >
Regards,
Lorenzo

Quoted 6 lines
Quoted 1 line

Not sure how this isn't updating the color itself, Lorenzo?

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#4 lore raw
Quoted 36 lines · Lorenzo Bianconi
Quoted 21 lines
Quoted 21 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:
Quoted 5 lines

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 5 lines
Signed-off-by Ben Greear <[email protected]>
---
  net/mac80211/cfg.c | 10 +++++-----
  1 file changed, 5 insertions(+), 5 deletions(-)
Quoted 13 lines
Quoted 10 lines

net/mac80211/cfg.c static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, · line 1333

Quoted 10 lines
net/mac80211/cfg.c +1 -2
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;

I think we should use beacon->he_bss_color_valid here instead of params->beacon.he_bss_color.enabled, agree?

Quoted 2 lines

Either way, the value changed from un-set/un-known to some value, so why not just mark it changed and flush to the driver?

IIUC what you mean here, if bss color changes from an un-set/un-known to a configured (valid) value, beacon->he_bss_color_valid will be true (he_bss_color_valid is used to indicate userspace provided a proper color for beacons). What is the difference?
The other way around ("undo" some leftover color from a previous run), it seems a driver/fw bug, and it must be fixed there. Don't you think?

Regards,
Lorenzo

Quoted 40 lines

Thanks,
Ben

Quoted 31 lines
Quoted 12 lines
Quoted 12 lines
net/mac80211/cfg.c +1 -0
  	}
  	if (params->he_cap) {

@@ -1512,6 +1511,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);

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
  		return err;
  	changed |= err;
Quoted 8 lines

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1556

Quoted 8 lines
net/mac80211/cfg.c +3 -3
-	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;
  	}

this seems fine to me. >
Regards,
Lorenzo

Quoted 6 lines
Quoted 1 line

Not sure how this isn't updating the color itself, Lorenzo?

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
author
#5 lore raw
Quoted 39 lines · Lorenzo Bianconi
Quoted 36 lines · Lorenzo Bianconi
Quoted 21 lines
Quoted 21 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:
Quoted 5 lines

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 5 lines
Signed-off-by Ben Greear <[email protected]>
---
   net/mac80211/cfg.c | 10 +++++-----
   1 file changed, 5 insertions(+), 5 deletions(-)
Quoted 13 lines
Quoted 10 lines

net/mac80211/cfg.c static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, · line 1333

Quoted 10 lines
net/mac80211/cfg.c +1 -2
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;

I think we should use beacon->he_bss_color_valid here instead of params->beacon.he_bss_color.enabled, agree?

Quoted 2 lines

Either way, the value changed from un-set/un-known to some value, so why not just mark it changed and flush to the driver?

Quoted 6 lines

IIUC what you mean here, if bss color changes from an un-set/un-known to a configured (valid) value, beacon->he_bss_color_valid will be true (he_bss_color_valid is used to indicate userspace provided a proper color for beacons). What is the difference?
The other way around ("undo" some leftover color from a previous run), it seems a driver/fw bug, and it must be fixed there. Don't you think?

Well, no. I think the stack should set to known state, thus my original patch to do so.

But, not worth arguing about as it seems to have no functional difference at this point.

Thanks,
Ben

Quoted 44 lines

Regards,
Lorenzo

Quoted 40 lines

Thanks,
Ben

Quoted 31 lines
Quoted 12 lines
Quoted 12 lines
net/mac80211/cfg.c +1 -0
   	}
   	if (params->he_cap) {

@@ -1512,6 +1511,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);

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
   		return err;
   	changed |= err;
Quoted 8 lines

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1556

Quoted 8 lines
net/mac80211/cfg.c +3 -3
-	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;
   	}

this seems fine to me. >
Regards,
Lorenzo

Quoted 6 lines
Quoted 1 line

Not sure how this isn't updating the color itself, Lorenzo?

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
#6 lore raw
Quoted 47 lines · Lorenzo Bianconi
Quoted 39 lines
Quoted 36 lines · Lorenzo Bianconi
Quoted 21 lines
Quoted 21 lines · On Tue, 2024-01-30 at 10:08 -0800, [email protected] wrote:
Quoted 5 lines

From: Ben Greear <[email protected]>

Old code would not set it to disabled, just assumed that driver would default to disabled. Change this to explicitly request bss color be flushed on initial driver configuration.

Arguably, the current behaviour is in line with the documentation of BSS_CHANGED_HE_BSS_COLOR ... but I would tend to agree that enabling/disabling coloring should be covered by it as well. Lorenzo?

Quoted 2 lines

And I think the beacon-change logic was slightly wrong, so adjust that as well.

That's not a great thing for a commit message to say ...

Quoted 5 lines
Signed-off-by Ben Greear <[email protected]>
---
   net/mac80211/cfg.c | 10 +++++-----
   1 file changed, 5 insertions(+), 5 deletions(-)
Quoted 13 lines
Quoted 10 lines

net/mac80211/cfg.c static int ieee80211_start_ap(struct wiphy *wiphy, struct net_device *dev, · line 1333

Quoted 10 lines
net/mac80211/cfg.c +1 -2
index 1c7fb0959cfd..1a6c6c764cbc 100644
@@ -1331,8 +1331,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)
-			changed |= BSS_CHANGED_HE_BSS_COLOR;
+		changed |= BSS_CHANGED_HE_BSS_COLOR;

I think we should use beacon->he_bss_color_valid here instead of params->beacon.he_bss_color.enabled, agree?

Quoted 2 lines

Either way, the value changed from un-set/un-known to some value, so why not just mark it changed and flush to the driver?

Quoted 6 lines

IIUC what you mean here, if bss color changes from an un-set/un-known to a configured (valid) value, beacon->he_bss_color_valid will be true (he_bss_color_valid is used to indicate userspace provided a proper color for beacons). What is the difference?
The other way around ("undo" some leftover color from a previous run), it seems a driver/fw bug, and it must be fixed there. Don't you think?

Quoted 5 lines

Well, no. I think the stack should set to known state, thus my original patch to do so.

But, not worth arguing about as it seems to have no functional difference at this point.

I think the second part is fine and should be applied.

Regards,
Lorenzo

Quoted 54 lines

Thanks,
Ben

Quoted 44 lines

Regards,
Lorenzo

Quoted 40 lines

Thanks,
Ben

Quoted 31 lines
Quoted 12 lines
Quoted 12 lines
net/mac80211/cfg.c +1 -0
   	}
   	if (params->he_cap) {

@@ -1512,6 +1511,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);

@@ -1549,9 +1549,9 @@ static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev,
   		return err;
   	changed |= err;
Quoted 8 lines

net/mac80211/cfg.c static int ieee80211_change_beacon(struct wiphy *wiphy, struct net_device *dev, · line 1556

Quoted 8 lines
net/mac80211/cfg.c +3 -3
-	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;
   	}

this seems fine to me. >
Regards,
Lorenzo

Quoted 6 lines
Quoted 1 line

Not sure how this isn't updating the color itself, Lorenzo?

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com