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: <b11b11c0dab8e4eb527bba7b332287efe0bc2cbc.camel@sipsolutions.net>
Subject: Re: [PATCH 1/2] mac80211:  Allow drivers to report avg chain signal.
From:   Johannes Berg <johannes@sipsolutions.net>
To:     greearb@candelatech.com, linux-wireless@vger.kernel.org
Date:   Wed, 04 May 2022 12:53:11 +0200
In-Reply-To: <20220225232842.32028-1-greearb@candelatech.com>
References: <20220225232842.32028-1-greearb@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 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?

(Also code style is off wrt. indentation and braces, I feel like I'm
telling you that or fixing such things on every other patch, please take
a little more care before sending patches upstream, you can even run
checkpatch.)

johannes