[PATCH v4] wifi: ath12k: report tx bitrate for iw dev xxx station dump
Lingbo Kong
quic_lingbok at quicinc.com
Tue Feb 27 05:07:36 PST 2024
On 2024/2/26 23:37, Kalle Valo wrote:
> Lingbo Kong <quic_lingbok at quicinc.com> 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 at quicinc.com>
>
> 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?
>
More information about the ath12k
mailing list