All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] mac80211:  Optimize scans on current operating channel.
@ 2011-01-21 18:05 greearb
  2011-01-23  9:18 ` Johannes Berg
  2011-01-26 15:36 ` Johannes Berg
  0 siblings, 2 replies; 13+ messages in thread
From: greearb @ 2011-01-21 18:05 UTC (permalink / raw)
  To: linux-wireless; +Cc: Ben Greear

From: Ben Greear <greearb@candelatech.com>

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.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

v2:  Check channels instead of flag when determining if we should
  do a channel change in scan_completed_finish.

:100644 100644 c47d7c0... 59fe5e7... M	net/mac80211/ieee80211_i.h
:100644 100644 1236710... e6de0e7... M	net/mac80211/rx.c
:100644 100644 3e660db... fa0aeb9... M	net/mac80211/scan.c
 net/mac80211/ieee80211_i.h |    5 +++++
 net/mac80211/rx.c          |   11 ++++++++---
 net/mac80211/scan.c        |   41 +++++++++++++++++++++++++----------------
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c47d7c0..59fe5e7 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -660,6 +660,10 @@ struct tpt_led_trigger {
  *	that the scan completed.
  * @SCAN_ABORTED: Set for our scan work function when the driver reported
  *	a scan complete for an aborted scan.
+ * @SCAN_LEFT_OPER_CHANNEL:  Set this flag if the scan process leaves the
+ *      operating channel at any time.  If scanning ONLY the current operating
+ *      channel this flag should not be set, and this will allow fewer
+ *      offchannel changes.
  */
 enum {
 	SCAN_SW_SCANNING,
@@ -667,6 +671,7 @@ enum {
 	SCAN_OFF_CHANNEL,
 	SCAN_COMPLETED,
 	SCAN_ABORTED,
+	SCAN_LEFT_OPER_CHANNEL,
 };
 
 /**
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 1236710..e6de0e7 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -388,6 +388,7 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 	struct ieee80211_local *local = rx->local;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
 	struct sk_buff *skb = rx->skb;
+	int ret;
 
 	if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN)))
 		return RX_CONTINUE;
@@ -396,10 +397,14 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 		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)
+		ret = ieee80211_scan_rx(rx->sdata, skb);
+		/* drop all the other packets while scanning off channel */
+		if (ret != RX_QUEUED &&
+		    test_bit(SCAN_OFF_CHANNEL, &local->scanning)) {
 			dev_kfree_skb(skb);
-		return RX_QUEUED;
+			return RX_QUEUED;
+		}
+		return ret;
 	}
 
 	/* scanning finished during invoking of handlers */
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 3e660db..fa0aeb9 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -293,11 +293,14 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+	if ((local->oper_channel != local->hw.conf.channel) || was_hw_scan)
+		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+
 	if (!was_hw_scan) {
 		ieee80211_configure_filter(local);
 		drv_sw_scan_complete(local);
-		ieee80211_offchannel_return(local, true);
+		if (test_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning))
+			ieee80211_offchannel_return(local, true);
 	}
 
 	mutex_lock(&local->mtx);
@@ -397,13 +400,10 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
 
 	drv_sw_scan_start(local);
 
-	ieee80211_offchannel_stop_beaconing(local);
-
 	local->leave_oper_channel_time = 0;
 	local->next_scan_state = SCAN_DECISION;
 	local->scan_channel_idx = 0;
-
-	drv_flush(local, false);
+	__clear_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning);
 
 	ieee80211_configure_filter(local);
 
@@ -543,7 +543,18 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 	}
 	mutex_unlock(&local->iflist_mtx);
 
-	if (local->scan_channel) {
+	next_chan = local->scan_req->channels[local->scan_channel_idx];
+
+	if (local->oper_channel == local->hw.conf.channel) {
+		if (next_chan == local->oper_channel)
+			local->next_scan_state = SCAN_SET_CHANNEL;
+		else
+			/*
+			 * we're on the operating channel currently, let's
+			 * leave that channel now to scan another one
+			 */
+			local->next_scan_state = SCAN_LEAVE_OPER_CHANNEL;
+	} else {
 		/*
 		 * we're currently scanning a different channel, let's
 		 * see if we can scan another channel without interfering
@@ -559,7 +570,6 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 		 *
 		 * Otherwise switch back to the operating channel.
 		 */
-		next_chan = local->scan_req->channels[local->scan_channel_idx];
 
 		bad_latency = time_after(jiffies +
 				ieee80211_scan_get_channel_time(next_chan),
@@ -577,12 +587,6 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 			local->next_scan_state = SCAN_ENTER_OPER_CHANNEL;
 		else
 			local->next_scan_state = SCAN_SET_CHANNEL;
-	} else {
-		/*
-		 * we're on the operating channel currently, let's
-		 * leave that channel now to scan another one
-		 */
-		local->next_scan_state = SCAN_LEAVE_OPER_CHANNEL;
 	}
 
 	*next_delay = 0;
@@ -591,9 +595,12 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
 						    unsigned long *next_delay)
 {
+	ieee80211_offchannel_stop_beaconing(local);
+
 	ieee80211_offchannel_stop_station(local);
 
 	__set_bit(SCAN_OFF_CHANNEL, &local->scanning);
+	__set_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning);
 
 	/*
 	 * What if the nullfunc frames didn't arrive?
@@ -640,8 +647,10 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,
 	chan = local->scan_req->channels[local->scan_channel_idx];
 
 	local->scan_channel = chan;
-	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;
 
 	/* advance state machine to next channel/band */
 	local->scan_channel_idx++;
-- 
1.7.2.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-21 18:05 [PATCH v2] mac80211: Optimize scans on current operating channel greearb
@ 2011-01-23  9:18 ` Johannes Berg
  2011-01-23 15:49   ` Ben Greear
  2011-01-24 22:12   ` Ben Greear
  2011-01-26 15:36 ` Johannes Berg
  1 sibling, 2 replies; 13+ messages in thread
From: Johannes Berg @ 2011-01-23  9:18 UTC (permalink / raw)
  To: greearb; +Cc: linux-wireless

On Fri, 2011-01-21 at 10:05 -0800, greearb@candelatech.com wrote:

> + * @SCAN_LEFT_OPER_CHANNEL:  Set this flag if the scan process leaves the
> + *      operating channel at any time.  If scanning ONLY the current operating
> + *      channel this flag should not be set, and this will allow fewer
> + *      offchannel changes.

Why does this make sense? In the code below, you do


> -	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
> +	if ((local->oper_channel != local->hw.conf.channel) || was_hw_scan)
> +		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
> +
>  	if (!was_hw_scan) {
>  		ieee80211_configure_filter(local);
>  		drv_sw_scan_complete(local);
> -		ieee80211_offchannel_return(local, true);
> +		if (test_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning))
> +			ieee80211_offchannel_return(local, true);

so you could just as well use a local variable (the first if you add is
exactly this test).

>  static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
>  						    unsigned long *next_delay)
>  {
> +	ieee80211_offchannel_stop_beaconing(local);
> +

Won't that confuse drivers that expect not to be beaconing between
sw_scan_start and sw_scan_end?


Also, are you sure the filter configuration works correctly this way? If
you can, I'd like to look at a binary trace of the commands going to the
device with this change: "trace-cmd record -e mac80211".


johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-23  9:18 ` Johannes Berg
@ 2011-01-23 15:49   ` Ben Greear
  2011-01-24 22:12   ` Ben Greear
  1 sibling, 0 replies; 13+ messages in thread
From: Ben Greear @ 2011-01-23 15:49 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On 01/23/2011 01:18 AM, Johannes Berg wrote:
> On Fri, 2011-01-21 at 10:05 -0800, greearb@candelatech.com wrote:
>
>> + * @SCAN_LEFT_OPER_CHANNEL:  Set this flag if the scan process leaves the
>> + *      operating channel at any time.  If scanning ONLY the current operating
>> + *      channel this flag should not be set, and this will allow fewer
>> + *      offchannel changes.
>
> Why does this make sense? In the code below, you do
>
>
>> -	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
>> +	if ((local->oper_channel != local->hw.conf.channel) || was_hw_scan)
>> +		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
>> +
>>   	if (!was_hw_scan) {
>>   		ieee80211_configure_filter(local);
>>   		drv_sw_scan_complete(local);
>> -		ieee80211_offchannel_return(local, true);
>> +		if (test_bit(SCAN_LEFT_OPER_CHANNEL,&local->scanning))
>> +			ieee80211_offchannel_return(local, true);
>
> so you could just as well use a local variable (the first if you add is
> exactly this test).

I'm sorry, but I don't understand your comments.  I need some way to track
whether we ever called the offchannel-stop-beaconing code so that it can
be selectively re-enabled in the code above.  I was using a flag
instead of adding another boolean to the local struct.

The flag could be called 'SCAN_STOPPED_BEACONING' instead if that makes more
sense?

>>   static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
>>   						    unsigned long *next_delay)
>>   {
>> +	ieee80211_offchannel_stop_beaconing(local);
>> +
>
> Won't that confuse drivers that expect not to be beaconing between
> sw_scan_start and sw_scan_end?

I didn't know that any would care.  Maybe we need a phy_features set of
flags and let each driver enable this optimization as it is tested/verified?
Seems to work fine for ath9k with multiple VIFs, for what that's worth.

> Also, are you sure the filter configuration works correctly this way? If
> you can, I'd like to look at a binary trace of the commands going to the
> device with this change: "trace-cmd record -e mac80211".

I'll try to run this soon and post the results.

Thanks,
Ben


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-23  9:18 ` Johannes Berg
  2011-01-23 15:49   ` Ben Greear
@ 2011-01-24 22:12   ` Ben Greear
  2011-01-26 15:22     ` Johannes Berg
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Greear @ 2011-01-24 22:12 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On 01/23/2011 01:18 AM, Johannes Berg wrote:

> Also, are you sure the filter configuration works correctly this way? If
> you can, I'd like to look at a binary trace of the commands going to the
> device with this change: "trace-cmd record -e mac80211".

I uploaded two trace files.  One that scans the single operating
channel, and a second that scans all channels.  This is with my -v2
patch applied, using ath9k as the driver.  There are two virtual
STA interfaces configured on this system for this test.


http://www.candelatech.com/~greearb/trace-scan-all.dat.bz2
http://www.candelatech.com/~greearb/trace-scan-one.dat.bz2

Please let me know what you think.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-24 22:12   ` Ben Greear
@ 2011-01-26 15:22     ` Johannes Berg
  2011-01-26 17:28       ` Ben Greear
  0 siblings, 1 reply; 13+ messages in thread
From: Johannes Berg @ 2011-01-26 15:22 UTC (permalink / raw)
  To: Ben Greear; +Cc: linux-wireless

On Mon, 2011-01-24 at 14:12 -0800, Ben Greear wrote:
> On 01/23/2011 01:18 AM, Johannes Berg wrote:
> 
> > Also, are you sure the filter configuration works correctly this way? If
> > you can, I'd like to look at a binary trace of the commands going to the
> > device with this change: "trace-cmd record -e mac80211".
> 
> I uploaded two trace files.  One that scans the single operating
> channel, and a second that scans all channels.  This is with my -v2
> patch applied, using ath9k as the driver.  There are two virtual
> STA interfaces configured on this system for this test.
> 
> 
> http://www.candelatech.com/~greearb/trace-scan-all.dat.bz2
> http://www.candelatech.com/~greearb/trace-scan-one.dat.bz2
> 
> Please let me know what you think.

Thanks. I don't think the -one one really is just a single scan? But
anyway, looking at it now.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-21 18:05 [PATCH v2] mac80211: Optimize scans on current operating channel greearb
  2011-01-23  9:18 ` Johannes Berg
@ 2011-01-26 15:36 ` Johannes Berg
  2011-01-26 18:10   ` Ben Greear
  2011-01-26 18:39   ` Ben Greear
  1 sibling, 2 replies; 13+ messages in thread
From: Johannes Berg @ 2011-01-26 15:36 UTC (permalink / raw)
  To: greearb; +Cc: linux-wireless

On Fri, 2011-01-21 at 10:05 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> 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.
> 
> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
> 
> v2:  Check channels instead of flag when determining if we should
>   do a channel change in scan_completed_finish.

Can you look at work.c -- where we call
ieee80211_offchannel_stop_beaconing etc.

In this patch, you're moving the call to
ieee80211_offchannel_stop_beaconing next to
ieee80211_offchannel_stop_station in scan.c.

Therefore, you can combine those two into one function.

However, I guess it'd also be nice to also address the TODO in work.c
and move the "don't do so much if not really off-channel" logic into
that function? You must run into that when trying to associate on one
vif and the others get stopped etc. unnecessarily to do that.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-26 15:22     ` Johannes Berg
@ 2011-01-26 17:28       ` Ben Greear
  0 siblings, 0 replies; 13+ messages in thread
From: Ben Greear @ 2011-01-26 17:28 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On 01/26/2011 07:22 AM, Johannes Berg wrote:
> On Mon, 2011-01-24 at 14:12 -0800, Ben Greear wrote:
>> On 01/23/2011 01:18 AM, Johannes Berg wrote:
>>
>>> Also, are you sure the filter configuration works correctly this way? If
>>> you can, I'd like to look at a binary trace of the commands going to the
>>> device with this change: "trace-cmd record -e mac80211".
>>
>> I uploaded two trace files.  One that scans the single operating
>> channel, and a second that scans all channels.  This is with my -v2
>> patch applied, using ath9k as the driver.  There are two virtual
>> STA interfaces configured on this system for this test.
>>
>>
>> http://www.candelatech.com/~greearb/trace-scan-all.dat.bz2
>> http://www.candelatech.com/~greearb/trace-scan-one.dat.bz2
>>
>> Please let me know what you think.
>
> Thanks. I don't think the -one one really is just a single scan? But
> anyway, looking at it now.

No, sorry..it's probably several scans and other things..but it should
not scan more than one (operating) channel.

Also, I'm seeing problems where sometimes wpa_supplicant doesn't
see scan results when scanning a single operating channel.  I think this is
probably a bug in my mac80211 patch.  I plan to look at that in
detail today.

Thanks,
Ben

>
> johannes


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-26 15:36 ` Johannes Berg
@ 2011-01-26 18:10   ` Ben Greear
  2011-01-27 13:56     ` Johannes Berg
  2011-01-26 18:39   ` Ben Greear
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Greear @ 2011-01-26 18:10 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On 01/26/2011 07:36 AM, Johannes Berg wrote:
> On Fri, 2011-01-21 at 10:05 -0800, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> 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.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>> ---
>>
>> v2:  Check channels instead of flag when determining if we should
>>    do a channel change in scan_completed_finish.
>
> Can you look at work.c -- where we call
> ieee80211_offchannel_stop_beaconing etc.
>
> In this patch, you're moving the call to
> ieee80211_offchannel_stop_beaconing next to
> ieee80211_offchannel_stop_station in scan.c.
>
> Therefore, you can combine those two into one function.
>
> However, I guess it'd also be nice to also address the TODO in work.c
> and move the "don't do so much if not really off-channel" logic into
> that function? You must run into that when trying to associate on one
> vif and the others get stopped etc. unnecessarily to do that.

I think you are right about this, but due to the tricky nature
of this code, maybe save that for a second patch?

Thanks,
Ben

>
> johannes
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-26 15:36 ` Johannes Berg
  2011-01-26 18:10   ` Ben Greear
@ 2011-01-26 18:39   ` Ben Greear
  2011-01-27 13:55     ` Johannes Berg
  1 sibling, 1 reply; 13+ messages in thread
From: Ben Greear @ 2011-01-26 18:39 UTC (permalink / raw)
  To: Johannes Berg; +Cc: linux-wireless

On 01/26/2011 07:36 AM, Johannes Berg wrote:
> On Fri, 2011-01-21 at 10:05 -0800, greearb@candelatech.com wrote:
>> From: Ben Greear<greearb@candelatech.com>
>>
>> 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.
>>
>> Signed-off-by: Ben Greear<greearb@candelatech.com>
>> ---
>>
>> v2:  Check channels instead of flag when determining if we should
>>    do a channel change in scan_completed_finish.
>
> Can you look at work.c -- where we call
> ieee80211_offchannel_stop_beaconing etc.
>
> In this patch, you're moving the call to
> ieee80211_offchannel_stop_beaconing next to
> ieee80211_offchannel_stop_station in scan.c.

The offchannel_stop_beaconing doesn't actually change
state, it just calls

  	ieee80211_bss_info_change_notify(
				sdata, BSS_CHANGED_BEACON_ENABLED);

That method will disable beaconing if SCAN_SW_SCANNING is set,
regardless of current channel.

So, if that work.c logic is happening during a scan, we
will always disable scanning, but if we are not SW scanning,
then that offchannel_stop_beaconing won't actually do
anything useful.

My first inclination is to add another check in bss_info_change_notify
to test if we are really off-channel:

		if (local->quiescing || !ieee80211_sdata_running(sdata) ||
		    (test_bit(SCAN_SW_SCANNING, &local->scanning) &&
		     test_bit(SCAN_OFF_CHANNEL, &local->scanning)) {
			sdata->vif.bss_conf.enable_beacon = false;

And either set the SCAN_OFF_CHANNEL flag before calling offchannel_stop_beaconing,
or perhaps set that flag in offchannel_stop_beaconing.  The first might be
less risky, but that leaves the work.c logic funky in my opinion, since if
we are not actually scanning, beacons will still be enabled.

I could use some suggestions on how to proceed.  I am overly tempted to
start changing everything in sight, but that is likely to cause all sorts
of bugs, subtle and otherwise....

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-26 18:39   ` Ben Greear
@ 2011-01-27 13:55     ` Johannes Berg
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2011-01-27 13:55 UTC (permalink / raw)
  To: Ben Greear; +Cc: linux-wireless

On Wed, 2011-01-26 at 10:39 -0800, Ben Greear wrote:

> The offchannel_stop_beaconing doesn't actually change
> state, it just calls
> 
>   	ieee80211_bss_info_change_notify(
> 				sdata, BSS_CHANGED_BEACON_ENABLED);
> 
> That method will disable beaconing if SCAN_SW_SCANNING is set,
> regardless of current channel.
> 
> So, if that work.c logic is happening during a scan, we
> will always disable scanning, but if we are not SW scanning,
> then that offchannel_stop_beaconing won't actually do
> anything useful.


> My first inclination is to add another check in bss_info_change_notify
> to test if we are really off-channel:
> 
> 		if (local->quiescing || !ieee80211_sdata_running(sdata) ||
> 		    (test_bit(SCAN_SW_SCANNING, &local->scanning) &&
> 		     test_bit(SCAN_OFF_CHANNEL, &local->scanning)) {
> 			sdata->vif.bss_conf.enable_beacon = false;
> 
> And either set the SCAN_OFF_CHANNEL flag before calling offchannel_stop_beaconing,
> or perhaps set that flag in offchannel_stop_beaconing.  The first might be
> less risky, but that leaves the work.c logic funky in my opinion, since if
> we are not actually scanning, beacons will still be enabled.

Yes, I'm a bit concerned about that too -- that bit really shouldn't be
in the scan bits, and then we can set it in offchannel_stop_beaconing or
whatever the combination ends up being called.

> I could use some suggestions on how to proceed.  I am overly tempted to
> start changing everything in sight, but that is likely to cause all sorts
> of bugs, subtle and otherwise....

:)

I've long been tempted to rewrite scanning in terms of off-channel work
items ... Not really sure what to do.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-26 18:10   ` Ben Greear
@ 2011-01-27 13:56     ` Johannes Berg
  0 siblings, 0 replies; 13+ messages in thread
From: Johannes Berg @ 2011-01-27 13:56 UTC (permalink / raw)
  To: Ben Greear; +Cc: linux-wireless

On Wed, 2011-01-26 at 10:10 -0800, Ben Greear wrote:

> > However, I guess it'd also be nice to also address the TODO in work.c
> > and move the "don't do so much if not really off-channel" logic into
> > that function? You must run into that when trying to associate on one
> > vif and the others get stopped etc. unnecessarily to do that.
> 
> I think you are right about this, but due to the tricky nature
> of this code, maybe save that for a second patch?

Yeah, I think that might be good -- but maybe prepare in your current
version by not having the offchannel bit in the scan?

In fact -- I'm not even sure why you have two off-channel bits there,
but please reply to my comments on v3 rather than here.

johannes


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] mac80211:  Optimize scans on current operating channel.
  2011-01-21 18:03 greearb
@ 2011-01-21 18:06 ` Ben Greear
  0 siblings, 0 replies; 13+ messages in thread
From: Ben Greear @ 2011-01-21 18:06 UTC (permalink / raw)
  To: netdev

On 01/21/2011 10:03 AM, greearb@candelatech.com wrote:
> From: Ben Greear<greearb@candelatech.com>
>
> 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.

Sorry..meant to send this to linux-wireless....

Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] mac80211:  Optimize scans on current operating channel.
@ 2011-01-21 18:03 greearb
  2011-01-21 18:06 ` Ben Greear
  0 siblings, 1 reply; 13+ messages in thread
From: greearb @ 2011-01-21 18:03 UTC (permalink / raw)
  To: netdev; +Cc: Ben Greear

From: Ben Greear <greearb@candelatech.com>

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.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

v2:  Check channels instead of flag when determining if we should
  do a channel change in scan_completed_finish.

:100644 100644 c47d7c0... 59fe5e7... M	net/mac80211/ieee80211_i.h
:100644 100644 1236710... e6de0e7... M	net/mac80211/rx.c
:100644 100644 3e660db... fa0aeb9... M	net/mac80211/scan.c
 net/mac80211/ieee80211_i.h |    5 +++++
 net/mac80211/rx.c          |   11 ++++++++---
 net/mac80211/scan.c        |   41 +++++++++++++++++++++++++----------------
 3 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c47d7c0..59fe5e7 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -660,6 +660,10 @@ struct tpt_led_trigger {
  *	that the scan completed.
  * @SCAN_ABORTED: Set for our scan work function when the driver reported
  *	a scan complete for an aborted scan.
+ * @SCAN_LEFT_OPER_CHANNEL:  Set this flag if the scan process leaves the
+ *      operating channel at any time.  If scanning ONLY the current operating
+ *      channel this flag should not be set, and this will allow fewer
+ *      offchannel changes.
  */
 enum {
 	SCAN_SW_SCANNING,
@@ -667,6 +671,7 @@ enum {
 	SCAN_OFF_CHANNEL,
 	SCAN_COMPLETED,
 	SCAN_ABORTED,
+	SCAN_LEFT_OPER_CHANNEL,
 };
 
 /**
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 1236710..e6de0e7 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -388,6 +388,7 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 	struct ieee80211_local *local = rx->local;
 	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
 	struct sk_buff *skb = rx->skb;
+	int ret;
 
 	if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN)))
 		return RX_CONTINUE;
@@ -396,10 +397,14 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 		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)
+		ret = ieee80211_scan_rx(rx->sdata, skb);
+		/* drop all the other packets while scanning off channel */
+		if (ret != RX_QUEUED &&
+		    test_bit(SCAN_OFF_CHANNEL, &local->scanning)) {
 			dev_kfree_skb(skb);
-		return RX_QUEUED;
+			return RX_QUEUED;
+		}
+		return ret;
 	}
 
 	/* scanning finished during invoking of handlers */
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 3e660db..fa0aeb9 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -293,11 +293,14 @@ static void __ieee80211_scan_completed_finish(struct ieee80211_hw *hw,
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 
-	ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+	if ((local->oper_channel != local->hw.conf.channel) || was_hw_scan)
+		ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
+
 	if (!was_hw_scan) {
 		ieee80211_configure_filter(local);
 		drv_sw_scan_complete(local);
-		ieee80211_offchannel_return(local, true);
+		if (test_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning))
+			ieee80211_offchannel_return(local, true);
 	}
 
 	mutex_lock(&local->mtx);
@@ -397,13 +400,10 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
 
 	drv_sw_scan_start(local);
 
-	ieee80211_offchannel_stop_beaconing(local);
-
 	local->leave_oper_channel_time = 0;
 	local->next_scan_state = SCAN_DECISION;
 	local->scan_channel_idx = 0;
-
-	drv_flush(local, false);
+	__clear_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning);
 
 	ieee80211_configure_filter(local);
 
@@ -543,7 +543,18 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 	}
 	mutex_unlock(&local->iflist_mtx);
 
-	if (local->scan_channel) {
+	next_chan = local->scan_req->channels[local->scan_channel_idx];
+
+	if (local->oper_channel == local->hw.conf.channel) {
+		if (next_chan == local->oper_channel)
+			local->next_scan_state = SCAN_SET_CHANNEL;
+		else
+			/*
+			 * we're on the operating channel currently, let's
+			 * leave that channel now to scan another one
+			 */
+			local->next_scan_state = SCAN_LEAVE_OPER_CHANNEL;
+	} else {
 		/*
 		 * we're currently scanning a different channel, let's
 		 * see if we can scan another channel without interfering
@@ -559,7 +570,6 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 		 *
 		 * Otherwise switch back to the operating channel.
 		 */
-		next_chan = local->scan_req->channels[local->scan_channel_idx];
 
 		bad_latency = time_after(jiffies +
 				ieee80211_scan_get_channel_time(next_chan),
@@ -577,12 +587,6 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 			local->next_scan_state = SCAN_ENTER_OPER_CHANNEL;
 		else
 			local->next_scan_state = SCAN_SET_CHANNEL;
-	} else {
-		/*
-		 * we're on the operating channel currently, let's
-		 * leave that channel now to scan another one
-		 */
-		local->next_scan_state = SCAN_LEAVE_OPER_CHANNEL;
 	}
 
 	*next_delay = 0;
@@ -591,9 +595,12 @@ static void ieee80211_scan_state_decision(struct ieee80211_local *local,
 static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
 						    unsigned long *next_delay)
 {
+	ieee80211_offchannel_stop_beaconing(local);
+
 	ieee80211_offchannel_stop_station(local);
 
 	__set_bit(SCAN_OFF_CHANNEL, &local->scanning);
+	__set_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning);
 
 	/*
 	 * What if the nullfunc frames didn't arrive?
@@ -640,8 +647,10 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,
 	chan = local->scan_req->channels[local->scan_channel_idx];
 
 	local->scan_channel = chan;
-	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;
 
 	/* advance state machine to next channel/band */
 	local->scan_channel_idx++;
-- 
1.7.2.3


^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2011-01-27 13:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-01-21 18:05 [PATCH v2] mac80211: Optimize scans on current operating channel greearb
2011-01-23  9:18 ` Johannes Berg
2011-01-23 15:49   ` Ben Greear
2011-01-24 22:12   ` Ben Greear
2011-01-26 15:22     ` Johannes Berg
2011-01-26 17:28       ` Ben Greear
2011-01-26 15:36 ` Johannes Berg
2011-01-26 18:10   ` Ben Greear
2011-01-27 13:56     ` Johannes Berg
2011-01-26 18:39   ` Ben Greear
2011-01-27 13:55     ` Johannes Berg
  -- strict thread matches above, loose matches on Subject: below --
2011-01-21 18:03 greearb
2011-01-21 18:06 ` Ben Greear

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.