diff mbox series

wifi: mac80211: Improve bss-color configuration.

Message ID 20240307181039.3219840-1-greearb@candelatech.com
State New
Headers show
Series wifi: mac80211: Improve bss-color configuration. | expand

Commit Message

Ben Greear March 7, 2024, 6:10 p.m. UTC
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(-)

Comments

Jeff Johnson March 8, 2024, 5:40 p.m. UTC | #1
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;
>  	}
>
Ben Greear March 8, 2024, 5:44 p.m. UTC | #2
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;
>>   	}
>>   
>
Lorenzo Bianconi March 18, 2024, 10:48 p.m. UTC | #3
> 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 mbox series

Patch

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