diff mbox series

[1/2] mac80211: Allow drivers to report avg chain signal.

Message ID 20220225232842.32028-1-greearb@candelatech.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [1/2] mac80211: Allow drivers to report avg chain signal. | expand

Commit Message

Ben Greear Feb. 25, 2022, 11:28 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

Drivers that use RSS cannot get the avg signal from mac80211.
So allow drivers to report the avg chain signal while letting
mac80211 take care of the last chain signal.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 net/mac80211/sta_info.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Johannes Berg May 4, 2022, 10:53 a.m. UTC | #1 
On Fri, 2022-02-25 at 15:28 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Drivers that use RSS cannot get the avg signal from mac80211.
> So allow drivers to report the avg chain signal while letting
> mac80211 take care of the last chain signal.
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  net/mac80211/sta_info.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index 43a58b30c6a4..00836f587b6d 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -2543,6 +2543,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>  	if (last_rxstats->chains &&
>  	    !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL) |
>  			       BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG)))) {
> +		/* Neither chain signal nor chain signal avg is filled */
>  		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);

I don't think that comment adds value, in fact, since it's _after_ the
condition it applies to (rather than before), it's confusing? At least
to me it was ... And if you read the condition that already says so
pretty clearly anyway.

> @@ -2557,6 +2558,21 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>  		}
>  	}
>  
> +	/* Check if chain signal is not filled, for cases avg was filled by
> +	 * driver bug last chain signal was not.
> +	 */
> +	if (last_rxstats->chains &&
> +		 !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)))) {
> +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
> +
> +		sinfo->chains = last_rxstats->chains;
> +
> +		for (i = 0; i < ARRAY_SIZE(sinfo->chain_signal); i++) {
> +			sinfo->chain_signal[i] =
> +				last_rxstats->chain_signal_last[i];
> +		}
> +	}
> 

Now you've duplicated this code ... you can remove it above, no?

(Also code style is off wrt. indentation and braces, I feel like I'm
telling you that or fixing such things on every other patch, please take
a little more care before sending patches upstream, you can even run
checkpatch.)

johannes
Ben Greear May 4, 2022, 1:49 p.m. UTC | #2 
On 5/4/22 3:53 AM, Johannes Berg wrote:
> On Fri, 2022-02-25 at 15:28 -0800, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Drivers that use RSS cannot get the avg signal from mac80211.
>> So allow drivers to report the avg chain signal while letting
>> mac80211 take care of the last chain signal.
>>
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   net/mac80211/sta_info.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
>> index 43a58b30c6a4..00836f587b6d 100644
>> --- a/net/mac80211/sta_info.c
>> +++ b/net/mac80211/sta_info.c
>> @@ -2543,6 +2543,7 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>>   	if (last_rxstats->chains &&
>>   	    !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL) |
>>   			       BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG)))) {
>> +		/* Neither chain signal nor chain signal avg is filled */
>>   		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
> 
> I don't think that comment adds value, in fact, since it's _after_ the
> condition it applies to (rather than before), it's confusing? At least
> to me it was ... And if you read the condition that already says so
> pretty clearly anyway.
> 
>> @@ -2557,6 +2558,21 @@ void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
>>   		}
>>   	}
>>   
>> +	/* Check if chain signal is not filled, for cases avg was filled by
>> +	 * driver bug last chain signal was not.
>> +	 */
>> +	if (last_rxstats->chains &&
>> +		 !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)))) {
>> +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
>> +
>> +		sinfo->chains = last_rxstats->chains;
>> +
>> +		for (i = 0; i < ARRAY_SIZE(sinfo->chain_signal); i++) {
>> +			sinfo->chain_signal[i] =
>> +				last_rxstats->chain_signal_last[i];
>> +		}
>> +	}
>>
> 
> Now you've duplicated this code ... you can remove it above, no?

The conditional check in this second block is different.  It is one reason
why I added the other comment in the preceeding code.

I can fix the typo (bug -> but) in the comment and the indentation.

If you still think code is duplicated, please let me know more precisely
what is duplicated...maybe I mis-understood your comment.

Thanks,
Ben
Johannes Berg May 4, 2022, 1:53 p.m. UTC | #3 
On Wed, 2022-05-04 at 06:49 -0700, Ben Greear wrote:
> 
> > > +	/* Check if chain signal is not filled, for cases avg was filled by
> > > +	 * driver bug last chain signal was not.
> > > +	 */
> > > +	if (last_rxstats->chains &&
> > > +		 !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)))) {
> > > +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
> > > +
> > > +		sinfo->chains = last_rxstats->chains;
> > > +
> > > +		for (i = 0; i < ARRAY_SIZE(sinfo->chain_signal); i++) {
> > > +			sinfo->chain_signal[i] =
> > > +				last_rxstats->chain_signal_last[i];
> > > +		}
> > > +	}
> > > 
> > 
> > Now you've duplicated this code ... you can remove it above, no?
> 
> The conditional check in this second block is different.  It is one reason
> why I added the other comment in the preceeding code.

Oh, sure, I get that.

But I mean you can end up setting sinfo->chains and all of the values in
sinfo->chain_signal[i] with both cases: when "both are unset" or when
"just chain signal is unset"?

So wouldn't it be more or less equivalent to do

 if (!signal-filled) { fill signal }

which is your new code here, and thus have

 if (!signal-filled) { fill signal }
 if (!signal-avg-filled) { fill avg signal }

rather than

 if (!signal-filled && !signal-avg-filled) {
    fill signal, fill avg-signal
 }
 if (!signal-filled) {
    fill signal
 }

or am I misreading that?

johannes
Ben Greear May 4, 2022, 2:31 p.m. UTC | #4 
On 5/4/22 6:53 AM, Johannes Berg wrote:
> On Wed, 2022-05-04 at 06:49 -0700, Ben Greear wrote:
>>
>>>> +	/* Check if chain signal is not filled, for cases avg was filled by
>>>> +	 * driver bug last chain signal was not.
>>>> +	 */
>>>> +	if (last_rxstats->chains &&
>>>> +		 !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)))) {
>>>> +		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
>>>> +
>>>> +		sinfo->chains = last_rxstats->chains;
>>>> +
>>>> +		for (i = 0; i < ARRAY_SIZE(sinfo->chain_signal); i++) {
>>>> +			sinfo->chain_signal[i] =
>>>> +				last_rxstats->chain_signal_last[i];
>>>> +		}
>>>> +	}
>>>>
>>>
>>> Now you've duplicated this code ... you can remove it above, no?
>>
>> The conditional check in this second block is different.  It is one reason
>> why I added the other comment in the preceeding code.
> 
> Oh, sure, I get that.
> 
> But I mean you can end up setting sinfo->chains and all of the values in
> sinfo->chain_signal[i] with both cases: when "both are unset" or when
> "just chain signal is unset"?
> 
> So wouldn't it be more or less equivalent to do
> 
>   if (!signal-filled) { fill signal }
> 
> which is your new code here, and thus have
> 
>   if (!signal-filled) { fill signal }
>   if (!signal-avg-filled) { fill avg signal }
> 
> rather than
> 
>   if (!signal-filled && !signal-avg-filled) {
>      fill signal, fill avg-signal
>   }
>   if (!signal-filled) {
>      fill signal
>   }
> 
> or am I misreading that?

You may be correct, but once that first clause happens, the second will not since the
first should set the signal-is-filled flag.

So maybe just put it in an else clause to save the second check.

I'll take a close look at it soon while re-working the typo and white-space.

Thanks,
Ben
diff mbox series

Patch

diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index 43a58b30c6a4..00836f587b6d 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -2543,6 +2543,7 @@  void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 	if (last_rxstats->chains &&
 	    !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL) |
 			       BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG)))) {
+		/* Neither chain signal nor chain signal avg is filled */
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
 		if (!sta->pcpu_rx_stats)
 			sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG);
@@ -2557,6 +2558,21 @@  void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo,
 		}
 	}
 
+	/* Check if chain signal is not filled, for cases avg was filled by
+	 * driver bug last chain signal was not.
+	 */
+	if (last_rxstats->chains &&
+		 !(sinfo->filled & (BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)))) {
+		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL);
+
+		sinfo->chains = last_rxstats->chains;
+
+		for (i = 0; i < ARRAY_SIZE(sinfo->chain_signal); i++) {
+			sinfo->chain_signal[i] =
+				last_rxstats->chain_signal_last[i];
+		}
+	}
+
 	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE))) {
 		sta_set_rate_info_tx(sta, &sta->tx_stats.last_rate,
 				     &sinfo->txrate);