* 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).