| Message ID | 1420645811-17877-3-git-send-email-eliad@wizery.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, Jan 07, 2015 at 05:50:11PM +0200, Eliad Peller wrote: > Radar detection can last indefinite time. There is no > point in deferring a scan request in this case - simply > return -EBUSY. > diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c > @@ -505,7 +505,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, > - if (local->scan_req) > + if (local->scan_req || ieee80211_is_radar_required(local)) > return -EBUSY; This seems to be breaking a hwsim test case sequence of ap_vht80plus80 followed by ap_vht80. In such a case, all the HT40 scans for ap_vht80 fail due to this added condition resulting in -EBUSY being returned every time. It looks like this happens even if I change ap_vht80 to use the same country code (US) as ap_vht80plus80, so the change in the country code does not explain this either. I'm not sure what is causing the issue here, but it looks like something in ap_vht80plus80 (i.e., an attempt to enable a channel combination that would require DFS on one of the 80 MHz segments) leaves behind state that makes ieee80211_is_radar_required(local) return true even when it shouldn't. DFS for 80+80 is not yet supported, so I'd assume this is somehow related to that. Anyway, I don't think mac80211 should behave in this way.
On 7 February 2015 at 12:57, Jouni Malinen <j@w1.fi> wrote: > On Wed, Jan 07, 2015 at 05:50:11PM +0200, Eliad Peller wrote: >> Radar detection can last indefinite time. There is no >> point in deferring a scan request in this case - simply >> return -EBUSY. > >> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c >> @@ -505,7 +505,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, >> - if (local->scan_req) >> + if (local->scan_req || ieee80211_is_radar_required(local)) >> return -EBUSY; > > This seems to be breaking a hwsim test case sequence of ap_vht80plus80 > followed by ap_vht80. In such a case, all the HT40 scans for ap_vht80 > fail due to this added condition resulting in -EBUSY being returned > every time. It looks like this happens even if I change ap_vht80 to use > the same country code (US) as ap_vht80plus80, so the change in the > country code does not explain this either. > > I'm not sure what is causing the issue here, but it looks like something > in ap_vht80plus80 (i.e., an attempt to enable a channel combination that > would require DFS on one of the 80 MHz segments) leaves behind state > that makes ieee80211_is_radar_required(local) return true even when it > shouldn't. DFS for 80+80 is not yet supported, so I'd assume this is > somehow related to that. Anyway, I don't think mac80211 should behave in > this way. > BTW, what in case we will start AP on first interface (DFS channel), on second one we will try to connect to other AP. As I understand this correctly, second iface (STA iface) will not allow to scan (connect) ...? Other case, what if we start DFS AP and allow to scan in AP mode (eg for ACS purpose, allow to choose better channel and do CSA ...)? BR Janusz -- 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
Hi Jouni, On Sat, Feb 7, 2015 at 1:57 PM, Jouni Malinen <j@w1.fi> wrote: > On Wed, Jan 07, 2015 at 05:50:11PM +0200, Eliad Peller wrote: >> Radar detection can last indefinite time. There is no >> point in deferring a scan request in this case - simply >> return -EBUSY. > >> diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c >> @@ -505,7 +505,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, >> - if (local->scan_req) >> + if (local->scan_req || ieee80211_is_radar_required(local)) >> return -EBUSY; > > This seems to be breaking a hwsim test case sequence of ap_vht80plus80 > followed by ap_vht80. In such a case, all the HT40 scans for ap_vht80 > fail due to this added condition resulting in -EBUSY being returned > every time. It looks like this happens even if I change ap_vht80 to use > the same country code (US) as ap_vht80plus80, so the change in the > country code does not explain this either. > > I'm not sure what is causing the issue here, but it looks like something > in ap_vht80plus80 (i.e., an attempt to enable a channel combination that > would require DFS on one of the 80 MHz segments) leaves behind state > that makes ieee80211_is_radar_required(local) return true even when it > shouldn't. DFS for 80+80 is not yet supported, so I'd assume this is > somehow related to that. Anyway, I don't think mac80211 should behave in > this way. > Thanks for the detailed report. There was indeed stale state. I've just sent a patch to fix it. I don't see why returning EBUSY is wrong, though. Actually, it just make the test fail immediately, instead of waiting indefinitely until a timeout occurs (i guess, didn't actually test it with the reverted patch). This was exactly the intended behavior, and i think it makes much more sense. Eliad. -- 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, Feb 7, 2015 at 9:17 PM, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: > BTW, what in case we will start AP on first interface (DFS channel), > on second one we will try to connect to other AP. As I understand this > correctly, second iface (STA iface) will not allow to scan (connect) > ...? > right. The rationale behind it that you must listen constantly to detect radar events, which prevents scanning off-channel. (i guess an exception for on-channel scanning can be added if needed, though) > Other case, what if we start DFS AP and allow to scan in AP mode (eg > for ACS purpose, allow to choose better channel and do CSA ...)? ditto. you just have to make sure you keep listening on the operating channel. Eliad. -- 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 8 February 2015 at 11:57, Eliad Peller <eliad@wizery.com> wrote: > On Sat, Feb 7, 2015 at 9:17 PM, Janusz Dziedzic > <janusz.dziedzic@tieto.com> wrote: >> BTW, what in case we will start AP on first interface (DFS channel), >> on second one we will try to connect to other AP. As I understand this >> correctly, second iface (STA iface) will not allow to scan (connect) >> ...? >> > right. > The rationale behind it that you must listen constantly to detect > radar events, which prevents scanning off-channel. > (i guess an exception for on-channel scanning can be added if needed, though) > Still I can image hw with hw_scan() support that could have dedicated "hw part" for scanning. So, driver should made this decision (allow or not) base on hw features. BR Janusz >> Other case, what if we start DFS AP and allow to scan in AP mode (eg >> for ACS purpose, allow to choose better channel and do CSA ...)? > ditto. you just have to make sure you keep listening on the operating channel. > > Eliad. -- 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 Sun, Feb 8, 2015 at 3:17 PM, Janusz Dziedzic <janusz.dziedzic@tieto.com> wrote: > On 8 February 2015 at 11:57, Eliad Peller <eliad@wizery.com> wrote: >> On Sat, Feb 7, 2015 at 9:17 PM, Janusz Dziedzic >> <janusz.dziedzic@tieto.com> wrote: >>> BTW, what in case we will start AP on first interface (DFS channel), >>> on second one we will try to connect to other AP. As I understand this >>> correctly, second iface (STA iface) will not allow to scan (connect) >>> ...? >>> >> right. >> The rationale behind it that you must listen constantly to detect >> radar events, which prevents scanning off-channel. >> (i guess an exception for on-channel scanning can be added if needed, though) >> > Still I can image hw with hw_scan() support that could have dedicated > "hw part" for scanning. > So, driver should made this decision (allow or not) base on hw features. > this will probably require additional radio... anyway, i don't have any objection here :) just explained the original logic (and my patch only changed the behavior from blocking indefinitely to returning error immediately) Eliad. -- 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 Sun, Feb 08, 2015 at 12:41:14PM +0200, Eliad Peller wrote: > There was indeed stale state. I've just sent a patch to fix it. Thanks, that fixed the issue. > I don't see why returning EBUSY is wrong, though. > Actually, it just make the test fail immediately, instead of waiting > indefinitely until a timeout occurs (i guess, didn't actually test it > with the reverted patch). > This was exactly the intended behavior, and i think it makes much more sense. I have no issues with EBUSY being returned for a case where an offchannel operation would be required while on a channel that require constant monitoring for radars. I guess it would be fine to run a single-channel scan on that same channel in such a state, but I'm not sure whether this code prevents that or not. (Or whether there is really that much of a real use case for such an operation.)
On Sun, Feb 8, 2015 at 5:56 PM, Jouni Malinen <j@w1.fi> wrote: >> I don't see why returning EBUSY is wrong, though. >> Actually, it just make the test fail immediately, instead of waiting >> indefinitely until a timeout occurs (i guess, didn't actually test it >> with the reverted patch). >> This was exactly the intended behavior, and i think it makes much more sense. > > I have no issues with EBUSY being returned for a case where an > offchannel operation would be required while on a channel that require > constant monitoring for radars. I guess it would be fine to run a > single-channel scan on that same channel in such a state, but I'm not > sure whether this code prevents that or not. (Or whether there is really > that much of a real use case for such an operation.) > See my answers to Janusz. The current code blocks any scan (including on-channel one). I guess an exception for on-channel scan can be added if needed. Eliad. -- 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
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c index cdd8258..4bee1f97 100644 --- a/net/mac80211/scan.c +++ b/net/mac80211/scan.c @@ -505,7 +505,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata, lockdep_assert_held(&local->mtx); - if (local->scan_req) + if (local->scan_req || ieee80211_is_radar_required(local)) return -EBUSY; if (!ieee80211_can_scan(local, sdata)) {
Radar detection can last indefinite time. There is no point in deferring a scan request in this case - simply return -EBUSY. Signed-off-by: Eliad Peller <eliad@wizery.com> --- v1->v2: use the newly introduced funcion net/mac80211/scan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)