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

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

[Chun-Yeow Yeoh]

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 <yeohchunyeow@gmail.com>
---
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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kalle Valo]

Chun-Yeow Yeoh <yeohchunyeow@gmail.com> 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 <yeohchunyeow@gmail.com>
> ---
> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Yeoh Chun-Yeow]

> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Michal Kazior]

On 10 January 2014 10:19, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> 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 <yeohchunyeow@gmail.com>
> ---
> 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ł

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Michal Kazior]

On 10 January 2014 10:19, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> 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 <yeohchunyeow@gmail.com>
> ---
> 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ł

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 10 January 2014 10:19, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Yeoh Chun-Yeow]

> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

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

[Kalle Valo]

Yeoh Chun-Yeow <yeohchunyeow@gmail.com> 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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k