The tx bitrate of "iw dev xxx station dump" always show an invalid value
"tx bitrate: 6.0MBit/s".
To address this issue, parse the tx complete report from firmware and
indicate the tx rate to mac80211.
After that, "iw dev xxx station dump" show the correct tx bitrate such as:
tx bitrate: 104.0 MBit/s MCS 13
tx bitrate: 144.4 MBit/s MCS 15 short GI
tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
Signed-off-by: Lingbo Kong <[email protected]>
---
v4:
1.delete single_pdev_only judgement
2.update hal_tx.h copyright to include 2024
3.remove extraneous code
v3:
1.update hal_tx.h copyright to include 2023
v2:
1.modify types
drivers/net/wireless/ath/ath12k/core.h | 1 +
drivers/net/wireless/ath/ath12k/dp_mon.c | 2 +-
drivers/net/wireless/ath/ath12k/dp_rx.c | 6 +-
drivers/net/wireless/ath/ath12k/dp_tx.c | 121 ++++++++++++++++++++++-
drivers/net/wireless/ath/ath12k/hal_tx.h | 9 +-
drivers/net/wireless/ath/ath12k/mac.c | 92 +++++++++++++++++
drivers/net/wireless/ath/ath12k/mac.h | 3 +
7 files changed, 224 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/ath/ath12k/core.h b/drivers/net/wireless/ath/ath12k/core.h
index 97e5a0ccd233..2c8e23ab894f 100644
--- a/drivers/net/wireless/ath/ath12k/core.h
+++ b/drivers/net/wireless/ath/ath12k/core.h
@@ -425,6 +425,7 @@ struct ath12k_sta {
struct ath12k_rx_peer_stats *rx_stats;
struct ath12k_wbm_tx_stats *wbm_tx_stats;
u32 bw_prev;
+ u32 peer_nss;
};
#define ATH12K_MIN_5G_FREQ 4150
diff --git a/drivers/net/wireless/ath/ath12k/dp_mon.c b/drivers/net/wireless/ath/ath12k/dp_mon.c
index 2d56913a75d0..c3466c493c5f 100644
--- a/drivers/net/wireless/ath/ath12k/dp_mon.c
+++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
@@ -290,7 +290,7 @@ static void ath12k_dp_mon_parse_he_sig_b1_mu(u8 *tlv_data,
ru_tones = u32_get_bits(info0,
HAL_RX_HE_SIG_B1_MU_INFO_INFO0_RU_ALLOCATION);
- ppdu_info->ru_alloc = ath12k_he_ru_tones_to_nl80211_he_ru_alloc(ru_tones);
+ ppdu_info->ru_alloc = ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(ru_tones);
ppdu_info->he_RU[0] = ru_tones;
ppdu_info->reception_type = HAL_RX_RECEPTION_TYPE_MU_MIMO;
}
diff --git a/drivers/net/wireless/ath/ath12k/dp_rx.c b/drivers/net/wireless/ath/ath12k/dp_rx.c
index ca76c018dd0c..345f14cfbc9e 100644
--- a/drivers/net/wireless/ath/ath12k/dp_rx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_rx.c
@@ -1422,10 +1422,10 @@ ath12k_update_per_peer_tx_stats(struct ath12k *ar,
arsta->txrate.mcs = mcs;
arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
arsta->txrate.he_dcm = dcm;
- arsta->txrate.he_gi = ath12k_he_gi_to_nl80211_he_gi(sgi);
+ arsta->txrate.he_gi = ath12k_mac_he_gi_to_nl80211_he_gi(sgi);
tones = le16_to_cpu(user_rate->ru_end) -
le16_to_cpu(user_rate->ru_start) + 1;
- v = ath12k_he_ru_tones_to_nl80211_he_ru_alloc(tones);
+ v = ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(tones);
arsta->txrate.he_ru_alloc = v;
break;
}
@@ -2334,7 +2334,7 @@ static void ath12k_dp_rx_h_rate(struct ath12k *ar, struct hal_rx_desc *rx_desc,
}
rx_status->encoding = RX_ENC_HE;
rx_status->nss = nss;
- rx_status->he_gi = ath12k_he_gi_to_nl80211_he_gi(sgi);
+ rx_status->he_gi = ath12k_mac_he_gi_to_nl80211_he_gi(sgi);
rx_status->bw = ath12k_mac_bw_to_mac80211_bw(bw);
break;
}
diff --git a/drivers/net/wireless/ath/ath12k/dp_tx.c b/drivers/net/wireless/ath/ath12k/dp_tx.c
index 572b87153647..3189ca66d9fc 100644
--- a/drivers/net/wireless/ath/ath12k/dp_tx.c
+++ b/drivers/net/wireless/ath/ath12k/dp_tx.c
@@ -8,6 +8,8 @@
#include "dp_tx.h"
#include "debug.h"
#include "hw.h"
+#include "peer.h"
+#include "mac.h"
static enum hal_tcl_encap_type
ath12k_dp_tx_get_encap_type(struct ath12k_vif *arvif, struct sk_buff *skb)
@@ -443,6 +445,99 @@ ath12k_dp_tx_process_htt_tx_complete(struct ath12k_base *ab,
}
}
+static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
+{
+ struct ath12k_base *ab = ar->ab;
+ struct ath12k_peer *peer;
+ struct ath12k_sta *arsta;
+ struct ieee80211_sta *sta;
+ u16 rate;
+ u8 rate_idx = 0;
+ int ret;
+
+ spin_lock_bh(&ab->base_lock);
+ peer = ath12k_peer_find_by_id(ab, ts->peer_id);
+ if (!peer || !peer->sta) {
+ ath12k_dbg(ab, ATH12K_DBG_DP_TX,
+ "failed to find the peer by id %u\n", ts->peer_id);
+ goto err_out;
+ }
+
+ sta = peer->sta;
+ arsta = ath12k_sta_to_arsta(sta);
+
+ memset(&arsta->txrate, 0, sizeof(arsta->txrate));
+
+ /* This is to prefer choose the real NSS value arsta->last_txrate.nss,
+ * if it is invalid, then choose the NSS value while assoc.
+ */
+ if (arsta->last_txrate.nss)
+ arsta->txrate.nss = arsta->last_txrate.nss;
+ else
+ arsta->txrate.nss = arsta->peer_nss;
+
+ if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11A ||
+ ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11B) {
+ ret = ath12k_mac_hw_ratecode_to_legacy_rate(ts->mcs,
+ ts->pkt_type,
+ &rate_idx,
+ &rate);
+ if (ret < 0)
+ goto err_out;
+
+ arsta->txrate.legacy = rate;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11N) {
+ if (ts->mcs > ATH12K_HT_MCS_MAX)
+ goto err_out;
+
+ if (arsta->txrate.nss != 0)
+ arsta->txrate.mcs = ts->mcs + 8 * (arsta->txrate.nss - 1);
+ arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
+ if (ts->sgi)
+ arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AC) {
+ if (ts->mcs > ATH12K_VHT_MCS_MAX)
+ goto err_out;
+
+ arsta->txrate.mcs = ts->mcs;
+ arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
+ if (ts->sgi)
+ arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
+ } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
+ if (ts->mcs > ATH12K_HE_MCS_MAX)
+ goto err_out;
+
+ arsta->txrate.mcs = ts->mcs;
+ arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
+ arsta->txrate.he_gi = ath12k_mac_he_gi_to_nl80211_he_gi(ts->sgi);
+ }
+
+ arsta->txrate.bw = ath12k_mac_bw_to_mac80211_bw(ts->bw);
+ if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
+ arsta->txrate.bw = RATE_INFO_BW_HE_RU;
+ arsta->txrate.he_ru_alloc =
+ ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(ts->ru_tones);
+ }
+
+err_out:
+ spin_unlock_bh(&ab->base_lock);
+}
+
+static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
+{
+ if (ar->last_ppdu_id == 0)
+ goto update_last_ppdu_id;
+
+ if (ar->last_ppdu_id == ts->ppdu_id ||
+ ar->cached_ppdu_id == ar->last_ppdu_id)
+ ar->cached_ppdu_id = ar->last_ppdu_id;
+
+ ath12k_dp_tx_update_txcompl(ar, ts);
+
+update_last_ppdu_id:
+ ar->last_ppdu_id = ts->ppdu_id;
+}
+
static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
struct sk_buff *msdu,
struct hal_tx_status *ts)
@@ -498,6 +593,8 @@ static void ath12k_dp_tx_complete_msdu(struct ath12k *ar,
* Might end up reporting it out-of-band from HTT stats.
*/
+ ath12k_dp_tx_update(ar, ts);
+
ieee80211_tx_status_skb(ath12k_ar_to_hw(ar), msdu);
exit:
@@ -522,10 +619,26 @@ static void ath12k_dp_tx_status_parse(struct ath12k_base *ab,
ts->ppdu_id = le32_get_bits(desc->info1,
HAL_WBM_COMPL_TX_INFO1_TQM_STATUS_NUMBER);
- if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID)
- ts->rate_stats = le32_to_cpu(desc->rate_stats.info0);
- else
- ts->rate_stats = 0;
+
+ if (le32_to_cpu(desc->info2) & HAL_WBM_COMPL_TX_INFO2_FIRST_MSDU)
+ ts->flags |= HAL_TX_STATUS_FLAGS_FIRST_MSDU;
+
+ ts->peer_id = le32_get_bits(desc->info3, HAL_WBM_COMPL_TX_INFO3_PEER_ID);
+
+ if (le32_to_cpu(desc->rate_stats.info0) & HAL_TX_RATE_STATS_INFO0_VALID) {
+ ts->pkt_type = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_PKT_TYPE);
+ ts->mcs = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_MCS);
+ ts->sgi = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_SGI);
+ ts->bw = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_BW);
+ ts->ru_tones = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_TONES_IN_RU);
+ ts->ofdma = le32_get_bits(desc->rate_stats.info0,
+ HAL_TX_RATE_STATS_INFO0_OFDMA_TX);
+ }
}
void ath12k_dp_tx_completion_handler(struct ath12k_base *ab, int ring_id)
diff --git a/drivers/net/wireless/ath/ath12k/hal_tx.h b/drivers/net/wireless/ath/ath12k/hal_tx.h
index 7c837094a6f7..7cfc584649ad 100644
--- a/drivers/net/wireless/ath/ath12k/hal_tx.h
+++ b/drivers/net/wireless/ath/ath12k/hal_tx.h
@@ -1,7 +1,7 @@
/* SPDX-License-Identifier: BSD-3-Clause-Clear */
/*
* Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
- * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
*/
#ifndef ATH12K_HAL_TX_H
@@ -63,7 +63,12 @@ struct hal_tx_status {
u8 try_cnt;
u8 tid;
u16 peer_id;
- u32 rate_stats;
+ enum hal_tx_rate_stats_pkt_type pkt_type;
+ enum hal_tx_rate_stats_sgi sgi;
+ enum ath12k_supported_bw bw;
+ u8 mcs;
+ u16 ru_tones;
+ u8 ofdma;
};
#define HAL_TX_PHY_DESC_INFO0_BF_TYPE GENMASK(17, 16)
diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
index 52a5fb8b03e9..39fe845a6137 100644
--- a/drivers/net/wireless/ath/ath12k/mac.c
+++ b/drivers/net/wireless/ath/ath12k/mac.c
@@ -2365,8 +2365,12 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
struct ath12k_wmi_peer_assoc_arg *arg,
bool reassoc)
{
+ struct ath12k_sta *arsta;
+
lockdep_assert_held(&ar->conf_mutex);
+ arsta = ath12k_sta_to_arsta(sta);
+
memset(arg, 0, sizeof(*arg));
reinit_completion(&ar->peer_assoc_done);
@@ -2383,6 +2387,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
ath12k_peer_assoc_h_phymode(ar, vif, sta, arg);
ath12k_peer_assoc_h_smps(sta, arg);
+ arsta->peer_nss = arg->peer_nss;
/* TODO: amsdu_disable req? */
}
@@ -8324,3 +8329,90 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
return ret;
}
+
+enum nl80211_he_ru_alloc ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(u16 ru_phy)
+{
+ enum nl80211_he_ru_alloc ret;
+
+ switch (ru_phy) {
+ case RU_26:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ case RU_52:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
+ break;
+ case RU_106:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
+ break;
+ case RU_242:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
+ break;
+ case RU_484:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
+ break;
+ case RU_996:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ }
+
+ return ret;
+}
+
+enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
+{
+ enum nl80211_he_ru_alloc ret;
+
+ switch (ru_tones) {
+ case 26:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ case 52:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
+ break;
+ case 106:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
+ break;
+ case 242:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
+ break;
+ case 484:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
+ break;
+ case 996:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
+ break;
+ case (996 * 2):
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
+ break;
+ }
+
+ return ret;
+}
+
+enum nl80211_he_gi ath12k_mac_he_gi_to_nl80211_he_gi(u8 sgi)
+{
+ enum nl80211_he_gi ret;
+
+ switch (sgi) {
+ case RX_MSDU_START_SGI_0_8_US:
+ ret = NL80211_RATE_INFO_HE_GI_0_8;
+ break;
+ case RX_MSDU_START_SGI_1_6_US:
+ ret = NL80211_RATE_INFO_HE_GI_1_6;
+ break;
+ case RX_MSDU_START_SGI_3_2_US:
+ ret = NL80211_RATE_INFO_HE_GI_3_2;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_HE_GI_0_8;
+ break;
+ }
+
+ return ret;
+}
diff --git a/drivers/net/wireless/ath/ath12k/mac.h b/drivers/net/wireless/ath/ath12k/mac.h
index 3f5e1be0dff9..c7c48d5efca4 100644
--- a/drivers/net/wireless/ath/ath12k/mac.h
+++ b/drivers/net/wireless/ath/ath12k/mac.h
@@ -78,4 +78,7 @@ enum ath12k_supported_bw ath12k_mac_mac80211_bw_to_ath12k_bw(enum rate_info_bw b
enum hal_encrypt_type ath12k_dp_tx_get_encrypt_type(u32 cipher);
int ath12k_mac_rfkill_enable_radio(struct ath12k *ar, bool enable);
int ath12k_mac_rfkill_config(struct ath12k *ar);
+enum nl80211_he_gi ath12k_mac_he_gi_to_nl80211_he_gi(u8 sgi);
+enum nl80211_he_ru_alloc ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(u16 ru_phy);
+enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones);
#endif
base-commit: 707e306f3573fa321ae197d77366578e4566cff5
--
2.34.1
On 2/19/2024 1:58 AM, Lingbo Kong wrote:
> The tx bitrate of "iw dev xxx station dump" always show an invalid value
> "tx bitrate: 6.0MBit/s".
>
> To address this issue, parse the tx complete report from firmware and
> indicate the tx rate to mac80211.
>
> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
> tx bitrate: 104.0 MBit/s MCS 13
> tx bitrate: 144.4 MBit/s MCS 15 short GI
> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
With one correction below which Kalle can make in the pending branch
Acked-by: Jeff Johnson <[email protected]>
> diff --git a/drivers/net/wireless/ath/ath12k/hal_tx.h b/drivers/net/wireless/ath/ath12k/hal_tx.h
> index 7c837094a6f7..7cfc584649ad 100644
> --- a/drivers/net/wireless/ath/ath12k/hal_tx.h
> +++ b/drivers/net/wireless/ath/ath12k/hal_tx.h
> @@ -1,7 +1,7 @@
> /* SPDX-License-Identifier: BSD-3-Clause-Clear */
> /*
> * Copyright (c) 2018-2021 The Linux Foundation. All rights reserved.
> - * Copyright (c) 2021-2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + * Copyright (c) 2021-2024 Qualcomm Innovation Center, Inc. All rights reserved.
since no changes were made in 2023 should be 2021-2022, 2024
Lingbo Kong <[email protected]> writes:
> The tx bitrate of "iw dev xxx station dump" always show an invalid value
> "tx bitrate: 6.0MBit/s".
>
> To address this issue, parse the tx complete report from firmware and
> indicate the tx rate to mac80211.
>
> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
> tx bitrate: 104.0 MBit/s MCS 13
> tx bitrate: 144.4 MBit/s MCS 15 short GI
> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>
> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>
> Signed-off-by: Lingbo Kong <[email protected]>
Please use full englist words like transmit instead of tx. Also the
title could be simplified to:
wifi: ath12k: report station mode transmit rate to user space
Here I assumed this only works in station mode. Or does this also
support AP and P2P mode? The commit message should explain that.
> --- a/drivers/net/wireless/ath/ath12k/dp_mon.c
> +++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
> @@ -290,7 +290,7 @@ static void ath12k_dp_mon_parse_he_sig_b1_mu(u8 *tlv_data,
>
> ru_tones = u32_get_bits(info0,
> HAL_RX_HE_SIG_B1_MU_INFO_INFO0_RU_ALLOCATION);
> - ppdu_info->ru_alloc = ath12k_he_ru_tones_to_nl80211_he_ru_alloc(ru_tones);
> + ppdu_info->ru_alloc = ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(ru_tones);
Now there would be two very similar functions:
ath12k_mac_he_gi_to_nl80211_he_gi() vs ath12k_he_gi_to_nl80211_he_gi()
ath12k_he_ru_tones_to_nl80211_he_ru_alloc() vs ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc()
Why do we need those?
> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + struct ath12k_base *ab = ar->ab;
> + struct ath12k_peer *peer;
> + struct ath12k_sta *arsta;
> + struct ieee80211_sta *sta;
> + u16 rate;
> + u8 rate_idx = 0;
> + int ret;
> +
> + spin_lock_bh(&ab->base_lock);
Empty line after spin_lock_bh(), please.
> + peer = ath12k_peer_find_by_id(ab, ts->peer_id);
> + if (!peer || !peer->sta) {
> + ath12k_dbg(ab, ATH12K_DBG_DP_TX,
> + "failed to find the peer by id %u\n", ts->peer_id);
> + goto err_out;
> + }
> +
> + sta = peer->sta;
> + arsta = ath12k_sta_to_arsta(sta);
> +
> + memset(&arsta->txrate, 0, sizeof(arsta->txrate));
> +
> + /* This is to prefer choose the real NSS value arsta->last_txrate.nss,
> + * if it is invalid, then choose the NSS value while assoc.
> + */
> + if (arsta->last_txrate.nss)
> + arsta->txrate.nss = arsta->last_txrate.nss;
> + else
> + arsta->txrate.nss = arsta->peer_nss;
> +
> + if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11A ||
> + ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11B) {
> + ret = ath12k_mac_hw_ratecode_to_legacy_rate(ts->mcs,
> + ts->pkt_type,
> + &rate_idx,
> + &rate);
> + if (ret < 0)
> + goto err_out;
Should we print a warning here? Otherwise we might miss something.
> +
> + arsta->txrate.legacy = rate;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11N) {
> + if (ts->mcs > ATH12K_HT_MCS_MAX)
> + goto err_out;
Warning?
> +
> + if (arsta->txrate.nss != 0)
> + arsta->txrate.mcs = ts->mcs + 8 * (arsta->txrate.nss - 1);
Empty line here, please.
> + arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
And here.
> + if (ts->sgi)
> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AC) {
> + if (ts->mcs > ATH12K_VHT_MCS_MAX)
> + goto err_out;
Warning?
> + arsta->txrate.mcs = ts->mcs;
> + arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
Empty line.
> + if (ts->sgi)
> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
> + if (ts->mcs > ATH12K_HE_MCS_MAX)
> + goto err_out;
> +
> + arsta->txrate.mcs = ts->mcs;
> + arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
> + arsta->txrate.he_gi = ath12k_mac_he_gi_to_nl80211_he_gi(ts->sgi);
> + }
> +
> + arsta->txrate.bw = ath12k_mac_bw_to_mac80211_bw(ts->bw);
Empty line.
> + if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
> + arsta->txrate.bw = RATE_INFO_BW_HE_RU;
> + arsta->txrate.he_ru_alloc =
> + ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(ts->ru_tones);
> + }
> +
> +err_out:
> + spin_unlock_bh(&ab->base_lock);
> +}
> +
> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
> +{
> + if (ar->last_ppdu_id == 0)
> + goto update_last_ppdu_id;
> +
> + if (ar->last_ppdu_id == ts->ppdu_id ||
> + ar->cached_ppdu_id == ar->last_ppdu_id)
> + ar->cached_ppdu_id = ar->last_ppdu_id;
> +
> + ath12k_dp_tx_update_txcompl(ar, ts);
> +
> +update_last_ppdu_id:
> + ar->last_ppdu_id = ts->ppdu_id;
> +}
I think like this you could avoid the goto:
if (ar->last_ppdu_id != 0) {
if (ar->last_ppdu_id == ts->ppdu_id ||
ar->cached_ppdu_id == ar->last_ppdu_id)
ar->cached_ppdu_id = ar->last_ppdu_id;
ath12k_dp_tx_update_txcompl(ar, ts);
}
ar->last_ppdu_id = ts->ppdu_id;
> @@ -2383,6 +2387,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
> ath12k_peer_assoc_h_phymode(ar, vif, sta, arg);
> ath12k_peer_assoc_h_smps(sta, arg);
>
> + arsta->peer_nss = arg->peer_nss;
> /* TODO: amsdu_disable req? */
> }
Empty line before TODO comment, please.
> @@ -8324,3 +8329,90 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
>
> return ret;
> }
> +
> +enum nl80211_he_ru_alloc ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(u16 ru_phy)
> +{
> + enum nl80211_he_ru_alloc ret;
> +
> + switch (ru_phy) {
> + case RU_26:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + case RU_52:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
> + break;
> + case RU_106:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
> + break;
> + case RU_242:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
> + break;
> + case RU_484:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
> + break;
> + case RU_996:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
> +{
> + enum nl80211_he_ru_alloc ret;
> +
> + switch (ru_tones) {
> + case 26:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + case 52:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
> + break;
> + case 106:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
> + break;
> + case 242:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
> + break;
> + case 484:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
> + break;
> + case 996:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
> + break;
> + case (996 * 2):
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
> + break;
> + }
> +
> + return ret;
> +}
> +
> +enum nl80211_he_gi ath12k_mac_he_gi_to_nl80211_he_gi(u8 sgi)
> +{
> + enum nl80211_he_gi ret;
> +
> + switch (sgi) {
> + case RX_MSDU_START_SGI_0_8_US:
> + ret = NL80211_RATE_INFO_HE_GI_0_8;
> + break;
> + case RX_MSDU_START_SGI_1_6_US:
> + ret = NL80211_RATE_INFO_HE_GI_1_6;
> + break;
> + case RX_MSDU_START_SGI_3_2_US:
> + ret = NL80211_RATE_INFO_HE_GI_3_2;
> + break;
> + default:
> + ret = NL80211_RATE_INFO_HE_GI_0_8;
> + break;
> + }
> +
> + return ret;
> +}
Please don't add new function to the end of file, rather to the
beginning or the middle. But like I mentioned earlier, do we really need
these new functions?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2024/2/26 23:37, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> The tx bitrate of "iw dev xxx station dump" always show an invalid value
>> "tx bitrate: 6.0MBit/s".
>>
>> To address this issue, parse the tx complete report from firmware and
>> indicate the tx rate to mac80211.
>>
>> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
>> tx bitrate: 104.0 MBit/s MCS 13
>> tx bitrate: 144.4 MBit/s MCS 15 short GI
>> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
>> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>
>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
>> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>
>> Signed-off-by: Lingbo Kong <[email protected]>
>
> Please use full englist words like transmit instead of tx. Also the
> title could be simplified to:
>
> wifi: ath12k: report station mode transmit rate to user space
>
> Here I assumed this only works in station mode. Or does this also
> support AP and P2P mode? The commit message should explain that.
>
Ok, i will apply it in next version. Thanks for pointing out.
>> --- a/drivers/net/wireless/ath/ath12k/dp_mon.c
>> +++ b/drivers/net/wireless/ath/ath12k/dp_mon.c
>> @@ -290,7 +290,7 @@ static void ath12k_dp_mon_parse_he_sig_b1_mu(u8 *tlv_data,
>>
>> ru_tones = u32_get_bits(info0,
>> HAL_RX_HE_SIG_B1_MU_INFO_INFO0_RU_ALLOCATION);
>> - ppdu_info->ru_alloc = ath12k_he_ru_tones_to_nl80211_he_ru_alloc(ru_tones);
>> + ppdu_info->ru_alloc = ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(ru_tones);
>
> Now there would be two very similar functions:
>
> ath12k_mac_he_gi_to_nl80211_he_gi() vs ath12k_he_gi_to_nl80211_he_gi()
>
> ath12k_he_ru_tones_to_nl80211_he_ru_alloc() vs ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc()
>
> Why do we need those?
>
Yes, you're right, we don't need these functions. i will delete these
functions and directly modify ath12k_he_gi_to_nl80211_he_gi() and
ath12k_he_ru_tones_to_nl80211_he_ru_alloc() in next version.
Thanks for pointing out.
>> +static void ath12k_dp_tx_update_txcompl(struct ath12k *ar, struct hal_tx_status *ts)
>> +{
>> + struct ath12k_base *ab = ar->ab;
>> + struct ath12k_peer *peer;
>> + struct ath12k_sta *arsta;
>> + struct ieee80211_sta *sta;
>> + u16 rate;
>> + u8 rate_idx = 0;
>> + int ret;
>> +
>> + spin_lock_bh(&ab->base_lock);
>
> Empty line after spin_lock_bh(), please.
>
>
>> + peer = ath12k_peer_find_by_id(ab, ts->peer_id);
>> + if (!peer || !peer->sta) {
>> + ath12k_dbg(ab, ATH12K_DBG_DP_TX,
>> + "failed to find the peer by id %u\n", ts->peer_id);
>> + goto err_out;
>> + }
>> +
>> + sta = peer->sta;
>> + arsta = ath12k_sta_to_arsta(sta);
>> +
>> + memset(&arsta->txrate, 0, sizeof(arsta->txrate));
>> +
>> + /* This is to prefer choose the real NSS value arsta->last_txrate.nss,
>> + * if it is invalid, then choose the NSS value while assoc.
>> + */
>> + if (arsta->last_txrate.nss)
>> + arsta->txrate.nss = arsta->last_txrate.nss;
>> + else
>> + arsta->txrate.nss = arsta->peer_nss;
>> +
>> + if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11A ||
>> + ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11B) {
>> + ret = ath12k_mac_hw_ratecode_to_legacy_rate(ts->mcs,
>> + ts->pkt_type,
>> + &rate_idx,
>> + &rate);
>> + if (ret < 0)
>> + goto err_out;
>
> Should we print a warning here? Otherwise we might miss something.
>
>> +
>> + arsta->txrate.legacy = rate;
>> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11N) {
>> + if (ts->mcs > ATH12K_HT_MCS_MAX)
>> + goto err_out;
>
> Warning?
>
>> +
>> + if (arsta->txrate.nss != 0)
>> + arsta->txrate.mcs = ts->mcs + 8 * (arsta->txrate.nss - 1);
>
> Empty line here, please.
>
>> + arsta->txrate.flags = RATE_INFO_FLAGS_MCS;
>
> And here.
>
>> + if (ts->sgi)
>> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
>> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AC) {
>> + if (ts->mcs > ATH12K_VHT_MCS_MAX)
>> + goto err_out;
>
> Warning?
>
>> + arsta->txrate.mcs = ts->mcs;
>> + arsta->txrate.flags = RATE_INFO_FLAGS_VHT_MCS;
>
> Empty line.
>
>> + if (ts->sgi)
>> + arsta->txrate.flags |= RATE_INFO_FLAGS_SHORT_GI;
>> + } else if (ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
>> + if (ts->mcs > ATH12K_HE_MCS_MAX)
>> + goto err_out;
>> +
>> + arsta->txrate.mcs = ts->mcs;
>> + arsta->txrate.flags = RATE_INFO_FLAGS_HE_MCS;
>> + arsta->txrate.he_gi = ath12k_mac_he_gi_to_nl80211_he_gi(ts->sgi);
>> + }
>> +
>> + arsta->txrate.bw = ath12k_mac_bw_to_mac80211_bw(ts->bw);
>
> Empty line.
>
Ok, i will add warnings and empty line where appropriate.
Thanks for pointing out.
>> + if (ts->ofdma && ts->pkt_type == HAL_TX_RATE_STATS_PKT_TYPE_11AX) {
>> + arsta->txrate.bw = RATE_INFO_BW_HE_RU;
>> + arsta->txrate.he_ru_alloc =
>> + ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(ts->ru_tones);
>> + }
>> +
>> +err_out:
>> + spin_unlock_bh(&ab->base_lock);
>> +}
>> +
>> +static void ath12k_dp_tx_update(struct ath12k *ar, struct hal_tx_status *ts)
>> +{
>> + if (ar->last_ppdu_id == 0)
>> + goto update_last_ppdu_id;
>> +
>> + if (ar->last_ppdu_id == ts->ppdu_id ||
>> + ar->cached_ppdu_id == ar->last_ppdu_id)
>> + ar->cached_ppdu_id = ar->last_ppdu_id;
>> +
>> + ath12k_dp_tx_update_txcompl(ar, ts);
>> +
>> +update_last_ppdu_id:
>> + ar->last_ppdu_id = ts->ppdu_id;
>> +}
>
> I think like this you could avoid the goto:
>
> if (ar->last_ppdu_id != 0) {
> if (ar->last_ppdu_id == ts->ppdu_id ||
> ar->cached_ppdu_id == ar->last_ppdu_id)
> ar->cached_ppdu_id = ar->last_ppdu_id;
>
> ath12k_dp_tx_update_txcompl(ar, ts);
> }
>
> ar->last_ppdu_id = ts->ppdu_id;
>
you're right. i will apply it in next version.
>> @@ -2383,6 +2387,7 @@ static void ath12k_peer_assoc_prepare(struct ath12k *ar,
>> ath12k_peer_assoc_h_phymode(ar, vif, sta, arg);
>> ath12k_peer_assoc_h_smps(sta, arg);
>>
>> + arsta->peer_nss = arg->peer_nss;
>> /* TODO: amsdu_disable req? */
>> }
>
> Empty line before TODO comment, please.
>
>> @@ -8324,3 +8329,90 @@ int ath12k_mac_allocate(struct ath12k_base *ab)
>>
>> return ret;
>> }
>> +
>> +enum nl80211_he_ru_alloc ath12k_mac_phy_he_ru_to_nl80211_he_ru_alloc(u16 ru_phy)
>> +{
>> + enum nl80211_he_ru_alloc ret;
>> +
>> + switch (ru_phy) {
>> + case RU_26:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + case RU_52:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>> + break;
>> + case RU_106:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>> + break;
>> + case RU_242:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>> + break;
>> + case RU_484:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>> + break;
>> + case RU_996:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +enum nl80211_he_ru_alloc ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc(u16 ru_tones)
>> +{
>> + enum nl80211_he_ru_alloc ret;
>> +
>> + switch (ru_tones) {
>> + case 26:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + case 52:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_52;
>> + break;
>> + case 106:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_106;
>> + break;
>> + case 242:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_242;
>> + break;
>> + case 484:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_484;
>> + break;
>> + case 996:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_996;
>> + break;
>> + case (996 * 2):
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_2x996;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +enum nl80211_he_gi ath12k_mac_he_gi_to_nl80211_he_gi(u8 sgi)
>> +{
>> + enum nl80211_he_gi ret;
>> +
>> + switch (sgi) {
>> + case RX_MSDU_START_SGI_0_8_US:
>> + ret = NL80211_RATE_INFO_HE_GI_0_8;
>> + break;
>> + case RX_MSDU_START_SGI_1_6_US:
>> + ret = NL80211_RATE_INFO_HE_GI_1_6;
>> + break;
>> + case RX_MSDU_START_SGI_3_2_US:
>> + ret = NL80211_RATE_INFO_HE_GI_3_2;
>> + break;
>> + default:
>> + ret = NL80211_RATE_INFO_HE_GI_0_8;
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>
> Please don't add new function to the end of file, rather to the
> beginning or the middle. But like I mentioned earlier, do we really need
> these new functions?
>
Lingbo Kong <[email protected]> writes:
> On 2024/2/26 23:37, Kalle Valo wrote:
>> Lingbo Kong <[email protected]> writes:
>>
>>> The tx bitrate of "iw dev xxx station dump" always show an invalid value
>>> "tx bitrate: 6.0MBit/s".
>>>
>>> To address this issue, parse the tx complete report from firmware and
>>> indicate the tx rate to mac80211.
>>>
>>> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
>>> tx bitrate: 104.0 MBit/s MCS 13
>>> tx bitrate: 144.4 MBit/s MCS 15 short GI
>>> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
>>> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>
>>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
>>> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Lingbo Kong <[email protected]>
>> Please use full englist words like transmit instead of tx. Also the
>> title could be simplified to:
>> wifi: ath12k: report station mode transmit rate to user space
>> Here I assumed this only works in station mode. Or does this also
>> support AP and P2P mode? The commit message should explain that.
>>
>
> Ok, i will apply it in next version. Thanks for pointing out.
After rereading my comments maybe keep the title simple like:
wifi: ath12k: report station mode transmit rate
But it would be good to clarify in the commit message what modes this is
supported. And what hardware families support this.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2024/2/27 21:23, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> On 2024/2/26 23:37, Kalle Valo wrote:
>>> Lingbo Kong <[email protected]> writes:
>>>
>>>> The tx bitrate of "iw dev xxx station dump" always show an invalid value
>>>> "tx bitrate: 6.0MBit/s".
>>>>
>>>> To address this issue, parse the tx complete report from firmware and
>>>> indicate the tx rate to mac80211.
>>>>
>>>> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
>>>> tx bitrate: 104.0 MBit/s MCS 13
>>>> tx bitrate: 144.4 MBit/s MCS 15 short GI
>>>> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>>
>>>> Tested-on: WCN7850 hw2.0 PCI WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>>> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
>>>> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>>>
>>>> Signed-off-by: Lingbo Kong <[email protected]>
>>> Please use full englist words like transmit instead of tx. Also the
>>> title could be simplified to:
>>> wifi: ath12k: report station mode transmit rate to user space
>>> Here I assumed this only works in station mode. Or does this also
>>> support AP and P2P mode? The commit message should explain that.
>>>
>>
>> Ok, i will apply it in next version. Thanks for pointing out.
>
> After rereading my comments maybe keep the title simple like:
>
> wifi: ath12k: report station mode transmit rate
>
> But it would be good to clarify in the commit message what modes this is
> supported. And what hardware families support this.
Hi kalle, Could you please offer your opinion on this commit message?
wifi: ath12k: report station mode transmit rate
Currently, the transmit rate of "iw dev xxx station dump" command always
show an invalid value.
To address this issue, ath12k parse the info of transmit complete report
from firmware and indicate the transmit rate to mac80211.
This patch only applies to the WCN7850's station mode.
After that, "iw dev xxx station dump" show the correct transmit rate.
Such as:
tx bitrate: 104.0 MBit/s MCS 13
tx bitrate: 144.4 MBit/s MCS 15 short GI tx bitrate: 626.9 MBit/s
80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0 tx bitrate: 1921.5 MBit/s
160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
Lingbo Kong <[email protected]> writes:
> On 2024/2/27 21:23, Kalle Valo wrote:
>> Lingbo Kong <[email protected]> writes:
>>
>>> On 2024/2/26 23:37, Kalle Valo wrote:
>>>> Lingbo Kong <[email protected]> writes:
>>>>
>>>>> The tx bitrate of "iw dev xxx station dump" always show an invalid value
>>>>> "tx bitrate: 6.0MBit/s".
>>>>>
>>>>> To address this issue, parse the tx complete report from firmware and
>>>>> indicate the tx rate to mac80211.
>>>>>
>>>>> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
>>>>> tx bitrate: 104.0 MBit/s MCS 13
>>>>> tx bitrate: 144.4 MBit/s MCS 15 short GI
>>>>> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>>> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>>>
>>>>> Tested-on: WCN7850 hw2.0 PCI
>>>>> WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>>>> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
>>>>> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>>>>
>>>>> Signed-off-by: Lingbo Kong <[email protected]>
>>>> Please use full englist words like transmit instead of tx. Also the
>>>> title could be simplified to:
>>>> wifi: ath12k: report station mode transmit rate to user space
>>>> Here I assumed this only works in station mode. Or does this also
>>>> support AP and P2P mode? The commit message should explain that.
>>>>
>>>
>>> Ok, i will apply it in next version. Thanks for pointing out.
>> After rereading my comments maybe keep the title simple like:
>> wifi: ath12k: report station mode transmit rate
>> But it would be good to clarify in the commit message what modes
>> this is
>> supported. And what hardware families support this.
>
> Hi kalle, Could you please offer your opinion on this commit message?
>
> wifi: ath12k: report station mode transmit rate
>
> Currently, the transmit rate of "iw dev xxx station dump" command
> always show an invalid value.
>
> To address this issue, ath12k parse the info of transmit complete
> report from firmware and indicate the transmit rate to mac80211.
>
> This patch only applies to the WCN7850's station mode.
>
> After that, "iw dev xxx station dump" show the correct transmit rate.
> Such as:
> tx bitrate: 104.0 MBit/s MCS 13
> tx bitrate: 144.4 MBit/s MCS 15 short GI tx bitrate: 626.9 MBit/s
> 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0 tx bitrate: 1921.5 MBit/s
> 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
Looks good, except for readability I would add an empty line after "Such
as:".
I noticed that the signal patch depends on this patchset:
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
In that you should submit both patchses in same patchset. But please
wait until I have reviewed the signal strength patch.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2024/2/28 19:08, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>> On 2024/2/27 21:23, Kalle Valo wrote:
>>> Lingbo Kong <[email protected]> writes:
>>>
>>>> On 2024/2/26 23:37, Kalle Valo wrote:
>>>>> Lingbo Kong <[email protected]> writes:
>>>>>
>>>>>> The tx bitrate of "iw dev xxx station dump" always show an invalid value
>>>>>> "tx bitrate: 6.0MBit/s".
>>>>>>
>>>>>> To address this issue, parse the tx complete report from firmware and
>>>>>> indicate the tx rate to mac80211.
>>>>>>
>>>>>> After that, "iw dev xxx station dump" show the correct tx bitrate such as:
>>>>>> tx bitrate: 104.0 MBit/s MCS 13
>>>>>> tx bitrate: 144.4 MBit/s MCS 15 short GI
>>>>>> tx bitrate: 626.9 MBit/s 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>>>> tx bitrate: 1921.5 MBit/s 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>>>>>
>>>>>> Tested-on: WCN7850 hw2.0 PCI
>>>>>> WLAN.HMT.1.0.c5-00481-QCAHMTSWPL_V1.0_V2.0_SILICONZ-3
>>>>>> Tested-on: QCN9274 hw2.0 PCI QCN9274 hw2.0 PCI
>>>>>> WLAN.WBE.1.0.1-00029-QCAHKSWPL_SILICONZ-1
>>>>>>
>>>>>> Signed-off-by: Lingbo Kong <[email protected]>
>>>>> Please use full englist words like transmit instead of tx. Also the
>>>>> title could be simplified to:
>>>>> wifi: ath12k: report station mode transmit rate to user space
>>>>> Here I assumed this only works in station mode. Or does this also
>>>>> support AP and P2P mode? The commit message should explain that.
>>>>>
>>>>
>>>> Ok, i will apply it in next version. Thanks for pointing out.
>>> After rereading my comments maybe keep the title simple like:
>>> wifi: ath12k: report station mode transmit rate
>>> But it would be good to clarify in the commit message what modes
>>> this is
>>> supported. And what hardware families support this.
>>
>> Hi kalle, Could you please offer your opinion on this commit message?
>>
>> wifi: ath12k: report station mode transmit rate
>>
>> Currently, the transmit rate of "iw dev xxx station dump" command
>> always show an invalid value.
>>
>> To address this issue, ath12k parse the info of transmit complete
>> report from firmware and indicate the transmit rate to mac80211.
>>
>> This patch only applies to the WCN7850's station mode.
>>
>> After that, "iw dev xxx station dump" show the correct transmit rate.
>> Such as:
>> tx bitrate: 104.0 MBit/s MCS 13
>> tx bitrate: 144.4 MBit/s MCS 15 short GI tx bitrate: 626.9 MBit/s
>> 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0 tx bitrate: 1921.5 MBit/s
>> 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>
> Looks good, except for readability I would add an empty line after "Such
> as:".
>
> I noticed that the signal patch depends on this patchset:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>
> In that you should submit both patchses in same patchset. But please
> wait until I have reviewed the signal strength patch.
Hi, kalle,
Would you mind if I kindly inquire whether you might have forgotten to
review "wifi: ath12k: report signal for iw dev xxx station dump" patch?
After you review, I can put the two patches into the same patchset:)
Best regards
Lingbo Kong
Lingbo Kong <[email protected]> writes:
> On 2024/2/28 19:08, Kalle Valo wrote:
>> Lingbo Kong <[email protected]> writes:
>>
>>> On 2024/2/27 21:23, Kalle Valo wrote:
>>>> Lingbo Kong <[email protected]> writes:
>>>>
>>>>> On 2024/2/26 23:37, Kalle Valo wrote:
>>>>>> Please use full englist words like transmit instead of tx. Also the
>>>>>> title could be simplified to:
>>>>>> wifi: ath12k: report station mode transmit rate to user space
>>>>>> Here I assumed this only works in station mode. Or does this also
>>>>>> support AP and P2P mode? The commit message should explain that.
>>>>>>
>>>>>
>>>>> Ok, i will apply it in next version. Thanks for pointing out.
>>>> After rereading my comments maybe keep the title simple like:
>>>> wifi: ath12k: report station mode transmit rate
>>>> But it would be good to clarify in the commit message what modes
>>>> this is
>>>> supported. And what hardware families support this.
>>>
>>> Hi kalle, Could you please offer your opinion on this commit message?
>>>
>>> wifi: ath12k: report station mode transmit rate
>>>
>>> Currently, the transmit rate of "iw dev xxx station dump" command
>>> always show an invalid value.
>>>
>>> To address this issue, ath12k parse the info of transmit complete
>>> report from firmware and indicate the transmit rate to mac80211.
>>>
>>> This patch only applies to the WCN7850's station mode.
>>>
>>> After that, "iw dev xxx station dump" show the correct transmit rate.
>>> Such as:
>>> tx bitrate: 104.0 MBit/s MCS 13
>>> tx bitrate: 144.4 MBit/s MCS 15 short GI tx bitrate: 626.9 MBit/s
>>> 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0 tx bitrate: 1921.5 MBit/s
>>> 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>> Looks good, except for readability I would add an empty line after
>> "Such
>> as:".
>> I noticed that the signal patch depends on this patchset:
>> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>> In that you should submit both patchses in same patchset. But please
>> wait until I have reviewed the signal strength patch.
>
> Hi, kalle,
>
> Would you mind if I kindly inquire whether you might have forgotten to
> review "wifi: ath12k: report signal for iw dev xxx station dump"
> patch?
I don't rely on my memory and instead use patchwork for that:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#checking_state_of_patches_from_patchwork
And the patch you are referring is in state 'New':
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
So it is in the queue, please just wait patiently.
And as general comment to all Qualcomm engineers (not just Lingbo): if
you want patches reviewed and applied faster then help Jeff and me!
Review other patches, test our drivers and stack, investigate bug
reports, make sure all firmware releases are uploaded etc. There is so
much to do. Don't just throw patches over the wall and disappear.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2024/3/14 18:41, Kalle Valo wrote:
> Lingbo Kong <[email protected]> writes:
>
>>>>>>
>>>>>> Ok, i will apply it in next version. Thanks for pointing out.
>>>>> After rereading my comments maybe keep the title simple like:
>>>>> wifi: ath12k: report station mode transmit rate
>>>>> But it would be good to clarify in the commit message what modes
>>>>> this is
>>>>> supported. And what hardware families support this.
>>>>
>>>> Hi kalle, Could you please offer your opinion on this commit message?
>>>>
>>>> wifi: ath12k: report station mode transmit rate
>>>>
>>>> Currently, the transmit rate of "iw dev xxx station dump" command
>>>> always show an invalid value.
>>>>
>>>> To address this issue, ath12k parse the info of transmit complete
>>>> report from firmware and indicate the transmit rate to mac80211.
>>>>
>>>> This patch only applies to the WCN7850's station mode.
>>>>
>>>> After that, "iw dev xxx station dump" show the correct transmit rate.
>>>> Such as:
>>>> tx bitrate: 104.0 MBit/s MCS 13
>>>> tx bitrate: 144.4 MBit/s MCS 15 short GI tx bitrate: 626.9 MBit/s
>>>> 80MHz HE-MCS 6 HE-NSS 2 HE-GI 0 HE-DCM 0 tx bitrate: 1921.5 MBit/s
>>>> 160MHz HE-MCS 9 HE-NSS 2 HE-GI 0 HE-DCM 0
>>> Looks good, except for readability I would add an empty line after
>>> "Such
>>> as:".
>>> I noticed that the signal patch depends on this patchset:
>>> https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
>>> In that you should submit both patchses in same patchset. But please
>>> wait until I have reviewed the signal strength patch.
Hi Kalle,
Please ignore the patches submitted separately below.
[PATCH v4] wifi: ath12k: report tx bitrate for iw dev xxx station dump
[PATCH] wifi: ath12k: report signal for iw dev xxx station dump
[PATCH] wifi: ath12k: add display tx and rx bitrate for 11be
I will resubmit these patches in same patchset:)
Best regards,
Lingbo Kong