diff mbox

ath10k: Fix getting stats from firmware.

Message ID 1395360324-20433-1-git-send-email-greearb@candelatech.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Ben Greear March 21, 2014, 12:05 a.m. UTC
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(+)

Comments

Michal Kazior March 21, 2014, 6:33 a.m. UTC | #1
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?
--
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
Chun-Yeow Yeoh March 21, 2014, 6:41 a.m. UTC | #2
>
> 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
--
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
Ben Greear March 21, 2014, 3:56 p.m. UTC | #3
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 March 21, 2014, 4:03 p.m. UTC | #4
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
>
Kalle Valo March 21, 2014, 4:09 p.m. UTC | #5
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 March 21, 2014, 4:12 p.m. UTC | #6
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.
Chun-Yeow Yeoh March 21, 2014, 4:42 p.m. UTC | #7
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
--
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
Chun-Yeow Yeoh March 21, 2014, 4:48 p.m. UTC | #8
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
--
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
Ben Greear March 21, 2014, 5:29 p.m. UTC | #9
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 March 21, 2014, 6:51 p.m. UTC | #10
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
diff mbox

Patch

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 */