On 2012-07-26 12:31 PM, Nick Kossifidis wrote: > 2012/7/26 Felix Fietkau <nbd@xxxxxxxxxxx>: >> On 2012-07-26 12:20 PM, Nick Kossifidis wrote: >>> 2012/7/26 Thomas Huehn <thomas@xxxxxxxxxxxxxxxxxxxxxxx>: >>>> Hi Nick, >>>> >>>> Nick Kossifidis schrieb: >>>> >>>>> 2012/7/26 Thomas Huehn <thomas@xxxxxxxxxxxxxxxxxxxxxxx>: >>>> >>>>> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
