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

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

[greearb]

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>
---
:100644 100644 c47d7c0... 9508486... M	net/mac80211/ieee80211_i.h
:100644 100644 09a2744... 1e845c5... M	net/mac80211/main.c
:100644 100644 b4e5267... 0441b16... M	net/mac80211/offchannel.c
:100644 100644 f36d70f... 76b3a68... M	net/mac80211/rx.c
:100644 100644 f246d8a... b2eca7e... M	net/mac80211/scan.c
:100644 100644 36305e0... 62ffc8e... M	net/mac80211/work.c
 net/mac80211/ieee80211_i.h |   10 +++++++-
 net/mac80211/main.c        |   53 +++++++++++++++++++++++++++++++++++++++-----
 net/mac80211/offchannel.c  |   44 ++++++++----------------------------
 net/mac80211/rx.c          |   13 +++++++---
 net/mac80211/scan.c        |   43 +++++++++++++++++++++--------------
 net/mac80211/work.c        |   19 +++++++--------
 6 files changed, 110 insertions(+), 72 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index c47d7c0..9508486 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,
 };
 
 /**
@@ -1147,7 +1152,10 @@ void ieee80211_rx_bss_put(struct ieee80211_local *local,
 			  struct ieee80211_bss *bss);
 
 /* off-channel helpers */
-void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local);
+/* Returns true if hardware is currently configured for the operating channel
+ * and if the logical configured state is to be on the operating channel.
+ */
+bool ieee80211_on_oper_channel(struct ieee80211_local *local);
 void ieee80211_offchannel_stop_station(struct ieee80211_local *local);
 void ieee80211_offchannel_return(struct ieee80211_local *local,
 				 bool enable_beaconing);
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 09a2744..1e845c5 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -98,6 +98,39 @@ static void ieee80211_reconfig_filter(struct work_struct *work)
 	ieee80211_configure_filter(local);
 }
 
+/** If we are configured to be off the operating channel,
+ * or if we are already off the operating channel, return
+ * false.
+ */
+bool ieee80211_on_oper_channel(struct ieee80211_local *local)
+{
+	struct ieee80211_channel *chan, *scan_chan;
+	enum nl80211_channel_type channel_type;
+
+	/** This logic needs to match logic in ieee80211_hw_config */
+	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;
+	}
+
+	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;
+
+	return true;
+}
+
 int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
 {
 	struct ieee80211_channel *chan, *scan_chan;
@@ -110,21 +143,27 @@ int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
 
 	scan_chan = local->scan_channel;
 
+	/* If this off-channel logic ever changes,  ieee80211_on_oper_channel
+	 * may need to change as well.
+	 */
 	offchannel_flag = local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
 	if (scan_chan) {
 		chan = scan_chan;
 		channel_type = NL80211_CHAN_NO_HT;
-		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
-	} else if (local->tmp_channel &&
-		   local->oper_channel != local->tmp_channel) {
+	} else if (local->tmp_channel) {
 		chan = scan_chan = local->tmp_channel;
 		channel_type = local->tmp_channel_type;
-		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
 	} else {
 		chan = local->oper_channel;
 		channel_type = local->_oper_channel_type;
-		local->hw.conf.flags &= ~IEEE80211_CONF_OFFCHANNEL;
 	}
+
+	if (chan != local->oper_channel ||
+	    channel_type != local->_oper_channel_type)
+		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
+	else
+		local->hw.conf.flags &= ~IEEE80211_CONF_OFFCHANNEL;
+	
 	offchannel_flag ^= local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
 
 	if (offchannel_flag || chan != local->hw.conf.channel ||
@@ -183,6 +222,7 @@ int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
 	return ret;
 }
 
+
 void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 				      u32 changed)
 {
@@ -231,7 +271,8 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 
 	if (changed & BSS_CHANGED_BEACON_ENABLED) {
 		if (local->quiescing || !ieee80211_sdata_running(sdata) ||
-		    test_bit(SCAN_SW_SCANNING, &local->scanning)) {
+		    test_bit(SCAN_OFF_CHANNEL, &local->scanning) ||
+		    !ieee80211_on_oper_channel(local)) {
 			sdata->vif.bss_conf.enable_beacon = false;
 		} else {
 			/*
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index b4e5267..0441b16 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -95,57 +95,33 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
 	ieee80211_sta_reset_conn_monitor(sdata);
 }
 
-void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
+void ieee80211_offchannel_stop_station(struct ieee80211_local *local)
 {
 	struct ieee80211_sub_if_data *sdata;
 
+	/*
+	 * notify the AP about us leaving the channel and stop all STA interfaces
+	 */
 	mutex_lock(&local->iflist_mtx);
 	list_for_each_entry(sdata, &local->interfaces, list) {
 		if (!ieee80211_sdata_running(sdata))
 			continue;
 
-		/* disable beaconing */
+		/* Check to see if we should disable beaconing. */
 		if (sdata->vif.type == NL80211_IFTYPE_AP ||
 		    sdata->vif.type == NL80211_IFTYPE_ADHOC ||
 		    sdata->vif.type == NL80211_IFTYPE_MESH_POINT)
 			ieee80211_bss_info_change_notify(
 				sdata, BSS_CHANGED_BEACON_ENABLED);
 
-		/*
-		 * only handle non-STA interfaces here, STA interfaces
-		 * are handled in ieee80211_offchannel_stop_station(),
-		 * e.g., from the background scan state machine.
-		 *
-		 * In addition, do not stop monitor interface to allow it to be
-		 * used from user space controlled off-channel operations.
-		 */
-		if (sdata->vif.type != NL80211_IFTYPE_STATION &&
-		    sdata->vif.type != NL80211_IFTYPE_MONITOR) {
-			set_bit(SDATA_STATE_OFFCHANNEL, &sdata->state);
-			netif_tx_stop_all_queues(sdata->dev);
-		}
-	}
-	mutex_unlock(&local->iflist_mtx);
-}
-
-void ieee80211_offchannel_stop_station(struct ieee80211_local *local)
-{
-	struct ieee80211_sub_if_data *sdata;
-
-	/*
-	 * notify the AP about us leaving the channel and stop all STA interfaces
-	 */
-	mutex_lock(&local->iflist_mtx);
-	list_for_each_entry(sdata, &local->interfaces, list) {
-		if (!ieee80211_sdata_running(sdata))
-			continue;
-
-		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+		if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
 			set_bit(SDATA_STATE_OFFCHANNEL, &sdata->state);
 			netif_tx_stop_all_queues(sdata->dev);
-			if (sdata->u.mgd.associated)
+			if ((sdata->vif.type == NL80211_IFTYPE_STATION) &&
+			    sdata->u.mgd.associated)
 				ieee80211_offchannel_ps_enable(sdata);
 		}
+
 	}
 	mutex_unlock(&local->iflist_mtx);
 }
@@ -181,7 +157,7 @@ void ieee80211_offchannel_return(struct ieee80211_local *local,
 			netif_tx_wake_all_queues(sdata->dev);
 		}
 
-		/* re-enable beaconing */
+		/* Check to see if we should re-enable beaconing */
 		if (enable_beaconing &&
 		    (sdata->vif.type == NL80211_IFTYPE_AP ||
 		     sdata->vif.type == NL80211_IFTYPE_ADHOC ||
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index f36d70f..76b3a68 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 */
@@ -2749,7 +2754,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 		local->dot11ReceivedFragmentCount++;
 
 	if (unlikely(test_bit(SCAN_HW_SCANNING, &local->scanning) ||
-		     test_bit(SCAN_OFF_CHANNEL, &local->scanning)))
+		     test_bit(SCAN_SW_SCANNING, &local->scanning)))
 		status->rx_flags |= IEEE80211_RX_IN_SCAN;
 
 	if (ieee80211_is_mgmt(fc))
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index f246d8a..b2eca7e 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -294,11 +294,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);
@@ -398,13 +401,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);
 
@@ -544,7 +544,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
@@ -560,7 +571,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),
@@ -578,12 +588,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;
@@ -592,9 +596,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)
 {
+	/* This must be set before we do the stop_station logic. */
+	__set_bit(SCAN_OFF_CHANNEL, &local->scanning);
+
 	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?
@@ -641,8 +648,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++;
diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index 36305e0..62ffc8e 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -924,18 +924,17 @@ static void ieee80211_work_work(struct work_struct *work)
 		}
 
 		if (!started && !local->tmp_channel) {
-			/*
-			 * TODO: could optimize this by leaving the
-			 *	 station vifs in awake mode if they
-			 *	 happen to be on the same channel as
-			 *	 the requested channel
-			 */
-			ieee80211_offchannel_stop_beaconing(local);
-			ieee80211_offchannel_stop_station(local);
-
 			local->tmp_channel = wk->chan;
 			local->tmp_channel_type = wk->chan_type;
-			ieee80211_hw_config(local, 0);
+			if (!ieee80211_on_oper_channel(local)) {
+				/*
+				 * Leave the station vifs in awake mode if they
+				 * happen to be on the same channel as
+				 * the requested channel
+				 */
+				ieee80211_offchannel_stop_station(local);
+				ieee80211_hw_config(local, 0);
+			}
 			started = true;
 			wk->timeout = jiffies;
 		}
-- 
1.7.2.3

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

[Johannes Berg]

On Wed, 2011-01-26 at 12:37 -0800, greearb@candelatech.com wrote:


> +/* Returns true if hardware is currently configured for the operating channel
> + * and if the logical configured state is to be on the operating channel.
> + */
> +bool ieee80211_on_oper_channel(struct ieee80211_local *local);

Maybe use proper kernel-doc


> +/** If we are configured to be off the operating channel,
> + * or if we are already off the operating channel, return
> + * false.
> + */

But that clearly isn't kernel-doc, so no /**

> +bool ieee80211_on_oper_channel(struct ieee80211_local *local)
> +{
> +	struct ieee80211_channel *chan, *scan_chan;
> +	enum nl80211_channel_type channel_type;
> +
> +	/** This logic needs to match logic in ieee80211_hw_config */

ditto.

> +	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?

> +	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?

> +	/* If this off-channel logic ever changes,  ieee80211_on_oper_channel
> +	 * may need to change as well.
> +	 */
>  	offchannel_flag = local->hw.conf.flags & IEEE80211_CONF_OFFCHANNEL;
>  	if (scan_chan) {
>  		chan = scan_chan;
>  		channel_type = NL80211_CHAN_NO_HT;
> -		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
> -	} else if (local->tmp_channel &&
> -		   local->oper_channel != local->tmp_channel) {
> +	} else if (local->tmp_channel) {
>  		chan = scan_chan = local->tmp_channel;
>  		channel_type = local->tmp_channel_type;
> -		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;

> +
> +	if (chan != local->oper_channel ||
> +	    channel_type != local->_oper_channel_type)
> +		local->hw.conf.flags |= IEEE80211_CONF_OFFCHANNEL;
> +	else
> +		local->hw.conf.flags &= ~IEEE80211_CONF_OFFCHANNEL;
> +	$

whitespace damage ($ inserted by me)


> @@ -183,6 +222,7 @@ int ieee80211_hw_config(struct ieee80211_local *local, u32 changed)
>  	return ret;
>  }
>  
> +
>  void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
>  				      u32 changed)
>  {

pointless

> diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
> index b4e5267..0441b16 100644
> --- a/net/mac80211/offchannel.c
> +++ b/net/mac80211/offchannel.c
> @@ -95,57 +95,33 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
>  	ieee80211_sta_reset_conn_monitor(sdata);
>  }
>  
> -void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
> +void ieee80211_offchannel_stop_station(struct ieee80211_local *local)

I think you should find a new name for the combined function, like
"stop_interface".



> -		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> +		if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
>  			set_bit(SDATA_STATE_OFFCHANNEL, &sdata->state);
>  			netif_tx_stop_all_queues(sdata->dev);
> -			if (sdata->u.mgd.associated)
> +			if ((sdata->vif.type == NL80211_IFTYPE_STATION) &&
> +			    sdata->u.mgd.associated)
>  				ieee80211_offchannel_ps_enable(sdata);

what's the difference between sdata_state_offchannel and
scan_off_channel?

>  		}
> +
>  	}

spurious whitespace

>  	mutex_unlock(&local->iflist_mtx);
>  }
> @@ -181,7 +157,7 @@ void ieee80211_offchannel_return(struct ieee80211_local *local,
>  			netif_tx_wake_all_queues(sdata->dev);
>  		}
>  
> -		/* re-enable beaconing */
> +		/* Check to see if we should re-enable beaconing */
>  		if (enable_beaconing &&
>  		    (sdata->vif.type == NL80211_IFTYPE_AP ||
>  		     sdata->vif.type == NL80211_IFTYPE_ADHOC ||

How does scan_off_channel get cleared before this?

> @@ -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;

Alright -- but does the mlme.c code know not to expect beacons during an
on-channel scan?

> @@ -2749,7 +2754,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
>  		local->dot11ReceivedFragmentCount++;
>  
>  	if (unlikely(test_bit(SCAN_HW_SCANNING, &local->scanning) ||
> -		     test_bit(SCAN_OFF_CHANNEL, &local->scanning)))
> +		     test_bit(SCAN_SW_SCANNING, &local->scanning)))
>  		status->rx_flags |= IEEE80211_RX_IN_SCAN;

Not sure I understand this change?

> +++ b/net/mac80211/scan.c
> @@ -294,11 +294,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);

test_and_clear_bit ? Actually it's locked, no? So no need for atomic
ops.

> @@ -544,7 +544,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) {

what's that for?

> +		if (next_chan == local->oper_channel)
> +			local->next_scan_state = SCAN_SET_CHANNEL;

channel type?

> @@ -592,9 +596,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)
>  {
> +	/* This must be set before we do the stop_station logic. */
> +	__set_bit(SCAN_OFF_CHANNEL, &local->scanning);
> +
>  	ieee80211_offchannel_stop_station(local);
>  
> -	__set_bit(SCAN_OFF_CHANNEL, &local->scanning);
> +	__set_bit(SCAN_LEFT_OPER_CHANNEL, &local->scanning);



> @@ -641,8 +648,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;

Why doesn't that test the bit? Or does this only cause setting it?

> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
> index 36305e0..62ffc8e 100644
> --- a/net/mac80211/work.c
> +++ b/net/mac80211/work.c
> @@ -924,18 +924,17 @@ static void ieee80211_work_work(struct work_struct *work)
>  		}
>  
>  		if (!started && !local->tmp_channel) {
> -			/*
> -			 * TODO: could optimize this by leaving the
> -			 *	 station vifs in awake mode if they
> -			 *	 happen to be on the same channel as
> -			 *	 the requested channel
> -			 */
> -			ieee80211_offchannel_stop_beaconing(local);
> -			ieee80211_offchannel_stop_station(local);
> -
>  			local->tmp_channel = wk->chan;
>  			local->tmp_channel_type = wk->chan_type;
> -			ieee80211_hw_config(local, 0);
> +			if (!ieee80211_on_oper_channel(local)) {
> +				/*
> +				 * Leave the station vifs in awake mode if they
> +				 * happen to be on the same channel as
> +				 * the requested channel
> +				 */
> +				ieee80211_offchannel_stop_station(local);
> +				ieee80211_hw_config(local, 0);
> +			}

Now I think ieee80211_on_oper_channel() is a confusing name -- should it
be "_already_on_target_channel" or something?

Also, won't this do some weird things like not stop, but try to start
stations again?

johannes

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

[Ben Greear]

On 01/27/2011 05:52 AM, Johannes Berg 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.

>> +	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.

>
>> diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
>> index b4e5267..0441b16 100644
>> --- a/net/mac80211/offchannel.c
>> +++ b/net/mac80211/offchannel.c
>> @@ -95,57 +95,33 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
>>   	ieee80211_sta_reset_conn_monitor(sdata);
>>   }
>>
>> -void ieee80211_offchannel_stop_beaconing(struct ieee80211_local *local)
>> +void ieee80211_offchannel_stop_station(struct ieee80211_local *local)
>
> I think you should find a new name for the combined function, like
> "stop_interface".

Maybe stop_vifs() ?
It stops everything but Monitor interfaces...

>> -		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
>> +		if (sdata->vif.type != NL80211_IFTYPE_MONITOR) {
>>   			set_bit(SDATA_STATE_OFFCHANNEL,&sdata->state);
>>   			netif_tx_stop_all_queues(sdata->dev);
>> -			if (sdata->u.mgd.associated)
>> +			if ((sdata->vif.type == NL80211_IFTYPE_STATION)&&
>> +			    sdata->u.mgd.associated)
>>   				ieee80211_offchannel_ps_enable(sdata);
>
> what's the difference between sdata_state_offchannel and
> scan_off_channel?

No idea, will have to take a look.  Was just coping existing code...

>> -		/* re-enable beaconing */
>> +		/* Check to see if we should re-enable beaconing */
>>   		if (enable_beaconing&&
>>   		(sdata->vif.type == NL80211_IFTYPE_AP ||
>>   		     sdata->vif.type == NL80211_IFTYPE_ADHOC ||
>
> How does scan_off_channel get cleared before this?

Will check.

>
>> @@ -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;
>
> Alright -- but does the mlme.c code know not to expect beacons during an
> on-channel scan?

I think that code is just supposed to let regular packets through if
we are scanning on the operating channel?  I don't see how anything gets
less beacons?

>
>> @@ -2749,7 +2754,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
>>   		local->dot11ReceivedFragmentCount++;
>>
>>   	if (unlikely(test_bit(SCAN_HW_SCANNING,&local->scanning) ||
>> -		     test_bit(SCAN_OFF_CHANNEL,&local->scanning)))
>> +		     test_bit(SCAN_SW_SCANNING,&local->scanning)))
>>   		status->rx_flags |= IEEE80211_RX_IN_SCAN;
>
> Not sure I understand this change?

Otherwise, scan results are not gathered while we are scanning on channel.
This was the root cause of me not seeing scan results while scanning on
channel.  (There is other code that will ignore beacons if the RX_IN_SCAN
bit isn't set).


>> @@ -544,7 +544,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) {
>
> what's that for?
>
>> +		if (next_chan == local->oper_channel)
>> +			local->next_scan_state = SCAN_SET_CHANNEL;
>
> channel type?

Yes, will add checks for channel type.  That first bit is to try to
avoid changes if we're already on the right channel, but it could
be wrong.  I'll review all of that.

>
>> @@ -592,9 +596,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)
>>   {
>> +	/* This must be set before we do the stop_station logic. */
>> +	__set_bit(SCAN_OFF_CHANNEL,&local->scanning);
>> +
>>   	ieee80211_offchannel_stop_station(local);
>>
>> -	__set_bit(SCAN_OFF_CHANNEL,&local->scanning);
>> +	__set_bit(SCAN_LEFT_OPER_CHANNEL,&local->scanning);
>
>
>
>> @@ -641,8 +648,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;
>
> Why doesn't that test the bit? Or does this only cause setting it?

Which bit?

Either way, at the least I need to check for channel-type as well.

>
>> diff --git a/net/mac80211/work.c b/net/mac80211/work.c
>> index 36305e0..62ffc8e 100644
>> --- a/net/mac80211/work.c
>> +++ b/net/mac80211/work.c
>> @@ -924,18 +924,17 @@ static void ieee80211_work_work(struct work_struct *work)
>>   		}
>>
>>   		if (!started&&  !local->tmp_channel) {
>> -			/*
>> -			 * TODO: could optimize this by leaving the
>> -			 *	 station vifs in awake mode if they
>> -			 *	 happen to be on the same channel as
>> -			 *	 the requested channel
>> -			 */
>> -			ieee80211_offchannel_stop_beaconing(local);
>> -			ieee80211_offchannel_stop_station(local);
>> -
>>   			local->tmp_channel = wk->chan;
>>   			local->tmp_channel_type = wk->chan_type;
>> -			ieee80211_hw_config(local, 0);
>> +			if (!ieee80211_on_oper_channel(local)) {
>> +				/*
>> +				 * Leave the station vifs in awake mode if they
>> +				 * happen to be on the same channel as
>> +				 * the requested channel
>> +				 */
>> +				ieee80211_offchannel_stop_station(local);
>> +				ieee80211_hw_config(local, 0);
>> +			}
>
> Now I think ieee80211_on_oper_channel() is a confusing name -- should it
> be "_already_on_target_channel" or something?

That does sound better.

>
> 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.

Thanks for the review...I'll go over everything and try to repost
something that incorporates your ideas.

Thanks,
Ben

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

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

[Ben Greear]

On 01/27/2011 05:52 AM, Johannes Berg wrote:

>> @@ -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;
>
> Alright -- but does the mlme.c code know not to expect beacons during an
> on-channel scan?

I have a more basic question on this:

Should we just pass all packets on up the stack, regardless of whether we are
offchannel or not?  I think that would simplify things here,
and if/when we ever support things other than just scanning on
different off-channels, that code would just work.

Thanks,
Ben

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

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

[Johannes Berg]

On Thu, 2011-01-27 at 10:33 -0800, Ben Greear wrote:

> >> @@ -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;
> >
> > Alright -- but does the mlme.c code know not to expect beacons during an
> > on-channel scan?
> 
> I have a more basic question on this:
> 
> Should we just pass all packets on up the stack, regardless of whether we are
> offchannel or not?  I think that would simplify things here,
> and if/when we ever support things other than just scanning on
> different off-channels, that code would just work.

I'm not really exactly sure why we don't.

However -- I was thinking of something else, not data packets. While
scanning, all received beacons will be handed to the scan code. But if
the mlme code isn't told to stop looking for them, it'll still expect to
see the beacons.

johannes

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

[Johannes Berg]

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

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

[Ben Greear]

On 01/28/2011 05:24 AM, Johannes Berg wrote:
> 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.

The code is similar..seems like it could be put into a common helper
method, but I haven't thought of a clean way to do that yet.

>>>> +	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.

On that note:  It seems to me that __ieee80211_scan_completed_finish
must grab the mutex_lock(&local->mtx); early in this method, before
any calls to ieee80211_hw_config, for instance.  I believe this would
be an issue in both my patch and the existing code.  I'm adding code
to grab it early, but still release after recalc_idle() is called.
I'll test it with lockdep enabled to make sure it's at least mostly
sane.

>>> 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.

Yeah, the v5 patch was better about this.  The code is still complex,
but perhaps we'll think of something simpler down the road.

I'll post a v6 soon, with the earlier mutex grab I mention above
and the debug stuff removed.

Hopefully it's getting close!

Thanks,
Ben

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

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

[Ben Greear]

On 01/28/2011 05:20 AM, Johannes Berg wrote:
> On Thu, 2011-01-27 at 10:33 -0800, Ben Greear wrote:
>
>>>> @@ -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;
>>>
>>> Alright -- but does the mlme.c code know not to expect beacons during an
>>> on-channel scan?
>>
>> I have a more basic question on this:
>>
>> Should we just pass all packets on up the stack, regardless of whether we are
>> offchannel or not?  I think that would simplify things here,
>> and if/when we ever support things other than just scanning on
>> different off-channels, that code would just work.
>
> I'm not really exactly sure why we don't.
>
> However -- I was thinking of something else, not data packets. While
> scanning, all received beacons will be handed to the scan code. But if
> the mlme code isn't told to stop looking for them, it'll still expect to
> see the beacons.

Currently, it seems mlme timers are stopped when we start scanning,
and then started when scanning is complete.

However, I don't see any similar effort in the work_work() method
when it goes off-channel.

Should we move the timer pause & restart logic into the offchannel_stop_vifs
and offchannel_return methods?

If that seems like a good idea, I'll attempt that as a follow on patch.

I don't think my changes make this any worse, so not critical to add this
to the current patch I'm working on.

Thanks,
Ben

>
> johannes


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

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

[Johannes Berg]

> > However -- I was thinking of something else, not data packets. While
> > scanning, all received beacons will be handed to the scan code. But if
> > the mlme code isn't told to stop looking for them, it'll still expect to
> > see the beacons.
> 
> Currently, it seems mlme timers are stopped when we start scanning,
> and then started when scanning is complete.

Ok, I wasn't quite sure about that part -- whether it happened when
going off-channel or when starting scan. If it happened while going
off-channel this would've been problematic here.

> However, I don't see any similar effort in the work_work() method
> when it goes off-channel.
> 
> Should we move the timer pause & restart logic into the offchannel_stop_vifs
> and offchannel_return methods?

Well, sort of no, and sort of yes -- both will disrupt that code in a
sense, but even scanning on the same channel will disrupt it due to the
beacon diversion, unless we copy the beacons and give them to both sides
(scan and mlme)?

johannes

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

[Ben Greear]

On 01/31/2011 05:56 AM, Johannes Berg wrote:
>
>>> However -- I was thinking of something else, not data packets. While
>>> scanning, all received beacons will be handed to the scan code. But if
>>> the mlme code isn't told to stop looking for them, it'll still expect to
>>> see the beacons.
>>
>> Currently, it seems mlme timers are stopped when we start scanning,
>> and then started when scanning is complete.
>
> Ok, I wasn't quite sure about that part -- whether it happened when
> going off-channel or when starting scan. If it happened while going
> off-channel this would've been problematic here.
>
>> However, I don't see any similar effort in the work_work() method
>> when it goes off-channel.
>>
>> Should we move the timer pause&  restart logic into the offchannel_stop_vifs
>> and offchannel_return methods?
>
> Well, sort of no, and sort of yes -- both will disrupt that code in a
> sense, but even scanning on the same channel will disrupt it due to the
> beacon diversion, unless we copy the beacons and give them to both sides
> (scan and mlme)?

Seems to me both sides should get them.  I'll try to figure out how to
do that properly today.

But, if work_work() takes us off-channel often enough, wouldn't that
cause undue beacon loss and/or timed-out tx acks?  Are there any guarantees
about how often and how long work_work() can take us off-channel?

Thanks,
Ben

>
> johannes


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

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

[Johannes Berg]

On Mon, 2011-01-31 at 09:30 -0800, Ben Greear wrote:

> > Well, sort of no, and sort of yes -- both will disrupt that code in a
> > sense, but even scanning on the same channel will disrupt it due to the
> > beacon diversion, unless we copy the beacons and give them to both sides
> > (scan and mlme)?
> 
> Seems to me both sides should get them.  I'll try to figure out how to
> do that properly today.
> 
> But, if work_work() takes us off-channel often enough, wouldn't that
> cause undue beacon loss and/or timed-out tx acks?  

Not TX ACKs really -- the device shouldn't be switching inbetween, but
beacon loss yeah.

> Are there any guarantees
> about how often and how long work_work() can take us off-channel?

No, with p2p it can be quite a bit of time.

johannes