Re: [PATCH v4 1/3] wifi: ath12k: report station mode transmit rate

[Kalle Valo <kvalo@kernel.org>]

Lingbo Kong <quic_lingbok@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@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?

> +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()?

> +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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches