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>
 <d2c03c10-70cf-81a5-f2fa-73ad58b81a92@candelatech.com>
 <1001665f4de469761c4f786954912c40d0c88e72.camel@sipsolutions.net>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <562596f2-5c69-367d-5359-0a79f0e52a2f@candelatech.com>
Date:   Wed, 4 May 2022 07:31:19 -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: <1001665f4de469761c4f786954912c40d0c88e72.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 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

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