[PATCH] ath10k: Fix getting stats from firmware.

[PATCH] ath10k: Fix getting stats from firmware.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Make the request command object the right size so that
firmware will not just throw it away.
Tested customized and upstream firmware.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 drivers/net/wireless/ath/ath10k/wmi.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index fa1b9e0..4946471 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -2766,6 +2766,11 @@ enum wmi_stats_id {
 	WMI_REQUEST_AP_STAT	= 0x02
 };
 
+struct wlan_inst_rssi_args {
+	__le16 cfg_retry_count;
+	__le16 retry_count;
+};
+
 struct wmi_request_stats_cmd {
 	__le32 stats_id;
 
@@ -2773,6 +2778,12 @@ struct wmi_request_stats_cmd {
 	 * Space to add parameters like
 	 * peer mac addr
 	 */
+	__le32 vdev_id;
+	/* peer MAC address */
+	struct wmi_mac_addr peer_macaddr;
+
+	/* Instantaneous RSSI arguments */
+	struct wlan_inst_rssi_args inst_rssi_args;
 } __packed;
 
 /* Suspend option */
-- 
1.7.11.7

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Michal Kazior]

On 21 March 2014 01:05,  <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> Make the request command object the right size so that
> firmware will not just throw it away.
> Tested customized and upstream firmware.

Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.


> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  drivers/net/wireless/ath/ath10k/wmi.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
> index fa1b9e0..4946471 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.h
> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
> @@ -2766,6 +2766,11 @@ enum wmi_stats_id {
>         WMI_REQUEST_AP_STAT     = 0x02
>  };
>
> +struct wlan_inst_rssi_args {
> +       __le16 cfg_retry_count;
> +       __le16 retry_count;
> +};
> +
>  struct wmi_request_stats_cmd {
>         __le32 stats_id;
>
> @@ -2773,6 +2778,12 @@ struct wmi_request_stats_cmd {
>          * Space to add parameters like
>          * peer mac addr
>          */

You can probably remove the comment now :-)


> +       __le32 vdev_id;
> +       /* peer MAC address */
> +       struct wmi_mac_addr peer_macaddr;
> +
> +       /* Instantaneous RSSI arguments */
> +       struct wlan_inst_rssi_args inst_rssi_args;
>  } __packed;


Michał

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Yeoh Chun-Yeow]

>
> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>
Nope. Tested with 636 not working.

----
Chun-Yeow

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Ben Greear]

On 03/20/2014 11:41 PM, Yeoh Chun-Yeow wrote:
>>
>> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>>
> Nope. Tested with 636 not working.

Does the existing code work with 636?  If so, we can add two different cmd
structs and use the smaller one with 636, and the one I modified with
10.x firmware?

Thanks,
Ben

> 
> ----
> Chun-Yeow
> 


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

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> On 03/20/2014 11:41 PM, Yeoh Chun-Yeow wrote:
>>>
>>> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>>>
>> Nope. Tested with 636 not working.
>
> Does the existing code work with 636?  If so, we can add two different cmd
> structs and use the smaller one with 636, and the one I modified with
> 10.x firmware?

For this issue that would be the best approach. See Marek P's patch
"ath10k: update regulatory domain settings for 10.x firmware" (not yet
applied, still in ath-next-test branch) as an example how this can be
done.

-- 
Kalle Valo

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Yeoh Chun-Yeow]

Hi, Ben

I did a quick check again on STA mode. With or without your patch, the
printed peer stats are both identical but both wrong. See below:

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

              Peer MAC address 00:00:00:00:00:00
                     Peer RSSI 256
                  Peer TX rate 0

              Peer MAC address 00:00:00:00:04:f0
                     Peer RSSI 17317
                  Peer TX rate 73

So I think that your patch makes no difference but do indeed providing
"correct" peer stats on latest AP firmware.

After applying the "ath10k: add the Rx rate in FW stats", I am able to
get the Tx and Rx Rate for all connected STAs to my AP.

---
Chun-Yeow

On Sat, Mar 22, 2014 at 12:12 AM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Ben Greear <greearb@candelatech.com> writes:
>
>> On 03/20/2014 11:41 PM, Yeoh Chun-Yeow wrote:
>>>>
>>>> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>>>>
>>> Nope. Tested with 636 not working.
>>
>> Does the existing code work with 636?  If so, we can add two different cmd
>> structs and use the smaller one with 636, and the one I modified with
>> 10.x firmware?
>
> For this issue that would be the best approach. See Marek P's patch
> "ath10k: update regulatory domain settings for 10.x firmware" (not yet
> applied, still in ath-next-test branch) as an example how this can be
> done.
>
> --
> Kalle Valo

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Yeoh Chun-Yeow]

BTW, is anyone ever tested the get stats for firmware 636 before your
patch and getting positive results?

---
Chun-Yeow

On Sat, Mar 22, 2014 at 12:42 AM, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
> Hi, Ben
>
> I did a quick check again on STA mode. With or without your patch, the
> printed peer stats are both identical but both wrong. See below:
>
>              ath10k PEER stats
>              =================
>
>               Peer MAC address 00:00:00:00:00:00
>                      Peer RSSI 256
>                   Peer TX rate 0
>
>               Peer MAC address 00:00:00:00:04:f0
>                      Peer RSSI 17317
>                   Peer TX rate 73
>
> So I think that your patch makes no difference but do indeed providing
> "correct" peer stats on latest AP firmware.
>
> After applying the "ath10k: add the Rx rate in FW stats", I am able to
> get the Tx and Rx Rate for all connected STAs to my AP.
>
> ---
> Chun-Yeow
>
> On Sat, Mar 22, 2014 at 12:12 AM, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> Ben Greear <greearb@candelatech.com> writes:
>>
>>> On 03/20/2014 11:41 PM, Yeoh Chun-Yeow wrote:
>>>>>
>>>>> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>>>>>
>>>> Nope. Tested with 636 not working.
>>>
>>> Does the existing code work with 636?  If so, we can add two different cmd
>>> structs and use the smaller one with 636, and the one I modified with
>>> 10.x firmware?
>>
>> For this issue that would be the best approach. See Marek P's patch
>> "ath10k: update regulatory domain settings for 10.x firmware" (not yet
>> applied, still in ath-next-test branch) as an example how this can be
>> done.
>>
>> --
>> Kalle Valo

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Ben Greear]

On 03/21/2014 09:48 AM, Yeoh Chun-Yeow wrote:

>> I did a quick check again on STA mode. With or without your patch, the
>> printed peer stats are both identical but both wrong. See below:
>>
>>              ath10k PEER stats
>>              =================
>>
>>               Peer MAC address 00:00:00:00:00:00
>>                      Peer RSSI 256
>>                   Peer TX rate 0
>>
>>               Peer MAC address 00:00:00:00:04:f0
>>                      Peer RSSI 17317
>>                   Peer TX rate 73
>>
>> So I think that your patch makes no difference but do indeed providing
>> "correct" peer stats on latest AP firmware.

It looks like the peer stats are broken on my 10.x firmware as well.  Other pdev tx/rx
stats look like they are at least possibly correct.

Since ath10k supports more than one vdev, we also will want to extend it to
get peer for the rest of the vdevs some day.

Thanks,
Ben


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

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Ben Greear]

On 03/21/2014 09:42 AM, Yeoh Chun-Yeow wrote:
> Hi, Ben
> 
> I did a quick check again on STA mode. With or without your patch, the
> printed peer stats are both identical but both wrong. See below:

Ok, it's unlikely my patch would have changed it one way or another.

I'll leave my patch mostly as it was (ie, no special handling for
older v/s newer firmware).

I am just starting to look at stats, and since 10.x was completely
broken and has been for some time, probably no one else has paid
much attention to it yet either.

Thanks for testing.

Ben

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

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Ben Greear]

On 03/20/2014 11:33 PM, Michal Kazior wrote:
> On 21 March 2014 01:05,  <greearb@candelatech.com> wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Make the request command object the right size so that
>> firmware will not just throw it away.
>> Tested customized and upstream firmware.
> 
> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.

The firmware source I have checks in such a way that sending too
large of a cmd should be fine, only too small of a command packet
causes it to be ignored.

I do not have the 636 firmware to test against.

>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/wmi.h | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
>> index fa1b9e0..4946471 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>> @@ -2766,6 +2766,11 @@ enum wmi_stats_id {
>>         WMI_REQUEST_AP_STAT     = 0x02
>>  };
>>
>> +struct wlan_inst_rssi_args {
>> +       __le16 cfg_retry_count;
>> +       __le16 retry_count;
>> +};
>> +
>>  struct wmi_request_stats_cmd {
>>         __le32 stats_id;
>>
>> @@ -2773,6 +2778,12 @@ struct wmi_request_stats_cmd {
>>          * Space to add parameters like
>>          * peer mac addr
>>          */
> 
> You can probably remove the comment now :-)

Ok, I can do that.

Thanks,
Ben



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

Re: [PATCH] ath10k: Fix getting stats from firmware.

[Kalle Valo]

Ben Greear <greearb@candelatech.com> writes:

> On 03/20/2014 11:33 PM, Michal Kazior wrote:
>> On 21 March 2014 01:05,  <greearb@candelatech.com> wrote:
>>> From: Ben Greear <greearb@candelatech.com>
>>>
>>> Make the request command object the right size so that
>>> firmware will not just throw it away.
>>> Tested customized and upstream firmware.
>> 
>> Did you test 636 as well? 636 doesn't seem to support more than just `stats_id`.
>
> The firmware source I have checks in such a way that sending too
> large of a cmd should be fine, only too small of a command packet
> causes it to be ignored.
>
> I do not have the 636 firmware to test against.

The firmare image is here:

https://github.com/kvalo/ath10k-firmware/tree/master/main

-- 
Kalle Valo