[PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Pavel Roskin]

Move check for the final value of local->scan_channel_idx from
ieee80211_scan_state_decision() to ieee80211_scan_state_set_channel().

Stop the state machine in ieee80211_scan_work() by checking
local->scanning.  Don't return a value from
ieee80211_scan_state_decision().

Signed-off-by: Pavel Roskin <proski@gnu.org>
---
 net/mac80211/scan.c |   22 +++++++++++-----------
 1 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index b376775..8b5a4a3 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -470,18 +470,12 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	return rc;
 }
 
-static int ieee80211_scan_state_decision(struct ieee80211_local *local,
-					 unsigned long *next_delay)
+static void ieee80211_scan_state_decision(struct ieee80211_local *local,
+					  unsigned long *next_delay)
 {
 	bool associated = false;
 	struct ieee80211_sub_if_data *sdata;
 
-	/* if no more bands/channels left, complete scan and advance to the idle state */
-	if (local->scan_channel_idx >= local->scan_req->n_channels) {
-		ieee80211_scan_completed(&local->hw, false);
-		return 1;
-	}
-
 	/* check if at least one STA interface is associated */
 	mutex_lock(&local->iflist_mtx);
 	list_for_each_entry(sdata, &local->interfaces, list) {
@@ -517,7 +511,6 @@ static int ieee80211_scan_state_decision(struct ieee80211_local *local,
 	}
 
 	*next_delay = 0;
-	return 0;
 }
 
 static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
@@ -587,6 +580,12 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,
 	struct ieee80211_channel *chan;
 	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
 
+	/* if no more bands/channels left, complete scan and advance to the idle state */
+	if (local->scan_channel_idx >= local->scan_req->n_channels) {
+		ieee80211_scan_completed(&local->hw, false);
+		return;
+	}
+
 	skip = 0;
 	chan = local->scan_req->channels[local->scan_channel_idx];
 
@@ -695,8 +694,7 @@ void ieee80211_scan_work(struct work_struct *work)
 	do {
 		switch (local->next_scan_state) {
 		case SCAN_DECISION:
-			if (ieee80211_scan_state_decision(local, &next_delay))
-				return;
+			ieee80211_scan_state_decision(local, &next_delay);
 			break;
 		case SCAN_SET_CHANNEL:
 			ieee80211_scan_state_set_channel(local, &next_delay);
@@ -711,6 +709,8 @@ void ieee80211_scan_work(struct work_struct *work)
 			ieee80211_scan_state_enter_oper_channel(local, &next_delay);
 			break;
 		}
+		if (!local->scanning)
+			return;
 	} while (next_delay == 0);
 
 	queue_delayed_work(local->hw.workqueue, &local->scan_work,

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]

On Sat, 2009-07-25 at 01:18 -0400, Pavel Roskin wrote:
> Move check for the final value of local->scan_channel_idx from
> ieee80211_scan_state_decision() to ieee80211_scan_state_set_channel().
> 
> Stop the state machine in ieee80211_scan_work() by checking
> local->scanning.  Don't return a value from
> ieee80211_scan_state_decision().

Helmut, can you please take a look at this? I'm not entirely sure what's
going on.

johannes

> Signed-off-by: Pavel Roskin <proski@gnu.org>
> ---
>  net/mac80211/scan.c |   22 +++++++++++-----------
>  1 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
> index b376775..8b5a4a3 100644
> --- a/net/mac80211/scan.c
> +++ b/net/mac80211/scan.c
> @@ -470,18 +470,12 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
>  	return rc;
>  }
>  
> -static int ieee80211_scan_state_decision(struct ieee80211_local *local,
> -					 unsigned long *next_delay)
> +static void ieee80211_scan_state_decision(struct ieee80211_local *local,
> +					  unsigned long *next_delay)
>  {
>  	bool associated = false;
>  	struct ieee80211_sub_if_data *sdata;
>  
> -	/* if no more bands/channels left, complete scan and advance to the idle state */
> -	if (local->scan_channel_idx >= local->scan_req->n_channels) {
> -		ieee80211_scan_completed(&local->hw, false);
> -		return 1;
> -	}
> -
>  	/* check if at least one STA interface is associated */
>  	mutex_lock(&local->iflist_mtx);
>  	list_for_each_entry(sdata, &local->interfaces, list) {
> @@ -517,7 +511,6 @@ static int ieee80211_scan_state_decision(struct ieee80211_local *local,
>  	}
>  
>  	*next_delay = 0;
> -	return 0;
>  }
>  
>  static void ieee80211_scan_state_leave_oper_channel(struct ieee80211_local *local,
> @@ -587,6 +580,12 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,
>  	struct ieee80211_channel *chan;
>  	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
>  
> +	/* if no more bands/channels left, complete scan and advance to the idle state */
> +	if (local->scan_channel_idx >= local->scan_req->n_channels) {
> +		ieee80211_scan_completed(&local->hw, false);
> +		return;
> +	}
> +
>  	skip = 0;
>  	chan = local->scan_req->channels[local->scan_channel_idx];
>  
> @@ -695,8 +694,7 @@ void ieee80211_scan_work(struct work_struct *work)
>  	do {
>  		switch (local->next_scan_state) {
>  		case SCAN_DECISION:
> -			if (ieee80211_scan_state_decision(local, &next_delay))
> -				return;
> +			ieee80211_scan_state_decision(local, &next_delay);
>  			break;
>  		case SCAN_SET_CHANNEL:
>  			ieee80211_scan_state_set_channel(local, &next_delay);
> @@ -711,6 +709,8 @@ void ieee80211_scan_work(struct work_struct *work)
>  			ieee80211_scan_state_enter_oper_channel(local, &next_delay);
>  			break;
>  		}
> +		if (!local->scanning)
> +			return;
>  	} while (next_delay == 0);
>  
>  	queue_delayed_work(local->hw.workqueue, &local->scan_work,
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Helmut Schaa]

Am Samstag, 25. Juli 2009 schrieb Johannes Berg:
> On Sat, 2009-07-25 at 01:18 -0400, Pavel Roskin wrote:
> > Move check for the final value of local->scan_channel_idx from
> > ieee80211_scan_state_decision() to ieee80211_scan_state_set_channel().
> > 
> > Stop the state machine in ieee80211_scan_work() by checking
> > local->scanning.  Don't return a value from
> > ieee80211_scan_state_decision().
> 
> Helmut, can you please take a look at this? I'm not entirely sure what's
> going on.

That's strange. I cannot reproduce the oops here :(

Investigating ...

Thanks,
Helmut

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Helmut Schaa]

Am Samstag, 25. Juli 2009 schrieb Pavel Roskin:
> Move check for the final value of local->scan_channel_idx from
> ieee80211_scan_state_decision() to ieee80211_scan_state_set_channel().
> 
> Stop the state machine in ieee80211_scan_work() by checking
> local->scanning.  Don't return a value from
> ieee80211_scan_state_decision().

Hmm, I'd prefer to keep the decision state as entry and exit point to
the scan state machine. The patch below should also fix this issue
by returning back to the decision state after every skipped channel.

In the long run I would like to move the channel selection also to
the decision state in order to implement various improvements (like
scanning multiple channels in a row or reordering the channel list).

I was in the meantime able to reproduce the oops by setting an other
regulatory domain. Pavel, Larry, does this patch help you as well? 

Thanks,
Helmut

---
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index b376775..147772a 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -605,8 +605,11 @@ static void ieee80211_scan_state_set_channel(struct ieee80211_local *local,
 	/* advance state machine to next channel/band */
 	local->scan_channel_idx++;
 
-	if (skip)
+	if (skip) {
+		/* if we skip this channel return to the decision state */
+		local->next_scan_state = SCAN_DECISION;
 		return;
+	}
 
 	/*
 	 * Probe delay is used to update the NAV, cf. 11.1.3.2.2

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Pavel Roskin]

On Sat, 2009-07-25 at 15:06 +0200, Helmut Schaa wrote:

> Hmm, I'd prefer to keep the decision state as entry and exit point to
> the scan state machine. The patch below should also fix this issue
> by returning back to the decision state after every skipped channel.

The patch is working and I'm fine with it.  We should fix that in the
tree as soon as possible, or it will stand in the way of bisection.

I forgot to mention that the oops was happening in x86_64 with rt61pci
and US regulatory domain.  The device only works in the 2.4 GHz range.

> In the long run I would like to move the channel selection also to
> the decision state in order to implement various improvements (like
> scanning multiple channels in a row or reordering the channel list).

I don't know the code enough, but two things surprised me:

Lack of SCAN_DONE in mac80211_scan_state.  We exit scanning through the
"entry point".

Use of "unsigned long" for bitwise fields, such as queue_stop_reasons
and scanning.  This reminds me of the good old days where long was
always 32 bit, but int wasn't.  I think "unsigned int" should be enough,
and you can annotate it with __bitwise to make sparse catch some
misuses.

-- 
Regards,
Pavel Roskin

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Helmut Schaa]

Am Samstag, 25. Juli 2009 schrieb Pavel Roskin:
> On Sat, 2009-07-25 at 15:06 +0200, Helmut Schaa wrote:
> 
> > Hmm, I'd prefer to keep the decision state as entry and exit point to
> > the scan state machine. The patch below should also fix this issue
> > by returning back to the decision state after every skipped channel.
> 
> The patch is working and I'm fine with it. 

Great, thanks a lot.

> We should fix that in the 
> tree as soon as possible, or it will stand in the way of bisection.

Yes, you're right.

> I forgot to mention that the oops was happening in x86_64 with rt61pci
> and US regulatory domain.  The device only works in the 2.4 GHz range.
> 
> > In the long run I would like to move the channel selection also to
> > the decision state in order to implement various improvements (like
> > scanning multiple channels in a row or reordering the channel list).
> 
> I don't know the code enough, but two things surprised me:
> 
> Lack of SCAN_DONE in mac80211_scan_state.  We exit scanning through the
> "entry point".

I also thought of a separate state SCAN_DONE or something similar but
dropped that idea as the only thing this state would have to do is the
call to ieee80211_scan_completed. So, once the scan is finished we
just stay in SCAN_DECISION as long as the scan state machine gets poked
again by a start_scan call.

> Use of "unsigned long" for bitwise fields, such as queue_stop_reasons
> and scanning.  This reminds me of the good old days where long was
> always 32 bit, but int wasn't.  I think "unsigned int" should be enough,
> and you can annotate it with __bitwise to make sparse catch some
> misuses.

No objections :)

Helmut

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Helmut Schaa]

Am Samstag, 25. Juli 2009 schrieb Helmut Schaa:
> Am Samstag, 25. Juli 2009 schrieb Pavel Roskin:
> > Lack of SCAN_DONE in mac80211_scan_state.  We exit scanning through the
> > "entry point".
> 
> I also thought of a separate state SCAN_DONE or something similar but
> dropped that idea as the only thing this state would have to do is the
> call to ieee80211_scan_completed. So, once the scan is finished we
> just stay in SCAN_DECISION as long as the scan state machine gets poked
> again by a start_scan call.

Maybe we should add a state SCAN_IDLE which is entered after the scan
finished just for consistency?

Helmut

Re: [PATCH] mac80211: fix oops in ieee80211_scan_state_set_channel()

[Pavel Roskin]

On Sat, 2009-07-25 at 20:15 +0200, Helmut Schaa wrote:

> > Lack of SCAN_DONE in mac80211_scan_state.  We exit scanning through the
> > "entry point".
> 
> I also thought of a separate state SCAN_DONE or something similar but
> dropped that idea as the only thing this state would have to do is the
> call to ieee80211_scan_completed. So, once the scan is finished we
> just stay in SCAN_DECISION as long as the scan state machine gets poked
> again by a start_scan call.

It's just an idea.  I only touched that code because it was failing for
me.

There is some duplication of information between local->scanning and
local->next_scan_state, but it's probably hard to avoid.

> > Use of "unsigned long" for bitwise fields, such as queue_stop_reasons
> > and scanning.  This reminds me of the good old days where long was
> > always 32 bit, but int wasn't.  I think "unsigned int" should be enough,
> > and you can annotate it with __bitwise to make sparse catch some
> > misuses.
> 
> No objections :)

Sorry, it turns out test_bit() wants unsigned long.  I don't quite like
what it does, but I'm not going to rewrite it.

-- 
Regards,
Pavel Roskin