On Wed, Aug 26, 2009 at 07:08:55AM -0700, Bob Copeland wrote:
> On Tue, Aug 25, 2009 at 7:38 PM, Luis R. Rodriguez<lrodriguez@xxxxxxxxxxx>
> > First thing I saw upon a quick review was we were relying on our own
> > curband for RX instead of the cfg80211 hw->conf band. Now granted
> > that *may* be updated properly, and it seems that may be the case,
>
> Looks good at first glance, except it conflicts with changes I
> recently posted. I'm all for getting rid of driver copies of stuff
> in the stack.
I can rebase.
> > - /* NB: setup here so ath5k_rate_update is happy */
> > - if (test_bit(AR5K_MODE_11A, ah->ah_modes))
> > - ath5k_setcurmode(sc, AR5K_MODE_11A);
> > - else
> > - ath5k_setcurmode(sc, AR5K_MODE_11B);
> > -
>
> I take it ath5k_rate_update didn't really care?
Well I nuked ath5k_setcurmode() and didn't see anything relying on that
stuff.
> > ath5k_chan_set(struct ath5k_softc *sc, struct ieee80211_channel *chan)
> > {
> > ATH5K_DBG(sc, ATH5K_DEBUG_RESET, "(%u MHz) -> (%u MHz)\n",
> > - sc->curchan->center_freq, chan->center_freq);
> > + sc->hw->conf.channel->center_freq, chan->center_freq);
>
> So, does hw->conf.channel change before we're told to change channel,
> or after?
Oh bummer, good thing you asked as I simply assumed it would be set *after*.
Turns out its done prior to the drv_config() call.
if (chan != local->hw.conf.channel ||
channel_type != local->hw.conf.channel_type) {
local->hw.conf.channel = chan;
local->hw.conf.channel_type = channel_type;
changed |= IEEE80211_CONF_CHANGE_CHANNEL;
}
...
if (changed && local->open_count) {
ret = drv_config(local, changed);
...
}
I suppose we can just remove that printk ? :)
> > - if (chan) {
> > - ath5k_hw_set_imr(ah, 0);
> > - ath5k_txq_cleanup(sc);
> > - ath5k_rx_stop(sc);
> > -
> > - sc->curchan = chan;
> > - sc->curband = &sc->sbands[chan->band];
> > - }
>
> Unless I missed something I think we still want:
>
> if (chan_change)
> ath5k_chan_set(...);
Not sure I follow. Let me explain the logic here a little better.
ath5k_reset() had a check to see if we switched channels. The check
is above, and I moved it ath5k_chan_set() called by the config callback.
Reason for this is channel change *only* occurs through the config callback
so we can be certain no other path will call reset with a channel change
request.
> > - ret = ath5k_hw_reset(ah, sc->opmode, sc->curchan, chan != NULL);
> > + ret = ath5k_hw_reset(ah, sc->opmode, chan, chan_change);
>
> There's just no synchronization of this stuff, not too surprising there
> are races.
config calls for reset seem to be with sc->lock. ath9k uses a mutex to
protect races between mac80211 callback calls, ath5k seems to use the
sc->lock *sometimes*, a good review of that may help but for channel
change this seems protected unless I missed something. Since channel
change goes through the config callback and since the callback protects
through sc->lock I can't see how we'd race against changing channels.
Luis
--
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
