[PATCH v2 0/2] wcn36xx: Implement tx_rate reporting

[PATCH v2 0/2] wcn36xx: Implement tx_rate reporting

[Edmond Gagnon]

Implements HAL_GET_STATS message in order to report tx_rate to
ieee80211_tx_rate_update. Tested on MSM8939 with WCN3680B running
firmware CNSS-PR-2-0-1-2-c1-00083 on 5.17.

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.

Edmond Gagnon (2):
  wcn36xx: Expose get_sta_index in wcn36xx.h
  wcn36xx: Implement tx_rate reporting

 drivers/net/wireless/ath/wcn36xx/hal.h     |  7 ++-
 drivers/net/wireless/ath/wcn36xx/main.c    | 33 +++++++++---
 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 | 12 +++++
 7 files changed, 163 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] wcn36xx: Expose get_sta_index in wcn36xx.h

[Edmond Gagnon]

Move this helper into wcn36xx.h for use in smd.c when constructing
HAL_GET_STATS messages.

Signed-off-by: Edmond Gagnon <egagnon@squareup.com>
Reviewed-by: Benjamin Li <benl@squareup.com>
---
 drivers/net/wireless/ath/wcn36xx/main.c    | 8 --------
 drivers/net/wireless/ath/wcn36xx/wcn36xx.h | 8 ++++++++
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/wcn36xx/main.c b/drivers/net/wireless/ath/wcn36xx/main.c
index b545d4b2b8c4..70531f62777e 100644
--- a/drivers/net/wireless/ath/wcn36xx/main.c
+++ b/drivers/net/wireless/ath/wcn36xx/main.c
@@ -184,14 +184,6 @@ static const struct wiphy_wowlan_support wowlan_support = {
 
 #endif
 
-static inline u8 get_sta_index(struct ieee80211_vif *vif,
-			       struct wcn36xx_sta *sta_priv)
-{
-	return NL80211_IFTYPE_STATION == vif->type ?
-	       sta_priv->bss_sta_index :
-	       sta_priv->sta_index;
-}
-
 static const char * const wcn36xx_caps_names[] = {
 	"MCC",				/* 0 */
 	"P2P",				/* 1 */
diff --git a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
index 9aa08b636d08..80a4e7aea419 100644
--- a/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
+++ b/drivers/net/wireless/ath/wcn36xx/wcn36xx.h
@@ -335,4 +335,12 @@ struct wcn36xx_sta *wcn36xx_sta_to_priv(struct ieee80211_sta *sta)
 	return (struct wcn36xx_sta *)sta->drv_priv;
 }
 
+static inline u8 get_sta_index(struct ieee80211_vif *vif,
+			       struct wcn36xx_sta *sta_priv)
+{
+	return vif->type == NL80211_IFTYPE_STATION ?
+	       sta_priv->bss_sta_index :
+	       sta_priv->sta_index;
+}
+
 #endif	/* _WCN36XX_H_ */
-- 
2.25.1

[PATCH v2 2/2] wcn36xx: Implement tx_rate reporting

[Edmond Gagnon]

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 <egagnon@squareup.com>
Reviewed-by: Benjamin Li <benl@squareup.com>
---
 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

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

[kernel test robot]

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/202203190720.E8jZHrLo-lkp@intel.com/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 <lkp@intel.com>

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/kbuild-all@lists.01.org

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

[Bryan O'Donoghue]

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

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

[Bryan O'Donoghue]

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

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

[Kalle Valo]

Bryan O'Donoghue <bryan.odonoghue@linaro.org> 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

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

[Kalle Valo]

Edmond Gagnon <egagnon@squareup.com> writes:

> 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 <egagnon@squareup.com>
> Reviewed-by: Benjamin Li <benl@squareup.com>

[...]

> +	// 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;

No C++ comments, please. And IMHO the comments are not really providing
any extra value anyway.

https://www.kernel.org/doc/html/latest/process/coding-style.html

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

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

[PATCH v3] wcn36xx: Implement tx_rate reporting

[Edmond Gagnon]

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 <egagnon@squareup.com>
Reviewed-by: Benjamin Li <benl@squareup.com>
---

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

Re: [PATCH v3] wcn36xx: Implement tx_rate reporting

[Benjamin Li]

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 <benl@squareup.com>

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 <egagnon@squareup.com>
> Reviewed-by: Benjamin Li <benl@squareup.com>
> ---
> 
> 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_ */

Re: [PATCH v3] wcn36xx: Implement tx_rate reporting

[Bryan O'Donoghue]

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 <egagnon@squareup.com>
> Reviewed-by: Benjamin Li <benl@squareup.com>
> ---
> 
> 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 <bryan.odonoghue@linaro.org>

Re: [PATCH v3] wcn36xx: Implement tx_rate reporting

[Bryan O'Donoghue]

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 <egagnon@squareup.com>
>> Reviewed-by: Benjamin Li <benl@squareup.com>
>> ---
>>
>> 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 <bryan.odonoghue@linaro.org>

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

[PATCH v4] wcn36xx: Implement tx_rate reporting

[Edmond Gagnon]

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 <egagnon@squareup.com>
Reviewed-by: Benjamin Li <benl@squareup.com>
---

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

Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

[Jeff Johnson]

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 <egagnon@squareup.com>
> Reviewed-by: Benjamin Li <benl@squareup.com>
> ---
> 
> 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_ */

Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

[Kalle Valo]

Jeff Johnson <quic_jjohnson@quicinc.com> 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 <egagnon@squareup.com>
>> Reviewed-by: Benjamin Li <benl@squareup.com>

[...]

>>   +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

Re: [PATCH v4] wcn36xx: Implement tx_rate reporting

[Kalle Valo]

Edmond Gagnon <egagnon@squareup.com> 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 <egagnon@squareup.com>
> Reviewed-by: Benjamin Li <benl@squareup.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

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

1216c4d30723 wcn36xx: Implement tx_rate reporting

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20220325224212.159690-1-egagnon@squareup.com/

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