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

[Johannes Berg <johannes@sipsolutions.net>]

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