2012/7/23 Thomas Huehn <thomas@xxxxxxxxxxxxxxxxxxxxxxx>:
> In such cases where phy_init() function got called, tx_power is always
> set to ah->ah_txpower.txp_cur_pwr which is never updated with txpower
> specified by the user instead it equale max chan power and got
> potentially incremented by ah_txpower.txp_offset.
Never updated ?
It gets updated when setting the rate powertable (the final step of
the whole process):
ath5k_setup_rate_powertable:
3577 ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
> In any case the card was switching to a txpower level higher that
> someone specified to use. This patch fix this by introducing
> ah_txpower.txp_user_pwr which holds the tx_power specified at userland.
> Function ath5k_hw_txpower is restructured and uses the value at
> directly.
>
> Signed-off-by: Thomas Huehn <thomas@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> restructure of ath5k_hw_txpower as suggested by Felix Fietkau.
> ---
> drivers/net/wireless/ath/ath5k/ath5k.h | 1 +
> drivers/net/wireless/ath/ath5k/base.c | 3 +++
> drivers/net/wireless/ath/ath5k/phy.c | 27 ++++++++++++++-------------
> 3 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath5k/ath5k.h b/drivers/net/wireless/ath/ath5k/ath5k.h
> index 64a453a..89d9ac54 100644
> --- a/drivers/net/wireless/ath/ath5k/ath5k.h
> +++ b/drivers/net/wireless/ath/ath5k/ath5k.h
> @@ -1418,6 +1418,7 @@ struct ath5k_hw {
> s16 txp_min_pwr;
> s16 txp_max_pwr;
> s16 txp_cur_pwr;
> + s16 txp_user_pwr;
> /* Values in 0.5dB units */
> s16 txp_offset;
> s16 txp_ofdm;
We already have
1334 int power_level; /* Requested tx
power in dBm */
and
s16 txp_cur_pwr;
What's the reason to use another one ? If you want a different
type/name just change power_pevel.
We use power_level on ath5k_config when someone updates txpower:
209 if ((changed & IEEE80211_CONF_CHANGE_POWER) &&
210 (ah->power_level != conf->power_level)) {
211 ah->power_level = conf->power_level;
212
213 /* Half dB steps */
214 ath5k_hw_set_txpower_limit(ah, (conf->power_level * 2));
215 }
And on each descriptor (it's ignored for now by hardware since we
haven't enabled TPC):
723 ret = ah->ah_setup_tx_desc(ah, ds, pktlen,
724 ieee80211_get_hdrlen_from_skb(skb), padsize,
725 get_hw_packet_type(skb),
726 (ah->power_level * 2),
727 hw_rate,
728 info->control.rates[0].count, keyidx, ah->ah_tx_ant, flags,
729 cts_rate, duration);
We use txp_cur_pwr when setting txpower level on ath5k_setup_rate_powertable
3577 ah->ah_txpower.txp_cur_pwr = 2 * rates[0];
as I mentioned above, and in ath5k_hw_phy_init to preserve this
setting across resets
3792 ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
3793 ah->ah_txpower.txp_cur_pwr / 2 :
AR5K_TUNE_MAX_TXPOWER);
To give you an idea why we already have 2 places to keep tx power
setting, base.c/mac80211-ops.c should use power_level thats in 1db
steps (and makes sense to someone reading the driver part and doesn't
want to understand why we use 0.5 and 0.25db steps on the hw
functions), and ah->ah_txpower.txp_cur_pwr on phy.c functions
internally.
I find this to be clean and simple and I don't see any problem with
it, if I did some cleanup that would be maybe to change power_level to
ah_tx_power_level and from int to s16 to match the rest but other than
that I think it's O.K. the way it is..
> diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
> index 8c4c040..e831f69 100644
> --- a/drivers/net/wireless/ath/ath5k/base.c
> +++ b/drivers/net/wireless/ath/ath5k/base.c
> @@ -2951,6 +2951,9 @@ ath5k_init(struct ieee80211_hw *hw)
> hw->queues = 1;
> }
>
> + /* init tx_power setting to maximum */
> + ah->ah_txpower.txp_user_pwr = AR5K_TUNE_MAX_TXPOWER;
> +
> tasklet_init(&ah->rxtq, ath5k_tasklet_rx, (unsigned long)ah);
> tasklet_init(&ah->txtq, ath5k_tasklet_tx, (unsigned long)ah);
> tasklet_init(&ah->beacontq, ath5k_tasklet_beacon, (unsigned long)ah);
No need to do that, if txpower is not initialized we use max power
anyway on phy_init
3792 ret = ath5k_hw_txpower(ah, channel, ah->ah_txpower.txp_cur_pwr ?
3793 ah->ah_txpower.txp_cur_pwr / 2 :
AR5K_TUNE_MAX_TXPOWER);
> diff --git a/drivers/net/wireless/ath/ath5k/phy.c b/drivers/net/wireless/ath/ath5k/phy.c
> index 8b71a2d..728ff07 100644
> --- a/drivers/net/wireless/ath/ath5k/phy.c
> +++ b/drivers/net/wireless/ath/ath5k/phy.c
> @@ -3583,14 +3583,12 @@ ath5k_setup_rate_powertable(struct ath5k_hw *ah, u16 max_pwr,
> * ath5k_hw_txpower() - Set transmission power limit for a given channel
> * @ah: The &struct ath5k_hw
> * @channel: The &struct ieee80211_channel
> - * @txpower: Requested tx power in 0.5dB steps
> *
> * Combines all of the above to set the requested tx power limit
> - * on hw.
> + * on hw to ah->ah_txpower.txp_user_pwr.
> */
> static int
> -ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> - u8 txpower)
> +ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel)
> {
> struct ath5k_rate_pcal_info rate_info;
> struct ieee80211_channel *curr_channel = ah->ah_current_channel;
> @@ -3598,11 +3596,6 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> u8 type;
> int ret;
>
> - if (txpower > AR5K_TUNE_MAX_TXPOWER) {
> - ATH5K_ERR(ah, "invalid tx power: %u\n", txpower);
> - return -EINVAL;
> - }
> -
> ee_mode = ath5k_eeprom_mode_from_channel(channel);
> if (ee_mode < 0) {
> ATH5K_ERR(ah,
> @@ -3667,7 +3660,7 @@ ath5k_hw_txpower(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> ath5k_get_rate_pcal_data(ah, channel, &rate_info);
>
> /* Setup rate power table */
> - ath5k_setup_rate_powertable(ah, txpower, &rate_info, ee_mode);
> + ath5k_setup_rate_powertable(ah, ah->ah_txpower.txp_user_pwr, &rate_info, ee_mode);
>
txpower here comes from above in 0.5db steps as the kerneldoc says,
that's what ath5k_setup_rate_powertable expects, by passing
ah->ah_txpower.txp_user_pwr directly to ath5k_setup_rate_powertable,
it would result getting half the power requested because hw expects
this to be an index to the channel powertable (check the
implementation of ath5k_setup_rate_powertable) that uses 0.5db steps.
> /* Write rate power table on hw */
> ath5k_hw_reg_write(ah, AR5K_TXPOWER_OFDM(3, 24) |
> @@ -3717,8 +3710,16 @@ ath5k_hw_set_txpower_limit(struct ath5k_hw *ah, u8 txpower)
> {
> ATH5K_DBG(ah, ATH5K_DEBUG_TXPOWER,
> "changing txpower to %d\n", txpower);
> + if (txpower) {
> + ah->ah_txpower.txp_user_pwr = txpower;
> +
> + if (ah->ah_txpower.txp_user_pwr > AR5K_TUNE_MAX_TXPOWER) {
> + ATH5K_ERR(ah, "invalid tx power: %u\n", ah->ah_txpower.txp_user_pwr);
> + return -EINVAL;
> + }
> + }
>
Again this would result getting half the power requested, you should
*2 this if you want it to be used directly by
ath5k_setup_rate_powertable later.
> - return ath5k_hw_txpower(ah, ah->ah_current_channel, txpower);
> + return ath5k_hw_txpower(ah, ah->ah_current_channel);
> }
>
>
> @@ -3789,8 +3790,8 @@ ath5k_hw_phy_init(struct ath5k_hw *ah, struct ieee80211_channel *channel,
> * 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);
> +
> if (ret)
> return ret;
>
> --
> 1.7.11.1
>
To summarize IMHO this patch doesn't fix anything and breaks current
txpower setting so from my point of view it's a NACK.
--
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
