Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 1/2] mac80211: Allow drivers to report avg chain signal.
To:     Johannes Berg <johannes@sipsolutions.net>,
        linux-wireless@vger.kernel.org
References: <20220225232842.32028-1-greearb@candelatech.com>
 <b11b11c0dab8e4eb527bba7b332287efe0bc2cbc.camel@sipsolutions.net>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <d2c03c10-70cf-81a5-f2fa-73ad58b81a92@candelatech.com>
Date:   Wed, 4 May 2022 06:49:35 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <b11b11c0dab8e4eb527bba7b332287efe0bc2cbc.camel@sipsolutions.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-MW
Content-Transfer-Encoding: 7bit
Precedence: bulk

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