2022-05-04 15:20:48

by Johannes Berg

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

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

2022-05-04 17:50:09

by Ben Greear

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

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 <[email protected]>
Candela Technologies Inc http://www.candelatech.com