[PATCH 0/3] some scan patches

[PATCH 0/3] some scan patches

[Johannes Berg]

Been wanting to do this for a while,
mostly for code complexity reasons.

johannes

[PATCH 1/3] mac80211: remove unused scan expire define

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Since cfg80211 manages the BSS list completely,
this define hasn't been used for a long time
and will never be used again.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/ieee80211_i.h |    6 ------
 1 file changed, 6 deletions(-)

--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2010-08-26 13:10:20.000000000 +0200
+++ wireless-testing/net/mac80211/ieee80211_i.h	2010-08-26 13:10:24.000000000 +0200
@@ -50,12 +50,6 @@ struct ieee80211_local;
  * increased memory use (about 2 kB of RAM per entry). */
 #define IEEE80211_FRAGMENT_MAX 4
 
-/*
- * Time after which we ignore scan results and no longer report/use
- * them in any way.
- */
-#define IEEE80211_SCAN_RESULT_EXPIRE (10 * HZ)
-
 #define TU_TO_EXP_TIME(x)	(jiffies + usecs_to_jiffies((x) * 1024))
 
 #define IEEE80211_DEFAULT_UAPSD_QUEUES \

[PATCH 2/3] mac80211: allow scan to complete from any context

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The ieee80211_scan_completed() function was a frequent
source of potential deadlocks, since it is called by
drivers but may call back into drivers, so drivers had
to make sure to call it without any locks held, which
frequently lead to more complex code in drivers. Avoid
that problem by allowing the function to be called in
any context, and queueing the actual work it does.
Also update the documentation for it to indicate this.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h     |    3 ++-
 net/mac80211/ieee80211_i.h |    6 ++++++
 net/mac80211/scan.c        |   34 ++++++++++++++++++++++++++--------
 3 files changed, 34 insertions(+), 9 deletions(-)

--- wireless-testing.orig/net/mac80211/scan.c	2010-08-26 13:29:57.000000000 +0200
+++ wireless-testing/net/mac80211/scan.c	2010-08-26 13:30:01.000000000 +0200
@@ -249,13 +249,11 @@ static bool ieee80211_prep_hw_scan(struc
 	return true;
 }
 
-void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
+static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
 {
 	struct ieee80211_local *local = hw_to_local(hw);
 	bool was_hw_scan;
 
-	trace_api_scan_completed(local, aborted);
-
 	mutex_lock(&local->mtx);
 
 	/*
@@ -313,6 +311,18 @@ void ieee80211_scan_completed(struct iee
 	ieee80211_mesh_notify_scan_completed(local);
 	ieee80211_queue_work(&local->hw, &local->work_work);
 }
+
+void ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted)
+{
+	struct ieee80211_local *local = hw_to_local(hw);
+
+	trace_api_scan_completed(local, aborted);
+
+	set_bit(SCAN_COMPLETED, &local->scanning);
+	if (aborted)
+		set_bit(SCAN_ABORTED, &local->scanning);
+	ieee80211_queue_delayed_work(&local->hw, &local->scan_work, 0);
+}
 EXPORT_SYMBOL(ieee80211_scan_completed);
 
 static int ieee80211_start_sw_scan(struct ieee80211_local *local)
@@ -450,7 +460,7 @@ static int ieee80211_scan_state_decision
 
 	/* 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);
+		__ieee80211_scan_completed(&local->hw, false);
 		return 1;
 	}
 
@@ -642,6 +652,14 @@ void ieee80211_scan_work(struct work_str
 	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
 	unsigned long next_delay = 0;
 
+	if (test_and_clear_bit(SCAN_COMPLETED, &local->scanning)) {
+		bool aborted;
+
+		aborted = test_and_clear_bit(SCAN_ABORTED, &local->scanning);
+		__ieee80211_scan_completed(&local->hw, aborted);
+		return;
+	}
+
 	mutex_lock(&local->mtx);
 	if (!sdata || !local->scan_req) {
 		mutex_unlock(&local->mtx);
@@ -652,7 +670,7 @@ void ieee80211_scan_work(struct work_str
 		int rc = drv_hw_scan(local, sdata, local->hw_scan_req);
 		mutex_unlock(&local->mtx);
 		if (rc)
-			ieee80211_scan_completed(&local->hw, true);
+			__ieee80211_scan_completed(&local->hw, true);
 		return;
 	}
 
@@ -667,7 +685,7 @@ void ieee80211_scan_work(struct work_str
 		mutex_unlock(&local->mtx);
 
 		if (rc)
-			ieee80211_scan_completed(&local->hw, true);
+			__ieee80211_scan_completed(&local->hw, true);
 		return;
 	}
 
@@ -677,7 +695,7 @@ void ieee80211_scan_work(struct work_str
 	 * Avoid re-scheduling when the sdata is going away.
 	 */
 	if (!ieee80211_sdata_running(sdata)) {
-		ieee80211_scan_completed(&local->hw, true);
+		__ieee80211_scan_completed(&local->hw, true);
 		return;
 	}
 
@@ -784,5 +802,5 @@ void ieee80211_scan_cancel(struct ieee80
 	mutex_unlock(&local->mtx);
 
 	if (abortscan)
-		ieee80211_scan_completed(&local->hw, true);
+		__ieee80211_scan_completed(&local->hw, true);
 }
--- wireless-testing.orig/net/mac80211/ieee80211_i.h	2010-08-26 13:30:00.000000000 +0200
+++ wireless-testing/net/mac80211/ieee80211_i.h	2010-08-26 13:30:01.000000000 +0200
@@ -596,11 +596,17 @@ enum queue_stop_reason {
  *	determine if we are on the operating channel or not
  * @SCAN_OFF_CHANNEL: We're off our operating channel for scanning,
  *	gets only set in conjunction with SCAN_SW_SCANNING
+ * @SCAN_COMPLETED: Set for our scan work function when the driver reported
+ *	that the scan completed.
+ * @SCAN_ABORTED: Set for our scan work function when the driver reported
+ *	a scan complete for an aborted scan.
  */
 enum {
 	SCAN_SW_SCANNING,
 	SCAN_HW_SCANNING,
 	SCAN_OFF_CHANNEL,
+	SCAN_COMPLETED,
+	SCAN_ABORTED,
 };
 
 /**
--- wireless-testing.orig/include/net/mac80211.h	2010-08-26 13:29:58.000000000 +0200
+++ wireless-testing/include/net/mac80211.h	2010-08-26 13:30:01.000000000 +0200
@@ -2268,7 +2268,8 @@ void ieee80211_wake_queues(struct ieee80
  *
  * When hardware scan offload is used (i.e. the hw_scan() callback is
  * assigned) this function needs to be called by the driver to notify
- * mac80211 that the scan finished.
+ * mac80211 that the scan finished. This function can be called from
+ * any context, including hardirq context.
  *
  * @hw: the hardware that finished the scan
  * @aborted: set to true if scan was aborted

[PATCH 3/3] wl12xx: remove unneeded locking

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

With the scan callback now being callable from
any context, these unlocks/locks can go away.
This makes the code easier to understand, since
callers of these functions must no longer be
aware that the mutex may be dropped.

As Stanislaw is working on iwlwifi scanning, I
didn't change it to take advantage of the new
mac80211 semantics.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/net/wireless/wl12xx/wl1251_event.c |    2 --
 drivers/net/wireless/wl12xx/wl1251_main.c  |    2 --
 drivers/net/wireless/wl12xx/wl1271_main.c  |    2 --
 drivers/net/wireless/wl12xx/wl1271_scan.c  |    2 --
 4 files changed, 8 deletions(-)

--- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1251_event.c	2010-08-26 13:29:57.000000000 +0200
+++ wireless-testing/drivers/net/wireless/wl12xx/wl1251_event.c	2010-08-26 13:30:03.000000000 +0200
@@ -36,9 +36,7 @@ static int wl1251_event_scan_complete(st
 		     mbox->scheduled_scan_channels);
 
 	if (wl->scanning) {
-		mutex_unlock(&wl->mutex);
 		ieee80211_scan_completed(wl->hw, false);
-		mutex_lock(&wl->mutex);
 		wl1251_debug(DEBUG_MAC80211, "mac80211 hw scan completed");
 		wl->scanning = false;
 	}
--- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1251_main.c	2010-08-26 13:29:57.000000000 +0200
+++ wireless-testing/drivers/net/wireless/wl12xx/wl1251_main.c	2010-08-26 13:30:03.000000000 +0200
@@ -469,9 +469,7 @@ static void wl1251_op_stop(struct ieee80
 	WARN_ON(wl->state != WL1251_STATE_ON);
 
 	if (wl->scanning) {
-		mutex_unlock(&wl->mutex);
 		ieee80211_scan_completed(wl->hw, true);
-		mutex_lock(&wl->mutex);
 		wl->scanning = false;
 	}
 
--- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1271_main.c	2010-08-26 13:29:57.000000000 +0200
+++ wireless-testing/drivers/net/wireless/wl12xx/wl1271_main.c	2010-08-26 13:30:03.000000000 +0200
@@ -948,9 +948,7 @@ static void wl1271_op_remove_interface(s
 		ieee80211_enable_dyn_ps(wl->vif);
 
 	if (wl->scan.state != WL1271_SCAN_STATE_IDLE) {
-		mutex_unlock(&wl->mutex);
 		ieee80211_scan_completed(wl->hw, true);
-		mutex_lock(&wl->mutex);
 		wl->scan.state = WL1271_SCAN_STATE_IDLE;
 		kfree(wl->scan.scanned_ch);
 		wl->scan.scanned_ch = NULL;
--- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1271_scan.c	2010-08-26 13:29:57.000000000 +0200
+++ wireless-testing/drivers/net/wireless/wl12xx/wl1271_scan.c	2010-08-26 13:30:03.000000000 +0200
@@ -215,9 +215,7 @@ void wl1271_scan_stm(struct wl1271 *wl)
 		break;
 
 	case WL1271_SCAN_STATE_DONE:
-		mutex_unlock(&wl->mutex);
 		ieee80211_scan_completed(wl->hw, false);
-		mutex_lock(&wl->mutex);
 
 		kfree(wl->scan.scanned_ch);
 		wl->scan.scanned_ch = NULL;

Re: [PATCH 3/3] wl12xx: remove unneeded locking

[Kalle Valo]

Johannes Berg <johannes@sipsolutions.net> writes:

> From: Johannes Berg <johannes.berg@intel.com>
>
> With the scan callback now being callable from
> any context, these unlocks/locks can go away.
> This makes the code easier to understand, since
> callers of these functions must no longer be
> aware that the mutex may be dropped.
>
> As Stanislaw is working on iwlwifi scanning, I
> didn't change it to take advantage of the new
> mac80211 semantics.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/net/wireless/wl12xx/wl1251_event.c |    2 --
>  drivers/net/wireless/wl12xx/wl1251_main.c  |    2 --

Thanks, wl1251 part looks good. I didn't test them yet, but will do
soon.

-- 
Kalle Valo

Re: [PATCH 3/3] wl12xx: remove unneeded locking

[Luciano Coelho]

On Thu, 2010-08-26 at 13:30 +0200, ext Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> With the scan callback now being callable from
> any context, these unlocks/locks can go away.
> This makes the code easier to understand, since
> callers of these functions must no longer be
> aware that the mutex may be dropped.
> 
> As Stanislaw is working on iwlwifi scanning, I
> didn't change it to take advantage of the new
> mac80211 semantics.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---

[...]

> --- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1271_main.c	2010-08-26 13:29:57.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/wl12xx/wl1271_main.c	2010-08-26 13:30:03.000000000 +0200
> @@ -948,9 +948,7 @@ static void wl1271_op_remove_interface(s
>  		ieee80211_enable_dyn_ps(wl->vif);
>  
>  	if (wl->scan.state != WL1271_SCAN_STATE_IDLE) {
> -		mutex_unlock(&wl->mutex);
>  		ieee80211_scan_completed(wl->hw, true);
> -		mutex_lock(&wl->mutex);
>  		wl->scan.state = WL1271_SCAN_STATE_IDLE;
>  		kfree(wl->scan.scanned_ch);
>  		wl->scan.scanned_ch = NULL;
> --- wireless-testing.orig/drivers/net/wireless/wl12xx/wl1271_scan.c	2010-08-26 13:29:57.000000000 +0200
> +++ wireless-testing/drivers/net/wireless/wl12xx/wl1271_scan.c	2010-08-26 13:30:03.000000000 +0200
> @@ -215,9 +215,7 @@ void wl1271_scan_stm(struct wl1271 *wl)
>  		break;
>  
>  	case WL1271_SCAN_STATE_DONE:
> -		mutex_unlock(&wl->mutex);
>  		ieee80211_scan_completed(wl->hw, false);
> -		mutex_lock(&wl->mutex);
>  
>  		kfree(wl->scan.scanned_ch);
>  		wl->scan.scanned_ch = NULL;

Yes, the wl1271 part also looks good.  This definitely simplifies things
and makes the code more robust.


-- 
Cheers,
Luca.