Received-SPF: pass (google.com: domain of linux-wireless+bounces-4087-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <6f39eb1c-97f7-4029-a5f2-5ab36ff0f0a6@quicinc.com>
Date: Tue, 27 Feb 2024 21:07:36 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v4] wifi: ath12k: report tx bitrate for iw dev xxx station
 dump
Content-Language: en-US
To: Kalle Valo <kvalo@kernel.org>
CC: <ath12k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
References: <20240219095802.1147-1-quic_lingbok@quicinc.com>
 <87y1b7jkxu.fsf@kernel.org>
From: Lingbo Kong <quic_lingbok@quicinc.com>
In-Reply-To: <87y1b7jkxu.fsf@kernel.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit


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