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;
Message-ID: <1001665f4de469761c4f786954912c40d0c88e72.camel@sipsolutions.net>
Subject: Re: [PATCH 1/2] mac80211: Allow drivers to report avg chain signal.
From:   Johannes Berg <johannes@sipsolutions.net>
To:     Ben Greear <greearb@candelatech.com>,
        linux-wireless@vger.kernel.org
Date:   Wed, 04 May 2022 15:53:17 +0200
In-Reply-To: <d2c03c10-70cf-81a5-f2fa-73ad58b81a92@candelatech.com>
References: <20220225232842.32028-1-greearb@candelatech.com>
         <b11b11c0dab8e4eb527bba7b332287efe0bc2cbc.camel@sipsolutions.net>
         <d2c03c10-70cf-81a5-f2fa-73ad58b81a92@candelatech.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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