Latest wireless-testing has broken scan locking.

Unless I really screwed up a merge in a strange way, the ieee80211_scan_work is broken.

It calls mutex_unlock(&local->mtx) too many times when it hits this code:

		case SCAN_DECISION:
			/* if no more bands/channels left, complete scan */
			if (local->scan_channel_idx >= local->scan_req->n_channels) {
				aborted = false;
				goto out_complete;
			}

Quoted 6 lines · Ben Greear

Unless I really screwed up a merge in a strange way, the ieee80211_scan_work is broken.

It calls mutex_unlock(&local->mtx) too many times when it hits this code:

Quoted 6 lines

		case SCAN_DECISION:
			/* if no more bands/channels left, complete scan */
			if (local->scan_channel_idx >= local->scan_req->n_channels) {
				aborted = false;
				goto out_complete;
			}

So much for trusting Stanislaw :-( I can reproduce this in hwsim. Stanislaw, did you ever test software scan at all?

Below patch fixes this.

johannes

---
 net/mac80211/scan.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

net/mac80211/scan.c +0 -2

@@ -693,8 +693,6 @@ void ieee80211_scan_work(struct work_str
 		goto out_complete;
 	}
 
-	mutex_unlock(&local->mtx);
-
 	/*
 	 * as long as no delay is required advance immediately
 	 * without scheduling a new work

Quoted 14 lines

net/mac80211/scan.c +0 -2

@@ -693,8 +693,6 @@ void ieee80211_scan_work(struct work_str
 		goto out_complete;
 	}
 
-	mutex_unlock(&local->mtx);
-
 	/*
 	 * as long as no delay is required advance immediately
 	 * without scheduling a new work

@@ -725,6 +723,7 @@ void ieee80211_scan_work(struct work_str
 	} while (next_delay == 0);
 

net/mac80211/scan.c void ieee80211_scan_work(struct work_str · line 729

Quoted 5 lines

net/mac80211/scan.c +1 -0

 	ieee80211_queue_delayed_work(&local->hw, &local->scan_work, next_delay);
+	mutex_unlock(&local->mtx);
 	return;
 
 out_complete:

We never keep locking inside do { } while (next_delay == 0) loop, perhaps below patch is something better:

Quoted 18 lines · Stanislaw Gruszka

Quoted 18 lines

net/mac80211/scan.c +1 -2

@@ -693,8 +693,6 @@ void ieee80211_scan_work(struct work_str
 		goto out_complete;
 	}
 
-	mutex_unlock(&local->mtx);
-
 	/*
 	 * as long as no delay is required advance immediately
 	 * without scheduling a new work

@@ -725,6 +723,7 @@ void ieee80211_scan_work(struct work_str
 	} while (next_delay == 0);
 
 	ieee80211_queue_delayed_work(&local->hw, &local->scan_work, next_delay);
+	mutex_unlock(&local->mtx);
 	return;
 

Quoted 4 lines

net/mac80211/scan.c void ieee80211_scan_work(struct work_str · line 729

Quoted 1 line

net/mac80211/scan.c +0 -0

 out_complete:

We never keep locking inside do { } while (next_delay == 0) loop, perhaps below patch is something better:

Yeah, we didn't have that before, but weren't you yourself arguing that dropping and re-acquiring could lead to issues? :)

FWIW, I've looked at the code and all we do is possibly acquire the iflist_mtx (which is fine under local->mtx) and do some driver calls, which should also be fine.

So what do you think?

Quoted 5 lines

FWIW, I've looked at the code and all we do is possibly acquire the iflist_mtx (which is fine under local->mtx) and do some driver calls, which should also be fine.

So what do you think?

Let's fix with your patch. I'm going to test it (on hw scan :/ at least).