[PATCH 0/2] ath/ath10k: DFS Host Confirmation

[PATCH 0/2] ath/ath10k: DFS Host Confirmation

[Sriram R]

This feature adds support for the ath10k host to send information
about the specifications of the detected radar to the firmware for validation.
A New WMI interface is added for enabling this communication
of this information between the host and the firmware.

Sriram R (2):
  ath: Add support to get the detected radar specifications
  ath10k: DFS Host Confirmation

 drivers/net/wireless/ath/ath10k/core.h          |  21 +++
 drivers/net/wireless/ath/ath10k/mac.c           |  12 ++
 drivers/net/wireless/ath/ath10k/wmi-ops.h       |  32 ++++
 drivers/net/wireless/ath/ath10k/wmi.c           | 185 ++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/wmi.h           |  32 ++++
 drivers/net/wireless/ath/ath9k/dfs.c            |   2 +-
 drivers/net/wireless/ath/dfs_pattern_detector.c |   5 +-
 drivers/net/wireless/ath/dfs_pattern_detector.h |   3 +-
 drivers/net/wireless/ath/dfs_pri_detector.h     |   3 +-
 9 files changed, 281 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH 1/2] ath: Add support to get the detected radar specifications

[Sriram R]

This enables ath10k/ath9k drivers to collect the specifications of the
radar type once it is detected by the dfs pattern detector unit.
Usage of the collected info is specific to driver implementation.
For example, collected radar info could be used by the host driver
to send to co-processors for additional processing/validation.

Note: 'radar_detector_specs' data containing the specifications of
different radar types which was private within dfs_pattern_detector/
dfs_pri_detector is now shared with drivers as well for making use
of this information.

Signed-off-by: Sriram R <srirrama@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/wmi.c           | 2 +-
 drivers/net/wireless/ath/ath9k/dfs.c            | 2 +-
 drivers/net/wireless/ath/dfs_pattern_detector.c | 5 ++++-
 drivers/net/wireless/ath/dfs_pattern_detector.h | 3 ++-
 drivers/net/wireless/ath/dfs_pri_detector.h     | 3 ++-
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index bb8e9e2..f79ef1a 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -3765,7 +3765,7 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
 
 	ATH10K_DFS_STAT_INC(ar, pulses_detected);
 
-	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe)) {
+	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL)) {
 		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
 			   "dfs no pulse pattern detected, yet\n");
 		return;
diff --git a/drivers/net/wireless/ath/ath9k/dfs.c b/drivers/net/wireless/ath/ath9k/dfs.c
index c8844f5..acb9602 100644
--- a/drivers/net/wireless/ath/ath9k/dfs.c
+++ b/drivers/net/wireless/ath/ath9k/dfs.c
@@ -277,7 +277,7 @@ ath9k_dfs_process_radar_pulse(struct ath_softc *sc, struct pulse_event *pe)
 	DFS_STAT_INC(sc, pulses_processed);
 	if (pd == NULL)
 		return;
-	if (!pd->add_pulse(pd, pe))
+	if (!pd->add_pulse(pd, pe, NULL))
 		return;
 	DFS_STAT_INC(sc, radar_detected);
 	ieee80211_radar_detected(sc->hw);
diff --git a/drivers/net/wireless/ath/dfs_pattern_detector.c b/drivers/net/wireless/ath/dfs_pattern_detector.c
index 448b83e..d52b31b 100644
--- a/drivers/net/wireless/ath/dfs_pattern_detector.c
+++ b/drivers/net/wireless/ath/dfs_pattern_detector.c
@@ -268,7 +268,8 @@ static void dpd_exit(struct dfs_pattern_detector *dpd)
 }
 
 static bool
-dpd_add_pulse(struct dfs_pattern_detector *dpd, struct pulse_event *event)
+dpd_add_pulse(struct dfs_pattern_detector *dpd, struct pulse_event *event,
+	      struct radar_detector_specs *rs)
 {
 	u32 i;
 	struct channel_detector *cd;
@@ -294,6 +295,8 @@ dpd_add_pulse(struct dfs_pattern_detector *dpd, struct pulse_event *event)
 		struct pri_detector *pd = cd->detectors[i];
 		struct pri_sequence *ps = pd->add_pulse(pd, event);
 		if (ps != NULL) {
+			if (rs != NULL)
+				memcpy(rs, pd->rs, sizeof(*rs));
 			ath_dbg(dpd->common, DFS,
 				"DFS: radar found on freq=%d: id=%d, pri=%d, "
 				"count=%d, count_false=%d\n",
diff --git a/drivers/net/wireless/ath/dfs_pattern_detector.h b/drivers/net/wireless/ath/dfs_pattern_detector.h
index 92be353..18db6f4 100644
--- a/drivers/net/wireless/ath/dfs_pattern_detector.h
+++ b/drivers/net/wireless/ath/dfs_pattern_detector.h
@@ -97,7 +97,8 @@ struct dfs_pattern_detector {
 	bool (*set_dfs_domain)(struct dfs_pattern_detector *dpd,
 			   enum nl80211_dfs_regions region);
 	bool (*add_pulse)(struct dfs_pattern_detector *dpd,
-			  struct pulse_event *pe);
+			  struct pulse_event *pe,
+			  struct radar_detector_specs *rs);
 
 	struct ath_dfs_pool_stats (*get_stats)(struct dfs_pattern_detector *dpd);
 	enum nl80211_dfs_regions region;
diff --git a/drivers/net/wireless/ath/dfs_pri_detector.h b/drivers/net/wireless/ath/dfs_pri_detector.h
index 79f0fff..86339f2 100644
--- a/drivers/net/wireless/ath/dfs_pri_detector.h
+++ b/drivers/net/wireless/ath/dfs_pri_detector.h
@@ -62,8 +62,9 @@ struct pri_detector {
 	     (*add_pulse)(struct pri_detector *de, struct pulse_event *e);
 	void (*reset)    (struct pri_detector *de, u64 ts);
 
-/* private: internal use only */
 	const struct radar_detector_specs *rs;
+
+/* private: internal use only */
 	u64 last_ts;
 	struct list_head sequences;
 	struct list_head pulses;
-- 
2.7.4

Re: [PATCH 1/2] ath: Add support to get the detected radar specifications

[Peter Oh]

On 04/30/2018 10:45 AM, Sriram R wrote:
> This enables ath10k/ath9k drivers to collect the specifications of the
> radar type once it is detected by the dfs pattern detector unit.
> Usage of the collected info is specific to driver implementation.
> For example, collected radar info could be used by the host driver
> to send to co-processors for additional processing/validation.
What kind of additional processing/validation does it do?
Please give us more details for that.

Thanks,
Peter

Re: [PATCH 1/2] ath: Add support to get the detected radar specifications

[Kalle Valo]

Peter Oh <peter.oh@bowerswilkins.com> writes:

> On 04/30/2018 10:45 AM, Sriram R wrote:
>> This enables ath10k/ath9k drivers to collect the specifications of the
>> radar type once it is detected by the dfs pattern detector unit.
>> Usage of the collected info is specific to driver implementation.
>> For example, collected radar info could be used by the host driver
>> to send to co-processors for additional processing/validation.
>
> What kind of additional processing/validation does it do?
> Please give us more details for that.

This is handled in patch 2.

-- 
Kalle Valo

[PATCH 2/2] ath10k: DFS Host Confirmation

[Sriram R]

In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.

This new features enables the ath10k host to send information to the
firmware on the specifications of detected radar type. This allows the
firmware to validate if the host's radar pattern detector unit is
operational and check if the radar information shared by host matches
the radar pulses sent as phy error events from firmware. If the check
fails the firmware won't allow use of DFS channels on AP mode when using
FCC regulatory region.

Supported Chipsets : QCA9984/QCA9888/QCA4019
Firmware Version : 10.4-3.6-00104

Signed-off-by: Sriram R <srirrama@codeaurora.org>
---
 drivers/net/wireless/ath/ath10k/core.h    |  21 ++++
 drivers/net/wireless/ath/ath10k/mac.c     |  12 ++
 drivers/net/wireless/ath/ath10k/wmi-ops.h |  32 ++++++
 drivers/net/wireless/ath/ath10k/wmi.c     | 185 ++++++++++++++++++++++++++++--
 drivers/net/wireless/ath/ath10k/wmi.h     |  32 ++++++
 5 files changed, 272 insertions(+), 10 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index e4ac8f2..c95774e 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -175,6 +175,7 @@ struct ath10k_wmi {
 	struct completion service_ready;
 	struct completion unified_ready;
 	struct completion barrier;
+	struct completion radar_confirm;
 	wait_queue_head_t tx_credits_wq;
 	DECLARE_BITMAP(svc_map, WMI_SERVICE_MAX);
 	struct wmi_cmd_map *cmd;
@@ -377,6 +378,21 @@ struct ath10k_dfs_stats {
 	u32 radar_detected;
 };
 
+enum ath10k_radar_confirmation_state {
+	ATH10K_RADAR_CONFIRMATION_IDLE = 0,
+	ATH10K_RADAR_CONFIRMATION_INPROGRESS,
+	ATH10K_RADAR_CONFIRMATION_STOPPED,
+};
+
+struct ath10k_radar_found_info {
+	u32 pri_min;
+	u32 pri_max;
+	u32 width_min;
+	u32 width_max;
+	u32 sidx_min;
+	u32 sidx_max;
+};
+
 #define ATH10K_MAX_NUM_PEER_IDS (1 << 11) /* htt rx_desc limit */
 
 struct ath10k_peer {
@@ -1109,6 +1125,11 @@ struct ath10k {
 
 	u32 sta_tid_stats_mask;
 
+	/* protected by data_lock */
+	enum ath10k_radar_confirmation_state radar_conf_state;
+	struct ath10k_radar_found_info last_radar_info;
+	struct work_struct radar_confirmation_work;
+
 	/* must be last */
 	u8 drv_priv[0] __aligned(sizeof(void *));
 };
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 3d7119a..7463432 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -3217,6 +3217,15 @@ static void ath10k_reg_notifier(struct wiphy *wiphy,
 					       ar->hw->wiphy->bands[NL80211_BAND_5GHZ]);
 }
 
+static void ath10k_stop_radar_confirmation(struct ath10k *ar)
+{
+	spin_lock_bh(&ar->data_lock);
+	ar->radar_conf_state = ATH10K_RADAR_CONFIRMATION_STOPPED;
+	spin_unlock_bh(&ar->data_lock);
+
+	cancel_work_sync(&ar->radar_confirmation_work);
+}
+
 /***************/
 /* TX handlers */
 /***************/
@@ -4333,6 +4342,7 @@ void ath10k_halt(struct ath10k *ar)
 
 	ath10k_scan_finish(ar);
 	ath10k_peer_cleanup_all(ar);
+	ath10k_stop_radar_confirmation(ar);
 	ath10k_core_stop(ar);
 	ath10k_hif_power_down(ar);
 
@@ -4758,6 +4768,8 @@ static int ath10k_start(struct ieee80211_hw *hw)
 	ath10k_spectral_start(ar);
 	ath10k_thermal_set_throttling(ar);
 
+	ar->radar_conf_state = ATH10K_RADAR_CONFIRMATION_IDLE;
+
 	mutex_unlock(&ar->conf_mutex);
 	return 0;
 
diff --git a/drivers/net/wireless/ath/ath10k/wmi-ops.h b/drivers/net/wireless/ath/ath10k/wmi-ops.h
index e37d16b..5ecce04 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-ops.h
+++ b/drivers/net/wireless/ath/ath10k/wmi-ops.h
@@ -55,6 +55,8 @@ struct wmi_ops {
 			      struct wmi_wow_ev_arg *arg);
 	int (*pull_echo_ev)(struct ath10k *ar, struct sk_buff *skb,
 			    struct wmi_echo_ev_arg *arg);
+	int (*pull_dfs_status_ev)(struct ath10k *ar, struct sk_buff *skb,
+				  struct wmi_dfs_status_ev_arg *arg);
 	int (*pull_svc_avail)(struct ath10k *ar, struct sk_buff *skb,
 			      struct wmi_svc_avail_ev_arg *arg);
 
@@ -188,6 +190,9 @@ struct wmi_ops {
 						const struct wmi_tdls_peer_update_cmd_arg *arg,
 						const struct wmi_tdls_peer_capab_arg *cap,
 						const struct wmi_channel_arg *chan);
+	struct sk_buff *(*gen_radar_found)
+			(struct ath10k *ar,
+			 const struct ath10k_radar_found_info *arg);
 	struct sk_buff *(*gen_adaptive_qcs)(struct ath10k *ar, bool enable);
 	struct sk_buff *(*gen_pdev_get_tpc_config)(struct ath10k *ar,
 						   u32 param);
@@ -395,6 +400,16 @@ ath10k_wmi_pull_echo_ev(struct ath10k *ar, struct sk_buff *skb,
 	return ar->wmi.ops->pull_echo_ev(ar, skb, arg);
 }
 
+static inline int
+ath10k_wmi_pull_dfs_status(struct ath10k *ar, struct sk_buff *skb,
+			   struct wmi_dfs_status_ev_arg *arg)
+{
+	if (!ar->wmi.ops->pull_dfs_status_ev)
+		return -EOPNOTSUPP;
+
+	return ar->wmi.ops->pull_dfs_status_ev(ar, skb, arg);
+}
+
 static inline enum wmi_txbf_conf
 ath10k_wmi_get_txbf_conf_scheme(struct ath10k *ar)
 {
@@ -1511,4 +1526,21 @@ ath10k_wmi_pdev_get_tpc_table_cmdid(struct ath10k *ar, u32 param)
 				   ar->wmi.cmd->pdev_get_tpc_table_cmdid);
 }
 
+static inline int
+ath10k_wmi_report_radar_found(struct ath10k *ar,
+			      const struct ath10k_radar_found_info *arg)
+{
+	struct sk_buff *skb;
+
+	if (!ar->wmi.ops->gen_radar_found)
+		return -EOPNOTSUPP;
+
+	skb = ar->wmi.ops->gen_radar_found(ar, arg);
+	if (IS_ERR(skb))
+		return PTR_ERR(skb);
+
+	return ath10k_wmi_cmd_send(ar, skb,
+				   ar->wmi.cmd->radar_found_cmdid);
+}
+
 #endif
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index f79ef1a..5ac5c4c 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -34,6 +34,7 @@
 
 #define ATH10K_WMI_BARRIER_ECHO_ID 0xBA991E9
 #define ATH10K_WMI_BARRIER_TIMEOUT_HZ (3 * HZ)
+#define ATH10K_WMI_DFS_CONF_TIMEOUT_HZ (HZ / 2)
 
 /* MAIN WMI cmd track */
 static struct wmi_cmd_map wmi_cmd_map = {
@@ -199,6 +200,7 @@ static struct wmi_cmd_map wmi_cmd_map = {
 	.set_cca_params_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_bss_chan_info_request_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_get_tpc_table_cmdid = WMI_CMD_UNSUPPORTED,
+	.radar_found_cmdid = WMI_CMD_UNSUPPORTED,
 };
 
 /* 10.X WMI cmd track */
@@ -367,6 +369,7 @@ static struct wmi_cmd_map wmi_10x_cmd_map = {
 	.set_cca_params_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_bss_chan_info_request_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_get_tpc_table_cmdid = WMI_CMD_UNSUPPORTED,
+	.radar_found_cmdid = WMI_CMD_UNSUPPORTED,
 };
 
 /* 10.2.4 WMI cmd track */
@@ -535,6 +538,7 @@ static struct wmi_cmd_map wmi_10_2_4_cmd_map = {
 	.pdev_bss_chan_info_request_cmdid =
 		WMI_10_2_PDEV_BSS_CHAN_INFO_REQUEST_CMDID,
 	.pdev_get_tpc_table_cmdid = WMI_CMD_UNSUPPORTED,
+	.radar_found_cmdid = WMI_CMD_UNSUPPORTED,
 };
 
 /* 10.4 WMI cmd track */
@@ -745,6 +749,7 @@ static struct wmi_cmd_map wmi_10_4_cmd_map = {
 	.tdls_set_state_cmdid = WMI_10_4_TDLS_SET_STATE_CMDID,
 	.tdls_peer_update_cmdid = WMI_10_4_TDLS_PEER_UPDATE_CMDID,
 	.tdls_set_offchan_mode_cmdid = WMI_10_4_TDLS_SET_OFFCHAN_MODE_CMDID,
+	.radar_found_cmdid = WMI_10_4_RADAR_FOUND_CMDID,
 };
 
 /* MAIN WMI VDEV param map */
@@ -1490,6 +1495,7 @@ static struct wmi_cmd_map wmi_10_2_cmd_map = {
 	.pdev_get_ani_ofdm_config_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_reserve_ast_entry_cmdid = WMI_CMD_UNSUPPORTED,
 	.pdev_get_tpc_table_cmdid = WMI_CMD_UNSUPPORTED,
+	.radar_found_cmdid = WMI_CMD_UNSUPPORTED,
 };
 
 static struct wmi_pdev_param_map wmi_10_4_pdev_param_map = {
@@ -3691,6 +3697,67 @@ void ath10k_wmi_event_tbttoffset_update(struct ath10k *ar, struct sk_buff *skb)
 	ath10k_dbg(ar, ATH10K_DBG_WMI, "WMI_TBTTOFFSET_UPDATE_EVENTID\n");
 }
 
+static void ath10k_radar_detected(struct ath10k *ar)
+{
+	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
+	ATH10K_DFS_STAT_INC(ar, radar_detected);
+
+	/* Control radar events reporting in debugfs file
+	 * dfs_block_radar_events
+	 */
+	if (ar->dfs_block_radar_events)
+		ath10k_info(ar, "DFS Radar detected, but ignored as requested\n");
+	else
+		ieee80211_radar_detected(ar->hw);
+}
+
+static void ath10k_radar_confirmation_work(struct work_struct *work)
+{
+	struct ath10k *ar = container_of(work, struct ath10k,
+					 radar_confirmation_work);
+	struct ath10k_radar_found_info radar_info;
+	int ret, time_left;
+
+	reinit_completion(&ar->wmi.radar_confirm);
+
+	spin_lock_bh(&ar->data_lock);
+	memcpy(&radar_info, &ar->last_radar_info, sizeof(radar_info));
+	spin_unlock_bh(&ar->data_lock);
+
+	ret = ath10k_wmi_report_radar_found(ar, &radar_info);
+	if (ret) {
+		ath10k_warn(ar, "failed to send radar found %d\n", ret);
+		goto wait_complete;
+	}
+
+	time_left = wait_for_completion_timeout(&ar->wmi.radar_confirm,
+						ATH10K_WMI_DFS_CONF_TIMEOUT_HZ);
+	if (time_left) {
+		/* DFS Confirmation status event received and
+		 * necessary action completed.
+		 */
+		goto wait_complete;
+	} else {
+		/* DFS Confirmation event not received from FW.Considering this
+		 * as real radar.
+		 */
+		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
+			   "dfs confirmation not received from fw, considering as radar\n");
+		goto radar_detected;
+	}
+
+radar_detected:
+	ath10k_radar_detected(ar);
+
+	/* Reset state to allow sending confirmation on consecutive radar
+	 * detections, unless radar confirmation is disabled/stopped.
+	 */
+wait_complete:
+	spin_lock_bh(&ar->data_lock);
+	if (ar->radar_conf_state != ATH10K_RADAR_CONFIRMATION_STOPPED)
+		ar->radar_conf_state = ATH10K_RADAR_CONFIRMATION_IDLE;
+	spin_unlock_bh(&ar->data_lock);
+}
 static void ath10k_dfs_radar_report(struct ath10k *ar,
 				    struct wmi_phyerr_ev_arg *phyerr,
 				    const struct phyerr_radar_report *rr,
@@ -3699,8 +3766,10 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
 	u32 reg0, reg1, tsf32l;
 	struct ieee80211_channel *ch;
 	struct pulse_event pe;
+	struct radar_detector_specs rs;
 	u64 tsf64;
 	u8 rssi, width;
+	struct ath10k_radar_found_info *radar_info;
 
 	reg0 = __le32_to_cpu(rr->reg0);
 	reg1 = __le32_to_cpu(rr->reg1);
@@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
 
 	ATH10K_DFS_STAT_INC(ar, pulses_detected);
 
-	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL)) {
+	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
 		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
 			   "dfs no pulse pattern detected, yet\n");
 		return;
 	}
 
-radar_detected:
-	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
-	ATH10K_DFS_STAT_INC(ar, radar_detected);
+	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
+	    ar->dfs_detector->region == NL80211_DFS_FCC) {
+		/* Consecutive radar indications need not be
+		 * sent to the firmware until we get confirmation
+		 * for the previous detected radar.
+		 */
+		spin_lock_bh(&ar->data_lock);
+		if (ar->radar_conf_state != ATH10K_RADAR_CONFIRMATION_IDLE) {
+			spin_unlock_bh(&ar->data_lock);
+			return;
+		}
+		ar->radar_conf_state = ATH10K_RADAR_CONFIRMATION_INPROGRESS;
+		radar_info = &ar->last_radar_info;
 
-	/* Control radar events reporting in debugfs file
-	 * dfs_block_radar_events
-	 */
-	if (ar->dfs_block_radar_events) {
-		ath10k_info(ar, "DFS Radar detected, but ignored as requested\n");
+		radar_info->pri_min = rs.pri_min;
+		radar_info->pri_max = rs.pri_max;
+		radar_info->width_min = rs.width_min;
+		radar_info->width_max = rs.width_max;
+		/*TODO Find sidx_min and sidx_max */
+		radar_info->sidx_min = MS(reg0, RADAR_REPORT_REG0_PULSE_SIDX);
+		radar_info->sidx_max = MS(reg0, RADAR_REPORT_REG0_PULSE_SIDX);
+
+		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
+			   "sending wmi radar found cmd pri_min %d pri_max %d width_min %d width_max %d sidx_min %d sidx_max %d\n",
+			   radar_info->pri_min, radar_info->pri_max,
+			   radar_info->width_min, radar_info->width_max,
+			   radar_info->sidx_min, radar_info->sidx_max);
+		ieee80211_queue_work(ar->hw, &ar->radar_confirmation_work);
+		spin_unlock_bh(&ar->data_lock);
 		return;
 	}
 
-	ieee80211_radar_detected(ar->hw);
+radar_detected:
+	ath10k_radar_detected(ar);
 }
 
 static int ath10k_dfs_fft_report(struct ath10k *ar,
@@ -4133,6 +4223,47 @@ void ath10k_wmi_event_phyerr(struct ath10k *ar, struct sk_buff *skb)
 	}
 }
 
+static int
+ath10k_wmi_10_4_op_pull_dfs_status_ev(struct ath10k *ar, struct sk_buff *skb,
+				      struct wmi_dfs_status_ev_arg *arg)
+{
+	struct wmi_dfs_status_ev_arg *ev = (void *)skb->data;
+
+	if (skb->len < sizeof(*ev))
+		return -EPROTO;
+
+	arg->status = ev->status;
+
+	return 0;
+}
+
+static void
+ath10k_wmi_event_dfs_status_check(struct ath10k *ar, struct sk_buff *skb)
+{
+	struct wmi_dfs_status_ev_arg status_arg = {};
+	int ret;
+
+	ret = ath10k_wmi_pull_dfs_status(ar, skb, &status_arg);
+
+	if (ret) {
+		ath10k_warn(ar, "failed to parse dfs status event: %d\n", ret);
+		return;
+	}
+
+	ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
+		   "dfs status event received from fw: %d\n",
+		   status_arg.status);
+
+	/* Even in case of radar detection failure we follow the same
+	 * behaviour as if radar is detected i.e to switch to a different
+	 * channel.
+	 */
+	if (status_arg.status == WMI_HW_RADAR_DETECTED ||
+	    status_arg.status == WMI_RADAR_DETECTION_FAIL)
+		ath10k_radar_detected(ar);
+	complete(&ar->wmi.radar_confirm);
+}
+
 void ath10k_wmi_event_roam(struct ath10k *ar, struct sk_buff *skb)
 {
 	struct wmi_roam_ev_arg arg = {};
@@ -5884,6 +6015,9 @@ static void ath10k_wmi_10_4_op_rx(struct ath10k *ar, struct sk_buff *skb)
 	case WMI_10_4_PDEV_TPC_TABLE_EVENTID:
 		ath10k_wmi_event_tpc_final_table(ar, skb);
 		break;
+	case WMI_10_4_DFS_STATUS_CHECK_EVENTID:
+		ath10k_wmi_event_dfs_status_check(ar, skb);
+		break;
 	default:
 		ath10k_warn(ar, "Unknown eventid: %d\n", id);
 		break;
@@ -8470,6 +8604,32 @@ ath10k_wmi_10_4_gen_tdls_peer_update(struct ath10k *ar,
 }
 
 static struct sk_buff *
+ath10k_wmi_10_4_gen_radar_found(struct ath10k *ar,
+				const struct ath10k_radar_found_info *arg)
+{
+	struct wmi_radar_found_info *cmd;
+	struct sk_buff *skb;
+
+	skb = ath10k_wmi_alloc_skb(ar, sizeof(*cmd));
+	if (!skb)
+		return ERR_PTR(-ENOMEM);
+
+	cmd = (struct wmi_radar_found_info *)skb->data;
+	cmd->pri_min   = __cpu_to_le32(arg->pri_min);
+	cmd->pri_max   = __cpu_to_le32(arg->pri_max);
+	cmd->width_min = __cpu_to_le32(arg->width_min);
+	cmd->width_max = __cpu_to_le32(arg->width_max);
+	cmd->sidx_min  = __cpu_to_le32(arg->sidx_min);
+	cmd->sidx_max  = __cpu_to_le32(arg->sidx_max);
+
+	ath10k_dbg(ar, ATH10K_DBG_WMI,
+		   "wmi radar found pri_min %d pri_max %d width_min %d width_max %d sidx_min %d sidx_max %d\n",
+		   arg->pri_min, arg->pri_max, arg->width_min,
+		   arg->width_max, arg->sidx_min, arg->sidx_max);
+	return skb;
+}
+
+static struct sk_buff *
 ath10k_wmi_op_gen_echo(struct ath10k *ar, u32 value)
 {
 	struct wmi_echo_cmd *cmd;
@@ -8806,6 +8966,7 @@ static const struct wmi_ops wmi_10_4_ops = {
 	.pull_svc_rdy = ath10k_wmi_main_op_pull_svc_rdy_ev,
 	.pull_rdy = ath10k_wmi_op_pull_rdy_ev,
 	.pull_roam_ev = ath10k_wmi_op_pull_roam_ev,
+	.pull_dfs_status_ev = ath10k_wmi_10_4_op_pull_dfs_status_ev,
 	.get_txbf_conf_scheme = ath10k_wmi_10_4_txbf_conf_scheme,
 
 	.gen_pdev_suspend = ath10k_wmi_op_gen_pdev_suspend,
@@ -8852,6 +9013,7 @@ static const struct wmi_ops wmi_10_4_ops = {
 	.gen_tdls_peer_update = ath10k_wmi_10_4_gen_tdls_peer_update,
 	.gen_pdev_get_tpc_table_cmdid =
 			ath10k_wmi_10_4_op_gen_pdev_get_tpc_table_cmdid,
+	.gen_radar_found = ath10k_wmi_10_4_gen_radar_found,
 
 	/* shared with 10.2 */
 	.pull_echo_ev = ath10k_wmi_op_pull_echo_ev,
@@ -8914,8 +9076,11 @@ int ath10k_wmi_attach(struct ath10k *ar)
 	init_completion(&ar->wmi.service_ready);
 	init_completion(&ar->wmi.unified_ready);
 	init_completion(&ar->wmi.barrier);
+	init_completion(&ar->wmi.radar_confirm);
 
 	INIT_WORK(&ar->svc_rdy_work, ath10k_wmi_event_service_ready_work);
+	INIT_WORK(&ar->radar_confirmation_work,
+		  ath10k_radar_confirmation_work);
 
 	return 0;
 }
diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
index 700b12f..b48db54 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.h
+++ b/drivers/net/wireless/ath/ath10k/wmi.h
@@ -969,6 +969,7 @@ struct wmi_cmd_map {
 	u32 vdev_sifs_trigger_time_cmdid;
 	u32 pdev_wds_entry_list_cmdid;
 	u32 tdls_set_offchan_mode_cmdid;
+	u32 radar_found_cmdid;
 };
 
 /*
@@ -1803,6 +1804,11 @@ enum wmi_10_4_cmd_id {
 	WMI_10_4_TDLS_SET_STATE_CMDID,
 	WMI_10_4_TDLS_PEER_UPDATE_CMDID,
 	WMI_10_4_TDLS_SET_OFFCHAN_MODE_CMDID,
+	WMI_10_4_PDEV_SEND_FD_CMDID,
+	WMI_10_4_ENABLE_FILS_CMDID,
+	WMI_10_4_PDEV_SET_BRIDGE_MACADDR_CMDID,
+	WMI_10_4_ATF_GROUP_WMM_AC_CONFIG_REQUEST_CMDID,
+	WMI_10_4_RADAR_FOUND_CMDID,
 	WMI_10_4_PDEV_UTF_CMDID = WMI_10_4_END_CMDID - 1,
 };
 
@@ -1878,6 +1884,9 @@ enum wmi_10_4_event_id {
 	WMI_10_4_PDEV_TPC_TABLE_EVENTID,
 	WMI_10_4_PDEV_WDS_ENTRY_LIST_EVENTID,
 	WMI_10_4_TDLS_PEER_EVENTID,
+	WMI_10_4_HOST_SWFDA_EVENTID,
+	WMI_10_4_ESP_ESTIMATE_EVENTID,
+	WMI_10_4_DFS_STATUS_CHECK_EVENTID,
 	WMI_10_4_PDEV_UTF_EVENTID = WMI_10_4_END_EVENTID - 1,
 };
 
@@ -3398,6 +3407,25 @@ struct wmi_10_4_phyerr_event {
 	u8 buf[0];
 } __packed;
 
+struct wmi_radar_found_info {
+	__le32 pri_min;
+	__le32 pri_max;
+	__le32 width_min;
+	__le32 width_max;
+	__le32 sidx_min;
+	__le32 sidx_max;
+} __packed;
+
+enum wmi_radar_confirmation_status {
+	/* Detected radar was due to SW pulses */
+	WMI_SW_RADAR_DETECTED    = 0,
+
+	WMI_RADAR_DETECTION_FAIL = 1,
+
+	/* Real radar detected */
+	WMI_HW_RADAR_DETECTED    = 2,
+};
+
 #define PHYERR_TLV_SIG				0xBB
 #define PHYERR_TLV_TAG_SEARCH_FFT_REPORT	0xFB
 #define PHYERR_TLV_TAG_RADAR_PULSE_SUMMARY	0xF8
@@ -6631,6 +6659,10 @@ struct wmi_phyerr_hdr_arg {
 	const void *phyerrs;
 };
 
+struct wmi_dfs_status_ev_arg {
+	u32 status;
+};
+
 struct wmi_svc_rdy_ev_arg {
 	__le32 min_tx_power;
 	__le32 max_tx_power;
-- 
2.7.4

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Peter Oh]

On 04/30/2018 10:45 AM, Sriram R wrote:
> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>
> This new features enables the ath10k host to send information to the
> firmware on the specifications of detected radar type. This allows the
> firmware to validate if the host's radar pattern detector unit is
> operational and check if the radar information shared by host matches
> the radar pulses sent as phy error events from firmware. If the check
> fails the firmware won't allow use of DFS channels on AP mode when using
> FCC regulatory region.
What's the main reason you introduce this feature?
What are you trying to solve with this change?
> +#define ATH10K_WMI_DFS_CONF_TIMEOUT_HZ (HZ / 2)
>   
> +static void ath10k_radar_confirmation_work(struct work_struct *work)
> +{
> +	struct ath10k *ar = container_of(work, struct ath10k,
> +					 radar_confirmation_work);
> +	struct ath10k_radar_found_info radar_info;
> +	int ret, time_left;
> +
> +	reinit_completion(&ar->wmi.radar_confirm);
> +
> +	spin_lock_bh(&ar->data_lock);
> +	memcpy(&radar_info, &ar->last_radar_info, sizeof(radar_info));
> +	spin_unlock_bh(&ar->data_lock);
> +
> +	ret = ath10k_wmi_report_radar_found(ar, &radar_info);
> +	if (ret) {
> +		ath10k_warn(ar, "failed to send radar found %d\n", ret);
> +		goto wait_complete;
> +	}
> +
> +	time_left = wait_for_completion_timeout(&ar->wmi.radar_confirm,
> +						ATH10K_WMI_DFS_CONF_TIMEOUT_HZ);
It looks wrong to me in terms of timeout value.
Typical channel closing time in FCC domain is 200ms (excluding control 
signals), but you're waiting for 500ms for response from FW.
> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
>   
>   	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>   
> -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL)) {
> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>   		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>   			   "dfs no pulse pattern detected, yet\n");
>   		return;
>   	}
>   
> -radar_detected:
> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
I feel risky that host drivers have no way to control this new feature 
and totally rely on FW feature mask. We should have a host drivers' 
feature mask such as module param and set it false (don't use) by 
default until it proves safe to use.
> +static void
> +ath10k_wmi_event_dfs_status_check(struct ath10k *ar, struct sk_buff *skb)
> +{
> +	struct wmi_dfs_status_ev_arg status_arg = {};
> +	int ret;
> +
> +	ret = ath10k_wmi_pull_dfs_status(ar, skb, &status_arg);
> +
> +	if (ret) {
> +		ath10k_warn(ar, "failed to parse dfs status event: %d\n", ret);
> +		return;
> +	}
> +
> +	ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
> +		   "dfs status event received from fw: %d\n",
> +		   status_arg.status);
> +
> +	/* Even in case of radar detection failure we follow the same
> +	 * behaviour as if radar is detected i.e to switch to a different
> +	 * channel.
> +	 */
> +	if (status_arg.status == WMI_HW_RADAR_DETECTED ||
> +	    status_arg.status == WMI_RADAR_DETECTION_FAIL)
> +		ath10k_radar_detected(ar);
> +	complete(&ar->wmi.radar_confirm);
> +}
What is typical average duration from 
wait_for_completion_timeout(&ar->wmi.radar_confirm) to this completion 
called ?

Thanks,
Peter

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Peter Oh <peter.oh@bowerswilkins.com> writes:

> On 04/30/2018 10:45 AM, Sriram R wrote:
>> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
>> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>>
>> This new features enables the ath10k host to send information to the
>> firmware on the specifications of detected radar type. This allows the
>> firmware to validate if the host's radar pattern detector unit is
>> operational and check if the radar information shared by host matches
>> the radar pulses sent as phy error events from firmware. If the check
>> fails the firmware won't allow use of DFS channels on AP mode when using
>> FCC regulatory region.
>
> What's the main reason you introduce this feature?
> What are you trying to solve with this change?

Otherwise one cannot use DFS channels on FCC regions with a firmware
from 10.4-3.6 branch.

>> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
>>     	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>>   -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL))
>> {
>> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>>   		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>>   			   "dfs no pulse pattern detected, yet\n");
>>   		return;
>>   	}
>>   -radar_detected:
>> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
>> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
>> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
>
> I feel risky that host drivers have no way to control this new feature
> and totally rely on FW feature mask. We should have a host drivers'
> feature mask such as module param and set it false (don't use) by
> default until it proves safe to use.

This is for regulatory enforcement so it's not possible to disable the
feature from host.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Peter Oh]

On 05/02/2018 04:27 AM, Kalle Valo wrote:
> Peter Oh <peter.oh@bowerswilkins.com> writes:
>
>> On 04/30/2018 10:45 AM, Sriram R wrote:
>>> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
>>> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>>>
>>> This new features enables the ath10k host to send information to the
>>> firmware on the specifications of detected radar type. This allows the
>>> firmware to validate if the host's radar pattern detector unit is
>>> operational and check if the radar information shared by host matches
>>> the radar pulses sent as phy error events from firmware. If the check
>>> fails the firmware won't allow use of DFS channels on AP mode when using
>>> FCC regulatory region.
>> What's the main reason you introduce this feature?
>> What are you trying to solve with this change?
> Otherwise one cannot use DFS channels on FCC regions with a firmware
> from 10.4-3.6 branch.
>
It's a big change and none of us knows until I asked and you answered.
>>> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
>>>      	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>>>    -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL))
>>> {
>>> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>>>    		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>>>    			   "dfs no pulse pattern detected, yet\n");
>>>    		return;
>>>    	}
>>>    -radar_detected:
>>> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>>> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
>>> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
>>> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
>> I feel risky that host drivers have no way to control this new feature
>> and totally rely on FW feature mask. We should have a host drivers'
>> feature mask such as module param and set it false (don't use) by
>> default until it proves safe to use.
> This is for regulatory enforcement so it's not possible to disable the
> feature from host.
Which regulatory enforcement are you talking to?
Are you talking about such "prevent users from changing regulatory 
domain" or "don't provide a manner to change regulatory domain or 
channel list" ?
If not, can you share the section of document?

In addition to that, How do you make sure if FW side DFS radar detection 
algorithm has no defects and it always categorizes phy errors to correct 
radar type?
I'm asking it, because it's known that DFS detector in ath module does 
sometimes detect radar patterns as wrong radar type in both of ath10k 
and ath9k.
I couldn't think FW side detector has no such defects at all.
It's like blind patch to open source community, since no one can review 
the implementation in FW even though no one can guarantee its level of 
integrity.

Peter

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Peter Oh <peter.oh@bowerswilkins.com> writes:

> On 05/02/2018 04:27 AM, Kalle Valo wrote:
>> Peter Oh <peter.oh@bowerswilkins.com> writes:
>>
>>> On 04/30/2018 10:45 AM, Sriram R wrote:
>>>> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
>>>> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>>>>
>>>> This new features enables the ath10k host to send information to the
>>>> firmware on the specifications of detected radar type. This allows the
>>>> firmware to validate if the host's radar pattern detector unit is
>>>> operational and check if the radar information shared by host matches
>>>> the radar pulses sent as phy error events from firmware. If the check
>>>> fails the firmware won't allow use of DFS channels on AP mode when using
>>>> FCC regulatory region.
>>> What's the main reason you introduce this feature?
>>> What are you trying to solve with this change?
>> Otherwise one cannot use DFS channels on FCC regions with a firmware
>> from 10.4-3.6 branch.
>
> It's a big change and none of us knows until I asked and you answered.

I was hoping that would be clear from the commit log but I guess Sriram
could include my sentence above to enhance it?

>>>> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
>>>>      	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>>>>    -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL))
>>>> {
>>>> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>>>>    		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>>>>    			   "dfs no pulse pattern detected, yet\n");
>>>>    		return;
>>>>    	}
>>>>    -radar_detected:
>>>> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>>>> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
>>>> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
>>>> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
>>>
>>> I feel risky that host drivers have no way to control this new feature
>>> and totally rely on FW feature mask. We should have a host drivers'
>>> feature mask such as module param and set it false (don't use) by
>>> default until it proves safe to use.
>>
>> This is for regulatory enforcement so it's not possible to disable the
>> feature from host.
>
> Which regulatory enforcement are you talking to? Are you talking about
> such "prevent users from changing regulatory domain" or "don't provide
> a manner to change regulatory domain or channel list" ? If not, can
> you share the section of document?

Sorry, I don't have any references about that.

> In addition to that, How do you make sure if FW side DFS radar
> detection algorithm has no defects and it always categorizes phy
> errors to correct radar type? I'm asking it, because it's known that
> DFS detector in ath module does sometimes detect radar patterns as
> wrong radar type in both of ath10k and ath9k. I couldn't think FW side
> detector has no such defects at all. It's like blind patch to open
> source community, since no one can review the implementation in FW
> even though no one can guarantee its level of integrity.

It's not about replacing the DFS detector in ath.ko, it's about adding
extra checks in firmware for regulatory compliance. So at least in
theory DFS detection should still work same as before, just with an
extra roundtrip via firmware.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Peter Oh]

>> Which regulatory enforcement are you talking to? Are you talking about
>> such "prevent users from changing regulatory domain" or "don't provide
>> a manner to change regulatory domain or channel list" ? If not, can
>> you share the section of document?
> Sorry, I don't have any references about that.
>
This question is for "what the patch is trying to solve" and "to comply 
FCC regulatory rule" is not the right answer. It should be answered by 
"FCC regulatory rule section xx.xx.xx".
So we all can double check if this patch addresses "the root cause of 
the requirement" and "if it's the best approach or not"

Thanks,
Peter

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Sebastian Gottschall]

Am 02.05.2018 um 13:27 schrieb Kalle Valo:
> Peter Oh <peter.oh@bowerswilkins.com> writes:
>
>> On 04/30/2018 10:45 AM, Sriram R wrote:
>>> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
>>> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>>>
>>> This new features enables the ath10k host to send information to the
>>> firmware on the specifications of detected radar type. This allows the
>>> firmware to validate if the host's radar pattern detector unit is
>>> operational and check if the radar information shared by host matches
>>> the radar pulses sent as phy error events from firmware. If the check
>>> fails the firmware won't allow use of DFS channels on AP mode when using
>>> FCC regulatory region.
>> What's the main reason you introduce this feature?
>> What are you trying to solve with this change?
> Otherwise one cannot use DFS channels on FCC regions with a firmware
> from 10.4-3.6 branch.
>
>>> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct ath10k *ar,
>>>      	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>>>    -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL))
>>> {
>>> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>>>    		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>>>    			   "dfs no pulse pattern detected, yet\n");
>>>    		return;
>>>    	}
>>>    -radar_detected:
>>> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>>> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
>>> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) &&
>>> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
>> I feel risky that host drivers have no way to control this new feature
>> and totally rely on FW feature mask. We should have a host drivers'
>> feature mask such as module param and set it false (don't use) by
>> default until it proves safe to use.
> This is for regulatory enforcement so it's not possible to disable the
> feature from host.
mmh from what i see it is possible to disable it from host since the 
firmware seem to be sensitive on the regdomain set by the host
so if host sets anything else than a FCC regdomain, it will still work 
in the old way. so its more a pseudo enforcement. dont know if this is
usefull and it also will it make impossible to fix radar detection 
issues for certification as mentioned by peter and adrian.

Sebastian
>

-- 
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Sriram R]

On 2018-05-01 01:20, Peter Oh wrote:
> On 04/30/2018 10:45 AM, Sriram R wrote:
>> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
>> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT 
>> flag.
>> 
>> This new features enables the ath10k host to send information to the
>> firmware on the specifications of detected radar type. This allows the
>> firmware to validate if the host's radar pattern detector unit is
>> operational and check if the radar information shared by host matches
>> the radar pulses sent as phy error events from firmware. If the check
>> fails the firmware won't allow use of DFS channels on AP mode when 
>> using
>> FCC regulatory region.
> What's the main reason you introduce this feature?
> What are you trying to solve with this change?
>> +#define ATH10K_WMI_DFS_CONF_TIMEOUT_HZ (HZ / 2)
>>   +static void ath10k_radar_confirmation_work(struct work_struct 
>> *work)
>> +{
>> +	struct ath10k *ar = container_of(work, struct ath10k,
>> +					 radar_confirmation_work);
>> +	struct ath10k_radar_found_info radar_info;
>> +	int ret, time_left;
>> +
>> +	reinit_completion(&ar->wmi.radar_confirm);
>> +
>> +	spin_lock_bh(&ar->data_lock);
>> +	memcpy(&radar_info, &ar->last_radar_info, sizeof(radar_info));
>> +	spin_unlock_bh(&ar->data_lock);
>> +
>> +	ret = ath10k_wmi_report_radar_found(ar, &radar_info);
>> +	if (ret) {
>> +		ath10k_warn(ar, "failed to send radar found %d\n", ret);
>> +		goto wait_complete;
>> +	}
>> +
>> +	time_left = wait_for_completion_timeout(&ar->wmi.radar_confirm,
>> +						ATH10K_WMI_DFS_CONF_TIMEOUT_HZ);
> It looks wrong to me in terms of timeout value.
> Typical channel closing time in FCC domain is 200ms (excluding control
> signals), but you're waiting for 500ms for response from FW.
Right Peter, We haven't hit this limit during our testing. On an average 
we
received completion event within few milliseconds ,say less than 
10-15ms. I'll correct
this timeout in my next patch revision.
>> @@ -3765,25 +3834,46 @@ static void ath10k_dfs_radar_report(struct 
>> ath10k *ar,
>>     	ATH10K_DFS_STAT_INC(ar, pulses_detected);
>>   -	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, NULL)) {
>> +	if (!ar->dfs_detector->add_pulse(ar->dfs_detector, &pe, &rs)) {
>>   		ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>>   			   "dfs no pulse pattern detected, yet\n");
>>   		return;
>>   	}
>>   -radar_detected:
>> -	ath10k_dbg(ar, ATH10K_DBG_REGULATORY, "dfs radar detected\n");
>> -	ATH10K_DFS_STAT_INC(ar, radar_detected);
>> +	if ((test_bit(WMI_SERVICE_HOST_DFS_CHECK_SUPPORT, ar->wmi.svc_map)) 
>> &&
>> +	    ar->dfs_detector->region == NL80211_DFS_FCC) {
> I feel risky that host drivers have no way to control this new feature
> and totally rely on FW feature mask. We should have a host drivers'
> feature mask such as module param and set it false (don't use) by
> default until it proves safe to use.
>> +static void
>> +ath10k_wmi_event_dfs_status_check(struct ath10k *ar, struct sk_buff 
>> *skb)
>> +{
>> +	struct wmi_dfs_status_ev_arg status_arg = {};
>> +	int ret;
>> +
>> +	ret = ath10k_wmi_pull_dfs_status(ar, skb, &status_arg);
>> +
>> +	if (ret) {
>> +		ath10k_warn(ar, "failed to parse dfs status event: %d\n", ret);
>> +		return;
>> +	}
>> +
>> +	ath10k_dbg(ar, ATH10K_DBG_REGULATORY,
>> +		   "dfs status event received from fw: %d\n",
>> +		   status_arg.status);
>> +
>> +	/* Even in case of radar detection failure we follow the same
>> +	 * behaviour as if radar is detected i.e to switch to a different
>> +	 * channel.
>> +	 */
>> +	if (status_arg.status == WMI_HW_RADAR_DETECTED ||
>> +	    status_arg.status == WMI_RADAR_DETECTION_FAIL)
>> +		ath10k_radar_detected(ar);
>> +	complete(&ar->wmi.radar_confirm);
>> +}
> What is typical average duration from
> wait_for_completion_timeout(&ar->wmi.radar_confirm) to this completion
> called ?
We didn't see this taking more than 20ms to reach this completion.
As mentioned above I'll change the timeout value in the next patch.

Thanks,
Sriram.R
> 
> Thanks,
> Peter

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Adrian Chadd]

Hi,

May we have a little more information about how this is supposed to work?

It looks like we're supposed to send the information about the matched
radar pattern back to the firmware for confirmation? What's the intended
behaviour from the firmware? Will the firmware have a hard-coded set of
patterns we have to answer in/by?

I ask (like Peter, we work together) because we've had to tweak this
behaviour a little to actually pass FCC / ETSI DFS certification. My
general concern is that this'll cause a lot of false detects on boards that
haven't had things tweaked for the given board. As far as I'm aware the DFS
parameters are still hard-coded into the firmware image so if you have to
change those you're SOL without the relevant NDAs - this makes running the
open source DFS stuff a little tricksy on vendor boards.



-adrian

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Adrian Chadd <adrian@freebsd.org> writes:

> May we have a little more information about how this is supposed to work?
>
> It looks like we're supposed to send the information about the matched
> radar pattern back to the firmware for confirmation? What's the intended
> behaviour from the firmware? Will the firmware have a hard-coded set of
> patterns we have to answer in/by?

That's really an implementation detail inside the firmware and from
ath10k point of view we don't care what checks the firmware has, we just
provide all the necessary information. The checks in firmware might even
change in later releases.

> I ask (like Peter, we work together) because we've had to tweak this
> behaviour a little to actually pass FCC / ETSI DFS certification. My
> general concern is that this'll cause a lot of false detects on boards that
> haven't had things tweaked for the given board. As far as I'm aware the DFS
> parameters are still hard-coded into the firmware image so if you have to
> change those you're SOL without the relevant NDAs - this makes running the
> open source DFS stuff a little tricksy on vendor boards.

This shouldn't cause more false detections, the pattern detection from
ath.ko is still used as before. The firmware will just disable DFS
altogether if it thinks ath10k is not compliant.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Adrian Chadd]

On Mon, 14 May 2018 at 11:25, Kalle Valo <kvalo@codeaurora.org> wrote:

> Adrian Chadd <adrian@freebsd.org> writes:

> > May we have a little more information about how this is supposed to
work?
> >
> > It looks like we're supposed to send the information about the matched
> > radar pattern back to the firmware for confirmation? What's the intended
> > behaviour from the firmware? Will the firmware have a hard-coded set of
> > patterns we have to answer in/by?

> That's really an implementation detail inside the firmware and from
> ath10k point of view we don't care what checks the firmware has, we just
> provide all the necessary information. The checks in firmware might even
> change in later releases.

> > I ask (like Peter, we work together) because we've had to tweak this
> > behaviour a little to actually pass FCC / ETSI DFS certification. My
> > general concern is that this'll cause a lot of false detects on boards
that
> > haven't had things tweaked for the given board. As far as I'm aware the
DFS
> > parameters are still hard-coded into the firmware image so if you have
to
> > change those you're SOL without the relevant NDAs - this makes running
the
> > open source DFS stuff a little tricksy on vendor boards.

> This shouldn't cause more false detections, the pattern detection from
> ath.ko is still used as before. The firmware will just disable DFS
> altogether if it thinks ath10k is not compliant.


Heh, well the fun one for production for us is "ok, so what's
non-compliant" ?

Eg - if it's 1 out of 100 that we don't hit the explicit timing
requirements because of the rest of the linux kernel (eg someone holds a
spinlock more than they should) then I'd prefer that we got a notification
that something happened so we can fix it. Otherwise in the field it'll just
be "hey, our stuff stopped working" because whatever the firmware
expectations are aren't being met.

Again, we're OK because we can at least inspect what's going on, but not
everyone doing ath10k development/deployment will be so lucky :(




-adrian

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Adrian Chadd <adrian@freebsd.org> writes:

> On Mon, 14 May 2018 at 11:25, Kalle Valo <kvalo@codeaurora.org> wrote:
>
>> Adrian Chadd <adrian@freebsd.org> writes:
>
>> > May we have a little more information about how this is supposed to
> work?
>> >
>> > It looks like we're supposed to send the information about the matched
>> > radar pattern back to the firmware for confirmation? What's the intended
>> > behaviour from the firmware? Will the firmware have a hard-coded set of
>> > patterns we have to answer in/by?
>
>> That's really an implementation detail inside the firmware and from
>> ath10k point of view we don't care what checks the firmware has, we just
>> provide all the necessary information. The checks in firmware might even
>> change in later releases.
>
>> > I ask (like Peter, we work together) because we've had to tweak this
>> > behaviour a little to actually pass FCC / ETSI DFS certification. My
>> > general concern is that this'll cause a lot of false detects on boards
> that
>> > haven't had things tweaked for the given board. As far as I'm aware the
> DFS
>> > parameters are still hard-coded into the firmware image so if you have
> to
>> > change those you're SOL without the relevant NDAs - this makes running
> the
>> > open source DFS stuff a little tricksy on vendor boards.
>
>> This shouldn't cause more false detections, the pattern detection from
>> ath.ko is still used as before. The firmware will just disable DFS
>> altogether if it thinks ath10k is not compliant.
>
>
> Heh, well the fun one for production for us is "ok, so what's
> non-compliant" ?
>
> Eg - if it's 1 out of 100 that we don't hit the explicit timing
> requirements because of the rest of the linux kernel (eg someone holds a
> spinlock more than they should) then I'd prefer that we got a notification
> that something happened so we can fix it. Otherwise in the field it'll just
> be "hey, our stuff stopped working" because whatever the firmware
> expectations are aren't being met.
>
> Again, we're OK because we can at least inspect what's going on, but not
> everyone doing ath10k development/deployment will be so lucky :(

Sure, every software change can cause regressions. But the thing is that
this isn't an optional, ath10k has to have this to be able to continue
using DFS channels.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Peter Oh]

> Sure, every software change can cause regressions. But the thing is that
> this isn't an optional, ath10k has to have this to be able to continue
> using DFS channels.
>
Kalle, you said you don't know which exact FCC requirement this is for 
in another email thread.
Then how come you're sure this is not an optional and this approach is 
right one?

Peter

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Peter Oh <peter.oh@bowerswilkins.com> writes:

>> Sure, every software change can cause regressions. But the thing is that
>> this isn't an optional, ath10k has to have this to be able to continue
>> using DFS channels.
>
> Kalle, you said you don't know which exact FCC requirement this is for
> in another email thread. Then how come you're sure this is not an
> optional 

To me this is a mandatory ath10k feature because if the host driver does
not have this DFS channels won't work anymore.

> and this approach is right one?

That's something you need to ask the firmware team who designed and
implemented this.

This discussion is going circles, it would be a lot easier if you could
clearly say your point. But if your point is just "I don't like this"
then there's not much, if any, I can do about that.

-- 
Kalle Valo

Re: [PATCH 2/2] ath10k: DFS Host Confirmation

[Kalle Valo]

Sriram R <srirrama@codeaurora.org> writes:

> In the 10.4-3.6 firmware branch there's a new DFS Host confirmation
> feature which is advertised using WMI_SERVICE_HOST_DFS_CHECK_SUPPORT flag.
>
> This new features enables the ath10k host to send information to the
> firmware on the specifications of detected radar type. This allows the
> firmware to validate if the host's radar pattern detector unit is
> operational and check if the radar information shared by host matches
> the radar pulses sent as phy error events from firmware. If the check
> fails the firmware won't allow use of DFS channels on AP mode when using
> FCC regulatory region.
>
> Supported Chipsets : QCA9984/QCA9888/QCA4019
> Firmware Version : 10.4-3.6-00104
>
> Signed-off-by: Sriram R <srirrama@codeaurora.org>

This introduces a new checkpatch warning:

drivers/net/wireless/ath/ath10k/wmi.c:3753: Please use a blank line after function/struct/union/enum declarations

-- 
Kalle Valo