Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes

[Nick Kossifidis <mickflemm@gmail.com>]

2012/7/26 Felix Fietkau <nbd@openwrt.org>:
> On 2012-07-26 12:31 PM, Nick Kossifidis wrote:
>> 2012/7/26 Felix Fietkau <nbd@openwrt.org>:
>>> On 2012-07-26 12:20 PM, Nick Kossifidis wrote:
>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>>> Hi Nick,
>>>>>
>>>>> Nick Kossifidis schrieb:
>>>>>
>>>>>> 2012/7/26 Thomas Huehn <thomas@net.t-labs.tu-berlin.de>:
>>>>>
>>>>>> 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...
>>>>
>>>>> 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 ?
> Unless I'm missing something here, it does not seem to get used at reset
> time, only when mac80211 requests a txpower change (and for TPC values
> in descriptors).
> On every reset, ah->ah_txpower.txp_cur_pwr gets reused as configured tx
> power value (and subsequently clamped to the hw channel power limit).
> I guess this patch could be simplified to use ah->power_level in
> ath5k_hw_phy_init instead of ah->ah_txpower.txp_cur_pwr, but I do see an
> issue in the code as it is without this patch.
>
> - Felix
>

I think this is a better approach (I'll prepare a proper patch as soon
as I have some bandwidth to work with wireless-testing, maybe
tomorrow)...

--- old/phy.c	2012-07-26 20:40:00.869150187 +0300
+++ new/phy.c	2012-07-26 20:43:25.074710577 +0300
@@ -3562,6 +3562,12 @@
 		for (i = 8; i <= 15; i++)
 			rates[i] -= ah->ah_txpower.txp_cck_ofdm_gainf_delta;

+
+	/* Min/max in 0.25dB units */
+	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
+	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
+	ah->ah_txpower.txp_ofdm = rates[7];
+
 	/* Now that we have all rates setup use table offset to
 	 * match the power range set by user with the power indices
 	 * on PCDAC/PDADC table */
@@ -3571,11 +3577,6 @@
 		if (rates[i] > 63)
 			rates[i] = 63;
 	}
-
-	/* Min/max in 0.25dB units */
-	ah->ah_txpower.txp_min_pwr = 2 * rates[7];
-	ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
-	ah->ah_txpower.txp_ofdm = rates[7];
 }


@@ -3789,8 +3790,8 @@
 	 * RF buffer settings on 5211/5212+ so that we
 	 * properly set curve indices.
 	 */
-	ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
-			ah->ah_txpower.txp_cur_pwr / 2 : AR5K_TUNE_MAX_TXPOWER);
+	ret = ath5k_hw_txpower(ah, channel, ah->power_level ?
+			ah->power_level * 2 : AR5K_TUNE_MAX_TXPOWER);
 	if (ret)
 		return ret;

Works for you ?

BTW is there a way to pass the actual tx power set back to
mac80211/cfg80211 so that user knows what power his card is actually
transmitting at ?

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