2022-03-18 20:50:20

by Edmond Gagnon

[permalink] [raw]
Subject: [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
rate:

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
SSID: SQ-DEVICETEST
freq: 5200
RX: 4141 bytes (32 packets)
TX: 2082 bytes (15 packets)
signal: -77 dBm
rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
tx bitrate: 6.0 MBit/s

bss flags: short-slot-time
dtim period: 1
beacon int: 100

This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
firmware message and reports it via ieee80211_tx_rate_update:

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:98:21 (on wlan0)
SSID: SQ-DEVICETEST
freq: 2412
RX: 440785 bytes (573 packets)
TX: 60526 bytes (571 packets)
signal: -64 dBm
rx bitrate: 72.2 MBit/s MCS 7 short GI
tx bitrate: 52.0 MBit/s MCS 5

bss flags: short-preamble short-slot-time
dtim period: 1
beacon int: 100

Tested on MSM8939 with WCN3680B running CNSS-PR-2-0-1-2-c1-00083 with
5.17, and verified by sniffing frames over the air with Wireshark to
ensure the MCS indices match.

Signed-off-by: Edmond Gagnon <[email protected]>
Reviewed-by: Benjamin Li <[email protected]>
---
drivers/net/wireless/ath/wcn36xx/hal.h | 7 ++-
drivers/net/wireless/ath/wcn36xx/main.c | 25 +++++++++
drivers/net/wireless/ath/wcn36xx/smd.c | 58 +++++++++++++++++++++
drivers/net/wireless/ath/wcn36xx/smd.h | 1 +
drivers/net/wireless/ath/wcn36xx/txrx.c | 59 ++++++++++++++++++++++
drivers/net/wireless/ath/wcn36xx/txrx.h | 2 +
drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 4 ++
7 files changed, 155 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
index 2a1db9756fd5..46a49f0a51b3 100644
--- a/drivers/net/wireless/ath/wcn36xx/hal.h
+++ b/drivers/net/wireless/ath/wcn36xx/hal.h
@@ -2626,7 +2626,12 @@ enum tx_rate_info {
HAL_TX_RATE_SGI = 0x8,

/* Rate with Long guard interval */
- HAL_TX_RATE_LGI = 0x10
+ HAL_TX_RATE_LGI = 0x10,
+
+ /* VHT rates */
+ HAL_TX_RATE_VHT20 = 0x20,
+ HAL_TX_RATE_VHT40 = 0x40,
+ HAL_TX_RATE_VHT80 = 0x80,
};

struct ani_global_class_a_stats_info {
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index 70531f62777e..75453a3744a6 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -25,6 +25,7 @@
#include <linux/rpmsg.h>
#include <linux/soc/qcom/smem_state.h>
#include <linux/soc/qcom/wcnss_ctrl.h>
+#include <linux/workqueue.h>
#include <net/ipv6.h>
#include "wcn36xx.h"
#include "testmode.h"
@@ -960,6 +961,10 @@ static void wcn36xx_bss_info_changed(struct ieee80211_hw *hw,
if (vif->type == NL80211_IFTYPE_STATION)
wcn36xx_smd_add_beacon_filter(wcn, vif);
wcn36xx_enable_keep_alive_null_packet(wcn, vif);
+
+ wcn->get_stats_sta = sta;
+ wcn->get_stats_vif = vif;
+ schedule_delayed_work(&wcn->get_stats_work, msecs_to_jiffies(3000));
} else {
wcn36xx_dbg(WCN36XX_DBG_MAC,
"disassociated bss %pM vif %pM AID=%d\n",
@@ -967,6 +972,9 @@ static void wcn36xx_bss_info_changed(struct ieee80211_hw *hw,
vif->addr,
bss_conf->aid);
vif_priv->sta_assoc = false;
+ cancel_delayed_work_sync(&wcn->get_stats_work);
+ wcn->get_stats_vif = NULL;
+ wcn->get_stats_sta = NULL;
wcn36xx_smd_set_link_st(wcn,
bss_conf->bssid,
vif->addr,
@@ -1598,6 +1606,17 @@ static int wcn36xx_platform_get_resources(struct wcn36xx *wcn,
return ret;
}

+void wcn36xx_get_stats_work(struct work_struct *work)
+{
+ struct delayed_work *delayed_work = container_of(work, struct delayed_work, work);
+ struct wcn36xx *wcn = container_of(delayed_work, struct wcn36xx, get_stats_work);
+ int stats_status;
+
+ stats_status = wcn36xx_smd_get_stats(wcn, HAL_GLOBAL_CLASS_A_STATS_INFO);
+
+ schedule_delayed_work(&wcn->get_stats_work, msecs_to_jiffies(WCN36XX_HAL_STATS_INTERVAL));
+}
+
static int wcn36xx_probe(struct platform_device *pdev)
{
struct ieee80211_hw *hw;
@@ -1640,6 +1659,8 @@ static int wcn36xx_probe(struct platform_device *pdev)
goto out_wq;
}

+ INIT_DELAYED_WORK(&wcn->get_stats_work, wcn36xx_get_stats_work);
+
ret = dma_set_mask_and_coherent(wcn->dev, DMA_BIT_MASK(32));
if (ret < 0) {
wcn36xx_err("failed to set DMA mask: %d\n", ret);
@@ -1713,6 +1734,10 @@ static int wcn36xx_remove(struct platform_device *pdev)
__skb_queue_purge(&wcn->amsdu);

mutex_destroy(&wcn->hal_mutex);
+
+ cancel_delayed_work_sync(&wcn->get_stats_work);
+ wcn->get_stats_vif = NULL;
+ wcn->get_stats_sta = NULL;
ieee80211_free_hw(hw);

return 0;
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index caeb68901326..ecb083d5b43a 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2627,6 +2627,63 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index)
return ret;
}

+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u32 stats_mask)
+{
+ struct wcn36xx_hal_stats_req_msg msg_body;
+ struct wcn36xx_hal_stats_rsp_msg *rsp;
+ void *rsp_body;
+ int ret = 0;
+
+ if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
+ stats_mask);
+ return -EINVAL;
+ }
+
+ mutex_lock(&wcn->hal_mutex);
+ INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
+
+ msg_body.sta_id = get_sta_index(wcn->get_stats_vif,
+ wcn36xx_sta_to_priv(wcn->get_stats_sta));
+ msg_body.stats_mask = stats_mask;
+
+ PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
+
+ ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
+ if (ret) {
+ wcn36xx_err("sending hal_get_stats failed\n");
+ goto out;
+ }
+
+ ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
+ if (ret) {
+ wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
+ goto out;
+ }
+
+ rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
+ rsp_body = (wcn->hal_buf + sizeof(struct wcn36xx_hal_stats_rsp_msg));
+
+ if (rsp->stats_mask != stats_mask) {
+ wcn36xx_err("stat_mask 0x%x differs from requested 0x%x\n",
+ rsp->stats_mask, stats_mask);
+ goto out;
+ }
+
+ if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ ret = wcn36xx_report_tx_rate(wcn, (struct ani_global_class_a_stats_info *)rsp_body);
+ if (ret) {
+ wcn36xx_err("failed to report TX rate\n");
+ goto out;
+ }
+ rsp_body += sizeof(struct ani_global_class_a_stats_info);
+ }
+out:
+ mutex_unlock(&wcn->hal_mutex);
+
+ return ret;
+}
+
static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len, struct add_ba_info *ba_info)
{
struct wcn36xx_hal_trigger_ba_rsp_candidate *candidate;
@@ -3316,6 +3373,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
case WCN36XX_HAL_ADD_BA_SESSION_RSP:
case WCN36XX_HAL_ADD_BA_RSP:
case WCN36XX_HAL_DEL_BA_RSP:
+ case WCN36XX_HAL_GET_STATS_RSP:
case WCN36XX_HAL_TRIGGER_BA_RSP:
case WCN36XX_HAL_UPDATE_CFG_RSP:
case WCN36XX_HAL_JOIN_RSP:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 957cfa87fbde..a30f28f4130d 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -138,6 +138,7 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index);
int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u16 *ssn);
+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u32 stats_mask);

int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);

diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index df749b114568..ac55f8d62440 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -699,3 +699,62 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,

return ret;
}
+
+int wcn36xx_report_tx_rate(struct wcn36xx *wcn, struct ani_global_class_a_stats_info *stats)
+{
+ struct ieee80211_tx_info info;
+ struct ieee80211_tx_rate *tx_rate;
+
+ memset(&info, 0, sizeof(struct ieee80211_tx_info));
+ tx_rate = &info.status.rates[0];
+
+ tx_rate->idx = stats->mcs_index;
+ tx_rate->count = 0;
+ tx_rate->flags = 0;
+
+ if (stats->tx_rate_flags & HAL_TX_RATE_LEGACY) {
+ // tx_rate is in units of 500kbps, while wcn36xx_rate_table's rates
+ // are in units of 100kbps.
+ unsigned int reported_rate = stats->tx_rate * 5;
+ int i;
+
+ // Iterate over the legacy rates to convert bitrate to rate index.
+ for (i = 0; i < ARRAY_SIZE(wcn36xx_rate_table); i++) {
+ const struct wcn36xx_rate *rate = &wcn36xx_rate_table[i];
+
+ if (rate->encoding != RX_ENC_LEGACY) {
+ wcn36xx_warn("legacy rate %u not present in rate table\n",
+ reported_rate);
+ break;
+ }
+ if (rate->bitrate == reported_rate) {
+ tx_rate->idx = rate->mcs_or_legacy_index;
+ break;
+ }
+ }
+ }
+
+ // HT?
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_HT40))
+ tx_rate->flags |= IEEE80211_TX_RC_MCS;
+
+ // VHT?
+ if (stats->tx_rate_flags & (HAL_TX_RATE_VHT20 | HAL_TX_RATE_VHT40 | HAL_TX_RATE_VHT80))
+ tx_rate->flags |= IEEE80211_TX_RC_VHT_MCS;
+
+ // SGI / LGI?
+ if (stats->tx_rate_flags & HAL_TX_RATE_SGI)
+ tx_rate->flags |= IEEE80211_TX_RC_SHORT_GI;
+
+ // 40MHz?
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT40 | HAL_TX_RATE_VHT40))
+ tx_rate->flags |= IEEE80211_TX_RC_40_MHZ_WIDTH;
+
+ // 80MHz?
+ if (stats->tx_rate_flags & HAL_TX_RATE_VHT80)
+ tx_rate->flags |= IEEE80211_TX_RC_80_MHZ_WIDTH;
+
+ ieee80211_tx_rate_update(wcn->hw, wcn->get_stats_sta, &info);
+
+ return 0;
+}
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.h b/drivers/net/wireless/ath/wcn36xx/txrx.h
index b54311ffde9c..28cf45ce6c89 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.h
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.h
@@ -18,6 +18,7 @@
#define _TXRX_H_

#include <linux/etherdevice.h>
+#include <linux/string.h>
#include "wcn36xx.h"

/* TODO describe all properties */
@@ -164,5 +165,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
int wcn36xx_start_tx(struct wcn36xx *wcn,
struct wcn36xx_sta *sta_priv,
struct sk_buff *skb);
+int wcn36xx_report_tx_rate(struct wcn36xx *wcn, struct ani_global_class_a_stats_info *stats);

#endif /* _TXRX_H_ */
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 80a4e7aea419..121195991ee2 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -32,6 +32,7 @@

#define WLAN_NV_FILE "wlan/prima/WCNSS_qcom_wlan_nv.bin"
#define WCN36XX_AGGR_BUFFER_SIZE 64
+#define WCN36XX_HAL_STATS_INTERVAL 3000

extern unsigned int wcn36xx_dbg_mask;

@@ -295,6 +296,9 @@ struct wcn36xx {

spinlock_t survey_lock; /* protects chan_survey */
struct wcn36xx_chan_survey *chan_survey;
+ struct delayed_work get_stats_work;
+ struct ieee80211_sta *get_stats_sta;
+ struct ieee80211_vif *get_stats_vif;
};

static inline bool wcn36xx_is_fw_version(struct wcn36xx *wcn,
--
2.25.1


2022-03-20 21:55:54

by Bryan O'Donoghue

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

On 18/03/2022 19:58, Edmond Gagnon wrote:
> + INIT_DELAYED_WORK(&wcn->get_stats_work, wcn36xx_get_stats_work);

Instead of forking a worker and polling we could add the relevant SMD
command to

static int wcn36xx_smd_tx_compl_ind(struct wcn36xx *wcn, void *buf,
size_t len)
{
wcn36xx_smd_get_stats(wcn, 0xSomeMask);
}

That way we only ever ask for and report a new TX data rate when we know
a TX event - and hence a potential TX data-rate update - has taken place.

---
bod

2022-03-21 09:40:35

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

Hi Edmond,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on wireless-next/main]
[also build test WARNING on kvalo-ath/ath-next next-20220318]
[cannot apply to wireless/main kvalo-wireless-drivers/master v5.17-rc8]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Edmond-Gagnon/wcn36xx-Implement-tx_rate-reporting/20220319-040030
base: https://git.kernel.org/pub/scm/linux/kernel/git/wireless/wireless-next.git main
config: i386-randconfig-a005 (https://download.01.org/0day-ci/archive/20220319/[email protected]/config)
compiler: gcc-9 (Ubuntu 9.4.0-1ubuntu1~20.04) 9.4.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/ec06272b313bdabd805efd65a0a6c2a74b82803f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Edmond-Gagnon/wcn36xx-Implement-tx_rate-reporting/20220319-040030
git checkout ec06272b313bdabd805efd65a0a6c2a74b82803f
# save the config file to linux build tree
mkdir build_dir
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/wireless/ath/wcn36xx/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

>> drivers/net/wireless/ath/wcn36xx/main.c:1604:6: warning: no previous prototype for 'wcn36xx_get_stats_work' [-Wmissing-prototypes]
1604 | void wcn36xx_get_stats_work(struct work_struct *work)
| ^~~~~~~~~~~~~~~~~~~~~~
drivers/net/wireless/ath/wcn36xx/main.c: In function 'wcn36xx_get_stats_work':
>> drivers/net/wireless/ath/wcn36xx/main.c:1608:6: warning: variable 'stats_status' set but not used [-Wunused-but-set-variable]
1608 | int stats_status;
| ^~~~~~~~~~~~


vim +/wcn36xx_get_stats_work +1604 drivers/net/wireless/ath/wcn36xx/main.c

1603
> 1604 void wcn36xx_get_stats_work(struct work_struct *work)
1605 {
1606 struct delayed_work *delayed_work = container_of(work, struct delayed_work, work);
1607 struct wcn36xx *wcn = container_of(delayed_work, struct wcn36xx, get_stats_work);
> 1608 int stats_status;
1609
1610 stats_status = wcn36xx_smd_get_stats(wcn, HAL_GLOBAL_CLASS_A_STATS_INFO);
1611
1612 schedule_delayed_work(&wcn->get_stats_work, msecs_to_jiffies(WCN36XX_HAL_STATS_INTERVAL));
1613 }
1614

---
0-DAY CI Kernel Test Service
https://lists.01.org/hyperkitty/list/[email protected]

2022-03-21 11:02:16

by Bryan O'Donoghue

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

On 20/03/2022 13:21, Bryan O'Donoghue wrote:
> On 18/03/2022 19:58, Edmond Gagnon wrote:
>> +    INIT_DELAYED_WORK(&wcn->get_stats_work, wcn36xx_get_stats_work);
>
> Instead of forking a worker and polling we could add the relevant SMD
> command to
>
> static int wcn36xx_smd_tx_compl_ind(struct wcn36xx *wcn, void *buf,
> size_t len)
> {
>     wcn36xx_smd_get_stats(wcn, 0xSomeMask);
> }
>
> That way we only ever ask for and report a new TX data rate when we know
> a TX event - and hence a potential TX data-rate update - has taken place.
>
> ---
> bod
>

Thinking a bit more

- Do the SMD get_stats in the tx completion
This might be a problem initiating another SMD transaction inside
of an SMD callback. But is the most straight forward way to
get the data while avoiding alot of needless polling.

- Schedule your worker from the TX completion
Again you should only care about gathering the data when you know
something has happened which necessitates gathering that data
like TX completion

- Schedule your worker from the RX indication routine
Seems not as logical as the first two but it might be easier
to schedule the worker in the RX data handler

Either way, I do think you should only gather this data on an event, not
as a continuous poll.

---
bod

2022-03-21 14:13:49

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

Bryan O'Donoghue <[email protected]> writes:

> On 20/03/2022 13:21, Bryan O'Donoghue wrote:
>> On 18/03/2022 19:58, Edmond Gagnon wrote:
>>> +    INIT_DELAYED_WORK(&wcn->get_stats_work, wcn36xx_get_stats_work);
>>
>> Instead of forking a worker and polling we could add the relevant
>> SMD command to
>>
>> static int wcn36xx_smd_tx_compl_ind(struct wcn36xx *wcn, void *buf,
>> size_t len)
>> {
>>     wcn36xx_smd_get_stats(wcn, 0xSomeMask);
>> }
>>
>> That way we only ever ask for and report a new TX data rate when we
>> know a TX event - and hence a potential TX data-rate update - has
>> taken place.
>>
>> ---
>> bod
>>
>
> Thinking a bit more
>
> - Do the SMD get_stats in the tx completion
> This might be a problem initiating another SMD transaction inside
> of an SMD callback. But is the most straight forward way to
> get the data while avoiding alot of needless polling.
>
> - Schedule your worker from the TX completion
> Again you should only care about gathering the data when you know
> something has happened which necessitates gathering that data
> like TX completion
>
> - Schedule your worker from the RX indication routine
> Seems not as logical as the first two but it might be easier
> to schedule the worker in the RX data handler
>
> Either way, I do think you should only gather this data on an event,
> not as a continuous poll.

I agree, a continuous poll is not a good idea as it affects power
consumption. What about struct ieee80211_ops::sta_statistics? AFAIK
that's called only when user space is requestings stats so the overhead
should be minimal.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2022-03-24 00:58:56

by Edmond Gagnon

[permalink] [raw]
Subject: [PATCH v3] wcn36xx: Implement tx_rate reporting

Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
rate:

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
SSID: SQ-DEVICETEST
freq: 5200
RX: 4141 bytes (32 packets)
TX: 2082 bytes (15 packets)
signal: -77 dBm
rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
tx bitrate: 6.0 MBit/s

bss flags: short-slot-time
dtim period: 1
beacon int: 100

This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
firmware message and reports it via ieee80211_ops::sta_statistics.

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
SSID: SQ-DEVICETEST
freq: 5700
RX: 26788094 bytes (19859 packets)
TX: 1101376 bytes (12119 packets)
signal: -75 dBm
rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1

bss flags: short-slot-time
dtim period: 1
beacon int: 100

Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
and verified by sniffing frames over the air with Wireshark to ensure the
MCS indices match.

Signed-off-by: Edmond Gagnon <[email protected]>
Reviewed-by: Benjamin Li <[email protected]>
---

Changes in v3:
- Refactored to report tx_rate via ieee80211_ops::sta_statistics
- Dropped get_sta_index patch
- Addressed style comments
Changes in v2:
- Refactored to use existing wcn36xx_hal_get_stats_{req,rsp}_msg structs.
- Added more notes about testing.
- Reduced reporting interval to 3000msec.
- Assorted type and memory safety fixes.
- Make wcn36xx_smd_get_stats friendlier to future message implementors.

drivers/net/wireless/ath/wcn36xx/hal.h | 7 +++-
drivers/net/wireless/ath/wcn36xx/main.c | 16 +++++++
drivers/net/wireless/ath/wcn36xx/smd.c | 56 +++++++++++++++++++++++++
drivers/net/wireless/ath/wcn36xx/smd.h | 2 +
drivers/net/wireless/ath/wcn36xx/txrx.c | 29 +++++++++++++
drivers/net/wireless/ath/wcn36xx/txrx.h | 1 +
6 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
index 2a1db9756fd5..46a49f0a51b3 100644
--- a/drivers/net/wireless/ath/wcn36xx/hal.h
+++ b/drivers/net/wireless/ath/wcn36xx/hal.h
@@ -2626,7 +2626,12 @@ enum tx_rate_info {
HAL_TX_RATE_SGI = 0x8,

/* Rate with Long guard interval */
- HAL_TX_RATE_LGI = 0x10
+ HAL_TX_RATE_LGI = 0x10,
+
+ /* VHT rates */
+ HAL_TX_RATE_VHT20 = 0x20,
+ HAL_TX_RATE_VHT40 = 0x40,
+ HAL_TX_RATE_VHT80 = 0x80,
};

struct ani_global_class_a_stats_info {
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index b545d4b2b8c4..fc76b090c39f 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1400,6 +1400,21 @@ static int wcn36xx_get_survey(struct ieee80211_hw *hw, int idx,
return 0;
}

+static void wcn36xx_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta, struct station_info *sinfo)
+{
+ struct wcn36xx *wcn;
+ u8 sta_index;
+ int status = 0;
+
+ wcn = hw->priv;
+ sta_index = get_sta_index(vif, wcn36xx_sta_to_priv(sta));
+ status = wcn36xx_smd_get_stats(wcn, sta_index, HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
+
+ if (status)
+ wcn36xx_err("wcn36xx_smd_get_stats failed\n");
+}
+
static const struct ieee80211_ops wcn36xx_ops = {
.start = wcn36xx_start,
.stop = wcn36xx_stop,
@@ -1423,6 +1438,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
.set_rts_threshold = wcn36xx_set_rts_threshold,
.sta_add = wcn36xx_sta_add,
.sta_remove = wcn36xx_sta_remove,
+ .sta_statistics = wcn36xx_sta_statistics,
.ampdu_action = wcn36xx_ampdu_action,
#if IS_ENABLED(CONFIG_IPV6)
.ipv6_addr_change = wcn36xx_ipv6_addr_change,
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index caeb68901326..8f9aa892e5ec 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2627,6 +2627,61 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index)
return ret;
}

+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
+ struct station_info *sinfo)
+{
+ struct wcn36xx_hal_stats_req_msg msg_body;
+ struct wcn36xx_hal_stats_rsp_msg *rsp;
+ void *rsp_body;
+ int ret = 0;
+
+ if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
+ stats_mask);
+ return -EINVAL;
+ }
+
+ mutex_lock(&wcn->hal_mutex);
+ INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
+
+ msg_body.sta_id = sta_index;
+ msg_body.stats_mask = stats_mask;
+
+ PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
+
+ ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
+ if (ret) {
+ wcn36xx_err("sending hal_get_stats failed\n");
+ goto out;
+ }
+
+ ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
+ if (ret) {
+ wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
+ goto out;
+ }
+
+ rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
+ rsp_body = (wcn->hal_buf + sizeof(struct wcn36xx_hal_stats_rsp_msg));
+
+ if (rsp->stats_mask != stats_mask) {
+ wcn36xx_err("stats_mask 0x%x differs from requested 0x%x\n",
+ rsp->stats_mask, stats_mask);
+ goto out;
+ }
+
+ if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ wcn36xx_process_tx_rate((struct ani_global_class_a_stats_info *)rsp_body,
+ &sinfo->txrate);
+ sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
+ rsp_body += sizeof(struct ani_global_class_a_stats_info);
+ }
+out:
+ mutex_unlock(&wcn->hal_mutex);
+
+ return ret;
+}
+
static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len, struct add_ba_info *ba_info)
{
struct wcn36xx_hal_trigger_ba_rsp_candidate *candidate;
@@ -3316,6 +3371,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
case WCN36XX_HAL_ADD_BA_SESSION_RSP:
case WCN36XX_HAL_ADD_BA_RSP:
case WCN36XX_HAL_DEL_BA_RSP:
+ case WCN36XX_HAL_GET_STATS_RSP:
case WCN36XX_HAL_TRIGGER_BA_RSP:
case WCN36XX_HAL_UPDATE_CFG_RSP:
case WCN36XX_HAL_JOIN_RSP:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 957cfa87fbde..3fd598ac2a27 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -138,6 +138,8 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index);
int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u16 *ssn);
+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
+ struct station_info *sinfo);

int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);

diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index df749b114568..8da3955995b6 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -699,3 +699,32 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,

return ret;
}
+
+void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info)
+{
+ /* tx_rate is in units of 500kbps; mac80211 wants them in 100kbps */
+ if (stats->tx_rate_flags & HAL_TX_RATE_LEGACY)
+ info->legacy = stats->tx_rate * 5;
+
+ info->flags = 0;
+ info->mcs = stats->mcs_index;
+ info->nss = 1;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_HT40))
+ info->flags |= RATE_INFO_FLAGS_MCS;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_VHT20 | HAL_TX_RATE_VHT40 | HAL_TX_RATE_VHT80))
+ info->flags |= RATE_INFO_FLAGS_VHT_MCS;
+
+ if (stats->tx_rate_flags & HAL_TX_RATE_SGI)
+ info->flags |= RATE_INFO_FLAGS_SHORT_GI;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_VHT20))
+ info->bw = RATE_INFO_BW_20;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT40 | HAL_TX_RATE_VHT40))
+ info->bw = RATE_INFO_BW_40;
+
+ if (stats->tx_rate_flags & HAL_TX_RATE_VHT80)
+ info->bw = RATE_INFO_BW_80;
+}
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.h b/drivers/net/wireless/ath/wcn36xx/txrx.h
index b54311ffde9c..fb0d6cabd52b 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.h
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.h
@@ -164,5 +164,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
int wcn36xx_start_tx(struct wcn36xx *wcn,
struct wcn36xx_sta *sta_priv,
struct sk_buff *skb);
+void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info);

#endif /* _TXRX_H_ */
--
2.25.1

2022-03-24 09:55:13

by Benjamin Li

[permalink] [raw]
Subject: Re: [PATCH v3] wcn36xx: Implement tx_rate reporting

Tested on DB410c (WCN3620, 802.11n/2.4GHz only) running firmware
CNSS-PR-2-0-1-2-c1-74-130449-3, also with 5.17.

root@linaro-developer:~# speedtest
Retrieving speedtest.net configuration...
Testing from Square (135.84.132.205)...
Retrieving speedtest.net server list...
Selecting best server based on ping...
Hosted by Open5G Inc. (Atherton, CA) [40.28 km]: 11.828 ms
Testing download speed................................................................................
Download: 3.00 Mbit/s
Testing upload speed......................................................................................................
Upload: 23.69 Mbit/s
root@linaro-developer:~#
root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:98:21 (on wlan0)
SSID: SQ-DEVICETEST
freq: 2412
RX: 34119054 bytes (37804 packets)
TX: 32924504 bytes (35073 packets)
signal: -59 dBm
rx bitrate: 57.8 MBit/s MCS 5 short GI
tx bitrate: 58.5 MBit/s MCS 6

bss flags: short-preamble short-slot-time
dtim period: 1
beacon int: 100

Tested-by: Benjamin Li <[email protected]>

Ben

On 3/23/22 2:45 PM, Edmond Gagnon wrote:
> Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
> rate:
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5200
> RX: 4141 bytes (32 packets)
> TX: 2082 bytes (15 packets)
> signal: -77 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 6.0 MBit/s
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
> firmware message and reports it via ieee80211_ops::sta_statistics.
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5700
> RX: 26788094 bytes (19859 packets)
> TX: 1101376 bytes (12119 packets)
> signal: -75 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
> and verified by sniffing frames over the air with Wireshark to ensure the
> MCS indices match.
>
> Signed-off-by: Edmond Gagnon <[email protected]>
> Reviewed-by: Benjamin Li <[email protected]>
> ---
>
> Changes in v3:
> - Refactored to report tx_rate via ieee80211_ops::sta_statistics
> - Dropped get_sta_index patch
> - Addressed style comments
> Changes in v2:
> - Refactored to use existing wcn36xx_hal_get_stats_{req,rsp}_msg structs.
> - Added more notes about testing.
> - Reduced reporting interval to 3000msec.
> - Assorted type and memory safety fixes.
> - Make wcn36xx_smd_get_stats friendlier to future message implementors.
>
> drivers/net/wireless/ath/wcn36xx/hal.h | 7 +++-
> drivers/net/wireless/ath/wcn36xx/main.c | 16 +++++++
> drivers/net/wireless/ath/wcn36xx/smd.c | 56 +++++++++++++++++++++++++
> drivers/net/wireless/ath/wcn36xx/smd.h | 2 +
> drivers/net/wireless/ath/wcn36xx/txrx.c | 29 +++++++++++++
> drivers/net/wireless/ath/wcn36xx/txrx.h | 1 +
> 6 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
> index 2a1db9756fd5..46a49f0a51b3 100644
> --- a/drivers/net/wireless/ath/wcn36xx/hal.h
> +++ b/drivers/net/wireless/ath/wcn36xx/hal.h
> @@ -2626,7 +2626,12 @@ enum tx_rate_info {
> HAL_TX_RATE_SGI = 0x8,
>
> /* Rate with Long guard interval */
> - HAL_TX_RATE_LGI = 0x10
> + HAL_TX_RATE_LGI = 0x10,
> +
> + /* VHT rates */
> + HAL_TX_RATE_VHT20 = 0x20,
> + HAL_TX_RATE_VHT40 = 0x40,
> + HAL_TX_RATE_VHT80 = 0x80,
> };
>
> struct ani_global_class_a_stats_info {
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index b545d4b2b8c4..fc76b090c39f 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -1400,6 +1400,21 @@ static int wcn36xx_get_survey(struct ieee80211_hw *hw, int idx,
> return 0;
> }
>
> +static void wcn36xx_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta, struct station_info *sinfo)
> +{
> + struct wcn36xx *wcn;
> + u8 sta_index;
> + int status = 0;
> +
> + wcn = hw->priv;
> + sta_index = get_sta_index(vif, wcn36xx_sta_to_priv(sta));
> + status = wcn36xx_smd_get_stats(wcn, sta_index, HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
> +
> + if (status)
> + wcn36xx_err("wcn36xx_smd_get_stats failed\n");
> +}
> +
> static const struct ieee80211_ops wcn36xx_ops = {
> .start = wcn36xx_start,
> .stop = wcn36xx_stop,
> @@ -1423,6 +1438,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
> .set_rts_threshold = wcn36xx_set_rts_threshold,
> .sta_add = wcn36xx_sta_add,
> .sta_remove = wcn36xx_sta_remove,
> + .sta_statistics = wcn36xx_sta_statistics,
> .ampdu_action = wcn36xx_ampdu_action,
> #if IS_ENABLED(CONFIG_IPV6)
> .ipv6_addr_change = wcn36xx_ipv6_addr_change,
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index caeb68901326..8f9aa892e5ec 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2627,6 +2627,61 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index)
> return ret;
> }
>
> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
> + struct station_info *sinfo)
> +{
> + struct wcn36xx_hal_stats_req_msg msg_body;
> + struct wcn36xx_hal_stats_rsp_msg *rsp;
> + void *rsp_body;
> + int ret = 0;
> +
> + if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
> + wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
> + stats_mask);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&wcn->hal_mutex);
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
> +
> + msg_body.sta_id = sta_index;
> + msg_body.stats_mask = stats_mask;
> +
> + PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
> +
> + ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> + if (ret) {
> + wcn36xx_err("sending hal_get_stats failed\n");
> + goto out;
> + }
> +
> + ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> + if (ret) {
> + wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
> + goto out;
> + }
> +
> + rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
> + rsp_body = (wcn->hal_buf + sizeof(struct wcn36xx_hal_stats_rsp_msg));
> +
> + if (rsp->stats_mask != stats_mask) {
> + wcn36xx_err("stats_mask 0x%x differs from requested 0x%x\n",
> + rsp->stats_mask, stats_mask);
> + goto out;
> + }
> +
> + if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
> + wcn36xx_process_tx_rate((struct ani_global_class_a_stats_info *)rsp_body,
> + &sinfo->txrate);
> + sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
> + rsp_body += sizeof(struct ani_global_class_a_stats_info);
> + }
> +out:
> + mutex_unlock(&wcn->hal_mutex);
> +
> + return ret;
> +}
> +
> static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len, struct add_ba_info *ba_info)
> {
> struct wcn36xx_hal_trigger_ba_rsp_candidate *candidate;
> @@ -3316,6 +3371,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
> case WCN36XX_HAL_ADD_BA_SESSION_RSP:
> case WCN36XX_HAL_ADD_BA_RSP:
> case WCN36XX_HAL_DEL_BA_RSP:
> + case WCN36XX_HAL_GET_STATS_RSP:
> case WCN36XX_HAL_TRIGGER_BA_RSP:
> case WCN36XX_HAL_UPDATE_CFG_RSP:
> case WCN36XX_HAL_JOIN_RSP:
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
> index 957cfa87fbde..3fd598ac2a27 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.h
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.h
> @@ -138,6 +138,8 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
> int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
> int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index);
> int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u16 *ssn);
> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
> + struct station_info *sinfo);
>
> int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
> index df749b114568..8da3955995b6 100644
> --- a/drivers/net/wireless/ath/wcn36xx/txrx.c
> +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
> @@ -699,3 +699,32 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
>
> return ret;
> }
> +
> +void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info)
> +{
> + /* tx_rate is in units of 500kbps; mac80211 wants them in 100kbps */
> + if (stats->tx_rate_flags & HAL_TX_RATE_LEGACY)
> + info->legacy = stats->tx_rate * 5;
> +
> + info->flags = 0;
> + info->mcs = stats->mcs_index;
> + info->nss = 1;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_HT40))
> + info->flags |= RATE_INFO_FLAGS_MCS;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_VHT20 | HAL_TX_RATE_VHT40 | HAL_TX_RATE_VHT80))
> + info->flags |= RATE_INFO_FLAGS_VHT_MCS;
> +
> + if (stats->tx_rate_flags & HAL_TX_RATE_SGI)
> + info->flags |= RATE_INFO_FLAGS_SHORT_GI;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_VHT20))
> + info->bw = RATE_INFO_BW_20;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT40 | HAL_TX_RATE_VHT40))
> + info->bw = RATE_INFO_BW_40;
> +
> + if (stats->tx_rate_flags & HAL_TX_RATE_VHT80)
> + info->bw = RATE_INFO_BW_80;
> +}
> diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.h b/drivers/net/wireless/ath/wcn36xx/txrx.h
> index b54311ffde9c..fb0d6cabd52b 100644
> --- a/drivers/net/wireless/ath/wcn36xx/txrx.h
> +++ b/drivers/net/wireless/ath/wcn36xx/txrx.h
> @@ -164,5 +164,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
> int wcn36xx_start_tx(struct wcn36xx *wcn,
> struct wcn36xx_sta *sta_priv,
> struct sk_buff *skb);
> +void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info);
>
> #endif /* _TXRX_H_ */

2022-03-24 18:17:52

by Bryan O'Donoghue

[permalink] [raw]
Subject: Re: [PATCH v3] wcn36xx: Implement tx_rate reporting

On 24/03/2022 12:42, Bryan O'Donoghue wrote:
> On 23/03/2022 21:45, Edmond Gagnon wrote:
>> Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
>> rate:
>>
>> root@linaro-developer:~# iw wlan0 link
>> Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
>>          SSID: SQ-DEVICETEST
>>          freq: 5200
>>          RX: 4141 bytes (32 packets)
>>          TX: 2082 bytes (15 packets)
>>          signal: -77 dBm
>>          rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
>>          tx bitrate: 6.0 MBit/s
>>
>>          bss flags:      short-slot-time
>>          dtim period:    1
>>          beacon int:     100
>>
>> This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
>> firmware message and reports it via ieee80211_ops::sta_statistics.
>>
>> root@linaro-developer:~# iw wlan0 link
>> Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
>>          SSID: SQ-DEVICETEST
>>          freq: 5700
>>          RX: 26788094 bytes (19859 packets)
>>          TX: 1101376 bytes (12119 packets)
>>          signal: -75 dBm
>>          rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
>>          tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1
>>
>>          bss flags:      short-slot-time
>>          dtim period:    1
>>          beacon int:     100
>>
>> Tested on MSM8939 with WCN3680B running firmware
>> CNSS-PR-2-0-1-2-c1-00083,
>> and verified by sniffing frames over the air with Wireshark to ensure the
>> MCS indices match.
>>
>> Signed-off-by: Edmond Gagnon <[email protected]>
>> Reviewed-by: Benjamin Li <[email protected]>
>> ---
>>
>> Changes in v3:
>>   - Refactored to report tx_rate via ieee80211_ops::sta_statistics
>>   - Dropped get_sta_index patch
>>   - Addressed style comments
>> Changes in v2:
>>   - Refactored to use existing wcn36xx_hal_get_stats_{req,rsp}_msg
>> structs.
>>   - Added more notes about testing.
>>   - Reduced reporting interval to 3000msec.
>>   - Assorted type and memory safety fixes.
>>   - Make wcn36xx_smd_get_stats friendlier to future message implementors.
>>
>>   drivers/net/wireless/ath/wcn36xx/hal.h  |  7 +++-
>>   drivers/net/wireless/ath/wcn36xx/main.c | 16 +++++++
>>   drivers/net/wireless/ath/wcn36xx/smd.c  | 56 +++++++++++++++++++++++++
>>   drivers/net/wireless/ath/wcn36xx/smd.h  |  2 +
>>   drivers/net/wireless/ath/wcn36xx/txrx.c | 29 +++++++++++++
>>   drivers/net/wireless/ath/wcn36xx/txrx.h |  1 +
>>   6 files changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h
>> b/drivers/net/wireless/ath/wcn36xx/hal.h
>> index 2a1db9756fd5..46a49f0a51b3 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/hal.h
>> +++ b/drivers/net/wireless/ath/wcn36xx/hal.h
>> @@ -2626,7 +2626,12 @@ enum tx_rate_info {
>>       HAL_TX_RATE_SGI = 0x8,
>>       /* Rate with Long guard interval */
>> -    HAL_TX_RATE_LGI = 0x10
>> +    HAL_TX_RATE_LGI = 0x10,
>> +
>> +    /* VHT rates */
>> +    HAL_TX_RATE_VHT20  = 0x20,
>> +    HAL_TX_RATE_VHT40  = 0x40,
>> +    HAL_TX_RATE_VHT80  = 0x80,
>>   };
>>   struct ani_global_class_a_stats_info {
>> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c
>> b/drivers/net/wireless/ath/wcn36xx/main.c
>> index b545d4b2b8c4..fc76b090c39f 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/main.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
>> @@ -1400,6 +1400,21 @@ static int wcn36xx_get_survey(struct
>> ieee80211_hw *hw, int idx,
>>       return 0;
>>   }
>> +static void wcn36xx_sta_statistics(struct ieee80211_hw *hw, struct
>> ieee80211_vif *vif,
>> +                   struct ieee80211_sta *sta, struct station_info
>> *sinfo)
>
>
> Consider running this through checkpatch.pl and fixing most of the
> complaints, use your discretion.
>
> scripts/checkpatch.pl --strict
> ~/Development/patches/linux/wifi/v3-wcn36xx-Implement-tx_rate-reporting.patch
>
>
> static void wcn36xx_sta_statistics(struct ieee80211_hw *hw,
>                                    struct ieee80211_vif *vif,
>                                    struct ieee80211_sta *sta,
>                                    struct station_info *sinfo)
>
>> +{
>> +    struct wcn36xx *wcn;
>> +    u8 sta_index;
>> +    int status = 0;
>> +
>> +    wcn = hw->priv;
>> +    sta_index = get_sta_index(vif, wcn36xx_sta_to_priv(sta));
>> +    status = wcn36xx_smd_get_stats(wcn, sta_index,
>> HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
>
> status = wcn36xx_smd_get_stats(wcn, sta_index,
>                                HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
>
>
>> +
>> +    if (status)
>> +        wcn36xx_err("wcn36xx_smd_get_stats failed\n");
>> +}
>> +
>>   static const struct ieee80211_ops wcn36xx_ops = {
>>       .start            = wcn36xx_start,
>>       .stop            = wcn36xx_stop,
>> @@ -1423,6 +1438,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
>>       .set_rts_threshold    = wcn36xx_set_rts_threshold,
>>       .sta_add        = wcn36xx_sta_add,
>>       .sta_remove        = wcn36xx_sta_remove,
>> +    .sta_statistics        = wcn36xx_sta_statistics,
>>       .ampdu_action        = wcn36xx_ampdu_action,
>>   #if IS_ENABLED(CONFIG_IPV6)
>>       .ipv6_addr_change    = wcn36xx_ipv6_addr_change,
>> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c
>> b/drivers/net/wireless/ath/wcn36xx/smd.c
>> index caeb68901326..8f9aa892e5ec 100644
>> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
>> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
>> @@ -2627,6 +2627,61 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16
>> tid, u8 direction, u8 sta_index)
>>       return ret;
>>   }
>> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32
>> stats_mask,
>> +              struct station_info *sinfo)
>> +{
>> +    struct wcn36xx_hal_stats_req_msg msg_body;
>> +    struct wcn36xx_hal_stats_rsp_msg *rsp;
>         struct ani_global_class_a_stats_info *stats_info;
>> +    void *rsp_body;
>> +    int ret = 0;
>> +
>> +    if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
>> +        wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
>> +                stats_mask);
>> +        return -EINVAL;
>> +    }
>> +
>> +    mutex_lock(&wcn->hal_mutex);
>> +    INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
>> +
>> +    msg_body.sta_id = sta_index;
>> +    msg_body.stats_mask = stats_mask;
>> +
>> +    PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
>> +
>> +    ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
>> +    if (ret) {
>> +        wcn36xx_err("sending hal_get_stats failed\n");
>> +        goto out;
>> +    }
>> +
>> +    ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
>> +    if (ret) {
>> +        wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
>> +        goto out;
>> +    }
>> +
>> +    rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
>> +    rsp_body = (wcn->hal_buf + sizeof(struct
>> wcn36xx_hal_stats_rsp_msg));
>> +
>> +    if (rsp->stats_mask != stats_mask) {
>> +        wcn36xx_err("stats_mask 0x%x differs from requested 0x%x\n",
>> +                rsp->stats_mask, stats_mask);
>> +        goto out;
>> +    }
>> +
> If you take a pointer and cast it, then you won't have this very long
> line with the cast
>
>         stats_info = (struct ani_global_class_a_stats_info *)rsp_body;
>> +    if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
>> +        wcn36xx_process_tx_rate((struct ani_global_class_a_stats_info
>> *)rsp_body,
>> +                    &sinfo->txrate);
>                 wcn36xx_process_tx_rate(stats_info, &sinfo->txrate);
>
>
> Other than that LGTM
>
> Reviewed-by: Bryan O'Donoghue <[email protected]>

Tested-by: Bryan O'Donoghue <[email protected]>

2022-03-25 23:09:53

by Edmond Gagnon

[permalink] [raw]
Subject: [PATCH v4] wcn36xx: Implement tx_rate reporting

Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
rate:

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
SSID: SQ-DEVICETEST
freq: 5200
RX: 4141 bytes (32 packets)
TX: 2082 bytes (15 packets)
signal: -77 dBm
rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
tx bitrate: 6.0 MBit/s

bss flags: short-slot-time
dtim period: 1
beacon int: 100

This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
firmware message and reports it via ieee80211_ops::sta_statistics.

root@linaro-developer:~# iw wlan0 link
Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
SSID: SQ-DEVICETEST
freq: 5700
RX: 26788094 bytes (19859 packets)
TX: 1101376 bytes (12119 packets)
signal: -75 dBm
rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1

bss flags: short-slot-time
dtim period: 1
beacon int: 100

Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
and verified by sniffing frames over the air with Wireshark to ensure the
MCS indices match.

Signed-off-by: Edmond Gagnon <[email protected]>
Reviewed-by: Benjamin Li <[email protected]>
---

Changes in v4:
- Shortened very long line in smd.c
- Fixed every checkpatch issue I could find:
scripts/checkpatch.pl --strict
0001-wcn36xx-Implement-tx_rate-reporting.patch
total: 0 errors, 0 warnings, 0 checks, 156 lines checked
Changes in v3:
- Refactored to report tx_rate via ieee80211_ops::sta_statistics
- Dropped get_sta_index patch
- Addressed style comments
Changes in v2:
- Refactored to use existing wcn36xx_hal_get_stats_{req,rsp}_msg structs.
- Added more notes about testing.
- Reduced reporting interval to 3000msec.
- Assorted type and memory safety fixes.
- Make wcn36xx_smd_get_stats friendlier to future message implementors.

drivers/net/wireless/ath/wcn36xx/hal.h | 7 ++-
drivers/net/wireless/ath/wcn36xx/main.c | 16 +++++++
drivers/net/wireless/ath/wcn36xx/smd.c | 57 +++++++++++++++++++++++++
drivers/net/wireless/ath/wcn36xx/smd.h | 2 +
drivers/net/wireless/ath/wcn36xx/txrx.c | 29 +++++++++++++
drivers/net/wireless/ath/wcn36xx/txrx.h | 1 +
6 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
index 2a1db9756fd5..46a49f0a51b3 100644
--- a/drivers/net/wireless/ath/wcn36xx/hal.h
+++ b/drivers/net/wireless/ath/wcn36xx/hal.h
@@ -2626,7 +2626,12 @@ enum tx_rate_info {
HAL_TX_RATE_SGI = 0x8,

/* Rate with Long guard interval */
- HAL_TX_RATE_LGI = 0x10
+ HAL_TX_RATE_LGI = 0x10,
+
+ /* VHT rates */
+ HAL_TX_RATE_VHT20 = 0x20,
+ HAL_TX_RATE_VHT40 = 0x40,
+ HAL_TX_RATE_VHT80 = 0x80,
};

struct ani_global_class_a_stats_info {
diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index b545d4b2b8c4..fc76b090c39f 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -1400,6 +1400,21 @@ static int wcn36xx_get_survey(struct ieee80211_hw *hw, int idx,
return 0;
}

+static void wcn36xx_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
+ struct ieee80211_sta *sta, struct station_info *sinfo)
+{
+ struct wcn36xx *wcn;
+ u8 sta_index;
+ int status = 0;
+
+ wcn = hw->priv;
+ sta_index = get_sta_index(vif, wcn36xx_sta_to_priv(sta));
+ status = wcn36xx_smd_get_stats(wcn, sta_index, HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
+
+ if (status)
+ wcn36xx_err("wcn36xx_smd_get_stats failed\n");
+}
+
static const struct ieee80211_ops wcn36xx_ops = {
.start = wcn36xx_start,
.stop = wcn36xx_stop,
@@ -1423,6 +1438,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
.set_rts_threshold = wcn36xx_set_rts_threshold,
.sta_add = wcn36xx_sta_add,
.sta_remove = wcn36xx_sta_remove,
+ .sta_statistics = wcn36xx_sta_statistics,
.ampdu_action = wcn36xx_ampdu_action,
#if IS_ENABLED(CONFIG_IPV6)
.ipv6_addr_change = wcn36xx_ipv6_addr_change,
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index caeb68901326..a2188b41e308 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2627,6 +2627,62 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index)
return ret;
}

+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
+ struct station_info *sinfo)
+{
+ struct wcn36xx_hal_stats_req_msg msg_body;
+ struct wcn36xx_hal_stats_rsp_msg *rsp;
+ void *rsp_body;
+ int ret = 0;
+
+ if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
+ stats_mask);
+ return -EINVAL;
+ }
+
+ mutex_lock(&wcn->hal_mutex);
+ INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
+
+ msg_body.sta_id = sta_index;
+ msg_body.stats_mask = stats_mask;
+
+ PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
+
+ ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
+ if (ret) {
+ wcn36xx_err("sending hal_get_stats failed\n");
+ goto out;
+ }
+
+ ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
+ if (ret) {
+ wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
+ goto out;
+ }
+
+ rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
+ rsp_body = (wcn->hal_buf + sizeof(struct wcn36xx_hal_stats_rsp_msg));
+
+ if (rsp->stats_mask != stats_mask) {
+ wcn36xx_err("stats_mask 0x%x differs from requested 0x%x\n",
+ rsp->stats_mask, stats_mask);
+ goto out;
+ }
+
+ if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
+ struct ani_global_class_a_stats_info *stats_info = rsp_body;
+
+ wcn36xx_process_tx_rate(stats_info, &sinfo->txrate);
+ sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
+ rsp_body += sizeof(struct ani_global_class_a_stats_info);
+ }
+out:
+ mutex_unlock(&wcn->hal_mutex);
+
+ return ret;
+}
+
static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len, struct add_ba_info *ba_info)
{
struct wcn36xx_hal_trigger_ba_rsp_candidate *candidate;
@@ -3316,6 +3372,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
case WCN36XX_HAL_ADD_BA_SESSION_RSP:
case WCN36XX_HAL_ADD_BA_RSP:
case WCN36XX_HAL_DEL_BA_RSP:
+ case WCN36XX_HAL_GET_STATS_RSP:
case WCN36XX_HAL_TRIGGER_BA_RSP:
case WCN36XX_HAL_UPDATE_CFG_RSP:
case WCN36XX_HAL_JOIN_RSP:
diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
index 957cfa87fbde..3fd598ac2a27 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.h
+++ b/drivers/net/wireless/ath/wcn36xx/smd.h
@@ -138,6 +138,8 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index);
int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u16 *ssn);
+int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
+ struct station_info *sinfo);

int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);

diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
index df749b114568..8da3955995b6 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.c
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
@@ -699,3 +699,32 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,

return ret;
}
+
+void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info)
+{
+ /* tx_rate is in units of 500kbps; mac80211 wants them in 100kbps */
+ if (stats->tx_rate_flags & HAL_TX_RATE_LEGACY)
+ info->legacy = stats->tx_rate * 5;
+
+ info->flags = 0;
+ info->mcs = stats->mcs_index;
+ info->nss = 1;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_HT40))
+ info->flags |= RATE_INFO_FLAGS_MCS;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_VHT20 | HAL_TX_RATE_VHT40 | HAL_TX_RATE_VHT80))
+ info->flags |= RATE_INFO_FLAGS_VHT_MCS;
+
+ if (stats->tx_rate_flags & HAL_TX_RATE_SGI)
+ info->flags |= RATE_INFO_FLAGS_SHORT_GI;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_VHT20))
+ info->bw = RATE_INFO_BW_20;
+
+ if (stats->tx_rate_flags & (HAL_TX_RATE_HT40 | HAL_TX_RATE_VHT40))
+ info->bw = RATE_INFO_BW_40;
+
+ if (stats->tx_rate_flags & HAL_TX_RATE_VHT80)
+ info->bw = RATE_INFO_BW_80;
+}
diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.h b/drivers/net/wireless/ath/wcn36xx/txrx.h
index b54311ffde9c..fb0d6cabd52b 100644
--- a/drivers/net/wireless/ath/wcn36xx/txrx.h
+++ b/drivers/net/wireless/ath/wcn36xx/txrx.h
@@ -164,5 +164,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
int wcn36xx_start_tx(struct wcn36xx *wcn,
struct wcn36xx_sta *sta_priv,
struct sk_buff *skb);
+void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info);

#endif /* _TXRX_H_ */
--
2.25.1

2022-03-26 00:22:54

by Jeff Johnson

[permalink] [raw]
Subject: Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

On 3/25/2022 3:42 PM, Edmond Gagnon wrote:
> Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
> rate:
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5200
> RX: 4141 bytes (32 packets)
> TX: 2082 bytes (15 packets)
> signal: -77 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 6.0 MBit/s
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
> firmware message and reports it via ieee80211_ops::sta_statistics.
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5700
> RX: 26788094 bytes (19859 packets)
> TX: 1101376 bytes (12119 packets)
> signal: -75 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
> and verified by sniffing frames over the air with Wireshark to ensure the
> MCS indices match.
>
> Signed-off-by: Edmond Gagnon <[email protected]>
> Reviewed-by: Benjamin Li <[email protected]>
> ---
>
> Changes in v4:
> - Shortened very long line in smd.c
> - Fixed every checkpatch issue I could find:
> scripts/checkpatch.pl --strict
> 0001-wcn36xx-Implement-tx_rate-reporting.patch
> total: 0 errors, 0 warnings, 0 checks, 156 lines checked
> Changes in v3:
> - Refactored to report tx_rate via ieee80211_ops::sta_statistics
> - Dropped get_sta_index patch
> - Addressed style comments
> Changes in v2:
> - Refactored to use existing wcn36xx_hal_get_stats_{req,rsp}_msg structs.
> - Added more notes about testing.
> - Reduced reporting interval to 3000msec.
> - Assorted type and memory safety fixes.
> - Make wcn36xx_smd_get_stats friendlier to future message implementors.
>
> drivers/net/wireless/ath/wcn36xx/hal.h | 7 ++-
> drivers/net/wireless/ath/wcn36xx/main.c | 16 +++++++
> drivers/net/wireless/ath/wcn36xx/smd.c | 57 +++++++++++++++++++++++++
> drivers/net/wireless/ath/wcn36xx/smd.h | 2 +
> drivers/net/wireless/ath/wcn36xx/txrx.c | 29 +++++++++++++
> drivers/net/wireless/ath/wcn36xx/txrx.h | 1 +
> 6 files changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/hal.h b/drivers/net/wireless/ath/wcn36xx/hal.h
> index 2a1db9756fd5..46a49f0a51b3 100644
> --- a/drivers/net/wireless/ath/wcn36xx/hal.h
> +++ b/drivers/net/wireless/ath/wcn36xx/hal.h
> @@ -2626,7 +2626,12 @@ enum tx_rate_info {
> HAL_TX_RATE_SGI = 0x8,
>
> /* Rate with Long guard interval */
> - HAL_TX_RATE_LGI = 0x10
> + HAL_TX_RATE_LGI = 0x10,
> +
> + /* VHT rates */
> + HAL_TX_RATE_VHT20 = 0x20,
> + HAL_TX_RATE_VHT40 = 0x40,
> + HAL_TX_RATE_VHT80 = 0x80,
> };
>
> struct ani_global_class_a_stats_info {
> diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
> index b545d4b2b8c4..fc76b090c39f 100644
> --- a/drivers/net/wireless/ath/wcn36xx/main.c
> +++ b/drivers/net/wireless/ath/wcn36xx/main.c
> @@ -1400,6 +1400,21 @@ static int wcn36xx_get_survey(struct ieee80211_hw *hw, int idx,
> return 0;
> }
>
> +static void wcn36xx_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> + struct ieee80211_sta *sta, struct station_info *sinfo)
> +{
> + struct wcn36xx *wcn;
> + u8 sta_index;
> + int status = 0;

remove initializer that is always overwritten

> +
> + wcn = hw->priv;
> + sta_index = get_sta_index(vif, wcn36xx_sta_to_priv(sta));
> + status = wcn36xx_smd_get_stats(wcn, sta_index, HAL_GLOBAL_CLASS_A_STATS_INFO, sinfo);
> +
> + if (status)
> + wcn36xx_err("wcn36xx_smd_get_stats failed\n");
> +}
> +
> static const struct ieee80211_ops wcn36xx_ops = {
> .start = wcn36xx_start,
> .stop = wcn36xx_stop,
> @@ -1423,6 +1438,7 @@ static const struct ieee80211_ops wcn36xx_ops = {
> .set_rts_threshold = wcn36xx_set_rts_threshold,
> .sta_add = wcn36xx_sta_add,
> .sta_remove = wcn36xx_sta_remove,
> + .sta_statistics = wcn36xx_sta_statistics,
> .ampdu_action = wcn36xx_ampdu_action,
> #if IS_ENABLED(CONFIG_IPV6)
> .ipv6_addr_change = wcn36xx_ipv6_addr_change,
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
> index caeb68901326..a2188b41e308 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.c
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.c
> @@ -2627,6 +2627,62 @@ int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index)
> return ret;
> }
>
> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
> + struct station_info *sinfo)
> +{
> + struct wcn36xx_hal_stats_req_msg msg_body;
> + struct wcn36xx_hal_stats_rsp_msg *rsp;
> + void *rsp_body;
> + int ret = 0;

remove initializer that is always overwritten before use

> +
> + if (stats_mask & ~HAL_GLOBAL_CLASS_A_STATS_INFO) {
> + wcn36xx_err("stats_mask 0x%x contains unimplemented types\n",
> + stats_mask);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&wcn->hal_mutex);
> + INIT_HAL_MSG(msg_body, WCN36XX_HAL_GET_STATS_REQ);
> +
> + msg_body.sta_id = sta_index;
> + msg_body.stats_mask = stats_mask;
> +
> + PREPARE_HAL_BUF(wcn->hal_buf, msg_body);
> +
> + ret = wcn36xx_smd_send_and_wait(wcn, msg_body.header.len);
> + if (ret) {
> + wcn36xx_err("sending hal_get_stats failed\n");
> + goto out;
> + }
> +
> + ret = wcn36xx_smd_rsp_status_check(wcn->hal_buf, wcn->hal_rsp_len);
> + if (ret) {
> + wcn36xx_err("hal_get_stats response failed err=%d\n", ret);
> + goto out;
> + }
> +
> + rsp = (struct wcn36xx_hal_stats_rsp_msg *)wcn->hal_buf;
> + rsp_body = (wcn->hal_buf + sizeof(struct wcn36xx_hal_stats_rsp_msg));
> +
> + if (rsp->stats_mask != stats_mask) {
> + wcn36xx_err("stats_mask 0x%x differs from requested 0x%x\n",
> + rsp->stats_mask, stats_mask);
> + goto out;
> + }
> +
> + if (rsp->stats_mask & HAL_GLOBAL_CLASS_A_STATS_INFO) {
> + struct ani_global_class_a_stats_info *stats_info = rsp_body;
> +
> + wcn36xx_process_tx_rate(stats_info, &sinfo->txrate);
> + sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_BITRATE);
> + rsp_body += sizeof(struct ani_global_class_a_stats_info);
> + }
> +out:
> + mutex_unlock(&wcn->hal_mutex);
> +
> + return ret;
> +}
> +
> static int wcn36xx_smd_trigger_ba_rsp(void *buf, int len, struct add_ba_info *ba_info)
> {
> struct wcn36xx_hal_trigger_ba_rsp_candidate *candidate;
> @@ -3316,6 +3372,7 @@ int wcn36xx_smd_rsp_process(struct rpmsg_device *rpdev,
> case WCN36XX_HAL_ADD_BA_SESSION_RSP:
> case WCN36XX_HAL_ADD_BA_RSP:
> case WCN36XX_HAL_DEL_BA_RSP:
> + case WCN36XX_HAL_GET_STATS_RSP:
> case WCN36XX_HAL_TRIGGER_BA_RSP:
> case WCN36XX_HAL_UPDATE_CFG_RSP:
> case WCN36XX_HAL_JOIN_RSP:
> diff --git a/drivers/net/wireless/ath/wcn36xx/smd.h b/drivers/net/wireless/ath/wcn36xx/smd.h
> index 957cfa87fbde..3fd598ac2a27 100644
> --- a/drivers/net/wireless/ath/wcn36xx/smd.h
> +++ b/drivers/net/wireless/ath/wcn36xx/smd.h
> @@ -138,6 +138,8 @@ int wcn36xx_smd_add_ba_session(struct wcn36xx *wcn,
> int wcn36xx_smd_add_ba(struct wcn36xx *wcn, u8 session_id);
> int wcn36xx_smd_del_ba(struct wcn36xx *wcn, u16 tid, u8 direction, u8 sta_index);
> int wcn36xx_smd_trigger_ba(struct wcn36xx *wcn, u8 sta_index, u16 tid, u16 *ssn);
> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32 stats_mask,
> + struct station_info *sinfo);
>
> int wcn36xx_smd_update_cfg(struct wcn36xx *wcn, u32 cfg_id, u32 value);
>
> diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.c b/drivers/net/wireless/ath/wcn36xx/txrx.c
> index df749b114568..8da3955995b6 100644
> --- a/drivers/net/wireless/ath/wcn36xx/txrx.c
> +++ b/drivers/net/wireless/ath/wcn36xx/txrx.c
> @@ -699,3 +699,32 @@ int wcn36xx_start_tx(struct wcn36xx *wcn,
>
> return ret;
> }
> +
> +void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info)
> +{
> + /* tx_rate is in units of 500kbps; mac80211 wants them in 100kbps */
> + if (stats->tx_rate_flags & HAL_TX_RATE_LEGACY)
> + info->legacy = stats->tx_rate * 5;
> +
> + info->flags = 0;
> + info->mcs = stats->mcs_index;
> + info->nss = 1;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_HT40))
> + info->flags |= RATE_INFO_FLAGS_MCS;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_VHT20 | HAL_TX_RATE_VHT40 | HAL_TX_RATE_VHT80))
> + info->flags |= RATE_INFO_FLAGS_VHT_MCS;
> +
> + if (stats->tx_rate_flags & HAL_TX_RATE_SGI)
> + info->flags |= RATE_INFO_FLAGS_SHORT_GI;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT20 | HAL_TX_RATE_VHT20))
> + info->bw = RATE_INFO_BW_20;
> +
> + if (stats->tx_rate_flags & (HAL_TX_RATE_HT40 | HAL_TX_RATE_VHT40))
> + info->bw = RATE_INFO_BW_40;
> +
> + if (stats->tx_rate_flags & HAL_TX_RATE_VHT80)
> + info->bw = RATE_INFO_BW_80;
> +}
> diff --git a/drivers/net/wireless/ath/wcn36xx/txrx.h b/drivers/net/wireless/ath/wcn36xx/txrx.h
> index b54311ffde9c..fb0d6cabd52b 100644
> --- a/drivers/net/wireless/ath/wcn36xx/txrx.h
> +++ b/drivers/net/wireless/ath/wcn36xx/txrx.h
> @@ -164,5 +164,6 @@ int wcn36xx_rx_skb(struct wcn36xx *wcn, struct sk_buff *skb);
> int wcn36xx_start_tx(struct wcn36xx *wcn,
> struct wcn36xx_sta *sta_priv,
> struct sk_buff *skb);
> +void wcn36xx_process_tx_rate(struct ani_global_class_a_stats_info *stats, struct rate_info *info);
>
> #endif /* _TXRX_H_ */

2022-03-26 13:54:21

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

Jeff Johnson <[email protected]> writes:

> On 3/25/2022 3:42 PM, Edmond Gagnon wrote:
>> Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
>> rate:
>>
>> root@linaro-developer:~# iw wlan0 link
>> Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
>> SSID: SQ-DEVICETEST
>> freq: 5200
>> RX: 4141 bytes (32 packets)
>> TX: 2082 bytes (15 packets)
>> signal: -77 dBm
>> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
>> tx bitrate: 6.0 MBit/s
>>
>> bss flags: short-slot-time
>> dtim period: 1
>> beacon int: 100
>>
>> This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
>> firmware message and reports it via ieee80211_ops::sta_statistics.
>>
>> root@linaro-developer:~# iw wlan0 link
>> Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
>> SSID: SQ-DEVICETEST
>> freq: 5700
>> RX: 26788094 bytes (19859 packets)
>> TX: 1101376 bytes (12119 packets)
>> signal: -75 dBm
>> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
>> tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1
>>
>> bss flags: short-slot-time
>> dtim period: 1
>> beacon int: 100
>>
>> Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
>> and verified by sniffing frames over the air with Wireshark to ensure the
>> MCS indices match.
>>
>> Signed-off-by: Edmond Gagnon <[email protected]>
>> Reviewed-by: Benjamin Li <[email protected]>

[...]

>> +static void wcn36xx_sta_statistics(struct ieee80211_hw *hw,
>> struct ieee80211_vif *vif,
>> + struct ieee80211_sta *sta, struct station_info *sinfo)
>> +{
>> + struct wcn36xx *wcn;
>> + u8 sta_index;
>> + int status = 0;
>
> remove initializer that is always overwritten

I can fix that in the pending branch, no need to resend because of this.

>> +int wcn36xx_smd_get_stats(struct wcn36xx *wcn, u8 sta_index, u32
>> stats_mask,
>> + struct station_info *sinfo)
>> +{
>> + struct wcn36xx_hal_stats_req_msg msg_body;
>> + struct wcn36xx_hal_stats_rsp_msg *rsp;
>> + void *rsp_body;
>> + int ret = 0;
>
> remove initializer that is always overwritten before use

Ditto.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2022-03-30 10:19:11

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

Edmond Gagnon <[email protected]> wrote:

> Currently, the driver reports a tx_rate of 6.0 MBit/s no matter the true
> rate:
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:9b:92 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5200
> RX: 4141 bytes (32 packets)
> TX: 2082 bytes (15 packets)
> signal: -77 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 6.0 MBit/s
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> This patch requests HAL_GLOBAL_CLASS_A_STATS_INFO via a hal_get_stats
> firmware message and reports it via ieee80211_ops::sta_statistics.
>
> root@linaro-developer:~# iw wlan0 link
> Connected to 6c:f3:7f:eb:73:b2 (on wlan0)
> SSID: SQ-DEVICETEST
> freq: 5700
> RX: 26788094 bytes (19859 packets)
> TX: 1101376 bytes (12119 packets)
> signal: -75 dBm
> rx bitrate: 135.0 MBit/s MCS 6 40MHz short GI
> tx bitrate: 108.0 MBit/s VHT-MCS 5 40MHz VHT-NSS 1
>
> bss flags: short-slot-time
> dtim period: 1
> beacon int: 100
>
> Tested on MSM8939 with WCN3680B running firmware CNSS-PR-2-0-1-2-c1-00083,
> and verified by sniffing frames over the air with Wireshark to ensure the
> MCS indices match.
>
> Signed-off-by: Edmond Gagnon <[email protected]>
> Reviewed-by: Benjamin Li <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

Patch applied to ath-next branch of ath.git, thanks.

1216c4d30723 wcn36xx: Implement tx_rate reporting

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches