On 02/01/2011 07:31 AM, Helmut Schaa wrote:
Hi Ben,
looks as if your special case (only scanning the operating channel) would be
covered quite well with this patch. However, in cases where we scan multiple
channels consecutively (including the operating channel) the operating channel
might get scanned (if the previous scan channel was off channel and we still
got time left for another channel to scan) without reenabling the interfaces
(beaconing, waking up etc.). It wasn't like this before your patch, so that
shouldn't prevent your optimization from being merged :) but might be
considered in the future.
Yes. In particular, I think it would be nice to re-order the scan
channels so that the first channel scanned is the operating channel.
That should optimize things to do as few on/off channel operations
as possible, without adding any more complexity to the scan
state machine.
Am Montag, 31. Januar 2011 schrieb greearb@xxxxxxxxxxxxxxx:
From: Ben Greear<greearb@xxxxxxxxxxxxxxx>
This should decrease un-necessary flushes, on/off channel work,
and channel changes in cases where the only scanned channel is
the current operating channel.
* Removes SCAN_OFF_CHANNEL flag, uses SDATA_STATE_OFFCHANNEL
and is-scanning flags instead.
Nice!
* Add helper method to determine if we are currently configured
for the operating channel.
* Do no blindly go off/on channel in work.c Instead, only call
appropriate on/off code when we really need to change channels.
* Consolidate ieee80211_offchannel_stop_station and
ieee80211_offchannel_stop_beaconing, call it
ieee80211_offchannel_stop_vifs instead.
* Accept non-beacon frames when scanning, even if off-channel.
As far as I can see ieee80211_scan_rx will pass the frame up _only_
if we are on-channel. Is the comment wrong?
I believe I was talking about this particular piece of code, but
the scan_rx is going to purge any pkts that are not scan results
when not on channel, so that comment is wrong about the off-channel
bit.
+++ b/net/mac80211/rx.c
> @@ -409,16 +409,10 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
> if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN)))
> return RX_CONTINUE;
>
> - if (test_bit(SCAN_HW_SCANNING, &local->scanning))
> + if (test_bit(SCAN_HW_SCANNING, &local->scanning) ||
> + test_bit(SCAN_SW_SCANNING, &local->scanning))
> return ieee80211_scan_rx(rx->sdata, skb);
>
> - if (test_bit(SCAN_SW_SCANNING, &local->scanning)) {
> - /* drop all the other packets during a software scan anyway */
> - if (ieee80211_scan_rx(rx->sdata, skb) != RX_QUEUED)
> - dev_kfree_skb(skb);
> - return RX_QUEUED;
> - }
> -
@@ -293,15 +300,25 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
bool was_hw_scan)
{
struct ieee80211_local *local = hw_to_local(hw);
+ bool ooc;
+
+ mutex_lock(&local->mtx);
+ ooc = ieee80211_cfg_on_oper_channel(local);
+
+ if (was_hw_scan || !ooc)
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
if (!was_hw_scan) {
+ bool ooc2;
ieee80211_configure_filter(local);
drv_sw_scan_complete(local);
- ieee80211_offchannel_return(local, true);
+ ooc2 = ieee80211_cfg_on_oper_channel(local);
+ /* We should always be on-channel at this point. */
+ WARN_ON(!ooc2);
+ if (ooc2&& (ooc != ooc2))
+ ieee80211_offchannel_return(local, true);
Is this additional check necessary? Looks as if that can only happen
if the call to ieee80211_hw_config failes, right? And in that case we'll hit
other problems as well I guess.
Yeah, this code looks a bit wrong. My idea was to make absolutely sure that
we are back on the operating channel and have run the offchannel_return.
In that first bit of code that
does the hw_config(), I need to explicitly clear the scan-channel (and warn
if it is not already cleared).
/*
* What if the nullfunc frames didn't arrive?
@@ -617,15 +635,13 @@ static void ieee80211_scan_state_enter_oper_channel(struct ieee80211_local *loca
{
/* switch back to the operating channel */
local->scan_channel = NULL;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+ if (!ieee80211_cfg_on_oper_channel(local))
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
/*
- * Only re-enable station mode interface now; beaconing will be
- * re-enabled once the full scan has been completed.
+ * Re-enable vifs and beaconing.
*/
- ieee80211_offchannel_return(local, false);
-
- __clear_bit(SCAN_OFF_CHANNEL,&local->scanning);
+ ieee80211_offchannel_return(local, true);
In cases where the qos latency is configured to something small
we'll end up disabling/enabling beaconing for each scanned channel.
However, as we'll stay at least 200ms on the operating channel
it shouldn't hurt I guess.
That was my hope....
All in all at least the changes to the scan code look reasonable to me.
Thanks for the review. I'll work on the issues you brought up
and re-submit.
Thanks,
Ben
Thanks,
Helmut
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
--
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
