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

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

[greearb]

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);
 		if (!sta->pcpu_rx_stats)
 			sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG);
@@ -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];
+		}
+	}
+
 	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE))) {
 		sta_set_rate_info_tx(sta, &sta->tx_stats.last_rate,
 				     &sinfo->txrate);
-- 
2.20.1

[PATCH 2/2] iwlwifi: RX signal improvements.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Sum up multi-chain signals instead of just taking the maximum value.
This gives more correct total on-air signal value.

And, the firmware managed averages do not match expected per-packet values,
so ignore those for MQ drivers and instead calculate averages in the driver
and report that.

Combined with the previous patch, this gives better looking RX signal report:

	signal:  	-39 [-42, -41] dBm
	signal avg:	-39 [-42, -41] dBm
	beacon signal avg:	-46 dBm

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 .../net/wireless/intel/iwlwifi/mvm/mac80211.c | 38 ++++++++++++++---
 drivers/net/wireless/intel/iwlwifi/mvm/mvm.h  | 34 +++++++++++++++
 drivers/net/wireless/intel/iwlwifi/mvm/rx.c   |  4 +-
 drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c | 41 ++++++++++++++++---
 drivers/net/wireless/intel/iwlwifi/mvm/sta.c  |  5 +++
 drivers/net/wireless/intel/iwlwifi/mvm/sta.h  |  7 ++++
 6 files changed, 117 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
index 959ad85e3430..c11b20d847f3 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mac80211.c
@@ -4869,9 +4869,32 @@ static void iwl_mvm_mac_sta_statistics(struct ieee80211_hw *hw,
 	struct iwl_mvm_vif *mvmvif = iwl_mvm_vif_from_mac80211(vif);
 	struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta);
 
-	if (mvmsta->avg_energy) {
-		sinfo->signal_avg = -(s8)mvmsta->avg_energy;
+	if (iwl_mvm_has_new_rx_api(mvm)) { /* rxmq logic */
+		/* Grab chain signal avg, mac80211 cannot do it because
+		 * this driver uses RSS.  Grab signal_avg here too because firmware
+		 * appears go not do DB summing and/or has other bugs. --Ben
+		 */
 		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL_AVG);
+		sinfo->signal_avg = -ewma_signal_read(&mvmsta->rx_avg_signal);
+
+		if (!mvmvif->bf_data.bf_enabled) {
+			/* The firmware reliably reports different signal (2db weaker in my case)
+			 * than if I calculate it from the rx-status.  So, fill that here.
+			 * Beacons are only received if you turn off beacon filtering, however.
+			 */
+			sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BEACON_SIGNAL_AVG);
+			sinfo->rx_beacon_signal_avg = -ewma_signal_read(&mvmsta->rx_avg_beacon_signal);
+		}
+
+		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG);
+		sinfo->chain_signal_avg[0] = -ewma_signal_read(&mvmsta->rx_avg_chain_signal[0]);
+		sinfo->chain_signal_avg[1] = -ewma_signal_read(&mvmsta->rx_avg_chain_signal[1]);
+	}
+	else {
+		if (mvmsta->avg_energy) {
+			sinfo->signal_avg = -(s8)mvmsta->avg_energy;
+			sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL_AVG);
+		}
 	}
 
 	if (iwl_mvm_has_tlc_offload(mvm)) {
@@ -4899,10 +4922,13 @@ static void iwl_mvm_mac_sta_statistics(struct ieee80211_hw *hw,
 	sinfo->rx_beacon = mvmvif->beacon_stats.num_beacons +
 			   mvmvif->beacon_stats.accu_num_beacons;
 	sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BEACON_RX);
-	if (mvmvif->beacon_stats.avg_signal) {
-		/* firmware only reports a value after RXing a few beacons */
-		sinfo->rx_beacon_signal_avg = mvmvif->beacon_stats.avg_signal;
-		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BEACON_SIGNAL_AVG);
+
+	if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_BEACON_SIGNAL_AVG))) {
+		if (mvmvif->beacon_stats.avg_signal) {
+			/* firmware only reports a value after RXing a few beacons */
+			sinfo->rx_beacon_signal_avg = mvmvif->beacon_stats.avg_signal;
+			sinfo->filled |= BIT_ULL(NL80211_STA_INFO_BEACON_SIGNAL_AVG);
+		}
 	}
  unlock:
 	mutex_unlock(&mvm->mutex);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
index cd5645c3d15c..421094477edd 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/mvm.h
@@ -2150,4 +2150,38 @@ enum iwl_location_cipher iwl_mvm_cipher_to_location_cipher(u32 cipher)
 		return IWL_LOCATION_CIPHER_INVALID;
 	}
 }
+
+static inline int iwl_mvm_sum_sigs_2(int a, int b)
+{
+       int diff;
+
+       /* S8_MIN means value-is-not-set */
+       if (b == S8_MIN)
+               return a;
+       if (a == S8_MIN)
+               return b;
+
+       if (a >= b) {
+               /* a is largest value, add to it. */
+               diff = a - b;
+               if (diff == 0)
+                       return a + 3;
+               else if (diff <= 2)
+                       return a + 2;
+               else if (diff <= 6)
+                       return a + 1;
+               return a;
+       } else {
+               /* b is largest value, add to it. */
+               diff = b - a;
+               if (diff == 0)
+                       return b + 3;
+               else if (diff <= 2)
+                       return b + 2;
+               else if (diff <= 6)
+                       return b + 1;
+               return b;
+       }
+}
+
 #endif /* __IWL_MVM_H__ */
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rx.c b/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
index d22f40a5354d..e8d753e855e1 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rx.c
@@ -114,7 +114,9 @@ static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
 	energy_b = (val & IWL_RX_INFO_ENERGY_ANT_B_MSK) >>
 						IWL_RX_INFO_ENERGY_ANT_B_POS;
 	energy_b = energy_b ? -energy_b : S8_MIN;
-	max_energy = max(energy_a, energy_b);
+
+	/* use DB summing to get better RSSI reporting */
+	max_energy = iwl_mvm_sum_sigs_2(energy_a, energy_b);
 
 	IWL_DEBUG_STATS(mvm, "energy In A %d B %d  , and max %d\n",
 			energy_a, energy_b, max_energy);
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
index 31a8c5083f61..489f8a843f82 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
@@ -278,14 +278,26 @@ static void iwl_mvm_pass_packet_to_mac80211(struct iwl_mvm *mvm,
 static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
 					struct ieee80211_rx_status *rx_status,
 					u32 rate_n_flags, int energy_a,
-					int energy_b)
+					int energy_b, struct ieee80211_sta *sta,
+					bool is_beacon, bool my_beacon)
 {
 	int max_energy;
 	u32 rate_flags = rate_n_flags;
+	struct iwl_mvm_sta *mvmsta = NULL;
+
+	if (sta && !(is_beacon && !my_beacon)) {
+		mvmsta = iwl_mvm_sta_from_mac80211(sta);
+		if (energy_a)
+			ewma_signal_add(&mvmsta->rx_avg_chain_signal[0], energy_a);
+		if (energy_b)
+			ewma_signal_add(&mvmsta->rx_avg_chain_signal[1], energy_b);
+	}
 
 	energy_a = energy_a ? -energy_a : S8_MIN;
 	energy_b = energy_b ? -energy_b : S8_MIN;
-	max_energy = max(energy_a, energy_b);
+
+	/* use DB summing to get better RSSI reporting */
+	max_energy = iwl_mvm_sum_sigs_2(energy_a, energy_b);
 
 	IWL_DEBUG_STATS(mvm, "energy In A %d B %d, and max %d\n",
 			energy_a, energy_b, max_energy);
@@ -295,6 +307,15 @@ static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
 		(rate_flags & RATE_MCS_ANT_AB_MSK) >> RATE_MCS_ANT_POS;
 	rx_status->chain_signal[0] = energy_a;
 	rx_status->chain_signal[1] = energy_b;
+
+	if (mvmsta) {
+		if (is_beacon) {
+			if (my_beacon)
+				ewma_signal_add(&mvmsta->rx_avg_beacon_signal, -max_energy);
+		} else {
+			ewma_signal_add(&mvmsta->rx_avg_signal, -max_energy);
+		}
+	}
 }
 
 static int iwl_mvm_rx_mgmt_prot(struct ieee80211_sta *sta,
@@ -1685,6 +1706,8 @@ void iwl_mvm_rx_mpdu_mq(struct iwl_mvm *mvm, struct napi_struct *napi,
 	};
 	u32 format;
 	bool is_sgi;
+	bool is_beacon;
+	bool my_beacon = false;
 
 	if (unlikely(test_bit(IWL_MVM_STATUS_IN_HW_RESTART, &mvm->status)))
 		return;
@@ -1828,8 +1851,6 @@ void iwl_mvm_rx_mpdu_mq(struct iwl_mvm *mvm, struct napi_struct *napi,
 	}
 	rx_status->freq = ieee80211_channel_to_frequency(channel,
 							 rx_status->band);
-	iwl_mvm_get_signal_strength(mvm, rx_status, rate_n_flags, energy_a,
-				    energy_b);
 
 	/* update aggregation data for monitor sake on default queue */
 	if (!queue && (phy_info & IWL_RX_MPDU_PHY_AMPDU)) {
@@ -1878,6 +1899,16 @@ void iwl_mvm_rx_mpdu_mq(struct iwl_mvm *mvm, struct napi_struct *napi,
 		goto out;
 	}
 
+	is_beacon = ieee80211_is_beacon(hdr->frame_control);
+	if (is_beacon && sta) {
+		/* see if it is beacon destined for us */
+		if (memcmp(sta->addr, hdr->addr2, ETH_ALEN) == 0)
+			my_beacon = true;
+	}
+
+	iwl_mvm_get_signal_strength(mvm, rx_status, rate_n_flags, energy_a,
+				    energy_b, sta, is_beacon, my_beacon);
+
 	if (sta) {
 		struct iwl_mvm_sta *mvmsta = iwl_mvm_sta_from_mac80211(sta);
 		struct ieee80211_vif *tx_blocked_vif =
@@ -2138,7 +2169,7 @@ void iwl_mvm_rx_monitor_no_data(struct iwl_mvm *mvm, struct napi_struct *napi,
 	rx_status->freq = ieee80211_channel_to_frequency(channel,
 							 rx_status->band);
 	iwl_mvm_get_signal_strength(mvm, rx_status, rate_n_flags, energy_a,
-				    energy_b);
+				    energy_b, sta, false, false);
 
 	rcu_read_lock();
 
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
index a64874c05ced..e54051830f1a 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.c
@@ -1577,6 +1577,11 @@ int iwl_mvm_add_sta(struct iwl_mvm *mvm,
 
 	spin_lock_init(&mvm_sta->lock);
 
+	ewma_signal_init(&mvm_sta->rx_avg_chain_signal[0]);
+	ewma_signal_init(&mvm_sta->rx_avg_chain_signal[1]);
+	ewma_signal_init(&mvm_sta->rx_avg_signal);
+	ewma_signal_init(&mvm_sta->rx_avg_beacon_signal);
+
 	/* if this is a HW restart re-alloc existing queues */
 	if (test_bit(IWL_MVM_STATUS_IN_HW_RESTART, &mvm->status)) {
 		struct iwl_mvm_int_sta tmp_sta = {
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/sta.h b/drivers/net/wireless/intel/iwlwifi/mvm/sta.h
index 32b4d1935788..4c86e33e1667 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/sta.h
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/sta.h
@@ -10,6 +10,7 @@
 #include <linux/spinlock.h>
 #include <net/mac80211.h>
 #include <linux/wait.h>
+#include <linux/average.h>
 
 #include "iwl-trans.h" /* for IWL_MAX_TID_COUNT */
 #include "fw-api.h" /* IWL_MVM_STATION_COUNT_MAX */
@@ -18,6 +19,9 @@
 struct iwl_mvm;
 struct iwl_mvm_vif;
 
+/* This makes us a 'struct ewma_signal {' object. */
+DECLARE_EWMA(signal, 10, 8);
+
 /**
  * DOC: DQA - Dynamic Queue Allocation -introduction
  *
@@ -415,6 +419,9 @@ struct iwl_mvm_sta {
 	u8 sleep_tx_count;
 	u8 avg_energy;
 	u8 tx_ant;
+	struct ewma_signal rx_avg_chain_signal[2];
+	struct ewma_signal rx_avg_signal;
+	struct ewma_signal rx_avg_beacon_signal;
 };
 
 u16 iwl_mvm_tid_queued(struct iwl_mvm *mvm, struct iwl_mvm_tid_data *tid_data);
-- 
2.20.1

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

[Johannes Berg]

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

Re: [PATCH 2/2] iwlwifi: RX signal improvements.

[Johannes Berg]

On Fri, 2022-02-25 at 15:28 -0800, greearb@candelatech.com wrote:
> 
> -	if (mvmsta->avg_energy) {
> -		sinfo->signal_avg = -(s8)mvmsta->avg_energy;
> +	if (iwl_mvm_has_new_rx_api(mvm)) { /* rxmq logic */
> +		/* Grab chain signal avg, mac80211 cannot do it because
> +		 * this driver uses RSS.  Grab signal_avg here too because firmware
> +		 * appears go not do DB summing and/or has other bugs. --Ben
> +		 */

That "--Ben" really seems inappropriate ... please take more care when
sending patches to the list.

>  		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL_AVG);
> +		sinfo->signal_avg = -ewma_signal_read(&mvmsta->rx_avg_signal);
> +
> +		if (!mvmvif->bf_data.bf_enabled) {
> +			/* The firmware reliably reports different signal (2db weaker in my case)


dB, but I guess we'll want to fix that instead or so ...

> +static inline int iwl_mvm_sum_sigs_2(int a, int b)
> +{

Feels like that calls for a helper function somewhere else, and a
comment explaining it :)

> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
> @@ -278,14 +278,26 @@ static void iwl_mvm_pass_packet_to_mac80211(struct iwl_mvm *mvm,
>  static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
>  					struct ieee80211_rx_status *rx_status,
>  					u32 rate_n_flags, int energy_a,
> -					int energy_b)
> +					int energy_b, struct ieee80211_sta *sta,
> +					bool is_beacon, bool my_beacon)
>  {
>  	int max_energy;
>  	u32 rate_flags = rate_n_flags;
> +	struct iwl_mvm_sta *mvmsta = NULL;
> +
> +	if (sta && !(is_beacon && !my_beacon)) {
> +		mvmsta = iwl_mvm_sta_from_mac80211(sta);
> +		if (energy_a)
> +			ewma_signal_add(&mvmsta->rx_avg_chain_signal[0], energy_a);
> +		if (energy_b)
> +			ewma_signal_add(&mvmsta->rx_avg_chain_signal[1], energy_b);
> +	}

This is obviously racy for the exact same reason that mac80211 doesn't
give you averages ... you cannot do that.

Without locking, you have to rely on the firmware, and I don't think we
want any locking here.

> @@ -295,6 +307,15 @@ static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
>  		(rate_flags & RATE_MCS_ANT_AB_MSK) >> RATE_MCS_ANT_POS;
>  	rx_status->chain_signal[0] = energy_a;
>  	rx_status->chain_signal[1] = energy_b;
> +
> +	if (mvmsta) {
> +		if (is_beacon) {
> +			if (my_beacon)
> +				ewma_signal_add(&mvmsta->rx_avg_beacon_signal, -max_energy);
> +		} else {
> +			ewma_signal_add(&mvmsta->rx_avg_signal, -max_energy);
> +		}
> +	}

Why would you ignore "is_beacon && !my_beacon" cases, but handle all
other management frames from everyone?

> @@ -1878,6 +1899,16 @@ void iwl_mvm_rx_mpdu_mq(struct iwl_mvm *mvm, struct napi_struct *napi,
>  		goto out;
>  	}
>  
> +	is_beacon = ieee80211_is_beacon(hdr->frame_control);
> +	if (is_beacon && sta) {
> +		/* see if it is beacon destined for us */
> +		if (memcmp(sta->addr, hdr->addr2, ETH_ALEN) == 0)
> +			my_beacon = true;

don't use memcmp() for that.


Anyway this really needs a lot of work, it cannot work as is.

johannes

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

[Ben Greear]

On 5/4/22 3:53 AM, Johannes Berg wrote:
> 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?

The conditional check in this second block is different.  It is one reason
why I added the other comment in the preceeding code.

I can fix the typo (bug -> but) in the comment and the indentation.

If you still think code is duplicated, please let me know more precisely
what is duplicated...maybe I mis-understood your comment.

Thanks,
Ben



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

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

[Johannes Berg]

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

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

[Ben Greear]

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

Re: [PATCH 2/2] iwlwifi: RX signal improvements.

[Ben Greear]

On 5/4/22 3:57 AM, Johannes Berg wrote:
> On Fri, 2022-02-25 at 15:28 -0800, greearb@candelatech.com wrote:
>>
>> -	if (mvmsta->avg_energy) {
>> -		sinfo->signal_avg = -(s8)mvmsta->avg_energy;
>> +	if (iwl_mvm_has_new_rx_api(mvm)) { /* rxmq logic */
>> +		/* Grab chain signal avg, mac80211 cannot do it because
>> +		 * this driver uses RSS.  Grab signal_avg here too because firmware
>> +		 * appears go not do DB summing and/or has other bugs. --Ben
>> +		 */
> 
> That "--Ben" really seems inappropriate ... please take more care when
> sending patches to the list.

I figure if I am submitting a patch that is blaming firmware, and I have no access
to firmware to actually back up my assertion, then I should put my name on it
so that folks looking at the driver don't think it is Intel disparaging it's own firmware.

> 
>>   		sinfo->filled |= BIT_ULL(NL80211_STA_INFO_SIGNAL_AVG);
>> +		sinfo->signal_avg = -ewma_signal_read(&mvmsta->rx_avg_signal);
>> +
>> +		if (!mvmvif->bf_data.bf_enabled) {
>> +			/* The firmware reliably reports different signal (2db weaker in my case)
> 
> 
> dB, but I guess we'll want to fix that instead or so ...
> 
>> +static inline int iwl_mvm_sum_sigs_2(int a, int b)
>> +{
> 
> Feels like that calls for a helper function somewhere else, and a
> comment explaining it :)

Yes, most drivers have these issues, but each driver may have a different
way of specifying that 'a' or 'b' is valid signal or not, so simple helper method
is not so simple to write efficiently.

> 
>> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/rxmq.c
>> @@ -278,14 +278,26 @@ static void iwl_mvm_pass_packet_to_mac80211(struct iwl_mvm *mvm,
>>   static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
>>   					struct ieee80211_rx_status *rx_status,
>>   					u32 rate_n_flags, int energy_a,
>> -					int energy_b)
>> +					int energy_b, struct ieee80211_sta *sta,
>> +					bool is_beacon, bool my_beacon)
>>   {
>>   	int max_energy;
>>   	u32 rate_flags = rate_n_flags;
>> +	struct iwl_mvm_sta *mvmsta = NULL;
>> +
>> +	if (sta && !(is_beacon && !my_beacon)) {
>> +		mvmsta = iwl_mvm_sta_from_mac80211(sta);
>> +		if (energy_a)
>> +			ewma_signal_add(&mvmsta->rx_avg_chain_signal[0], energy_a);
>> +		if (energy_b)
>> +			ewma_signal_add(&mvmsta->rx_avg_chain_signal[1], energy_b);
>> +	}
> 
> This is obviously racy for the exact same reason that mac80211 doesn't
> give you averages ... you cannot do that.

I was thinking about that.  The ewma code is pretty simple and has some read_once
logic and on-stack variables.  So, since it is a running avg anyway, I did not see how it could race
and actually cause any significantly invalid result.

At any rate, in testing under load and otherwise, my patch gave expected results
that were more accurate than what I had before.  A 1 / billion chance of temporarily
bad RSSI avg report is fine trade-off for me if it keeps locking out of the hot rx path
and reports better values the other billion times it is read.

> 
> Without locking, you have to rely on the firmware, and I don't think we
> want any locking here.
> 
>> @@ -295,6 +307,15 @@ static void iwl_mvm_get_signal_strength(struct iwl_mvm *mvm,
>>   		(rate_flags & RATE_MCS_ANT_AB_MSK) >> RATE_MCS_ANT_POS;
>>   	rx_status->chain_signal[0] = energy_a;
>>   	rx_status->chain_signal[1] = energy_b;
>> +
>> +	if (mvmsta) {
>> +		if (is_beacon) {
>> +			if (my_beacon)
>> +				ewma_signal_add(&mvmsta->rx_avg_beacon_signal, -max_energy);
>> +		} else {
>> +			ewma_signal_add(&mvmsta->rx_avg_signal, -max_energy);
>> +		}
>> +	}
> 
> Why would you ignore "is_beacon && !my_beacon" cases, but handle all
> other management frames from everyone?

I want RSSI to report signal from the peer device (AP in STA mode).  Would the
driver actually receive any other mgt frames (aside from beacons) from non-peer transmitters at this
level of code?  I'm assuming we are not in monitor mode, as reporting RSSI in
that case doesn't make much sense anyway.

> 
>> @@ -1878,6 +1899,16 @@ void iwl_mvm_rx_mpdu_mq(struct iwl_mvm *mvm, struct napi_struct *napi,
>>   		goto out;
>>   	}
>>   
>> +	is_beacon = ieee80211_is_beacon(hdr->frame_control);
>> +	if (is_beacon && sta) {
>> +		/* see if it is beacon destined for us */
>> +		if (memcmp(sta->addr, hdr->addr2, ETH_ALEN) == 0)
>> +			my_beacon = true;
> 
> don't use memcmp() for that.
> 
> 
> Anyway this really needs a lot of work, it cannot work as is.

Thanks for the comments.

--Ben

> 
> johannes
> 


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

Re: [PATCH 2/2] iwlwifi: RX signal improvements.

[Ben Greear]

On 5/4/22 07:46, Ben Greear wrote:
> On 5/4/22 3:57 AM, Johannes Berg wrote:
>> On Fri, 2022-02-25 at 15:28 -0800, greearb@candelatech.com wrote:
>>>
>>> -    if (mvmsta->avg_energy) {
>>> -        sinfo->signal_avg = -(s8)mvmsta->avg_energy;
>>> +    if (iwl_mvm_has_new_rx_api(mvm)) { /* rxmq logic */
>>> +        /* Grab chain signal avg, mac80211 cannot do it because
>>> +         * this driver uses RSS.  Grab signal_avg here too because firmware
>>> +         * appears go not do DB summing and/or has other bugs. --Ben
>>> +         */
>>
>> That "--Ben" really seems inappropriate ... please take more care when
>> sending patches to the list.
> 
> I figure if I am submitting a patch that is blaming firmware, and I have no access
> to firmware to actually back up my assertion, then I should put my name on it
> so that folks looking at the driver don't think it is Intel disparaging it's own firmware.

Hello Johannes,

If there is no reasonable chance to make changes like this to iwlwifi (lockless
signal averaging is the unfixable part I think), let me know
and I'll consider the issue closed.

And, in that case, the mac80211 patch is un-needed since no driver that I know of
would need that code.

Thanks,
Ben


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