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

[Ben Greear <greearb@candelatech.com>]

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



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