| Message ID | 200907251506.34943.helmut.schaa@googlemail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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