On 14/04/2020, Yu Wang <yyuwang@xxxxxxxxxxxxxxxx> wrote:
>
>
>> -----Original Message-----
>> From: ath10k <ath10k-bounces@xxxxxxxxxxxxxxxxxxx> On Behalf Of Kalle Valo
>> Sent: Thursday, April 9, 2020 10:22 PM
>> To: Yu Wang <yyuwang@xxxxxxxxxxxxxx>
>> Cc: linux-wireless@xxxxxxxxxxxxxxx; ath10k@xxxxxxxxxxxxxxxxxxx
>> Subject: [EXT] Re: [PATCH v2 2/2] ath10k: correct legacy rate in tx stats
>>
>> Yu Wang <yyuwang@xxxxxxxxxxxxxx> wrote:
>>
>> > When working in station mode, after connected to a legacy AP, 11g
>> > only, for example, the tx bitrate is incorrect in output of command
>> > 'iw wlan0 link'.
>> >
>> > That's because the legacy tx bitrate value reported by firmware is not
>> > well handled:
>> > For QCA6174, the value represents rate index, but treated as a real
>> > rate; For QCA9888, the value is real rate, with unit 'Mbps', but
>> > treated as '100kbps'.
>> >
>> > To fix this issue:
>> > 1. Translate the rate index to real rate for QCA6174; 2. Translate the
>> > rate from 'Mbps' to 'kbps' for QCA9888.
>> >
>> > Tested with:
>> > QCA6174 PCIe with firmware WLAN.RM.4.4.1.c3-00031.
>> > QCA6174 SDIO with firmware WLAN.RMH.4.4.1-00029.
>> > QCA9888 PCIe with firmware 10.4-3.9.0.2-00040.
>> >
>> > Signed-off-by: Yu Wang <yyuwang@xxxxxxxxxxxxxx>
>> > Signed-off-by: Kalle Valo <kvalo@xxxxxxxxxxxxxx>
>>
>> My comments don't seem to go to patchwork, so trying again:
>>
>> What about QCA988X and WCN3990, how do they behave? Does this patch
>> break those?
> Since HTT_T2H_MSG_TYPE_PPDU_STATS_IND is a newly added message, suppose it's
> won't break the other functions.
> I don’t have environment to verify the patch with QCA988X and WCN3990.
> Can anyone help on this?
>
>>
>> > +cck = (preamble == WMI_RATE_PREAMBLE_CCK);
>> > +hw_rate = ATH10K_HW_LEGACY_RATE(*ratecode);
>> > +for (i = 0; i < sband->n_bitrates; i++) {
>> > +bitrates = &sband->bitrates[i];
>> > +if (ath10k_mac_bitrate_is_cck(bitrates->bitrate) != cck)
>> > +continue;
>> > +
>> > +if (bitrates->hw_value == hw_rate ||
>> > + (bitrates->flags & IEEE80211_RATE_SHORT_PREAMBLE &&
>> > + bitrates->hw_value_short == hw_rate)) {
>> > +bitrate = bitrates->bitrate;
>> > +
>> > +/* The bitrate will be recovered in
>> > + * ath10k_update_per_peer_tx_stats().
>> > + */
>> > +if (bitrate == 55)
>> > +bitrate = 60;
>> > +
>> > +bitrate = bitrate / 10;
>>
>> Here you use magic value 60 but in ath10k_update_per_peer_tx_stats() you
>> use
>> magic value 50:
>>
>> > +/* from 1Mbps to 100Kbps */
>> > +rate = rate * 10;
>> > +if (rate == 50)
>> > +rate = 55;
>>
>> Am I missing something or how is this supposed to work?
> In existing code, ath10k_update_per_peer_tx_stats() will check the bitrate
> and convert 6_CCK to 5(in the comment: FW sends CCK rate 5.5Mbps as 6), and
> then 5 will be recovered to 55.
> That's why we need to convert bitrate 55 to 6 when processing PPDU_STATS.
>
> if (txrate.flags == WMI_RATE_PREAMBLE_CCK ||
> txrate.flags == WMI_RATE_PREAMBLE_OFDM) {
> rate = ATH10K_HW_LEGACY_RATE(peer_stats->ratecode);
> /* This is hacky, FW sends CCK rate 5.5Mbps as 6 */
> if (rate == 6 && txrate.flags == WMI_RATE_PREAMBLE_CCK)
> rate = 5;
> rate_idx = ath10k_get_legacy_rate_idx(ar, rate);
> if (rate_idx < 0)
> return;
>
> /* from 1Mbps to 100Kbps */
> rate = rate * 10;
> if (rate == 50)
> rate = 55;
>>
>> --
>> https://patchwork.kernel.org/patch/11251001/
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatch
>> es
>>
>> _______________________________________________
>> ath10k mailing list
>> ath10k@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/ath10k
> _______________________________________________
> ath10k mailing list
> ath10k@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/ath10k
>
Hi
I don't understand why are you fixing this in driver when the comment
clearly says "FW sends CCK rate 5.5Mbps as 6"
Shouldn't a proper way be to fix this in firmware?
