On Thu, 2011-01-27 at 09:17 -0800, Ben Greear wrote:
> >> + if (local->scan_channel) {
> >> + chan = local->scan_channel;
> >> + channel_type = NL80211_CHAN_NO_HT;
> >> + } else if (local->tmp_channel) {
> >> + chan = scan_chan = local->tmp_channel;
> >> + channel_type = local->tmp_channel_type;
> >> + } else {
> >> + chan = local->oper_channel;
> >> + channel_type = local->_oper_channel_type;
> >> + }
> >
> > Don't understand -- why not return true in the else branch?
>
> Because the hardware might not actually be set to the oper_channel.
> The idea is that you configure the mac80211 state as you want it, and then
> use this method to figure out if you really need to make hardware
> changes.
Oh. Wouldn't it make more sense to stick that into the _config()
function then and return something there? Hmm. I kinda start to
understand I guess.
> >> + if (chan != local->oper_channel ||
> >> + channel_type != local->_oper_channel_type)
> >> + return false;
> >> +
> >> + /* Check current hardware-config against oper_channel. */
> >> + if ((local->oper_channel != local->hw.conf.channel) ||
> >> + (local->_oper_channel_type != local->hw.conf.channel_type))
> >> + return false;
> >
> > That's confusing, and kinda racy IIRC?
>
> This method should be locked such that the hardware conf
> cannot be changed while it is being called. I can double
> check that this is true.
Not all of this is always properly locked unfortunately. Not sure about
this case though.
> >> - if (ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL))
> >> - skip = 1;
> >> +
> >> + if (chan != local->hw.conf.channel)
> >> + if (ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL))
> >> + skip = 1;
> >
> > Why doesn't that test the bit? Or does this only cause setting it?
>
> Which bit?
Err. Not sure, maybe I got confused.
> > Also, won't this do some weird things like not stop, but try to start
> > stations again?
>
> I was thinking that should be harmless. As far as I can tell, current
> code would never actually stop beaconing in this method but might try
> to start it later, so it must not cause too much trouble.
Yeah, maybe you're right and it doesn't matter, but I think it'd be
nicer to always nest the calls. I see you've done that already.
> Thanks for the review...I'll go over everything and try to repost
> something that incorporates your ideas.
Thanks for your patience with this! It's quite tricky I guess...
johannes
--
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
