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 <[email protected]>
---
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
Jonas Jelonek <[email protected]> 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 <[email protected]>
So what effect does this bug have in practice? Also, how did you test
the patch? :)
-Toke
> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Jonas Jelonek <[email protected]> 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 <[email protected]>
>
> 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
Jonas Jelonek <[email protected]> writes:
>> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <[email protected]> wrote:
>>
>> Jonas Jelonek <[email protected]> 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 <[email protected]>
>>
>> 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
> On 30. Mar 2023, at 14:34, Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Jonas Jelonek <[email protected]> writes:
>
>>> On 30. Mar 2023, at 11:31, Toke Høiland-Jørgensen <[email protected]> wrote:
>>>
>>> Jonas Jelonek <[email protected]> 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 <[email protected]>
>>>
>>> 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