From patchwork Thu Jul 26 17:48:23 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nick Kossifidis X-Patchwork-Id: 1243981 Return-Path: X-Original-To: patchwork-linux-wireless@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 2F7B4DFFCE for ; Thu, 26 Jul 2012 17:48:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752866Ab2GZRs0 (ORCPT ); Thu, 26 Jul 2012 13:48:26 -0400 Received: from mail-ob0-f174.google.com ([209.85.214.174]:43077 "EHLO mail-ob0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752212Ab2GZRsY (ORCPT ); Thu, 26 Jul 2012 13:48:24 -0400 Received: by obbuo13 with SMTP id uo13so2980657obb.19 for ; Thu, 26 Jul 2012 10:48:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=SHHq5y9YwtH1WzY5jvQZ1iSAuoLE76/baaF1UxvGXTY=; b=rwp5kVl2IhsGD9kbP0CHwDzDiwNocTPXW0XoeCGCDpXtsU2UBoJYD87LcslAg1PF7j ZOfxWTgcrg4iKaE3PAqP3mEL4nOrkHCEE8k9OUaK328r4lxoHto4Zcmo4x/J0NAsSUir T76kimjO6gsoSmYlhwJGxaDxsQOEqlWgQDhbZ3jHS/+aOMgoyTcEgZO8tnc7ajlqi0cO ucmA6+OUthBb7QOdCeCwadAK1TboomzckWqcoXK33NyU0Y4/zzSKw9Fd66LQSDoBP/ov 46nLSbx2LhvMEY5dzRNI0ZEu0aO5ji2rul9pHe1PtAR1Iy7d65+Itjiliy7YmWQa+3dM EGFg== MIME-Version: 1.0 Received: by 10.182.174.68 with SMTP id bq4mr42996866obc.53.1343324903896; Thu, 26 Jul 2012 10:48:23 -0700 (PDT) Received: by 10.76.94.211 with HTTP; Thu, 26 Jul 2012 10:48:23 -0700 (PDT) In-Reply-To: <50111ECF.1020706@openwrt.org> References: <1343059275-49590-1-git-send-email-thomas@net.t-labs.tu-berlin.de> <1343059275-49590-3-git-send-email-thomas@net.t-labs.tu-berlin.de> <50107C35.9050807@net.t-labs.tu-berlin.de> <501083F6.8060002@net.t-labs.tu-berlin.de> <50111BB3.9090802@openwrt.org> <50111ECF.1020706@openwrt.org> Date: Thu, 26 Jul 2012 20:48:23 +0300 Message-ID: Subject: Re: [ath5k-devel] [PATCH 2/2] ath5k: fix phy_init() to respect user txpower changes From: Nick Kossifidis To: Felix Fietkau Cc: Thomas Huehn , jirislaby@gmail.com, johannes.berg@intel.com, ath5k-devel@lists.ath5k.org, linux-wireless@vger.kernel.org, linville@tuxdriver.com Sender: linux-wireless-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org 2012/7/26 Felix Fietkau : > On 2012-07-26 12:31 PM, Nick Kossifidis wrote: >> 2012/7/26 Felix Fietkau : >>> On 2012-07-26 12:20 PM, Nick Kossifidis wrote: >>>> 2012/7/26 Thomas Huehn : >>>>> Hi Nick, >>>>> >>>>> Nick Kossifidis schrieb: >>>>> >>>>>> 2012/7/26 Thomas Huehn : >>>>> >>>>>> 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 ? --- 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;