Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
In-Reply-To: <MWHPR02MB2496EE78C46698CE3E6101EB98DA0@MWHPR02MB2496.namprd02.prod.outlook.com>
References: <0101016e82883ded-63f88383-cd90-4cb0-b9bb-3dd6a1e9f4e9-000000@us-west-2.amazonses.com>
 <20200409142136.5490EC433BA@smtp.codeaurora.org> <MWHPR02MB2496EE78C46698CE3E6101EB98DA0@MWHPR02MB2496.namprd02.prod.outlook.com>
From:   Tom Psyborg <pozega.tomislav@gmail.com>
Date:   Tue, 14 Apr 2020 10:53:02 +0200
Message-ID: <CAKR_QVJX719-F7pF2FdjY0Q_uMbvN0-Uo0ac6u7FAWyfci_TAw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] ath10k: correct legacy rate in tx stats
To:     Yu Wang <yyuwang@qti.qualcomm.com>
Cc:     "kvalo@codeaurora.org" <kvalo@codeaurora.org>,
        Yu Wang <yyuwang@codeaurora.org>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>,
        "ath10k@lists.infradead.org" <ath10k@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On 14/04/2020, Yu Wang <yyuwang@qti.qualcomm.com> wrote:
>
>
>> -----Original Message-----
>> From: ath10k <ath10k-bounces@lists.infradead.org> On Behalf Of Kalle Val=
o
>> Sent: Thursday, April 9, 2020 10:22 PM
>> To: Yu Wang <yyuwang@codeaurora.org>
>> Cc: linux-wireless@vger.kernel.org; ath10k@lists.infradead.org
>> Subject: [EXT] Re: [PATCH v2 2/2] ath10k: correct legacy rate in tx stat=
s
>>
>> Yu Wang <yyuwang@codeaurora.org> 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@codeaurora.org>
>> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>>
>> 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 i=
t's
> won't break the other functions.
> I don=E2=80=99t have environment to verify the patch with QCA988X and WCN=
3990.
> Can anyone help on this?
>
>>
>> > +cck =3D (preamble =3D=3D WMI_RATE_PREAMBLE_CCK);
>> > +hw_rate =3D ATH10K_HW_LEGACY_RATE(*ratecode);
>> > +for (i =3D 0; i < sband->n_bitrates; i++) {
>> > +bitrates =3D &sband->bitrates[i];
>> > +if (ath10k_mac_bitrate_is_cck(bitrates->bitrate) !=3D cck)
>> > +continue;
>> > +
>> > +if (bitrates->hw_value =3D=3D hw_rate ||
>> > +    (bitrates->flags & IEEE80211_RATE_SHORT_PREAMBLE &&
>> > +     bitrates->hw_value_short =3D=3D hw_rate)) {
>> > +bitrate =3D bitrates->bitrate;
>> > +
>> > +/* The bitrate will be recovered in
>> > + * ath10k_update_per_peer_tx_stats().
>> > + */
>> > +if (bitrate =3D=3D 55)
>> > +bitrate =3D 60;
>> > +
>> > +bitrate =3D 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 =3D rate * 10;
>> > +if (rate =3D=3D 50)
>> > +rate =3D 55;
>>
>> Am I missing something or how is this supposed to work?
> In existing code, ath10k_update_per_peer_tx_stats() will check the bitrat=
e
> and convert 6_CCK to 5(in the comment: FW sends CCK rate 5.5Mbps as 6), a=
nd
> 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 =3D=3D WMI_RATE_PREAMBLE_CCK ||
> txrate.flags =3D=3D WMI_RATE_PREAMBLE_OFDM) {
> rate =3D ATH10K_HW_LEGACY_RATE(peer_stats->ratecode);
> /* This is hacky, FW sends CCK rate 5.5Mbps as 6 */
> if (rate =3D=3D 6 && txrate.flags =3D=3D WMI_RATE_PREAMBLE_CCK)
> rate =3D 5;
> rate_idx =3D ath10k_get_legacy_rate_idx(ar, rate);
> if (rate_idx < 0)
> return;
>
> /* from 1Mbps to 100Kbps */
> rate =3D rate * 10;
> if (rate =3D=3D 50)
> rate =3D 55;
>>
>> --
>> https://patchwork.kernel.org/patch/11251001/
>>
>> https://wireless.wiki.kernel.org/en/developers/documentation/submittingp=
atch
>> es
>>
>> _______________________________________________
>> ath10k mailing list
>> ath10k@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/ath10k
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> 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?