[PATCH] wifi: mac80211: Improve bss-color configuration.

[PATCH] wifi: mac80211: Improve bss-color configuration.

[greearb]

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;
 	}
 
@@ -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;
 	}
 
-- 
2.42.0

Re: [PATCH] wifi: mac80211: Improve bss-color configuration.

[Jeff Johnson]

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;
>  	}
>  

Re: [PATCH] wifi: mac80211: Improve bss-color configuration.

[Ben Greear]

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;
>>   	}
>>   
> 

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] wifi: mac80211: Improve bss-color configuration.

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]

> 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
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]