* Re: [PATCH v3] wcn36xx: Implement tx_rate reporting
2022-03-23 21:45 ` [PATCH v3] " Edmond Gagnon
@ 2022-03-23 21:55 ` Benjamin Li
2022-03-24 12:42 ` Bryan O'Donoghue
2022-03-25 22:42 ` [PATCH v4] " Edmond Gagnon
2 siblings, 0 replies; 16+ messages in thread
From: Benjamin Li @ 2022-03-23 21:55 UTC (permalink / raw)
To: Edmond Gagnon, Kalle Valo
Cc: Kalle Valo, David S. Miller, Jakub Kicinski, wcn36xx,
linux-wireless, netdev, linux-kernel
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_ */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] wcn36xx: Implement tx_rate reporting
2022-03-23 21:45 ` [PATCH v3] " Edmond Gagnon
2022-03-23 21:55 ` Benjamin Li
@ 2022-03-24 12:42 ` Bryan O'Donoghue
2022-03-24 14:41 ` Bryan O'Donoghue
2022-03-25 22:42 ` [PATCH v4] " Edmond Gagnon
2 siblings, 1 reply; 16+ messages in thread
From: Bryan O'Donoghue @ 2022-03-24 12:42 UTC (permalink / raw)
To: Edmond Gagnon, Kalle Valo
Cc: Benjamin Li, Kalle Valo, David S. Miller, Jakub Kicinski,
wcn36xx, linux-wireless, netdev, linux-kernel
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3] wcn36xx: Implement tx_rate reporting
2022-03-24 12:42 ` Bryan O'Donoghue
@ 2022-03-24 14:41 ` Bryan O'Donoghue
0 siblings, 0 replies; 16+ messages in thread
From: Bryan O'Donoghue @ 2022-03-24 14:41 UTC (permalink / raw)
To: Edmond Gagnon, Kalle Valo
Cc: Benjamin Li, Kalle Valo, David S. Miller, Jakub Kicinski,
wcn36xx, linux-wireless, netdev, linux-kernel
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4] wcn36xx: Implement tx_rate reporting
2022-03-23 21:45 ` [PATCH v3] " Edmond Gagnon
2022-03-23 21:55 ` Benjamin Li
2022-03-24 12:42 ` Bryan O'Donoghue
@ 2022-03-25 22:42 ` Edmond Gagnon
2022-03-26 0:09 ` Jeff Johnson
2022-03-30 8:04 ` Kalle Valo
2 siblings, 2 replies; 16+ messages in thread
From: Edmond Gagnon @ 2022-03-25 22:42 UTC (permalink / raw)
To: Kalle Valo
Cc: Edmond Gagnon, Benjamin Li, Kalle Valo, David S. Miller,
Jakub Kicinski, wcn36xx, linux-wireless, netdev, linux-kernel
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v4] wcn36xx: Implement tx_rate reporting
2022-03-25 22:42 ` [PATCH v4] " Edmond Gagnon
@ 2022-03-26 0:09 ` Jeff Johnson
2022-03-26 8:50 ` Kalle Valo
2022-03-30 8:04 ` Kalle Valo
1 sibling, 1 reply; 16+ messages in thread
From: Jeff Johnson @ 2022-03-26 0:09 UTC (permalink / raw)
To: Edmond Gagnon, Kalle Valo
Cc: Benjamin Li, Kalle Valo, David S. Miller, Jakub Kicinski,
wcn36xx, linux-wireless, netdev, linux-kernel
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_ */
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] wcn36xx: Implement tx_rate reporting
2022-03-26 0:09 ` Jeff Johnson
@ 2022-03-26 8:50 ` Kalle Valo
0 siblings, 0 replies; 16+ messages in thread
From: Kalle Valo @ 2022-03-26 8:50 UTC (permalink / raw)
To: Jeff Johnson
Cc: Edmond Gagnon, Benjamin Li, David S. Miller, Jakub Kicinski,
wcn36xx, linux-wireless, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4] wcn36xx: Implement tx_rate reporting
2022-03-25 22:42 ` [PATCH v4] " Edmond Gagnon
2022-03-26 0:09 ` Jeff Johnson
@ 2022-03-30 8:04 ` Kalle Valo
1 sibling, 0 replies; 16+ messages in thread
From: Kalle Valo @ 2022-03-30 8:04 UTC (permalink / raw)
To: Edmond Gagnon
Cc: Kalle Valo, Edmond Gagnon, Benjamin Li, David S. Miller,
Jakub Kicinski, wcn36xx, linux-wireless, netdev, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread