On 2024/4/26 0:54, Kalle Valo wrote:
Lingbo Kong <quic_lingbok@xxxxxxxxxxx> writes:
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 affects the station mode of WCN7850 and QCN9274.
After that, "iw dev xxx station dump" show the correct transmit rate.
Such as:
Station 00:03:7f:12:03:03 (on wlo1)
inactive time: 872 ms
rx bytes: 219111
rx packets: 1133
tx bytes: 53767
tx packets: 462
tx retries: 51
tx failed: 0
beacon loss: 0
beacon rx: 403
rx drop misc: 74
signal: -95 dBm
beacon signal avg: -18 dBm
tx bitrate: 1441.1 MBit/s 80MHz EHT-MCS 13 EHT-NSS 2 EHT-GI 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 WLAN.WBE.1.2.1-00201-QCAHKSWPL_SILICONZ-1
Signed-off-by: Lingbo Kong <quic_lingbok@xxxxxxxxxxx>
I'm still going throught the patchset, please don't send a new version
yet. Few quick comments:
+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);
Did you analyse how this function, and especially taking the base_lock,
affects performance?
The base_lock is used here because of the need to look for peers based
on the ts->peer_id when calling ath12k_peer_find_by_id() function, which
i think might affect performance.
Do i need to run a throughput test?
+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;
+}
How does this function compare to ath12k_he_ru_tones_to_nl80211_he_ru_alloc()?
ath12k_mac_he_ru_tones_to_nl80211_he_ru_alloc() is different from
ath12k_he_ru_tones_to_nl80211_he_ru_alloc().
the logic of ath12k_he_ru_tones_to_nl80211_he_ru_alloc() is
static inline
enum nl80211_he_ru_alloc ath12k_he_ru_tones_to_nl80211_he_ru_alloc(u16
ru_tones)
{
enum nl80211_he_ru_alloc ret;
switch (ru_tones) {
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;
case RU_26:
fallthrough;
default:
ret = NL80211_RATE_INFO_HE_RU_ALLOC_26;
break;
}
return ret;
}
#define RU_26 1
#define RU_52 2
#define RU_106 4
#define RU_242 9
#define RU_484 18
#define RU_996 37
+enum nl80211_eht_gi ath12k_mac_eht_gi_to_nl80211_eht_gi(u8 sgi)
+{
+ enum nl80211_eht_gi ret;
+
+ switch (sgi) {
+ case RX_MSDU_START_SGI_0_8_US:
+ ret = NL80211_RATE_INFO_EHT_GI_0_8;
+ break;
+ case RX_MSDU_START_SGI_1_6_US:
+ ret = NL80211_RATE_INFO_EHT_GI_1_6;
+ break;
+ case RX_MSDU_START_SGI_3_2_US:
+ ret = NL80211_RATE_INFO_EHT_GI_3_2;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_EHT_GI_0_8;
+ break;
+ }
+
+ return ret;
+}
BTW the ret variable is unnessary, this could be simplified to:
switch (foo) {
case FOO1:
return BAR1;
case FOO2:
return BAR2;
default:
return BAR3;
}
+enum nl80211_eht_ru_alloc ath12k_mac_eht_ru_tones_to_nl80211_eht_ru_alloc(u16 ru_tones)
+{
+ enum nl80211_eht_ru_alloc ret;
+
+ switch (ru_tones) {
+ case 26:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
+ break;
+ case 52:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52;
+ break;
+ case (52 + 26):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_52P26;
+ break;
+ case 106:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106;
+ break;
+ case (106 + 26):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_106P26;
+ break;
+ case 242:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_242;
+ break;
+ case 484:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484;
+ break;
+ case (484 + 242):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_484P242;
+ break;
+ case 996:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996;
+ break;
+ case (996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484;
+ break;
+ case (996 + 484 + 242):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_996P484P242;
+ break;
+ case (2 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996;
+ break;
+ case (2 * 996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_2x996P484;
+ break;
+ case (3 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996;
+ break;
+ case (3 * 996 + 484):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_3x996P484;
+ break;
+ case (4 * 996):
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_4x996;
+ break;
+ default:
+ ret = NL80211_RATE_INFO_EHT_RU_ALLOC_26;
+ }
+
+ return ret;
+}
Same here.
