[PATCH v2 1/3] mac80211: remove local->radar_detect_enabled

[PATCH v2 1/3] mac80211: remove local->radar_detect_enabled

[Eliad Peller]

local->radar_detect_enabled should tell whether
radar_detect is enabled on any interface belonging
to local.

However, it's not getting updated correctly
in many cases (actually, when testing with hwsim
it's never been set, even when the dfs master
is beaconing).

Instead of handling all the corner cases
(e.g. channel switch), simply check whether
radar detection is enabled only when needed,
instead of caching the result.

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
 net/mac80211/cfg.c         | 2 +-
 net/mac80211/chan.c        | 5 ++---
 net/mac80211/ieee80211_i.h | 3 +--
 net/mac80211/scan.c        | 2 +-
 4 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 1696658..3c53fa3 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -2557,7 +2557,7 @@ static int ieee80211_start_roc_work(struct ieee80211_local *local,
 
 	/* if there's one pending or we're scanning, queue this one */
 	if (!list_empty(&local->roc_list) ||
-	    local->scanning || local->radar_detect_enabled)
+	    local->scanning || ieee80211_is_radar_required(local))
 		goto out_check_combine;
 
 	/* if not HW assist, just queue & schedule work */
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 5d6dae9..f5d08d5 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -388,7 +388,7 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
 	return NULL;
 }
 
-static bool ieee80211_is_radar_required(struct ieee80211_local *local)
+bool ieee80211_is_radar_required(struct ieee80211_local *local)
 {
 	struct ieee80211_sub_if_data *sdata;
 
@@ -567,7 +567,7 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
 	bool radar_enabled;
 
 	lockdep_assert_held(&local->chanctx_mtx);
-	/* for setting local->radar_detect_enabled */
+	/* for ieee80211_is_radar_required */
 	lockdep_assert_held(&local->mtx);
 
 	radar_enabled = ieee80211_is_radar_required(local);
@@ -576,7 +576,6 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
 		return;
 
 	chanctx->conf.radar_enabled = radar_enabled;
-	local->radar_detect_enabled = chanctx->conf.radar_enabled;
 
 	if (!local->use_chanctx) {
 		local->hw.conf.radar_enabled = chanctx->conf.radar_enabled;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 4f45cab..9cdd94a 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -1168,8 +1168,6 @@ struct ieee80211_local {
 	/* wowlan is enabled -- don't reconfig on resume */
 	bool wowlan;
 
-	/* DFS/radar detection is enabled */
-	bool radar_detect_enabled;
 	struct work_struct radar_detected_work;
 
 	/* number of RX chains the hardware has */
@@ -1982,6 +1980,7 @@ void ieee80211_recalc_smps_chanctx(struct ieee80211_local *local,
 				   struct ieee80211_chanctx *chanctx);
 void ieee80211_recalc_chanctx_min_def(struct ieee80211_local *local,
 				      struct ieee80211_chanctx *ctx);
+bool ieee80211_is_radar_required(struct ieee80211_local *local);
 
 void ieee80211_dfs_cac_timer(unsigned long data);
 void ieee80211_dfs_cac_timer_work(struct work_struct *work);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index ae84267..cdd8258 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -432,7 +432,7 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local,
 static bool ieee80211_can_scan(struct ieee80211_local *local,
 			       struct ieee80211_sub_if_data *sdata)
 {
-	if (local->radar_detect_enabled)
+	if (ieee80211_is_radar_required(local))
 		return false;
 
 	if (!list_empty(&local->roc_list))
-- 
1.8.5.2.229.g4448466.dirty

[PATCH v2 2/3] mac80211: consider only relevant vifs for radar_required calculation

[Eliad Peller]

ctx->conf.radar_enabled should reflect whether radar
detection is enabled for the channel context.

When calculating it, make it consider only the vifs
that have this context assigned (instead of all the
vifs).

Signed-off-by: Eliad Peller <eliad@wizery.com>
---
v1->v2: 
* add a new function (instead of replacing existing one)
* init ctx->conf.radar_enabled to false

 net/mac80211/chan.c | 32 ++++++++++++++++++++++++++++++--
 1 file changed, 30 insertions(+), 2 deletions(-)

diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index f5d08d5..28da444 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -406,6 +406,34 @@ bool ieee80211_is_radar_required(struct ieee80211_local *local)
 	return false;
 }
 
+static bool
+ieee80211_chanctx_radar_required(struct ieee80211_local *local,
+				 struct ieee80211_chanctx *ctx)
+{
+	struct ieee80211_chanctx_conf *conf = &ctx->conf;
+	struct ieee80211_sub_if_data *sdata;
+	bool required = false;
+
+	lockdep_assert_held(&local->chanctx_mtx);
+	lockdep_assert_held(&local->mtx);
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+		if (!ieee80211_sdata_running(sdata))
+			continue;
+		if (rcu_access_pointer(sdata->vif.chanctx_conf) != conf)
+			continue;
+		if (!sdata->radar_required)
+			continue;
+
+		required = true;
+		break;
+	}
+	rcu_read_unlock();
+
+	return required;
+}
+
 static struct ieee80211_chanctx *
 ieee80211_alloc_chanctx(struct ieee80211_local *local,
 			const struct cfg80211_chan_def *chandef,
@@ -425,7 +453,7 @@ ieee80211_alloc_chanctx(struct ieee80211_local *local,
 	ctx->conf.rx_chains_static = 1;
 	ctx->conf.rx_chains_dynamic = 1;
 	ctx->mode = mode;
-	ctx->conf.radar_enabled = ieee80211_is_radar_required(local);
+	ctx->conf.radar_enabled = false;
 	ieee80211_recalc_chanctx_min_def(local, ctx);
 
 	return ctx;
@@ -570,7 +598,7 @@ static void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
 	/* for ieee80211_is_radar_required */
 	lockdep_assert_held(&local->mtx);
 
-	radar_enabled = ieee80211_is_radar_required(local);
+	radar_enabled = ieee80211_chanctx_radar_required(local, chanctx);
 
 	if (radar_enabled == chanctx->conf.radar_enabled)
 		return;
-- 
1.8.5.2.229.g4448466.dirty

[PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Eliad Peller]

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(-)

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)) {
-- 
1.8.5.2.229.g4448466.dirty

Re: [PATCH v2 1/3] mac80211: remove local->radar_detect_enabled

[Johannes Berg]

All three applied, thanks.

johannes

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Jouni Malinen]

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.
 
-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Janusz Dziedzic]

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

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Eliad Peller]

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.

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Eliad Peller]

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.

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Janusz Dziedzic]

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.

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Eliad Peller]

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.

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Jouni Malinen]

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

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [PATCH v2 3/3] mac80211: don't defer scans in case of radar detection

[Eliad Peller]

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.