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@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
