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

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)...

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 ?

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

Commit Message

Comments

Patch