[PATCH v1] wifi: mt76: fix backoff fields and max_power calculation
Sven Eckelmann
sven at narfation.org
Mon Jan 26 01:54:51 PST 2026
Please change your prefix from [PATCH v1] to [PATCH mt76 v1].
On Friday, 23 January 2026 10:32:34 CET Ryder Lee wrote:
> From: Allen Ye <allen.ye at mediatek.com>
>
> The maximum power value may exist in data or backoff field.
> To reponse the correct value of txpower, mt76 should also consider
> these values in sku table.
>
> Fixes: b05ab4be9fd7 (wifi: mt76: mt7915: add bf backoff limit table support)
> Signed-off-by: Allen Ye <allen.ye at mediatek.com>
> Signed-off-by: Ryder Lee <ryder.lee at mediatek.com>
> ---
> v1 - add "wifi:" prefix into subject
Please send your patches before submission through checkpatch. Tools like
b4 [1] can help you to have a good workflow by reminding your about this
(and provide good templates for many other things)
WARNING: 'reponse' may be misspelled - perhaps 'response'?
#126:
To reponse the correct value of txpower, mt76 should also consider
^^^^^^^
WARNING: Please use correct Fixes: style 'Fixes: <12+ chars of sha1> ("<title line>")' - ie: 'Fixes: b05ab4be9fd7 ("wifi: mt76: mt7915: add bf backoff limit table support")'
#129:
Fixes: b05ab4be9fd7 (wifi: mt76: mt7915: add bf backoff limit table support)
ERROR: that open brace { should be on the previous line
#173: FILE: drivers/net/wireless/mediatek/mt76/eeprom.c:319:
+ static const u8 connac3_table[] =
+ {1, 2, 3, 4, 5, 2, 3, 4, 5, 3, 4, 5, 4, 5, 5};
ERROR: that open brace { should be on the previous line
#175: FILE: drivers/net/wireless/mediatek/mt76/eeprom.c:321:
+ static const u8 connac2_table[] =
+ {1, 2, 3, 4, 2, 3, 4, 3, 4, 4, 0, 0, 0, 0, 0};
CHECK: Macro argument reuse 'arr' - possible side-effects?
#288: FILE: drivers/net/wireless/mediatek/mt76/eeprom.c:462:
+#define __apply_array_limit(arr, type) \
+ mt76_apply_array_limit(dev, (arr), ARRAY_SIZE(arr), val, target_power, \
+ txs_delta, &max_power, n_chains, type)
CHECK: Macro argument reuse 'arr' - possible side-effects?
#292: FILE: drivers/net/wireless/mediatek/mt76/eeprom.c:466:
+#define __apply_multi_array_limit(arr, type) \
+ mt76_apply_multi_array_limit(dev, (arr)[0], ARRAY_SIZE((arr)[0]), \
+ ARRAY_SIZE(arr), val, len, target_power, \
+ txs_delta, &max_power, n_chains, type)
total: 2 errors, 2 warnings, 2 checks, 233 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
> static void
> -mt76_apply_array_limit(s8 *pwr, size_t pwr_len, const s8 *data,
> - s8 target_power, s8 nss_delta, s8 *max_power)
> +mt76_apply_array_limit(struct mt76_dev *dev, s8 *pwr, size_t pwr_len,
> + const s8 *data, s8 target_power, s8 nss_delta,
> + s8 *max_power, int n_chains, enum mt76_sku_type type)
> {
> int i;
>
> @@ -316,18 +337,50 @@ mt76_apply_array_limit(s8 *pwr, size_t pwr_len, const s8 *data,
> return;
>
> for (i = 0; i < pwr_len; i++) {
> - pwr[i] = min_t(s8, target_power, data[i] + nss_delta);
> + s8 backoff_delta, delta = mt76_tx_power_path_delta(n_chains);
Please avoid multiple function declarations per line. "To this end, use just
one data declaration per line (no commas for multiple data declarations)"
This also looks on the first glance like destructuring (which doesn't exist
in C).
> + int backoff_n_chains = 0;
> +
> + switch (type) {
> + case MT76_SKU_RATE:
> + pwr[i] = min_t(s8, target_power, data[i] + nss_delta);
> + break;
> + case MT76_SKU_BACKOFF:
> + backoff_n_chains = mt76_backoff_n_chains(dev, i);
> + backoff_delta = mt76_tx_power_path_delta(backoff_n_chains);
> + pwr[i] = min_t(s8, target_power + delta - backoff_delta,
> + data[i] + nss_delta);
> + break;
> + case MT76_SKU_BACKOFF_BF_OFFSET:
> + backoff_n_chains = mt76_backoff_n_chains(dev, i + 1);
> + backoff_delta = mt76_tx_power_path_delta(backoff_n_chains);
> + pwr[i] = min_t(s8, target_power + delta - backoff_delta,
> + data[i] + nss_delta);
> + break;
> + default:
> + return;
> + }
Instead of calculating something different in every case, couldn't this be harmonized as
int backoff_n_chains;
s8 backoff_delta;
s8 delta;
switch (type) {
case MT76_SKU_RATE:
delta = 0;
backoff_n_chains = 0;
backoff_delta = 0;
break;
case MT76_SKU_BACKOFF:
delta = mt76_tx_power_path_delta(n_chains);
backoff_n_chains = mt76_backoff_n_chains(dev, i);
backoff_delta = mt76_tx_power_path_delta(backoff_n_chains);
break;
case MT76_SKU_BACKOFF_BF_OFFSET:
delta = mt76_tx_power_path_delta(n_chains);
backoff_n_chains = mt76_backoff_n_chains(dev, i + 1);
backoff_delta = mt76_tx_power_path_delta(backoff_n_chains);
break;
default:
return;
}
pwr[i] = min_t(s8, target_power + delta - backoff_delta, data[i] + nss_delta);
or
u8 backoff_chain_idx = i;
int backoff_n_chains;
s8 backoff_delta;
s8 delta;
switch (type) {
case MT76_SKU_RATE:
delta = 0;
backoff_n_chains = 0;
backoff_delta = 0;
break;
case MT76_SKU_BACKOFF:
backoff_chain_index += 1;
fallthrough;
case MT76_SKU_BACKOFF_BF_OFFSET:
delta = mt76_tx_power_path_delta(n_chains);
backoff_n_chains = mt76_backoff_n_chains(dev, backoff_chain_idx);
backoff_delta = mt76_tx_power_path_delta(backoff_n_chains);
break;
default:
return;
}
pwr[i] = min_t(s8, target_power + delta - backoff_delta, data[i] + nss_delta);
[...]
> static void
> -mt76_apply_multi_array_limit(s8 *pwr, size_t pwr_len, s8 pwr_num,
> - const s8 *data, size_t len, s8 target_power,
> - s8 nss_delta)
> +mt76_apply_multi_array_limit(struct mt76_dev *dev, s8 *pwr, size_t pwr_len,
> + s8 pwr_num, const s8 *data, size_t len,
> + s8 target_power, s8 nss_delta, s8 *max_power,
> + int n_chains, enum mt76_sku_type type)
> {
> int i, cur;
> - s8 max_power = -128;
> +
> +#define connac2_backoff_ru_idx 2
why this define in the middle of the function? Should this be in a header?
Should it just be a const?
>
> if (!data)
> return;
> @@ -337,8 +390,13 @@ mt76_apply_multi_array_limit(s8 *pwr, size_t pwr_len, s8 pwr_num,
> if (len < pwr_len + 1)
> break;
>
> - mt76_apply_array_limit(pwr + pwr_len * i, pwr_len, data + 1,
> - target_power, nss_delta, &max_power);
> + if (!is_mt799x(dev) && type == MT76_SKU_BACKOFF &&
> + i > connac2_backoff_ru_idx)
> + type = MT76_SKU_BACKOFF_BF_OFFSET;
Can you please explain in the commit message how this format is structured. So
why everything after the third line of the expanded multi-array limit must be
considered a MT76_SKU_BACKOFF_BF_OFFSET? Adding some kernel-doc to each item in
enum mt76_sku_type might help too.
I have only seen one device in the wild (MT7905DAN + MT7975DN) which actually
uses the DT for the limits
https://github.com/openwrt/openwrt/blob/17d45f677b0eb1da3c8190a2778fcdfcd9ecd234/target/linux/ramips/dts/mt7621_plasmacloud_pax1800-lite.dts#L11
And there, it just uses the 2T1ss to the place where 1T1ss might be for beamforming.
And this field doesn't seem to make a lot of sense (beamforming with 1 antennna?)
and needs to be skipped.
So yes, I can answer it by looking at some code I've used before for converting
some CSV [2] to DT entries:
rates_table = {
"paths-cck": [
row[0:4],
],
"paths-ofdm": [
row[4:8],
],
"paths-ofdm-bf": [
[
row[8],
*row[8:11],
], # duplicating first entry because it is needed in DTS but will not actually be used
],
"paths-ru": compress_rates(
[
[1, *row[87:97]], # RU26
[1, *row[107:117]], # RU52
[1, *row[127:137]], # RU106
[1, *row[11:21]], # BW20 (HT/VHT)
[1, *row[30:40]], # BW40 (HT/VHT)
[1, *row[49:59]], # BW80 (HT/VHT)
[1, *row[68:78]], # BW160 (HT/VHT)
]
),
"paths-ru-bf": compress_rates(
[
[1, *row[97:107]], # RU26
[1, *row[117:127]], # RU52
[1, *row[137:147]], # RU106
[
1,
row[21],
*row[21:30],
], # BW20 (HT/VHT), duplicating first entry
[
1,
row[40],
*row[40:49],
], # BW40 (HT/VHT), duplicating first entry
[
1,
row[59],
*row[59:68],
], # BW80 (HT/VHT), duplicating first entry
[
1,
row[78],
*row[78:87],
], # BW160 (HT/VHT), duplicating first entry
]
),
}
But it would be good to have this written down somewhere.
> @@ -360,18 +418,11 @@ s8 mt76_get_rate_power_limits(struct mt76_phy *phy,
> struct device_node *np;
> const s8 *val;
> char name[16];
> - u32 mcs_rates = dev->drv->mcs_rates;
> - u32 ru_rates = ARRAY_SIZE(dest->ru[0]);
> char band;
> size_t len;
> - s8 max_power = 0;
> - s8 max_power_backoff = -127;
> + s8 max_power = -127;
> s8 txs_delta;
> int n_chains = hweight16(phy->chainmask);
> - s8 target_power_combine = target_power + mt76_tx_power_path_delta(n_chains);
> -
> - if (!mcs_rates)
> - mcs_rates = 10;
>
> memset(dest, target_power, sizeof(*dest) - sizeof(dest->path));
> memset(&dest->path, 0, sizeof(dest->path));
> @@ -408,47 +459,51 @@ s8 mt76_get_rate_power_limits(struct mt76_phy *phy,
>
> txs_delta = mt76_get_txs_delta(np, hweight16(phy->chainmask));
>
> +#define __apply_array_limit(arr, type) \
> + mt76_apply_array_limit(dev, (arr), ARRAY_SIZE(arr), val, target_power, \
> + txs_delta, &max_power, n_chains, type)
> +
> +#define __apply_multi_array_limit(arr, type) \
> + mt76_apply_multi_array_limit(dev, (arr)[0], ARRAY_SIZE((arr)[0]), \
> + ARRAY_SIZE(arr), val, len, target_power, \
> + txs_delta, &max_power, n_chains, type)
> +
> val = mt76_get_of_array_s8(np, "rates-cck", &len, ARRAY_SIZE(dest->cck));
> - mt76_apply_array_limit(dest->cck, ARRAY_SIZE(dest->cck), val,
> - target_power, txs_delta, &max_power);
> -
> - val = mt76_get_of_array_s8(np, "rates-ofdm",
> - &len, ARRAY_SIZE(dest->ofdm));
> - mt76_apply_array_limit(dest->ofdm, ARRAY_SIZE(dest->ofdm), val,
> - target_power, txs_delta, &max_power);
> -
> - val = mt76_get_of_array_s8(np, "rates-mcs", &len, mcs_rates + 1);
> - mt76_apply_multi_array_limit(dest->mcs[0], ARRAY_SIZE(dest->mcs[0]),
> - ARRAY_SIZE(dest->mcs), val, len,
> - target_power, txs_delta);
> -
> - val = mt76_get_of_array_s8(np, "rates-ru", &len, ru_rates + 1);
> - mt76_apply_multi_array_limit(dest->ru[0], ARRAY_SIZE(dest->ru[0]),
> - ARRAY_SIZE(dest->ru), val, len,
> - target_power, txs_delta);
> -
> - max_power_backoff = max_power;
> - val = mt76_get_of_array_s8(np, "paths-cck", &len, ARRAY_SIZE(dest->path.cck));
> - mt76_apply_array_limit(dest->path.cck, ARRAY_SIZE(dest->path.cck), val,
> - target_power_combine, txs_delta, &max_power_backoff);
> -
> - val = mt76_get_of_array_s8(np, "paths-ofdm", &len, ARRAY_SIZE(dest->path.ofdm));
> - mt76_apply_array_limit(dest->path.ofdm, ARRAY_SIZE(dest->path.ofdm), val,
> - target_power_combine, txs_delta, &max_power_backoff);
> -
> - val = mt76_get_of_array_s8(np, "paths-ofdm-bf", &len, ARRAY_SIZE(dest->path.ofdm_bf));
> - mt76_apply_array_limit(dest->path.ofdm_bf, ARRAY_SIZE(dest->path.ofdm_bf), val,
> - target_power_combine, txs_delta, &max_power_backoff);
> -
> - val = mt76_get_of_array_s8(np, "paths-ru", &len, ARRAY_SIZE(dest->path.ru[0]) + 1);
> - mt76_apply_multi_array_limit(dest->path.ru[0], ARRAY_SIZE(dest->path.ru[0]),
> - ARRAY_SIZE(dest->path.ru), val, len,
> - target_power_combine, txs_delta);
> -
> - val = mt76_get_of_array_s8(np, "paths-ru-bf", &len, ARRAY_SIZE(dest->path.ru_bf[0]) + 1);
> - mt76_apply_multi_array_limit(dest->path.ru_bf[0], ARRAY_SIZE(dest->path.ru_bf[0]),
> - ARRAY_SIZE(dest->path.ru_bf), val, len,
> - target_power_combine, txs_delta);
> + __apply_array_limit(dest->cck, MT76_SKU_RATE);
> +
> + val = mt76_get_of_array_s8(np, "rates-ofdm", &len, ARRAY_SIZE(dest->ofdm));
> + __apply_array_limit(dest->ofdm, MT76_SKU_RATE);
> +
> + val = mt76_get_of_array_s8(np, "rates-mcs", &len,
> + ARRAY_SIZE(dest->mcs[0]) + 1);
> + __apply_multi_array_limit(dest->mcs, MT76_SKU_RATE);
> +
> + val = mt76_get_of_array_s8(np, "rates-ru", &len,
> + ARRAY_SIZE(dest->ru[0]) + 1);
> + __apply_multi_array_limit(dest->ru, MT76_SKU_RATE);
> +
> + val = mt76_get_of_array_s8(np, "paths-cck", &len,
> + ARRAY_SIZE(dest->path.cck));
> + __apply_array_limit(dest->path.cck, MT76_SKU_BACKOFF);
> +
> + val = mt76_get_of_array_s8(np, "paths-ofdm", &len,
> + ARRAY_SIZE(dest->path.ofdm));
> + __apply_array_limit(dest->path.ofdm, MT76_SKU_BACKOFF);
> +
> + val = mt76_get_of_array_s8(np, "paths-ofdm-bf", &len,
> + ARRAY_SIZE(dest->path.ofdm_bf));
> + __apply_array_limit(dest->path.ofdm_bf, MT76_SKU_BACKOFF_BF_OFFSET);
> +
> + val = mt76_get_of_array_s8(np, "paths-ru", &len,
> + ARRAY_SIZE(dest->path.ru[0]) + 1);
> + __apply_multi_array_limit(dest->path.ru, MT76_SKU_BACKOFF);
> +
> + val = mt76_get_of_array_s8(np, "paths-ru-bf", &len,
> + ARRAY_SIZE(dest->path.ru_bf[0]) + 1);
> + __apply_multi_array_limit(dest->path.ru_bf, MT76_SKU_BACKOFF);
> +
> +#undef __apply_array_limit
> +#undef __apply_multi_array_limit
Is such a big style change really necessary for a patch which is a fix
(according to you)? This might just make it harder to spot the differences and
might (in some cases) make it harder to backport it to stable kernels.
Regards,
Sven
[1] https://b4.docs.kernel.org/en/latest/
[2] https://github.com/openwrt/openwrt/pull/20152#issuecomment-3341491145
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20260126/2c4b2615/attachment-0001.sig>
More information about the Linux-mediatek
mailing list