[RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Sven Eckelmann]

From: Sven Eckelmann <seckelmann@datto.com>

Hi,

it was observed that ath9k provides accumulated survey counters but ath10k
neither provides deltas nor accumulated counters. Instead it returns
some value which was returned at some point from the firmware.

But as it turns out, this data is not reliable. To make it more useful,
ath10k has to:

* retrieve counters rather frequently for hardware which is known to use
  firmware versions with low number counter bits (for only 14-30s)
* clean up received counter values 
* accumulate counters from firmware

A comparison of the resulting output with these fixes can be seen under
https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86749f4d60&fullscreen&panelId=5&from=1568782046974&to=1568807068706

The left side of the graph shows the output before the patches were applied
and the right side the output with the patches applied. Just as reference, an
ath9k device in the same building is
https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86740037e0&fullscreen&panelId=5&from=1568782046974&to=1568807068706

Kind regards,
	Sven

Sven Eckelmann (2):
  ath10k: report survey info as accumulated values
  ath10k: regularly fetch survey counters

 drivers/net/wireless/ath/ath10k/core.c |  8 ++++
 drivers/net/wireless/ath/ath10k/core.h |  3 ++
 drivers/net/wireless/ath/ath10k/hw.c   | 13 +++--
 drivers/net/wireless/ath/ath10k/mac.c  | 52 ++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h  |  3 ++
 drivers/net/wireless/ath/ath10k/wmi.c  | 66 ++++++++++++++++++++++----
 6 files changed, 130 insertions(+), 15 deletions(-)

-- 
2.20.1

[RFC PATCH 1/2] ath10k: report survey info as accumulated values

[Sven Eckelmann]

From: Sven Eckelmann <seckelmann@datto.com>

The survey report is expected to contain a counter which is increasing all
the time. But ath10k reports some kind of delta. This can either be the
difference to the last get_survey or the difference to some even older
get_survey because the values are sometimes cached and sometimes
overwritten.

To make the returned values useful, they must be accumulated manually by
ath10k before send out to the upper layers. Special care must be taken when
accepting values from older firmware versions (which use
ATH10K_HW_CC_WRAP_SHIFTED_ALL) because they will not clear the cycle_busy
and cycle_total counter and will only use 31 bit of the 64 but counter.

Tested on QCA988x hw2.0 10.2.4-1.0-00043
Tested on QCA99x0 hw2.0 10.4.1.00030-1
Tested in QCA4019 hw1.0 10.4-3.5.3-00057

Signed-off-by: Sven Eckelmann <seckelmann@datto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |  2 +
 drivers/net/wireless/ath/ath10k/hw.c   | 13 +++--
 drivers/net/wireless/ath/ath10k/wmi.c  | 66 ++++++++++++++++++++++----
 3 files changed, 66 insertions(+), 15 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 4d7db07db6ba..25c699f3a73b 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -1127,6 +1127,8 @@ struct ath10k {
 	u32 survey_last_rx_clear_count;
 	u32 survey_last_cycle_count;
 	struct survey_info survey[ATH10K_NUM_CHANS];
+	u64 survey_last_total_cc[ATH10K_NUM_CHANS];
+	u64 survey_last_busy_cc[ATH10K_NUM_CHANS];
 
 	/* Channel info events are expected to come in pairs without and with
 	 * COMPLETE flag set respectively for each channel visit during scan.
diff --git a/drivers/net/wireless/ath/ath10k/hw.c b/drivers/net/wireless/ath/ath10k/hw.c
index c415e971735b..68ffdb8b8eaa 100644
--- a/drivers/net/wireless/ath/ath10k/hw.c
+++ b/drivers/net/wireless/ath/ath10k/hw.c
@@ -548,9 +548,10 @@ void ath10k_hw_fill_survey_time(struct ath10k *ar, struct survey_info *survey,
 	u32 cc_fix = 0;
 	u32 rcc_fix = 0;
 	enum ath10k_hw_cc_wraparound_type wraparound_type;
+	u32 filled = 0;
 
-	survey->filled |= SURVEY_INFO_TIME |
-			  SURVEY_INFO_TIME_BUSY;
+	filled |= SURVEY_INFO_TIME |
+		  SURVEY_INFO_TIME_BUSY;
 
 	wraparound_type = ar->hw_params.cc_wraparound_type;
 
@@ -559,7 +560,7 @@ void ath10k_hw_fill_survey_time(struct ath10k *ar, struct survey_info *survey,
 		case ATH10K_HW_CC_WRAP_SHIFTED_ALL:
 			if (cc < cc_prev) {
 				cc_fix = 0x7fffffff;
-				survey->filled &= ~SURVEY_INFO_TIME_BUSY;
+				filled &= ~SURVEY_INFO_TIME_BUSY;
 			}
 			break;
 		case ATH10K_HW_CC_WRAP_SHIFTED_EACH:
@@ -577,8 +578,10 @@ void ath10k_hw_fill_survey_time(struct ath10k *ar, struct survey_info *survey,
 	cc -= cc_prev - cc_fix;
 	rcc -= rcc_prev - rcc_fix;
 
-	survey->time = CCNT_TO_MSEC(ar, cc);
-	survey->time_busy = CCNT_TO_MSEC(ar, rcc);
+	survey->filled |= filled;
+	survey->time += CCNT_TO_MSEC(ar, cc);
+	if (survey->filled & SURVEY_INFO_TIME_BUSY)
+		survey->time_busy += CCNT_TO_MSEC(ar, rcc);
 }
 
 /* The firmware does not support setting the coverage class. Instead this
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 4f707c6394bb..19667e2f22fa 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5662,10 +5662,38 @@ static int ath10k_wmi_event_temperature(struct ath10k *ar, struct sk_buff *skb)
 	return 0;
 }
 
+static void ath10k_clean_survey(struct ath10k *ar, struct survey_info *survey,
+				int idx, u64 total, u64 busy)
+{
+	u32 total_diff;
+	u32 busy_diff;
+
+	lockdep_assert_held(&ar->data_lock);
+
+	if (ar->hw_params.cc_wraparound_type != ATH10K_HW_CC_WRAP_SHIFTED_ALL)
+		return;
+
+	if (total < ar->survey_last_total_cc[idx]) {
+		total_diff = total + 0x7fffffff - ar->survey_last_total_cc[idx];
+		busy_diff = 0;
+		survey->filled &= ~SURVEY_INFO_TIME_BUSY;
+	} else {
+		total_diff = total - ar->survey_last_total_cc[idx];
+		busy_diff = busy - ar->survey_last_busy_cc[idx];
+	}
+
+	survey->time      = CCNT_TO_MSEC(ar, total_diff);
+	survey->time_busy = CCNT_TO_MSEC(ar, busy_diff);
+
+	ar->survey_last_total_cc[idx] = total;
+	ar->survey_last_busy_cc[idx] = busy;
+}
+
 static int ath10k_wmi_event_pdev_bss_chan_info(struct ath10k *ar,
 					       struct sk_buff *skb)
 {
 	struct wmi_pdev_bss_chan_info_event *ev;
+	struct survey_info survey_tmp = {};
 	struct survey_info *survey;
 	u64 busy, total, tx, rx, rx_bss;
 	u32 freq, noise_floor;
@@ -5688,6 +5716,13 @@ static int ath10k_wmi_event_pdev_bss_chan_info(struct ath10k *ar,
 		   "wmi event pdev bss chan info:\n freq: %d noise: %d cycle: busy %llu total %llu tx %llu rx %llu rx_bss %llu\n",
 		   freq, noise_floor, busy, total, tx, rx, rx_bss);
 
+	/* everything zero means invalid data
+	 * -> drop it to avoid bogus noisefloor in survey report
+	 */
+	if (noise_floor == 0 && busy == 0 && total == 0 && tx == 0 && rx == 0 &&
+	    rx_bss == 0)
+		return -EPROTO;
+
 	spin_lock_bh(&ar->data_lock);
 	idx = freq_to_idx(ar, freq);
 	if (idx >= ARRAY_SIZE(ar->survey)) {
@@ -5696,18 +5731,29 @@ static int ath10k_wmi_event_pdev_bss_chan_info(struct ath10k *ar,
 		goto exit;
 	}
 
+	/* create delta result - might need fix of counters */
+	survey_tmp.noise     = noise_floor;
+	survey_tmp.time      = div_u64(total, cc_freq_hz);
+	survey_tmp.time_busy = div_u64(busy, cc_freq_hz);
+	survey_tmp.time_rx   = div_u64(rx_bss, cc_freq_hz);
+	survey_tmp.time_tx   = div_u64(tx, cc_freq_hz);
+	survey_tmp.filled    = (SURVEY_INFO_NOISE_DBM |
+				SURVEY_INFO_TIME |
+				SURVEY_INFO_TIME_BUSY |
+				SURVEY_INFO_TIME_RX |
+				SURVEY_INFO_TIME_TX);
+
+	ath10k_clean_survey(ar, &survey_tmp, idx, total, busy);
+
+	/* create accumulated result */
 	survey = &ar->survey[idx];
 
-	survey->noise     = noise_floor;
-	survey->time      = div_u64(total, cc_freq_hz);
-	survey->time_busy = div_u64(busy, cc_freq_hz);
-	survey->time_rx   = div_u64(rx_bss, cc_freq_hz);
-	survey->time_tx   = div_u64(tx, cc_freq_hz);
-	survey->filled   |= (SURVEY_INFO_NOISE_DBM |
-			     SURVEY_INFO_TIME |
-			     SURVEY_INFO_TIME_BUSY |
-			     SURVEY_INFO_TIME_RX |
-			     SURVEY_INFO_TIME_TX);
+	survey->noise      = survey_tmp.noise;
+	survey->time      += survey_tmp.time;
+	survey->time_busy += survey_tmp.time_busy;
+	survey->time_rx   += survey_tmp.time_rx;
+	survey->time_tx   += survey_tmp.time_tx;
+	survey->filled    |= survey_tmp.filled;
 exit:
 	spin_unlock_bh(&ar->data_lock);
 	complete(&ar->bss_survey_done);
-- 
2.20.1

Re: [RFC PATCH 1/2] ath10k: report survey info as accumulated values

[Kalle Valo]

Sven Eckelmann <sven@narfation.org> writes:

> From: Sven Eckelmann <seckelmann@datto.com>
>
> The survey report is expected to contain a counter which is increasing all
> the time. But ath10k reports some kind of delta. This can either be the
> difference to the last get_survey or the difference to some even older
> get_survey because the values are sometimes cached and sometimes
> overwritten.
>
> To make the returned values useful, they must be accumulated manually by
> ath10k before send out to the upper layers. Special care must be taken when
> accepting values from older firmware versions (which use
> ATH10K_HW_CC_WRAP_SHIFTED_ALL) because they will not clear the cycle_busy
> and cycle_total counter and will only use 31 bit of the 64 but counter.

Please clarify a bit more which older firmware versions you are talking
about, we have so many of them :) I assume you are talking about _all_
QCA988X and QCA9887 firmware versions, but it would be nice to clarify
that in the commit log.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[RFC PATCH 2/2] ath10k: regularly fetch survey counters

[Sven Eckelmann]

From: Sven Eckelmann <seckelmann@datto.com>

The survey counters from firmwares like 10.2.4 are not actually using the
full 64 bit. Instead, they only use the lower 31 bit and overflow ever
14-30s. The driver must frequently fetch the survey data and add it to the
survey data storage to avoid this problem and to present meaningful values
to the caller of .get_survey.

It is assumed for now that only the current rx_channel retrieves relevant
updates for the survey data. This should avoid that the bss channel survey
request times out too often.

Tested on QCA988x hw2.0 10.2.4-1.0-00043

Signed-off-by: Sven Eckelmann <seckelmann@datto.com>
---
 drivers/net/wireless/ath/ath10k/core.c |  8 ++++
 drivers/net/wireless/ath/ath10k/core.h |  1 +
 drivers/net/wireless/ath/ath10k/mac.c  | 52 ++++++++++++++++++++++++++
 drivers/net/wireless/ath/ath10k/mac.h  |  3 ++
 4 files changed, 64 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index dc45d16e8d21..754c46047b15 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -2788,8 +2788,14 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
 		goto err_hif_stop;
 	}
 
+	status = ath10k_survey_start(ar);
+	if (status)
+		goto err_debug_stop;
+
 	return 0;
 
+err_debug_stop:
+	ath10k_debug_stop(ar);
 err_hif_stop:
 	ath10k_hif_stop(ar);
 err_htt_rx_detach:
@@ -2829,6 +2835,7 @@ int ath10k_wait_for_suspend(struct ath10k *ar, u32 suspend_opt)
 void ath10k_core_stop(struct ath10k *ar)
 {
 	lockdep_assert_held(&ar->conf_mutex);
+	ath10k_survey_stop(ar);
 	ath10k_debug_stop(ar);
 
 	/* try to suspend target */
@@ -3179,6 +3186,7 @@ struct ath10k *ath10k_core_create(size_t priv_size, struct device *dev,
 	init_completion(&ar->peer_delete_done);
 
 	INIT_DELAYED_WORK(&ar->scan.timeout, ath10k_scan_timeout_work);
+	INIT_DELAYED_WORK(&ar->survey_dwork, ath10k_survey_dwork);
 
 	ar->workqueue = create_singlethread_workqueue("ath10k_wq");
 	if (!ar->workqueue)
diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 25c699f3a73b..66d2a1263898 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -1129,6 +1129,7 @@ struct ath10k {
 	struct survey_info survey[ATH10K_NUM_CHANS];
 	u64 survey_last_total_cc[ATH10K_NUM_CHANS];
 	u64 survey_last_busy_cc[ATH10K_NUM_CHANS];
+	struct delayed_work survey_dwork;
 
 	/* Channel info events are expected to come in pairs without and with
 	 * COMPLETE flag set respectively for each channel visit during scan.
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 12dad659bf68..4190b0148e97 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -24,6 +24,9 @@
 #include "wmi-ops.h"
 #include "wow.h"
 
+/* ms */
+#define ATH10K_SURVEY_INTERVAL 10000
+
 /*********/
 /* Rates */
 /*********/
@@ -7152,6 +7155,55 @@ ath10k_mac_update_bss_chan_survey(struct ath10k *ar,
 	}
 }
 
+static void ath10k_request_survey(struct ath10k *ar)
+{
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (ar->state != ATH10K_STATE_ON)
+		return;
+
+	if (!ar->rx_channel)
+		return;
+
+	ath10k_mac_update_bss_chan_survey(ar, ar->rx_channel);
+}
+
+void ath10k_survey_dwork(struct work_struct *work)
+{
+	struct ath10k *ar = container_of(work, struct ath10k,
+					 survey_dwork.work);
+
+	mutex_lock(&ar->conf_mutex);
+	ath10k_request_survey(ar);
+	mutex_unlock(&ar->conf_mutex);
+
+	queue_delayed_work(ar->workqueue, &ar->survey_dwork,
+			   msecs_to_jiffies(ATH10K_SURVEY_INTERVAL));
+}
+
+int ath10k_survey_start(struct ath10k *ar)
+{
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (ar->hw_params.cc_wraparound_type != ATH10K_HW_CC_WRAP_SHIFTED_ALL)
+		return 0;
+
+	queue_delayed_work(ar->workqueue, &ar->survey_dwork,
+			   msecs_to_jiffies(ATH10K_SURVEY_INTERVAL));
+
+	return 0;
+}
+
+void ath10k_survey_stop(struct ath10k *ar)
+{
+	lockdep_assert_held(&ar->conf_mutex);
+
+	if (ar->hw_params.cc_wraparound_type != ATH10K_HW_CC_WRAP_SHIFTED_ALL)
+		return;
+
+	cancel_delayed_work_sync(&ar->survey_dwork);
+}
+
 static int ath10k_get_survey(struct ieee80211_hw *hw, int idx,
 			     struct survey_info *survey)
 {
diff --git a/drivers/net/wireless/ath/ath10k/mac.h b/drivers/net/wireless/ath/ath10k/mac.h
index 1fe84948b868..17e4d65edbe0 100644
--- a/drivers/net/wireless/ath/ath10k/mac.h
+++ b/drivers/net/wireless/ath/ath10k/mac.h
@@ -40,6 +40,9 @@ void ath10k_offchan_tx_purge(struct ath10k *ar);
 void ath10k_offchan_tx_work(struct work_struct *work);
 void ath10k_mgmt_over_wmi_tx_purge(struct ath10k *ar);
 void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work);
+void ath10k_survey_dwork(struct work_struct *work);
+int ath10k_survey_start(struct ath10k *ar);
+void ath10k_survey_stop(struct ath10k *ar);
 void ath10k_halt(struct ath10k *ar);
 void ath10k_mac_vif_beacon_free(struct ath10k_vif *arvif);
 void ath10k_drain_tx(struct ath10k *ar);
-- 
2.20.1

Re: [RFC PATCH 2/2] ath10k: regularly fetch survey counters

[Kalle Valo]

Sven Eckelmann <sven@narfation.org> writes:

> From: Sven Eckelmann <seckelmann@datto.com>
>
> The survey counters from firmwares like 10.2.4 are not actually using the
> full 64 bit. Instead, they only use the lower 31 bit and overflow ever
> 14-30s. The driver must frequently fetch the survey data and add it to the
> survey data storage to avoid this problem and to present meaningful values
> to the caller of .get_survey.
>
> It is assumed for now that only the current rx_channel retrieves relevant
> updates for the survey data. This should avoid that the bss channel survey
> request times out too often.

Please clarify in the commit log on which hardware this workaround is
enabled. It should be all hardware with ATH10K_HW_CC_WRAP_SHIFTED_ALL
enabled so both QCA988X versions and QCA9887, right?

> Tested on QCA988x hw2.0 10.2.4-1.0-00043
>
> Signed-off-by: Sven Eckelmann <seckelmann@datto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/mac.h
> +++ b/drivers/net/wireless/ath/ath10k/mac.h
> @@ -40,6 +40,9 @@ void ath10k_offchan_tx_purge(struct ath10k *ar);
>  void ath10k_offchan_tx_work(struct work_struct *work);
>  void ath10k_mgmt_over_wmi_tx_purge(struct ath10k *ar);
>  void ath10k_mgmt_over_wmi_tx_work(struct work_struct *work);
> +void ath10k_survey_dwork(struct work_struct *work);
> +int ath10k_survey_start(struct ath10k *ar);
> +void ath10k_survey_stop(struct ath10k *ar);

Please use ath10k_mac_ prefix for all functions you are adding to mac.c.
Yeah, I know not all the existing one even have that, should fix it at
some point.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Kalle Valo]

Sven Eckelmann <sven@narfation.org> writes:

> it was observed that ath9k provides accumulated survey counters but ath10k
> neither provides deltas nor accumulated counters. Instead it returns
> some value which was returned at some point from the firmware.
>
> But as it turns out, this data is not reliable. To make it more useful,
> ath10k has to:
>
> * retrieve counters rather frequently for hardware which is known to use
>   firmware versions with low number counter bits (for only 14-30s)
> * clean up received counter values 
> * accumulate counters from firmware
>
> A comparison of the resulting output with these fixes can be seen under
> https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86749f4d60&fullscreen&panelId=5&from=1568782046974&to=1568807068706
>
> The left side of the graph shows the output before the patches were applied
> and the right side the output with the patches applied. Just as reference, an
> ath9k device in the same building is
> https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86740037e0&fullscreen&panelId=5&from=1568782046974&to=1568807068706

Thanks, this looks very good to me and I had only cosmetic comments.
Please submit next version without RFC so that I can apply these.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Sebastian Gottschall]

i checked your patch on 10.4 based chipsets with 9984. the values are 
now looking bogus and wrong at all. busy and active time time in ms does 
increase in hours each second
the problem seem to be that your patch is 10.2.4 only related. 
ath_clean_survey does not trigger on 10.4 so the values double itself 
each time the event raises since you add the full values and not just a 
delta on top

Sebastian

Am 18.09.2019 um 14:42 schrieb Sven Eckelmann:
> From: Sven Eckelmann <seckelmann@datto.com>
>
> Hi,
>
> it was observed that ath9k provides accumulated survey counters but ath10k
> neither provides deltas nor accumulated counters. Instead it returns
> some value which was returned at some point from the firmware.
>
> But as it turns out, this data is not reliable. To make it more useful,
> ath10k has to:
>
> * retrieve counters rather frequently for hardware which is known to use
>    firmware versions with low number counter bits (for only 14-30s)
> * clean up received counter values
> * accumulate counters from firmware
>
> A comparison of the resulting output with these fixes can be seen under
> https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86749f4d60&fullscreen&panelId=5&from=1568782046974&to=1568807068706
>
> The left side of the graph shows the output before the patches were applied
> and the right side the output with the patches applied. Just as reference, an
> ath9k device in the same building is
> https://stats.freifunk-vogtland.net/d/ffv_node/nodeinfo?orgId=1&var-node=ac86740037e0&fullscreen&panelId=5&from=1568782046974&to=1568807068706
>
> Kind regards,
> 	Sven
>
> Sven Eckelmann (2):
>    ath10k: report survey info as accumulated values
>    ath10k: regularly fetch survey counters
>
>   drivers/net/wireless/ath/ath10k/core.c |  8 ++++
>   drivers/net/wireless/ath/ath10k/core.h |  3 ++
>   drivers/net/wireless/ath/ath10k/hw.c   | 13 +++--
>   drivers/net/wireless/ath/ath10k/mac.c  | 52 ++++++++++++++++++++
>   drivers/net/wireless/ath/ath10k/mac.h  |  3 ++
>   drivers/net/wireless/ath/ath10k/wmi.c  | 66 ++++++++++++++++++++++----
>   6 files changed, 130 insertions(+), 15 deletions(-)
>

Re: [RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]

On Monday, 14 October 2019 00:15:20 CEST Sebastian Gottschall wrote:
> i checked your patch on 10.4 based chipsets with 9984. the values are 
> now looking bogus and wrong at all. busy and active time time in ms does 
> increase in hours each second
> the problem seem to be that your patch is 10.2.4 only related. 
> ath_clean_survey does not trigger on 10.4 so the values double itself 
> each time the event raises since you add the full values and not just a 
> delta on top

Thanks for the feedback. So we have now a firmware 10.2.4 which is counting 
busy + active up and has wraparound problems. And then we have a 10.4 firmware 
(on QCA9888 and QCA4019) which is clearing everything as expected with 
WMI_BSS_SURVEY_REQ_TYPE_READ_CLEAR  and then we have some 10.4 firmware (one 
QCA9984) which behaves more like ath 10.2.4 firmware but is marked as 
ATH10K_HW_CC_WRAP_SHIFTED_EACH like the QCA4019.

So I have no idea how to fix this when QCA4019 and QCA9984 are currently 
marked the same but behave differently. Does somebody have a overview how the 
different HW versions should behave or is there some special bit in the data 
reported by the firmware which can be used to evaluate the expected behavior?

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Kalle Valo]

Sven Eckelmann <sven@narfation.org> writes:

> On Monday, 14 October 2019 00:15:20 CEST Sebastian Gottschall wrote:
>> i checked your patch on 10.4 based chipsets with 9984. the values are 
>> now looking bogus and wrong at all. busy and active time time in ms does 
>> increase in hours each second
>> the problem seem to be that your patch is 10.2.4 only related. 
>> ath_clean_survey does not trigger on 10.4 so the values double itself 
>> each time the event raises since you add the full values and not just a 
>> delta on top
>
> Thanks for the feedback. So we have now a firmware 10.2.4 which is counting 
> busy + active up and has wraparound problems. And then we have a 10.4 firmware 
> (on QCA9888 and QCA4019) which is clearing everything as expected with 
> WMI_BSS_SURVEY_REQ_TYPE_READ_CLEAR  and then we have some 10.4 firmware (one 
> QCA9984) which behaves more like ath 10.2.4 firmware but is marked as 
> ATH10K_HW_CC_WRAP_SHIFTED_EACH like the QCA4019.
>
> So I have no idea how to fix this when QCA4019 and QCA9984 are currently 
> marked the same but behave differently. Does somebody have a overview how the 
> different HW versions should behave or is there some special bit in the data 
> reported by the firmware which can be used to evaluate the expected behavior?

I hope there's an easy way to detect this behaviour change, but if
nothing else we could add a new bit to enum ath10k_fw_features. But of
course that's the last resort, maintaining the firmware features
bitfield accross different firmware branches is quite cumbersome.

-- 
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [RFC PATCH 0/2] ath10k: provide survey info as accumulated data

[Sebastian Gottschall]

10.4 has additional 64 bit cycle counters. see wmi_pdev_bss_chan_info_event

the standard 32 cycle counters do individual wrap around as far as i know

Am 14.10.2019 um 09:07 schrieb Sven Eckelmann:
> On Monday, 14 October 2019 00:15:20 CEST Sebastian Gottschall wrote:
>> i checked your patch on 10.4 based chipsets with 9984. the values are
>> now looking bogus and wrong at all. busy and active time time in ms does
>> increase in hours each second
>> the problem seem to be that your patch is 10.2.4 only related.
>> ath_clean_survey does not trigger on 10.4 so the values double itself
>> each time the event raises since you add the full values and not just a
>> delta on top
> Thanks for the feedback. So we have now a firmware 10.2.4 which is 
> counting
> busy + active up and has wraparound problems. And then we have a 10.4 
> firmware
> (on QCA9888 and QCA4019) which is clearing everything as expected with
> WMI_BSS_SURVEY_REQ_TYPE_READ_CLEAR  and then we have some 10.4 
> firmware (one
> QCA9984) which behaves more like ath 10.2.4 firmware but is marked as
> ATH10K_HW_CC_WRAP_SHIFTED_EACH like the QCA4019.
>
> So I have no idea how to fix this when QCA4019 and QCA9984 are currently
> marked the same but behave differently. Does somebody have a overview 
> how the
> different HW versions should behave or is there some special bit in 
> the data
> reported by the firmware which can be used to evaluate the expected 
> behavior?
>
> Kind regards,
>     Sven