[RFC/RFT 0/5] mac80211: implement background scan

[RFC/RFT 0/5] mac80211: implement background scan

[Helmut Schaa]

Hi,

this patch series implements basic background scanning in mac80211
by interrupting the scan after each scanned channel to allow RX/TX.

I only tested the patches on current wireless-testing with iwlagn
in sw-scan mode and it works fine already. One thing I noticed but
seems to be unrelated is: in sw-scan mode the connection sometimes
stalls and no data is transferred anymore although the connection is
still available and I found no way to recover it without setting
it up again.

So, it would be great if somebody could test the patches on other
hardware as well.

Thanks a lot,
Helmut

[RFC/RFT 1/5] mac80211: refactor the scan code

[Helmut Schaa]

Move the processing of each scan state into its own functions for better
readability. This patch does not introduce functional changes.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

---
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 7482065..a20f9e7 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -474,13 +474,85 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	return rc;
 }
 
+static int ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsigned long *next_delay)
+{
+	int skip;
+	struct ieee80211_channel *chan;
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+
+	/* if no more bands/channels left, complete scan */
+	if (local->scan_channel_idx >= local->scan_req->n_channels) {
+		ieee80211_scan_completed(&local->hw, false);
+		return 1;
+	}
+	skip = 0;
+	chan = local->scan_req->channels[local->scan_channel_idx];
+
+	if (chan->flags & IEEE80211_CHAN_DISABLED ||
+	    (sdata->vif.type == NL80211_IFTYPE_ADHOC &&
+	     chan->flags & IEEE80211_CHAN_NO_IBSS))
+		skip = 1;
+
+	if (!skip) {
+		local->scan_channel = chan;
+		if (ieee80211_hw_config(local,
+					IEEE80211_CONF_CHANGE_CHANNEL))
+			skip = 1;
+	}
+
+	/* advance state machine to next channel/band */
+	local->scan_channel_idx++;
+
+	if (skip)
+		return 0;
+
+	/*
+	 * Probe delay is used to update the NAV, cf. 11.1.3.2.2
+	 * (which unfortunately doesn't say _why_ step a) is done,
+	 * but it waits for the probe delay or until a frame is
+	 * received - and the received frame would update the NAV).
+	 * For now, we do not support waiting until a frame is
+	 * received.
+	 *
+	 * In any case, it is not necessary for a passive scan.
+	 */
+	if (chan->flags & IEEE80211_CHAN_PASSIVE_SCAN ||
+	    !local->scan_req->n_ssids) {
+		*next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
+		return 0;
+	}
+
+	*next_delay = IEEE80211_PROBE_DELAY;
+	local->scan_state = SCAN_SEND_PROBE;
+
+	return 0;
+}
+
+static void ieee80211_scan_state_send_probe(struct ieee80211_local *local, unsigned long *next_delay)
+{
+	int i;
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+
+	for (i = 0; i < local->scan_req->n_ssids; i++)
+		ieee80211_send_probe_req(
+			sdata, NULL,
+			local->scan_req->ssids[i].ssid,
+			local->scan_req->ssids[i].ssid_len,
+			local->scan_req->ie, local->scan_req->ie_len);
+
+	/*
+	 * After sending probe requests, wait for probe responses
+	 * on the channel.
+	 */
+	*next_delay = IEEE80211_CHANNEL_TIME;
+	local->scan_state = SCAN_SET_CHANNEL;
+}
+
 void ieee80211_scan_work(struct work_struct *work)
 {
 	struct ieee80211_local *local =
 		container_of(work, struct ieee80211_local, scan_work.work);
 	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
-	struct ieee80211_channel *chan;
-	int skip, i;
 	unsigned long next_delay = 0;
 
 	mutex_lock(&local->scan_mtx);
@@ -515,65 +587,11 @@ void ieee80211_scan_work(struct work_struct *work)
 
 	switch (local->scan_state) {
 	case SCAN_SET_CHANNEL:
-		/* if no more bands/channels left, complete scan */
-		if (local->scan_channel_idx >= local->scan_req->n_channels) {
-			ieee80211_scan_completed(&local->hw, false);
+		if (ieee80211_scan_state_set_channel(local, &next_delay))
 			return;
-		}
-		skip = 0;
-		chan = local->scan_req->channels[local->scan_channel_idx];
-
-		if (chan->flags & IEEE80211_CHAN_DISABLED ||
-		    (sdata->vif.type == NL80211_IFTYPE_ADHOC &&
-		     chan->flags & IEEE80211_CHAN_NO_IBSS))
-			skip = 1;
-
-		if (!skip) {
-			local->scan_channel = chan;
-			if (ieee80211_hw_config(local,
-						IEEE80211_CONF_CHANGE_CHANNEL))
-				skip = 1;
-		}
-
-		/* advance state machine to next channel/band */
-		local->scan_channel_idx++;
-
-		if (skip)
-			break;
-
-		/*
-		 * Probe delay is used to update the NAV, cf. 11.1.3.2.2
-		 * (which unfortunately doesn't say _why_ step a) is done,
-		 * but it waits for the probe delay or until a frame is
-		 * received - and the received frame would update the NAV).
-		 * For now, we do not support waiting until a frame is
-		 * received.
-		 *
-		 * In any case, it is not necessary for a passive scan.
-		 */
-		if (chan->flags & IEEE80211_CHAN_PASSIVE_SCAN ||
-		    !local->scan_req->n_ssids) {
-			next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
-			break;
-		}
-
-		next_delay = IEEE80211_PROBE_DELAY;
-		local->scan_state = SCAN_SEND_PROBE;
 		break;
 	case SCAN_SEND_PROBE:
-		for (i = 0; i < local->scan_req->n_ssids; i++)
-			ieee80211_send_probe_req(
-				sdata, NULL,
-				local->scan_req->ssids[i].ssid,
-				local->scan_req->ssids[i].ssid_len,
-				local->scan_req->ie, local->scan_req->ie_len);
-
-		/*
-		 * After sending probe requests, wait for probe responses
-		 * on the channel.
-		 */
-		next_delay = IEEE80211_CHANNEL_TIME;
-		local->scan_state = SCAN_SET_CHANNEL;
+		ieee80211_scan_state_send_probe(local, &next_delay);
 		break;
 	}
 

[RFC/RFT 2/5] mac80211: advance the state machine immediately if no delay is needed

[Helmut Schaa]

Instead of queueing the scan work again without delay just process the
next state immediately.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

---
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index a20f9e7..60a4e00 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -585,15 +585,21 @@ void ieee80211_scan_work(struct work_struct *work)
 		return;
 	}
 
-	switch (local->scan_state) {
-	case SCAN_SET_CHANNEL:
-		if (ieee80211_scan_state_set_channel(local, &next_delay))
-			return;
-		break;
-	case SCAN_SEND_PROBE:
-		ieee80211_scan_state_send_probe(local, &next_delay);
-		break;
-	}
+	/*
+	 * as long as no delay is required advance immediately
+	 * without scheduling a new work
+	 */
+	do {
+		switch (local->scan_state) {
+		case SCAN_SET_CHANNEL:
+			if (ieee80211_scan_state_set_channel(local, &next_delay))
+				return;
+			break;
+		case SCAN_SEND_PROBE:
+			ieee80211_scan_state_send_probe(local, &next_delay);
+			break;
+		}
+	} while (next_delay == 0);
 
 	queue_delayed_work(local->hw.workqueue, &local->scan_work,
 			   next_delay);

[RFC/RFT 3/5] mac80211: introduce a new scan state "decision"

[Helmut Schaa]

Introduce a new scan state "decision" which is entered after
every completed scan operation and decides about the next steps.
At first the decision is in any case to scan the next channel.
This shouldn't introduce any functional changes.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

---
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 6a01771..4166418 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -678,7 +678,7 @@ struct ieee80211_local {
 	int scan_channel_idx;
 	int scan_ies_len;
 
-	enum { SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
+	enum { SCAN_DECISION, SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
 	struct delayed_work scan_work;
 	struct ieee80211_sub_if_data *scan_sdata;
 	enum nl80211_channel_type oper_channel_type;
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 60a4e00..ef9d86d 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -376,7 +376,7 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
 	}
 	mutex_unlock(&local->iflist_mtx);
 
-	local->scan_state = SCAN_SET_CHANNEL;
+	local->scan_state = SCAN_DECISION;
 	local->scan_channel_idx = 0;
 
 	spin_lock_bh(&local->filter_lock);
@@ -474,17 +474,25 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	return rc;
 }
 
-static int ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsigned long *next_delay)
+static int ieee80211_scan_state_decision(struct ieee80211_local *local, unsigned long *next_delay)
 {
-	int skip;
-	struct ieee80211_channel *chan;
-	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
-
 	/* if no more bands/channels left, complete scan */
 	if (local->scan_channel_idx >= local->scan_req->n_channels) {
 		ieee80211_scan_completed(&local->hw, false);
 		return 1;
 	}
+
+	*next_delay = 0;
+	local->scan_state = SCAN_SET_CHANNEL;
+	return 0;
+}
+
+static void ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsigned long *next_delay)
+{
+	int skip;
+	struct ieee80211_channel *chan;
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+
 	skip = 0;
 	chan = local->scan_req->channels[local->scan_channel_idx];
 
@@ -504,7 +512,7 @@ static int ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsig
 	local->scan_channel_idx++;
 
 	if (skip)
-		return 0;
+		return;
 
 	/*
 	 * Probe delay is used to update the NAV, cf. 11.1.3.2.2
@@ -519,13 +527,13 @@ static int ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsig
 	if (chan->flags & IEEE80211_CHAN_PASSIVE_SCAN ||
 	    !local->scan_req->n_ssids) {
 		*next_delay = IEEE80211_PASSIVE_CHANNEL_TIME;
-		return 0;
+		local->scan_state = SCAN_DECISION;
+		return;
 	}
 
+	/* active scan, send probes */
 	*next_delay = IEEE80211_PROBE_DELAY;
 	local->scan_state = SCAN_SEND_PROBE;
-
-	return 0;
 }
 
 static void ieee80211_scan_state_send_probe(struct ieee80211_local *local, unsigned long *next_delay)
@@ -545,7 +553,7 @@ void ieee80211_scan_state_send_probe(struct ieee80211_local *local, unsigned lon
 	 * on the channel.
 	 */
 	*next_delay = IEEE80211_CHANNEL_TIME;
-	local->scan_state = SCAN_SET_CHANNEL;
+	local->scan_state = SCAN_DECISION;
 }
 
 void ieee80211_scan_work(struct work_struct *work)
@@ -591,9 +599,12 @@ void ieee80211_scan_work(struct work_struct *work)
 	 */
 	do {
 		switch (local->scan_state) {
+	 	case SCAN_DECISION:
+	 		if (ieee80211_scan_state_decision(local, &next_delay))
+ 				return;
+ 			break;
 		case SCAN_SET_CHANNEL:
-			if (ieee80211_scan_state_set_channel(local, &next_delay))
-				return;
+			ieee80211_scan_state_set_channel(local, &next_delay);
 			break;
 		case SCAN_SEND_PROBE:
 			ieee80211_scan_state_send_probe(local, &next_delay);

[RFC/RFT 4/5] mac80211: Replace {sw,hw}_scanning variables with a bitfield

[Helmut Schaa]

Use a bitfield to store the current scan mode instead of two boolean
variables {sw,hw}_scanning. This patch does not introduce functional
changes.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.com>

---
diff --git a/net/mac80211/ibss.c b/net/mac80211/ibss.c
index 8e22200..6e3cca6 100644
--- a/net/mac80211/ibss.c
+++ b/net/mac80211/ibss.c
@@ -742,7 +742,7 @@ static void ieee80211_ibss_work(struct work_struct *work)
 	if (!netif_running(sdata->dev))
 		return;
 
-	if (local->sw_scanning || local->hw_scanning)
+	if (local->scanning)
 		return;
 
 	if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_ADHOC))
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 4166418..be880b5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -570,6 +570,11 @@ enum queue_stop_reason {
 	IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
 };
 
+enum mac80211_scan_flag {
+	SCAN_SW_SCANNING = 1,
+	SCAN_HW_SCANNING = 2,
+};
+
 struct ieee80211_local {
 	/* embed the driver visible part.
 	 * don't cast (use the static inlines below), but we keep
@@ -668,7 +673,7 @@ struct ieee80211_local {
 
 	/* Scanning and BSS list */
 	struct mutex scan_mtx;
-	bool sw_scanning, hw_scanning;
+	unsigned long scanning;
 	struct cfg80211_ssid scan_ssid;
 	struct cfg80211_scan_request int_scan_req;
 	struct cfg80211_scan_request *scan_req;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index cadb0f6..2e514f2 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -513,7 +513,7 @@ static int ieee80211_stop(struct net_device *dev)
 			 * the scan_sdata is NULL already don't send out a
 			 * scan event to userspace -- the scan is incomplete.
 			 */
-			if (local->sw_scanning)
+			if (local->scanning & SCAN_SW_SCANNING)
 				ieee80211_scan_completed(&local->hw, true);
 		}
 
@@ -903,7 +903,7 @@ u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
 	struct ieee80211_sub_if_data *sdata;
 	int count = 0;
 
-	if (local->hw_scanning || local->sw_scanning)
+	if (local->scanning)
 		return ieee80211_idle_off(local, "scanning");
 
 	list_for_each_entry(sdata, &local->interfaces, list) {
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 3234f37..a1b4887 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -198,7 +198,7 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 	}
 
 	if (changed & BSS_CHANGED_BEACON_ENABLED) {
-		if (local->sw_scanning) {
+		if (local->scanning & SCAN_SW_SCANNING) {
 			sdata->vif.bss_conf.enable_beacon = false;
 		} else {
 			/*
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 8a97b14..9a38269 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -597,7 +597,7 @@ static void ieee80211_mesh_work(struct work_struct *work)
 	if (!netif_running(sdata->dev))
 		return;
 
-	if (local->sw_scanning || local->hw_scanning)
+	if (local->scanning)
 		return;
 
 	while ((skb = skb_dequeue(&ifmsh->skb_queue)))
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 18dad22..6d62ee9 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -581,7 +581,7 @@ void ieee80211_sta_process_chanswitch(struct ieee80211_sub_if_data *sdata,
 	if (!ifmgd->associated)
 		return;
 
-	if (sdata->local->sw_scanning || sdata->local->hw_scanning)
+	if (sdata->local->scanning)
 		return;
 
 	/* Disregard subsequent beacons if we are already running a timer
@@ -639,7 +639,7 @@ static void ieee80211_enable_ps(struct ieee80211_local *local,
 	 * If we are scanning right now then the parameters will
 	 * take effect when scan finishes.
 	 */
-	if (local->hw_scanning || local->sw_scanning)
+	if (local->scanning)
 		return;
 
 	if (conf->dynamic_ps_timeout > 0 &&
@@ -2035,7 +2035,7 @@ static void ieee80211_sta_work(struct work_struct *work)
 	if (!netif_running(sdata->dev))
 		return;
 
-	if (local->sw_scanning || local->hw_scanning)
+	if (local->scanning)
 		return;
 
 	if (WARN_ON(sdata->vif.type != NL80211_IFTYPE_STATION))
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 66c797c..3df8a6e 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -418,10 +418,10 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 	struct ieee80211_local *local = rx->local;
 	struct sk_buff *skb = rx->skb;
 
-	if (unlikely(local->hw_scanning))
+	if (unlikely(local->scanning & SCAN_HW_SCANNING))
 		return ieee80211_scan_rx(rx->sdata, skb);
 
-	if (unlikely(local->sw_scanning)) {
+	if (unlikely(local->scanning & SCAN_SW_SCANNING)) {
 		/* drop all the other packets during a software scan anyway */
 		if (ieee80211_scan_rx(rx->sdata, skb) != RX_QUEUED)
 			dev_kfree_skb(skb);
@@ -2136,7 +2136,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 		return;
 	}
 
-	if (unlikely(local->sw_scanning || local->hw_scanning))
+	if (unlikely(local->scanning))
 		rx.flags |= IEEE80211_RX_IN_SCAN;
 
 	ieee80211_parse_qos(&rx);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index ef9d86d..b4cc556 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -265,7 +265,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
 
 	mutex_lock(&local->scan_mtx);
 
-	if (WARN_ON(!local->hw_scanning && !local->sw_scanning)) {
+	if (WARN_ON(!local->scanning)) {
 		mutex_unlock(&local->scan_mtx);
 		return;
 	}
@@ -275,16 +275,15 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
 		return;
 	}
 
-	if (local->hw_scanning)
+	if (local->scanning & SCAN_HW_SCANNING)
 		ieee80211_restore_scan_ies(local);
 
 	if (local->scan_req != &local->int_scan_req)
 		cfg80211_scan_done(local->scan_req, aborted);
 	local->scan_req = NULL;
 
-	was_hw_scan = local->hw_scanning;
-	local->hw_scanning = false;
-	local->sw_scanning = false;
+	was_hw_scan = local->scanning & SCAN_HW_SCANNING;
+	local->scanning = 0;
 	local->scan_channel = NULL;
 
 	/* we only have to protect scan_req and hw/sw scan */
@@ -434,9 +433,9 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	}
 
 	if (local->ops->hw_scan)
-		local->hw_scanning = true;
+		local->scanning |= SCAN_HW_SCANNING;
 	else
-		local->sw_scanning = true;
+		local->scanning |= SCAN_SW_SCANNING;
 	/*
 	 * Kicking off the scan need not be protected,
 	 * only the scan variable stuff, since now
@@ -459,11 +458,9 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	mutex_lock(&local->scan_mtx);
 
 	if (rc) {
-		if (local->ops->hw_scan) {
-			local->hw_scanning = false;
+		if (local->ops->hw_scan)
 			ieee80211_restore_scan_ies(local);
-		} else
-			local->sw_scanning = false;
+		local->scanning = 0;
 
 		ieee80211_recalc_idle(local);
 
@@ -569,7 +566,7 @@ void ieee80211_scan_work(struct work_struct *work)
 		return;
 	}
 
-	if (local->scan_req && !(local->sw_scanning || local->hw_scanning)) {
+	if (local->scan_req && !local->scanning) {
 		struct cfg80211_scan_request *req = local->scan_req;
 		int rc;
 
@@ -660,7 +657,7 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
 	 * queued -- mostly at suspend under RTNL.
 	 */
 	mutex_lock(&local->scan_mtx);
-	swscan = local->sw_scanning;
+	swscan = !!(local->scanning & SCAN_SW_SCANNING);
 	mutex_unlock(&local->scan_mtx);
 
 	if (swscan)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 969a4b2..4f3d7f1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -192,7 +192,7 @@ ieee80211_tx_h_check_assoc(struct ieee80211_tx_data *tx)
 	if (unlikely(info->flags & IEEE80211_TX_CTL_INJECTED))
 		return TX_CONTINUE;
 
-	if (unlikely(tx->local->sw_scanning) &&
+	if (unlikely(tx->local->scanning & SCAN_SW_SCANNING) &&
 	    !ieee80211_is_probe_req(hdr->frame_control) &&
 	    !ieee80211_is_nullfunc(hdr->frame_control))
 		/*
@@ -1376,7 +1376,7 @@ static void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
 
 	if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
 	    local->hw.conf.dynamic_ps_timeout > 0 &&
-	    !local->sw_scanning && !local->hw_scanning && local->ps_sdata) {
+	    !(local->scanning) && local->ps_sdata) {
 		if (local->hw.conf.flags & IEEE80211_CONF_PS) {
 			ieee80211_stop_queues_by_reason(&local->hw,
 					IEEE80211_QUEUE_STOP_REASON_PS);

Re: [RFC/RFT 4/5] mac80211: Replace {sw,hw}_scanning variables with a bitfield

[Luis R. Rodriguez]

On Thu, Jul 16, 2009 at 2:08 AM, Helmut
Schaa<helmut.schaa@googlemail.com> wrote:
> Use a bitfield to store the current scan mode instead of two boolean
> variables {sw,hw}_scanning. This patch does not introduce functional
> changes.
>

> +enum mac80211_scan_flag {
> +       SCAN_SW_SCANNING = 1,
> +       SCAN_HW_SCANNING = 2,
> +};
> +

But why is that needed? The commit log doesn't state that. We won't
use both at the same time.

  Luis

Re: [RFC/RFT 4/5] mac80211: Replace {sw,hw}_scanning variables with a bitfield

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 722 bytes --]

On Thu, 2009-07-16 at 09:30 -0700, Luis R. Rodriguez wrote:
> On Thu, Jul 16, 2009 at 2:08 AM, Helmut
> Schaa<helmut.schaa@googlemail.com> wrote:
> > Use a bitfield to store the current scan mode instead of two boolean
> > variables {sw,hw}_scanning. This patch does not introduce functional
> > changes.
> >
> 
> > +enum mac80211_scan_flag {
> > +       SCAN_SW_SCANNING = 1,
> > +       SCAN_HW_SCANNING = 2,
> > +};
> > +
> 
> But why is that needed? The commit log doesn't state that. We won't
> use both at the same time.

You're nitpicking ;)
We currently have two variables, and this just changes them into two
bits instead, so he can extend the sw-scanning case with another bit :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 4/5] mac80211: Replace {sw,hw}_scanning variables with a bitfield

[Luis R. Rodriguez]

On Thu, Jul 16, 2009 at 9:43 AM, Johannes Berg<johannes@sipsolutions.net> wrote:
> On Thu, 2009-07-16 at 09:30 -0700, Luis R. Rodriguez wrote:
>> On Thu, Jul 16, 2009 at 2:08 AM, Helmut
>> Schaa<helmut.schaa@googlemail.com> wrote:
>> > Use a bitfield to store the current scan mode instead of two boolean
>> > variables {sw,hw}_scanning. This patch does not introduce functional
>> > changes.
>> >
>>
>> > +enum mac80211_scan_flag {
>> > +       SCAN_SW_SCANNING = 1,
>> > +       SCAN_HW_SCANNING = 2,
>> > +};
>> > +
>>
>> But why is that needed? The commit log doesn't state that. We won't
>> use both at the same time.
>
> You're nitpicking ;)
> We currently have two variables, and this just changes them into two
> bits instead, so he can extend the sw-scanning case with another bit :)

I didn't realize that until reading the next patch.

 Luis

[RFC/RFT 5/5] mac80211: implement basic background scanning

[Helmut Schaa]

Introduce a new scan flag "SCAN_BG_SCANNING" which basically tells us
that we are currently scanning but may as well be on the operating channel
to RX/TX data whereas "SCAN_SW_SCANNING" tells us that we are currently
on a different channel for scanning. While "SCAN_BG_SCANNING" is set
during the whole scan "SCAN_SW_SCANNING" is set when leaving the operating
channel and unset when coming back.

Introduce two new scan states "SCAN_LEAVE_OPER_CHANNEL" and
"SCAN_ENTER_OPER_CHANNEL" which basically implement the functionality we
need to leave the operating channel (send a nullfunc to the AP and stop
the queues) and enter it again (send a nullfunc to the AP and start the
queues again).

Enhance the scan state "SCAN_DECISION" to switch back to the operating
channel after each scanned channel. In the future it sould be simple
to enhance the decision state to scan as much channels in a row as the
qos latency allows us.

Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.>

---
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index be880b5..15e8b4a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -570,9 +570,40 @@ enum queue_stop_reason {
 	IEEE80211_QUEUE_STOP_REASON_SKB_ADD,
 };
 
+/**
+ * enum mac80211_scan_flag - currently active scan mode
+ *
+ * @SCAN_SW_SCANNING: We're off our operating channel for scanning
+ * @SCAN_HW_SCANNING: The hardware is scanning for us, we have no way to
+ *	determine if we are on the operating channel or not
+ * @SCAN_BG_SCANNING: We're currently in the process of scanning but may
+ *	as well be on the operating channel
+ */
 enum mac80211_scan_flag {
 	SCAN_SW_SCANNING = 1,"SCAN_ENTER_OPER_CHANNEL"
 	SCAN_HW_SCANNING = 2,
+	SCAN_BG_SCANNING = 4,
+};
+
+/**
+ * enum mac80211_scan_state - scan state machine states
+ *
+ * @SCAN_DECISION: Main entry point to the scan state machine, this state
+ *	determines if we should keep on scanning or switch back to the
+ *	operating channel
+ * @SCAN_SET_CHANNEL: Set the next channel to be scanned
+ * @SCAN_SEND_PROBE: Send probe requests and wait for probe responses
+ * @SCAN_LEAVE_OPER_CHANNEL: Leave the operating channel, notify the AP
+ *	about us leaving the channel and stop all associated STA interfaces
+ * @SCAN_ENTER_OPER_CHANNEL: Enter the operating channel again, notify the
+ *	AP about us being back and restart all associated STA interfaces
+ */
+enum mac80211_scan_state {
+	SCAN_DECISION,
+	SCAN_SET_CHANNEL,
+	SCAN_SEND_PROBE,
+	SCAN_LEAVE_OPER_CHANNEL,
+	SCAN_ENTER_OPER_CHANNEL,
 };
 
 struct ieee80211_local {
@@ -683,7 +714,7 @@ struct ieee80211_local {
 	int scan_channel_idx;
 	int scan_ies_len;
 
-	enum { SCAN_DECISION, SCAN_SET_CHANNEL, SCAN_SEND_PROBE } scan_state;
+	enum mac80211_scan_state scan_state;
 	struct delayed_work scan_work;
 	struct ieee80211_sub_if_data *scan_sdata;
 	enum nl80211_channel_type oper_channel_type;
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 2e514f2..634442c 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -513,7 +513,7 @@ static int ieee80211_stop(struct net_device *dev)
 			 * the scan_sdata is NULL already don't send out a
 			 * scan event to userspace -- the scan is incomplete.
 			 */
-			if (local->scanning & SCAN_SW_SCANNING)
+			if (local->scanning & SCAN_BG_SCANNING)
 				ieee80211_scan_completed(&local->hw, true);
 		}
 
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index a1b4887..a87522f 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -198,7 +198,7 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
 	}
 
 	if (changed & BSS_CHANGED_BEACON_ENABLED) {
-		if (local->scanning & SCAN_SW_SCANNING) {
+		if (local->scanning & SCAN_BG_SCANNING) {
 			sdata->vif.bss_conf.enable_beacon = false;
 		} else {
 			/*
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 3df8a6e..24739ab 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2136,7 +2136,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 		return;
 	}
 
-	if (unlikely(local->scanning))
+	if (unlikely((local->scanning & SCAN_HW_SCANNING) || (local->scanning & SCAN_SW_SCANNING)))
 		rx.flags |= IEEE80211_RX_IN_SCAN;
 
 	ieee80211_parse_qos(&rx);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index b4cc556..8f33fb5 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -282,7 +282,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
 		cfg80211_scan_done(local->scan_req, aborted);
 	local->scan_req = NULL;
 
-	was_hw_scan = local->scanning & SCAN_HW_SCANNING;
+	was_hw_scan = !!(local->scanning & SCAN_HW_SCANNING);
 	local->scanning = 0;
 	local->scan_channel = NULL;
 
@@ -365,12 +365,11 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
 			ieee80211_bss_info_change_notify(
 				sdata, BSS_CHANGED_BEACON_ENABLED);
 
-		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
-			if (sdata->u.mgd.associated) {
-				netif_tx_stop_all_queues(sdata->dev);
-				ieee80211_scan_ps_enable(sdata);
-			}
-		} else
+		/*
+		 * only handle non-STA interfaces here, STA interfaces
+		 * are handled in the scan state machine
+		 */
+		if (sdata->vif.type != NL80211_IFTYPE_STATION)
 			netif_tx_stop_all_queues(sdata->dev);
 	}
 	mutex_unlock(&local->iflist_mtx);
@@ -435,7 +434,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	if (local->ops->hw_scan)
 		local->scanning |= SCAN_HW_SCANNING;
 	else
-		local->scanning |= SCAN_SW_SCANNING;
+		local->scanning |= SCAN_BG_SCANNING;
 	/*
 	 * Kicking off the scan need not be protected,
 	 * only the scan variable stuff, since now
@@ -473,17 +472,115 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 
 static int ieee80211_scan_state_decision(struct ieee80211_local *local, unsigned long *next_delay)
 {
-	/* if no more bands/channels left, complete scan */
+	bool associated = false;
+	struct ieee80211_sub_if_data *sdata;
+
+	/* if no more bands/channels left, complete scan and advance to the idle state */
 	if (local->scan_channel_idx >= local->scan_req->n_channels) {
 		ieee80211_scan_completed(&local->hw, false);
 		return 1;
 	}
 
+	/* check if at least one STA interface is associated */
+	mutex_lock(&local->iflist_mtx);
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		if (!netif_running(sdata->dev))
+			continue;
+
+		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+			if (sdata->u.mgd.associated) {
+				associated = true;
+				break;
+			}
+		}
+	}
+	mutex_unlock(&local->iflist_mtx);
+
+	if (local->scan_channel) {
+		/*
+		 * we're currently scanning a different channel, let's
+		 * switch back to the operating channel now if at least
+		 * one interface is associated. Otherwise just scan the
+		 * next channel
+		 */
+		if (associated)
+			local->scan_state = SCAN_ENTER_OPER_CHANNEL;
+		else
+			local->scan_state = SCAN_SET_CHANNEL;
+	} else {
+		/*
+		 * we're on the operating channel currently, let's
+		 * leave that channel now to scan another one
+		 */
+		local->scan_state = SCAN_LEAVE_OPER_CHANNEL;
+	}
+
 	*next_delay = 0;
-	local->scan_state = SCAN_SET_CHANNEL;
 	return 0;
 }
 
+static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local, unsigned long *next_delay)
+{
+	struct ieee80211_sub_if_data *sdata;
+
+	local->scanning |= SCAN_SW_SCANNING;
+
+	/*
+	 * 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 (!netif_running(sdata->dev))
+			continue;
+
+		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+			netif_tx_stop_all_queues(sdata->dev);
+			if (sdata->u.mgd.associated)
+				ieee80211_scan_ps_enable(sdata);
+		}
+	}
+	mutex_unlock(&local->iflist_mtx);
+
+	/* advance to the next channel to be scanned */
+	*next_delay = HZ / 10;
+	local->scan_state = SCAN_SET_CHANNEL;
+}
+
+static void ieee80211_scan_state_enter_oper_channel(struct ieee80211_local *local, unsigned long *next_delay)
+{
+	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+
+	/* switch back to the operating channel */
+	local->scan_channel = NULL;
+	if (ieee80211_hw_config(local,
+					IEEE80211_CONF_CHANGE_CHANNEL))
+		printk(KERN_DEBUG "%s: failed to restore operational "
+		"channel during scan\n", sdata->dev->name);
+
+	/*
+	 * notify the AP about us being back and restart all STA interfaces
+	 */
+	mutex_lock(&local->iflist_mtx);
+	list_for_each_entry(sdata, &local->interfaces, list) {
+		if (!netif_running(sdata->dev))
+			continue;
+
+		/* Tell AP we're back */
+		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
+			if (sdata->u.mgd.associated) {
+				ieee80211_scan_ps_disable(sdata);
+			}
+			netif_tx_wake_all_queues(sdata->dev);
+		}
+	}
+	mutex_unlock(&local->iflist_mtx);
+
+	local->scanning &= ~SCAN_SW_SCANNING;
+
+	*next_delay = HZ / 5;
+	local->scan_state = SCAN_DECISION;
+}
+
 static void ieee80211_scan_state_set_channel(struct ieee80211_local *local, unsigned long *next_delay)
 {
 	int skip;
@@ -606,6 +703,12 @@ void ieee80211_scan_work(struct work_struct *work)
 		case SCAN_SEND_PROBE:
 			ieee80211_scan_state_send_probe(local, &next_delay);
 			break;
+		case SCAN_LEAVE_OPER_CHANNEL:
+			ieee80211_scan_state_leave_oper_channel(local, &next_delay);
+			break;
+		case SCAN_ENTER_OPER_CHANNEL:
+			ieee80211_scan_state_enter_oper_channel(local, &next_delay);
+			break;
 		}
 	} while (next_delay == 0);
 
@@ -657,7 +760,7 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
 	 * queued -- mostly at suspend under RTNL.
 	 */
 	mutex_lock(&local->scan_mtx);
-	swscan = !!(local->scanning & SCAN_SW_SCANNING);
+	swscan = !!(local->scanning & SCAN_BG_SCANNING);
 	mutex_unlock(&local->scan_mtx);
 
 	if (swscan)

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 5093 bytes --]

On Thu, 2009-07-16 at 11:09 +0200, Helmut Schaa wrote:

Looks nice! Some nitpicks ;)

> Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.>

Missing "com" :)

> +/**
> + * enum mac80211_scan_flag - currently active scan mode
> + *
> + * @SCAN_SW_SCANNING: We're off our operating channel for scanning
> + * @SCAN_HW_SCANNING: The hardware is scanning for us, we have no way to
> + *	determine if we are on the operating channel or not
> + * @SCAN_BG_SCANNING: We're currently in the process of scanning but may
> + *	as well be on the operating channel
> + */
>  enum mac80211_scan_flag {
>  	SCAN_SW_SCANNING = 1,"SCAN_ENTER_OPER_CHANNEL"
>  	SCAN_HW_SCANNING = 2,
> +	SCAN_BG_SCANNING = 4,

There's some random stuff in there that doesn't belong. Also I would
prefer you used BIT(0) etc. or maybe __test_bit().

> --- a/net/mac80211/iface.c
> +++ b/net/mac80211/iface.c
> @@ -513,7 +513,7 @@ static int ieee80211_stop(struct net_device *dev)
>  			 * the scan_sdata is NULL already don't send out a
>  			 * scan event to userspace -- the scan is incomplete.
>  			 */
> -			if (local->scanning & SCAN_SW_SCANNING)
> +			if (local->scanning & SCAN_BG_SCANNING)
>  				ieee80211_scan_completed(&local->hw, true);
>  		}

That doesn't seem correct -- it should be kept I think.

> diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> index a1b4887..a87522f 100644
> --- a/net/mac80211/main.c
> +++ b/net/mac80211/main.c
> @@ -198,7 +198,7 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
>  	}
>  
>  	if (changed & BSS_CHANGED_BEACON_ENABLED) {
> -		if (local->scanning & SCAN_SW_SCANNING) {
> +		if (local->scanning & SCAN_BG_SCANNING) {
>  			sdata->vif.bss_conf.enable_beacon = false;

That too.

> diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> index 3df8a6e..24739ab 100644
> --- a/net/mac80211/rx.c
> +++ b/net/mac80211/rx.c
> @@ -2136,7 +2136,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
>  		return;
>  	}
>  
> -	if (unlikely(local->scanning))
> +	if (unlikely((local->scanning & SCAN_HW_SCANNING) || (local->scanning & SCAN_SW_SCANNING)))

I would prefer
	if (unlikely(local->scanning & (SCAN_HW_SCANNING | SCAN_SW_SCANNING)))

>  		rx.flags |= IEEE80211_RX_IN_SCAN;
>  
>  	ieee80211_parse_qos(&rx);
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index b4cc556..8f33fb5 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -282,7 +282,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
>  		cfg80211_scan_done(local->scan_req, aborted);
>  	local->scan_req = NULL;
>  
> -	was_hw_scan = local->scanning & SCAN_HW_SCANNING;
> +	was_hw_scan = !!(local->scanning & SCAN_HW_SCANNING);

Should that be in the other patch?

> @@ -435,7 +434,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
>  	if (local->ops->hw_scan)
>  		local->scanning |= SCAN_HW_SCANNING;
>  	else
> -		local->scanning |= SCAN_SW_SCANNING;
> +		local->scanning |= SCAN_BG_SCANNING;

Ok now I'm confused. Did you really intend to replace all these?

Based on your initial description I thought it was going to be
	scanning == SW_SCANNING
or
	scanning == SW_SCANNING | BG_SCANNING

> +	if (local->scan_channel) {
> +		/*
> +		 * we're currently scanning a different channel, let's
> +		 * switch back to the operating channel now if at least
> +		 * one interface is associated. Otherwise just scan the
> +		 * next channel
> +		 */
> +		if (associated)
> +			local->scan_state = SCAN_ENTER_OPER_CHANNEL;
> +		else
> +			local->scan_state = SCAN_SET_CHANNEL;

:)
 
> +	/* advance to the next channel to be scanned */
> +	*next_delay = HZ / 10;
> +	local->scan_state = SCAN_SET_CHANNEL;

Maybe we should rename scan_state to next_scan_state.

> +	if (ieee80211_hw_config(local,
> +					IEEE80211_CONF_CHANGE_CHANNEL))

That looks weird. I don't think you really need to care about the return
value anyway, now that we have rfkill integrated it shouldn't ever be
nonzero (and if it is, while rfkill is being activated, it doesn't
really matter since we're taking down interfaces)

> +	/*
> +	 * notify the AP about us being back and restart all STA interfaces
> +	 */
> +	mutex_lock(&local->iflist_mtx);
> +	list_for_each_entry(sdata, &local->interfaces, list) {
> +		if (!netif_running(sdata->dev))
> +			continue;
> +
> +		/* Tell AP we're back */
> +		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> +			if (sdata->u.mgd.associated) {
> +				ieee80211_scan_ps_disable(sdata);
> +			}

Could drop a set of {} here.

> @@ -657,7 +760,7 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
>  	 * queued -- mostly at suspend under RTNL.
>  	 */
>  	mutex_lock(&local->scan_mtx);
> -	swscan = !!(local->scanning & SCAN_SW_SCANNING);
> +	swscan = !!(local->scanning & SCAN_BG_SCANNING);

and another one -- please explain?

Anyway looks pretty good to me! How does it fare during ping -f or
something?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Helmut Schaa]

Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> On Thu, 2009-07-16 at 11:09 +0200, Helmut Schaa wrote:
> 
> Looks nice! Some nitpicks ;)

Great, thanks ;)

> > Signed-off-by: Helmut Schaa <helmut.schaa@googlemail.>
> 
> Missing "com" :)

Oops.

> > +/**
> > + * enum mac80211_scan_flag - currently active scan mode
> > + *
> > + * @SCAN_SW_SCANNING: We're off our operating channel for scanning
> > + * @SCAN_HW_SCANNING: The hardware is scanning for us, we have no way to
> > + *	determine if we are on the operating channel or not
> > + * @SCAN_BG_SCANNING: We're currently in the process of scanning but may
> > + *	as well be on the operating channel
> > + */
> >  enum mac80211_scan_flag {
> >  	SCAN_SW_SCANNING = 1,"SCAN_ENTER_OPER_CHANNEL"
> >  	SCAN_HW_SCANNING = 2,
> > +	SCAN_BG_SCANNING = 4,
> 
> There's some random stuff in there that doesn't belong.

Right, that does not belong there.

> Also I would 
> prefer you used BIT(0) etc. or maybe __test_bit().

Fine with me.

> > --- a/net/mac80211/iface.c
> > +++ b/net/mac80211/iface.c
> > @@ -513,7 +513,7 @@ static int ieee80211_stop(struct net_device *dev)
> >  			 * the scan_sdata is NULL already don't send out a
> >  			 * scan event to userspace -- the scan is incomplete.
> >  			 */
> > -			if (local->scanning & SCAN_SW_SCANNING)
> > +			if (local->scanning & SCAN_BG_SCANNING)
> >  				ieee80211_scan_completed(&local->hw, true);
> >  		}
> 
> That doesn't seem correct -- it should be kept I think.

See below.

> > diff --git a/net/mac80211/main.c b/net/mac80211/main.c
> > index a1b4887..a87522f 100644
> > --- a/net/mac80211/main.c
> > +++ b/net/mac80211/main.c
> > @@ -198,7 +198,7 @@ void ieee80211_bss_info_change_notify(struct ieee80211_sub_if_data *sdata,
> >  	}
> >  
> >  	if (changed & BSS_CHANGED_BEACON_ENABLED) {
> > -		if (local->scanning & SCAN_SW_SCANNING) {
> > +		if (local->scanning & SCAN_BG_SCANNING) {
> >  			sdata->vif.bss_conf.enable_beacon = false;
> 
> That too.
> 
> > diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
> > index 3df8a6e..24739ab 100644
> > --- a/net/mac80211/rx.c
> > +++ b/net/mac80211/rx.c
> > @@ -2136,7 +2136,7 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
> >  		return;
> >  	}
> >  
> > -	if (unlikely(local->scanning))
> > +	if (unlikely((local->scanning & SCAN_HW_SCANNING) || (local->scanning & SCAN_SW_SCANNING)))
> 
> I would prefer
> 	if (unlikely(local->scanning & (SCAN_HW_SCANNING | SCAN_SW_SCANNING)))

Ack.

> >  		rx.flags |= IEEE80211_RX_IN_SCAN;
> >  
> >  	ieee80211_parse_qos(&rx);
> > diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> > index b4cc556..8f33fb5 100644
> > --- a/net/mac80211/scan.c
> > +++ b/net/mac80211/scan.c
> > @@ -282,7 +282,7 @@ void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
> >  		cfg80211_scan_done(local->scan_req, aborted);
> >  	local->scan_req = NULL;
> >  
> > -	was_hw_scan = local->scanning & SCAN_HW_SCANNING;
> > +	was_hw_scan = !!(local->scanning & SCAN_HW_SCANNING);
> 
> Should that be in the other patch?

Yep.

> > @@ -435,7 +434,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
> >  	if (local->ops->hw_scan)
> >  		local->scanning |= SCAN_HW_SCANNING;
> >  	else
> > -		local->scanning |= SCAN_SW_SCANNING;
> > +		local->scanning |= SCAN_BG_SCANNING;
> 
> Ok now I'm confused. Did you really intend to replace all these?
> 
> Based on your initial description I thought it was going to be
> 	scanning == SW_SCANNING
> or
> 	scanning == SW_SCANNING | BG_SCANNING

It's exactly the other way round :)

It is either BG_SCANNING or SW_SCANNING | BG_SCANNING.

I already thought that this might cause confusion but I think
BG_SCANNING better reflects that we are currently running a scan
(independant of the current scan state) whereas SW_SCANNING better
reflects that we are on a different channel for scanning.

Maybe I should use other terms. Ideas?

> > +	if (local->scan_channel) {
> > +		/*
> > +		 * we're currently scanning a different channel, let's
> > +		 * switch back to the operating channel now if at least
> > +		 * one interface is associated. Otherwise just scan the
> > +		 * next channel
> > +		 */
> > +		if (associated)
> > +			local->scan_state = SCAN_ENTER_OPER_CHANNEL;
> > +		else
> > +			local->scan_state = SCAN_SET_CHANNEL;
> 
> :)
>  
> > +	/* advance to the next channel to be scanned */
> > +	*next_delay = HZ / 10;
> > +	local->scan_state = SCAN_SET_CHANNEL;
> 
> Maybe we should rename scan_state to next_scan_state.

Makes sense.

> > +	if (ieee80211_hw_config(local,
> > +					IEEE80211_CONF_CHANGE_CHANNEL))
> 
> That looks weird. I don't think you really need to care about the return
> value anyway, now that we have rfkill integrated it shouldn't ever be
> nonzero (and if it is, while rfkill is being activated, it doesn't
> really matter since we're taking down interfaces)

Ok, will change that.

> > +	/*
> > +	 * notify the AP about us being back and restart all STA interfaces
> > +	 */
> > +	mutex_lock(&local->iflist_mtx);
> > +	list_for_each_entry(sdata, &local->interfaces, list) {
> > +		if (!netif_running(sdata->dev))
> > +			continue;
> > +
> > +		/* Tell AP we're back */
> > +		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
> > +			if (sdata->u.mgd.associated) {
> > +				ieee80211_scan_ps_disable(sdata);
> > +			}
> 
> Could drop a set of {} here.

Right.

> > @@ -657,7 +760,7 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
> >  	 * queued -- mostly at suspend under RTNL.
> >  	 */
> >  	mutex_lock(&local->scan_mtx);
> > -	swscan = !!(local->scanning & SCAN_SW_SCANNING);
> > +	swscan = !!(local->scanning & SCAN_BG_SCANNING);
> 
> and another one -- please explain?

See above.

> Anyway looks pretty good to me! How does it fare during ping -f or
> something?

I compared it to the hw_scan implementation of iwlwifi. We loose a few
more frames (I guess due to not flushing the queues before channel switch)
but it's not really much, it was <1% for ping -f).

I didn't do much performance testing, just a single wget and the performance
dropped to about 50%. I still have to run some iperf tests (both RX and TX) to
see how it behaves.

Helmut

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1714 bytes --]

On Thu, 2009-07-16 at 11:50 +0200, Helmut Schaa wrote:

> > Based on your initial description I thought it was going to be
> > 	scanning == SW_SCANNING
> > or
> > 	scanning == SW_SCANNING | BG_SCANNING
> 
> It's exactly the other way round :)
> 
> It is either BG_SCANNING or SW_SCANNING | BG_SCANNING.
> 
> I already thought that this might cause confusion but I think
> BG_SCANNING better reflects that we are currently running a scan
> (independant of the current scan state) whereas SW_SCANNING better
> reflects that we are on a different channel for scanning.
> 
> Maybe I should use other terms. Ideas?

Ah, ok. Since your patch 4/5 changes sw_scanning to SW_SCANNING, I think
at least change it to BG_SCANNING there already. OTOH, I think people
are used to sw_scanning so it would be better to keep it. Maybe do
	SW_SCANNING
and
	SW_SCANNING | OFF_CHANNEL

or maybe
	SW_SCANNING | PROBING
or something like that?

> > Anyway looks pretty good to me! How does it fare during ping -f or
> > something?
> 
> I compared it to the hw_scan implementation of iwlwifi. We loose a few
> more frames (I guess due to not flushing the queues before channel switch)
> but it's not really much, it was <1% for ping -f).

Yeah, we still need to add a queue flush callback for the hardware, but
that can wait some more.

> I didn't do much performance testing, just a single wget and the performance
> dropped to about 50%. I still have to run some iperf tests (both RX and TX) to
> see how it behaves.

I'd be more interested in the rtt stats that ping -f prints after you
abort it:

rtt min/avg/max/mdev = 0.021/0.028/1.726/0.051 ms

(this was on 127.0.0.1)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Helmut Schaa]

Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> On Thu, 2009-07-16 at 11:50 +0200, Helmut Schaa wrote:
> > Maybe I should use other terms. Ideas?
> 
> Ah, ok. Since your patch 4/5 changes sw_scanning to SW_SCANNING, I think
> at least change it to BG_SCANNING there already. OTOH, I think people
> are used to sw_scanning so it would be better to keep it. Maybe do
> 	SW_SCANNING
> and
> 	SW_SCANNING | OFF_CHANNEL
> 
> or maybe
> 	SW_SCANNING | PROBING
> or something like that?

Yes, something like this sounds better and does not cause so much confusion.

> > > Anyway looks pretty good to me! How does it fare during ping -f or
> > > something?
> > 
> > I compared it to the hw_scan implementation of iwlwifi. We loose a few
> > more frames (I guess due to not flushing the queues before channel switch)
> > but it's not really much, it was <1% for ping -f).
> 
> Yeah, we still need to add a queue flush callback for the hardware, but
> that can wait some more.

Right, that can wait :)

> > I didn't do much performance testing, just a single wget and the performance
> > dropped to about 50%. I still have to run some iperf tests (both RX and TX) to
> > see how it behaves.
> 
> I'd be more interested in the rtt stats that ping -f prints after you
> abort it:
> 
> rtt min/avg/max/mdev = 0.021/0.028/1.726/0.051 ms

I don't have any stats here anymore but if I remember correctly the max rtt I
got was around 250ms. Will try that again when I'm back in the evening.

Helmut

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Helmut Schaa]

Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> On Thu, 2009-07-16 at 11:50 +0200, Helmut Schaa wrote:
> > I didn't do much performance testing, just a single wget and the performance
> > dropped to about 50%. I still have to run some iperf tests (both RX and TX) to
> > see how it behaves.
> 
> I'd be more interested in the rtt stats that ping -f prints after you
> abort it:
> 
> rtt min/avg/max/mdev = 0.021/0.028/1.726/0.051 ms

JFI, here's the result:

30851 packets transmitted, 30796 received, 0% packet loss, time 80592ms
rtt min/avg/max/mdev = 1.512/8.235/251.145/32.479 ms, pipe 12, ipg/ewma 2.612/1.709 ms

Helmut

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 793 bytes --]

On Thu, 2009-07-16 at 22:52 +0200, Helmut Schaa wrote:
> Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> > On Thu, 2009-07-16 at 11:50 +0200, Helmut Schaa wrote:
> > > I didn't do much performance testing, just a single wget and the performance
> > > dropped to about 50%. I still have to run some iperf tests (both RX and TX) to
> > > see how it behaves.
> > 
> > I'd be more interested in the rtt stats that ping -f prints after you
> > abort it:
> > 
> > rtt min/avg/max/mdev = 0.021/0.028/1.726/0.051 ms
> 
> JFI, here's the result:
> 
> 30851 packets transmitted, 30796 received, 0% packet loss, time 80592ms
> rtt min/avg/max/mdev = 1.512/8.235/251.145/32.479 ms, pipe 12, ipg/ewma 2.612/1.709 ms

Nice. Much better than the multiple seconds I had :)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 5/5] mac80211: implement basic background scanning

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 818 bytes --]

On Thu, 2009-07-16 at 11:50 +0200, Helmut Schaa wrote:

> > > +	if (ieee80211_hw_config(local,
> > > +					IEEE80211_CONF_CHANGE_CHANNEL))
> > 
> > That looks weird. I don't think you really need to care about the return
> > value anyway, now that we have rfkill integrated it shouldn't ever be
> > nonzero (and if it is, while rfkill is being activated, it doesn't
> > really matter since we're taking down interfaces)
> 
> Ok, will change that.

Oh and you can also pass 0 as the change flags, the routine will detect
scan_channel and set the flag accordingly. Doesn't matter much, except
in the case where you're not actually changing the channel because it's
the same as the operational channel (which would be easy to optimise
away by just sending probes on the current oper_channel)

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 0/5] mac80211: implement background scan

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 343 bytes --]

On Thu, 2009-07-16 at 11:04 +0200, Helmut Schaa wrote:

> So, it would be great if somebody could test the patches on other
> hardware as well.

If you repost with the stuff we talked about fixed, I'll give them a
spin, but for all I can tell they should be good to go in, it's a huge
improvement and we can build on that.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [RFC/RFT 0/5] mac80211: implement background scan

[Helmut Schaa]

Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> On Thu, 2009-07-16 at 11:04 +0200, Helmut Schaa wrote:
> 
> > So, it would be great if somebody could test the patches on other
> > hardware as well.
> 
> If you repost with the stuff we talked about fixed, I'll give them a
> spin, but for all I can tell they should be good to go in, it's a huge
> improvement and we can build on that.

Will do that soon. However, I still have an issue that sometimes
the connection is stuck and I'm still unsure if it is related to the
bg scan patches or something else :(

Helmut

Re: [RFC/RFT 0/5] mac80211: implement background scan

[Helmut Schaa]

Am Donnerstag, 16. Juli 2009 schrieb Helmut Schaa:
> Am Donnerstag, 16. Juli 2009 schrieb Johannes Berg:
> > On Thu, 2009-07-16 at 11:04 +0200, Helmut Schaa wrote:
> > 
> > > So, it would be great if somebody could test the patches on other
> > > hardware as well.
> > 
> > If you repost with the stuff we talked about fixed, I'll give them a
> > spin, but for all I can tell they should be good to go in, it's a huge
> > improvement and we can build on that.
> 
> Will do that soon. However, I still have an issue that sometimes
> the connection is stuck and I'm still unsure if it is related to the
> bg scan patches or something else :(

Ok, this is also reproducible without the bg scan patches by just using
sw scan together with iwlagn. Sometimes (I still have no clue under which
circumstances) just after a scan the connection is stuck while iwconfig
still shows association and also the signal levels are still updated.

Helmut