On 2024/2/26 23:37, Kalle Valo wrote:
Lingbo Kong <quic_lingbok@xxxxxxxxxxx> 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 <quic_lingbok@xxxxxxxxxxx>
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?
