Re: [PATCH] cfg80211: fix maximum tx power handling

Re: [PATCH] cfg80211: fix maximum tx power handling

[Mark Mentovai]

Felix Fietkau wrote:
> When setting a new regulatory domain after the initial one had
> been set by the driver, the original maximum power is
> overwritten with the new regulatory power value. This is wrong
> because chan->orig_mpwr is supposed to contain the hardware tx
> power limit.

I don’t think this is correct. In the existing code, chan->orig_mpwr
contains the channel’s maximum power in the driver’s requested
regulatory domain. There isn’t currently any field that’s used to
track the hardware limit.

The comment in net/wireless/reg.c explains the use of the orig_* fields:

		/*
		 * This gaurantees the driver's requested regulatory domain
		 * will always be used as a base for further regulatory
		 * settings
		 */

orig_flags and orig_mag track the values of flags and max_antenna_gain
for the driver’s requested regulatory domain in the same way that
orig_mpwr tracks max_power.

Your patch permits max_power to exceed the power level permitted in
the regulatory domain specified by the driver itself. This is contrary
to the design of the wireless regulatory framework. Values derived
from a driver hint must never be exceeded.

Your patch also doesn’t work properly for me using the ath9k driver.
With your patch, I wind up with an artificial power limit of 20dBm
across the board, although the hardware limit is actually 25dBm on the
AR9223 I’m testing with. This stems from the fact that ath9k doesn’t
know how to properly compute the hardware limit.
ath9k_init_txpower_limits is implemented on top of
ath9k_hw_set_txpowerlimit, which uses a channel’s current max_power
value as a basis for its computation. This means that it will either
use the hard-coded initial value of 20dBm from CHAN2G and CHAN5G in
drivers/net/wireless/ath/ath9k/init.c, or it will use world regulatory
domain values (also 20dBm) set by ath_regd_init_wiphy calling
wiphy_apply_custom_regulatory.

I don’t know if other drivers currently know how to properly compute
hardware power limits. I suspect that some (most?) also don’t.

I certainly like the idea of providing better feedback of the
effective power level, or even the power level limit, back to users.
This patch seems to artificially limit the effective power level in
some cases, while allowing a value in excess of what would be
appropriate in others.

Re: [PATCH] cfg80211: fix maximum tx power handling

[Luis R. Rodriguez]

John, I'll review this today.

  Luis

Re: [PATCH] cfg80211: fix maximum tx power handling

[John W. Linville]

On Mon, Jan 31, 2011 at 10:43:41AM -0800, Luis R. Rodriguez wrote:
> John, I'll review this today.

Any follow-up?

-- 
John W. Linville		Someday the world will need a hero, and you
linville@tuxdriver.com			might be all we have.  Be ready.

Re: [PATCH] cfg80211: fix maximum tx power handling

[Luis R. Rodriguez]

On Thu, Feb 3, 2011 at 1:25 PM, John W. Linville <linville@tuxdriver.com> wrote:
> On Mon, Jan 31, 2011 at 10:43:41AM -0800, Luis R. Rodriguez wrote:
>> John, I'll review this today.
>
> Any follow-up?

Sorry for the delay.. will review in a bit.....

 Luis

Re: [PATCH] cfg80211: fix maximum tx power handling

[Luis R. Rodriguez]

On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
> Felix Fietkau wrote:
>> When setting a new regulatory domain after the initial one had
>> been set by the driver, the original maximum power is
>> overwritten with the new regulatory power value. This is wrong
>> because chan->orig_mpwr is supposed to contain the hardware tx
>> power limit.
>
> I don’t think this is correct. In the existing code, chan->orig_mpwr
> contains the channel’s maximum power in the driver’s requested
> regulatory domain. There isn’t currently any field that’s used to
> track the hardware limit.
>
> The comment in net/wireless/reg.c explains the use of the orig_* fields:
>
>                /*
>                 * This gaurantees the driver's requested regulatory domain
>                 * will always be used as a base for further regulatory
>                 * settings
>                 */
>
> orig_flags and orig_mag track the values of flags and max_antenna_gain
> for the driver’s requested regulatory domain in the same way that
> orig_mpwr tracks max_power.
>
> Your patch permits max_power to exceed the power level permitted in
> the regulatory domain specified by the driver itself. This is contrary
> to the design of the wireless regulatory framework. Values derived
> from a driver hint must never be exceeded.
>
> Your patch also doesn’t work properly for me using the ath9k driver.
> With your patch, I wind up with an artificial power limit of 20dBm
> across the board, although the hardware limit is actually 25dBm on the
> AR9223 I’m testing with. This stems from the fact that ath9k doesn’t
> know how to properly compute the hardware limit.
> ath9k_init_txpower_limits is implemented on top of
> ath9k_hw_set_txpowerlimit, which uses a channel’s current max_power
> value as a basis for its computation. This means that it will either
> use the hard-coded initial value of 20dBm from CHAN2G and CHAN5G in
> drivers/net/wireless/ath/ath9k/init.c, or it will use world regulatory
> domain values (also 20dBm) set by ath_regd_init_wiphy calling
> wiphy_apply_custom_regulatory.

The real limit on ath9k comes from analyzing the CTLs from the EEPROM,
and using that as a max when a CTL is present. The value from cfg80211
is simply a local regulatory one, the CTLs take this one step further
and are calibration specific to help optimize performance on every
frequency range.

> I don’t know if other drivers currently know how to properly compute
> hardware power limits. I suspect that some (most?) also don’t.

This will vary on device and firmware. I started to bark down this
road a while ago but it was hard, you need a lot of internal knowledge
and participation from other vendors. At that time it was just
Atheros.

> I certainly like the idea of providing better feedback of the
> effective power level, or even the power level limit, back to users.
> This patch seems to artificially limit the effective power level in
> some cases, while allowing a value in excess of what would be
> appropriate in others.

John, Mark's interpretation is correct, the exception here is accepted
because the driver request is being respected to override the max
regulatory power, nothing more. Felix are you observing userspace
requests overriding the orig power?

 Luis

Re: [PATCH] cfg80211: fix maximum tx power handling

[Felix Fietkau]

On 2011-02-04 8:20 PM, Luis R. Rodriguez wrote:
> On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
>> I certainly like the idea of providing better feedback of the
>> effective power level, or even the power level limit, back to users.
>> This patch seems to artificially limit the effective power level in
>> some cases, while allowing a value in excess of what would be
>> appropriate in others.
> 
> John, Mark's interpretation is correct, the exception here is accepted
> because the driver request is being respected to override the max
> regulatory power, nothing more. Felix are you observing userspace
> requests overriding the orig power?
What I'm observing is that with the first regdomain setting it sets
orig_power to something that exceeds the tx power value that the
hardware is capable of using.
Because of that, the tx power setting reported to the user is wrong.

- Felix

Re: [PATCH] cfg80211: fix maximum tx power handling

[Luis R. Rodriguez]

On Fri, Feb 4, 2011 at 11:26 AM, Felix Fietkau <nbd@openwrt.org> wrote:
> On 2011-02-04 8:20 PM, Luis R. Rodriguez wrote:
>> On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
>>> I certainly like the idea of providing better feedback of the
>>> effective power level, or even the power level limit, back to users.
>>> This patch seems to artificially limit the effective power level in
>>> some cases, while allowing a value in excess of what would be
>>> appropriate in others.
>>
>> John, Mark's interpretation is correct, the exception here is accepted
>> because the driver request is being respected to override the max
>> regulatory power, nothing more. Felix are you observing userspace
>> requests overriding the orig power?
>
> What I'm observing is that with the first regdomain setting it sets
> orig_power to something that exceeds the tx power value that the
> hardware is capable of using.

Can you provide me with steps to repro?

> Because of that, the tx power setting reported to the user is wrong.

I see

  Luis

Re: [PATCH] cfg80211: fix maximum tx power handling

[Mark Mentovai]

Luis R. Rodriguez wrote:
> On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
>> Your patch also doesn’t work properly for me using the ath9k driver.
>> With your patch, I wind up with an artificial power limit of 20dBm
>> across the board, although the hardware limit is actually 25dBm on the
>> AR9223 I’m testing with. This stems from the fact that ath9k doesn’t
>> know how to properly compute the hardware limit.
>> ath9k_init_txpower_limits is implemented on top of
>> ath9k_hw_set_txpowerlimit, which uses a channel’s current max_power
>> value as a basis for its computation. This means that it will either
>> use the hard-coded initial value of 20dBm from CHAN2G and CHAN5G in
>> drivers/net/wireless/ath/ath9k/init.c, or it will use world regulatory
>> domain values (also 20dBm) set by ath_regd_init_wiphy calling
>> wiphy_apply_custom_regulatory.
>
> The real limit on ath9k comes from analyzing the CTLs from the EEPROM,
> and using that as a max when a CTL is present. The value from cfg80211
> is simply a local regulatory one, the CTLs take this one step further
> and are calibration specific to help optimize performance on every
> frequency range.

Of course. My point was merely that the CTL-based calculations that
ath9k does in ath9k_init_txpower_limits when it tries to set a better
.max_power don’t even come out correctly, because they’re based on the
initial values or on regdomain values. They’re not actually the
hardware’s limits. I’m not even positive that there’s a single right
“hardware transmit power limit per channel” value, unless you were to
examine all of the possible rates and perhaps antenna configurations
and other parameters. ath9k_init_txpower_limits doesn’t do this.
That’s why I saw the patch restricting transmit power to be 20dBm even
though the CTL data would allow higher power.

In other words, I think babcbc295fee766ca710235e431686fef744d9a6 was wrong.

Felix Fietkau wrote:
> What I'm observing is that with the first regdomain setting it sets
> orig_power to something that exceeds the tx power value that the
> hardware is capable of using.

But orig_mpwr is only supposed to be the maximum transmit power for
the channel in the driver’s requested regulatory domain. It’s not
supposed to have anything to do with the hardware.

If you can come up with a good way to compute the hardware maximum per
channel (overcoming ath9k_init_txpower_limits’s current faults), then
I think this can be accommodated with another field, but I don’t think
orig_mpwr should be overloaded. Maybe renamed to better reflect its
purpose, though.

> Because of that, the tx power setting reported to the user is wrong.

I don’t think that a good way for a driver to reflect the transmit
power it has actually put into effect (after taking the effects of CTL
limits) back to user space currently exists.

Re: [PATCH] cfg80211: fix maximum tx power handling

[Luis R. Rodriguez]

On Fri, Feb 4, 2011 at 11:42 AM, Mark Mentovai <mark@moxienet.com> wrote:
> Luis R. Rodriguez wrote:
>> On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
>>> Your patch also doesn’t work properly for me using the ath9k driver.
>>> With your patch, I wind up with an artificial power limit of 20dBm
>>> across the board, although the hardware limit is actually 25dBm on the
>>> AR9223 I’m testing with. This stems from the fact that ath9k doesn’t
>>> know how to properly compute the hardware limit.
>>> ath9k_init_txpower_limits is implemented on top of
>>> ath9k_hw_set_txpowerlimit, which uses a channel’s current max_power
>>> value as a basis for its computation. This means that it will either
>>> use the hard-coded initial value of 20dBm from CHAN2G and CHAN5G in
>>> drivers/net/wireless/ath/ath9k/init.c, or it will use world regulatory
>>> domain values (also 20dBm) set by ath_regd_init_wiphy calling
>>> wiphy_apply_custom_regulatory.
>>
>> The real limit on ath9k comes from analyzing the CTLs from the EEPROM,
>> and using that as a max when a CTL is present. The value from cfg80211
>> is simply a local regulatory one, the CTLs take this one step further
>> and are calibration specific to help optimize performance on every
>> frequency range.
>
> Of course. My point was merely that the CTL-based calculations that
> ath9k does in ath9k_init_txpower_limits when it tries to set a better
> .max_power don’t even come out correctly, because they’re based on the
> initial values or on regdomain values. They’re not actually the
> hardware’s limits. I’m not even positive that there’s a single right
> “hardware transmit power limit per channel” value, unless you were to
> examine all of the possible rates and perhaps antenna configurations
> and other parameters. ath9k_init_txpower_limits doesn’t do this.
> That’s why I saw the patch restricting transmit power to be 20dBm even
> though the CTL data would allow higher power.

Well the final real power programmed is computed dynamically because
as you mentioned there are things to consider other than regulatory /
device limits to get a right power we need to use.

> In other words, I think babcbc295fee766ca710235e431686fef744d9a6 was wrong.

I'll need some time to review this again...

> Felix Fietkau wrote:
>> What I'm observing is that with the first regdomain setting it sets
>> orig_power to something that exceeds the tx power value that the
>> hardware is capable of using.
>
> But orig_mpwr is only supposed to be the maximum transmit power for
> the channel in the driver’s requested regulatory domain. It’s not
> supposed to have anything to do with the hardware.

Mark is correct, the device limit is also used as part of the dynamic
computation for what power you want to use, the upper boundary is this
hardware limit.

> If you can come up with a good way to compute the hardware maximum per
> channel (overcoming ath9k_init_txpower_limits’s current faults), then
> I think this can be accommodated with another field, but I don’t think
> orig_mpwr should be overloaded. Maybe renamed to better reflect its
> purpose, though.

I do wonder if the patch above introduced a regression I overlooked..

>> Because of that, the tx power setting reported to the user is wrong.
>
> I don’t think that a good way for a driver to reflect the transmit
> power it has actually put into effect (after taking the effects of CTL
> limits) back to user space currently exists.

What do you mean here, sorry I could not follow.

  Luis

Re: [PATCH] cfg80211: fix maximum tx power handling

[Mark Mentovai]

Luis R. Rodriguez wrote:
> On Fri, Feb 4, 2011 at 11:42 AM, Mark Mentovai <mark@moxienet.com> wrote:
>> If you can come up with a good way to compute the hardware maximum per
>> channel (overcoming ath9k_init_txpower_limits’s current faults), then
>> I think this can be accommodated with another field, but I don’t think
>> orig_mpwr should be overloaded. Maybe renamed to better reflect its
>> purpose, though.
>
> I do wonder if the patch above introduced a regression I overlooked..

(Re babcbc295fee766ca710235e431686fef744d9a6)

I don’t think it introduced any regressions, because max_power and
orig_mpwr are always updated with regulatory-based values. I think
that the patch was effectively a no-op. It tried to set better initial
values, but the regulatory values that superseded them didn’t change.

>> I don’t think that a good way for a driver to reflect the transmit
>> power it has actually put into effect (after taking the effects of CTL
>> limits) back to user space currently exists.
>
> What do you mean here, sorry I could not follow.

The user space interface for txpower winds up getting piped through
cfg80211_ops’ set_tx_power and get_tx_power fields. For mac80211,
net/mac80211/cfg.c sends set_tx_power requests through to the driver
(ieee80211_set_tx_power calls ieee80211_hw_config which calls through
to an ieee80211_ops config field), but get_tx_power requests are
satisfied without ever checking the driver. mac80211 get_tx_power just
pulls a cached value out of an ieee80211_local. A driver couldn’t even
push a better value back into mac80211, because its ieee80211_local is
private.

Felix Fietkau wrote:
> Right, for testing the tx power, we need to override the
> twiceMaxRegulatoryPower parameter that we pass to the set_txpower op.

That would work for ath9k. I still think it’d be prudent to examine a
handful of rates. I also seem to remember that some other values that
were being used within ath9k_init_txpower_limits were invalid. I think
ntxchains was invalid during that early phase of initialization, for
example. That might not have any ultimate effect on the result, but
it’s not right to hit that ath_debug claiming "Invalid chainmask
configuration" once for every supported channel every time the driver
initializes.

Re: [PATCH] cfg80211: fix maximum tx power handling

[Felix Fietkau]

On 2011-02-04 9:19 PM, Mark Mentovai wrote:
> That would work for ath9k. I still think it’d be prudent to examine a
> handful of rates. I also seem to remember that some other values that
> were being used within ath9k_init_txpower_limits were invalid. I think
> ntxchains was invalid during that early phase of initialization, for
> example. That might not have any ultimate effect on the result, but
> it’s not right to hit that ath_debug claiming "Invalid chainmask
> configuration" once for every supported channel every time the driver
> initializes.
I'll take a look at fixing all of that stuff soon, then we can go back
to treating the driver's tx power values as a limit afterwards.

- Felix

Re: [PATCH] cfg80211: fix maximum tx power handling

[Felix Fietkau]

On 2011-02-04 8:42 PM, Mark Mentovai wrote:
> Luis R. Rodriguez wrote:
>> On Sun, Jan 30, 2011 at 9:01 AM, Mark Mentovai <mark@moxienet.com> wrote:
>>> Your patch also doesn’t work properly for me using the ath9k driver.
>>> With your patch, I wind up with an artificial power limit of 20dBm
>>> across the board, although the hardware limit is actually 25dBm on the
>>> AR9223 I’m testing with. This stems from the fact that ath9k doesn’t
>>> know how to properly compute the hardware limit.
>>> ath9k_init_txpower_limits is implemented on top of
>>> ath9k_hw_set_txpowerlimit, which uses a channel’s current max_power
>>> value as a basis for its computation. This means that it will either
>>> use the hard-coded initial value of 20dBm from CHAN2G and CHAN5G in
>>> drivers/net/wireless/ath/ath9k/init.c, or it will use world regulatory
>>> domain values (also 20dBm) set by ath_regd_init_wiphy calling
>>> wiphy_apply_custom_regulatory.
>>
>> The real limit on ath9k comes from analyzing the CTLs from the EEPROM,
>> and using that as a max when a CTL is present. The value from cfg80211
>> is simply a local regulatory one, the CTLs take this one step further
>> and are calibration specific to help optimize performance on every
>> frequency range.
> 
> Of course. My point was merely that the CTL-based calculations that
> ath9k does in ath9k_init_txpower_limits when it tries to set a better
> .max_power don’t even come out correctly, because they’re based on the
> initial values or on regdomain values. They’re not actually the
> hardware’s limits. I’m not even positive that there’s a single right
> “hardware transmit power limit per channel” value, unless you were to
> examine all of the possible rates and perhaps antenna configurations
> and other parameters. ath9k_init_txpower_limits doesn’t do this.
> That’s why I saw the patch restricting transmit power to be 20dBm even
> though the CTL data would allow higher power.
> 
> In other words, I think babcbc295fee766ca710235e431686fef744d9a6 was wrong.
Right, for testing the tx power, we need to override the
twiceMaxRegulatoryPower parameter that we pass to the set_txpower op.

Additionally, we could examine more rates, but I haven't seen any card
where higher rates have a higher tx power setting that lower rates. I
think Atheros internally also uses the tx power setting for the lowest rate.

Other than the different rates, I don't think you need to examine more
configurations. The power increase by using multiple chains is already
added to the max tx power value.

- Felix

[PATCH] cfg80211: fix maximum tx power handling

[Felix Fietkau]

When setting a new regulatory domain after the initial one had been set by
the driver, the original maximum power is overwritten with the new regulatory
power value. This is wrong because chan->orig_mpwr is supposed to contain the
hardware tx power limit.
Because of this, the displayed tx power is often much higher than what the
hardware is actually capable of using.

Fix this by always using chan->orig_mpwr as a maximum and never overwriting
it in the regulatory handling code.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>
Cc: stable@kernel.org
---
 net/wireless/reg.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index c565689..2eb6eca 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -838,6 +838,12 @@ static void handle_channel(struct wiphy *wiphy,
 	if (freq_range->max_bandwidth_khz < MHZ_TO_KHZ(40))
 		bw_flags = IEEE80211_CHAN_NO_HT40;
 
+	if (chan->orig_mpwr)
+		chan->max_power = min(chan->orig_mpwr,
+			(int) MBM_TO_DBM(power_rule->max_eirp));
+	else
+		chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
+
 	if (last_request->initiator == NL80211_REGDOM_SET_BY_DRIVER &&
 	    request_wiphy && request_wiphy == wiphy &&
 	    request_wiphy->flags & WIPHY_FLAG_STRICT_REGULATORY) {
@@ -850,19 +856,12 @@ static void handle_channel(struct wiphy *wiphy,
 			map_regdom_flags(reg_rule->flags) | bw_flags;
 		chan->max_antenna_gain = chan->orig_mag =
 			(int) MBI_TO_DBI(power_rule->max_antenna_gain);
-		chan->max_power = chan->orig_mpwr =
-			(int) MBM_TO_DBM(power_rule->max_eirp);
 		return;
 	}
 
 	chan->flags = flags | bw_flags | map_regdom_flags(reg_rule->flags);
 	chan->max_antenna_gain = min(chan->orig_mag,
 		(int) MBI_TO_DBI(power_rule->max_antenna_gain));
-	if (chan->orig_mpwr)
-		chan->max_power = min(chan->orig_mpwr,
-			(int) MBM_TO_DBM(power_rule->max_eirp));
-	else
-		chan->max_power = (int) MBM_TO_DBM(power_rule->max_eirp);
 }
 
 static void handle_band(struct wiphy *wiphy,
-- 
1.7.3.2