Return-path: Received: from wx-out-0506.google.com ([66.249.82.237]:5349 "EHLO wx-out-0506.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1760520AbXJLWhx (ORCPT ); Fri, 12 Oct 2007 18:37:53 -0400 Received: by wx-out-0506.google.com with SMTP id h31so1044175wxd for ; Fri, 12 Oct 2007 15:37:50 -0700 (PDT) Date: Fri, 12 Oct 2007 18:37:57 -0400 From: "Luis R. Rodriguez" To: Jiri Slaby Cc: John Linville , linux-wireless@vger.kernel.org, Nick Kossifidis Subject: Re: [PATCH 5/5] ath5k: Fix and clean mode initialization, prefer G for AR5212 Message-ID: <20071012223757.GA4994@pogo> (sfid-20071012_233806_084522_608E42FD) References: <20071012150438.GC9407@pogo> <20071012150543.GD9407@pogo> <20071012150709.GE9407@pogo> <20071012150745.GF9407@pogo> <4af2d03a0710121433r1f32aaabt4bb8d50c61ef12d5@mail.gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <4af2d03a0710121433r1f32aaabt4bb8d50c61ef12d5@mail.gmail.com> Sender: linux-wireless-owner@vger.kernel.org List-ID: On Fri, Oct 12, 2007 at 11:33:05PM +0200, Jiri Slaby wrote: > I didn't sleep too much due to travelling, maybe I'm wrong in some > cases, some comments follows. Seems it was me that didn't get the sleep ;) > On 10/12/07, Luis R. Rodriguez wrote: > > + modes[0].mode = MODE_IEEE80211G; > > + modes[1].mode = MODE_IEEE80211B; > > + modes[2].mode = MODE_IEEE80211A; > > + > > + max_r = ARRAY_SIZE(sc->rates); > > + max_c = ARRAY_SIZE(sc->channels); > > + modes[0].rates = sc->rates; I'll delete this line above. > > + > > + for (i = 0; i <= 2; i++) { > > + struct ieee80211_hw_mode *mode = &modes[i]; > > + const struct ath5k_rate_table *hw_rates; > > + > > + if (i == 0) { > > + mode->rates = sc->rates; > > + modes->channels = sc->channels; > > mode/modes, it's doesn't matter here, but it doesn't look well And put it above as modes[0].rates = sc->rates then. > > + } else { > > + struct ieee80211_hw_mode *prev_mode = &modes[i-1]; > > + int prev_num_r = prev_mode->num_rates; > > + int prev_num_c = prev_mode->num_channels; > > + mode->rates = &prev_mode->rates[prev_num_r]; > > + mode->channels = &prev_mode->channels[prev_num_c]; > > } > > + > > + hw_rates = ath5k_hw_get_rate_table(ah, mode->mode); > > + mode->num_rates = ath_copy_rates(mode->rates, hw_rates, > > + max_r); > > + mode->num_channels = ath_copy_channels(ah, mode->channels, > > + mode->mode, max_c); > > + max_r -= modes->num_rates; > > modes? but here it matters... Good catch. > > + max_c -= mode->num_channels; > > + } > > + > > + switch (ah->ah_version) { > > + case AR5K_AR5210: > > + REGISTER_MODE(MODE_IEEE80211A); > > + break; > > + case AR5K_AR5211: > > + REGISTER_MODE(MODE_IEEE80211B); > > + break; > > + case AR5K_AR5212: > > + if (!ah->ah_single_chip) > > + REGISTER_MODE(MODE_IEEE80211A); > > + REGISTER_MODE(MODE_IEEE80211G); > > And this seems totally wrong. you use 0, 1, 2 indexes above and here > you index it by mac80211 integers. That's exactly why the enum was > there (a shortcut like in intel drivers) -- to not get into it. Actually you are right, that was not what I intended though. The A, B, and G enums seemed to add confusion so I just got rid of them. What I really *intended* on doing was: #define REGISTER_MODE(m) do { \ for (i=0; i<2; i++) { \ if (modes[i].mode != m || !modes[i].num_channels) \ continue; \ ret = ieee80211_register_hwmode(hw, &modes[i]); \ if (ret) { \ printk(KERN_ERR "can't register hwmode %u\n",m); \ goto err; \ } \ } \ } while (0) Also, you forgot to catch I wasn't registering MODE_IEEE80211A for AR5211, I've added it now :). BTW with johill's patch, [PATCH v2] mac80211: fix set_channel regression the mode that is used first changes now is reversed again depending on the order in which you registered it so I've moved things around to assure we prefer keep preferring G for AR5212s and B for AR5211s. Below you'll find the new revised patch. This was tested with AR5212 and AR5211. Checkpatch was run too ;) Luis [PATCH 5/5] ath5k: Fix and clean mode initialization, prefer G for AR5212 Currently you get locked on B mode with AR5212s. This could be partly mac80211's fault with a recent regression introduced but ath5k mode initialization right now is pretty sloppy. For AR5212s we also currently start scanning in 5GHz. I've made the mode initialization on ath5k a bit clearer and only am registering G mode now instead of both B and G for AR5212s. 11Mbps is still the only stable rate but at least now we can work and test the other rates again. Note: mac80211 simple rate algo throws us to 1Mbps after assoc, this is by design. I recommend users to set rate to 11M for now after assoc. Changes-licensed-under: 3-clause-BSD Signed-off-by: Luis R. Rodriguez --- drivers/net/wireless/ath5k/base.c | 106 ++++++++++++++++++++----------------- net/mac80211/ieee80211_ioctl.c | 5 ++- 2 files changed, 61 insertions(+), 50 deletions(-) diff --git a/drivers/net/wireless/ath5k/base.c b/drivers/net/wireless/ath5k/base.c index 18ee995..8413950 100644 --- a/drivers/net/wireless/ath5k/base.c +++ b/drivers/net/wireless/ath5k/base.c @@ -1919,67 +1919,75 @@ static void ath_dump_modes(struct ieee80211_hw_mode *modes) static inline void ath_dump_modes(struct ieee80211_hw_mode *modes) {} #endif +#define REGISTER_MODE(m) do { \ + for (i = 0; i <= 2; i++) { \ + if (modes[i].mode != m || !modes[i].num_channels) \ + continue; \ + ret = ieee80211_register_hwmode(hw, &modes[i]); \ + if (ret) { \ + printk(KERN_ERR "can't register hwmode %u\n", m); \ + goto err; \ + } \ + } \ +} while (0) + static int ath_getchannels(struct ieee80211_hw *hw) { struct ath_softc *sc = hw->priv; struct ath_hw *ah = sc->ah; struct ieee80211_hw_mode *modes = sc->modes; - unsigned int i, max; + unsigned int i, max_r, max_c; int ret; - enum { - A = MODE_IEEE80211A, - B = MODE_IEEE80211G, /* this is not a typo, but workaround */ - G = MODE_IEEE80211B, /* to prefer g over b */ - T = MODE_ATHEROS_TURBO, - TG = MODE_ATHEROS_TURBOG, - }; BUILD_BUG_ON(ARRAY_SIZE(sc->modes) < 3); ah->ah_country_code = countrycode; - modes[A].mode = MODE_IEEE80211A; - modes[B].mode = MODE_IEEE80211B; - modes[G].mode = MODE_IEEE80211G; - - max = ARRAY_SIZE(sc->rates); - modes[A].rates = sc->rates; - max -= modes[A].num_rates = ath_copy_rates(modes[A].rates, - ath5k_hw_get_rate_table(ah, MODE_IEEE80211A), max); - modes[B].rates = &modes[A].rates[modes[A].num_rates]; - max -= modes[B].num_rates = ath_copy_rates(modes[B].rates, - ath5k_hw_get_rate_table(ah, MODE_IEEE80211B), max); - modes[G].rates = &modes[B].rates[modes[B].num_rates]; - max -= modes[G].num_rates = ath_copy_rates(modes[G].rates, - ath5k_hw_get_rate_table(ah, MODE_IEEE80211G), max); - - if (!max) - printk(KERN_WARNING "yet another rates found, but there is not " - "sufficient space to store them\n"); - - max = ARRAY_SIZE(sc->channels); - modes[A].channels = sc->channels; - max -= modes[A].num_channels = ath_copy_channels(ah, modes[A].channels, - MODE_IEEE80211A, max); - modes[B].channels = &modes[A].channels[modes[A].num_channels]; - max -= modes[B].num_channels = ath_copy_channels(ah, modes[B].channels, - MODE_IEEE80211B, max); - modes[G].channels = &modes[B].channels[modes[B].num_channels]; - max -= modes[G].num_channels = ath_copy_channels(ah, modes[G].channels, - MODE_IEEE80211G, max); - - if (!max) - printk(KERN_WARNING "yet another modes found, but there is not " - "sufficient space to store them\n"); - - for (i = 0; i < ARRAY_SIZE(sc->modes); i++) - if (modes[i].num_channels) { - ret = ieee80211_register_hwmode(hw, &modes[i]); - if (ret) { - printk(KERN_ERR "can't register hwmode %u\n",i); - goto err; - } + modes[0].mode = MODE_IEEE80211G; + modes[1].mode = MODE_IEEE80211B; + modes[2].mode = MODE_IEEE80211A; + + max_r = ARRAY_SIZE(sc->rates); + max_c = ARRAY_SIZE(sc->channels); + + for (i = 0; i <= 2; i++) { + struct ieee80211_hw_mode *mode = &modes[i]; + const struct ath5k_rate_table *hw_rates; + + if (i == 0) { + modes[0].rates = sc->rates; + modes->channels = sc->channels; + } else { + struct ieee80211_hw_mode *prev_mode = &modes[i-1]; + int prev_num_r = prev_mode->num_rates; + int prev_num_c = prev_mode->num_channels; + mode->rates = &prev_mode->rates[prev_num_r]; + mode->channels = &prev_mode->channels[prev_num_c]; } + + hw_rates = ath5k_hw_get_rate_table(ah, mode->mode); + mode->num_rates = ath_copy_rates(mode->rates, hw_rates, + max_r); + mode->num_channels = ath_copy_channels(ah, mode->channels, + mode->mode, max_c); + max_r -= mode->num_rates; + max_c -= mode->num_channels; + } + + switch (ah->ah_version) { + case AR5K_AR5210: + REGISTER_MODE(MODE_IEEE80211A); + break; + case AR5K_AR5211: + REGISTER_MODE(MODE_IEEE80211B); + REGISTER_MODE(MODE_IEEE80211A); + break; + case AR5K_AR5212: + REGISTER_MODE(MODE_IEEE80211G); + if (!ah->ah_single_chip) + REGISTER_MODE(MODE_IEEE80211A); + break; + } ath_dump_modes(modes); return 0;