| Message ID | 1389345564-21042-1-git-send-email-yeohchunyeow@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
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?
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :/
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 {
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(-)