2012/7/26 Felix Fietkau [off-list ref]:

On 2012-07-26 12:20 PM, Nick Kossifidis wrote:

quoted

2012/7/26 Thomas Huehn [off-list ref]:

quoted

Hi Nick,

Nick Kossifidis schrieb:

quoted

2012/7/26 Thomas Huehn [off-list ref]:

quoted

There is nothing in your patch that suggests that's related to this.
Anyway there's a simple way to fix this:

Just move this:

3575         /* Min/max in 0.25dB units */
3576         ah->ah_txpower.txp_min_pwr = 2 * rates[7];
3577         ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
3578         ah->ah_txpower.txp_ofdm = rates[7];

above the for loop and you are done.

Note rates[i] don't hold tx power values, they hold indices to the
channel powertable.

Are you agreeing that current ath5k txpower handling set from user space
is not working and we need to fix it ?

Is that a rhetorical question ? Just check out the TODOs on wireless.kernel.org.

The current code does not preserve the tx power value across resets,
thats the problem and the change I mentioned above fixes it (patch on
the way, it's just that where I am right now I don't have bw to
download wireless-testing) but other than that when we set tx power
without reseting at least it does what it's supposed to do (and the
result is the same with madwifi, using a spectrum analyzer or another
client/monitor interface you see some power levels supported or only
the max txpower supported, it's really up to the vendor, not all of
them follow Atheros's reference designs). Your patch passes 1dbm units
on a function that expects 0.5dbm units, you fix the problem with
preserving tx power but you break the tx power setting.

The change I mentioned above fixes the problem without introducing new
variables just because "Felix will use the other one", I don't
understand why you have a problem with that and why you think I don't
want this to get fixed...

quoted

Beside that, what about a channel switch and wrongly re-use txp_cur_pwr
on a channel witch other max power levels ?

That won't be a problem, when channel changes we 'll call

reset -> phy_init -> set_txpower -> (calibration) -> set rate target power

and then it's going to get limited before it's written on hw here:

max_pwr = min(max_pwr, (u16) ah->ah_txpower.txp_max_pwr) / 2;

txp_max_pwr is initialized on calibration (the max power for this
channel), then it gets limited by CTL edge information on EEPROM, then
by max_pwr and then max_pwr is limited by rate_info->target_power_X
from EEPROM to create rates[i]. We write rates[i] on hw, not
txp_cur_pwr.

I think it's a bad idea to store the user's choice of txpower in a
variable that internally gets reused to store the hw limit. Even when
the offset isn't added to it, it's still fragile.

A problem with this is that different channels have different max power
values, so if you switch to a channel with a lower power, and then
switch back (without explicitly changing txpower inbetween), don't you
then end up with less power than you configured?

This can be easily avoided by storing the user's txpower choice
separately from the actual hw limit...

- Felix

That's what ah->power_level already does, what's the point of
introducing another one. Do you think power_level is baldy
used/implemented ?


-- 
GPG ID: 0xEE878588
As you read this post global entropy rises. Have Fun ;-)
Nick