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 ? -- GPG ID: 0xEE878588 As you read this post global entropy rises. Have Fun ;-) Nick -- 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
