Re: [RFC v3] mac80211: Optimize scans on current operating channel.

[Johannes Berg <johannes@sipsolutions.net>]

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