[PATCH] ath10k: fix initial radar detection logic

[PATCH] ath10k: fix initial radar detection logic

[Michal Kazior]

This fixes a problem of initial radar detection
(CAC) being stuck and blocking Rx in some cases
until all interfaces were brought down.

For userspace this meant first run of hostapd
would perform CAC but due to filtered Rx no
clients would associate. Subsequent runs of
hostapd would not perform CAC (as it was already
done) and would associate clients.

This also makes sure radar detection is performed
when bandwidth is widened. Before if 20MHz CAC was
performed then 40MHz CAC wouldn't start monitor
vdev effectively disabling initial radar
detection.

A driver should just start/stop radar detection
based on hw->conf.radar_enabled. However, since
ath10k needs to start a monitor vdev for the
initial radar detection special care needs to be
applied.

While at it cleanup the code a bit.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
 drivers/net/wireless/ath/ath10k/core.h |  2 ++
 drivers/net/wireless/ath/ath10k/mac.c  | 56 ++++++++++++----------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 8edd6da..ca2abe2 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -434,6 +434,8 @@ struct ath10k {
 	unsigned int filter_flags;
 	unsigned long dev_flags;
 	u32 dfs_block_radar_events;
+	bool radar_enabled; /* protected by conf_mutex */
+	int num_started_vdevs; /* protected by conf_mutex */
 
 	struct wmi_pdev_set_wmm_params_arg wmm_params;
 	struct completion install_key_done;
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 58ec5a7..024fce4 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -489,6 +489,8 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 	return 0;
 }
 
+static void ath10k_recalc_radar_detection(struct ath10k *ar);
+
 static int ath10k_vdev_start(struct ath10k_vif *arvif)
 {
 	struct ath10k *ar = arvif->ar;
@@ -545,6 +547,9 @@ static int ath10k_vdev_start(struct ath10k_vif *arvif)
 		return ret;
 	}
 
+	ar->num_started_vdevs++;
+	ath10k_recalc_radar_detection(ar);
+
 	return ret;
 }
 
@@ -571,6 +576,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
 		return ret;
 	}
 
+	if (WARN_ON(ar->num_started_vdevs == 0)) {
+		ar->num_started_vdevs--;
+		ath10k_recalc_radar_detection(ar);
+	}
+
 	return ret;
 }
 
@@ -804,49 +814,18 @@ static int ath10k_stop_cac(struct ath10k *ar)
 	return 0;
 }
 
-static const char *ath10k_dfs_state(enum nl80211_dfs_state dfs_state)
-{
-	switch (dfs_state) {
-	case NL80211_DFS_USABLE:
-		return "USABLE";
-	case NL80211_DFS_UNAVAILABLE:
-		return "UNAVAILABLE";
-	case NL80211_DFS_AVAILABLE:
-		return "AVAILABLE";
-	default:
-		WARN_ON(1);
-		return "bug";
-	}
-}
-
-static void ath10k_config_radar_detection(struct ath10k *ar)
+static void ath10k_recalc_radar_detection(struct ath10k *ar)
 {
-	struct ieee80211_channel *chan = ar->hw->conf.chandef.chan;
-	bool radar = ar->hw->conf.radar_enabled;
-	bool chan_radar = !!(chan->flags & IEEE80211_CHAN_RADAR);
-	enum nl80211_dfs_state dfs_state = chan->dfs_state;
 	int ret;
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	ath10k_dbg(ATH10K_DBG_MAC,
-		   "mac radar config update: chan %dMHz radar %d chan radar %d chan state %s\n",
-		   chan->center_freq, radar, chan_radar,
-		   ath10k_dfs_state(dfs_state));
-
-	/*
-	 * It's safe to call it even if CAC is not started.
-	 * This call here guarantees changing channel, etc. will stop CAC.
-	 */
 	ath10k_stop_cac(ar);
 
-	if (!radar)
-		return;
-
-	if (!chan_radar)
+	if (!ar->radar_enabled)
 		return;
 
-	if (dfs_state != NL80211_DFS_USABLE)
+	if (ar->num_started_vdevs > 0)
 		return;
 
 	ret = ath10k_start_cac(ar);
@@ -2341,6 +2320,7 @@ static int ath10k_start(struct ieee80211_hw *hw)
 		goto exit;
 	}
 
+	ar->num_started_vdevs = 0;
 	ath10k_regd_update(ar);
 	ret = 0;
 
@@ -2490,15 +2470,17 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 
 	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
 		ath10k_dbg(ATH10K_DBG_MAC,
-			   "mac config channel %d mhz flags 0x%x\n",
+			   "mac config channel %dMHz flags 0x%x radar %d\n",
 			   conf->chandef.chan->center_freq,
-			   conf->chandef.chan->flags);
+			   conf->chandef.chan->flags,
+			   conf->radar_enabled);
 
 		spin_lock_bh(&ar->data_lock);
 		ar->rx_channel = conf->chandef.chan;
 		spin_unlock_bh(&ar->data_lock);
 
-		ath10k_config_radar_detection(ar);
+		ar->radar_enabled = conf->radar_enabled;
+		ath10k_recalc_radar_detection(ar);
 
 		if (!cfg80211_chandef_identical(&ar->chandef, &conf->chandef)) {
 			ar->chandef = conf->chandef;
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: fix initial radar detection logic

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This fixes a problem of initial radar detection
> (CAC) being stuck and blocking Rx in some cases
> until all interfaces were brought down.

It would be good to describe more about the cases where this problem
happened.

> For userspace this meant first run of hostapd
> would perform CAC but due to filtered Rx no
> clients would associate. Subsequent runs of
> hostapd would not perform CAC (as it was already
> done) and would associate clients.
>
> This also makes sure radar detection is performed
> when bandwidth is widened. Before if 20MHz CAC was
> performed then 40MHz CAC wouldn't start monitor
> vdev effectively disabling initial radar
> detection.
>
> A driver should just start/stop radar detection
> based on hw->conf.radar_enabled. However, since
> ath10k needs to start a monitor vdev for the
> initial radar detection special care needs to be
> applied.
>
> While at it cleanup the code a bit.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -434,6 +434,8 @@ struct ath10k {
>  	unsigned int filter_flags;
>  	unsigned long dev_flags;
>  	u32 dfs_block_radar_events;
> +	bool radar_enabled; /* protected by conf_mutex */
> +	int num_started_vdevs; /* protected by conf_mutex */

I would prefer style like this:

        u32 dfs_block_radar_events;

        /* these are protected by conf_mutex */
        bool radar_enabled; 
        int num_started_vdevs;

> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -489,6 +489,8 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
>  	return 0;
>  }
>  
> +static void ath10k_recalc_radar_detection(struct ath10k *ar);

Forward declarations should be avoided if possible. Can you add a new
patch which just moves ath10k_recalc_radar_detection() and in this patch
you do the modifications in the function?

> @@ -571,6 +576,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
>  		return ret;
>  	}
>  
> +	if (WARN_ON(ar->num_started_vdevs == 0)) {
> +		ar->num_started_vdevs--;
> +		ath10k_recalc_radar_detection(ar);

Now num_started_vdevs will be -1, what does that mean? It would be good
to document that in struct ath10k.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: fix initial radar detection logic

[Michal Kazior]

On 2 April 2014 07:27, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This fixes a problem of initial radar detection
>> (CAC) being stuck and blocking Rx in some cases
>> until all interfaces were brought down.
>
> It would be good to describe more about the cases where this problem
> happened.

Ok.


>> For userspace this meant first run of hostapd
>> would perform CAC but due to filtered Rx no
>> clients would associate. Subsequent runs of
>> hostapd would not perform CAC (as it was already
>> done) and would associate clients.
>>
>> This also makes sure radar detection is performed
>> when bandwidth is widened. Before if 20MHz CAC was
>> performed then 40MHz CAC wouldn't start monitor
>> vdev effectively disabling initial radar
>> detection.
>>
>> A driver should just start/stop radar detection
>> based on hw->conf.radar_enabled. However, since
>> ath10k needs to start a monitor vdev for the
>> initial radar detection special care needs to be
>> applied.
>>
>> While at it cleanup the code a bit.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -434,6 +434,8 @@ struct ath10k {
>>       unsigned int filter_flags;
>>       unsigned long dev_flags;
>>       u32 dfs_block_radar_events;
>> +     bool radar_enabled; /* protected by conf_mutex */
>> +     int num_started_vdevs; /* protected by conf_mutex */
>
> I would prefer style like this:
>
>         u32 dfs_block_radar_events;
>
>         /* these are protected by conf_mutex */
>         bool radar_enabled;
>         int num_started_vdevs;

Ok.


>
>> --- a/drivers/net/wireless/ath/ath10k/mac.c
>> +++ b/drivers/net/wireless/ath/ath10k/mac.c
>> @@ -489,6 +489,8 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
>>       return 0;
>>  }
>>
>> +static void ath10k_recalc_radar_detection(struct ath10k *ar);
>
> Forward declarations should be avoided if possible. Can you add a new
> patch which just moves ath10k_recalc_radar_detection() and in this patch
> you do the modifications in the function?

The we need to move ath10k_cac_start/stop and possibly a few other
functions as well. I'll check that out.


>> @@ -571,6 +576,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
>>               return ret;
>>       }
>>
>> +     if (WARN_ON(ar->num_started_vdevs == 0)) {
>> +             ar->num_started_vdevs--;
>> +             ath10k_recalc_radar_detection(ar);
>
> Now num_started_vdevs will be -1, what does that mean? It would be good
> to document that in struct ath10k.

Ah, it's missing "!" in front of the WARN_ON..


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 0/2] ath10k: fix radar/cac

[Michal Kazior]

Hi,

This loosely depends on `ath10k: refactor monitor
code`. Without it patch this patchset introduces
some (harmless) runtime warnings.

This v2 introduces a non-functional patch to avoid
forward declaration. The [1/2] has a little
conflict with `ath10k: refactor monitor code`
which renames ath10k_monitor_stop() to
ath10k_monitor_vdev_stop().

Part of the original CAC problem is also addressed
in `mac80211: fix radar_enabled propagation`.


Michal Kazior (2):
  ath10k: reorder functions
  ath10k: refactor radar detection code

 drivers/net/wireless/ath/ath10k/core.h |   4 +
 drivers/net/wireless/ath/ath10k/mac.c  | 224 +++++++++++++++------------------
 2 files changed, 106 insertions(+), 122 deletions(-)

-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 1/2] ath10k: reorder functions

[Michal Kazior]

This is done to avoid forward declarations with
upcomming patches.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * introduced to avoid forward declaration

 drivers/net/wireless/ath/ath10k/mac.c | 170 +++++++++++++++++-----------------
 1 file changed, 85 insertions(+), 85 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 58ec5a7..e1f4227 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -489,91 +489,6 @@ static inline int ath10k_vdev_setup_sync(struct ath10k *ar)
 	return 0;
 }
 
-static int ath10k_vdev_start(struct ath10k_vif *arvif)
-{
-	struct ath10k *ar = arvif->ar;
-	struct cfg80211_chan_def *chandef = &ar->chandef;
-	struct wmi_vdev_start_request_arg arg = {};
-	int ret = 0;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	reinit_completion(&ar->vdev_setup_done);
-
-	arg.vdev_id = arvif->vdev_id;
-	arg.dtim_period = arvif->dtim_period;
-	arg.bcn_intval = arvif->beacon_interval;
-
-	arg.channel.freq = chandef->chan->center_freq;
-	arg.channel.band_center_freq1 = chandef->center_freq1;
-	arg.channel.mode = chan_to_phymode(chandef);
-
-	arg.channel.min_power = 0;
-	arg.channel.max_power = chandef->chan->max_power * 2;
-	arg.channel.max_reg_power = chandef->chan->max_reg_power * 2;
-	arg.channel.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
-
-	if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
-		arg.ssid = arvif->u.ap.ssid;
-		arg.ssid_len = arvif->u.ap.ssid_len;
-		arg.hidden_ssid = arvif->u.ap.hidden_ssid;
-
-		/* For now allow DFS for AP mode */
-		arg.channel.chan_radar =
-			!!(chandef->chan->flags & IEEE80211_CHAN_RADAR);
-	} else if (arvif->vdev_type == WMI_VDEV_TYPE_IBSS) {
-		arg.ssid = arvif->vif->bss_conf.ssid;
-		arg.ssid_len = arvif->vif->bss_conf.ssid_len;
-	}
-
-	ath10k_dbg(ATH10K_DBG_MAC,
-		   "mac vdev %d start center_freq %d phymode %s\n",
-		   arg.vdev_id, arg.channel.freq,
-		   ath10k_wmi_phymode_str(arg.channel.mode));
-
-	ret = ath10k_wmi_vdev_start(ar, &arg);
-	if (ret) {
-		ath10k_warn("failed to start WMI vdev %i: %d\n",
-			    arg.vdev_id, ret);
-		return ret;
-	}
-
-	ret = ath10k_vdev_setup_sync(ar);
-	if (ret) {
-		ath10k_warn("failed to synchronise setup for vdev %i: %d\n",
-			    arg.vdev_id, ret);
-		return ret;
-	}
-
-	return ret;
-}
-
-static int ath10k_vdev_stop(struct ath10k_vif *arvif)
-{
-	struct ath10k *ar = arvif->ar;
-	int ret;
-
-	lockdep_assert_held(&ar->conf_mutex);
-
-	reinit_completion(&ar->vdev_setup_done);
-
-	ret = ath10k_wmi_vdev_stop(ar, arvif->vdev_id);
-	if (ret) {
-		ath10k_warn("failed to stop WMI vdev %i: %d\n",
-			    arvif->vdev_id, ret);
-		return ret;
-	}
-
-	ret = ath10k_vdev_setup_sync(ar);
-	if (ret) {
-		ath10k_warn("failed to syncronise setup for vdev %i: %d\n",
-			    arvif->vdev_id, ret);
-		return ret;
-	}
-
-	return ret;
-}
-
 static int ath10k_monitor_start(struct ath10k *ar, int vdev_id)
 {
 	struct cfg80211_chan_def *chandef = &ar->chandef;
@@ -861,6 +776,91 @@ static void ath10k_config_radar_detection(struct ath10k *ar)
 	}
 }
 
+static int ath10k_vdev_start(struct ath10k_vif *arvif)
+{
+	struct ath10k *ar = arvif->ar;
+	struct cfg80211_chan_def *chandef = &ar->chandef;
+	struct wmi_vdev_start_request_arg arg = {};
+	int ret = 0;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	reinit_completion(&ar->vdev_setup_done);
+
+	arg.vdev_id = arvif->vdev_id;
+	arg.dtim_period = arvif->dtim_period;
+	arg.bcn_intval = arvif->beacon_interval;
+
+	arg.channel.freq = chandef->chan->center_freq;
+	arg.channel.band_center_freq1 = chandef->center_freq1;
+	arg.channel.mode = chan_to_phymode(chandef);
+
+	arg.channel.min_power = 0;
+	arg.channel.max_power = chandef->chan->max_power * 2;
+	arg.channel.max_reg_power = chandef->chan->max_reg_power * 2;
+	arg.channel.max_antenna_gain = chandef->chan->max_antenna_gain * 2;
+
+	if (arvif->vdev_type == WMI_VDEV_TYPE_AP) {
+		arg.ssid = arvif->u.ap.ssid;
+		arg.ssid_len = arvif->u.ap.ssid_len;
+		arg.hidden_ssid = arvif->u.ap.hidden_ssid;
+
+		/* For now allow DFS for AP mode */
+		arg.channel.chan_radar =
+			!!(chandef->chan->flags & IEEE80211_CHAN_RADAR);
+	} else if (arvif->vdev_type == WMI_VDEV_TYPE_IBSS) {
+		arg.ssid = arvif->vif->bss_conf.ssid;
+		arg.ssid_len = arvif->vif->bss_conf.ssid_len;
+	}
+
+	ath10k_dbg(ATH10K_DBG_MAC,
+		   "mac vdev %d start center_freq %d phymode %s\n",
+		   arg.vdev_id, arg.channel.freq,
+		   ath10k_wmi_phymode_str(arg.channel.mode));
+
+	ret = ath10k_wmi_vdev_start(ar, &arg);
+	if (ret) {
+		ath10k_warn("failed to start WMI vdev %i: %d\n",
+			    arg.vdev_id, ret);
+		return ret;
+	}
+
+	ret = ath10k_vdev_setup_sync(ar);
+	if (ret) {
+		ath10k_warn("failed to synchronise setup for vdev %i: %d\n",
+			    arg.vdev_id, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
+static int ath10k_vdev_stop(struct ath10k_vif *arvif)
+{
+	struct ath10k *ar = arvif->ar;
+	int ret;
+
+	lockdep_assert_held(&ar->conf_mutex);
+
+	reinit_completion(&ar->vdev_setup_done);
+
+	ret = ath10k_wmi_vdev_stop(ar, arvif->vdev_id);
+	if (ret) {
+		ath10k_warn("failed to stop WMI vdev %i: %d\n",
+			    arvif->vdev_id, ret);
+		return ret;
+	}
+
+	ret = ath10k_vdev_setup_sync(ar);
+	if (ret) {
+		ath10k_warn("failed to syncronise setup for vdev %i: %d\n",
+			    arvif->vdev_id, ret);
+		return ret;
+	}
+
+	return ret;
+}
+
 static void ath10k_control_beaconing(struct ath10k_vif *arvif,
 				struct ieee80211_bss_conf *info)
 {
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/2] ath10k: reorder functions

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> This is done to avoid forward declarations with
> upcomming patches.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

Like you said, I had a conflict with this patch. Can you double check my
resolution, please:

https://github.com/kvalo/ath/commit/0953accf20c325adde537a57c3e6a6a0f1ff04d7

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 1/2] ath10k: reorder functions

[Michal Kazior]

On 7 April 2014 15:52, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> This is done to avoid forward declarations with
>> upcomming patches.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> Like you said, I had a conflict with this patch. Can you double check my
> resolution, please:
>
> https://github.com/kvalo/ath/commit/0953accf20c325adde537a57c3e6a6a0f1ff04d7

Looks good.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH v2 2/2] ath10k: refactor radar detection code

[Michal Kazior]

If 20MHz CAC completed successfully then
subsequent CAC with wider bandwidth (40Mhz, 80Mhz)
with identical control frequency did not start
monitor vdev making it impossible to detect any
radar pulses during intended CAC.

It also was incorrect to assume ath10k_config() will
be called after CAC is finished. Theoretically for
non-HT channels nothing changes between CAC and
start_ap() (albeit in practice this can be
different). The incorrect assumption led to CAC
not being stopped on non-HT chandefs leading to
all Rx being drooped making it impossible for
clients to associate.

While at it clean up the code a bit.

Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
---
v2:
 * change locking comment style [Kalle]
 * get rid of forward declaration [Kalle]
 * fix vdev_stop / WARN_ON (missing !) [Kalle]
 * change commit message
   (the original issue found was CAC not being
    stopped for non-HT channels but this was a
    side-effect of a mac80211 bug, although the
    use of hw->conf.radar_enabled in ath10k was
    also incorrect either way)

 drivers/net/wireless/ath/ath10k/core.h |  4 +++
 drivers/net/wireless/ath/ath10k/mac.c  | 54 +++++++++++-----------------------
 2 files changed, 21 insertions(+), 37 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index 8edd6da..5cb9903 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -435,6 +435,10 @@ struct ath10k {
 	unsigned long dev_flags;
 	u32 dfs_block_radar_events;
 
+	/* protected by conf_mutex */
+	bool radar_enabled;
+	int num_started_vdevs;
+
 	struct wmi_pdev_set_wmm_params_arg wmm_params;
 	struct completion install_key_done;
 
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index e1f4227..3333b1f 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -719,49 +719,18 @@ static int ath10k_stop_cac(struct ath10k *ar)
 	return 0;
 }
 
-static const char *ath10k_dfs_state(enum nl80211_dfs_state dfs_state)
+static void ath10k_recalc_radar_detection(struct ath10k *ar)
 {
-	switch (dfs_state) {
-	case NL80211_DFS_USABLE:
-		return "USABLE";
-	case NL80211_DFS_UNAVAILABLE:
-		return "UNAVAILABLE";
-	case NL80211_DFS_AVAILABLE:
-		return "AVAILABLE";
-	default:
-		WARN_ON(1);
-		return "bug";
-	}
-}
-
-static void ath10k_config_radar_detection(struct ath10k *ar)
-{
-	struct ieee80211_channel *chan = ar->hw->conf.chandef.chan;
-	bool radar = ar->hw->conf.radar_enabled;
-	bool chan_radar = !!(chan->flags & IEEE80211_CHAN_RADAR);
-	enum nl80211_dfs_state dfs_state = chan->dfs_state;
 	int ret;
 
 	lockdep_assert_held(&ar->conf_mutex);
 
-	ath10k_dbg(ATH10K_DBG_MAC,
-		   "mac radar config update: chan %dMHz radar %d chan radar %d chan state %s\n",
-		   chan->center_freq, radar, chan_radar,
-		   ath10k_dfs_state(dfs_state));
-
-	/*
-	 * It's safe to call it even if CAC is not started.
-	 * This call here guarantees changing channel, etc. will stop CAC.
-	 */
 	ath10k_stop_cac(ar);
 
-	if (!radar)
-		return;
-
-	if (!chan_radar)
+	if (!ar->radar_enabled)
 		return;
 
-	if (dfs_state != NL80211_DFS_USABLE)
+	if (ar->num_started_vdevs > 0)
 		return;
 
 	ret = ath10k_start_cac(ar);
@@ -832,6 +801,9 @@ static int ath10k_vdev_start(struct ath10k_vif *arvif)
 		return ret;
 	}
 
+	ar->num_started_vdevs++;
+	ath10k_recalc_radar_detection(ar);
+
 	return ret;
 }
 
@@ -858,6 +830,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
 		return ret;
 	}
 
+	if (!WARN_ON(ar->num_started_vdevs == 0)) {
+		ar->num_started_vdevs--;
+		ath10k_recalc_radar_detection(ar);
+	}
+
 	return ret;
 }
 
@@ -2341,6 +2318,7 @@ static int ath10k_start(struct ieee80211_hw *hw)
 		goto exit;
 	}
 
+	ar->num_started_vdevs = 0;
 	ath10k_regd_update(ar);
 	ret = 0;
 
@@ -2490,15 +2468,17 @@ static int ath10k_config(struct ieee80211_hw *hw, u32 changed)
 
 	if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
 		ath10k_dbg(ATH10K_DBG_MAC,
-			   "mac config channel %d mhz flags 0x%x\n",
+			   "mac config channel %dMHz flags 0x%x radar %d\n",
 			   conf->chandef.chan->center_freq,
-			   conf->chandef.chan->flags);
+			   conf->chandef.chan->flags,
+			   conf->radar_enabled);
 
 		spin_lock_bh(&ar->data_lock);
 		ar->rx_channel = conf->chandef.chan;
 		spin_unlock_bh(&ar->data_lock);
 
-		ath10k_config_radar_detection(ar);
+		ar->radar_enabled = conf->radar_enabled;
+		ath10k_recalc_radar_detection(ar);
 
 		if (!cfg80211_chandef_identical(&ar->chandef, &conf->chandef)) {
 			ar->chandef = conf->chandef;
-- 
1.8.5.3


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 2/2] ath10k: refactor radar detection code

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> If 20MHz CAC completed successfully then
> subsequent CAC with wider bandwidth (40Mhz, 80Mhz)
> with identical control frequency did not start
> monitor vdev making it impossible to detect any
> radar pulses during intended CAC.
>
> It also was incorrect to assume ath10k_config() will
> be called after CAC is finished. Theoretically for
> non-HT channels nothing changes between CAC and
> start_ap() (albeit in practice this can be
> different). The incorrect assumption led to CAC
> not being stopped on non-HT chandefs leading to
> all Rx being drooped making it impossible for
> clients to associate.
>
> While at it clean up the code a bit.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>

[...]

> @@ -858,6 +830,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
>  		return ret;
>  	}
>  
> +	if (!WARN_ON(ar->num_started_vdevs == 0)) {

Even though this is correct, I think it's still a bit evil. For example,
I missed '!' the first time I looked at this. Even this is a line
longer, I think it would be easier to read:

WARN_ON(ar->num_started_vdevs == 0);

if (ar->num_started_vdevs != 0) {
        ...
}

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 2/2] ath10k: refactor radar detection code

[Michal Kazior]

On 7 April 2014 16:04, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior@tieto.com> writes:
>
>> If 20MHz CAC completed successfully then
>> subsequent CAC with wider bandwidth (40Mhz, 80Mhz)
>> with identical control frequency did not start
>> monitor vdev making it impossible to detect any
>> radar pulses during intended CAC.
>>
>> It also was incorrect to assume ath10k_config() will
>> be called after CAC is finished. Theoretically for
>> non-HT channels nothing changes between CAC and
>> start_ap() (albeit in practice this can be
>> different). The incorrect assumption led to CAC
>> not being stopped on non-HT chandefs leading to
>> all Rx being drooped making it impossible for
>> clients to associate.
>>
>> While at it clean up the code a bit.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
>
> [...]
>
>> @@ -858,6 +830,11 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
>>               return ret;
>>       }
>>
>> +     if (!WARN_ON(ar->num_started_vdevs == 0)) {
>
> Even though this is correct, I think it's still a bit evil. For example,
> I missed '!' the first time I looked at this. Even this is a line
> longer, I think it would be easier to read:
>
> WARN_ON(ar->num_started_vdevs == 0);
>
> if (ar->num_started_vdevs != 0) {
>         ...
> }

Sound reasonably.


Michał

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 2/2] ath10k: refactor radar detection code

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

>> Even though this is correct, I think it's still a bit evil. For example,
>> I missed '!' the first time I looked at this. Even this is a line
>> longer, I think it would be easier to read:
>>
>> WARN_ON(ar->num_started_vdevs == 0);
>>
>> if (ar->num_started_vdevs != 0) {
>>         ...
>> }
>
> Sound reasonably.

I changed it like this now:

--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -878,7 +878,9 @@ static int ath10k_vdev_stop(struct ath10k_vif *arvif)
                return ret;
        }
 
-       if (!WARN_ON(ar->num_started_vdevs == 0)) {
+       WARN_ON(ar->num_started_vdevs == 0);
+
+       if (ar->num_started_vdevs != 0) {
                ar->num_started_vdevs--;
                ath10k_recalc_radar_detection(ar);

Full patch here:

https://github.com/kvalo/ath/commit/0d57ff8b99539895c5a3714d7ecd80a58b6a8928

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH v2 0/2] ath10k: fix radar/cac

[Kalle Valo]

Michal Kazior <michal.kazior@tieto.com> writes:

> Hi,
>
> This loosely depends on `ath10k: refactor monitor
> code`. Without it patch this patchset introduces
> some (harmless) runtime warnings.
>
> This v2 introduces a non-functional patch to avoid
> forward declaration. The [1/2] has a little
> conflict with `ath10k: refactor monitor code`
> which renames ath10k_monitor_stop() to
> ath10k_monitor_vdev_stop().
>
> Part of the original CAC problem is also addressed
> in `mac80211: fix radar_enabled propagation`.
>
>
> Michal Kazior (2):
>   ath10k: reorder functions
>   ath10k: refactor radar detection code

Thanks, both patches applied.

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k