[PATCH v4 1/3] wifi: ath12k: report station mode transmit rate
Lingbo Kong
quic_lingbok at quicinc.com
Thu Apr 25 23:41:33 PDT 2024
On 2024/4/26 0:54, Kalle Valo wrote:
> Lingbo Kong <quic_lingbok at quicinc.com> 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 at quicinc.com>
>
> 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.
>
More information about the ath12k
mailing list