[PATCH] ath9k: fix per-packet TX-power cap for TPC

[PATCH] ath9k: fix per-packet TX-power cap for TPC

[Jonas Jelonek]

Fix incorrect usage of plain rate_idx as index into the max (power) per
rate lookup table.

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.

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.

Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 30 +++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index ef9a8e0b75e6..f6f2ab7a63ff 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -34,6 +34,12 @@
 #define NUM_SYMBOLS_PER_USEC(_usec) (_usec >> 2)
 #define NUM_SYMBOLS_PER_USEC_HALFGI(_usec) (((_usec*5)-4)/18)
 
+/* Shifts in ar5008_phy.c and ar9003_phy.c are equal for all revisions */
+#define ATH9K_PWRTBL_11NA_OFDM_SHIFT    0
+#define ATH9K_PWRTBL_11NG_OFDM_SHIFT    4
+#define ATH9K_PWRTBL_11NA_HT_SHIFT      8
+#define ATH9K_PWRTBL_11NG_HT_SHIFT      12
+
 
 static u16 bits_per_symbol[][2] = {
 	/* 20MHz 40MHz */
@@ -1169,13 +1175,14 @@ void ath_update_max_aggr_framelen(struct ath_softc *sc, int queue, int txop)
 }
 
 static u8 ath_get_rate_txpower(struct ath_softc *sc, struct ath_buf *bf,
-			       u8 rateidx, bool is_40, bool is_cck)
+			       u8 rateidx, bool is_40, bool is_cck, bool is_mcs)
 {
 	u8 max_power;
 	struct sk_buff *skb;
 	struct ath_frame_info *fi;
 	struct ieee80211_tx_info *info;
 	struct ath_hw *ah = sc->sc_ah;
+	bool is_2ghz, is_5ghz, use_stbc;
 
 	if (sc->tx99_state || !ah->tpc_enabled)
 		return MAX_RATE_POWER;
@@ -1184,6 +1191,19 @@ static u8 ath_get_rate_txpower(struct ath_softc *sc, struct ath_buf *bf,
 	fi = get_frame_info(skb);
 	info = IEEE80211_SKB_CB(skb);
 
+	is_2ghz = info->band == NL80211_BAND_2GHZ;
+	is_5ghz = info->band == NL80211_BAND_5GHZ;
+	use_stbc = is_mcs && rateidx < 8 && (info->flags &
+					     IEEE80211_TX_CTL_STBC);
+
+	if (is_mcs)
+		rateidx += is_5ghz ? ATH9K_PWRTBL_11NA_HT_SHIFT
+				   : ATH9K_PWRTBL_11NG_HT_SHIFT;
+	else if (is_2ghz && !is_cck)
+		rateidx += ATH9K_PWRTBL_11NG_OFDM_SHIFT;
+	else
+		rateidx += ATH9K_PWRTBL_11NA_OFDM_SHIFT;
+
 	if (!AR_SREV_9300_20_OR_LATER(ah)) {
 		int txpower = fi->tx_power;
 
@@ -1193,10 +1213,8 @@ static u8 ath_get_rate_txpower(struct ath_softc *sc, struct ath_buf *bf,
 			u16 eeprom_rev = ah->eep_ops->get_eeprom_rev(ah);
 
 			if (eeprom_rev >= AR5416_EEP_MINOR_VER_2) {
-				bool is_2ghz;
 				struct modal_eep_header *pmodal;
 
-				is_2ghz = info->band == NL80211_BAND_2GHZ;
 				pmodal = &eep->modalHeader[is_2ghz];
 				power_ht40delta = pmodal->ht40PowerIncForPdadc;
 			} else {
@@ -1229,7 +1247,7 @@ static u8 ath_get_rate_txpower(struct ath_softc *sc, struct ath_buf *bf,
 		if (!max_power && !AR_SREV_9280_20_OR_LATER(ah))
 			max_power = 1;
 	} else if (!bf->bf_state.bfs_paprd) {
-		if (rateidx < 8 && (info->flags & IEEE80211_TX_CTL_STBC))
+		if (use_stbc)
 			max_power = min_t(u8, ah->tx_power_stbc[rateidx],
 					  fi->tx_power);
 		else
@@ -1319,7 +1337,7 @@ static void ath_buf_set_rate(struct ath_softc *sc, struct ath_buf *bf,
 			}
 
 			info->txpower[i] = ath_get_rate_txpower(sc, bf, rix,
-								is_40, false);
+								is_40, false, true);
 			continue;
 		}
 
@@ -1350,7 +1368,7 @@ static void ath_buf_set_rate(struct ath_softc *sc, struct ath_buf *bf,
 
 		is_cck = IS_CCK_RATE(info->rates[i].Rate);
 		info->txpower[i] = ath_get_rate_txpower(sc, bf, rix, false,
-							is_cck);
+							is_cck, false);
 	}
 
 	/* For AR5416 - RTS cannot be followed by a frame larger than 8K */
-- 
2.30.2

Re: [PATCH] ath9k: fix per-packet TX-power cap for TPC

[Toke Høiland-Jørgensen]

Jonas Jelonek <jelonek.jonas@gmail.com> writes:

> Fix incorrect usage of plain rate_idx as index into the max (power) per
> rate lookup table.
>
> 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.
>
> 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.
>
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>

So what effect does this bug have in practice? Also, how did you test
the patch? :)

-Toke

Re: [PATCH] ath9k: fix per-packet TX-power cap for TPC

[Jonas Jelonek]

> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <toke@kernel.org> wrote:
> 
> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
> 
>> Fix incorrect usage of plain rate_idx as index into the max (power) per
>> rate lookup table.
>> 
>> 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.
>> 
>> 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.
>> 
>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> 
> So what effect does this bug have in practice? Also, how did you test
> the patch? :)
> 

It actually may not have an effect in most of current practical applications. The
member ‘tpc_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. 
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 
manually set a proper TX-power via my testing API. With other rates it is working
fine. I also double-checked that it isn’t a problem of how TX-power is set and
reported with our upcoming API.

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’t 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. 
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.

Jonas

Re: [PATCH] ath9k: fix per-packet TX-power cap for TPC

[Toke Høiland-Jørgensen]

Jonas Jelonek <jelonek.jonas@gmail.com> writes:

>> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <toke@kernel.org> wrote:
>> 
>> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>> 
>>> Fix incorrect usage of plain rate_idx as index into the max (power) per
>>> rate lookup table.
>>> 
>>> 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.
>>> 
>>> 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.
>>> 
>>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>> 
>> So what effect does this bug have in practice? Also, how did you test
>> the patch? :)
>> 
>
> It actually may not have an effect in most of current practical applications. The
> member ‘tpc_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. 
> 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 
> manually set a proper TX-power via my testing API. With other rates it is working
> fine. I also double-checked that it isn’t a problem of how TX-power is set and
> reported with our upcoming API.
>
> 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’t 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. 
> 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.

Alright, cool! Could you please add all this to the commit message and
resubmit?

-Toke

Re: [PATCH] ath9k: fix per-packet TX-power cap for TPC

[Jonas Jelonek]

> On 30. Mar 2023, at 14:34, Toke Høiland-Jørgensen <toke@kernel.org> wrote:
> 
> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
> 
>>> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <toke@kernel.org> wrote:
>>> 
>>> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>>> 
>>>> Fix incorrect usage of plain rate_idx as index into the max (power) per
>>>> rate lookup table.
>>>> 
>>>> 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.
>>>> 
>>>> 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.
>>>> 
>>>> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
>>> 
>>> So what effect does this bug have in practice? Also, how did you test
>>> the patch? :)
>>> 
>> 
>> It actually may not have an effect in most of current practical applications. The
>> member ‘tpc_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. 
>> 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 
>> manually set a proper TX-power via my testing API. With other rates it is working
>> fine. I also double-checked that it isn’t a problem of how TX-power is set and
>> reported with our upcoming API.
>> 
>> 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’t 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. 
>> 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.
> 
> Alright, cool! Could you please add all this to the commit message and
> resubmit?
> 
> -Toke

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

Jonas