[PATCH] mac80211: fix deferred hardware scan requests

[PATCH] mac80211: fix deferred hardware scan requests

[Reinette Chatre]

From: Reinette Chatre <reinette.chatre@intel.com>

mac80211 will defer the handling of scan requests if it is busy
with management work at the time. The scan requests are
deferred and run after the work has completed. When this occurs
there are currently two problems.

* The scan request for hardware scan is not fully populated with
  the band and channels to scan not initialized.
* When the scan is queued the state is not correctly updated to
  reflect that a scan is in progress. The problem here is that when
  the driver completes the scan and calls ieee80211_scan_completed()
  a warning will be triggered since mac80211 was not aware that a scan
  was in progress.

Both issues are fixed here by first ensuring that scan request is fully
initialized before it is queued and next by setting the scanning state
correctly before it is queued for execution.

Signed-off-by: Reinette Chatre <reinette.chatre@intel.com>
---
* I would like to confirm one change I made in ieee80211_work_work. From
  what I can tell software scanning does not expect the scanning state to
  be set in ieee80211_scan_work, but hardware scanning does. Is this
  correct?

* This is a proposed fix for
  http://bugzilla.kernel.org/show_bug.cgi?id=15119, which is tracked as a
  regression. The user is having some system problems and is not able to
  test it.

* This is an important fix to get in, otherwise wireless will become a top
  performer on kerneloops.org.

 net/mac80211/scan.c |    8 +++++---
 net/mac80211/work.c |    9 ++++++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index bc061f6..9cde4a8 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -369,6 +369,9 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	local->scan_req = req;
 	local->scan_sdata = sdata;
 
+	if (local->ops->hw_scan)
+		WARN_ON(!ieee80211_prep_hw_scan(local));
+
 	if (!list_empty(&local->work_list)) {
 		/* wait for the work to finish/time out */
 		return 0;
@@ -392,10 +395,9 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	ieee80211_recalc_idle(local);
 	mutex_unlock(&local->scan_mtx);
 
-	if (local->ops->hw_scan) {
-		WARN_ON(!ieee80211_prep_hw_scan(local));
+	if (local->ops->hw_scan)
 		rc = drv_hw_scan(local, local->hw_scan_req);
-	} else
+	else
 		rc = ieee80211_start_sw_scan(local);
 
 	mutex_lock(&local->scan_mtx);
diff --git a/net/mac80211/work.c b/net/mac80211/work.c
index 7e708d5..e4c3fd4 100644
--- a/net/mac80211/work.c
+++ b/net/mac80211/work.c
@@ -918,10 +918,17 @@ static void ieee80211_work_work(struct work_struct *work)
 		run_again(local, jiffies + HZ/2);
 	}
 
-	if (list_empty(&local->work_list) && local->scan_req)
+	if (list_empty(&local->work_list) && local->scan_req) {
+		if (local->ops->hw_scan)
+			__set_bit(SCAN_HW_SCANNING, &local->scanning);
+		/*
+		 * For software scanning ieee80211_scan_work expects
+		 * to be called without local->scanning set.
+		 */
 		ieee80211_queue_delayed_work(&local->hw,
 					     &local->scan_work,
 					     round_jiffies_relative(0));
+	}
 
 	mutex_unlock(&local->work_mtx);
 
-- 
1.6.3.3

Re: [PATCH] mac80211: fix deferred hardware scan requests

[Johannes Berg]

On Tue, 2010-02-02 at 10:54 -0800, Reinette Chatre wrote:

> mac80211 will defer the handling of scan requests if it is busy
> with management work at the time. The scan requests are
> deferred and run after the work has completed. When this occurs
> there are currently two problems.
> 
> * The scan request for hardware scan is not fully populated with
>   the band and channels to scan not initialized.
> * When the scan is queued the state is not correctly updated to
>   reflect that a scan is in progress. The problem here is that when
>   the driver completes the scan and calls ieee80211_scan_completed()
>   a warning will be triggered since mac80211 was not aware that a scan
>   was in progress.

Good analysis, thanks.

> Both issues are fixed here by first ensuring that scan request is fully
> initialized before it is queued and next by setting the scanning state
> correctly before it is queued for execution.

> * I would like to confirm one change I made in ieee80211_work_work. From
>   what I can tell software scanning does not expect the scanning state to
>   be set in ieee80211_scan_work, but hardware scanning does. Is this
>   correct?

I think so, but I kinda think it makes that code rely too much on the
internals.

> * This is a proposed fix for
>   http://bugzilla.kernel.org/show_bug.cgi?id=15119, which is tracked as a
>   regression. The user is having some system problems and is not able to
>   test it.
> 
> * This is an important fix to get in, otherwise wireless will become a top
>   performer on kerneloops.org.

Well said ;)

Thanks to your analysis of the problem, and the remain-on-channel work,
I was able to reproduce the problem, and the following seems to fix it
for me; what do you think?

johannes


--- wireless-testing.orig/net/mac80211/scan.c	2010-02-02 20:37:28.000000000 +0100
+++ wireless-testing/net/mac80211/scan.c	2010-02-02 20:39:35.000000000 +0100
@@ -345,6 +345,14 @@ static int __ieee80211_start_scan(struct
 	if (local->scan_req)
 		return -EBUSY;
 
+	local->scan_req = req;
+	local->scan_sdata = sdata;
+
+	if (!list_empty(&local->work_list)) {
+		/* wait for the work to finish/time out */
+		return 0;
+	}
+
 	if (local->ops->hw_scan) {
 		u8 *ies;
 
@@ -353,8 +361,11 @@ static int __ieee80211_start_scan(struct
 				req->n_channels * sizeof(req->channels[0]) +
 				2 + IEEE80211_MAX_SSID_LEN + local->scan_ies_len +
 				req->ie_len, GFP_KERNEL);
-		if (!local->hw_scan_req)
+		if (!local->hw_scan_req) {
+			local->scan_req = NULL;
+			local->scan_sdata = NULL;
 			return -ENOMEM;
+		}
 
 		local->hw_scan_req->ssids = req->ssids;
 		local->hw_scan_req->n_ssids = req->n_ssids;
@@ -366,14 +377,6 @@ static int __ieee80211_start_scan(struct
 		local->hw_scan_band = 0;
 	}
 
-	local->scan_req = req;
-	local->scan_sdata = sdata;
-
-	if (!list_empty(&local->work_list)) {
-		/* wait for the work to finish/time out */
-		return 0;
-	}
-
 	if (local->ops->hw_scan)
 		__set_bit(SCAN_HW_SCANNING, &local->scanning);
 	else

Re: [PATCH] mac80211: fix deferred hardware scan requests

[reinette chatre]

Hi Johannes,

On Tue, 2010-02-02 at 11:49 -0800, Johannes Berg wrote:
> I was able to reproduce the problem, and the following seems to fix it
> for me; what do you think?

Thank you very much. This looks good, much cleaner. The code touched
here changed since 2.6.33 ... could you please create a 2.6.33 version
also?

Reinette

[PATCH] mac80211: fix deferred hardware scan requests

[Johannes Berg]

Reinette found the reason for the warnings that
happened occasionally when a hw-offloaded scan
finished; her description of the problem:

  mac80211 will defer the handling of scan requests if it is
  busy with management work at the time. The scan requests
  are deferred and run after the work has completed. When
  this occurs there are currently two problems.

  * The scan request for hardware scan is not fully populated
    with the band and channels to scan not initialized.

  * When the scan is queued the state is not correctly updated
    to reflect that a scan is in progress. The problem here is
    that when the driver completes the scan and calls
    ieee80211_scan_completed() a warning will be triggered
    since mac80211 was not aware that a scan was in progress.

The reason is that the queued scan work will start
the hw scan right away when the hw_scan_req struct
has already been allocated. However, in the first
pass it will not have been filled, which happens
at the same time as setting the bits. To fix this,
simply move the allocation after the pending work
test as well, so that the first iteration of the
scan work will call __ieee80211_start_scan() even
in the hardware scan case.

Also, fix a wrong locking comment that I noticed,
a dependency between those two mutexes exists but
others are potentially avoided (especially with
driver locks).

Bug-identified-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
 net/mac80211/scan.c |   27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

--- wireless-testing.orig/net/mac80211/scan.c	2010-02-03 09:53:28.000000000 +0100
+++ wireless-testing/net/mac80211/scan.c	2010-02-03 10:05:59.000000000 +0100
@@ -345,6 +345,13 @@ static int __ieee80211_start_scan(struct
 	if (local->scan_req)
 		return -EBUSY;
 
+	if (!list_empty(&local->work_list)) {
+		/* wait for the work to finish/time out */
+		local->scan_req = req;
+		local->scan_sdata = sdata;
+		return 0;
+	}
+
 	if (local->ops->hw_scan) {
 		u8 *ies;
 
@@ -364,29 +371,33 @@ static int __ieee80211_start_scan(struct
 		local->hw_scan_req->ie = ies;
 
 		local->hw_scan_band = 0;
+
+		/*
+		 * After allocating local->hw_scan_req, we must
+		 * go through until ieee80211_prep_hw_scan(), so
+		 * anything that might be changed here and leave
+		 * this function early must not go after this
+		 * allocation.
+		 */
 	}
 
 	local->scan_req = req;
 	local->scan_sdata = sdata;
 
-	if (!list_empty(&local->work_list)) {
-		/* wait for the work to finish/time out */
-		return 0;
-	}
-
 	if (local->ops->hw_scan)
 		__set_bit(SCAN_HW_SCANNING, &local->scanning);
 	else
 		__set_bit(SCAN_SW_SCANNING, &local->scanning);
+
 	/*
 	 * Kicking off the scan need not be protected,
 	 * only the scan variable stuff, since now
 	 * local->scan_req is assigned and other callers
 	 * will abort their scan attempts.
 	 *
-	 * This avoids getting a scan_mtx -> iflist_mtx
-	 * dependency, so that the scan completed calls
-	 * have more locking freedom.
+	 * This avoids too many locking dependencies
+	 * so that the scan completed calls have more
+	 * locking freedom.
 	 */
 
 	ieee80211_recalc_idle(local);