local->hw.conf maybe not be synced when recalcing whether radar is
enabled, sometimes leaving radar enabled even if it's not neccesary
anymore.
Fix this by always applying the setting.
Reported-by: Zefir Kurtisi <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
net/mac80211/chan.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 931be41..9442c46 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -251,9 +251,6 @@ void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
}
rcu_read_unlock();
- if (radar_enabled == chanctx->conf.radar_enabled)
- return;
-
chanctx->conf.radar_enabled = radar_enabled;
local->radar_detect_enabled = chanctx->conf.radar_enabled;
--
1.7.10.4
On 04/05/2013 12:08 PM, Zefir Kurtisi wrote:
> 1) enable DFS log output at ath9k
> echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/debug
>
C&P garbage, the right command is of course:
echo 0x20400 > /sys/kernel/debug/ieee80211/phy0/ath9k/debug
On 04/04/2013 08:22 PM, Simon Wunderlich wrote:
> [...]
>
> So the patch does not resolve the problem for you? I've checked it again with
> a little printk in ath9ks config function.
>
> With the patch the radar_enabled flag gets disabled when changing the channel (5500 -> 5200).
> If I don't apply the patch, it stays enabled. I did the same thing (start hostapd on
> channel 5500, wait for CAC, echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/simulate_radar).
>
> Maybe I'm missing something?
>
Hi Simon,
here is how to reproduce (assuming you are using the patches for DFS testing on
ath9k posted yesterday).
1) enable DFS log output at ath9k
echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/debug
2) run hostapd on DFS channel, my config is as follows
interface=wlan3
driver=nl80211
ssid=testap
hw_mode=a
channel=108
wmm_enabled=1
ieee80211d=1
ieee80211h=1
country_code=DE
wpa_group_rekey=300
wpa_gmk_rekey=640
wpa=2
wpa_key_mgmt=WPA-PSK
wpa_pairwise=CCMP
wpa_passphrase=testtest
The log output I see is:
Apr 5 11:44:00: [ 335.210749] IPv6: ADDRCONF(NETDEV_UP): wlan3: link is not ready
Apr 5 11:44:00: [ 335.221152] ath: phy0: DFS enabled at freq 5540
Apr 5 11:44:04: [ 339.234487] ath: phy0: DFS enabled at freq 5540
Apr 5 11:44:04: [ 339.260298] ath: phy0: DFS enabled at freq 5540
Apr 5 11:44:04: [ 339.262750] ath: phy0: DFS enabled at freq 5540
Apr 5 11:44:04: [ 339.262786] IPv6: ADDRCONF(NETDEV_CHANGE): wlan3: link becomes ready
So we passed the (shortened to 4s) CAC and AP is operating.
3) fire radar
echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/simulate_radar
I see:
Apr 5 11:44:25: [ 360.294667] ath: phy0: DFS enabled at freq 5540
Apr 5 11:44:25: [ 360.297217] ath: phy0: DFS enabled at freq 5240
The 'DFS enabled ...' message is print if ath9k_config() is called with
hw->conf.radar_enabled, which should never happen for freq 5240.
I wish I had time to learn using ftrace to track it down... As said before, it is
not critical for ath9k, but might be a hint to something going wrong above.
Thanks,
Zefir
On Mon, 2013-04-08 at 22:43 +0200, Simon Wunderlich wrote:
> local->hw.conf maybe not be synced when recalcing whether radar is
> enabled, sometimes leaving radar enabled even if it's not neccesary
> anymore.
>
> Fix this by:
> * setting radar_enabled when creating the chanctx
> * turning radar_enabled off before destroying the last channel context
Applied.
johannes
Hello Zefir,
On Fri, Apr 05, 2013 at 12:08:05PM +0200, Zefir Kurtisi wrote:
> On 04/04/2013 08:22 PM, Simon Wunderlich wrote:
> > [...]
> >
> > So the patch does not resolve the problem for you? I've checked it again with
> > a little printk in ath9ks config function.
> >
> > With the patch the radar_enabled flag gets disabled when changing the channel (5500 -> 5200).
> > If I don't apply the patch, it stays enabled. I did the same thing (start hostapd on
> > channel 5500, wait for CAC, echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/simulate_radar).
> >
> > Maybe I'm missing something?
> >
> Hi Simon,
>
> here is how to reproduce (assuming you are using the patches for DFS testing on
> ath9k posted yesterday).
>
> 1) enable DFS log output at ath9k
> echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/debug
>
> 2) run hostapd on DFS channel, my config is as follows
> interface=wlan3
> driver=nl80211
> ssid=testap
> hw_mode=a
> channel=108
> wmm_enabled=1
> ieee80211d=1
> ieee80211h=1
> country_code=DE
> wpa_group_rekey=300
> wpa_gmk_rekey=640
> wpa=2
> wpa_key_mgmt=WPA-PSK
> wpa_pairwise=CCMP
> wpa_passphrase=testtest
>
> The log output I see is:
> Apr 5 11:44:00: [ 335.210749] IPv6: ADDRCONF(NETDEV_UP): wlan3: link is not ready
> Apr 5 11:44:00: [ 335.221152] ath: phy0: DFS enabled at freq 5540
> Apr 5 11:44:04: [ 339.234487] ath: phy0: DFS enabled at freq 5540
> Apr 5 11:44:04: [ 339.260298] ath: phy0: DFS enabled at freq 5540
> Apr 5 11:44:04: [ 339.262750] ath: phy0: DFS enabled at freq 5540
> Apr 5 11:44:04: [ 339.262786] IPv6: ADDRCONF(NETDEV_CHANGE): wlan3: link becomes ready
>
> So we passed the (shortened to 4s) CAC and AP is operating.
>
> 3) fire radar
> echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/simulate_radar
>
> I see:
> Apr 5 11:44:25: [ 360.294667] ath: phy0: DFS enabled at freq 5540
> Apr 5 11:44:25: [ 360.297217] ath: phy0: DFS enabled at freq 5240
>
>
> The 'DFS enabled ...' message is print if ath9k_config() is called with
> hw->conf.radar_enabled, which should never happen for freq 5240.
>
>
> I wish I had time to learn using ftrace to track it down... As said before, it is
> not critical for ath9k, but might be a hint to something going wrong above.
Thanks for explaining, I could now see what's happening. I've added another
debug messages for "DFS not enabled" in the other case. What I get when running
your example (and with my patch applied) is:
NOTE: hostapd started
[ 668.576380] ath: phy0: DFS not enabled at freq 5540
[ 668.594108] ath: phy0: DFS enabled at freq 5540
[ 672.607996] ath: phy0: DFS enabled at freq 5540
[ 672.928911] ath: phy0: DFS enabled at freq 5540
[ 672.939416] ath: phy0: DFS enabled at freq 5540
[ 704.128876] ath: phy0: DFS enabled at freq 5540
NOTE: radar triggered here
[ 704.138482] ath: phy0: DFS enabled at freq 5240
[ 704.149684] ath: phy0: DFS not enabled at freq 5240
Now what happens is:
1. vif_use_channel() calls ieee80211_new_chanctx(), which calls ieee80211_hw_config().
radar flags are not "recalc"d, so the previous value is used
2. Later in vif_use_channel(), ieee80211_recalc_radar_chanctx() is called, which
sets the radar flag and calls ieee80211_hw_config() again.
As you can see in the output, the right value is applied ~10 - 20ms after the wrong
value, at least with the patch originally posted here.
Of course this is not beautiful, but should work in practice.
We could consider changing this by already applying the correct radar flag in
ieee80211_new_chanctx()? If you want, I can post a patch for that. SMPS probably
has a similar problem to have the wrong value you set for a short time ...
Cheers,
Simon
On Thu, Apr 04, 2013 at 03:38:33PM +0200, Zefir Kurtisi wrote:
> On 04/03/2013 02:46 PM, Johannes Berg wrote:
> > On Tue, 2013-04-02 at 18:39 +0200, Simon Wunderlich wrote:
> >> local->hw.conf maybe not be synced when recalcing whether radar is
> >> enabled, sometimes leaving radar enabled even if it's not neccesary
> >> anymore.
> >
> > I don't really see how, can you explain more?
As far as I see, the problem happens when changing from a DFS to a non-DFS
channel. local->hw.conf.radar_enabled is true from the last (DFS) channel,
but the channel gets released when stopping the AP, and the channel context
is freed. Then when changing to the new channel, what happens is:
1. ieee80211_vif_use_channel() creates a new channel context. This channel
contxt has chanctx->conf.radar_enabled = false by default.
2. ieee80211_recalc_radar_chanctx() is called, and finds that no radar is
currently enabled => local radar_enabled = false
3. ieee80211_recalc_radar_chanctx() checks if
chanctx->conf.radar_enabled == radar_enabled and both are false, and
returns
=> but local->hw.conf.radar_enabled would be changed later in this function
and is therefore not updated.
Therefore I'm removing the check to always update the hw.conf.radar_enabled
field, to be safe in any case.
> >
> > johannes
> >
>
> You seem to be right, the patch does not resolve the observed problem.
>
> I am not deep enough in the DFS master code and its integration to resolve it
> myself. What I see is that ath9k_config() is called with a non-DFS channel but
> with ieee80211_hw->conf.radar_enabled set. For ath9k that's no problem at all
> (radar pulse detection can be enabled on any channel), but indicates a problem in
> the channel context handling for DFS.
>
> To reproduce, start an AP on a DFS channel, wait until CAC is finished and fire a
> radar. hostapd will switch to a non-DFS channel with the radar_enabled flag set.
So the patch does not resolve the problem for you? I've checked it again with
a little printk in ath9ks config function.
With the patch the radar_enabled flag gets disabled when changing the channel (5500 -> 5200).
If I don't apply the patch, it stays enabled. I did the same thing (start hostapd on
channel 5500, wait for CAC, echo 1 > /sys/kernel/debug/ieee80211/phy0/ath9k/simulate_radar).
Maybe I'm missing something?
Cheers,
Simon
On Tue, 2013-04-02 at 18:39 +0200, Simon Wunderlich wrote:
> local->hw.conf maybe not be synced when recalcing whether radar is
> enabled, sometimes leaving radar enabled even if it's not neccesary
> anymore.
I don't really see how, can you explain more?
johannes
local->hw.conf maybe not be synced when recalcing whether radar is
enabled, sometimes leaving radar enabled even if it's not neccesary
anymore.
Fix this by:
* setting radar_enabled when creating the chanctx
* turning radar_enabled off before destroying the last channel context
Reported-by: Zefir Kurtisi <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
Changes to PATCHv2:
* add comment about single context, and check for it
* rebase on mac80211-next
---
net/mac80211/chan.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 8024874..23cb270 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -57,6 +57,22 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
return NULL;
}
+static bool ieee80211_is_radar_required(struct ieee80211_local *local)
+{
+ struct ieee80211_sub_if_data *sdata;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->radar_required) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
static struct ieee80211_chanctx *
ieee80211_new_chanctx(struct ieee80211_local *local,
const struct cfg80211_chan_def *chandef,
@@ -75,6 +91,9 @@ ieee80211_new_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);
+ if (!local->use_chanctx)
+ local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
if (!local->use_chanctx) {
local->_oper_chandef = *chandef;
@@ -99,6 +118,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
static void ieee80211_free_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
+ bool check_single_channel = false;
lockdep_assert_held(&local->chanctx_mtx);
WARN_ON_ONCE(ctx->refcount != 0);
@@ -108,6 +128,13 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
chandef->center_freq1 = chandef->chan->center_freq;
chandef->center_freq2 = 0;
+
+ /* NOTE: Disabling radar he is only valid here for
+ * single channel context. To be sure, check it ... */
+ if (local->hw.conf.radar_enabled)
+ check_single_channel = true;
+ local->hw.conf.radar_enabled = false;
+
ieee80211_hw_config(local, 0);
} else {
drv_remove_chanctx(local, ctx);
@@ -116,6 +143,10 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
list_del_rcu(&ctx->list);
kfree_rcu(ctx, rcu_head);
+ /* throw a warning if this wasn't the only channel context. */
+ if (check_single_channel)
+ WARN_ON(!list_empty(&local->chanctx_list));
+
mutex_lock(&local->mtx);
ieee80211_recalc_idle(local);
mutex_unlock(&local->mtx);
@@ -227,19 +258,11 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *chanctx)
{
- struct ieee80211_sub_if_data *sdata;
- bool radar_enabled = false;
+ bool radar_enabled;
lockdep_assert_held(&local->chanctx_mtx);
- rcu_read_lock();
- list_for_each_entry_rcu(sdata, &local->interfaces, list) {
- if (sdata->radar_required) {
- radar_enabled = true;
- break;
- }
- }
- rcu_read_unlock();
+ radar_enabled = ieee80211_is_radar_required(local);
if (radar_enabled == chanctx->conf.radar_enabled)
return;
--
1.7.10.4
On 04/08/2013 02:09 PM, Simon Wunderlich wrote:
> local->hw.conf maybe not be synced when recalcing whether radar is
> enabled, sometimes leaving radar enabled even if it's not neccesary
> anymore.
>
> Fix this by:
> * setting radar_enabled when creating the chanctx
> * turning radar_enabled off before destroying the last channel context
>
> Reported-by: Zefir Kurtisi <[email protected]>
> Signed-off-by: Simon Wunderlich <[email protected]>
> ---
Thanks, this one does what it should, so feel free to add
Tested-by: Zefir Kurtisi <[email protected]>
local->hw.conf maybe not be synced when recalcing whether radar is
enabled, sometimes leaving radar enabled even if it's not neccesary
anymore.
Fix this by:
* setting radar_enabled when creating the chanctx
* turning radar_enabled off before destroying the last channel context
Reported-by: Zefir Kurtisi <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
net/mac80211/chan.c | 32 ++++++++++++++++++++++----------
1 file changed, 22 insertions(+), 10 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 931be41..1f54ffe 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -57,6 +57,22 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
return NULL;
}
+static bool ieee80211_is_radar_required(struct ieee80211_local *local)
+{
+ struct ieee80211_sub_if_data *sdata;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->radar_required) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
static struct ieee80211_chanctx *
ieee80211_new_chanctx(struct ieee80211_local *local,
const struct cfg80211_chan_def *chandef,
@@ -76,6 +92,9 @@ ieee80211_new_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);
+ if (!local->use_chanctx)
+ local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
/* acquire mutex to prevent idle from changing */
mutex_lock(&local->mtx);
@@ -118,6 +137,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
if (!local->use_chanctx) {
local->_oper_channel_type = NL80211_CHAN_NO_HT;
+ local->hw.conf.radar_enabled = false;
ieee80211_hw_config(local, 0);
} else {
drv_remove_chanctx(local, ctx);
@@ -237,19 +257,11 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *chanctx)
{
- struct ieee80211_sub_if_data *sdata;
- bool radar_enabled = false;
+ bool radar_enabled;
lockdep_assert_held(&local->chanctx_mtx);
- rcu_read_lock();
- list_for_each_entry_rcu(sdata, &local->interfaces, list) {
- if (sdata->radar_required) {
- radar_enabled = true;
- break;
- }
- }
- rcu_read_unlock();
+ radar_enabled = ieee80211_is_radar_required(local);
if (radar_enabled == chanctx->conf.radar_enabled)
return;
--
1.7.10.4
On Thu, 2013-04-04 at 20:22 +0200, Simon Wunderlich wrote:
> As far as I see, the problem happens when changing from a DFS to a non-DFS
> channel. local->hw.conf.radar_enabled is true from the last (DFS) channel,
> but the channel gets released when stopping the AP, and the channel context
> is freed.
At this point, why doesn't it disable hw.conf.radar_enabled? I really
think it should?
johannes
On 04/05/2013 01:29 PM, Simon Wunderlich wrote:
> Hello Zefir,
>
>
> Thanks for explaining, I could now see what's happening. I've added another
> debug messages for "DFS not enabled" in the other case. What I get when running
> your example (and with my patch applied) is:
>
> NOTE: hostapd started
> [ 668.576380] ath: phy0: DFS not enabled at freq 5540
> [ 668.594108] ath: phy0: DFS enabled at freq 5540
> [ 672.607996] ath: phy0: DFS enabled at freq 5540
> [ 672.928911] ath: phy0: DFS enabled at freq 5540
> [ 672.939416] ath: phy0: DFS enabled at freq 5540
> [ 704.128876] ath: phy0: DFS enabled at freq 5540
> NOTE: radar triggered here
> [ 704.138482] ath: phy0: DFS enabled at freq 5240
> [ 704.149684] ath: phy0: DFS not enabled at freq 5240
>
> Now what happens is:
> 1. vif_use_channel() calls ieee80211_new_chanctx(), which calls ieee80211_hw_config().
> radar flags are not "recalc"d, so the previous value is used
> 2. Later in vif_use_channel(), ieee80211_recalc_radar_chanctx() is called, which
> sets the radar flag and calls ieee80211_hw_config() again.
>
> As you can see in the output, the right value is applied ~10 - 20ms after the wrong
> value, at least with the patch originally posted here.
>
> Of course this is not beautiful, but should work in practice.
>
> We could consider changing this by already applying the correct radar flag in
> ieee80211_new_chanctx()? If you want, I can post a patch for that. SMPS probably
> has a similar problem to have the wrong value you set for a short time ...
>
>
Thanks for double-checking, Simon.
I failed to see the obvious (that ath_config() is called immediately again with
the correct DFS flag), sorry for raising dust.
Whether fix it or not, I'd say it is good enough for ath9k. Might be different for
other chips that don't allow enabling radar detection on non-DFS channels (even if
it is only for some ms).
Cheers,
Zefir
On 04/03/2013 02:46 PM, Johannes Berg wrote:
> On Tue, 2013-04-02 at 18:39 +0200, Simon Wunderlich wrote:
>> local->hw.conf maybe not be synced when recalcing whether radar is
>> enabled, sometimes leaving radar enabled even if it's not neccesary
>> anymore.
>
> I don't really see how, can you explain more?
>
> johannes
>
You seem to be right, the patch does not resolve the observed problem.
I am not deep enough in the DFS master code and its integration to resolve it
myself. What I see is that ath9k_config() is called with a non-DFS channel but
with ieee80211_hw->conf.radar_enabled set. For ath9k that's no problem at all
(radar pulse detection can be enabled on any channel), but indicates a problem in
the channel context handling for DFS.
To reproduce, start an AP on a DFS channel, wait until CAC is finished and fire a
radar. hostapd will switch to a non-DFS channel with the radar_enabled flag set.
On Fri, Apr 05, 2013 at 02:36:04PM +0200, Johannes Berg wrote:
> On Fri, 2013-04-05 at 14:16 +0200, Simon Wunderlich wrote:
> > On Fri, Apr 05, 2013 at 01:22:51PM +0200, Johannes Berg wrote:
> > > On Thu, 2013-04-04 at 20:22 +0200, Simon Wunderlich wrote:
> > >
> > > > As far as I see, the problem happens when changing from a DFS to a non-DFS
> > > > channel. local->hw.conf.radar_enabled is true from the last (DFS) channel,
> > > > but the channel gets released when stopping the AP, and the channel context
> > > > is freed.
> > >
> > > At this point, why doesn't it disable hw.conf.radar_enabled? I really
> > > think it should?
> >
> > As far as I see release_channel() -> unassign_vif_chanctx() -> ieee80211_recalc_radar_chanctx()
> > is called while the interface with radar enabled is still present, and therefore it is assumed
> > that the radar enable is still required.
> >
> > We could reset local->hw.conf.radar_enabled ieee80211_free_chanctx() though, but IMHO the cleaner
> > way would be to properly initialize it when the the channel is used next time through
> > vif_use_channel().
>
> I disagree, it would leave the hardware in a weird state -- idle but
> having to detect radars??
>
> johannes
Well, you have a point ...
I'll look into it, it should be doable that the radar flag is always set
correctly. Please drop this patch for now.
Cheers,
Simon
On Mon, Apr 08, 2013 at 07:04:58PM +0200, Simon Wunderlich wrote:
[...]
> @@ -108,6 +128,13 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
> chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
> chandef->center_freq1 = chandef->chan->center_freq;
> chandef->center_freq2 = 0;
> +
> + /* NOTE: Disabling radar he is only valid here for
typo? ^^
> + * single channel context. To be sure, check it ... */
> + if (local->hw.conf.radar_enabled)
> + check_single_channel = true;
> + local->hw.conf.radar_enabled = false;
> +
> ieee80211_hw_config(local, 0);
> } else {
> drv_remove_chanctx(local, ctx);
> @@ -116,6 +143,10 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
> list_del_rcu(&ctx->list);
> kfree_rcu(ctx, rcu_head);
>
> + /* throw a warning if this wasn't the only channel context. */
> + if (check_single_channel)
> + WARN_ON(!list_empty(&local->chanctx_list));
> +
Why not using
WARN_ON(check_single_channel && !list_empty(&local->chanctx_list));
? Moving the entire condition in the WARN_ON will optimise it all for being 'unlikely'.
But Johannes can simply destroy my comment if he thinks it is wrong :)
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
On Mon, 2013-04-08 at 14:09 +0200, Simon Wunderlich wrote:
> @@ -118,6 +137,7 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
>
> if (!local->use_chanctx) {
> local->_oper_channel_type = NL80211_CHAN_NO_HT;
> + local->hw.conf.radar_enabled = false;
That should probably get a comment (and an assertion?) that radar
detection is only possible with a single channel context, so when that
is freed radar detection can no longer be turned on.
Also, the patch doesn't apply on mac80211-next.
johannes
On Fri, 2013-04-05 at 14:16 +0200, Simon Wunderlich wrote:
> On Fri, Apr 05, 2013 at 01:22:51PM +0200, Johannes Berg wrote:
> > On Thu, 2013-04-04 at 20:22 +0200, Simon Wunderlich wrote:
> >
> > > As far as I see, the problem happens when changing from a DFS to a non-DFS
> > > channel. local->hw.conf.radar_enabled is true from the last (DFS) channel,
> > > but the channel gets released when stopping the AP, and the channel context
> > > is freed.
> >
> > At this point, why doesn't it disable hw.conf.radar_enabled? I really
> > think it should?
>
> As far as I see release_channel() -> unassign_vif_chanctx() -> ieee80211_recalc_radar_chanctx()
> is called while the interface with radar enabled is still present, and therefore it is assumed
> that the radar enable is still required.
>
> We could reset local->hw.conf.radar_enabled ieee80211_free_chanctx() though, but IMHO the cleaner
> way would be to properly initialize it when the the channel is used next time through
> vif_use_channel().
I disagree, it would leave the hardware in a weird state -- idle but
having to detect radars??
johannes
On Fri, Apr 05, 2013 at 01:22:51PM +0200, Johannes Berg wrote:
> On Thu, 2013-04-04 at 20:22 +0200, Simon Wunderlich wrote:
>
> > As far as I see, the problem happens when changing from a DFS to a non-DFS
> > channel. local->hw.conf.radar_enabled is true from the last (DFS) channel,
> > but the channel gets released when stopping the AP, and the channel context
> > is freed.
>
> At this point, why doesn't it disable hw.conf.radar_enabled? I really
> think it should?
As far as I see release_channel() -> unassign_vif_chanctx() -> ieee80211_recalc_radar_chanctx()
is called while the interface with radar enabled is still present, and therefore it is assumed
that the radar enable is still required.
We could reset local->hw.conf.radar_enabled ieee80211_free_chanctx() though, but IMHO the cleaner
way would be to properly initialize it when the the channel is used next time through
vif_use_channel().
Cheers,
Simon
local->hw.conf maybe not be synced when recalcing whether radar is
enabled, sometimes leaving radar enabled even if it's not neccesary
anymore.
Fix this by:
* setting radar_enabled when creating the chanctx
* turning radar_enabled off before destroying the last channel context
Reported-by: Zefir Kurtisi <[email protected]>
Signed-off-by: Simon Wunderlich <[email protected]>
---
Changes to PATCHv3:
* fix typo
* move whole check into WARN_ON
Changes to PATCHv2:
* add comment about single context, and check for it
* rebase on mac80211-next
---
net/mac80211/chan.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
diff --git a/net/mac80211/chan.c b/net/mac80211/chan.c
index 8024874..166165e 100644
--- a/net/mac80211/chan.c
+++ b/net/mac80211/chan.c
@@ -57,6 +57,22 @@ ieee80211_find_chanctx(struct ieee80211_local *local,
return NULL;
}
+static bool ieee80211_is_radar_required(struct ieee80211_local *local)
+{
+ struct ieee80211_sub_if_data *sdata;
+
+ rcu_read_lock();
+ list_for_each_entry_rcu(sdata, &local->interfaces, list) {
+ if (sdata->radar_required) {
+ rcu_read_unlock();
+ return true;
+ }
+ }
+ rcu_read_unlock();
+
+ return false;
+}
+
static struct ieee80211_chanctx *
ieee80211_new_chanctx(struct ieee80211_local *local,
const struct cfg80211_chan_def *chandef,
@@ -75,6 +91,9 @@ ieee80211_new_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);
+ if (!local->use_chanctx)
+ local->hw.conf.radar_enabled = ctx->conf.radar_enabled;
if (!local->use_chanctx) {
local->_oper_chandef = *chandef;
@@ -99,6 +118,7 @@ ieee80211_new_chanctx(struct ieee80211_local *local,
static void ieee80211_free_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *ctx)
{
+ bool check_single_channel = false;
lockdep_assert_held(&local->chanctx_mtx);
WARN_ON_ONCE(ctx->refcount != 0);
@@ -108,6 +128,14 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
chandef->width = NL80211_CHAN_WIDTH_20_NOHT;
chandef->center_freq1 = chandef->chan->center_freq;
chandef->center_freq2 = 0;
+
+ /* NOTE: Disabling radar is only valid here for
+ * single channel context. To be sure, check it ...
+ */
+ if (local->hw.conf.radar_enabled)
+ check_single_channel = true;
+ local->hw.conf.radar_enabled = false;
+
ieee80211_hw_config(local, 0);
} else {
drv_remove_chanctx(local, ctx);
@@ -116,6 +144,9 @@ static void ieee80211_free_chanctx(struct ieee80211_local *local,
list_del_rcu(&ctx->list);
kfree_rcu(ctx, rcu_head);
+ /* throw a warning if this wasn't the only channel context. */
+ WARN_ON(check_single_channel && !list_empty(&local->chanctx_list));
+
mutex_lock(&local->mtx);
ieee80211_recalc_idle(local);
mutex_unlock(&local->mtx);
@@ -227,19 +258,11 @@ static void __ieee80211_vif_release_channel(struct ieee80211_sub_if_data *sdata)
void ieee80211_recalc_radar_chanctx(struct ieee80211_local *local,
struct ieee80211_chanctx *chanctx)
{
- struct ieee80211_sub_if_data *sdata;
- bool radar_enabled = false;
+ bool radar_enabled;
lockdep_assert_held(&local->chanctx_mtx);
- rcu_read_lock();
- list_for_each_entry_rcu(sdata, &local->interfaces, list) {
- if (sdata->radar_required) {
- radar_enabled = true;
- break;
- }
- }
- rcu_read_unlock();
+ radar_enabled = ieee80211_is_radar_required(local);
if (radar_enabled == chanctx->conf.radar_enabled)
return;
--
1.7.10.4