[ath9k-devel] [PATCH] ath9k: interpret requested txpower in EIRP domain

[ath9k-devel] [PATCH] ath9k: interpret requested txpower in EIRP domain

[Zefir Kurtisi]

Tx power limitations at upper layers are interpreted in
the EIRP domain. When the user requests a given maximum
txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
he expects the EIRP to be at or below 15dBm.

In ath9k_hw_apply_txpower(), the interpretation is
different: the antenna-gain is capped against the
current txpower limit in the regulatory, but not
against the user set value. It ensures that the
resulting EIRP is below the limit defined by the
active countrycode, but not below the value the
user requested.

In a scenario like e.g.
 a) antenna_gain=6
 b) countrycode limits to eirp=18
 c) user set txpower=15
this will cause a setting for AR_PHY_POWER_TX_RATE
regs resulting in an EIRP > 15.

This patch ensures that antenna-gain is considered
whenever the txpower limit is adjusted and with that
the user set limits are kept.

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
 drivers/net/wireless/ath/ath9k/hw.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index e7a3101..30be8a5 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -2834,7 +2834,6 @@ void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan,
 	struct ath_regulatory *reg = ath9k_hw_regulatory(ah);
 	struct ieee80211_channel *channel;
 	int chan_pwr, new_pwr, max_gain;
-	int ant_gain, ant_reduction = 0;
 
 	if (!chan)
 		return;
@@ -2842,15 +2841,10 @@ void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan,
 	channel = chan->chan;
 	chan_pwr = min_t(int, channel->max_power * 2, MAX_RATE_POWER);
 	new_pwr = min_t(int, chan_pwr, reg->power_limit);
-	max_gain = chan_pwr - new_pwr + channel->max_antenna_gain * 2;
-
-	ant_gain = get_antenna_gain(ah, chan);
-	if (ant_gain > max_gain)
-		ant_reduction = ant_gain - max_gain;
 
 	ah->eep_ops->set_txpower(ah, chan,
 				 ath9k_regd_get_ctl(reg, chan),
-				 ant_reduction, new_pwr, test);
+				 get_antenna_gain(ah, chan), new_pwr, test);
 }
 
 void ath9k_hw_set_txpowerlimit(struct ath_hw *ah, u32 limit, bool test)
-- 
2.7.4

[ath9k-devel] [PATCH] ath9k: interpret requested txpower in EIRP domain

[kbuild test robot]

Hi Zefir,

[auto build test WARNING on wireless-drivers/master]
[also build test WARNING on v4.6-rc1 next-20160401]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Zefir-Kurtisi/ath9k-interpret-requested-txpower-in-EIRP-domain/20160401-001612
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers.git master
config: i386-allmodconfig (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/net/wireless/ath/ath9k/hw.c: In function 'ath9k_hw_apply_txpower':
>> drivers/net/wireless/ath/ath9k/hw.c:2836:25: warning: unused variable 'max_gain' [-Wunused-variable]
     int chan_pwr, new_pwr, max_gain;
                            ^

vim +/max_gain +2836 drivers/net/wireless/ath/ath9k/hw.c

ca2c68cc Felix Fietkau 2011-10-08  2820  {
ca2c68cc Felix Fietkau 2011-10-08  2821  	enum eeprom_param gain_param;
ca2c68cc Felix Fietkau 2011-10-08  2822  
ca2c68cc Felix Fietkau 2011-10-08  2823  	if (IS_CHAN_2GHZ(chan))
ca2c68cc Felix Fietkau 2011-10-08  2824  		gain_param = EEP_ANTENNA_GAIN_2G;
ca2c68cc Felix Fietkau 2011-10-08  2825  	else
ca2c68cc Felix Fietkau 2011-10-08  2826  		gain_param = EEP_ANTENNA_GAIN_5G;
ca2c68cc Felix Fietkau 2011-10-08  2827  
ca2c68cc Felix Fietkau 2011-10-08  2828  	return ah->eep_ops->get_eeprom(ah, gain_param);
ca2c68cc Felix Fietkau 2011-10-08  2829  }
ca2c68cc Felix Fietkau 2011-10-08  2830  
64ea57d0 Gabor Juhos   2012-04-15  2831  void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan,
64ea57d0 Gabor Juhos   2012-04-15  2832  			    bool test)
ca2c68cc Felix Fietkau 2011-10-08  2833  {
ca2c68cc Felix Fietkau 2011-10-08  2834  	struct ath_regulatory *reg = ath9k_hw_regulatory(ah);
ca2c68cc Felix Fietkau 2011-10-08  2835  	struct ieee80211_channel *channel;
ca2c68cc Felix Fietkau 2011-10-08 @2836  	int chan_pwr, new_pwr, max_gain;
ca2c68cc Felix Fietkau 2011-10-08  2837  
ca2c68cc Felix Fietkau 2011-10-08  2838  	if (!chan)
ca2c68cc Felix Fietkau 2011-10-08  2839  		return;
ca2c68cc Felix Fietkau 2011-10-08  2840  
ca2c68cc Felix Fietkau 2011-10-08  2841  	channel = chan->chan;
ca2c68cc Felix Fietkau 2011-10-08  2842  	chan_pwr = min_t(int, channel->max_power * 2, MAX_RATE_POWER);
ca2c68cc Felix Fietkau 2011-10-08  2843  	new_pwr = min_t(int, chan_pwr, reg->power_limit);
ca2c68cc Felix Fietkau 2011-10-08  2844  

:::::: The code at line 2836 was first introduced by commit
:::::: ca2c68cc7bc80fc4504fb420df04cce99c9ee6ec ath9k_hw: clean up tx power handling

:::::: TO: Felix Fietkau <nbd@openwrt.org>
:::::: CC: John W. Linville <linville@tuxdriver.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 54113 bytes
Desc: not available
Url : http://lists.ath9k.org/pipermail/ath9k-devel/attachments/20160401/c86e4011/attachment-0001.obj 

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Zefir Kurtisi]

Tx power limitations at upper layers are interpreted in
the EIRP domain. When the user requests a given maximum
txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
he expects the EIRP to be at or below 15dBm.

In ath9k_hw_apply_txpower(), the interpretation is
different: the antenna-gain is capped against the
current txpower limit in the regulatory, but not
against the user set value. It ensures that the
resulting EIRP is below the limit defined by the
active countrycode, but not below the value the
user requested.

In a scenario like e.g.
 a) antenna_gain=6
 b) countrycode limits to eirp=18
 c) user set txpower=15
this will cause a setting for AR_PHY_POWER_TX_RATE
regs resulting in an EIRP > 15.

This patch ensures that antenna-gain is considered
whenever the txpower limit is adjusted and with that
the user set limits are kept.

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
v2: remove unused variable max_gain as reported by kbuild test robot
---
 drivers/net/wireless/ath/ath9k/hw.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/hw.c b/drivers/net/wireless/ath/ath9k/hw.c
index e7a3101..9030574 100644
--- a/drivers/net/wireless/ath/ath9k/hw.c
+++ b/drivers/net/wireless/ath/ath9k/hw.c
@@ -2833,8 +2833,7 @@ void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan,
 {
 	struct ath_regulatory *reg = ath9k_hw_regulatory(ah);
 	struct ieee80211_channel *channel;
-	int chan_pwr, new_pwr, max_gain;
-	int ant_gain, ant_reduction = 0;
+	int chan_pwr, new_pwr;
 
 	if (!chan)
 		return;
@@ -2842,15 +2841,10 @@ void ath9k_hw_apply_txpower(struct ath_hw *ah, struct ath9k_channel *chan,
 	channel = chan->chan;
 	chan_pwr = min_t(int, channel->max_power * 2, MAX_RATE_POWER);
 	new_pwr = min_t(int, chan_pwr, reg->power_limit);
-	max_gain = chan_pwr - new_pwr + channel->max_antenna_gain * 2;
-
-	ant_gain = get_antenna_gain(ah, chan);
-	if (ant_gain > max_gain)
-		ant_reduction = ant_gain - max_gain;
 
 	ah->eep_ops->set_txpower(ah, chan,
 				 ath9k_regd_get_ctl(reg, chan),
-				 ant_reduction, new_pwr, test);
+				 get_antenna_gain(ah, chan), new_pwr, test);
 }
 
 void ath9k_hw_set_txpowerlimit(struct ath_hw *ah, u32 limit, bool test)
-- 
2.7.4

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Kalle Valo]

Zefir Kurtisi <zefir.kurtisi@neratec.com> writes:

> Tx power limitations at upper layers are interpreted in
> the EIRP domain. When the user requests a given maximum
> txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
> he expects the EIRP to be at or below 15dBm.
>
> In ath9k_hw_apply_txpower(), the interpretation is
> different: the antenna-gain is capped against the
> current txpower limit in the regulatory, but not
> against the user set value. It ensures that the
> resulting EIRP is below the limit defined by the
> active countrycode, but not below the value the
> user requested.
>
> In a scenario like e.g.
>  a) antenna_gain=6
>  b) countrycode limits to eirp=18
>  c) user set txpower=15
> this will cause a setting for AR_PHY_POWER_TX_RATE
> regs resulting in an EIRP > 15.
>
> This patch ensures that antenna-gain is considered
> whenever the txpower limit is adjusted and with that
> the user set limits are kept.
>
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>

Applied, thanks.

-- 
Kalle Valo

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Felix Fietkau]

On 2016-04-01 11:37, Zefir Kurtisi wrote:
> Tx power limitations at upper layers are interpreted in
> the EIRP domain. When the user requests a given maximum
> txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
> he expects the EIRP to be at or below 15dBm.
> 
> In ath9k_hw_apply_txpower(), the interpretation is
> different: the antenna-gain is capped against the
> current txpower limit in the regulatory, but not
> against the user set value. It ensures that the
> resulting EIRP is below the limit defined by the
> active countrycode, but not below the value the
> user requested.
> 
> In a scenario like e.g.
>  a) antenna_gain=6
>  b) countrycode limits to eirp=18
>  c) user set txpower=15
> this will cause a setting for AR_PHY_POWER_TX_RATE
> regs resulting in an EIRP > 15.
> 
> This patch ensures that antenna-gain is considered
> whenever the txpower limit is adjusted and with that
> the user set limits are kept.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
I just noticed this change and I believe it should be reverted. In many
cases the EEPROM antenna gain value does not accurately reflect the real
antenna gain and is used more as a worst case value to prevent exceeding
regulatory limits.

I believe using this to limit the user specified tx power values will
not only make this inconsistent with other drivers, but it will also
confuse users by using significantly lower tx power than they wanted.

The EEPROM antenna gain value is already causing more tx power reduction
than necessary, because AFAIK at least the FCC regulatory rules allow an
antenna gain of 3 dB while at the power limit, yet this is not
subtracted from the EEPROM antenna gain value when considering the limit.

- Felix

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Zefir Kurtisi]

On 05/14/2016 02:50 PM, Felix Fietkau wrote:
> On 2016-04-01 11:37, Zefir Kurtisi wrote:
>> Tx power limitations at upper layers are interpreted in
>> the EIRP domain. When the user requests a given maximum
>> txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
>> he expects the EIRP to be at or below 15dBm.
>>
>> In ath9k_hw_apply_txpower(), the interpretation is
>> different: the antenna-gain is capped against the
>> current txpower limit in the regulatory, but not
>> against the user set value. It ensures that the
>> resulting EIRP is below the limit defined by the
>> active countrycode, but not below the value the
>> user requested.
>>
>> In a scenario like e.g.
>>  a) antenna_gain=6
>>  b) countrycode limits to eirp=18
>>  c) user set txpower=15
>> this will cause a setting for AR_PHY_POWER_TX_RATE
>> regs resulting in an EIRP > 15.
>>
>> This patch ensures that antenna-gain is considered
>> whenever the txpower limit is adjusted and with that
>> the user set limits are kept.
>>
>> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> I just noticed this change and I believe it should be reverted. In many
> cases the EEPROM antenna gain value does not accurately reflect the real
> antenna gain and is used more as a worst case value to prevent exceeding
> regulatory limits.
> 
> I believe using this to limit the user specified tx power values will
> not only make this inconsistent with other drivers, but it will also
> confuse users by using significantly lower tx power than they wanted.
> 
> The EEPROM antenna gain value is already causing more tx power reduction
> than necessary, because AFAIK at least the FCC regulatory rules allow an
> antenna gain of 3 dB while at the power limit, yet this is not
> subtracted from the EEPROM antenna gain value when considering the limit.
> 
> - Felix
> 
Two things to be considered before reverting that commit:
1) the change affects only setups where a valid antenna gain value is set
I checked some consumer ath9k cards I collected over time from (rather old) WiFi
routers - none of them have an antenna gain set in EEPROM. Therefore, none of them
would be affected by the change (alas I have no current ath9k NICs at hand to
check if this is still valid). I'd argue that valid antenna gain values are set by
manufacturers or professional integrators who took time to calibrate and measure
the antennas' characteristics accurately.

2) EIRP is what matters
As explained in the commit, every layer above the driver is interpreting txpower
in the EIRP domain. When you visit a certification lab and the engineer sets a max
txpower of 15dBm but measures an EIRP of 18 (as in the example above), the device
won't pass the test.


I think the latter point is a strong argument to leave the change intact.



Cheers,
Zefir

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Felix Fietkau]

On 2016-05-17 12:54, Zefir Kurtisi wrote:
> On 05/14/2016 02:50 PM, Felix Fietkau wrote:
>> On 2016-04-01 11:37, Zefir Kurtisi wrote:
>>> Tx power limitations at upper layers are interpreted in
>>> the EIRP domain. When the user requests a given maximum
>>> txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
>>> he expects the EIRP to be at or below 15dBm.
>>>
>>> In ath9k_hw_apply_txpower(), the interpretation is
>>> different: the antenna-gain is capped against the
>>> current txpower limit in the regulatory, but not
>>> against the user set value. It ensures that the
>>> resulting EIRP is below the limit defined by the
>>> active countrycode, but not below the value the
>>> user requested.
>>>
>>> In a scenario like e.g.
>>>  a) antenna_gain=6
>>>  b) countrycode limits to eirp=18
>>>  c) user set txpower=15
>>> this will cause a setting for AR_PHY_POWER_TX_RATE
>>> regs resulting in an EIRP > 15.
>>>
>>> This patch ensures that antenna-gain is considered
>>> whenever the txpower limit is adjusted and with that
>>> the user set limits are kept.
>>>
>>> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
>> I just noticed this change and I believe it should be reverted. In many
>> cases the EEPROM antenna gain value does not accurately reflect the real
>> antenna gain and is used more as a worst case value to prevent exceeding
>> regulatory limits.
>> 
>> I believe using this to limit the user specified tx power values will
>> not only make this inconsistent with other drivers, but it will also
>> confuse users by using significantly lower tx power than they wanted.
>> 
>> The EEPROM antenna gain value is already causing more tx power reduction
>> than necessary, because AFAIK at least the FCC regulatory rules allow an
>> antenna gain of 3 dB while at the power limit, yet this is not
>> subtracted from the EEPROM antenna gain value when considering the limit.
>> 
>> - Felix
>> 
> Two things to be considered before reverting that commit:
> 1) the change affects only setups where a valid antenna gain value is set
> I checked some consumer ath9k cards I collected over time from (rather old) WiFi
> routers - none of them have an antenna gain set in EEPROM. Therefore, none of them
> would be affected by the change (alas I have no current ath9k NICs at hand to
> check if this is still valid). I'd argue that valid antenna gain values are set by
> manufacturers or professional integrators who took time to calibrate and measure
> the antennas' characteristics accurately.
Maybe not many NICs have antenna gain set, but I did see several
embedded devices with antenna gain set. Sometimes the value was set to
just 6 db, sometimes it was more than that.

> 2) EIRP is what matters
> As explained in the commit, every layer above the driver is interpreting txpower
> in the EIRP domain. When you visit a certification lab and the engineer sets a max
> txpower of 15dBm but measures an EIRP of 18 (as in the example above), the device
> won't pass the test.
> 
> I think the latter point is a strong argument to leave the change intact.
I completely agree that this is something that needs fixing, but I think
this change is not the way to do it. On devices with detachable antennas
and a programmed antenna gain value in EEPROM this will make it really
hard for the user to set the right tx power, especially when using
different antennas. I think this is a serious regression.

A much better approach would be to expose the antenna gain to the stack,
filling in the default from the EEPROM. That way the user at least has a
chance to figure out what's going on if the tx power is too low and can
set the antenna gain value to something sane as opposed to whatever
potentially bogus value is in the EEPROM.

- Felix

[ath9k-devel] [PATCH v2] ath9k: interpret requested txpower in EIRP domain

[Zefir Kurtisi]

On 05/17/2016 01:11 PM, Felix Fietkau wrote:
> On 2016-05-17 12:54, Zefir Kurtisi wrote:
>> On 05/14/2016 02:50 PM, Felix Fietkau wrote:
>>> On 2016-04-01 11:37, Zefir Kurtisi wrote:
>>>> Tx power limitations at upper layers are interpreted in
>>>> the EIRP domain. When the user requests a given maximum
>>>> txpower, e.g. with: 'iw phy0 set txpower fixed 1500',
>>>> he expects the EIRP to be at or below 15dBm.
>>>>
>>>> In ath9k_hw_apply_txpower(), the interpretation is
>>>> different: the antenna-gain is capped against the
>>>> current txpower limit in the regulatory, but not
>>>> against the user set value. It ensures that the
>>>> resulting EIRP is below the limit defined by the
>>>> active countrycode, but not below the value the
>>>> user requested.
>>>>
>>>> In a scenario like e.g.
>>>>  a) antenna_gain=6
>>>>  b) countrycode limits to eirp=18
>>>>  c) user set txpower=15
>>>> this will cause a setting for AR_PHY_POWER_TX_RATE
>>>> regs resulting in an EIRP > 15.
>>>>
>>>> This patch ensures that antenna-gain is considered
>>>> whenever the txpower limit is adjusted and with that
>>>> the user set limits are kept.
>>>>
>>>> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
>>> I just noticed this change and I believe it should be reverted. In many
>>> cases the EEPROM antenna gain value does not accurately reflect the real
>>> antenna gain and is used more as a worst case value to prevent exceeding
>>> regulatory limits.
>>>
>>> I believe using this to limit the user specified tx power values will
>>> not only make this inconsistent with other drivers, but it will also
>>> confuse users by using significantly lower tx power than they wanted.
>>>
>>> The EEPROM antenna gain value is already causing more tx power reduction
>>> than necessary, because AFAIK at least the FCC regulatory rules allow an
>>> antenna gain of 3 dB while at the power limit, yet this is not
>>> subtracted from the EEPROM antenna gain value when considering the limit.
>>>
>>> - Felix
>>>
>> Two things to be considered before reverting that commit:
>> 1) the change affects only setups where a valid antenna gain value is set
>> I checked some consumer ath9k cards I collected over time from (rather old) WiFi
>> routers - none of them have an antenna gain set in EEPROM. Therefore, none of them
>> would be affected by the change (alas I have no current ath9k NICs at hand to
>> check if this is still valid). I'd argue that valid antenna gain values are set by
>> manufacturers or professional integrators who took time to calibrate and measure
>> the antennas' characteristics accurately.
> Maybe not many NICs have antenna gain set, but I did see several
> embedded devices with antenna gain set. Sometimes the value was set to
> just 6 db, sometimes it was more than that.
> 
Yes, of course this can and will happen. Alas, if it does, user is in trouble
anyway, since - even without the modification - invalid antenna gain values in the
eeprom would add unjustified limitations. If there is a 6dBi set and the
countrycode allows for 18dBm, with or without the modification the user won't be
able to set antenna port power to more than 12dBm. When typically the txpower is
set to auto, nothing changes with the patch applied.

What it in fact does is, further reducing the antenna port power if the user
*explicitly* sets a lower txpower (below what the countrycode allows).


>> 2) EIRP is what matters
>> As explained in the commit, every layer above the driver is interpreting txpower
>> in the EIRP domain. When you visit a certification lab and the engineer sets a max
>> txpower of 15dBm but measures an EIRP of 18 (as in the example above), the device
>> won't pass the test.
>>
>> I think the latter point is a strong argument to leave the change intact.
> I completely agree that this is something that needs fixing, but I think
> this change is not the way to do it. On devices with detachable antennas
> and a programmed antenna gain value in EEPROM this will make it really
> hard for the user to set the right tx power, especially when using
> different antennas. I think this is a serious regression.
> 
> A much better approach would be to expose the antenna gain to the stack,
> filling in the default from the EEPROM. That way the user at least has a
> chance to figure out what's going on if the tx power is too low and can
> set the antenna gain value to something sane as opposed to whatever
> potentially bogus value is in the EEPROM.
> 
This currently is a minefield and we need to very carefully keep track on what is
going on regulatory wise. FCC is planning to (or already does?) require WLAN
devices to be certified only in combination with the antennas to be used and
corresponding antenna gains set fix (and ETSI usually follows). The motivation is
clear: obviously, attaching a 12dBi antenna and claiming it is only 3dBi is the
easiest way to get the device operating outside certified specs.

Not wanting to wake up sleeping dogs, but given that the mechanisms to fix
manufacturers' mistakes of setting wrong eeprom antenna gain values essentially
are the same as those needed to bypass txpower limitations, 'fixing' bogus eeprom
antenna gain values long-term is not an option. We will have to respect the
manufacturer set values at lowest layer (driver or FW) anyway, therefore I don't
think it makes sense to pass them to the stack.

> - Felix
> 

Nevertheless, if you see serious risks for regressions with this change, I won't
oppose a revert and keep the patch applied privately. In that case it would make
sense to explicitly explain to the user that the txpower he is setting over the UI
/ uci, with ath9k (and others?) needs to be interpreted as antenna port power and
not EIRP.


Cheers,
Zefir