* Latest wireless-testing has broken scan locking.
@ 2010-10-07 4:13 Ben Greear
2010-10-07 8:34 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Ben Greear @ 2010-10-07 4:13 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
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;
}
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Latest wireless-testing has broken scan locking. 2010-10-07 4:13 Latest wireless-testing has broken scan locking Ben Greear @ 2010-10-07 8:34 ` Johannes Berg 2010-10-07 9:18 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2010-10-07 8:34 UTC (permalink / raw) To: Ben Greear; +Cc: linux-wireless@vger.kernel.org, Stanislaw Gruszka On Wed, 2010-10-06 at 21:13 -0700, Ben Greear wrote: > 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; > } 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(-) --- wireless-testing.orig/net/mac80211/scan.c 2010-10-07 10:27:37.000000000 +0200 +++ wireless-testing/net/mac80211/scan.c 2010-10-07 10:31:19.000000000 +0200 @@ -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; out_complete: ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Latest wireless-testing has broken scan locking. 2010-10-07 8:34 ` Johannes Berg @ 2010-10-07 9:18 ` Stanislaw Gruszka 2010-10-07 9:24 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2010-10-07 9:18 UTC (permalink / raw) To: Johannes Berg; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, Oct 07, 2010 at 10:34:48AM +0200, Johannes Berg wrote: > On Wed, 2010-10-06 at 21:13 -0700, Ben Greear wrote: > > 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; > > } > > So much for trusting Stanislaw :-( I can reproduce this in hwsim. > Stanislaw, did you ever test software scan at all? No, sorry. > Below patch fixes this. > > johannes > > --- > net/mac80211/scan.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > --- wireless-testing.orig/net/mac80211/scan.c 2010-10-07 10:27:37.000000000 +0200 > +++ wireless-testing/net/mac80211/scan.c 2010-10-07 10:31:19.000000000 +0200 > @@ -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; > > out_complete: We never keep locking inside do { } while (next_delay == 0) loop, perhaps below patch is something better: diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index 2c79b83..615a831 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -693,6 +693,7 @@ void ieee80211_scan_work(struct work_struct *work) case SCAN_DECISION: /* if no more bands/channels left, complete scan */ if (local->scan_channel_idx >= local->scan_req->n_channels) { + mutex_lock(&local->mtx); aborted = false; goto out_complete; } If so, I will post it officially shortly, if not please proceed with your patch. Sorry again Stanislaw ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: Latest wireless-testing has broken scan locking. 2010-10-07 9:18 ` Stanislaw Gruszka @ 2010-10-07 9:24 ` Johannes Berg 2010-10-07 9:38 ` Stanislaw Gruszka 0 siblings, 1 reply; 6+ messages in thread From: Johannes Berg @ 2010-10-07 9:24 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, 2010-10-07 at 11:18 +0200, Stanislaw Gruszka wrote: > > --- wireless-testing.orig/net/mac80211/scan.c 2010-10-07 10:27:37.000000000 +0200 > > +++ wireless-testing/net/mac80211/scan.c 2010-10-07 10:31:19.000000000 +0200 > > @@ -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; > > > > 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? johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Latest wireless-testing has broken scan locking. 2010-10-07 9:24 ` Johannes Berg @ 2010-10-07 9:38 ` Stanislaw Gruszka 2010-10-07 9:38 ` Johannes Berg 0 siblings, 1 reply; 6+ messages in thread From: Stanislaw Gruszka @ 2010-10-07 9:38 UTC (permalink / raw) To: Johannes Berg; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, Oct 07, 2010 at 11:24:46AM +0200, Johannes Berg wrote: > On Thu, 2010-10-07 at 11:18 +0200, Stanislaw Gruszka wrote: > > > > --- wireless-testing.orig/net/mac80211/scan.c 2010-10-07 10:27:37.000000000 +0200 > > > +++ wireless-testing/net/mac80211/scan.c 2010-10-07 10:31:19.000000000 +0200 > > > @@ -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; > > > > > > 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? :) Yep. > 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). Stanislaw ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Latest wireless-testing has broken scan locking. 2010-10-07 9:38 ` Stanislaw Gruszka @ 2010-10-07 9:38 ` Johannes Berg 0 siblings, 0 replies; 6+ messages in thread From: Johannes Berg @ 2010-10-07 9:38 UTC (permalink / raw) To: Stanislaw Gruszka; +Cc: Ben Greear, linux-wireless@vger.kernel.org On Thu, 2010-10-07 at 11:38 +0200, Stanislaw Gruszka wrote: > > 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). I've tested it on hwsim already, but you won't run into this code path at all with hw scan. johannes ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-10-07 9:38 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-07 4:13 Latest wireless-testing has broken scan locking Ben Greear 2010-10-07 8:34 ` Johannes Berg 2010-10-07 9:18 ` Stanislaw Gruszka 2010-10-07 9:24 ` Johannes Berg 2010-10-07 9:38 ` Stanislaw Gruszka 2010-10-07 9:38 ` Johannes Berg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).