[PATCH] ath10k: fix printing of peer stats in non-AP firmware

[PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Chun-Yeow Yeoh]

This patch is intended to fix the problem if we use the
firmware 999.999.0.636 to get peer stats when the number
of peer is more than 3. The WMI_UPDATE_STATS_EVENTID may
trigger more than 1 time if the number of peers is more
than 3. So this patch allows us to do the checking on this
and make sure that we print the peer stats correctly.

Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
---
 drivers/net/wireless/ath/ath10k/debug.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 7be284c..4a4072e 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -245,10 +245,17 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
 	if (num_peer_stats) {
 		struct wmi_peer_stats_10x *peer_stats;
 		struct ath10k_peer_stat *s;
+		int j = 0;
+
+		if (!test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features) &&
+		    !num_pdev_stats) {
+			j = 3;
+			num_peer_stats += 3;
+		}
 
 		stats->peers = num_peer_stats;
 
-		for (i = 0; i < num_peer_stats; i++) {
+		for (i = j; i < num_peer_stats; i++) {
 			peer_stats = (struct wmi_peer_stats_10x *)tmp;
 			s = &stats->peer_stat[i];
 
-- 
1.7.9.5

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Michal Kazior]

On 25 March 2014 08:55, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> wrote:
> This patch is intended to fix the problem if we use the
> firmware 999.999.0.636 to get peer stats when the number
> of peer is more than 3. The WMI_UPDATE_STATS_EVENTID may
> trigger more than 1 time if the number of peers is more
> than 3. So this patch allows us to do the checking on this
> and make sure that we print the peer stats correctly.
>
> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
> ---
>  drivers/net/wireless/ath/ath10k/debug.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 7be284c..4a4072e 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -245,10 +245,17 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
>         if (num_peer_stats) {
>                 struct wmi_peer_stats_10x *peer_stats;
>                 struct ath10k_peer_stat *s;
> +               int j = 0;
> +
> +               if (!test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features) &&
> +                   !num_pdev_stats) {
> +                       j = 3;
> +                       num_peer_stats += 3;
> +               }
>
>                 stats->peers = num_peer_stats;
>
> -               for (i = 0; i < num_peer_stats; i++) {
> +               for (i = j; i < num_peer_stats; i++) {
>                         peer_stats = (struct wmi_peer_stats_10x *)tmp;
>                         s = &stats->peer_stat[i];

I suppose there's a limit how much peer entries firmware is able to
pack up into a single event. The second event you handle here is still
going to have some sort of a limit and you'll end up dropping results
again. The same probably goes with vdev stats (which aren't handled
yet, but hey).

I think a better approach would be to make read_target_stats *append*
data to the stats structure and make ath10k wait for more possible
events instead of being satisfied with just one.

The way you do it now seems like you end up with the extra results
being shown upon *next* stat request-response.. i.e. you see old
results.


Michał

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Yeoh Chun-Yeow]

Hi, Michal

> I suppose there's a limit how much peer entries firmware is able to
> pack up into a single event. The second event you handle here is still
> going to have some sort of a limit and you'll end up dropping results
> again. The same probably goes with vdev stats (which aren't handled
> yet, but hey).

vdev stats is not supported by firmware 999.999.0.636, am I right?
BTW, I also saw that the same happen to latest AP firmware whenever
the peer stats is more than 1. But since the first peer stats is self
stats, so it is ok to overwrite it.

> I think a better approach would be to make read_target_stats *append*
> data to the stats structure and make ath10k wait for more possible
> events instead of being satisfied with just one.
>
> The way you do it now seems like you end up with the extra results
> being shown upon *next* stat request-response.. i.e. you see old
> results.

Will take a look on this.

----
Chun-Yeow

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Michal Kazior]

On 25 March 2014 09:33, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
> Hi, Michal
>
>> I suppose there's a limit how much peer entries firmware is able to
>> pack up into a single event. The second event you handle here is still
>> going to have some sort of a limit and you'll end up dropping results
>> again. The same probably goes with vdev stats (which aren't handled
>> yet, but hey).
>
> vdev stats is not supported by firmware 999.999.0.636, am I right?

True, but once it is supported then it will most likely need the same
treatment as peer stats.


> BTW, I also saw that the same happen to latest AP firmware whenever
> the peer stats is more than 1. But since the first peer stats is self
> stats, so it is ok to overwrite it.

I disagree. Why do you think it is okay to overwrite self peer stats?

Btw. 10.1 firmware has more pdev stats which breaks parsing of peer entries...


Michał

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Yeoh Chun-Yeow]

>
>> BTW, I also saw that the same happen to latest AP firmware whenever
>> the peer stats is more than 1. But since the first peer stats is self
>> stats, so it is ok to overwrite it.
>
> I disagree. Why do you think it is okay to overwrite self peer stats?
>
Because TxRate, RxRate, RSSI are all no value, just the MAC address.

> Btw. 10.1 firmware has more pdev stats which breaks parsing of peer entries...

Yes, let me investigate further.

-----
Chun-Yeow

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Michal Kazior]

On 25 March 2014 09:54, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
>>
>>> BTW, I also saw that the same happen to latest AP firmware whenever
>>> the peer stats is more than 1. But since the first peer stats is self
>>> stats, so it is ok to overwrite it.
>>
>> I disagree. Why do you think it is okay to overwrite self peer stats?
>>
> Because TxRate, RxRate, RSSI are all no value, just the MAC address.

I don't think that's true for 10.1 firmware and AP interface types.


Michał

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Yeoh Chun-Yeow]

> I don't think that's true for 10.1 firmware and AP interface types.

I try to play around with 10.1 firmware as AP (3 connected STAs) and I
get the following:

The first WMI_UPDATE_STATS_EVENTID as follow:
[  338.290000] ath10k: pdev 0 vdev 0 peer 3
[  338.300000] ath10k: MAC 04:f0:21:0c:a5:44
[  338.300000] ath10k: RSSI 60
[  338.300000] ath10k: Tx 1170000
[  338.310000] ath10k: Rx 1170000
[  338.310000] ath10k: MAC 04:f0:21:0c:a5:19
[  338.320000] ath10k: RSSI 64
[  338.320000] ath10k: Tx 1300000
[  338.320000] ath10k: Rx 975000
[  338.320000] ath10k: MAC 04:f0:21:0c:a5:1c
[  338.330000] ath10k: RSSI 71
[  338.330000] ath10k: Tx 975000
[  338.330000] ath10k: Rx 1300000

I think that above is correct.

The second WMI_UPDATE_STATS_EVENTID as follow:
[  338.340000] ath10k: pdev 1 vdev 0 peer 1
[  338.350000] ath10k: MAC a8:02:00:00:00:00
[  338.350000] ath10k: RSSI 0
[  338.350000] ath10k: Tx 565
[  338.360000] ath10k: Rx 0

Although indicating 1 peer, but the data seems to be not the peer
stats, not even self STA stats. Any idea?

---
Chun-Yeow

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Michal Kazior]

On 25 March 2014 10:38, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
>> I don't think that's true for 10.1 firmware and AP interface types.
>
> I try to play around with 10.1 firmware as AP (3 connected STAs) and I
> get the following:
>
> The first WMI_UPDATE_STATS_EVENTID as follow:
> [ 338.290000] ath10k: pdev 0 vdev 0 peer 3
> [  338.300000] ath10k: MAC 04:f0:21:0c:a5:44
> [  338.300000] ath10k: RSSI 60
> [  338.300000] ath10k: Tx 1170000
> [  338.310000] ath10k: Rx 1170000
> [  338.310000] ath10k: MAC 04:f0:21:0c:a5:19
> [  338.320000] ath10k: RSSI 64
> [  338.320000] ath10k: Tx 1300000
> [  338.320000] ath10k: Rx 975000
> [  338.320000] ath10k: MAC 04:f0:21:0c:a5:1c
> [  338.330000] ath10k: RSSI 71
> [  338.330000] ath10k: Tx 975000
> [  338.330000] ath10k: Rx 1300000
>
> I think that above is correct.
>
> The second WMI_UPDATE_STATS_EVENTID as follow:
> [  338.340000] ath10k: pdev 1 vdev 0 peer 1
> [  338.350000] ath10k: MAC a8:02:00:00:00:00
> [  338.350000] ath10k: RSSI 0
> [  338.350000] ath10k: Tx 565
> [  338.360000] ath10k: Rx 0
>
> Although indicating 1 peer, but the data seems to be not the peer
> stats, not even self STA stats. Any idea?

Yes.

The second stats event has pdev stats. As per my other mail, 10.1 has
extra pdev stats. Since ath10k doesn't account that it reads peer
stats too early from the buffer. What you see is a tail of pdev stats
for 10.1.

You can try the following to test *10.1*:

--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -2804,6 +2804,12 @@ struct wmi_pdev_stats {
        __le32 phy_err_count;  /* Phy error count */
        __le32 chan_tx_pwr;    /* channel tx power */
        struct wal_dbg_stats wal; /* WAL dbg stats */
+       __le32 ack_rx_bad;
+       __le32 rts_bad;
+       __le32 rts_good;
+       __le32 fcs_bad;
+       __le32 no_beacons;
+       __le32 mib_int_count;
 } __packed;


Michał

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Yeoh Chun-Yeow]

>
> The second stats event has pdev stats. As per my other mail, 10.1 has
> extra pdev stats. Since ath10k doesn't account that it reads peer
> stats too early from the buffer. What you see is a tail of pdev stats
> for 10.1.
>
> You can try the following to test *10.1*:
>
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -2804,6 +2804,12 @@ struct wmi_pdev_stats {
>         __le32 phy_err_count;  /* Phy error count */
>         __le32 chan_tx_pwr;    /* channel tx power */
>         struct wal_dbg_stats wal; /* WAL dbg stats */
> +       __le32 ack_rx_bad;
> +       __le32 rts_bad;
> +       __le32 rts_good;
> +       __le32 fcs_bad;
> +       __le32 no_beacons;
> +       __le32 mib_int_count;
>  } __packed;
>

Ok. Probably can come out with a patch extending the pdev stats on this.

I get the self STA stats as follow on 10.1 firmware:

AP:
[  754.280000] ath10k: pdev 1 vdev 0 peer 1
[  754.280000] ath10k: MAC 04:f0:21:0c:a5:43
[  754.280000] ath10k: RSSI 0
[  754.290000] ath10k: Tx 6000
[  754.290000] ath10k: Rx 0

STA:
[  880.060000] ath10k: pdev 1 vdev 0 peer 1
[  880.060000] ath10k: MAC 00:00:00:00:00:01
[  880.070000] ath10k: RSSI 0
[  880.070000] ath10k: Tx 6000
[  880.070000] ath10k: Rx 0

Similar to 636. But on 636, I don't find any tx rate, only rssi.

---
Chun-Yeow

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Ben Greear]

On 03/25/2014 01:11 AM, Michal Kazior wrote:
> On 25 March 2014 08:55, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> wrote:
>> This patch is intended to fix the problem if we use the
>> firmware 999.999.0.636 to get peer stats when the number
>> of peer is more than 3. The WMI_UPDATE_STATS_EVENTID may
>> trigger more than 1 time if the number of peers is more
>> than 3. So this patch allows us to do the checking on this
>> and make sure that we print the peer stats correctly.
>>
>> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/debug.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
>> index 7be284c..4a4072e 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -245,10 +245,17 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
>>         if (num_peer_stats) {
>>                 struct wmi_peer_stats_10x *peer_stats;
>>                 struct ath10k_peer_stat *s;
>> +               int j = 0;
>> +
>> +               if (!test_bit(ATH10K_FW_FEATURE_WMI_10X, ar->fw_features) &&
>> +                   !num_pdev_stats) {
>> +                       j = 3;
>> +                       num_peer_stats += 3;
>> +               }
>>
>>                 stats->peers = num_peer_stats;
>>
>> -               for (i = 0; i < num_peer_stats; i++) {
>> +               for (i = j; i < num_peer_stats; i++) {
>>                         peer_stats = (struct wmi_peer_stats_10x *)tmp;
>>                         s = &stats->peer_stat[i];
> 
> I suppose there's a limit how much peer entries firmware is able to
> pack up into a single event. The second event you handle here is still
> going to have some sort of a limit and you'll end up dropping results
> again. The same probably goes with vdev stats (which aren't handled
> yet, but hey).

At least in my 10.x 389 firmware, the limit is 11 peers.  There is some
round-robin logic that means next request of stats gets the next set of peer
stats, so calling get-stats multiple times would get the entire list of peers.

I'm not sure how to deal with this in the driver.  In my case, my multiple
stations mostly connect to the same AP, so peer-mac is often the same.
This makes it difficult for me to know if the round-robin logic is working
properly, but I think we might as well assume it does until proven otherwise.
I may try to poke a station identifier into the 16 unused bits of the mac-addr
struct eventually, but unless upstream firmware does the same, that doesn't
help any general issue...

Thanks,
Ben



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

Re: [PATCH] ath10k: fix printing of peer stats in non-AP firmware

[Kalle Valo]

Chun-Yeow Yeoh <yeohchunyeow@gmail.com> writes:

> This patch is intended to fix the problem if we use the
> firmware 999.999.0.636 to get peer stats when the number
> of peer is more than 3. The WMI_UPDATE_STATS_EVENTID may
> trigger more than 1 time if the number of peers is more
> than 3. So this patch allows us to do the checking on this
> and make sure that we print the peer stats correctly.
>
> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>

Based on Michal's comments I'm going to drop this one, we need to find a
better way to solve this.

-- 
Kalle Valo