Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.400.51.1.1\))
Subject: Re: [PATCH] ath9k: fix per-packet TX-power cap for TPC
From:   Jonas Jelonek <jelonek.jonas@gmail.com>
In-Reply-To: <87tty2mn83.fsf@toke.dk>
Date:   Thu, 30 Mar 2023 15:21:20 +0200
Cc:     linux-wireless@vger.kernel.org,
        =?utf-8?Q?Thomas_H=C3=BChn?= <thomas.huehn@hs-nordhausen.de>,
        Felix Fietkau <nbd@nbd.name>, kvalo@kernel.org,
        johannes.berg@intel.com, lorenzo@kernel.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <2C4031B3-5DBB-48A9-8799-BE58FC0148A3@gmail.com>
References: <20230330071854.757154-1-jelonek.jonas@gmail.com>
 <87wn2ymvph.fsf@toke.dk> <86B6FAF2-49B4-4993-A22D-FE102756AFD1@gmail.com>
 <87tty2mn83.fsf@toke.dk>
To:     =?utf-8?Q?Toke_H=C3=B8iland-J=C3=B8rgensen?= <toke@kernel.org>
Precedence: bulk


> On 30. Mar 2023, at 14:34, Toke H=C3=B8iland-J=C3=B8rgensen =
<toke@kernel.org> wrote:
>=20
> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>=20
>>> On 30. Mar 2023, at 11:31, Toke H=C3=B8iland-J=C3=B8rgensen =
<toke@kernel.org> wrote:
>>>=20
>>> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>>>=20
>>>> Fix incorrect usage of plain rate_idx as index into the max (power) =
per
>>>> rate lookup table.
>>>>=20
>>>> For transmit power control (TPC), the ath9k driver maintains =
internal
>>>> tables (in struct ath_hw) to store the max allowed power level per =
rate.
>>>> They are used to limit a given TX-power according to regulatory and =
user
>>>> limits in the TX-path per packet. The tables are filled in a =
predefined
>>>> order, starting with values for CCK + OFDM rates and followed by =
the
>>>> values for MCS rates. Thus, the maximum power levels for MCS do not
>>>> start at index 0 in the table but are shifted by a fixed value.
>>>>=20
>>>> The TX-power limiting in ath_get_rate_txpower did not apply this =
shift,
>>>> thus retrieved the incorrect maximum power level. For example, the
>>>> maximum power for OFDM rate 0 was used for MCS rate 0. If STBC was =
used,
>>>> the power was mostly limited to 0 because the STBC table is zeroed =
for
>>>> legacy CCK/OFDM rates. This patch fixes this table lookup.
>>>>=20
>>>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>>>=20
>>> So what effect does this bug have in practice? Also, how did you =
test
>>> the patch? :)
>>>=20
>>=20
>> It actually may not have an effect in most of current practical =
applications. The
>> member =E2=80=98tpc_enabled' in struct ath_hw is by default set to =
false and thus, TPC
>> in ath9k is usually disabled. The code path will be skipped in that =
case.=20
>> But it has an effect as I am still working on TPC per packet in ath9k =
as part of
>> research. Looking at my traces I saw that the TX-power is capped at 0 =
for
>> MCS rates 0-7 in case the STBC flag is set in struct =
ieee80211_tx_info. Thus,
>> transmission was significantly worse as usual, and I also was not =
able to=20
>> manually set a proper TX-power via my testing API. With other rates =
it is working
>> fine. I also double-checked that it isn=E2=80=99t a problem of how =
TX-power is set and
>> reported with our upcoming API.
>>=20
>> I tested this with my OpenWrt-based AP-STA desk setup using two =
different
>> ath9k wifi chips (AR9280, AR9580), Kernel 5.15.98. I guess that =
should be
>> sufficient as ath9k hasn=E2=80=99t changed that significantly since =
that and several
>> changes are already backported. I compile-tested it with latest 6.3 =
kernel
>> (+allyesconfig), the patch applied flawlessly and encountered no =
problems.=20
>> After applying my patch, I could see that everything was working as =
expected.
>> Transmission performs equally as if TPC was disabled, and setting + =
reporting
>> TX-power was working again. Also verified this with some verbose =
debugging
>> in ath_get_rate_txpower to see which index + max power is used.
>=20
> Alright, cool! Could you please add all this to the commit message and
> resubmit?
>=20
> -Toke

Sure, please have a look at it again if I missed something :)

Jonas