From: Bryan O'Donoghue <pure.logic@nexus-software.ie>
To: Edmond Gagnon <egagnon@squareup.com>, Kalle Valo <kvalo@codeaurora.org>
Cc: Benjamin Li <benl@squareup.com>, Kalle Valo <kvalo@kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>,
wcn36xx@lists.infradead.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] wcn36xx: Implement tx_rate reporting
Date: Thu, 24 Mar 2022 12:42:23 +0000 [thread overview]
Message-ID: <9c90300e-ac2d-be53-a7c9-7b00a059204e@nexus-software.ie> (raw)
In-Reply-To: <20220323214533.1951791-1-egagnon@squareup.com>
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>
next prev parent reply other threads:[~2022-03-24 12:42 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-18 19:58 [PATCH v2 0/2] " Edmond Gagnon
2022-03-18 19:58 ` [PATCH v2 1/2] wcn36xx: Expose get_sta_index in wcn36xx.h Edmond Gagnon
2022-03-18 19:58 ` [PATCH v2 2/2] wcn36xx: Implement tx_rate reporting Edmond Gagnon
2022-03-18 23:23 ` kernel test robot
2022-03-20 13:21 ` Bryan O'Donoghue
2022-03-20 18:03 ` Bryan O'Donoghue
2022-03-20 19:15 ` Kalle Valo
2022-03-20 19:17 ` Kalle Valo
2022-03-23 21:45 ` [PATCH v3] " Edmond Gagnon
2022-03-23 21:55 ` Benjamin Li
2022-03-24 12:42 ` Bryan O'Donoghue [this message]
2022-03-24 14:41 ` Bryan O'Donoghue
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9c90300e-ac2d-be53-a7c9-7b00a059204e@nexus-software.ie \
--to=pure.logic@nexus-software.ie \
--cc=benl@squareup.com \
--cc=davem@davemloft.net \
--cc=egagnon@squareup.com \
--cc=kuba@kernel.org \
--cc=kvalo@codeaurora.org \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=wcn36xx@lists.infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®