2014-01-10 09:19:43

by Chun-Yeow Yeoh

[permalink] [raw]
Subject: [PATCH v2] ath10k: fix the MAC address of peer statistic

Fix the MAC address of wmi_peer_stats so that it is
printed correctly. This is tested and verified using
firmware version 999.999.0.636.

Based on the verification, maximum only 3 peer statistics including
self STA able to be printed out.

Signed-off-by: Chun-Yeow Yeoh <[email protected]>
---
v2: offset the stats to ignore the first peer (Chun-Yeow)

drivers/net/wireless/ath/ath10k/debug.c | 4 ++++
drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++-------
2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
index 6bdfad3..b39bad8 100644
--- a/drivers/net/wireless/ath/ath10k/debug.c
+++ b/drivers/net/wireless/ath/ath10k/debug.c
@@ -242,6 +242,10 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
}
}

+ /* The first peer is self MAC address, ignore this */
+ num_peer_stats--;
+ tmp += sizeof(struct wmi_peer_stats);
+
if (num_peer_stats) {
struct wmi_peer_stats *peer_stats;
struct ath10k_peer_stat *s;
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 0087d69..106a23e 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -200,12 +200,12 @@ struct wmi_mac_addr {

/* macro to convert MAC address from WMI word format to char array */
#define WMI_MAC_ADDR_TO_CHAR_ARRAY(pwmi_mac_addr, c_macaddr) do { \
- (c_macaddr)[0] = ((pwmi_mac_addr)->word0) & 0xff; \
- (c_macaddr)[1] = (((pwmi_mac_addr)->word0) >> 8) & 0xff; \
- (c_macaddr)[2] = (((pwmi_mac_addr)->word0) >> 16) & 0xff; \
- (c_macaddr)[3] = (((pwmi_mac_addr)->word0) >> 24) & 0xff; \
- (c_macaddr)[4] = ((pwmi_mac_addr)->word1) & 0xff; \
- (c_macaddr)[5] = (((pwmi_mac_addr)->word1) >> 8) & 0xff; \
+ (c_macaddr)[0] = (((pwmi_mac_addr)->word0) >> 24) & 0xff; \
+ (c_macaddr)[1] = (((pwmi_mac_addr)->word0) >> 16) & 0xff; \
+ (c_macaddr)[2] = (((pwmi_mac_addr)->word0) >> 8) & 0xff; \
+ (c_macaddr)[3] = ((pwmi_mac_addr)->word0) & 0xff; \
+ (c_macaddr)[4] = (((pwmi_mac_addr)->word1) >> 24) & 0xff; \
+ (c_macaddr)[5] = (((pwmi_mac_addr)->word1) >> 16) & 0xff; \
} while (0)

struct wmi_cmd_map {
@@ -2820,9 +2820,9 @@ struct wmi_vdev_stats {
* TODO: add more stats
*/
struct wmi_peer_stats {
+ __le32 peer_tx_rate; /* TBA */
struct wmi_mac_addr peer_macaddr;
__le32 peer_rssi;
- __le32 peer_tx_rate;
} __packed;

struct wmi_vdev_create_cmd {
--
1.7.9.5



2014-01-17 13:38:01

by Michal Kazior

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

On 10 January 2014 10:19, Chun-Yeow Yeoh <[email protected]> wrote:
> Fix the MAC address of wmi_peer_stats so that it is
> printed correctly. This is tested and verified using
> firmware version 999.999.0.636.
>
> Based on the verification, maximum only 3 peer statistics including
> self STA able to be printed out.
>
> Signed-off-by: Chun-Yeow Yeoh <[email protected]>
> ---
> v2: offset the stats to ignore the first peer (Chun-Yeow)
>
> drivers/net/wireless/ath/ath10k/debug.c | 4 ++++
> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++-------
> 2 files changed, 11 insertions(+), 7 deletions(-)
>

[...]

> @@ -2820,9 +2820,9 @@ struct wmi_vdev_stats {
> * TODO: add more stats
> */
> struct wmi_peer_stats {
> + __le32 peer_tx_rate; /* TBA */
> struct wmi_mac_addr peer_macaddr;
> __le32 peer_rssi;
> - __le32 peer_tx_rate;
> } __packed;

This looks wrong too. I've just checked it and it seems the
`wal_dbg_tx_stats` is out of date. It's missing a `__le32
stateless_tid_alloc_failure` that has been inserted at some point in
time in the firmware between `pdev_resets` and `phy_underrun`. You
should add this missing field instead of moving `peer_tx_rate`.

Unfortunately AP firmware has even more fields inserted in
`wmi_pdev_stats`. Making this work with both firmwares is a mess.


Michał

2014-01-17 13:11:26

by Chun-Yeow Yeoh

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

> Any ideas what's happening here?

My one is MIPS32 (Compex WPJ344) and the following PEER stats are
printed correctly:

# cat /sys/kernel/debug/ieee80211/phy0/ath10k/fw_stats

MPDU errors (FCS, MIC, ENC) 0

ath10k PEER stats
=================

Peer MAC address 04:f0:21:0c:a5:44
Peer RSSI 68
Peer TX rate 0

Could this be the Endianness problem? How about if you remain the
struct wmi_peer_stats? Also, just to point out the TxRate is not
working (always 0).


>> + /* The first peer is self MAC address, ignore this */
>> + num_peer_stats--;
>> + tmp += sizeof(struct wmi_peer_stats);
>
> Should we show "self peer" separately? Does it provide any useful
> information?

So far, we only have RSSI and Tx Rate. Both are not really useful for
"self peer".

----
Chun-Yeow

2014-01-17 13:18:45

by Michal Kazior

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

On 10 January 2014 10:19, Chun-Yeow Yeoh <[email protected]> wrote:
> Fix the MAC address of wmi_peer_stats so that it is
> printed correctly. This is tested and verified using
> firmware version 999.999.0.636.
>
> Based on the verification, maximum only 3 peer statistics including
> self STA able to be printed out.
>
> Signed-off-by: Chun-Yeow Yeoh <[email protected]>
> ---
> v2: offset the stats to ignore the first peer (Chun-Yeow)
>
> drivers/net/wireless/ath/ath10k/debug.c | 4 ++++
> drivers/net/wireless/ath/ath10k/wmi.h | 14 +++++++-------
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 6bdfad3..b39bad8 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -242,6 +242,10 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
> }
> }
>
> + /* The first peer is self MAC address, ignore this */
> + num_peer_stats--;
> + tmp += sizeof(struct wmi_peer_stats);
> +
> if (num_peer_stats) {
> struct wmi_peer_stats *peer_stats;
> struct ath10k_peer_stat *s;
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index 0087d69..106a23e 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -200,12 +200,12 @@ struct wmi_mac_addr {
>
> /* macro to convert MAC address from WMI word format to char array */
> #define WMI_MAC_ADDR_TO_CHAR_ARRAY(pwmi_mac_addr, c_macaddr) do { \
> - (c_macaddr)[0] = ((pwmi_mac_addr)->word0) & 0xff; \
> - (c_macaddr)[1] = (((pwmi_mac_addr)->word0) >> 8) & 0xff; \
> - (c_macaddr)[2] = (((pwmi_mac_addr)->word0) >> 16) & 0xff; \
> - (c_macaddr)[3] = (((pwmi_mac_addr)->word0) >> 24) & 0xff; \
> - (c_macaddr)[4] = ((pwmi_mac_addr)->word1) & 0xff; \
> - (c_macaddr)[5] = (((pwmi_mac_addr)->word1) >> 8) & 0xff; \
> + (c_macaddr)[0] = (((pwmi_mac_addr)->word0) >> 24) & 0xff; \
> + (c_macaddr)[1] = (((pwmi_mac_addr)->word0) >> 16) & 0xff; \
> + (c_macaddr)[2] = (((pwmi_mac_addr)->word0) >> 8) & 0xff; \
> + (c_macaddr)[3] = ((pwmi_mac_addr)->word0) & 0xff; \
> + (c_macaddr)[4] = (((pwmi_mac_addr)->word1) >> 24) & 0xff; \
> + (c_macaddr)[5] = (((pwmi_mac_addr)->word1) >> 16) & 0xff; \
> } while (0)

This is totally wrong. This macro shouldn't be used anymore. It
shouldn't even be here. There's no hardware byte-swapping anymore.
This means mac addresses received from firmware should be treated
as-is, e.g. with memcpy(). See wmi_mac_addr definition - it has
`addr[6]` field which should be used.


Michał

2014-01-18 10:05:34

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

Yeoh Chun-Yeow <[email protected]> writes:

>> This reminds me that Bartosz sent something related to this few months back:
>>
>> http://lists.infradead.org/pipermail/ath10k/2013-September/000317.html
>>
>
> Thanks, Michal and Kalle Valo for both your comments
>
> I just would like to know which version of firmware should we use?

It depends what you want to do. 10.1 firmare supports AP mode best, but
other modes are not supported or are otherwise buggy.

The main firmware "999.x" supports all modes, but AP mode support is not
that good as in 10.1.

--
Kalle Valo

2014-01-17 12:25:08

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

Chun-Yeow Yeoh <[email protected]> writes:

> Fix the MAC address of wmi_peer_stats so that it is
> printed correctly. This is tested and verified using
> firmware version 999.999.0.636.
>
> Based on the verification, maximum only 3 peer statistics including
> self STA able to be printed out.
>
> Signed-off-by: Chun-Yeow Yeoh <[email protected]>
> ---
> v2: offset the stats to ignore the first peer (Chun-Yeow)

I think something is wrong still. I tested this with 999.999.0.636 on
x86 32 bit laptop in AP mode, connected STA 00:24:d7:9b:0b:7c to it and
still MAC address is wrong:

PHY errors drops 0
MPDU errors (FCS, MIC, ENC) 0

ath10k PEER stats
=================

Peer MAC address 9b:d7:24:00:00:00
Peer RSSI 36
Peer TX rate 0

Without your patch it's also wrong:

ath10k PEER stats
=================

Peer MAC address 00:00:00:00:02:00
Peer RSSI 3
Peer TX rate 0

Peer MAC address 00:00:00:00:00:24
Peer RSSI 31755
Peer TX rate 0

Any ideas what's happening here?

> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -242,6 +242,10 @@ void ath10k_debug_read_target_stats(struct ath10k *ar,
> }
> }
>
> + /* The first peer is self MAC address, ignore this */
> + num_peer_stats--;
> + tmp += sizeof(struct wmi_peer_stats);

Should we show "self peer" separately? Does it provide any useful
information?

--
Kalle Valo

2014-01-18 09:53:20

by Chun-Yeow Yeoh

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

> This reminds me that Bartosz sent something related to this few months back:
>
> http://lists.infradead.org/pipermail/ath10k/2013-September/000317.html
>

Thanks, Michal and Kalle Valo for both your comments

I just would like to know which version of firmware should we use?

----
Chun-Yeow

2014-01-17 13:40:46

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2] ath10k: fix the MAC address of peer statistic

Michal Kazior <[email protected]> writes:

> On 10 January 2014 10:19, Chun-Yeow Yeoh <[email protected]> wrote:
>
>> struct wmi_peer_stats {
>> + __le32 peer_tx_rate; /* TBA */
>> struct wmi_mac_addr peer_macaddr;
>> __le32 peer_rssi;
>> - __le32 peer_tx_rate;
>> } __packed;
>
> This looks wrong too. I've just checked it and it seems the
> `wal_dbg_tx_stats` is out of date. It's missing a `__le32
> stateless_tid_alloc_failure` that has been inserted at some point in
> time in the firmware between `pdev_resets` and `phy_underrun`. You
> should add this missing field instead of moving `peer_tx_rate`.
>
> Unfortunately AP firmware has even more fields inserted in
> `wmi_pdev_stats`. Making this work with both firmwares is a mess.

This reminds me that Bartosz sent something related to this few months back:

http://lists.infradead.org/pipermail/ath10k/2013-September/000317.html

I promised to finish it, but it's still in my todo folder :/

--
Kalle Valo